medic / cht-core

The CHT Core Framework makes it faster to build responsive, offline-first digital health apps that equip health workers to provide better care in their communities. It is a central resource of the Community Health Toolkit.
https://communityhealthtoolkit.org
GNU Affero General Public License v3.0
441 stars 212 forks source link

Support making countdown-widgets `required` in forms #8681

Closed jkuester closed 6 months ago

jkuester commented 11 months ago

Is your feature request related to a problem? Please describe. Currently, when including the countdown-widget in a form, there is no way to ensure that the widget was actually used when filling out the form. Users can simply skip the widget and continue to fill out the form without waiting for the timer to run.

Describe the solution you'd like It seems like the cleanest way to proceed would be to have the required value be respected for fields with the countdown-timer appearance. This way, a form designer could control whether or not the user has to use the timer by setting the required expression (and, by default, the behavior would remain the same as now where a user is not required to wait on the timer).

Additional context This is a feature request from a partner.

Implementing this could be tricky since currently the countdown-widget operates as a note field in the form. The note fields actually cannot be required in recent versions of Enekto (since objectively it does not really make sense to require a note). So, we may need to support using the countdown-timer appearance on a different type of question besides note. Options off the to of my head are: boolean field that is set to true when the timer completes, or integer field that contains the number of seconds remaining on the timer.

Jeanfaith234 commented 11 months ago

@jkuester The request is for the preceding question to be restricted / dependent on the successful completion of the timer after 60 seconds / one minute. Below is the video explaining the current implementation in detail and the requested change.

Countdown timer.webm

jkuester commented 10 months ago

I just wanted to capture a few more details about this request based on today's conversations!

The main pain point that this feature would address is ensuring data accuracy in forms that involve a timer, For example, when capturing a value for Breaths per Minute, we can only get an accurate count of breaths when they are tracked for the full minute. Requiring the Breaths per Minute question does ensure that we get a value, but being able to also require the timer to be run would help to ensure that the provided value was accurate!

Also, it seems that modeling the countdown timer as a boolean question makes the most sense! (An integer value would be more appropriate for a theoretical stopwatch widget.)

jkuester commented 9 months ago

@Kymoraa and Abigael, here are some additional design considerations I had after taking a closer look at the code:

jkuester commented 8 months ago

Okay, now that @SheilaAbby has got us some working code to play around with, I think we have been able to shake out the majority of the complexities here and can land on a particular functionality. @garethbowen please let me know what you think (as this proposal does result in a change to how we would recommend representing timers in xforms)!

Design proposal

Currently, the count-down timer widget works by putting the countdown-timer appearance onto a note field. You can set your custom time on the timer via the default column in your xform. We cannot continue to use this approach, while at the same time adding support for the require because you cannot require a note in Enketo.

My proposal is we deprecate note timers (and do not support require for them). Instead we switch to supporting the countdown-timer appearance on trigger/acknowledge questions. A custom time for the timer could be provided via a duration parameter (note that parameter is a separate column in your xform that is used for providing configuration params for a question (e.g. you can set how many rows to display for a text question)). For trigger/acknowledge timers, we would set the value of the question to OK (equivalent to selecting the checkbox) once the timer has completed. This will allow the normal Enketo require functionality to operate on these questions.

In my opinion, a trigger question with a countdown-timer appearance, configured by a parameter is the least surprising (most appropriate) way model this functionality in Enketo. Basically we are just adding a visible delay to the "acknowledgement". I have tinkered a bit with using the code in countdown-widget with a trigger question. It will not just be a drop-in replacement, but it should be feasible to implement in a clean way (will be a bit of extra logic to maintain the note functionality alongside, but should not be a problem).

garethbowen commented 8 months ago

I think that's a big improvement! I have no problem deprecating the old widget definition and maintaining it till the next major (or beyond if necessary). I imagine the code will be reused for this anyway.

The new design looks much more idiomatic of Enketo forms and like you say it means we can leverage this elsewhere with require to give designers more flexibility.

Do you know if the parameter parameter is supported by our build tools, eg: xls2xform?

jkuester commented 8 months ago

Thanks @garethbowen!

Do you know if the parameter parameter is supported by our build tools, eg: xls2xform?

~Yes!~ (Edit: no, it turns out the supported params are hardcoded in pyxform.) I recently tested a full end-to-end of using parameters for a range question (xform > cht-conf > cht-core > webapp > Enketo) and everything was converted/included as expected for the parameters!

