Allow forms with parse errors to be downloaded, show errors at form open time #6428

grzesiek2010 · 2024-09-24T19:24:30Z

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

As discussed in the issue, I have implemented a new form metadata parser to read the metadata without performing a full Javarosa parse. The most difficult and potentially risky part is how we read the geometry XPath. I think we should release another beta with these changes to ensure the parser doesn’t throw errors due to certain types of forms. We have tested many different forms, and everything is working well, so I’m hopeful that everything will be fine.

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 have introduced a new form parser that reads form metadata when a form is added to the database. This can occur after a manual download, a match exactly syncing, or syncing forms from disk after moving them to the forms directory manually (using ADB or a file manager).
Testing this new parser with various forms is crucial to ensure it functions correctly, so please try loading as many forms as possible.
Another change introduced by this pull request is that we no longer ignore broken forms after downloading. Previously, they were not displayed on the list. Now, they should be downloaded and shown to users, but an error will appear when attempting to open them. An example of this is using an entity form with an unsupported version.

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

strings/src/main/res/values/strings.xml

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

collect_app/src/main/java/org/odk/collect/android/utilities/FileUtils.java

lognaturel

The function to get geometry XPath doesn't seem quite right. It may be helpful to add a test where there's a setvalue action AND a geopoint with a user interface and the action's target is after the point with a UI.

.../src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java

collect_app/src/test/java/org/odk/collect/android/formmanagement/FormMetadataParserTest.java

lognaturel · 2024-09-26T16:40:59Z

.../src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java

