-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow deleting sent forms with entities #6416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also makes a change for drafts which I don't think was part of the initial issue. I believe it's safe and makes sense but would be good to explicitly think through that.
@lognaturel |
testDependencies.server.addForm("one-question-entity-registration.xml") | ||
|
||
rule.withMatchExactlyProject(testDependencies.server.url) | ||
// Drafts can be deleted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continue to be quite confident that before the change in this PR, drafts could not be deleted. Probably QA didn't think about that case because the prior PRs were framed as not allowing delete before send. In fact, we want to block delete for finalized/failed submissions and allow delete in other cases.
I think a better name for the db column would have been canDeleteFinalized
. Maybe we should even have considered something like mutatesEntity
. Since these db changes haven't been published yet, should we maybe consider making the change to avoid mistakes in the future?
@@ -36,7 +36,7 @@ class DeleteSavedFormFragment( | |||
private val multiSelectViewModel: MultiSelectViewModel<Instance> by viewModels { | |||
MultiSelectViewModel.Factory( | |||
savedFormListViewModel.formsToDisplay.map { | |||
it.filter { instance -> instance.canDeleteBeforeSend() } | |||
it.filter { instance -> instance.canDelete() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter here previously didn't consider instance state at all so I don't think there's any way it could have been excluding drafts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear, I see. The value of canDeleteBeforeSend
is changed in the db on finalization: https://github.com/getodk/collect/pull/6311/files#diff-fc2a9d08e9f222f7c3f787edbcbb45943c10b17525178e18b4df0e18711f92a4R164
I'm finding that pretty confusing too -- there are now two code paths about excluding drafts from not being deleted. I guess part of the reasoning there is that we don't know for sure whether a filled form will mutate entities until it's finalized. Maybe it's ok to leave.
Tested with Success! Verified on a device with Android 10 Verified cases:
|
Tested with Success Verified on device with Android 14 |
Closes #6380
Why is this the best possible solution? Were any other approaches considered?
I considered changing the instances database and renaming
CAN_DELETE_BEFORE_SEND
to a more genericCAN_DELETE
, and updating the row after sending a form. However, this approach would be significantly more complex and riskier, so I decided to simply introduce a helper function#canDelete
instead: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?
This should only adjust how we determine which forms can be deleted. Please test this filtering thoroughly with every possible form status to ensure it works correctly.
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:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still passDateFormatsTest