search

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

Provide common date and time functions to xform definitions #5468

Open kennsippell opened 5 years ago

kennsippell commented 5 years ago

Code like this is a very common practice to write in to our XForms. It is hard to maintain and very easy to fall out of sync with configuration.

if(${follow_up_count} = '1', "Suivi 48 Heures", if(${follow_up_count} = '2', "Suivi 5 jours", if(${follow_up_count} = '3', "Suivi 12 jours", if(${follow_up_count} = '4', "Suivi 19 jours", if(${follow_up_count} = '5', "Suivi 26 jours", if(${follow_up_count} = '6', "Suivi 33 jours", if(${follow_up_count} = '7', "Suivi 40 jours", if(${follow_up_count} = '8', "Suivi 47 jours", ""))))))))

This can be achieved very simply with the moment.js library (https://momentjs.com) with the command moment().add(${follow_up_count}, 'days').fromNow(); It would be an absolute dream for form authors to just have access to the moment library in their xform code.

SCdF commented 5 years ago

Agree, and extend to say we should be exposing moment to all of our "application code" (rules, purging, forms etc)

abbyad commented 5 years ago

I think the xpath equivalent to what you have is date(decimal-date-time(today())+${follow_up_count}). That said, I don't think either properly replaces the xpath from the original post because the mix of values, and they don't include the message. (Also, is the xpath pasted twice?)

Is the goal of this issue to make calculations from XForms more JS friendly, or is it to bring in momentjs? If it's the latter, we should consider adding moment style functions to our xpath evaluator -- we have brought in other helper functions in this way, and there are definitely existing ways to make that expression more compact. If it's the former, and that is a valid point, we should have a larger discussion.

kennsippell commented 5 years ago

My goal here is clarity, simplicity, and maintainability of code. As well as code reuse and providing familiar tooling to cht partners. I'm new to form creation, but there is a lot of date manipulation in muso forms and I find the code cumbersome.

MomentJs is what we use in webapp for all datetime manipulation. It is elegant, flexible, powerful, extensible, well documented and well supported, and broadly loved by JavaScript developers.

As you point out, this isnt the best example. A bit of data massaging is required - but the moment.fromNow() function outputs a localized string like "5 days from now" or "tomorrow" or "one year ago" in any of our supported languages. This is very useful. I believe the xpath function you provided outputs an unformatted Date object and does not include logic for formatting?

abbyad commented 5 years ago

Just noting that we have to come to a design decision here on how to include moment functionality. From what I can tell there are several approaches: 1) Allow JS as XPath expressions

It is important to maintain a standards approach, so am in favor of option 3, allowing and documenting new XPath-like expressions that bring in moment functions as needed. If we want to bring in more flexible JS, then I'd go with to 2 rather than 1 to make it clear to form developers that they are breaking out of XForms/XPath notation.

Some additional questions if we do allow JS:

garethbowen commented 5 years ago

I definitely prefer Option 3. Although it's much less flexible and more work for the product team to implement it'll be much easier to maintain backwards compatibility if we bump moment or replace it entirely.

Secondly, fewer evals is always good.

Thirdly, it's consistent with xpath syntax rather than coming up with something custom and unexpected.

kennsippell commented 5 years ago

I like option 3 also. Instead of "bring in specific moment functions" one by one, I believe all capabilities of moment are exposed through the single moment('foo').bar interfaces, so just bringing in that single moment function would expose all capabilities of the library in a well documented and supported interface (perhaps with a prefix as you suggested).

abbyad commented 5 years ago

Glad we are on the same page!

@kennsippell how were you thinking we could get all the moment capabilities with a syntax that is consistent with XPath? Would it be something like cht::moment('{function}', {args...}), for example cht::moment('from-now', ${follow_up_count}, 'days')? Or perhaps moment::from-now(${follow_up_count}, 'days')? I am trying to picture what it would look like in a usable way, and that could allow us to replace it out as Gareth pointed out -- if that is a priority for us. Would be great to flesh this out further, so feel free to post more ideas for discussion.

MaxDiz commented 4 years ago

bumping to be completed after enketo upgrade in 3.9 since it will have significant impact on xform xpaths

garethbowen commented 4 years ago

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

abbyad commented 4 years ago

The choice to bring in Moment-style functions as individual XPath functions (instead of opening direct use of momentjs) seems reinforced by Moment.js's current status:

We now generally consider Moment to be a legacy project in maintenance mode. It is not dead, but it is indeed done.

jkuester commented 2 years ago

@kennsippell I just wanted to note that we are looking to address some of this functionality in https://github.com/medic/cht-core/pull/7729 (adding a new add-date function). I would love your thoughts/feedback on the PR!

It is not intended to fully address this issue, but should be a place to start.