Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete savepoints when deleting blank forms they belong to #6139

Merged
merged 3 commits into from
May 21, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented May 16, 2024

Closes #6131

Why is this the best possible solution? Were any other approaches considered?

Removing savepoints when forms they belong to are being deleted seems to be the best solution that not only fixes the issue but also protects us from having orphaned savepoints. Another solution could be for example comparing dates of creation to avoid using savepoints that were created earlier than the form itself but those savepoints would still be present in the database and in the cache dir so I think getting rid of them is the proper approach.

I've implemented this only for blank forms as this turned out to be an issue but we should do the same for saved forms too in order to avoid keeping orphaned savepoints. I will file a separate issue for that.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The main change is that during deleting blank forms their savepoints should also be deleted to avoid keeping orphaned savepoints. This will happen not only if a blank form is completely deleted but also in the case of soft deletion. Thanks to that issue #6131 will be fixed.
So far it didn't work like that so if a blank form was deleted but had a savepoint that savepoint remained in the database and in the cache dir (savepoints were only removed when igorered in the form view). Such a savepoint would be useless and would only clutter the cache dir. This is something we probably haven't tested that much so now it's a good time to do that.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-6131 branch 2 times, most recently from e9c426a to b0802d1 Compare May 16, 2024 21:29
@grzesiek2010 grzesiek2010 changed the title Collect 6131 Delete savepoints when deleting blank forms they belong to May 16, 2024
@grzesiek2010 grzesiek2010 requested a review from seadowg May 16, 2024 22:02
@grzesiek2010 grzesiek2010 marked this pull request as ready for review May 16, 2024 22:02
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small tweak here.

This is a really interesting change as it's the first time we've really had repos acknowledge each other (here FormsRepository knows about SavepointsRepository). I was initially unsure about how I felt about this, but it seems like it most accurately reflects how I think things should work: forms, instances and savepoints in a single DB allowing us to implement these kinds of features with CASCADE etc.

@grzesiek2010 grzesiek2010 requested a review from seadowg May 20, 2024 22:22
@seadowg seadowg merged commit 079500c into getodk:master May 21, 2024
6 checks passed
@WKobus
Copy link

WKobus commented May 23, 2024

Tested with Success!

Verified on device with Android: 14

Verified cases:

@dbemke
Copy link

dbemke commented May 23, 2024

Tested with Success!

Verified on device with Android: 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Savepoints in softdeleted forms that are redownloaded
4 participants