Validate geopoly on changes when question has incremental param by seadowg · Pull Request #6989 · getodk/collect

seadowg · 2025-12-12T11:21:43Z

As well as adding the feature I made two other changes:

Improved how (transient) snackbars are asserted on in UI tests. I was running into flakes that were getting in my way.
Removed PMD exclusions for deleted rules. These were making it hard to read some quality errors I was seeing.

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

There's probably not a lot of surprising here after the last sets of changes made for this feature: I basically just hooked up FormEntryViewModel's currentIndex and onAnswerQuestion to GeoPolyFragment through GeoPolyDialogFragment so that the question is answered (and constraints validated) every time a point is added or removed.

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 did convert the geopoly UI from Java to Kotlin, so outside of testing the new feature, it's worth testing the geopoly flow as a whole. Again, nothing with the actual map integration has changed, so I think it's safe to just use one engine.

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

androidshared/src/main/java/org/odk/collect/androidshared/ui/AlertStore.kt

test-shared/src/main/java/org/odk/collect/testshared/Assertions.kt

grzesiek2010 · 2025-12-14T13:44:03Z

geo/src/test/java/org/odk/collect/geo/geopoly/GeoPolyFragmentTest.kt

 import android.os.Bundle
 import androidx.fragment.app.FragmentResultListener
 import androidx.lifecycle.Lifecycle
+import androidx.lifecycle.MutableLiveData


There is a lot of !! in kotlin version. Can we review the class and improve that?

As far as I can see we only use !! for accessing result in FragmentResultRecorder. I'm not sure I have a better solution for asserting on nullable values (as doing something like assertNotNull won't get you smart cast and lateinit isn't really correct), but it is something that comes up in tests as a lot. Maybe we should write a helper to avoid it that asserts and casts for us? Maybe something like:

assertNotNull(result) { assertThat(it, equalTo(GeoPolyFragment.REQUEST_GEOPOLY)) }

Sorry this comments was mant to be added in GeoPolyFragment, not in the test class.

Ah I see! I can take a look there.

I've removed a chunk of them, but there's still quite a few for map. I think these should really be reworked by moving the polyline state to MapViewModel so that hosts don't have to interact with a child Fragment. I can hopefully do that refactor as part of the upcoming restyle of the screen.

geo/src/main/java/org/odk/collect/geo/geopoly/GeoPolyFragment.kt

geo/src/test/java/org/odk/collect/geo/geopoly/GeoPolyFragmentTest.kt

grzesiek2010 · 2025-12-14T16:52:01Z

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

+            val geopoly = result.getString(GeoPolyFragment.RESULT_GEOPOLY)
+            val incremental = FormEntryPromptUtils.getBindAttribute(prompt, "incremental")
+
+            if (incremental == "true" && geopolyChange != null) {


Shouldn't we perform a case-insensitive check for true?
Maybe we should introduce getBooleanBindAttribute function?

I don't think we need to be that permissive, but I do recall we might do that elsewhere?

We already sanitize appearances here:
https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/utilities/Appearances.kt#L97
In theory, we could rely on pyxform, but for various reasons probably because it’s not guaranteed to be perfect, or because other tools can be used to parse XLS forms, we started doing this ourselves.
That’s why I thought we might want to do the same for bind attributes as well. If you’re not sure about this approach, we can always file a separate issue and discuss it there.

Let's go with another issue. The current implementation fits the spec and I think doing anything else is something we need to discuss.

...ect_app/src/test/java/org/odk/collect/android/widgets/utilities/GeoPolyDialogFragmentTest.kt

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

grzesiek2010

#6989 (comment)
and
#6989 (comment)

geo/src/main/java/org/odk/collect/geo/geopoly/GeoPolyFragment.kt

dbemke · 2025-12-17T12:13:06Z

Could you check if the form I created is ok? When I try to upload it to Central there's information that Central doesn't know the incremental parameter.
geoshapeTraceInvalidPolygonIncremental.xlsx.txt

dbemke · 2025-12-17T12:49:23Z

When I try to upload it to Central there's information that Central doesn't know the incremental parameter.

Central only allows adding xml with incremental

seadowg · 2025-12-17T13:20:25Z

Central only allows adding xml with incremental

Sorry should have specified this in the PR description. Yes you'll need to add incremental to the bind for the question in the XML rather than in the XLSForm for the moment.

seadowg added 16 commits December 12, 2025 10:51

Extract class for helping test transient alerts

44d441c

Use AlertStore for testing Snackbars

6854b74

Add docs for AlertStore

c67d0d3

Update nullable annotations in GeoPolyFragment

8fe6c45

Switch uses of ArrayList to List in GeoPolyFragment

ccae6c3

Remove excludes for remove rules

e6d13ed

Convert GeoPolyFragment to Kotlin

73bc669

Clean up generated code

182f081

Allow SnackbarUtils to show created Snackbars

52ae54c

Add invalidMessage feature to GeoPolyFragment

cf8c147

Emit event when polygon changes

fd79f14

Answer geopoly question incrementally

72cbcad

Show snackbar when geopoly is invalid

2b553b0

Validate polygon changes

596ee47

Fallback to default constrain message

9cf508b

Use helper to grab bind attribute

07cac2e

seadowg marked this pull request as ready for review December 12, 2025 14:24

seadowg requested a review from grzesiek2010 December 12, 2025 14:24

seadowg added 2 commits December 12, 2025 14:58

Fix assertion

7eae49b

Correct method overload under test

1ee4d96

grzesiek2010 requested changes Dec 14, 2025

View reviewed changes

grzesiek2010 reviewed Dec 15, 2025

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/widgets/utilities/GeoPolyDialogFragment.kt Show resolved Hide resolved

seadowg added 8 commits December 16, 2025 12:16

Rename method

6a4b8bd

Update assertion implementation to use Kotlin collections

c351e37

Correct boolean operator

178bef5

Remove unneeded parenthesis

23231bd

Use isNotEmpty

c0b70b4

Use isVisible

7cdeae6

Use SnackbarUtils

abdd2bd

Update constant

711c0a7

seadowg added 4 commits December 16, 2025 13:52

Move test helper to test-shared

780e93e

Clarify test

9c14279

Fix adding points to non-incremental questions

0239124

Create constants object for bind attributes

85bb499

seadowg requested a review from grzesiek2010 December 16, 2025 16:15

grzesiek2010 requested changes Dec 16, 2025

View reviewed changes

Remove view fields from GeoPolyFragment

8711daa

seadowg requested a review from grzesiek2010 December 17, 2025 09:36

grzesiek2010 requested changes Dec 17, 2025

View reviewed changes

geo/src/main/java/org/odk/collect/geo/geopoly/GeoPolyFragment.kt Outdated Show resolved Hide resolved

seadowg requested a review from grzesiek2010 December 17, 2025 10:44

grzesiek2010 approved these changes Dec 17, 2025

View reviewed changes

grzesiek2010 merged commit cba0e5a into getodk:master Dec 17, 2025
7 checks passed

grzesiek2010 added the needs testing label Dec 17, 2025

seadowg deleted the incremental branch December 17, 2025 13:20

dbemke added behavior verified and removed needs testing labels Dec 18, 2025

Conversation

seadowg commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Before submitting this PR, please make sure you have:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grzesiek2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbemke commented Dec 17, 2025

Uh oh!

dbemke commented Dec 17, 2025

Uh oh!

seadowg commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seadowg commented Dec 12, 2025 •

edited

Loading