O3-2476 - isOmrsDateStrict should accept dates with negative timezone… #785
Conversation
| @@ -50,7 +50,7 @@ export function isOmrsDateStrict(omrsPayloadString: string): boolean { | |||
| } | |||
|
|
|||
| // checking UTC offset format | |||
| if (!(omrsPayloadString[23] === "+" && omrsPayloadString[26] !== ":")) { | |||
| if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-")) { | |||
There was a problem hiding this comment.
So, I don't know what that original omrsPayloadString[26] !== ":" was all about, it's not documented, and I don't really understand the use case, and all of the unit tests pass without it being there, so I removed it in addition to allowing the negative offset...
There was a problem hiding this comment.
Zone formats are specified like +05:00 or -05:00 so it's checking both there's a +, two characters, and a colon.
There was a problem hiding this comment.
This should work and be consistent with the code here that verifies all the non-numeric components:
| if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-")) { | |
| if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-" || omrsPayloadString[26] === ":")) { |
There was a problem hiding this comment.
I don't think so @ibacher ? Look at all of the test cases for this class, and look at the date string I was originally having an issue with on the ticket. It's in the format: 2023-10-06T12:56:56.065-0400. There is no colon in the TZ offset...
There was a problem hiding this comment.
Specifically @ibacher, the isoFormat specified in omrs-dates.js is:
const isoFormat = "YYYY-MM-DDTHH:mm:ss.SSSZZ";
And, per the day.js documentation, "ZZ" is "compact offset from UTC", whereas just "Z" is the offset with the ":". See https://day.js.org/docs/en/parse/string-format. So with our date format, we will get "-0500", not "-05:00".
There was a problem hiding this comment.
It looks like ISO considers both including and not including a : as valid (https://en.wikipedia.org/wiki/ISO_8601)
I can't remember where, but I do remember some case where we were having an issue with this... ie like an old version of Java only excepted one of the two?
Do note @ibacher that it looks like the initial code was testing that a colon was not present, so it was enforcing the "0400" format. I'm fine with removing it.... though see my comment above.
Also will flag @brandones @FlorianRappl in case they have any insight.
There was a problem hiding this comment.
In any event, removing the check on : only serves to make the code more permissive, not less permissive. And the unit test that checks this still works regardless, which presumably is because later on, we return dayjs(omrsPayloadString, isoFormat).isValid() with our isoFormat of "YYYY-MM-DDTHH:mm:ss.SSSZZ". So there isn't any need for us to have this additional custom check anyway.
There was a problem hiding this comment.
Yeah I assume there only is such a thing as "omrs ISO format" because, again I assume, the OpenMRS server must use an ISO parser that is not quite correct; perhaps it is more particular about format than the ISO spec. If the OpenMRS server understands any ISO-spec date strings than I don't know why any of those functions exist. I did not implement them, so I don't really know what the motivation was.
There was a problem hiding this comment.
Thanks @brandones ... this rang a bell and I was actually able to track this down, see bug here:
However, this assumedly was resolved years ago?
Note that even though @mseaton removed the test for the colon in the timezone, such a string is still considered invaid, I believe/assume because it's 29 characters in length instead of 28, and that is one of the first checks in the validator... fyi @ibacher @mseaton
|
Size Change: -327 kB (-10%) 👏 Total Size: 2.82 MB
ℹ️ View Unchanged
|
| @@ -50,7 +50,7 @@ export function isOmrsDateStrict(omrsPayloadString: string): boolean { | |||
| } | |||
|
|
|||
| // checking UTC offset format | |||
| if (!(omrsPayloadString[23] === "+" && omrsPayloadString[26] !== ":")) { | |||
| if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-")) { | |||
There was a problem hiding this comment.
It looks like ISO considers both including and not including a : as valid (https://en.wikipedia.org/wiki/ISO_8601)
I can't remember where, but I do remember some case where we were having an issue with this... ie like an old version of Java only excepted one of the two?
Do note @ibacher that it looks like the initial code was testing that a colon was not present, so it was enforcing the "0400" format. I'm fine with removing it.... though see my comment above.
Also will flag @brandones @FlorianRappl in case they have any insight.
| @@ -22,6 +22,7 @@ describe("Openmrs Dates", () => { | |||
| it("checks if a string is openmrs date", () => { | |||
| expect(isOmrsDateStrict("2018-03-19T00:00:00.000+0300")).toEqual(true); | |||
| expect(isOmrsDateStrict(" 2018-03-19T00:00:00.000+0300 ")).toEqual(true); | |||
| expect(isOmrsDateStrict("2023-10-06T12:56:56.065-0400")).toEqual(true); | |||
| // the exclusion test cases are important for strictness | |||
| expect(isOmrsDateStrict("2018-03-19 00:00:00.000+0300")).toEqual(false); | |||
| expect(isOmrsDateStrict("2018-03-19T00:00:00.000+03:00")).toEqual(false); | |||
There was a problem hiding this comment.
won't this fail test case fail without a colon check, or is there something else wrong with this that I'm missing?
There was a problem hiding this comment.
@mogoodrich , not sure I follow? This is basically already testing that having the colon is invalid. So according to the tests at least, having +03:00 is not valid, only +0300 is valid
There was a problem hiding this comment.
Right... so I guess I'm confused, but I was assuming that if you removed the omrsPayloadString[26] !== ":" the isOmrsDateStrict would return "true" for this (and therefore the test woud fail).
… offsets
Requirements
If applicable
Related Issue
https://issues.openmrs.org/browse/O3-2476