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

Make the recovery dialog non-cancelable #6620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Feb 20, 2025

Closes #6616

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

When we start opening a form with entities, we place a lock on the forms database to prevent downloading updates during form filling. This lock is removed when the form is closed. However, if a savepoint existed, we displayed a dialog after locking the database but before actually opening the form. As a result, if the user dismissed the dialog by clicking outside its area, the lock was never removed.

The simplest solution is to make the dialog non-cancelable, preventing this issue altogether. Another approach would be to remove the lock when the dialog is canceled, but is there any reason to keep it cancelable? I don't think so, which makes the first solution simpler.

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?

I think we can simply verify that the described issue no longer exists. Also, please check if making the recovery dialog non-cancelable causes any problems (I hope not but let's double-check). That should be sufficient.

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

Any form with entities.

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
  • added any new strings with date formatting to DateFormatsTest
  • 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 added the high priority Should be looked at before other PRs/issues label Feb 20, 2025
@grzesiek2010 grzesiek2010 requested a review from seadowg February 20, 2025 23:47
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.

Agreed that removing cancellation makes sense here. This needs a test though. If that's awkward with the current setup, the dialog code could just be pulled out to a helper (like QuitFormDialog) and unit tested. I just want to make sure the behaviour is locked in.

@grzesiek2010 grzesiek2010 requested a review from seadowg February 21, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening entity forms blocked after discarding savepoint by clicking outside the dialog
2 participants