SheilaAbby commented 8 months ago

Okay, now that @SheilaAbby has got us some working code to play around with, I think we have been able to shake out the majority of the complexities here and can land on a particular functionality. @garethbowen please let me know what you think (as this proposal does result in a change to how we would recommend representing timers in xforms)!

Design proposal

Currently, the count-down timer widget works by putting the countdown-timer appearance onto a note field. You can set your custom time on the timer via the default column in your xform. We cannot continue to use this approach, while at the same time adding support for the require because you cannot require a note in Enketo.

My proposal is we deprecate note timers (and do not support require for them). Instead we switch to supporting the countdown-timer appearance on trigger/acknowledge questions. A custom time for the timer could be provided via a duration parameter (note that parameter is a separate column in your xform that is used for providing configuration params for a question (e.g. you can set how many rows to display for a text question)). For trigger/acknowledge timers, we would set the value of the question to OK (equivalent to selecting the checkbox) once the timer has completed. This will allow the normal Enketo require functionality to operate on these questions.

In my opinion, a trigger question with a countdown-timer appearance, configured by a parameter is the least surprising (most appropriate) way model this functionality in Enketo. Basically we are just adding a visible delay to the "acknowledgement". I have tinkered a bit with using the code in countdown-widget with a trigger question. It will not just be a drop-in replacement, but it should be feasible to implement in a clean way (will be a bit of extra logic to maintain the note functionality alongside, but should not be a problem).

@jkuester Thank you for the valuable suggestions. I've attempted to integrate the timer widget with a trigger/acknowledgequestion. However, we encounter a minor drawback since the trigger/acknowledge field cannot be set to 'hidden'; it appears on the form, requiring users to check the box. Once the box is checked, 'OK' becomes the timer value even though the timer is still running. Nonetheless, the js code can assign a different value (not a 'OK' value) to the same field when the timer completes, and this new value is used to constrain the timer, not the initial 'OK' check input by the user.

please find attached screenshot

Screenshot 2024-02-02 at 16 48 05

