Skip to content

Trim date strings for the date picker#2258

Merged
pstaabp merged 4 commits into
openwebwork:developfrom
drgrice1:bugfix/datetrim
Nov 20, 2023
Merged

Trim date strings for the date picker#2258
pstaabp merged 4 commits into
openwebwork:developfrom
drgrice1:bugfix/datetrim

Conversation

@drgrice1

Copy link
Copy Markdown
Member

When the date picker attempts to convert a date string into a numeric timestamp, trim the string. Otherwise the luxon.DateTime.fromFormat method fails to parse the string into a numeric timestamp.

This most likely fixes issue #2257.

When the date picker attempts to convert a date string into a numeric
timestamp, trim the string.  Otherwise the `luxon.DateTime.fromFormat`
method fails to parse the string into a numeric timestamp.

This most likely fixes issue openwebwork#2257.
@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

This doesn't appear to fix the issue. I have ran npm ci and verified that trim() is part of the minified javascript that is being loaded. I have checked the issue occurs in both firefox-esr and chromium.

A little more fiddling around, if I delete the space between the time and AM or PM and add it back, it works as expected, so wonder if it is an encoding issue and the space between the time and AM/PM isn't being registered correctly. Also when I look at the date, I think you can see a difference in the size of the space between the date and time, and the space between the time and AM/PM (screenshot attached) -- though maybe I'm just seeing things.

image

@drgrice1

Copy link
Copy Markdown
Member Author

I am unable to reproduce what you are seeing in either Firefox or Chrome. If there is are spaces before or after the time, then I see the issue without this pull request though.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

I wonder if it is a translations issue now, if I go change the translation to Spanish, I notice that the input box of the set dates and the override dates are formatted differently (there is a comma in the set dates but not the override dates).

image

So using Spanish I'm unable to copy/paste the dates, but if I use German it works fine.

This is the match the Perl Spanish date time format.
@drgrice1

Copy link
Copy Markdown
Member Author

The date/time format for Spanish in the javascript did not match the Perl date/time format. I fixed that. So that will fix the issue for Spanish.

@drgrice1

Copy link
Copy Markdown
Member Author

That doesn't explain your AM/PM comments.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Yea, it is odd, I'm now fairly certain it is due to a space character being different. Here is a screenshot between a correctly formatted time and the one in the set input box. Do you also see a difference between the size of the space between the time an AM?

image

Maybe it is some issue with the locales on my server.

@Alex-Jordan

Copy link
Copy Markdown
Contributor

When I copy-pasted a date into:
https://www.babelstone.co.uk/Unicode/whatisit.html
I do not get a thinspace. It's just a regular space for me.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Thanks for the link @Alex-Jordan, this is what I get when I copy/paste the date into that link.

U+0031 : DIGIT ONE
U+0031 : DIGIT ONE
U+002F : SOLIDUS {slash, forward slash, virgule}
U+0036 : DIGIT SIX
U+002F : SOLIDUS {slash, forward slash, virgule}
U+0032 : DIGIT TWO
U+0033 : DIGIT THREE
U+002C : COMMA {decimal separator}
U+0020 : SPACE [SP]
U+0035 : DIGIT FIVE
U+003A : COLON
U+0030 : DIGIT ZERO
U+0030 : DIGIT ZERO
U+202F : NARROW NO-BREAK SPACE [NNBSP]
U+0050 : LATIN CAPITAL LETTER P
U+004D : LATIN CAPITAL LETTER M

For some reason my server is using the NARROW NO-BREAK SPACE [NNBSP] for that space.

@drgrice1

Copy link
Copy Markdown
Member Author

Note that there are some issues with time formats in some of the languages. The intent of this pull request was to fix your original issue which was in the US time format.

In Korean there is no way to make the Perl and Javascript formats match. Perl for some reason does not interpret the AM/PM, while javascript does. So the Perl format gives 23. 1. 1. AM 2:00, while the Javascript format gives 23. 1. 1. 오전 2:00, and there isn't much that can be done about that other than fix the Perl DateTime library that we use.

The Greek language has another issue. Flatpickr uses an invalid language code (gr) for Greek, while we use the correct language code (el). As such, the Flatpickr localization file fails to load.

I really, can not reproduce what you are seeing with the AM/PM thing. It could be the version of the Perl DateTime module on your system. What version of that module do you have installed?

@drgrice1

Copy link
Copy Markdown
Member Author

Also check the version of the Date::Format module.

@drgrice1

Copy link
Copy Markdown
Member Author

On my system I have version 1.55 of DateTime, and 2.24 of Date::Format (although Date::Format is not actually used for this now that I look closer). Probably the module that is most relevant is DateTime::Locale. Check that version. That is 1.33 on my system.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Here are the versions of the libraries in Debian stable (bookworm): Date::Format is 2.33 and DateTime is 1.59.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Oh missed one, DateTime::Locale is 1.37.

The most relevant thing I see in the changelog in 1.37 is "Rebuilt all locale data with the data from CLDR 42.0.0". Maybe there was an update to using that narrow space for AM/PM there.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Okay, I just verified it is due to that change in 1.37, as seems the new time specification wants to use a narrow space. I downgraded to 1.35 and I no longer see the issue.

Seems like this is something we should prepare for as more and more servers upgrade their DateTime::Locale module.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Also found this, seems Java knows about this already and has a note to expect some issues.

https://inside.java/2023/03/28/quality-heads-up/

@drgrice1

Copy link
Copy Markdown
Member Author

This now also replaces narrow no-break spaces with regular spaces. So it will work with newer versions of DateTime::Format.

@somiaj

somiaj commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

@drgrice1 Any reason you are only updated datepicker.js for the last commit and not problemsetlist.js like the other two commits?

I can confirm this works and fixes my issue. Thanks.

@drgrice1

Copy link
Copy Markdown
Member Author

Yeah, I forgot. I will update that when I get a chance.

This will be an issue with newer versions of DateTime::Format that are
built on newer versions of CLDR that use these types of spaces in some
formats.
@drgrice1

Copy link
Copy Markdown
Member Author

I fixed the other location. I decided to also fix the Greek language for this while I was at it. When the Greek language support was added to webwork, a date/time format for the javascript was not. Also, we need to work around flatpickr's incorrect gr language code.

@drgrice1 drgrice1 force-pushed the bugfix/datetrim branch 2 times, most recently from 7cb62cf to f65bb0c Compare November 18, 2023 00:28

@pstaabp pstaabp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested for a few languages.

There are two issues here.  First, the Greek language was added
recently, but a date/time format for the datepicker javascript was not
added.  Second, the flatpickr library incorrectly names the locale `gr`
instead of `el`.  So that has to be worked around.

@somiaj somiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates. Works well now.

Comment thread htdocs/js/DatePicker/datepicker.js Outdated
'cs-CZ': 'dd.LL.yy H:mm',
de: 'dd.LL.yy, HH:mm',
es: 'd/L/yy H:mm',
el: 'd/L/yy, h::mm a',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like you have a second : here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also in problemset.js. When testing this, perl only adds a single :.

@pstaabp pstaabp merged commit a11a2de into openwebwork:develop Nov 20, 2023
@drgrice1 drgrice1 deleted the bugfix/datetrim branch November 20, 2023 23:07
@drgrice1 drgrice1 restored the bugfix/datetrim branch November 21, 2023 16:34
@drgrice1 drgrice1 deleted the bugfix/datetrim branch November 21, 2023 16:34
pstaabp added a commit that referenced this pull request Nov 27, 2023
Trim date strings for the date picker and such (hotfix of #2258)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants