Prevent opening forms that use entities during form updates, and block form updates while filling out a form that uses entities #6440

grzesiek2010 · 2024-10-03T19:50:59Z

Closes #6232
Closes #6452

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

This pull request addresses two scenarios:

Prevent forms that use entities from being opened during the form update process. It doesn't matter which forms are being updated (whether it's the form being opened or not, or whether the updated forms use entities at all).
Prevent form updates while filling out forms that use entities. If an entity-based form is being filled out, all form updates should be blocked, regardless of which forms are set to be updated. The entire update process should be blocked.

To handle both cases, I used the ChangeLock mechanism as recommended in the issue. The flow is as follows:

If a user attempts to open a form that uses entities, we check whether the forms database is locked and displays an error. This differs slightly from the original approach, which suggested using a progress dialog to wait for the update process to finish. I believe this simpler solution is sufficient for now and aligns with other errors that can occur before opening a form (which we handle in FormUriActivity). The error will look like this:

We might want to convert this to a progress dialog in the future but for now, it should be enough.
If the database isn't locked, we lock it and open the form.
Any form updates triggered during form filling are blocked since the forms database remains locked.
Once the form is closed, we unlock the database, allowing any pending form updates to proceed.

#6452 says only about blocking opening forms that use entities during form updates but I implemented it the other way around as well - if the form being filled doesn’t use entities, then form updates are not blocked. I think it makes sense despite the fact it wasn't explicitly described but let me know if I'm wrong and missed something @seadowg

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?

As mentioned above, I used the mechanism of locking the forms database to control whether a forms update can proceed or if a form can be opened. However, I find this approach somewhat risky. I’m concerned about scenarios where the database may not be properly unlocked after exiting a form (since it must remain locked during form filling to prevent updates). In such cases, if a user tries to open another form, it could fail because the database is still locked. While this shouldn't happen based on my understanding, unexpected issues can always occur. I can go over the details of this process on a call if you'd like.

If you find testing this pull request difficult, as it may not be clear when the forms update process starts or ends, let me know, and I can add some visual indicators specifically for testing.

This pull request updates the forms database, so it’s important to test that aspect as well. A scenario worth testing is:

Install the old version and download some forms.
Install the new version and open the list of forms.
Inspect the forms database and verify that the new entitiesVersion column has been added, with its value set to null.

Here are the apks for testing:
ODK-Collect.zip

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
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

alyblenkin · 2024-10-07T22:22:33Z

I also like this approach because the user can continue doing something else and try again later rather than waiting for a progress dialog. A few ideas for copy:

Unable to open form
Your form cannot be opened right now because it's being updated. Please try again in a few minutes.

Unable to open form
We are trying to process your form but it cannot be opened right now. Please try again in a few minutes.

Updating form
Your form cannot be opened right now because it's being updated. Please try again shortly.

lognaturel · 2024-10-09T22:54:05Z

Do we have a way of knowing at form open time whether a form has an Entity List attachment? I keep thinking about how blocking form open could be really disruptive if form updates take some time. If there were a way to limit it only to forms that really need to be blocked that would reduce the risk a lot.

grzesiek2010 · 2024-10-10T06:39:29Z

Do we have a way of knowing at form open time whether a form has an Entity List attachment?

No, we could use the new form parser to determine that either during downloading and then saving that in the database or parsing again before opening a form without changing the database schema.

collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt

lognaturel · 2024-10-10T18:59:11Z

No, we could use #6428 to determine that either during downloading and then saving that in the database or parsing again before opening a form without changing the database schema.

Thanks!

Both forms that use Entity Lists and those that update/create Entities need to have the blocking behavior. Since there are two things to check from different sources, I think using a db column makes sense.

Here's a little brainstorm about how that could work. We would set that column to true if:

At least one attachment is indicated in the manifest as having type of entityList
There's an entity block in meta (can expand the new metadata parser to detect this)

Those can be completely independent from each other -- it's ok if we set true multiple times and there's no path to toggling it back to false.

We have a similar concept of "entity-related" forms in Central.

I think it's fine if forms that get pushed from adb rather than downloaded get missed. Most likely those wouldn't be updated from a server anyway.

~~Let's plan on merging this PR first and then following up with #6452~~ Ok to do it all here.

For the dialog text, let's go with @alyblenkin's last suggestion but use the default title:

Form can't be used
This form cannot be opened right now because forms are being updated. Please try again shortly.

grzesiek2010 · 2024-10-16T14:27:55Z

@seadowg you can review this pr but please don't merge yet. I'm going to add changes for #6452 so that it is tested once.

collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt

seadowg

one-question-entity-follow-up.xml has entities-version in the model. Am I missing something?

We discussed elsewhere that that form doesn't need the version so we can't rely on it.

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

collect_app/src/test/java/org/odk/collect/android/external/FormUriActivityTest.kt

+            ).build()
+        )
+
+        changeLock.tryLock()


