Update the model even when a cached choice list can be used #646
Conversation
| @@ -131,6 +131,7 @@ public void clearingValueAtLevel1_ShouldClearValuesAtLevels2And3() { | |||
| // If we set level2 to "aa", form validation passes. Currently, clearing a choice only updates filter expressions | |||
| // that directly depend on it. With this form, we could force clearing the third level when the first level is cleared | |||
| // by making the level3 filter expression in the form definition reference level1 AND level2. | |||
| scenario.answer("/data/level1", "b"); | |||
There was a problem hiding this comment.
A side-effect of this change is that it's no longer possible to set a value for the second level of a cascade if the first level isn't set. That's a good thing but in practice, I don't think it will make any difference for users since they'd have to visit the first level in order to change its value. I don't think Collect will let you end up in a state where level1 is not set and a value can be selected for level2.
|
@seadowg I'm not sure whether you'll want to review or wait until @grzesiek2010 gets back. I think it'd be good to get to QA sooner than later. Hopefully it will be an easy review. |
There was a problem hiding this comment.
Both solutions I've seen of this make sense to me, but I take @lognaturel's point that this preserves the performance benefits of caching for multiple different selects that use the same list. This is a pretty small change and the failing test (it's very specific, but I think it has to be) demonstrates this fixing the issue as far as I can see. I think it'd be good to get this to QA now!
|
I've tested this with Collect and it looks like there is a failing test - I also think we should have a quick chat about debugging JavaRosa from Collect as that process is now undocumented. |
|
After this change, the first two cases described by getodk/collect#4750 are no longer problems because the answer gets updated earlier. That means if there's a calculate that produces a choice that exists in the choice list, it will be bound appropriately. If there's a calculate that produces a choice not in the choice list, then it will be cleared. I think all that is ok, it just makes it more pressing to address XLSForm/pyxform#546 so users get feedback when they make the mistake of putting a calculate on a user-editable select. I believe we could still get a test to verify what the now-failing test did with something like getodk/collect#4170. For now, let's get a PR up for QA and I can work on that test.
I've always used the jar option from the instructions and that seems to work fine. Do you mean that the sourcetree option doesn't work? I've never used it because I get confused having multiple copies of the source. Perhaps we could just remove that part of the documentation? |
No, just the jar route. To get it all working now you need to add dependencies to Collect (
Yeah I think we should. |
After a bit of investigation it looks like we could set it up, so you can just do |
|
@seadowg I think this just needs your approval so I can merge and update Collect! |
Closes #642, replaces #643
#643 re-introduced performance issues. See #643 (comment) for a description of when performance would matter.
This solution is similar to 7d483f1 but with a narrower slice of work when cached choices are returned.
What has been done to verify that this works as intended?
Added and ran a test. Verified that all other tests pass, in particular those that ensure performance in cases with repeats (the ones that failed in #643).
Why is this the best possible solution? Were any other approaches considered?
Conceptually, there are two things we need to do for any select:
We don't want to do this on e.g. form load because it would mean having to iterate through all of the choices which we may never need to do. Doing this binding as late as possible is good for performance.
Semantically it feels like making sure the answer is synced with the choice list should happen any time the answer is requested. However, I don't think that's practical because the answers are represented as
SelectOneDataandSelectMultipleDataobjects which have no connection to the form definition or the currently filtered list of choices. I can't think of a way to kick off that syncing. What we've been doing which is updating the answer when the choice list is requested seems like the next best thing.@grzesiek2010 says at #643 (comment)
True, but no matter what I think we need to couple this update with some user-facing behavior. We don't want clients of
getChoices(or whatever other useful user-facing behavior we’d tie the model update to) to have to explicitly do the model update, we want all the right updates to be triggered by choices being requested. We could do some kind of hook or listener but fundamentally I think the result would be the same and it's not worth a huge refactor at this time.I briefly explored doing the update when the answer label is requested (currently in
getSelectItemText). But that's not quite right, there are cases such as in validation where the choice list would be requested but the answer label would not.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?
The intent is to narrowly fix the issue with editing forms with filtered selects in repeats. This feels like a quite safe change. It does mean that select answers are now updated any time a choice list is requested so there's risk there. We have very high test coverage, though.
There's a little bit more work done whenever returning a cached list but it should be very fast.
Do we need any specific form for testing your changes? If so, please attach one.
See issue.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.