Supporting the timer on a text question type won’t introduce an extra question on the form requiring a user to input a value. Surprisingly, I have observed it is possible to leave the default column for custom timer durations, as it has been( and remove the FALSE value I set here https://github.com/medic/cht-core/pull/8826). The text input will receive an 'ok' string value when the timer completes running. The text field won't display an 'input' field on the form requiring a user to enter a value and properly displays the countdown widget.

see,

Screenshot 2024-02-02 at 17 21 13
jkuester commented 8 months ago

:+1: Yes, we have a bit more logic to add to get everything working like we need it. Currently, the textbox for the string/note question is actually being hidden by some CSS (which is why you do not see it for questions of that type). We should be able to do the same for that OK radio button. However, we need a bit more changes to the logic for attaching the timer canvas to the page to avoid also hiding the timer too (even in your screenshot you can see that the timer canvas is actually a child of the OK radio...). I will share some more specific suggestions later today on the PR!

jkuester commented 8 months ago

So, in today's episode of why we cannot have nice things... it turns out my previous comment about how parameters should work fine for us, is wrong.... Apparently, in Pyxform, all the supported parameters are actually hard-coded for each question type and only those values are loaded. So, we cannot make-up custom params for trigger questions. :sob: There is an open Pyxform issue regarding making parameter handling more sophisticated. I have added a comment out there to see if they might support just passing through custom parameters. :shrug:

This does leave us in a bit of a dilemma here, though. Here are the options I have thought of:

  1. Just stick with using the default column to provide the custom duration. To make this work with trigger/require I think we could just add some code to the init of the countdown widget to get the custom duration value and then set the value for the question to '' before the user starts filling out the form. I think this would be workable, but not very "idiomatic".
  2. Update our custom fork of Pyxform to support the parameters we need. I would prefer to avoid additional custom pyxform code at all costs.
  3. Instead of the proper parameters column, we could instead support setting the duration into a new instance::parameters column. Pyxform would automatically load this data into the form xml instance model. If needed, we could update cht-conf to then copy the parameters down into the form body. The instance::parameters values could be comma separated properties just like the normal parameters column and the column could get re-used to set parameters on any kind of question we wanted.

#1 seems like the most feasible option for us to be able to immediately proceed here. (And has the bonus of being the closest to our current implementation of countdown-timer.) If ODK is open to supporting custom parameters in Pyxform, then I would rather not do the extra work of #2 or #3 just to change it again in the future when we get custom params. If ODK is not open to custom params (and does not have any other suggestions for a better way to go), then I think #3 is worth considering as a possible way forwards that might even be able to get us off of our custom pyxform fork... @garethbowen what do you think?

jkuester commented 8 months ago

Actually got a response already from ODK out on the Pyxform thread! TLDR is that they do not think we should be trying to use the properties column like this. BUT, they pointed me to this doc for the instance/bind/body extension columns and this more or less reinforces exactly what I was suggesting in #3 (only now I know that this is actually the intended usage!).

I am going to dig deeper on this next week and try to put together a more detailed proposal!

garethbowen commented 8 months ago

Sounds like #3 is the way forward and definitely worth the investigation. Thanks @jkuester

jkuester commented 8 months ago

Okay, @garethbowen, hopefully the last request for design review!

I think we should support a workflow where the xlsxform looks like:

Pyxform will automatically load data from the instance::cht::properties column into the form xml (as it is currently doing for db-doc). Then, in the cht-core code, we would need to update the generate-xform service in the api code to unpack the attributes from the form xml and set them into the form HTML (generated by the xsl translation) as attributes of the input element (<input cht:duration="100" ...). This will allow them to be accessible to the custom Enketo widgets. (This similar to how the official properties column values get set onto the Enketo widgets. Basically, pyxform translates the properties values into the form xml body section as attributes. Then, the enketo-transformer xsl copies the body attributes into the generated form HTML. The Enketo widgets read the attribute values to know the property values to use.)

This seems to me to be the best balance of following the standards, getting the functionality we need, and not being too difficult to implement/maintain. There are a couple questions this does raise:

  1. Should we use instance::cht::properties or bind::.. or body::...? All are supported by the spec. I am leaning towards instance:: because:
    • We already have instance::db-doc, etc. So, closer to what folks are used to seeing in forms.
    • The values from instance::... also get automatically loaded into the form model xml (by the xsl transition) and then are automatically included by Enketo in the form data returned when calling form.getDataStr(). This could make it more simple to use these values for post-processing form data (e.g. this is how we currently are doing the db-doc processing).
    • From the spec, the body of the xform xml seems more related to display of the data. Things like timer duration seem to be more than just a "display" customization.
    • On the other hand, bind::... may actually be a more semantically accurate place to store custom properties. As far as I can tell, the instance data is more related to the actual answers to the form questions (e.g. also includes default values) while the bind data is focused on the properties that affect the logic of the question (e.g. type, relevance, etc). Happy to go this way instead if it seems more correct.
  2. One alternative approach that seems worth pointing out is that instead of having a common instance::cht::properties column in the xlsxform, we could have different columns based on the actual property names (e.g. instance::cht::duration). Then, instead of adding custom code into generate-xforms to unpack the properties, we could just customize the xsl transition to automatically set the attributes onto the HTML. I currently prefer the singular instance::cht::properties column approach for the following reasons:
    • It avoids having a bunch of one-off columns in the xlsxform that are only used for one or two questions. Instead, many types of questions can have their parameters set in instance::cht::properties (could ultimately refactor db-doc to reuse this column). Will reduce the excessive number of columns in some forms.
    • We are not far from being able to avoid having to use a custom xsl file for translating the form xml. I would rather avoid adding more complexity to that situation (even if the alternative is manual post-processing).

Let me know what you think or if you have any better ideas for how to approach this! Thanks!

garethbowen commented 8 months ago

If our implementation for db-doc didn't exist then I think bind would be more correct, and I'm not too concerned about compatibility with db-doc if we feel bind is right. I assume bind gets handled just the same as instance in Enketo?

I prefer more columns with simple values (option 2). Defining properties is rare so far so I don't think it'll explode the number of columns in every xform. However I really don't like the idea of custom xsl. Can we use the same approach as 1 while using multiple columns like in 2?

If the answer is complicated (and I suspect it is) feel free to grab me for a call!

SheilaAbby commented 8 months ago

@jkuester @garethbowen Some great ideas here! But putting them into action seems a bit tricky to me! I've made some changes to the PR for using a text input question type. Please take a look when you can. https://github.com/medic/cht-core/pull/8826

Also, just a heads up, workflow devs can still use the 'default' column for setting custom timer durations....so basically am not setting any default value on the fields with the countdown timer appearance...but when the timer completes running, the field will be set to contain a 'true'string value

jkuester commented 8 months ago

@SheilaAbby Thanks for the patience here as we work through the best way to add support. It is true that the scope is shifting a bit here. My proposal, is that you and I work together to land this! You can keep focused on the countdown-timer widget code (I think we are pretty close on this code, just need to get some tests). And I will jump in and make the code changes needed on the api side to get the data we need from the xform. How does that sound?

jkuester commented 8 months ago

Had a brief discussion with @garethbowen today and we landed on this design approach:

Xlsxform:

api/generate-xform service:

webapp/countdown-timer widget:

jkuester commented 8 months ago

@garethbowen I have run into a challenge with using a custom namespace for the xlsx column name (e.g. instance::cht:duration). If you have a custom attribute column without the namespace (e.g. instance::duration) this gets converted to something like <my_timer duration="10"/>. However, with the custom namespace, you get something like <my_timer cht="{'duration': '5'}"/>. Of course, things get really fun, then, when you start including quotes and stuff in your column values: cht="{'duration': '5', 'message': 'Uses\'s &quot;both&quot; kinds of quotes'}". None of these serializes directly to JSON. I am hoping you can tell me that this is some standard format for dictionary data and there is a library for easily serializing this into a JS object! (I have not been able to find anything searching around the web, but maybe I am missing something... :shrug: ) I am afraid it is just the string serialization of a Python dict (that perhaps, by design, is XML-compatible). But, once again, in that case I am not seeing any convenient way to de-serialize it to a JS object.

Assuming there is no convenient library available for de-serializing this data, it seems like our options are:

  1. Specify that we do not support quotes in our custom attribute columns (not a problem at this point since at this point we just need to support a number for duration). Then it should be pretty straightforward to convert the data to JSON.
  2. Write some more advanced parsing code to properly de-serialize the data (and handle all the different quote combinations).
  3. Just don't use a custom namespace for our attribute columns. Instead we could say something like: instance::cht_duration.
  4. Log a Pyxform ticket to see if they can change the data format to something more parsable.

#2 seems like a bad idea due to a high level of complexity so support an unnecessary usecase that we do not even have yet. #3 would be okay, but does not really follow the true spirit of the spec. I almost did #4, but I realized that you cannot put JSON directly into XML attributes, so I was not sure exactly what the alternative approach should be here. If anyone has an idea of a better say data could be represented in the XML attribute (maybe just escaped JSON?) then I am happy to suggest it in Pyxform!

garethbowen commented 8 months ago

Okay. That's really painful!

I think the answer is some form of (3). I have no strong preference now about whether we use the cht_ convention or just drop it and use duration which seems like the supported approach. The chance of a conflict is probably low enough we can get away with it, and we can handle a conflict if one does arise. If the prefix is useful as a flag for which attributes to handle in our custom code then it's fine to keep it.

With (4), I'm not sure if they'll want to support JSON in xml, but you should be able to namespace the attribute somehow, eg: instance::cht:duration -> <my_timer cht:duration="10"/>. I think that's worth an issue...

jkuester commented 8 months ago

Logged a Pyxform issue! https://github.com/XLSForm/pyxform/issues/696

My vote is we wait a bit to see what the reaction is to the Pyxform suggestion. If they are open to changing that behavior, then maybe we can get away with #1 (or really, just hard-code support for duration for now and don't try and have generic support for all columns/value). Then, a new version of Pyxform would allow us to unlock more usages of custom columns and we could add more generic support at that point....

jkuester commented 8 months ago

Update from the Pyxform issue! It turns out I just cannot read properly (even typoed several times in this thread). I thought the spec said to use instance::cht::duration, but actually I should have been using instance::cht:duration. :facepalm: Fixing my column name resolves the weird object serialization issues (so we can just load one value per attribute). So, we are back in business with the same originally intended generic approach to translating those attributes. @SheilaAbby I am hoping to have the server-side code ready to add to your branch by the end of this week!

jkuester commented 6 months ago

Completed in https://github.com/medic/cht-core/commit/3e91ea65c0aa0d6799dcf1e3f7c6345d2cd2dd8b