Maybe we should introduce a lock method that explodes (probably with an IllegalStateException) if the already locked. This would be better in tests as we wouldn't have to worry about tryLock not working.

seadowg · 2024-10-28T10:28:30Z

.../src/test/java/org/odk/collect/android/formlists/blankformlist/BlankFormListViewModelTest.kt

@@ -47,7 +47,7 @@ class BlankFormListViewModelTest {
    private val changeLockProvider: ChangeLockProvider = mock()
    private val projectId = "projectId"

-    private val changeLock = BooleanChangeLock()
+    private val changeLock = ThreadSafeBooleanChangeLock()


I think I'd prefer to keep BooleanChangeLock around for tests as we don't need the extra synchronization performance overhead there.

collect_app/src/main/java/org/odk/collect/android/database/forms/FormDatabaseMigrator.java

shared/src/main/java/org/odk/collect/shared/locks/ChangeLock.kt

seadowg · 2024-10-28T10:39:07Z

collect_app/src/main/java/org/odk/collect/android/database/forms/FormDatabaseMigrator.java

+                + GEOMETRY_XPATH + " text, "
+                + DELETED_DATE + " integer, "
+                + LAST_DETECTED_ATTACHMENTS_UPDATE_DATE + " integer, " // milliseconds
+                + USES_ENTITIES + " text);");


This could be not null if we add a default (probably default "false")

Yes but there is no advantage in it if we then read those values in the way we do: Boolean.valueOf(cursor.getString(usesEntitiesColumnIndex))
This handles null values correctly.

seadowg · 2024-10-28T10:58:03Z

collect_app/src/test/java/org/odk/collect/android/projects/ProjectResetterTest.kt

