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

6345 - Uplift to enketo-core version 5.18.1 #7256

Merged
merged 201 commits into from
Aug 16, 2022
Merged

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented Aug 16, 2021

Description

The following changes had to be a made as a part of uplifting the enketo-core dependency in webapp to the latest version:

  1. Uplift enketo-core in webapp from 4.41.6 to ^5.18.1
    1. Cleaned up custom bootstrap-datepicker localisations (for bm and hi) since those are now included in the new version of bootstrap-datepicker pulled in by enketo-core.
    2. Remove the enketo-handle-no-active-pages.patch since this is no longer necessary.
    3. Add the enketo-repeat-name-collision.patch to resolve Name collision with calculate in repeat causes error enketo/enketo#99 since we cannot uplift to the latest version of enketo-core that contains the fix.
    4. Add polyfills for 7 functions that are being used in enketo-core, but are not present in Chrome 53 (the latest version supported by xWalk).
  2. Remove enketo-xslt dependency in api and migrate to enketo-transformer
    1. Note that this change did not include utilizing the transform functionality provided by enketo-transformer. That uplift will be make in Uplift to use transform functionality from enketo-transformer #7295 which is blocked until we no longer have to support Node 8/10.
    2. Support for markdown syntax in xforms was moved from a webapp service to instead be integrated in the generate-xform service in api. We should be able to remove this markdown logic in the future when utilizing the transform functionality provided by enketo-transformer.
      1. Note that now Markdown syntax is supported for all question labels, not just notes.
    3. Update custom xsl to avoid marking note fields as required since this causes issues when filling out the forms with the new version of Enketo.
  3. Uplift openrosa-xpath-evaluator in webapp from ^1.5.1 to ^2.0.7. Import medic-specific functions from openrosa-xpath-extensions #4517
    1. Removes support for the deprecated feature allowing for concatenation of strings. (Need to make sure this is documented in release notes).
    2. Removed the custom now function from webapp since that function (with an identical implementation) is in openrosa-xpath-evaluator
    3. Migrated the difference-in-months function from the old version of openrosa-xpath-evaluator to be a new custom function in webapp since it was removed from newer versions of openrosa-xpath-evaluator
    4. This uplift gives us access to a number of new xform functions that we can now use in forms. These are listed in a separate section below.
    5. (Interesting note: Enketo Express recently switched to use the openrosa-xpath-evaluator as well (Feb 2021)!)

Notable behavior changes in xforms:

  • note fields cannot be required. (Will now prevent the user from progressing to next page)
    • This was improper configuration to begin with, but previously was ignored.
    • Our custom xsl has been updated to automatically handle this case and make notes not required.
    • Adding validation to cht-conf to provide error when trying to upload a form with required notes: Error when note fields are required in form cht-conf#494
  • The value returned for unanswered number questions (when they are using in calculations or relevant logic) has changed from 0 to NaN. See Empty integer values in Enketo forms evaluate to 0 #7222 for more information.
  • The behavior of calculations involving non-relevant questions with default values has changed. Previously, the default value would be used, but now non-relevant questions always are evaluated as an empty value when the form is saved (due to this Enketo issue, the default value can still be returned while the form is being filled out, but will be cleared when saving (and everything will be recalculated)).
  • Similarly, the behavior of calculations involving answers to non-relevant questions has changed. Previously, if an answered question became non-relevant, its answer would immediately be cleared (and the answer value would not be available to calculations). Now, the answer value is not cleared until the form is submitted.
  • Parameters for concat must be separated by commas (or now the form will fail to load).
  • New xPath functions are now supported (see the list below).
  • New widgets are now supported (see the list below).
  • The horizontal and horizontal-compact appearances are deprecated. The columns functionality should now be used instead.
  • Markdown syntax is now supported for all question labels (and not just notes). This may cause issues in existing question labels that unintentionally are using Markdown syntax.
  • The behavior of calculations involving invalid xPath paths (both absolute and relative) has changed. Previously an invalid xPath path (one that points to a non-existent element) was evaluated in a calculation as being equivalent to an empty string. So, /invalid/xpath/path = '' would evaluate to true. Now, however, that expression will evaluate to false since invalid xPath paths are no longer considered equivalent to empty strings.
  • The format-date and formate-date-time functions no longer accept month values that are <= 0. See this comment for more information, but the TLDR is that this could affect processing for things like birth dates calculated from given values for months and years.
  • The behavior of the decimal-date-time function has been updated to correct an issue with the default timezone used when calculating the decimal value of a date that does not include any time zone information. (Note that the values from basic date questions do NOT include time zone information.)
    • This could potentially cause issues with existing logic in forms due to workarounds that were in-place.
      See this comment for more information.
    • Additionally, it is no longer safe to assume that the output from decimal-date-time, for a basic date question value, will be a whole number since now there will likely be a decimal value included representing the difference between midnight in the user's TZ vs midnight in UTC.
  • The behavior of the decimal-date-time function has changed with regards to what type of parameters it will accept. Previously it would accept any kind of string parameters (e.g. date strings with various formats) and would leverage the JS Date logic to evaluate the values. Now, it will only accept strings containing ISO 8601 date values. See this comment for more information.

Newly available Enketo Widgets:

