Block deleting saved forms if some of them are being sent #5929

grzesiek2010 · 2024-01-19T20:19:43Z

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

As I said in the issue I've considered adding a new status for forms that are being sent but this would require more changes and would be riskier. ChangeLock is something we already use in other places of the app to block accessing databases.
The difference between using ChangeLock and introducing a new status is that with a new status, we would be able to block deleting particular forms but with ChangeLock we block deleting all existing saved forms but I think that's ok.

I think this pr is enough for now because it protects users from losing data but we should consider some UI improvements to somehow let users know that sending forms is taking place in the background and deleting them is not possible. Maybe we should gray out and disable the list of saved forms (that we can use to delete them), or maybe something else. @alyblenkin COuld you think it through and file a new issue?

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?

Deleting saved forms (all of them) should be blocked when at least one of them is being sent. That means we can't delete them from the Main menu -> Delete form or by resetting saved forms in protected settings).
It's possible that we have let's say 10 saved forms but only one of them will be sent automatically because only one has auto-send enabled on a form level. In such a case according to what I said above deleting those other 9 forms should also be blocked.

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

seadowg · 2024-01-26T13:35:32Z

@grzesiek2010 I think we should include this in v2024.1. It'd be good to prevent any weirdness that might arise from someone deleting a saved form while it's being sent. Go ahead and mark this as ready for review and request a review if you think this is good to go!

seadowg · 2024-03-25T14:43:58Z

..._app/src/main/java/org/odk/collect/android/formlists/savedformlist/SavedFormListViewModel.kt