@@ -202,7 +202,7 @@ class ProjectResetterTest {
        saveTestInstanceFiles(currentProjectId)
        setupTestInstancesDatabase(currentProjectId)

-        (changeLockProvider.create(currentProjectId).instancesLock as BooleanChangeLock).lock()
+        (changeLockProvider.create(currentProjectId).instancesLock as ThreadSafeBooleanChangeLock).tryLock()


You drop the cast here I think.

seadowg · 2024-10-28T10:58:29Z

...ect_app/src/test/java/org/odk/collect/android/instancemanagement/InstancesDataServiceTest.kt

@@ -68,7 +68,7 @@ class InstancesDataServiceTest {

    @Test
    fun `instances should not be deleted if the instances database is locked`() {
-        (projectDependencyModule.instancesLock as BooleanChangeLock).lock()
+        (projectDependencyModule.instancesLock as ThreadSafeBooleanChangeLock).tryLock()


Again, we can drop this cast probably.

collect_app/src/main/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParser.kt

seadowg

I'm happy for all my outstanding comments to be handled in a follow-up PR. I'll accept this and create a new issue.

dbemke · 2024-10-30T06:46:08Z

I'm not sure about this case: After upgrading Collect with entities form the column "usesEntities" is null. If I open and entity form and wait for the match exactly, it downloads forms/new versions. Is it ok for forms after upgrading?

dbemke · 2024-10-30T06:50:05Z

Should we also test savepoints?

seadowg · 2024-10-30T09:23:19Z

I'm not sure about this case: After upgrading Collect with entities form the column "usesEntities" is null. If I open and entity form and wait for the match exactly, it downloads forms/new versions. Is it ok for forms after upgrading?

Yeah, so form updates will be possible during form entry for that form until it gets its first update with the new Collect right? That's fine as the form won't use offline/local entities until that update is done (and the form is opened again) and by that time usesEntities should be set to true.

Should we also test savepoints?

I can't think of any problems that might come up with save points, but if you have some suspicions you should go for it!

dbemke · 2024-10-30T09:26:47Z

til it gets its first update with the new Collect right

By update you mean the new version of the form or update meaning new entity in the dataset/ new property, or both?

seadowg · 2024-10-30T09:29:40Z

By update you mean the new version of the form or update meaning new entity in the dataset/ new property, or both?

Sorry that was unclear. Both. I just mean the form update runs (as that will lead to local entities being created from the entity list CSV).

dbemke · 2024-10-31T13:55:43Z

We finished testing the PR for now. There are issues: #6485, #6482 and the one we discussed in Slack about the expected behavior in "usesEntities" column, so we'll go back to testing after fix for #6485 which I think is the main blocker to label the PR "behavior verified".

grzesiek2010 force-pushed the COLLECT-6232 branch from 3e4d5f3 to b008359 Compare October 4, 2024 09:40

grzesiek2010 requested a review from lognaturel October 4, 2024 12:22

grzesiek2010 marked this pull request as ready for review October 4, 2024 12:22

seadowg requested review from seadowg and removed request for lognaturel October 8, 2024 14:58

seadowg requested changes Oct 10, 2024

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt Outdated Show resolved Hide resolved

lognaturel mentioned this pull request Oct 10, 2024

Only block form launch during update for Entity-related forms #6452

Closed

3 tasks

grzesiek2010 force-pushed the COLLECT-6232 branch from d89eccb to e16f39a Compare October 16, 2024 13:16

grzesiek2010 requested a review from seadowg October 16, 2024 14:17

seadowg requested changes Oct 17, 2024

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt Outdated Show resolved Hide resolved

grzesiek2010 marked this pull request as draft October 18, 2024 10:55

grzesiek2010 force-pushed the COLLECT-6232 branch 7 times, most recently from 51b53d5 to c8cfc2c Compare October 24, 2024 12:34

grzesiek2010 added 7 commits October 24, 2024 16:08

Updated ChangeLock interface

e399924

Added a new error message

7b16896

Added failing tests for starting a form during form updates

e6932be

Display an error when starting a form during form updates

da8840a

Moved form opening errors to one section

987b0d7

Added failing tests for updating forms during form filling

7507bb7

Lock forms db before opening a form

cdcdc0d

grzesiek2010 requested a review from seadowg October 25, 2024 14:00

seadowg requested changes Oct 25, 2024

View reviewed changes

Updated the new column name

5f8f639

grzesiek2010 marked this pull request as draft October 25, 2024 17:01

grzesiek2010 force-pushed the COLLECT-6232 branch from d367482 to 3c1e382 Compare October 25, 2024 17:13

grzesiek2010 added 4 commits October 25, 2024 20:25

Updated the form parser

ec60681

Check if forms contain entity attachemnts as well

5046168

Added new tests

c135792

Updated detecting forms with entities in FormUriActivity

c4e1d30

grzesiek2010 force-pushed the COLLECT-6232 branch from 16a9ce7 to c4e1d30 Compare October 25, 2024 18:25

grzesiek2010 marked this pull request as ready for review October 25, 2024 18:51

grzesiek2010 added 5 commits October 25, 2024 21:17

Only unlock lock if they were locked succesfully

39551d5

Removed redundant BooleanChangeLock

1bd04b2

Added tests for ThreadSafeBooleanChangeLock

c1ea862

Keep ChangeLock inteface and its only implementation in one file

6cceed3

Added a new test

cc46949

grzesiek2010 requested review from lognaturel and seadowg October 26, 2024 14:13

seadowg requested changes Oct 28, 2024

View reviewed changes

seadowg approved these changes Oct 28, 2024

View reviewed changes

seadowg added the needs testing label Oct 28, 2024

seadowg mentioned this pull request Oct 28, 2024

Address comments for PR 6440 #6476

Closed

grzesiek2010 merged commit 2e63b33 into getodk:master Oct 28, 2024
6 checks passed

grzesiek2010 mentioned this pull request Oct 29, 2024

Address comments for PR 6440 #6481

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent opening forms that use entities during form updates, and block form updates while filling out a form that uses entities #6440

Prevent opening forms that use entities during form updates, and block form updates while filling out a form that uses entities #6440

grzesiek2010 commented Oct 3, 2024 •

edited

Loading

alyblenkin commented Oct 7, 2024 •

edited

Loading

lognaturel commented Oct 9, 2024

grzesiek2010 commented Oct 10, 2024

lognaturel commented Oct 10, 2024 •

edited

Loading

grzesiek2010 commented Oct 16, 2024

seadowg left a comment

seadowg Oct 28, 2024

grzesiek2010 Oct 29, 2024

seadowg Oct 28, 2024

grzesiek2010 Oct 29, 2024

seadowg Oct 28, 2024

grzesiek2010 Oct 28, 2024

seadowg Oct 28, 2024

grzesiek2010 Oct 29, 2024

seadowg Oct 28, 2024

grzesiek2010 Oct 29, 2024

seadowg left a comment

dbemke commented Oct 30, 2024 •

edited

Loading

dbemke commented Oct 30, 2024

seadowg commented Oct 30, 2024

dbemke commented Oct 30, 2024

seadowg commented Oct 30, 2024

dbemke commented Oct 31, 2024

Prevent opening forms that use entities during form updates, and block form updates while filling out a form that uses entities #6440

Prevent opening forms that use entities during form updates, and block form updates while filling out a form that uses entities #6440

Conversation

grzesiek2010 commented Oct 3, 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:

alyblenkin commented Oct 7, 2024 • edited Loading

lognaturel commented Oct 9, 2024

grzesiek2010 commented Oct 10, 2024

lognaturel commented Oct 10, 2024 • edited Loading

grzesiek2010 commented Oct 16, 2024

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

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

dbemke commented Oct 30, 2024 • edited Loading

dbemke commented Oct 30, 2024

seadowg commented Oct 30, 2024

dbemke commented Oct 30, 2024

seadowg commented Oct 30, 2024

dbemke commented Oct 31, 2024

grzesiek2010 commented Oct 3, 2024 •

edited

Loading

alyblenkin commented Oct 7, 2024 •

edited

Loading

lognaturel commented Oct 10, 2024 •

edited

Loading

dbemke commented Oct 30, 2024 •

edited

Loading