Skip to content

Conversation

@obfusk
Copy link
Contributor

@obfusk obfusk commented Jun 23, 2023

Split off from #1343:

  • drop DB before importing
  • warn user about data being dropped
  • delete old images

@obfusk obfusk marked this pull request as ready for review June 23, 2023 22:04
@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

This drops the DB. But I forgot about the card images. We should probably also delete those.
But that's more complicated since we can't use the DB transaction for that and file names may be reused.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

Looks like the unit test is stuck again :/

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I forgot about the card images.

So as a result imported cards will sometimes show old images. Seems to work well otherwise in my tests.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

But that's more complicated since we can't use the DB transaction for that and file names may be reused.

So we'd probably have to make a list of existing files before the import. Then subtract the files after the import and delete the rest.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

Will look into that. But not today.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I don't think the images are ever deleted currently, even when the card is deleted.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I don't think the images are ever deleted currently, even when the card is deleted.

They are. I just looked in the wrong place.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 24, 2023

I added code to remove the old images.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 24, 2023

This won't delete any existing image files unless the import succeeds. But existing image files may be overwritten by a failed import.

To avoid that we'd have to e.g. save them to a temporary directory instead and move them after the import succeeds. I can make a follow-up PR for that if you'd like, but this seems like a worthwhile improvement already and I don't want to make it unnecessarily complex.

@obfusk obfusk marked this pull request as draft June 26, 2023 14:15
@obfusk obfusk mentioned this pull request Jun 26, 2023
@obfusk obfusk closed this Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant