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.

Sign up for GitHub

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

O3-2476 - isOmrsDateStrict should accept dates with negative timezone… #785

Merged
merged 1 commit into from
Oct 10, 2023
+2 −1
Merged

O3-2476 - isOmrsDateStrict should accept dates with negative timezone… #785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/framework/esm-utils/src/omrs-dates.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this fail test case fail without a colon check, or is there something else wrong with this that I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
2 changes: 1 addition & 1 deletion packages/framework/esm-utils/src/omrs-dates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export function isOmrsDateStrict(omrsPayloadString: string): boolean {
}

// checking UTC offset format
if (!(omrsPayloadString[23] === "+" && omrsPayloadString[26] !== ":")) {
if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-")) {
Copy link
Member Author

@mseaton mseaton Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zone formats are specified like +05:00 or -05:00 so it's checking both there's a +, two characters, and a colon.

Copy link
Member

@ibacher ibacher Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work and be consistent with the code here that verifies all the non-numeric components:

Suggested change
if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-")) {
if (!(omrsPayloadString[23] === "+" || omrsPayloadString[23] === "-" || omrsPayloadString[26] === ":")) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I misread that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@brandones brandones Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @brandones ... this rang a bell and I was actually able to track this down, see bug here:

https://issues.openmrs.org/browse/RESTWS-419?jql=project%20%3D%20RESTWS%20AND%20text%20~%20%22date%22

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

return false;
}

Expand Down
Loading