Safari shows incorrect reviewer assignment information

[asmecher]

Describe the bug

From https://forum.pkp.sfu.ca/t/days-since-last-review-errors-in-ojs/72558:

In the reviewer assignment area (after clicking on “Add Reviewer” in the main article interface), one sees a list of potential reviewers with some information about each one, including information about when they were last assigned a review. For two members of our team, we always see the value “never assigned” even in cases where the reviewer has in fact been assigned a review in the recent past. For another team member, the value is the real information (e.g. “3 days ago”). We have done some initial troubleshooting and cannot correlate this with any patterns in editor/user roles, whose queue the article is in, or anything specific about the article in question. We’ve all looked at the same articles and are seeing different things.

Later...

I am running MacOS 12.3 and Safari 15.4. However!! I just tried with Firefox (v99.0.1) and am getting correct review assignment information. My colleague was getting correct information on a Dell Inspiron using the latest version of Chrome. So it looks as if it could be a Safari problem.

What application are you using?
OJS 3.3.0-8

[ewhanson]

Hey @asmecher, this was an issue in the way Safari's JavaScript Date functions handled date strings separated with a - (which returned NaN for any dates formatted like "2022-05-04" instead of "2022/05/04"). I've made PRs for OJS and OMP in both the main and stable-3_3_0 branches. Could you have a look? Thanks!

PRs:

• ui-library:stable-3_3_0: pkp/pkp-lib#7900 Safari shows incorrect reviewer assignment information ui-library#199
• ui-library:main: pkp/pkp-lib#7900 Safari shows incorrect reviewer assignment information ui-library#200
• ojs:stable-3_3_0: Submodule update ##ewhanson/i7900_fix## ojs#3388
• ojs:main: Submodule update ##ewhanson/i7900_fix_main## ojs#3389
• omp:stable-3_3_0: Submodule update ##ewhanson/i7900_fix## omp#1111
• omp:main: Submodule update ##ewhanson/i7900_fix_main## omp#1112

[asmecher]

Thanks, @ewhanson! @NateWr, could you review the above PRs?

[edit: unless this was already sufficient? https://github.com/pkp/ui-library/pull/200#issuecomment-1118319912]

[NateWr]

Thanks @ewhanson. The solution looks good. Does this effect any use of new Date(some_variable), or just Date.parse(). I did a quick search and found a few other places where we do new Date(some_variable). Can you check those and see if they also need to be fixed for Safari?

[NateWr]

And if so, then we may want to roll fixDateForAllBrowsers into a global mixin.

[ewhanson]

Does this effect any use of new Date(some_variable), or just Date.parse().

Yes, I'll get the PRs updated to use fixDateForAllBrowsers as a global mixin and apply it to instances of new Date().

[ewhanson]

Hey @asmecher, in looking to address this for new Date(), I came across a few interesting things.

First, in the past week, I must have updated my version of macOS, because the most recent version of Safari (15.4) now has support for parsing dates in the 'YYYY-MM-DD' format. Because Safari updates are tied to OS updates, it's a bit hard to find an older version of Safari for testing. A good sign over all that it's been fixed, but not immediately helpful for users not on the latest version of Safari.

Second, there appears to be a difference in the way JS handles the different date string formats:

• new Date('2022-05-16') results in Sun May 15 2022 17:00:00 GMT-0700 (Pacific Daylight Saving Time) (midnight UTC offset to display the time with the browsers current time zone)
• new Date('2022/05/16') results in Mon May 16 2022 00:00:00 GMT-0700 (Pacific Daylight Saving Time) (midnight in the current zone (PDT in this case))

I need to do some more digging, but this doesn't look great. Ultimately, using the / between dates to work around a Safari Date.parse() issue looks like it could create a whole new set of problems.

A couple of questions:

1. Assuming new Date('2022/05/16') sets the time in the user's current time zone, will this create a problem for metrics? The JS doing this is only being used within the Vue components, but just wanted to see if there may be any issues here.2.
2. If new Date('2022/05/16') could be grabbing a different calendar date (e.g. May 15) than the information stored in the DB (e.g. May 16), will this cause issues and/or edge cases where info may be lost because of the way JS is evaluating a date string?

[asmecher]

@ewhanson, yuck. Is there official documentation for this behaviour? Maybe a polyfill or something 3rd party that we can rely on?

[ewhanson]

@asmecher, I'll do some digging and see what I turn up.

[ewhanson]

I couldn't find any Apple-specific documentation on how Safari parses date strings, but in general, MDN recommend against using new Date() or Date.parse() with date strings and kind of leave it at that. Improvements to these are coming via the Temporal API, but this is still a ways off. Long-term this looks like the way to go, but as of today, even the pollyfil is very experimental. In short, promising for the future, but it's not there yet.

In terms of 3rd party solutions, I came across what looks like the best candidate to me: Day.js. I found it via Moment.js's recommended alternatives. It's small (they say 2kb), has zero dependencies, and is an active project (latest version was released two weeks ago). This may still be overkill for our needs, though.

I tried installing it and inserting it into the Vue components, to see how it worked. It's usage would look something like dayjs.utc('2022-05-19').valueOf()which returns the Unix timestamp in milliseconds. This probably could replace our usage of the JS Date object, but it could also just be used to standardize date strings into Unix timestamps that can then be passed into new Date() or Date.parse() without problem. In my test, I tucked it away into a global mixin dateStringToMs. Whether we use this library or not, I think processing a date string to a standardized format that's consistent across browsers (Unix timestamp).

[asmecher]

I'd like @NateWr's sober second thought on this, but I like the library you've identified, and would prefer we rely on something like that than reinvent the wheel. Eventually we can move from there to the temporal API. 👍

[NateWr]

Thanks for digging into this, @ewhanson. On the library, we're already using moment.js, so if that provides a solution I'd suggest we go with it. (At least, it's a dependency of a dependency in our package-lock.json file. A good thing to do would be to check the build size before/after using it in a fix. If it doesn't change much, then we won't be adding to our build size by using it.)

new Date('2022-05-16') results in Sun May 15 2022 17:00:00 GMT-0700 (Pacific Daylight Saving Time) (midnight UTC offset to display the time with the browsers current time zone)

Maybe you can use Date.parse() with a timezone to force UTC timezone?

Date.parse('2022-05-16T00:00:01.000+00:00');

Assuming new Date('2022/05/16') sets the time in the user's current time zone, will this create a problem for metrics?

No, this only effects the dates that someone uses to request stats. It doesn't effect how stats are recorded.

If new Date('2022/05/16') could be grabbing a different calendar date (e.g. May 15) than the information stored in the DB (e.g. May 16), will this cause issues and/or edge cases where info may be lost because of the way JS is evaluating a date string?

I think we're ok here. The UI should always show the date that was requested, rather than getting stats to May 15 but displaying stats to May 16.

---

As I said, though, if moment.js provides a more robust solution here and it doesn't add to build size we can probably just make use of that.

[ewhanson]

Thanks @NateWr, I hadn't noticed we were already using moment.js (it looks like it was added as part of #5540). I checked the build size just to be safe and it was less than a kb of difference once I implemented my addition so that seems like the way to go. I'l get it tested out a bit more thoroughly and update the PRs.

[ewhanson]

Hey @asmecher, this should be good to go now. Could you have a look at the PRs? Thanks!

[asmecher]

Thanks, @ewhanson, I've merged those!