Resolves #4500 autocomplete minimal…

[dimwight]

Resolves #4500

This documents a revision of #4673 now closed.

What has been done to verify that this works as intended?

Tested with new SelectOneResetTest.

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

See design notes below.

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?

Improvement to behaviour from user perspective.
No apparent regression risks.

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

selectOneReset.zip

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

Perhaps form design advice could specify the benefits and limitations of the improved behaviour?

Before submitting this PR, please make sure you have:

• [x ] run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• [x ] verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• [x ] verified that any new UI elements use theme colors. UI Components Style guidelines

---

Design notes

A lot of stuff here, but both the issue and the proposed resolution have turned out to be quite complex.

The issue

A shout out to @aurdipas for a form that demonstrates clearly the undesired behaviour of selects with fast external itemsets.

• Most obviously, answers for cascades using fast external itemsets don't reset (or at best not immediately) when preceding members are updated, in constrast to their behaviour with internal itemsets.

• This behaviour is most glaringly apparent with minimal appearances, especially in field lists - plain select widgets do reset when they can’t match an existing value in their current choices.

Desirable if less obvious behaviours also implied by the form are that

• Members should reset even when unrelated fields are interpolated in the cascade.
• Members hidden by an interpolated select when non-relevant should be reset once unhidden.

And all this should work inside groups and especially field lists; the hierarchy views should be reset immediately upon each change.

Not only may the current behaviour confuse the user, it permits saving of incoherent data.

So a resolution to this issue must focus on resetting the answers for a complete cascade, rather than just ensuring that widgets reset when rendered.

The update

Preparatory changes

clearFollowingItemsetWidgets in SelectOneWidget
sort of resets succeding cascade members, but hitherto there’s been no equivalent in SelectOneMinimalWidget.

So to start with I recycled clearFollowingItemsetWidgets:

• Pulled it up to ItemsWidget
• Called it from SelectOneMinimalWidget by analogy with SelectOneWidget

Widget sequences (one per view)

SelectOneWidgetUtils now has checkFastExternalCascade, called (for now) from a stub clearFollowingItemsetWidgets, of which it extends the original algorithm.

The new method:

• Detects fast external cascade members from their query strings
• Returns at non-question or on encountering a new cascade
• Skips over interpolated questions - including unrelated selects as in the issue form
• Like the code it derives from, has the helpful side-effect of resetting questions that are about to be hidden by an interpolated select

Field lists

The basic fix aborts silently when the cascade is in a field list as in the issue form.

In FormEntryActivity, updateFieldListQuestions currently resets only the next cascade member (possibly as a side effect?).

In this improvement it calls checkFastExternalCascadeInFieldList, also living in SelectOneWidgetUtils, and implementing the same algorithm as checkFastExternalCascade; except that hidden questions are reset when unhidden.

Testing

The issue appears resolved as outlined above when changes are tested manually against the issue form (when slightly modified with no cascade splitting between groups).

For formal testing there are numerous potential use cases, representing combinations of:

• 2 types of itemset: internal (as a baseline) and fast external
• 4 appearances: plain, minimal, minimal autocomplete and just autocomplete (never used in practice?)
• 5 contexts: top-level; simple and nested groups; simple and nested field lists

Test form

The form derives from selectOneExternal.zip from #4258 - thanks @mmarciniak90. It covers

• four cascade members - seems the maximum in practice
• interpolated fields - actually relevance selects before last members as in the issue form
• use cases broken up into
  • sections covering each combination of itemset and appearance
  • each section split into blocks covering the contexts

The workbook includes these supporting sheets:

• The variants sheet defines section numbers in terms of the two itemset types and the two essentially different appearances.
• The asserts sheet records results of testing both before and after the update.
• The master sheet defines five functionally identical context blocks:
  • A - top level
  • B - simple group
  • C - nested group
  • D - field list
  • E - nested field list
• It includes two alternative type and four appearance columns, and prefills defaults ready for testing of reset behaviour.
• These are propagated in the survey sheet into sections testing the various combinations of appearances and itemsets.
• For this draft PR the form implements just the four core variants:
  • 0 and 1 - internal itemsets with plain and minimal appearances
  • 2 and 3 - fast external itemsets

Test class

SelectOneResetTest borrows extensively from FieldListUpdateTest. In each block it makes changes and asserts expected outcomes as set out in the workbook asserts sheet.

• The SectionVariant enum combines an itemset based on the type of its section with an appearance from the appearance.

• The Block enum provides the appropriate strings for each SectionVariant.

The single test method iterates through the variants, thus visiting all sections of the prefilled form and running logically equivalent tests against the context blocks comprising each section.

Tests are identified in the code as detailed in the workbook asserts sheet.

The code makes use of new convenience methods in
FormEntryPage that wrap useful formulations found in FieldListUpdateTest.

SelectOneResetTest can be validated by running against the current codebase and checking that it fails where expected.

[dimwight]

The test_app failure is puzzling as it passed for my original commits.

[dimwight]

I'm relieved to find that no one seems yet to have looked at this - I realised today that the core code can be reworked into something much simpler. Please watch this space!

[dimwight]

New revision now ready for PR, pending resolution of GitHub issues (my fork out of sync with upstream).

Maybe leave this until #4762 and #4797 have been reviewed?

[lognaturel]

core code can be reworked into something much simpler.

That's exciting! Apologies for the general radio silence. I'm not sure whether you've seen but I now have a newborn so my availability and sanity are low. Other folks are taking much-needed time off and working on a less intense release! We so appreciate all of what you've been up to here. 🙏

my fork out of sync with upstream

Please let us know if we can help. If your master is out of sync, you can likely create a new branch off of it to save your changes (git branch -b my-branch), then reset (git reset upstream/master), then clean (git clean -df), then force push to your fork (git push --force). That hopefully should get you to a clean place.

I saw somewhere that you had recommended an order for reviewing open PRs but I can't find it again. Is what you write here the best order to follow?

[dimwight]

Having been alerted to the idea of keeping the PR list as tidy as possible, I'm closing this one for now.