Skip to content

Always save the answer before evaluating it#7083

Merged
seadowg merged 3 commits intogetodk:v2026.1.xfrom
grzesiek2010:COLLECT-7078
Feb 10, 2026
Merged

Always save the answer before evaluating it#7083
seadowg merged 3 commits intogetodk:v2026.1.xfrom
grzesiek2010:COLLECT-7078

Conversation

@grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Feb 7, 2026

Closes #7078

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

Recently, we introduced changes to how answering, validating, and navigating between questions is handled. We unified these flows by using the #updateIndex mechanism (see FormEntryViewModel). The main advantage of this approach is that there is now a single, shared way of handling these actions, which makes the code easier to maintain and less error-prone.

That said, there are some drawbacks. One of them appears when the current question is required or has a constraint, the user violates it (by not answering or providing an invalid answer), and then tries to navigate forward with validate upon forward swipe enabled in the settings. In this case, the new mechanism refreshes the entire view to correctly highlight the violated question. Previously, we only updated the question layout without refreshing the whole view, which was lighter but also less consistent with how we handle similar actions elsewhere.

Although refreshing the view may seem like overkill, I still believe it’s better than the old approach. Instead of reverting the change or introducing special-case handling (for highlighting violated questions), it makes more sense to adjust how answers are saved.

The reason the answer disappears after the refresh is that we currently don’t save an answer if it’s invalid when navigating forward. I don’t see a strong reason for this behavior - answers can be saved first and then evaluated. I introduced this change, which fixes the issue. We already save invalid answers (without evaluating them) when navigating backward or when opening the hierarchy view, so this is consistent with existing behavior and actually makes the overall flow more uniform.

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 explained above, the main change is that when attempting to navigate to the next question while the current answer is invalid, the answer is now saved before it is evaluated. From a user’s perspective, there should be no visible difference this change only affects the internal flow.
While it’s always possible that there’s an edge case I haven’t considered, I don’t currently see a scenario where this behavior would cause issues.

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

The form is linked in the issue.

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

@grzesiek2010 grzesiek2010 marked this pull request as ready for review February 7, 2026 13:23
@grzesiek2010 grzesiek2010 requested a review from seadowg February 7, 2026 13:23
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason the answer disappears after the refresh is that we currently don’t save an answer if it’s invalid when navigating forward. I don’t see a strong reason for this behavior - answers can be saved first and then evaluated.

When we were first discussing incremental that came up! The behaviour wasn't tested, and it's hard to come with an argument in its favour. I'm just tagging @lognaturel in on this change in case there are wider ramifications here.

@seadowg seadowg merged commit e08225b into getodk:v2026.1.x Feb 10, 2026
7 checks passed
@seadowg seadowg added the high priority Should be looked at before other PRs/issues label Feb 10, 2026
@lognaturel
Copy link
Member

When the value is saved, that triggers the full evaluation cascade, right? I think it's mostly a performance consideration -- if you know the value is invalid, avoiding recomputation could avoid unpleasant lag. An invalid value in an evaluation cascade also could lead to confusing errors being thrown about later dependent values, I think. That said, I believe we already did run the whole cascade for invalid values in field lists so there's precedent. We can go with it and keep a close eye on crashes and ANRs.

@dbemke
Copy link

dbemke commented Feb 11, 2026

I reproduced the issue about the date time widget on 2026.1.0 beta 3, so the issue still occurs. Should it be fixed on beta 3?

@seadowg
Copy link
Member

seadowg commented Feb 11, 2026

@dbemke yeah as far as I'm aware!

@grzesiek2010 could this be specific to the date time widget because the answer has two components?

@grzesiek2010
Copy link
Member Author

grzesiek2010 commented Feb 11, 2026

Ahh, right when a dateTime question is required but only one component (date or time) is answered, it returns a null value. As a result, when we try to save the answers, it saves null.

The same thing happens (the answer gets cleared) when we open the hierarchy view or navigate to the previous question. I don’t think it’s something critical to address. The issue with other question types + constraints was way more serious.

@seadowg
Copy link
Member

seadowg commented Feb 11, 2026

The same thing happens (the answer gets cleared) when we open the hierarchy view or navigate to the previous question. I don’t think it’s something critical to address. The issue with other question types + constraints was way more serious.

Agreed! The question hasn't actually been answered yet. @dbemke let's validate that this only affects the date time questions. If so, we can reopen the issue and make a note that it's only broken for specifically that.

seadowg added a commit to seadowg/collect that referenced this pull request Feb 12, 2026
Always save the answer before evaluating it
@dbemke
Copy link

dbemke commented Feb 12, 2026

Tested with Success!

Verified on devices with Android 10

Verified cases:

  • issue with constraints and missing values doesn't occur
  • required question
  • constraint validation
  • relevant questions based on constraints
  • different constraints processing
  • check for error

@srujner
Copy link

srujner commented Feb 12, 2026

Tested with Success!

@WKobus
Copy link

WKobus commented Feb 12, 2026

Tested with Success!

Verified on device with Android 16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

behavior verified high priority Should be looked at before other PRs/issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants