#45 repeat-groups are not respecting the jr:count setting after a change to a lower value #61
Conversation
The problem description: Repeat groups that are controlled by the jr:count attribute which is a path to a node (a number question) that holds the actual value do not respect situation where the value is *decreased*. Example: User starts a form and puts 6 as the answer for the question that controls the jr:count of the repeat group. As (s)he is going forward, the repeat groups are properly created by FormEntryModel. The user goes back to the number question and changes it to 3. As (s)he is going forward, the new value is ignored by FormEntryModel and the number of repeat groups is still 6. Solution: The most of the magic takes place in FormEntryModel#createModelIfNecessary(FormIndex) method which takes care of creating the missing repeat groups by comparing the count value with the currently being processed group instance index. If the index is lower than the count value and the group does not exist, the method creates it. Now the method also checks if the instance index is *higher* than the count value. If so, it deletes the currently processed group and all the following (if any) in a loop.
If there is at least one repeat group to be deleted during updating a model, user will be taken to the first form index after the deleted repeat group(s). This happens after FormEntryController already skipped non-relevant indices so the skip must be repeated if the the first index after the deleted repeat group(s) is non-relevant (hidden).
* Changed FormEntryModel#createModelIfNecessary to FormEntryModel#updateModelIfNecessary. Now this methods creates and removes repeat groups so the new name makes more sense to me. * Changed FormEntryModel#updateModelIfNecessary body so it now uses Guard Clauses instead of Nested Conditions. The execution path should be cleaner now.
mdudzinski
changed the title
|
Thank you @mdudzinski! Will try to review soon. I want to make sure I get a chance to think through implications. In the mean time, other comments and reviews are welcome! |
|
@mdudzinski This has not been forgotten, I promise. As you have seen on the forum, there are a lot of exciting things happening all at once in the ODK community. I want to make sure we are thoughtful in assessing the impact of this change. |
|
@lognaturel no worries, I understand. This is being tested on our side so we'll let you know the results here. In the meantime, I'll appreciate any technical input because I still have a feeling that I missed something in the implementation:) |
|
@lognaturel Just to let you know that this change has been released with the latest TaroWorks version. It's been working fine so far but TaroWorks uses only a subset of Collect/JavaRosa XForm capabilities and therefore it would be nice to have another pair of eyes to look at this. I'll hold on with fixing the conflict till it's decided whether this approach is really wanted. |
|
Thank you for that update, @mdudzinski! Did you end up including some kind of notification or confirmation for the user? Since this is a change in existing behavior, I have mild concerns about data loss. It should probably be clear to a user when they change the value that they could be losing records. |
|
@lognaturel Nope. TW didn't have a requirement to inform the users about potential data loss. But I can imagine that some Collect's could be surprised. |
|
Heads up @mdudzinski, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@mdudzinski it contains:
If I select yes then I pass 3 for example I can see 3 groups. ✅ The workeround here is to add: My question is: isn't it something you should/could fix as a part of this pr too? |
|
@grzesiek2010 Hmm this is a quite different scenario. When |
|
Agreed that is pretty different although it's true that the effect feels similar to users. @mdudzinski is right that I'm concerned about accidental data loss. That's why this PR hasn't been merged -- the behavior has previously been considered a feature not a bug. Search for "repeat-groups are not respecting the jr:count setting after a change" on the page https://opendatakit.org/help/form-design/workarounds/. You'll see that the recommendation is to use relevance to hide any extra values. This works really well and means that if someone had entered 10, wanted to change the value to 12 but accidentally entered 2 they wouldn't lose a bunch of records. |
|
This continues to be considered intended behavior. |
Fixes #45.
I hope commits messages, code and comments are comprehensive but here goes a short summary.
The code responsible for actual deletion of the extraneous repeats is simple:
but after the deletion,
indexpoints to the non-existing repeat group which leads to an error shown on UI (ODK Collect) . To fix this, the index must be incremented after the deletion and before it's assigned tocurrentFormIndex. This leads to another problem - a non-relevant (hidden) form element may be present right after the deleted repeat group. Skipping over such indices takes place before updating the model so it must be repeated. It's achieved by checking the current index afterjumpToIndex(index)and recursively callingstepEvent(forward)if needed.