(A demo form containing these widgets can be found here.)

  • select-media/select-media - Media Picker. Hides text labels if a media label is present.
  • range/range-widget - Choose numeric values from graphical ranges
  • url/url-widget - Text widget that allows for url appearance to give a clickable url displayed
  • text-max/text-max - Hardcodes a maximum character length to text input fields. (Not an actual stand-alone widget)
  • rating/rating - Provide rating (e.g. 5 stars)
  • thousands-sep/thousands-sep - For number input. Will display the number below, separated by thousands.
  • note/notewidget - Formatted notes (replaced our custom note widget)
  • select-likert/likertitem - Select one displaying as a Likert item
  • select-desktop/selectpicker and select-mobile/selectpicker - Select one or more choices from pulldown
  • analog-scale/analog-scalepicker - Similar to the range widget (pick a value along a sliding scale...) but on a fixed scale (e.g. percentage based from 0-100)
  • textarea/textarea - Auto-resizes textarea elements. (Not an actual stand-alone widget)
  • select-autocomplete/autocomplete - Retains the ability to select from a list by typing to filter the list
Newly available xPath functions:

Native functions now implemented directly in openrosa:

  • abs
  • boolean
  • concat
  • count
  • ends-with
  • last
  • namespace-uri
  • normalize-space
  • number
  • position
  • string
  • string-length
  • sum

Custom functions implemented in openrosa

  • acos
  • asin
  • atan
  • atan2
  • area
  • checklist
  • cos
  • count-non-empty
  • decimal-time
  • distance
  • exp
  • exp10
  • log
  • log10
  • once
  • pi
  • randomize
  • sin
  • sqrt
  • tan
  • weighted-checklist

Note that this PR builds off of the changes from the earlier (unmerged) PR #6129.

Closes #6345
Closes #4517
Closes #7298
Closes #7237
Closes #1495
Closes #6803
Closes #7222
Closes #6846
Closes #5636
Closes #6589
Closes #5980
Closes #4132

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@jkuester jkuester force-pushed the 6345-enketo-uplift branch 3 times, most recently from 4ef441d to 0d87d65 Compare August 16, 2021 19:19
@jkuester jkuester force-pushed the 6345-enketo-uplift branch 3 times, most recently from d98b6e5 to f735118 Compare August 27, 2021 21:48
@jkuester jkuester force-pushed the 6345-enketo-uplift branch 4 times, most recently from 3b0195b to 0f17b49 Compare September 7, 2021 19:20
@jkuester jkuester requested a review from dianabarsan July 19, 2022 21:26
Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a couple of questions about the db-object-widget refactor.
Awesome job! I'm so happy to see this finalize!

@@ -75,7 +87,7 @@ const changeHandler = function() {
const doc = selected && selected[0] && selected[0].doc;
if (doc) {
const field = $this.attr('name');
const index = $('[name="' + field + '"]').index(this);
const index = $('select[name="' + field + '"]').index(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we end up having one hidden input and a select with the same name?
Is that required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is where things sit now.

Is that required?

It is not required that they have the same name. Things go bad in the Enketo logic if I change the name on the input (which is to be expected). However, I do not have to set the same name onto the select. I do have to set some kind of name on the select or other weird behavior happens in Enekto (a select with no name confuses the Enekto node resolution logic in ways that I do not fully understand...). But as far as I can tell it works fine to just set any random value as the name on the select. I am currently just copying the name from the input, (and am not really seeing any adverse effects), but let me know if you think it would be better to set a static name for all the db-object selects (or to create some kind of dynamic name for each...).

const $option = $('<option></option>');
const setOptionValue = value => $option.attr('value', value).text(value);
setOptionValue($textInput.val());
$textInput.on('inputupdate', () => setOptionValue($textInput.val()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all intents and purposes, I like this refactor.
Turning an input into a select just makes me nervous ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not meaning to add more work on your plate but, since we're already refactoring this, what is the value of cloning the input and then changing the tag versus just making a new select element?
Is it because there are classes and attributes to copy? Are they actually needed on the select?
This is similar to my question above about the duplicated name.

Could there be bugs if we suddenly have two elements that match an old query, be it an attribute or name search?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on the cloning! I had forgotten to circle back to that after I got everything else cleaned up. I went back and did some more testing and found that I could just use a brand new select element without copying anything (besides the name as discussed above).

I think the only thing we need to decide here is what we should use as the name for the the selects. I have left the name of the select the same as the input for now since it simplifies logic downstream in the changeHandler, but we can change that (as I mentioned above). I would just need to include some other way of getting the field path into the changeHandler function...

@jkuester jkuester requested a review from dianabarsan July 21, 2022 21:57
Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Congratulations for bringing this titanic task to completion!

@kawerewagaba
Copy link

following the conversation, because i believe this will be a life saver

@jkuester jkuester changed the title 6345 enketo-core uplift 6345 - Uplift to enketo-core version 5.18.1 Aug 16, 2022
@jkuester jkuester merged commit 2a25f87 into master Aug 16, 2022
@jkuester jkuester deleted the 6345-enketo-uplift branch August 16, 2022 19:03
jkuester added a commit that referenced this pull request Oct 17, 2022
Co-authored-by: Jennifer Q <[email protected]>

(cherry picked from commit 2a25f87)
@jkuester jkuester restored the 6345-enketo-uplift branch June 30, 2023 15:02
@jkuester jkuester deleted the 6345-enketo-uplift branch June 30, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment