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

'Invalid Date' returned by format-date function when given an empty value #864

Closed
jkuester opened this issue Mar 8, 2022 · 6 comments · Fixed by enketo/openrosa-xpath-evaluator#155

Comments

@jkuester
Copy link
Contributor

jkuester commented Mar 8, 2022

In the 1.x version of openrosa-xpath-evaluator, the format-date function returned an empty string. However, as part of the changes for 2.0.0, this logic shifted (perhaps unintentionally?) to return the string 'Invalid Date' instead.

Unfortunately, unless I am missing something, the spec for format-date does not mention what should be returned in this case.

The problem with this behavior change is that it results in some surprising down-stream effects for logic that is processing formatted date values originating from non-required questions. For example:

type name label calculation
date my_date My Date  
calculate my_formatted_date My Formatted Date format-date(${my_date},"%Y-%m-%d")
note my_note Formatted Date: ${my_formatted_date}  

With this form, if you do not enter a value for my_date, then the note displayed is Formatted Date: Invalid Date.

We are trying to figure how how to handle this behavior change in the CHT as we uplift to use 2.x of openrosa-xpath-evaluator and would appreciate any thoughts or feedback on this! Does it make sense for this function to return 'Invalid Date' even when the given value is just empty?

@eyelidlessness
Copy link
Collaborator

Thanks for filing this @jkuester. I took a look at how this works in JavaRosa, and it appears to return an empty string. I think it would make sense to restore that behavior here. Any objections @MartijnR @alxndrsn @lognaturel?

@alxndrsn
Copy link
Collaborator

I think `"Invalid Date"' is entirely Javascript-specific, and there is no particular reason to use it in this library.

The date and dateTime data type definitions reference the XML 1.0 spec, and from there ISO 8601, so perhaps there is something there which would have guidance on representing invalid dates as strings?

@MartijnR
Copy link
Member

MartijnR commented Mar 11, 2022

Seems fine to me to return '' for empty values. Not sure about invalid date values though. I think it might be helpful to return something like Invalid Date as equivalent of NaN (like before), or not?

(am actually referring to current PR which also changes behavior for invalid dates though not mentioned above)

@jkuester
Copy link
Contributor Author

Seems fine to me to return '' for empty values. Not sure about invalid date values though. I think it might be helpful to return something like Invalid Date as equivalent of NaN (like before), or not?

@MartijnR, I agree. It would be nice to still report Invalid Date when the date value is actually bad and not just empty. My main concern, though, is that the current behavior of the date function (as discussed here) might make this tricky since it will return an invalid Date when passed an unanswered question. So, chaining function calls would still result in undesired behavior.

So, format-date(date(${unanswered_question}), '%Y-%m-%d') would still result in Invalid Date since the value returned from the date function is not empty, but is actually an invalid Date value.

Having format-date just return an empty string in both cases is less than ideal, but for what it is worth it would be acceptable for our needs (mostly because it would just be a return to how things behaved before). The main issue we are seeing with this current behavior is that after the results of the form are saved (as JSON docs) there is down-stream logic to trigger certain things when the formatted date fields in the JSON are not empty. But when Invalid Date is saved as the formatted date, the same logic gets triggered (and unfortunately, we are not talking about code in cht-core that we could easily update to handle this case, but instead it is custom configuration built by various partners into their own CHT deployments...).

@eyelidlessness
Copy link
Collaborator

So, format-date(date(${unanswered_question}), '%Y-%m-%d') would still result in Invalid Date since the value returned from the date function is not empty, but is actually an invalid Date value.

I checked the JavaRosa behavior for date, and it also appears to return an empty string for date(''). I think it would make sense to align all of these unless there's a compelling historical reason not to do so.

It may cause a lot of churn to change the actual implementation signatures (e.g. to accept/return string instead of Date), in which case we could consider subclassing Date, e.g. something like:

class ORXEDate extends Date {
  constructor(...args) {
    super(args);

    /** @private */
    this.isEmpty = args.length === 1 && args[0] === '';
  }

  toString() {
    return this.isEmpty ? '' : super.toString();
  }
}

@jkuester
Copy link
Contributor Author

Okay, so I have spent a bunch of time thinking about this today and going through the code to see what makes the most sense. Ultimately, I am thinking that our changes related to this issue should be limited to these two things:

  1. Update format-date to always return '' when the provided value is not a valid date
  2. Update date to return '' when the provided value is ''

This will cause this logic to be aligned with JavaRosa. Anytime the JavaRosa implementation of format-date receives a value that is not a Date, it returns an empty string. There is similar explicit logic in date to always return the empty string when that is what is provided.

Just to be clear, here is the current behavior of OpenRosa vs JavaRosa:

JavaRosa:

assertEquals("", formatDateTime(Double.NaN, "%Y-%m-%d"));
assertEquals("", formatDateTime("", "%Y-%m-%d"));
assertEquals(Double.NaN, toDate(Double.NaN, false));
assertEquals("", toDate("", false));

OpenRosa:

assertString("format-date(NaN, '%Y-%m-%d')", 'Invalid Date');
assertString("format-date('', '%Y-%m-%d')", 'Invalid Date');
assertString("date(NaN)", 'Invalid Date');
assertString("date('')", 'Invalid Date');

I am proposing we update OpenRosa so that it behaves like this:

assertString("format-date(NaN, '%Y-%m-%d')", '');
assertString("format-date('', '%Y-%m-%d')", '');
assertString("date(NaN)", 'Invalid Date');
assertString("date('')", '');

@lognaturel lognaturel transferred this issue from enketo/openrosa-xpath-evaluator Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants