Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest enketo-core #6345

Closed
35 tasks done
garethbowen opened this issue Apr 7, 2020 · 111 comments · Fixed by #7256
Closed
35 tasks done

Update to latest enketo-core #6345

garethbowen opened this issue Apr 7, 2020 · 111 comments · Fixed by #7256
Assignees
Labels
Breaking change Has the potential to break a deployment when upgrading Priority: 2 - Medium Normal priority Type: Technical issue Improve something that users won't notice
Milestone

Comments

@garethbowen
Copy link
Member

garethbowen commented Apr 7, 2020

The next time we bump our dependency on enketo-core we will have to switch api to depend on enketo-transformer instead of the archived enketo-xslt. The transformer depends on the version of libxslt which works on node 12 so we could consider swapping to use that instead of the native xsltproc process. Additionally we may be able to use the transformer for more of the work that we do ourselves in the generate-xform service in API.

Summary of AT Issues:

  • 1. Default config. Form validation error needs to be specific when a phone number has been taken/used
  • 2. Default config. Client-side phone validation does not 'complain' where some characters are added to a 'valid' phone number
    • Existing behavior, not caused by Enketo uplift (comment)
    • Seems like this might actually be the desired behavior
  • 3. Default config. Custom place name label not showing on create/edit place (this is also the same on master)
  • 4. Default config. When adding/editing a person's DOB, if you check Date of birth unknown and then do not enter a value for Months then the value stored for the person's date_of_birth is an "Invalid Date"
    • Behavior change triggered by upgrading our version of openrosa-xpath-evaluator (comment)
    • We inadvertently fixed Empty integer values in Enketo forms evaluate to 0 #7222
    • Proposal is to use a patch to revert behavior change for now (and wait for a major version bump)
    • Decided to keep behavior. Have updated the configs (including the person add/edit forms) to properly handle the behavior change. (comment)
  • 5. Default config. Date picker pop up does not close when clicking outside of it. Only when pressing tab key.
    • Behavior changed. Now clicking anywhere within the date question's div will open the widget (comment). Need to decide if this is acceptable behavior...
  • 6. Default config. Also, date picker is already open when entering the Pregnancy registration form, when selecting Last menstrual period (LMP) or Expected date of delivery (EDD)
    • More generally, questions that are not part of a field-list group are not being automatically selected when the page is opened
    • Caused by a code change in enketo-core to deliberately focus on the first element on the page (comment)
    • I have updated the android-widget to prevent this from triggering text input (and on-screen keyboard) when a date question is focused on (the widget, itself, is not automatically opened by the focus).
  • 7. Default config. In the Pregnancy registration form, ANC visits at Health Facility (past) input, if you type a big number, the page completely freeze)
    • Existing behavior, not caused by Enketo uplift
    • The big number triggers a bunch of entries in the repeat so technically this is expected behavior that we probably do not need to worry about (comment)
  • 8. Default config. In the Gestational Age section and and ANC Visit at Health Facility (Scheduled) section, from the Pregnancy Registration, all the information presented is in bold.
    • Layout changed due to a change in the css. The css has been updated so that notes are not bold. (comment)
  • 9. Default config. When completing a delivery, the child registered during birth is not created.
    • Caused by behavior chance in Enketo form validation that caused inputs to be cleared. Fixed by updating our logic for making inputs always relevant. (comment)
  • 10. Default config. Date label not showing on ANC Visits at Health Facility (Scheduled) / Pregnancy registration
    • Existing behavior, not caused by Enketo uplift
    • This "works as expected" since the date field on the form in marked with NO_LABEL (comment)
  • 11. Default config. When a Pregnancy registration is submitted, and there are more than 4 ANC Visits registered, the report is is only showing the correct labels for the first four visits
    • Existing behavior, not caused by Enketo uplift
    • This "works as expected" since there are only translations for the first 4 past ANC visits (comment)
  • 12. Default config. On enketo branch with default config pregnancy_home_visit form throwing an error when trying to open
    • Initial error loading the form caused by config code in the form that is no longer supported. I updated the form to properly pass one parameter to the decimal-date-time function.
    • Additionally there were issues loading the external contact-summary data into the form which required code changes (comment)
  • 13. Default config. When a Health facility ANC reminder is created, the report is showing an Invalid Date value for the Pregnancy visit task date label.
    • This appears to be fixed now as I cannot recreate it. I expect the issue was caused by the problems loading the contact-summary that were fixed in #12 (comment)
  • 14. Default config. When completing a delivery, the child death registered during birth is not created.(related to #9)
    • This appears to be fixed now as I cannot recreate it. I expect the issue was fixed by changes related to #9.
  • 15. Standard config. After completing the child_health_registration form, on the Area detail view, the report generated is displayed showing an id instead of the child's name.
    • Caused by an issue in the code for making the inputs group relevant. Fixed now. (comment)
  • 16. Standard config. Adding to #15, the report generated for the Child Health Registration is not showing the correct information .
    • Caused by same issue with inputs group as #15. Fixed by same changes.
  • 17. Standard config. After creating an Immunization Visit the report say that Baby did not attend their immunizations visit, and the Vaccines that I chosen (Hepatitis A and B) are marked as No.
    • Caused by change in Enketo behavior for non-relevant fields with default behaviors. Refactored form logic to fix behavior. (comment)
  • 18. Standard config. Nutrition Treatment Exit report is showing long labels for the collected information. For example, report.nutrition_exit.patient_name instead of Name. This is also the same on master.
    • Existing behavior not caused by Enketo uplift. Similar to #11, the "long labels" are shown for this form because there is no provided translation.
  • 19 Standard config. Child Nutrition Screening is presenting #15, #16, and #18. The difference is that this is happening also in master.
  • 20 Standard config. Pregnancy Form displays a user icon and warning icon.
    • Caused by a change somewhere in the processing of embedded HTML where invalid whitespace is no longer supported. (comment)
    • I have updated the logic for parsing the embedded HTML to clear out any invalid whitespace
  • 21 Standard config. Pregnancy Form doesn't display summary information on submit step.
    • Caused by same issue as #21. Fixed by same solution.
  • 22 Standard config. The report generated by the pregnancy form do not match the correct person.
    • This seems to be working now since. I expect it was caused by the same issue as #9 and/or #15.
  • 23 Standard config. Adding to #22. The report generated by the delivery form do not match the correct person.
    • This seems to be working now since. I expect it was caused by the same issue as #9 and/or #15.
  • 24 Standard config. The analytics related to the delivery are not updated.
    • This seems to be working now since. I expect it was caused by the same issue as #15.
  • 25 Standard config. After submitting nutrition_followup form, several things break.
    • Caused by same issue as #17 where invalid data was being saved for forms. Fixed by updating the nutrition_followup form. (contact)
  • 26 Standard config. Postnatal Care Visit and Pregnancy Visit reports are not showing Danger Signs.
    • Caused by same issue as #17 where invalid data was being saved for forms. Fixed by updating the forms.
  • 27 Standard config. First report in the list has to be deleted twice to actually be deleted. This is also the same on master.
    • Not a real issue, it was a sync problem
  • 28 Muso config. Unable to install config-muso on an instance running 6345-enketo-uplift.
    • I am not able to recreate this issue. It may have been fixed by one of the previous changes, or we may need to see if there is some difference in our setup.
  • 29 Brac config. Creating a CHP Area or a Family, the error message for mobile number format is not displayed. This is present in master and 6345-enketo-uplift.
    • Existing issue, not caused by Enketo uplift. Some form configuration is missing and I have logged a separate issue on the config-brac repo to address it. (comment)
  • 30 Brac config. Unable to edit a person's information.
    • Caused by the custom phone-widget being too broadly applied now. Fixed by making the selector for that widget more precise. (comment)
  • 31 Brac config. Unable to load pregnancy follow up form from task tab.
    • Caused by behavior change in the new Enketo/Openrosa xPath logic to be more stringent with regards to mal-formed expressions.
    • The pregnancy_visit form will need to be updated. I have logged: https://github.com/medic/config-brac/issues/436
  • 32 Brac config. Target tab is not showing any information (one pregnancy was registered).
    • Caused (mostly?) by a change to the format-date function behavior. When the pregnancy form submits with valid data, this problem does not present. (comment)
    • We still need to land on a long-term fix for this. Waiting on 'Invalid Date' returned by format-date function when given an empty value enketo/enketo#864
      • In the meantime I have added a custom format-date implementation to the branch that reverts to the old behavior. Going forwards we can consider keeping these changes or reverting them in favor of something else.
    • Our change to openrosa was accepted! I have pulled in the latest version of that library so that we should have the original behavior for format-date
  • 33 Brac config. After submitting the pregnancy_referral_follow_up the task was not deleted and it throws an error.
    • I believe this was caused by invalid data created as a result of #32. Once I addressed those issues, this problem no longer presents.
  • 34 Brac config. Household Equity survey task is not being generated when a new family is created.
    • This problem is no longer presenting for me. I am honestly not sure which changes may have fixed it (maybe related to #15?), but it seems to be working fine now.
  • 35 Brac config. After creating a FP Visit, in master, the total number of visits was increment to 1, but in enketo branch the total number remained in 0.
    • As with #34,this problem is no longer presenting, but I am not sure exactly why or what was causing it in the first place (also maybe #15?).
@garethbowen garethbowen added Type: Technical issue Improve something that users won't notice Priority: 2 - Medium Normal priority labels Apr 7, 2020
@garethbowen
Copy link
Member Author

Moved to 3.12.0 to free up engineers to work on 3.11.0.

@garethbowen
Copy link
Member Author

This is blocked on updating to 5.9.2 first: #4386

@iesmail-znz
Copy link

@garethbowen, I wanted to know if CHT has the capability to track a CHV's visit length in XLSForm. I checked at the documentation and I found that the the user_edit_time in this link
mentions Enketo, and the XLSForm spec has astart/endproperty (http://xlsform.org/en/#metadata), but due to an Enketo or CHT bug/issue that was not usable in CHT XLSForms.

Would like to know when it will be fixed.

@abbyad
Copy link
Contributor

abbyad commented Oct 5, 2020

Hi @iesmail-znz, the start and end meta fields should be available since 3.7. What problems are you currently seeing trying to use them? If they aren't working as expected it would be worth opening a bug report with details as a new issue.

On a related note, I see documentation on those fields in our old (and now archived) docs site, but can't find it in the new CHT docs site. I will port that over now.

@MaxDiz
Copy link
Contributor

MaxDiz commented Oct 15, 2020

bumping to release following completion of #4386

@abbyad
Copy link
Contributor

abbyad commented Jan 6, 2021

Adding to the App Builders backlog too since this is needed to use supported ODK XPath functions in CHT apps.

Edit: added link to Forum post.

@MaxDiz
Copy link
Contributor

MaxDiz commented Jan 21, 2021

@garethbowen a couple of follow-up questions from the roadmap planning meeting:

  • Is there a QA plan to test forms?
  • Based on the QA plan, would it make more sense to upgrade to enketo latest in a single release (ie combine with improvements scheduled in 3.12 - Update enketo-core to 5.9.2 #4386 - the original plan, I believe)?

@garethbowen
Copy link
Member Author

Many forms are tested as part of the standard release test plan. It would be very useful to test a wider range of real world forms as well and that plan should be developed when this issue is being QAed. If time permits this test plan should be automated because we usually update enketo-core every release and we need to catch regressions early.

The reason there are two very similar looking issues is we've made good progress towards the other one, so it should be relatively easy from a development perspective to finish that off ready for AT. This issue would then be a separate body of work which may be trivial or highly difficult depending on what breaking changes exist in the later versions of enketo-core. Obviously the two issues can only be developed one at a time but they could be ATed together to save time. In short I'm happy for this issue to be scheduled in 3.12 to reduce testing effort, but we should keep the issues separate so we can ship the other one, and postpone this one if it turns out to be difficult.

@MaxDiz
Copy link
Contributor

MaxDiz commented Jan 25, 2021

ok, moving to v3.12 to align with #4386

@tatilepizs
Copy link
Contributor

tatilepizs commented Jun 23, 2022

Brac config - fp_follow_up_long_term

There is a section in the fp_follow_up_long_term form with the question: Did you receive the long term method?* (Q1) and depending on the answer (Yes/No) a different question will be displayed, and this new question (Q2) will also display messages or new questions depending on the answer. The problem is that if I select No from Q1 and then select several options from Q2 different messages will be displayed, and if after that I changed my mind and select Yes from Q1 the messages that were displayed will remain in there, and if I change my mind again, and select No again from Q1, all the options that I selected the first time will be marked, all the options that I selected before will be there, the answers are not clean.
This is not happening in master, no matter how many times I change the answer from Q1 the options that were selected and the messages will be clean.

This description might be a little confusing, I think that a video can help to clarify.

Video - 6345-enketo-uplift
enketoUplift.mp4
Video - master
master.mp4

_______________________________________________________

Other forms having the same problem in one of its questions:

fp_follow_up_prospective
fp_follow_up_prospective.-.enketoUplift.mp4
fp_follow_up_refill
fp_follow_up_refill.-.enketoUplift.mp4
fp_follow_up_short_term
fp_follow_up_short_term.-.enketoUplift.mp4

@jkuester
Copy link
Contributor

jkuester commented Jun 24, 2022

Okay, I think I have figured out what is going on with the person age constraints that are failing improperly for several forms. These forms try to ensure they are only applied to patients of a specific age range, calculated based on the patient's date_of_birth value. The problem is with the way our custom db-object-widget (for selecting the patient) interacts with Enketo. When the user selects a patient with the db-object widget, the patient's _id is immediately set as the value of the db-object field in the Enekto data model and then a lookup is triggered to go load the rest of the patient's data from the db. Setting the _id triggers Enketo to immediately run the validation on that field. For these particular forms, the constraint check for _id is based on the value for the date_of_birth field. However, the date_of_birth field in the Enketo data model is not populated right away because we are still looking it up from the db (and so the constraint initially fails). Once the db request returns the patient's doc, the date_of_birth field is populated in the Enketo model. And a "change" event is triggered for the db-object field. So far everything has gone just like it it works in master, but this is where things go off the rails.

Triggering that "change" event in master used to cause Enekto to re-validate the constraint. However, in the new version of Enekto, a check has been added so that when a "change" event is triggered for a field we only re-validate the field if its value actually updated. This, in general, makes good sense. However, in this case it causes the constraint error to continue to be displayed when it should not. This is because even though the "change" event is triggered, the actual _id value for the db-object field did not change (even though new data was added to the model for the patient's other fields). So, the constraint is not re-validated.

That is what changed since the old version of Enekto to break this functionality, but technically, I think the real issue here is that the constraint is not automatically re-validated when the value for date_of_birth is set in the model (since the constraint expression depends on the date_of_birth value). I have logged this as an issue with Enketo: enketo/enketo#62

In the meantime, though, I have updated the db-object-widget to manually trigger a re-validation of the field's constraint once the doc data is loaded. This should resolve the issue and return the behavior to how things work in master.

@jkuester
Copy link
Contributor

jkuester commented Jun 27, 2022

Regarding the issue where answers that become non-relevant are not cleared, this could be serious challenge. Basically it is connected to the same behavior change we found with an earlier inputs issue. The TLDR is that Enketo removed support for the clearIrrelevantImmediately option that we were using. This means that while the form, is being filled out, if a question becomes non-relevant (like in the described examples) its current value will not be cleared out. Instead it will remember what the answer is and will continue to return that answer when referenced in caluculates and other logic (such as relevant logic for notes as in the examples). The good (maybe?) news is that in the end, when the form is submitted, the non-relevant values will be cleared and not saved with the report. This confusing inconsistancy was called out in this Enketo issue and has been fixed in a new version of enekto-core (6.1.0).

Since this is all tied up in the interactions between form config and deep Enekto functionality, there is not much we can do to mitigate this issue. I think we need to decide if the problem is serious enough that we should either roll our "new" Enketo version back to 5.15.3 or push it forwards to 6.1.0 (currently the PR is uplifting the CHT to 5.18.1).

@tatilepizs
Copy link
Contributor

Thank you for the details @jkuester.

The problem that I see is that if you make a mistake and select the wrong option and the new question displayed is a required one you will not be able to continue with the next part of the form because you can't "unselect" a radio button.

Medic.Mobile.-.27.June.2022.mp4

@garethbowen
Copy link
Member Author

As discussed, but documenting here for anyone else watching. My preference is to not change the version of enketo-core (either to 5.15.3 or 6.1.0) because that would trigger another round of extensive testing and potentially find new issues.

Let's leave this PR as is so that we can lock in the gains we've made, and raise a new issue covering this specific bug. If a solution is found before 4.0.0 is released then it can be included too, but otherwise we can document this new bug as a "known issue" and solve in a minor release in the near future.

@tatilepizs
Copy link
Contributor

Muso config - pregnancy_family_planning

When a pregnancy is registered using the pregnancy_family_planning form, the delivery information is not shown.

image
image

@tatilepizs
Copy link
Contributor

tatilepizs commented Jun 29, 2022

Muso Config - Several forms

In the first page of the form, there is a required input for the patient's name, and no matter what value I select it is throwing an error saying: This field is required, I am not able to continue with the next page.

NOTE: This is only happening if I access the report through the "History" tab.

Another important thing is that, in master, if you go back to select another patient, it starts to show the same behavior that is in the new enketo (6345-enketo-uplift). At least the new enketo is more consistent, because it is failing every time, maybe it is doing a better validation from the beginning 🤷‍♀️

supervision_calendar
  • 6345-enketo-uplift
6345-enketo-uplift.mp4
  • master
master.mp4
supervision_with_chw_iccm
  • 6345-enketo-uplift
6345-enketo-uplift.mov
  • master
master.mov
supervision_with_chw_proccm
  • 6345-enketo-uplift
6345-enketo-uplift.mov
  • master
master.mov

@tatilepizs
Copy link
Contributor

tatilepizs commented Jul 1, 2022

Muso config - Some forms are not able to load.

Details of the user used to login:

...
"type": "person",
"is_active": "true",
"name": "Pumba",
...
"parent": {
  "_id": "ea2134e6-fe48-4aa1-be4a-b2421cb8fb90", -> "contact_type": "c40_chw_area"
  "parent": {
    "_id": "062a63ed-bc82-4fd7-9b28-495aeb09e05d", -> "contact_type": "c30_supervisor_area"
    "parent": {
      "_id": "2bec5f7a-5549-493a-8102-a2fd7fb2035c", -> "contact_type": "c20_health_area"
      "parent": {
        "_id": "3fa4a4fc-e492-470d-a98c-8f6281ffb834" -> "contact_type": "c10_site"
      }
    }
  }
},

Failed forms:

tb_followup_daily

image

tb_followup_weekly

image

tb_test_result_fp

image

treatment_followup_over_5

image

treatment_followup

image

edit contact

image

@jkuester
Copy link
Contributor

Regarding the issues with the Brac assessment_follow_up form, there are a could of things going on here. First of all, the issue with the constraint violation error message still being displayed (as seen in the video) is fixed by the new changes I recently made to the db-object widget.

Then, the problem with not being able to advance to the next page after selecting the answer to How are you following up? is caused because there is a hidden question on the page that is required. That question's value is set by input data that should be provided when by the task. Since, in this case, the form is being manually created, no data is provided from the task and the form cannot advance. This behavior is the same as in master and I think is acceptable (given how this form is supposed to be used...)

@jkuester
Copy link
Contributor

Regarding the Muso pregnancy_family_planning form display issues, this is actually caused by a relatively serious OpenRosa issue with the decimal-date-time function. I have logged enketo/enketo#862 and submitted enketo/openrosa-xpath-evaluator#162 to address this.

@jkuester
Copy link
Contributor

jkuester commented Jul 13, 2022

Regarding the issue with the Muso supervision forms where you could never successfully choose two persons, this turned out to be a bug in the db-object-widget where it could not properly handle multiple db-objects in nested groups (at least in some cases). That bug should be fixed by these changes.

@jkuester
Copy link
Contributor

Regarding the issues in the Muso notification_to_chw form trying to select a "Health Center", I cannot recreate the issue. Perhaps it was caused by not having any health_center contacts that were available to the user? Also, it may have been resolved by one of the recent changes to the db-object-widget.

@jkuester
Copy link
Contributor

Okay, I think I have finally figured out what was going on with the Muso contact edit form issue.

The db-object-widget was manually changing its input element to be a select element. However, Enketo had already cached the reference to the input element and this manual change broke that reference. So, when you have a form like Muso's edit contact form, that sets the value of a db-object input using a calculation, it causes an error because the Enekto framework does not have a working reference when it tries to set the calculation value.

This particular situation was made more complicated because, in certain circumstances the Muso edit contact form would load without error. @tatilepizs noticed that the form worked when the contact being edited was created by the user. It turns out this behavior was caused because the db-object value in the form could be set in a couple of different ways (due to some weird circular configuration in the form). Its value would initially come from the current user's id. Then it would later be set to the person_id on the contact being edited. Now, if the contact was one that was created by the user, then the person_id would be the user's id and Enketo would not try and set the value a second time (since it was already set to the user's id). However, if the contact was not created by the user, the person_id would be different and Enketo would try to set the db-object column a second time. This time, though, the db-object-widget will already have been initialized and the reference broken. So an error will result.

Regardless, I have been able to clearly recreate the error in the interaction between the db-object and the calculation with a very simple app form, so I don't think too much of the complexity from the Muso form is actually related to this issue. The following form will trigger an error in the Enekto branch, but not in master:

type name label::en appearance bind:jr:constraintMsg calculation
calculate my_calc       string('bd3fdfa2-caf9-4eac-88b2-09fa739f03ac')
db:person _id What is patients name db-object   ${my_calc}

I have had to do a pretty significant refactor of the db-object-widget to fix this situation. But in the end, this should be resolved.

@tatilepizs
Copy link
Contributor

tatilepizs commented Jul 21, 2022

🎉 Really good news 🎉

All the issues that were raised were retested and all of them are solved, some completely fixed, some with an external ticket created, and one (an unusual scenario) was marked as a known issue. All the details are in this comment

QA team is done with the testing and this uplift is good to go from QA perspective 🎉

Huge thank you to @jkuester for all the help, support and patience during the process!! 🎉 🎉

@jkuester
Copy link
Contributor

jkuester commented Aug 1, 2022

Following up again on the Muso pregancy_familiy_planning form display issues, the OpenRosa PR was not accepted (for spec-compliance reasons). Instead, this case will require a change to the form (unless we want to add a custom implementation of the function to the CHT).

To give some more details here, the form is currently calculating a value based off the result of a decimal-date-time call where the parameter passed to the function is an explicitly formatted date string (using format-date-time), but the format is not ISO 8601. One way to fix the issue is to pass the formatted date string to the date function before passing to decimal-date-time. Another possibility is that the form logic might be able to be simplified to not use the decimal-date-time function at all by leveraging the new XPath function we are adding in #7694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change Has the potential to break a deployment when upgrading Priority: 2 - Medium Normal priority Type: Technical issue Improve something that users won't notice
Projects
Status: Done
Archived in project
Development

Successfully merging a pull request may close this issue.