cht-core icon indicating copy to clipboard operation
cht-core copied to clipboard

Provide common date and time functions to xform definitions

Open kennsippell opened this issue 5 years ago • 11 comments

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.

kennsippell avatar Mar 01 '19 15:03 kennsippell

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

SCdF avatar Mar 01 '19 15:03 SCdF

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.

abbyad avatar Mar 01 '19 21:03 abbyad

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?

kennsippell avatar Mar 03 '19 12:03 kennsippell

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
    • Generic form: calculate="{put your javascript code here}"
    • Example: For example calculate="moment().add(${follow_up_count}, 'days').fromNow()"
  2. Allow JS within XPath expressions
    • Generic form: calculate="javascript('{put your javascript code here}')"
    • Example: calculate="javascript(moment().add(${follow_up_count}, 'days').fromNow())"
  3. Bring in specific moment functions styled as XPath expressions
    • We currently do something similar with functions like difference-in-months (note the TODO there that it should be in a separate file). We could use a prefix to be clear when an extension is used, and to be compliant with XForms.
    • Generic form: calculate="cht::{function}('{args}')"
    • Example: calculate="cht::from-now(${follow_up_count}, 'days')"

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:

  • What scope do we allow from JS?
  • Is any JS restricted?
  • How do we access form fields from JS?

abbyad avatar Jul 16 '19 14:07 abbyad

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.

garethbowen avatar Jul 16 '19 21:07 garethbowen

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).

kennsippell avatar Jul 17 '19 05:07 kennsippell

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.

abbyad avatar Jul 17 '19 13:07 abbyad

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

MaxDiz avatar Feb 06 '20 14:02 MaxDiz

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

garethbowen avatar May 26 '20 00:05 garethbowen

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.

abbyad avatar Sep 15 '20 13:09 abbyad

@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.

jkuester avatar Aug 17 '22 17:08 jkuester