Show repeat in hierarchy view even if first instance has no relevant content #4762

dimwight · 2021-07-30T09:51:11Z

Closes #4570
~~Blocked on #4570 discussion~~

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

Tested manually and with new test added to FormHierarchyTest (empty_first_repeat is a placeholder name, help needed for a suitable final one).

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

Trivial fix to ensure that

repeats appear in hierarchy even if first is empty
relevant repeats are nonetheless listed correctly

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?

Shows no regression when tested before and after with testDebug and connectedDebugAndroidTest.

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

The issue contains example forms.

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

No, behaviour is now as expected.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
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

dimwight · 2021-07-30T10:16:48Z

Once again test_app seems not to like me for no obvious reason.

dimwight · 2021-09-03T09:00:56Z

Definitely awaiting review, does this reopen it? (Seems not)

seadowg · 2021-09-03T09:06:07Z

@dimwight I've reopened and marked ready for review. When something is ready, you can just hit the "Ready for review" button above the test/quality checks, so we know you're not still working on it.

dimwight · 2021-09-03T09:15:18Z

"Ready for review" button above the test/quality checks

Doesn't seem to appear for me, maybe because of failed check which I can't see how to resolve.

Oops, seem to have closed it again. :(

seadowg · 2021-09-06T13:51:44Z

Blocking this on making sure we fully understand the issue (I don't).

dimwight · 2021-09-06T15:16:09Z

fully understand the issue

Would it help if I spelled out in detail what I think is going on? (I certainly had some difficulty getting my head round it.)

seadowg · 2021-09-06T15:19:20Z

Would it help if I spelled out in detail what I think is going on? (I certainly had some difficulty getting my head round it.)

Yes that'd be great! Happy to dive in, but if you've already got a good idea of what's happening, it'd be great to hear.

dimwight · 2021-09-12T20:20:52Z

Hi @seadowg, an overview as promised of the issue and my proposed resolution.

In FormHierarchyActivity, when refreshView hits EVENT_REPEAT it is ready to add a HierarchyElement for one of two cases.

A (the default) if the repeat is the first in its group, display the group details; or
B (set by shouldShowRepeatGroupPicker()) display the repeat in a picker view.

Before proceeding either way, refreshView checks if the repeat is empty. If it is,

in case A, the current logic assumes that if the first repeat in a group is empty, any subsequent repeats will be as well so the whole group should be skipped.
in case B it should clearly skip the empty repeat which has no content to display or edit.

Issue #4570 arises from the corner case of A where repeats derive from a multiple select. Even if the first repeat is empty, some of those following will not be - the group should definitely be displayed.

For this PR the existing logic is refined such that the first repeat is never skipped even when empty, so its group is always displayed. The unlikely case that all repeats in a group are empty simply results in a harmless (and maybe expressive) empty picker.

dimwight · 2021-09-12T20:22:54Z

I've also updated the PR as follows.

In FormHierarchyActivity, refreshView has been lightly reworked to make the logic clearer.
In FormHierarchyTest, empty_first_repeat (just a placeholder for a more descriptive name) has been extended to check inside the group picker.
In empty_first_repeat.xml the choice list has been truncated to simplify debugging.

seadowg · 2021-09-22T10:07:14Z

in case A, the current logic assumes that if the first repeat in a group is empty, any subsequent repeats will be as well so the whole group should be skipped.

What do you mean by "empty" here? No relevant questions?

Issue #4570 arises from the corner case of A where repeats derive from a multiple select. Even if the first repeat is empty, some of those following will not be - the group should definitely be displayed.

This is the part I'm having trouble understanding. Why does the example form in #4570 (and in this PR) cause that issue? I'm probably missing something obvious, but I couldn't see why selecting 'beans' would result in different behaviour from selecting other vegetables. Is there maybe a simpler/smaller form that could illustrate the problem?

dimwight · 2021-09-23T18:25:54Z

Hi @seadowg, I'm sorry you're finding this issue hard to disentangle, it's trivial on the face of it but quite subtle.

With a bit of luck you'll find my latest commit of some help. In essence it recapitulates my own investigations and proposed fix, by adding to FormHierarchyActivity a temporary enum Scenarios4570 and associated class constant SCENARIO_4570.

The four members of Scenarios4570 encapsulate the possible permutations of two flags.

Flag `testWithBeans`

(Where 'beans' is shorthand for 'first repeat with content because first item in select multiple was checked; so that FormEntryModel regards its FormIndex, other things being equal, as relevant')

This is tested in FormHierarchyTest in the method now renamed to showsRepeatsWhenFirstIsEmpty (hitherto with the placeholder name 'empty_first_repeat'). It defines which of two forms to open: empty_first_repeat.xml to test the issue as defined in #4570 , or empty_first_repeat_beans.xml (somehat confusingly named) for scenarios expecting a non-empty first repeat. The only difference between the two is at L10 between <veg_type>2 3 5</veg_type> and <veg_type>1 2 3 5</veg_type>.

Based on the state of testWithBeans the test then adjusts its expectations of what FormHierarchyTest should display given the form chosen.

Incidentally, I use the term 'empty' in preference to 'non-relevant' because it describes the repeat so neatly ie in XML <veg_repeat/> as opposed to a 'non-empty' repeat such as:

    <veg_repeat>
        <veg_group>
            <veg_name>Beans</veg_name>
           <veg_often />
        </veg_group>
    </veg_repeat>

Flag `restoreOldLogic`

This is tested in FormHierarchyActivity at L624, and when set negates the effect of my proposed fix.

By setting the values() index for SCENARIO_4570 you can test all permutations of the two flags, with the following results:

WITH_BEANS_OLD_LOGIC will obviously pass as its first repeat will be non-empty
WITHOUT_BEANS_OLD_LOGIC won't pass because of its empty first repeat - the issue as raised by @lognaturel in Repeat is not shown in hierarchy view if first instance has no relevant content #4570
WITH_BEANS_NEW_LOGIC will pass as before
WITHOUT_BEANS_NEW_LOGIC will also pass despite having an empty first repeat

You can observe all this happening by stepping through the relevant code. In FormHierarchyActivity you will note the importance of testing shouldShowRepeatGroupPicker to ensure that repeat items appear in a picker.

Once again rather a long explanation, I hope it helps.

dimwight · 2021-09-24T07:16:46Z

And to explicitly answer your other questions as well:

...why selecting 'beans' would result in different behaviour from selecting other vegetables.

Beans is special because it's the first item in the veg_type choices; if it's not selected the first repeat is empty. And it's the first repeat that triggers the Frequencies repeat header in the hierarchy view: under the existing logic it doesn't if empty and therefore non-relevant (hence the issue), with my fix it always does.

Finally

Is there maybe a simpler/smaller form that could illustrate the problem?

I do think this is the minimal form to demonstrate the issue.

seadowg

As per our discussion on Slack, next steps are:

Remove the SCENARIO_4570 debug code
Use the simpler form (posted in the issue) for the test rather than the veg form

dimwight · 2021-09-27T15:22:34Z

1. Remove the `SCENARIO_4570` debug code

Done in 'Removed debug code'

2. Use the simpler form ... for the test rather than the veg form

Done in 'Updated test with simpler form'.

Also checked that the test fails appropriately when in FormHierarchyActivity at L609ff the code is tweaked to

boolean asOldCode = true;
if (isEmpty && asOldCode ||
        // For #4570
        (!isFirst || forPicker)) {
    break;
}

dimwight · 2021-09-27T15:26:52Z

And then test_app failed code that passed when previously committed!

seadowg

Thanks again for wrapping your head around this and then explaining it to me. There are a couple of tweaks I'd like to see, and I've also thought about a slightly different implementation that'd be good to discuss.

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java

seadowg · 2021-09-28T08:42:57Z

...ct_app/src/androidTest/java/org/odk/collect/android/feature/formentry/FormHierarchyTest.java

@@ -30,6 +30,7 @@
            .around(new CopyFormRule("formHierarchy2.xml", null))
            .around(new CopyFormRule("formHierarchy3.xml", null))
            .around(new CopyFormRule("repeat_group_new.xml", null))
+            .around(new CopyFormRule("Empty First Repeat.xml", null))


Just a note to myself to rework this test file to remove CopyFormRule before merging this. I won't make you slog through that @dimwight.

Probably a good thing as my code here is pure copy and paste, I've no idea as to how it works.

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java

seadowg · 2021-10-04T08:55:47Z

@dimwight is this ready for another review? You can request that with the 🔃 icon next to my name in "Reviewers".

seadowg · 2021-10-04T12:51:17Z

...ct_app/src/androidTest/java/org/odk/collect/android/feature/formentry/FormHierarchyTest.java

+    }
+
+    @Test
+    public void empty_first_repeat() {


I don't think we need both tests now. As far as I can see showRepeatsPickerWhenFirstRepeatIsEmpty() enforces the correct code on its own.

Definitely, I only put the earlier one back as a check.

collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java

seadowg

I'm going to do a little work to rewrite a test file here in a newer style, but this is ready for QA I think! I think testing repeats in general, especially with relevance would be good in addition to testing the issue at hand.

dimwight · 2021-10-04T15:33:06Z

Excellent, thanks @seadowg for your help!

kkrawczyk123 · 2021-10-06T13:56:41Z

Tested with success!
Tested on Androids: 5.1 and 11.

Verified cases:

confirmed that the original issue has been fixed (both forms attached in issue)
regression checks on other forms with repeats: add, remove group, jump to hierarchy, nested repeats, edit group, screen rotation

mmarciniak90 · 2021-10-06T13:59:05Z

Successfully tested on Android versions: 8.1, 9.0, 10.0

lognaturel · 2021-10-06T17:46:21Z

Thank you for taking the time to work through all that, @dimwight and @seadowg! That is a very nice improvement.

Resolves getodk#4570?

758c601

dimwight mentioned this pull request Aug 17, 2021

Proposal for #4059 First nested repeat not created… #4797

Closed

dimwight mentioned this pull request Aug 27, 2021

Resolves #4500 autocomplete minimal… #4712

Closed

seadowg closed this Sep 3, 2021

seadowg reopened this Sep 3, 2021

seadowg marked this pull request as ready for review September 3, 2021 09:04

seadowg self-requested a review September 3, 2021 09:04

dimwight closed this Sep 3, 2021

seadowg reopened this Sep 3, 2021

seadowg added the blocked label Sep 6, 2021

Update for logic overview

9db55a9

dimwight mentioned this pull request Sep 21, 2021

Repeat is not shown in hierarchy view if first instance has no relevant content #4570

Closed

Scenarios4570

3fceab0

seadowg requested changes Sep 27, 2021

View reviewed changes

dimwight added 2 commits September 27, 2021 15:46

Removed debug code

ab0deae

Updated test with simpler form

9f3bda4

seadowg removed the blocked label Sep 27, 2021

dimwight requested a review from seadowg September 27, 2021 19:54

seadowg requested changes Sep 28, 2021

View reviewed changes

dimwight added 2 commits October 1, 2021 07:46

Reinstated old test

89b5566

Simplified logic

9e898d5

dimwight requested a review from seadowg October 4, 2021 09:51

seadowg requested changes Oct 4, 2021

View reviewed changes

dimwight added 2 commits October 4, 2021 14:43

Removed redundant test

b1ca097

Tidied up, explanatory comment

01b8e7f

dimwight requested a review from seadowg October 4, 2021 14:03

seadowg approved these changes Oct 4, 2021

View reviewed changes

seadowg added the needs testing label Oct 4, 2021

Remove deprecations from test

cc4e1c9

seadowg force-pushed the 4570-PR branch from cd9389d to cc4e1c9 Compare October 4, 2021 16:03

lognaturel changed the title ~~Resolves #4570 Repeat is not shown in hierarchy view…~~ Show repeat in hierarchy view even if first instance has no relevant content Oct 6, 2021

mmarciniak90 added behavior verified and removed needs testing labels Oct 6, 2021

seadowg merged commit 6977acd into getodk:master Oct 6, 2021

dimwight deleted the 4570-PR branch July 8, 2023 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show repeat in hierarchy view even if first instance has no relevant content #4762

Show repeat in hierarchy view even if first instance has no relevant content #4762

dimwight commented Jul 30, 2021 •

edited by seadowg

Loading

dimwight commented Jul 30, 2021

dimwight commented Sep 3, 2021 •

edited

Loading

seadowg commented Sep 3, 2021

dimwight commented Sep 3, 2021 •

edited

Loading

seadowg commented Sep 6, 2021

dimwight commented Sep 6, 2021

seadowg commented Sep 6, 2021 •

edited

Loading

dimwight commented Sep 12, 2021

dimwight commented Sep 12, 2021

seadowg commented Sep 22, 2021

dimwight commented Sep 23, 2021

dimwight commented Sep 24, 2021

seadowg left a comment •

edited

Loading

dimwight commented Sep 27, 2021

dimwight commented Sep 27, 2021

seadowg left a comment

seadowg Sep 28, 2021

dimwight Oct 1, 2021

seadowg commented Oct 4, 2021

seadowg Oct 4, 2021

dimwight Oct 4, 2021

seadowg left a comment •

edited

Loading

dimwight commented Oct 4, 2021

kkrawczyk123 commented Oct 6, 2021

mmarciniak90 commented Oct 6, 2021

lognaturel commented Oct 6, 2021

Show repeat in hierarchy view even if first instance has no relevant content #4762

Show repeat in hierarchy view even if first instance has no relevant content #4762

Conversation

dimwight commented Jul 30, 2021 • edited by seadowg Loading

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

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:

dimwight commented Jul 30, 2021

dimwight commented Sep 3, 2021 • edited Loading

seadowg commented Sep 3, 2021

dimwight commented Sep 3, 2021 • edited Loading

seadowg commented Sep 6, 2021

dimwight commented Sep 6, 2021

seadowg commented Sep 6, 2021 • edited Loading

dimwight commented Sep 12, 2021

dimwight commented Sep 12, 2021

seadowg commented Sep 22, 2021

dimwight commented Sep 23, 2021

Flag testWithBeans

Flag restoreOldLogic

dimwight commented Sep 24, 2021

seadowg left a comment • edited Loading

Choose a reason for hiding this comment

dimwight commented Sep 27, 2021

dimwight commented Sep 27, 2021

seadowg left a comment

Choose a reason for hiding this comment

seadowg Sep 28, 2021

Choose a reason for hiding this comment

dimwight Oct 1, 2021

Choose a reason for hiding this comment

seadowg commented Oct 4, 2021

seadowg Oct 4, 2021

Choose a reason for hiding this comment

dimwight Oct 4, 2021

Choose a reason for hiding this comment

seadowg left a comment • edited Loading

Choose a reason for hiding this comment

dimwight commented Oct 4, 2021

kkrawczyk123 commented Oct 6, 2021

mmarciniak90 commented Oct 6, 2021

lognaturel commented Oct 6, 2021

dimwight commented Jul 30, 2021 •

edited by seadowg

Loading

dimwight commented Sep 3, 2021 •

edited

Loading

dimwight commented Sep 3, 2021 •

edited

Loading

seadowg commented Sep 6, 2021 •

edited

Loading

Flag `testWithBeans`

Flag `restoreOldLogic`

seadowg left a comment •

edited

Loading

seadowg left a comment •

edited

Loading