-            },
-            foreground = {
-                result.value = Consumable(databaseIds.count())
+        changeLockProvider.getInstanceLock(projectId).withLock { acquiredLock: Boolean ->


I feel like this check would make more sense in InstancesDataService#deleteInstance (maybe in a modified method that takes a list of IDs rather just one). Then, the other places that delete instances can just call that and we'll only have to handle the check in one place.

seadowg

Just a few tweaks!

seadowg · 2024-03-26T10:28:55Z

collect_app/src/main/java/org/odk/collect/android/instancemanagement/InstancesDataService.kt


-        update()
+                    update()


Why do we need this update?

We don't, good catch.

seadowg · 2024-03-26T12:26:13Z

collect_app/src/main/java/org/odk/collect/android/utilities/ProjectResetter.kt

-        instancesRepositoryProvider.get().deleteAll()
+        changeLockProvider.getInstanceLock(projectId).withLock { acquiredLock: Boolean ->
+            if (acquiredLock) {
+                instancesRepositoryProvider.get().deleteAll()


Is there a reason not to use InstancesDataService here instead?

We can do that. Done.

seadowg · 2024-03-26T12:28:41Z

collect_app/src/main/java/org/odk/collect/android/tasks/InstanceUploaderTask.java

+                .filter(instance -> instance.getStatus().equals(Instance.STATUS_SUBMITTED))
+                .filter(instance -> InstanceAutoDeleteChecker.shouldInstanceBeDeleted(formsRepository, isFormAutoDeleteOptionEnabled, instance));
+
+        InstanceDeleter instanceDeleter = new InstanceDeleter(instancesRepository, formsRepository);


I'd prefer the InstancesDataService here, but I'm guessing we end up in a weird state where we've locked the ChangeLock so can't then call deleteInstances? If so, I guess down the line we can probably have a InstancesDataService#submitInstances that covers submission and the clean up. That will probably make sense once we rework the upload UI.

I'd prefer the InstancesDataService here, but I'm guessing we end up in a weird state where we've locked the ChangeLock so can't then call deleteInstances?

Correct.

If so, I guess down the line we can probably have a InstancesDataService#submitInstances that covers submission and the clean up. That will probably make sense once we rework the upload UI.

Yeah makes sens we also need to rework the whole task since it's still android AsyncTask. Let's do that in a separate pr.

seadowg · 2024-03-26T12:30:31Z

.../src/test/java/org/odk/collect/android/formlists/savedformlist/SavedFormListViewModelTest.kt

+
+        val result = viewModel.deleteForms(longArrayOf(1))
+        scheduler.flush()
+        assertThat(result.value!!.value, equalTo(0))


You can use getOrAwaitValue() here (and in the other test) instead of needing the !!.

Yes, I can but still will need !! as the value the livedata stores is Consumable and I need to unpack it see: 85a69ba

dbemke · 2024-03-29T14:04:40Z

@grzesiek2010 @seadowg we tried testing the PR but because of the issue #6017 we're having a hard time verifying what forms and how many forms are really deleted. We would like to wait for that issue to be fixed and then test it together, cause this issue makes a half of our tests unreliable. Would it be ok to wait with testing till the fix for you, too?

seadowg · 2024-03-29T17:36:24Z

@dbemke yeah that's fine!

dbemke · 2024-05-21T09:30:50Z

Tested with Success!

Verified on a device with Android 10

Verified Cases:

trying to delete a form/ forms while being sent
forms with big attachments, auto send sent to true and false
different auto send options
don't keep activities enabled/ disabled
deleting saved forms and blank forms
reported issue It's possible to delete a form just after another one was sent #6125

srujner · 2024-05-21T09:31:55Z

Tested with Success!

Verified on a device with Android 13

WKobus · 2024-05-21T09:32:33Z

Tested with Success!

Verified on a device with Android 14

grzesiek2010 force-pushed the deleteinstances2 branch from f471942 to 8d82162 Compare January 19, 2024 21:19

grzesiek2010 mentioned this pull request Jan 22, 2024

Deleting a form that is being sent #5800

Open

seadowg added this to the v2024.1 milestone Jan 26, 2024

seadowg modified the milestones: v2024.1, v2024.2 Feb 2, 2024

grzesiek2010 force-pushed the deleteinstances2 branch from 8d82162 to 3f5cfec Compare February 2, 2024 23:05

grzesiek2010 force-pushed the deleteinstances2 branch 2 times, most recently from 2ab6a59 to c277911 Compare March 22, 2024 12:11

grzesiek2010 requested a review from seadowg March 22, 2024 22:40

grzesiek2010 marked this pull request as ready for review March 22, 2024 22:40

seadowg requested changes Mar 25, 2024

View reviewed changes

grzesiek2010 force-pushed the deleteinstances2 branch from a21cf57 to 21d03e8 Compare March 26, 2024 00:01

grzesiek2010 requested a review from seadowg March 26, 2024 00:33

seadowg requested changes Mar 26, 2024

View reviewed changes

grzesiek2010 added 11 commits March 27, 2024 12:36

Do not reset instances if the instances db is locked

ff19f6a

Added a new test

ab00fdb

Do not delete instances if the instances db is locked

f1c38e9

Added tests

8a7c016

Removed redundant DeleteInstancesTask

c813d45

Merged InstanceUploaderTask with InstanceServerUploaderTask

6ec966b

Moved deleting sent forms to background

00801c7

Improve tests using getOrAwaitValue()

85a69ba

Removed redundant update() call

b8ff88d

Moved ProjectResetter to the projects package

9893557

Use InstancesDataService in ProjectResetter

35b597c

grzesiek2010 force-pushed the deleteinstances2 branch from 21d03e8 to 35b597c Compare March 27, 2024 12:28

grzesiek2010 requested a review from seadowg March 27, 2024 12:57

seadowg approved these changes Mar 29, 2024

View reviewed changes

seadowg added the needs testing label Mar 29, 2024

seadowg merged commit 031183e into getodk:master Mar 29, 2024
6 checks passed

WKobus added behavior verified and removed needs testing labels May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block deleting saved forms if some of them are being sent #5929

Block deleting saved forms if some of them are being sent #5929

grzesiek2010 commented Jan 19, 2024 •

edited

Loading

seadowg commented Jan 26, 2024

seadowg Mar 25, 2024

grzesiek2010 Mar 26, 2024

seadowg left a comment

seadowg Mar 26, 2024

grzesiek2010 Mar 27, 2024

seadowg Mar 26, 2024

grzesiek2010 Mar 27, 2024

seadowg Mar 26, 2024

grzesiek2010 Mar 27, 2024

seadowg Mar 26, 2024

grzesiek2010 Mar 27, 2024

dbemke commented Mar 29, 2024 •

edited

Loading

seadowg commented Mar 29, 2024

dbemke commented May 21, 2024

srujner commented May 21, 2024

WKobus commented May 21, 2024

Block deleting saved forms if some of them are being sent #5929

Block deleting saved forms if some of them are being sent #5929

Conversation

grzesiek2010 commented Jan 19, 2024 • edited Loading

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

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?

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

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

Before submitting this PR, please make sure you have:

seadowg commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemke commented Mar 29, 2024 • edited Loading

seadowg commented Mar 29, 2024

dbemke commented May 21, 2024

srujner commented May 21, 2024

WKobus commented May 21, 2024

grzesiek2010 commented Jan 19, 2024 •

edited

Loading

dbemke commented Mar 29, 2024 •

edited

Loading