-     * so we need to make sure we've got those files in the right place before we parse.
-     */
-    @Test
-    public void whenFormHasMediaFiles_downloadsAndSavesFormAndMediaFiles_beforeParsingForm() throws Exception {


This is not really redundant, right? It's more that there was some accidental interaction between between media files and metadata parsing that isn't there anymore and so doesn't need to be tested?

It seems that at one point, the order was important, and media files were considered during form parsing. However, this didn’t work as intended, as the parser ignored the media files (on current master). See my other comment: #6428 (comment)
So all in all we don't need it anymore.

lognaturel · 2024-09-26T16:43:23Z

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/EntityFormTest.kt

@@ -192,7 +192,7 @@ class EntityFormTest {
    }

    @Test
-    fun manualEntityFormDownload_withUnsupportedSpecVersion_completesSuccessfully() {
+    fun manualEntityFormDownload_withUnsupportedSpecVersion_completesSuccessfully_butThrowsAnErrorAfterOpeningIt() {


Do we really need all three of these? I feel like we could maybe use our knowledge of the shared metadata parsing implementation and only test one of the paths given that these tests are so slow to run.

I thought about that and keeping just one test, but since I didn't really favor any option, I decided to add all three. For me, the time required to run these tests has never been a significant issue, as I don’t run them in bulk locally. I always use Firebase and get results in 4-5 minutes. Additionally, we have recently removed many tests from the regression package—see the last part: #6383 so I believe we have fewer tests overall. Generally, I think it’s better to have such tests, even if they take some time to run. However, as I mentioned, I was unsure about keeping or removing them, so I'm fine with keeping just one test if that’s your preference, just let me know.

lognaturel · 2024-09-26T16:52:47Z

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

        val mainInstanceRoot = mainInstance.getElement(0)
        for (elementId in 0 until mainInstanceRoot.childCount) {
            val child = mainInstanceRoot.getElement(elementId)
            val ref = "/${mainInstanceRoot.name}/${child.name}"
-            if (ref == firstTopLevelBodyGeoPoint) {


The goal of this function is to only return the target of a setvalue action if it comes before a user-facing geopoint. I think returning here is right -- it stops the search. However, I think the value returned should be firstTopLevelBodyGeoPoint so maybe there was a bug in the original implementation.

The goal of this function is to only return the target of a setvalue action if it comes before a user-facing geopoint.

Hmm setgeopoint action represents Geolocation at survey start right? It's the action triggered after opening a form so how could anything go before? Isn't it always the first geo element?

Currently we only exposed the setvalue action triggered by the new instance event in XSLForm. You can also have setvalue triggered by other events like value change. Neither is guaranteed to be in any specific order. In the case of a setvalue triggered by value change, the action would be in the body.

I think we need to take a different approach on this one. When we had the processed form definition from JR we could do smarter things but I don’t think they’re practical in this context. Maybe we could do something like

iterate through the binds, build a set of the refs that have type geopoint

iterate through the primary instance, return the first ref that’s in the set of geopoints

That makes some assumptions that are not guaranteed by XForms, most notably that refs in the body are in the same order in the model. But in practice they usually are so I think it’s reasonable.

Alright, we can do that. However, my question is: if the order isn't guaranteed, why check the primary instance rather than just the binds? I assume the order of the binds is even less predictable?

Yes, exactly.

Ok so please take a look at the new implementation.

collect_app/src/androidTest/java/org/odk/collect/android/feature/formentry/InvalidFormTest.kt

lognaturel

Some of the naming tripped me up so I'd recommend updating it but otherwise this looks like what we agreed on. 👍

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

..._app/src/test/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParserTest.kt

dbemke · 2024-10-16T13:28:22Z

Tested with Success!

Verified on a device with Android 10

Verified cases:

adding a form with non-existing entity spec
downloading from Central forms which used to fail to download
downloading project with many forms
downloading new versions of forms and new media

WKobus · 2024-10-16T13:31:58Z

Tested with Success

Verified on device with Android 14

Added failing tests

7695411

grzesiek2010 force-pushed the formparser branch 2 times, most recently from 258bbf7 to 1b75c7b Compare September 24, 2024 23:16

grzesiek2010 added 12 commits September 25, 2024 15:16

Reoworked the FormMetadataParser to use simple form parsing

29e6b84

Use the new form parser

c2f9a0f

Removed redundant tests

6afec6e

Moved testing FormMetadataParser to FormMetadataParserTest

5ac242d

Converted FormMetadataParserTest to kotlin and simplified it

b1b4d34

Removed redundant test

7aa2427

Added clear error message for unrecognized entity version

7aa4a86

Improved tests

a132e07

Removed redundant code

c8f6a5f

Fixed broken form test

cbecde6

Fixed parsisng forms with comments

a3155cb

Moved form parsing to its own package

2cbdd45

grzesiek2010 force-pushed the formparser branch from 1b75c7b to 2cbdd45 Compare September 25, 2024 13:46

grzesiek2010 commented Sep 25, 2024

View reviewed changes

strings/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

grzesiek2010 commented Sep 25, 2024

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/formmanagement/metadata/FormMetadataParser.kt Outdated Show resolved Hide resolved

grzesiek2010 marked this pull request as ready for review September 25, 2024 16:30

grzesiek2010 requested a review from lognaturel September 25, 2024 16:30

grzesiek2010 commented Sep 25, 2024

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/utilities/FileUtils.java Show resolved Hide resolved

grzesiek2010 added 2 commits September 26, 2024 12:15

Improved the error message

454d0d5

Improved error dialog title

5d3aaff

lognaturel requested changes Sep 26, 2024

View reviewed changes

grzesiek2010 requested a review from lognaturel September 27, 2024 06:37

grzesiek2010 added 2 commits September 30, 2024 15:58

Improved reading geometry xpaths

3d65c6f

Reworked tests

126896e

lognaturel approved these changes Sep 30, 2024

View reviewed changes

lognaturel added the needs testing label Sep 30, 2024

lognaturel changed the title ~~Show Entities version error to the user~~ Allow forms with parse errors to be downloaded, show errors at form open time Sep 30, 2024

grzesiek2010 added 2 commits September 30, 2024 20:03

Naming improvements

f49862e

Simplified readMetadata_canParseFormsWithComments test

df9d70e

grzesiek2010 force-pushed the formparser branch from a7a4bd9 to df9d70e Compare September 30, 2024 18:14

lognaturel approved these changes Sep 30, 2024

View reviewed changes

grzesiek2010 merged commit 719e214 into getodk:master Oct 1, 2024
6 checks passed

grzesiek2010 mentioned this pull request Oct 10, 2024

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

Merged

6 tasks

WKobus added behavior verified and removed needs testing labels Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow forms with parse errors to be downloaded, show errors at form open time #6428

Allow forms with parse errors to be downloaded, show errors at form open time #6428

grzesiek2010 commented Sep 24, 2024 •

edited

Loading

lognaturel left a comment

lognaturel Sep 26, 2024

grzesiek2010 Sep 26, 2024

lognaturel Sep 26, 2024

grzesiek2010 Sep 26, 2024

lognaturel Sep 26, 2024

grzesiek2010 Sep 26, 2024

lognaturel Sep 27, 2024

lognaturel Sep 30, 2024

grzesiek2010 Sep 30, 2024

lognaturel Sep 30, 2024

grzesiek2010 Sep 30, 2024

lognaturel left a comment

dbemke commented Oct 16, 2024

WKobus commented Oct 16, 2024

Allow forms with parse errors to be downloaded, show errors at form open time #6428

Allow forms with parse errors to be downloaded, show errors at form open time #6428

Conversation

grzesiek2010 commented Sep 24, 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:

lognaturel 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

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

dbemke commented Oct 16, 2024

WKobus commented Oct 16, 2024

grzesiek2010 commented Sep 24, 2024 •

edited

Loading