Update format-date to return empty string instead of 'Invalid Date'

[jkuester]

Updates the format-date xPath function to return '' instead of 'Invalid Date' when provided with an empty value or invalid date.

This normalizes the behavior with 1.x of openrosa and also with JavaRosa. See the linked issue for more information.

Closes enketo/enketo#864

[alxndrsn]

I think there are still place(s) where a date can be converted into the string "Invalid Date"; would it be consistent to address those in the same PR?

[jkuester]

I think there are still place(s) where a date can be converted into the string "Invalid Date"; would it be consistent to address those in the same PR?

@alxndrsn I have done some digging (but am by no means an expert on this code base) and I have not found any other xPath functions that will directly return the String value "Invalid Date". (Which, I think, makes sense since the format-date/format-date-time functions are the only ones that I would expect to be converting date values to strings.)

That being said, the actual date function can return invalid Date values (Date objects that contain invalid data). When these are converted to strings (e.g. by calling date.toString()) they result in the string "Invalid Date" (because of the way Date is implemented in JS). Once easy way to get the date function to return an invalid value is to pass NaN.

type	name	label	calculation
integer	my_number	Pick a Number
calculate	date_from_number	My Date	date(${my_number})
note	show_date	My Date: ${date_from_number}

If no number value is entered, the note will show: My Date: Invalid Date. (Worth noting that this is the same behavior for the date function as in openrosa 1.x and so is not something that changed with the formate-date behavior.)

Does it make sense to try and update the date function to be smarter about its return value and instead of returning an invalid date, return an empty array node? This is what I see used when just directly getting the value of an unanswered date question:

{
  "t": "arr",
  "v": [ {} ]
}

Seems like that kind of change might have a lot of unintended side-effects, but I am not sure.

[eyelidlessness]

For such a small change, this review was a bit of a wild ride! First I mistook the purposes of the changes entirely, then I got a little nitpicky, then I really stared at it for a while until I decided to try another approach entirely.

This change does certainly work as expected, but I think the alternative approach with a subclass will likely be easier to understand for future maintenance.

[eyelidlessness]

This looks great, thank you!