fix(#8225): fix handling of inputs group with contact selector
#9382
Conversation
| @@ -247,7 +247,7 @@ describe('Contact Delivery Form', () => { | |||
| source_id: '', | |||
| user: { | |||
| contact_id: offlineUser.contact._id, | |||
| name: offlineUser.username, | |||
| name: offlineUser.contact.name, | |||
There was a problem hiding this comment.
This change actually demonstrates this fix in action! The regression scenario from the forum was doing the exact same thing as the delivery form where they are trying to load the user's contact document directly into the inputs/user group. The complication is that the inputs/user group already contains data from the users user-settings doc. So, when you use the contact selector to load the user's contact into the same group, it will override the data that is already there.
In this case, both the user's contact and the user-settings document contain a name field. For the user-settings it is the username. For the contact, it is just the normal contact name value. Because the contact-selector functionality in non-relevant inputs was broken before this PR, the user's contact data was not actually being loaded and so the name was just populated with the username from the user-settings document. Now that this PR has fixed the contact selector functionality, we see inputs/user/name getting overridden with the value from the user's contact (which is the proper behavior that we expect).
| - if ( | ||
| + // CHT-CORE PATCH | ||
| + // /inputs is ALWAYS relevant #4875 | ||
| + if(!result && /^\/[^/]+\/inputs$/.test(path)) { | ||
| + branchChange = this.enable(branchNode, path, options) || branchChange; | ||
| + branchNode.classList.add('disabled'); | ||
| + } | ||
| + else if ( |
There was a problem hiding this comment.
The previous patch here was attempting a much more surgical modification where the inputs group would actually still get set to be non-relevant, but we would not clear the data in the inputs group (as a part of this logic. It would remain available to other expressions in the form. Unfortunately, the side effect of this was that if any of the data in the non-relevant inputs changed, that change did not get stored/propagated to other expressions since technically the fields that changed were non-relevant. So, when the db-object widget was triggered inside the non-relevant inputs group, it would load the data as expected, but it would not get saved into the form data model.
This new patch takes a more heavy handed approach where we actually just never let the inputs group get set to be non-relevant (by hijacking this logic flow). The hope is that this will let us avoid any other kinds of weird side effects at the data-model level (especially since Enekto has some additional logic that gets trippy when some fields have never been relevant). Basically this lets is side-step all of that and as far as the rest of the form is concerned the inputs group is always relevant. The downside is that when the inputs relevant expression evaluates to false, _we do not want the user to actually see the inputs fields in the form. So, that is why we still slap the disabled class onto the inputs group to make sure it is not shown in the form.
|
Given this has been approved - should we move this to full PR instead of a Draft? |
|
@mrjones-plip Done! |
There was a problem hiding this comment.
Thank you for all the details @jkuester ⭐
I manually tested and created a pregnancy with danger signs that triggered (all as tasks):
- Pregnancy danger sign follow-up
- Health facility ANC reminder
- Delivery
- Postnatal danger sign follow-up - mother
- Postnatal danger sign follow-up - baby
And everything worked as expected 🚀
|
@tatilepizs @latin-panda could you have one more look through this? I ended up making a change to the patch (to always set
With my original patch in this PR, those non-relevant |
|
Test form file name changes coming soon! 😅 Forgot to post those changes before requesting a review! |
jkuester
changed the title
inputs from ever being non-relevantinputs group with contact selector
Description
Closes #8225
This PR updates our patch for the Eneketo relevant logic to make it so that the
inputsgroup never can get set to non-relevant (and so calculations and widgets and things involvinginputsall work as expected even when theinputsfields are not shown in the form).Testing considerations
I have expanded our db-object widget e2e tests to cover using the widget to load contact data inside the
inputsgroup. Also, the other existing tests all work as expected. The risk here is that the new Enekto patch somehow breaks Enekto code in other unexpected ways. I have done some manual testing to try to shake out any issues, but everything has worked as expected.It would be good, though, to perhaps do more manual testing of any workflows that involve passing
inputsdata to forms (e.g. forms triggered from tasks).Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.