Skip to content

drop DB before importing#1389

Closed
obfusk wants to merge 3 commits into
CatimaLoyalty:mainfrom
obfusk:drop-db
Closed

drop DB before importing#1389
obfusk wants to merge 3 commits into
CatimaLoyalty:mainfrom
obfusk:drop-db

Conversation

@obfusk

@obfusk obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor

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

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Looks like the unit test is stuck again :/

@obfusk

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Will look into that. But not today.

@obfusk

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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

@obfusk

obfusk commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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

obfusk commented Jun 24, 2023

Copy link
Copy Markdown
Contributor Author

I added code to remove the old images.

@obfusk

obfusk commented Jun 24, 2023

Copy link
Copy Markdown
Contributor Author

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