Bug 30673 - Improve is_valid_date function for validating date strings
Summary: Improve is_valid_date function for validating date strings
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on: 30514
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-03 16:22 UTC by Owen Leonard
Modified: 2022-08-09 09:12 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30673: Improve is_valid_date function for validating date strings (11.54 KB, patch)
2022-05-04 16:15 UTC, Owen Leonard
Details | Diff | Splinter Review
[DO NOT APPLY TO TEST FIRST PATCH] Bug 30673: Use moment.js to improve our date parsing (4.49 KB, patch)
2022-05-06 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30673: Improve is_valid_date function for validating date strings (10.33 KB, patch)
2022-05-06 14:07 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30673: Improve is_valid_date function for validating date strings (10.39 KB, patch)
2022-06-25 15:12 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2022-05-03 16:22:49 UTC
The is_valid_date() function in calendar.js has two jobs: Confirm that the date string matches the current date format according to a regular expression; and confirm that the date is an actual date. Both must be true.

For example:

"2002-03-14" matches the date format "Y-m-d" and is a valid date -- valid date.
"03/14/2002" doesn't match the date format "Y-m-d" even though it is a valid date -- invalid date.
"38-44-2103" doesn't match the date format "Y-m-d" and isn't a valid date -- invalid date.

is_valid_date() needs some changes in order for it to correctly handle custom date formats being specified which override the default date format (from the system preference).

It also needs a new way to check that a date string represents a valid date. The old jQueryUI method looks similar but behaves differently. $.datepicker.parseDate() would throw an exception if the date were invalid. 

flatpickr.parseDate() will try to interpret an invalid date as if it were valid.

For example:

flatpickr.parseDate("2002-48-66", "Y-m-d") -> Date Sat Feb 04 2006 00:00:00 
flatpickr.parseDate("2002-48-66", "m/d/Y") -> Date Mon Aug 02 2027 00:00:00
Comment 1 Owen Leonard 2022-05-04 16:15:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2022-05-06 12:07:26 UTC
-    var dateformat_pref = "[% Koha.Preference('dateformat ') | html %]";
+    var dateformat_pref = "[% Koha.Preference('dateformat') | html %]";

This needs to be moved to its own bug (for backport), see bug 30706.
Comment 3 Jonathan Druart 2022-05-06 12:33:18 UTC
1. 
-    var MSG_PLEASE_ENTER_A_VALID_DATE = ( __("Please enter a valid date (should match %s).") );
+    var MSG_PLEASE_ENTER_A_VALID_DATE = ( _("Please enter a valid date (should match %s).") );

Only at the OPAC, is that change expected? If so, why not at the intranet?

2. That's ugly. But we could go with this code for a quick fix.
A correct fix would be to switch to dayjs (see bug 30310) and use 
  dayjs('1970-00-00', 'YYYY-MM-DD', true).isValid()

See https://day.js.org/docs/en/parse/string-format
We should not need to deal with that by ourselves.
Comment 4 Jonathan Druart 2022-05-06 12:33:52 UTC
Well, no need to wait for dayjs, momentjs has the same syntax
  https://momentjs.com/docs/#/parsing/string-formats/
Comment 5 Jonathan Druart 2022-05-06 13:11:25 UTC
Created attachment 134735 [details] [review]
[DO NOT APPLY TO TEST FIRST PATCH] Bug 30673: Use moment.js to improve our date parsing
Comment 6 Jonathan Druart 2022-05-06 13:12:25 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 134735 [details] [review] [review]
> Bug 30673: Use moment.js to improve our date parsing

That's not working well enough but I think we should go into this direction.
Comment 7 Owen Leonard 2022-05-06 14:07:14 UTC
Created attachment 134740 [details] [review]
Bug 30673: Improve is_valid_date function for validating date strings

This patch corrects a couple of issues with our date validation in both
the staff interface and the OPAC. The "is_valid_date" function needs to
be able to accept a custom date format when one has been defined to
override the default Flatpickr configuration.

The function also needs to do better checking of the date itself
following the transition to Flatpickr. jQueryUI's parse method had
better built-in error handling, whereas Flatpickr's simply tries its
best to convert a string into a date. We can use the existing
Date_from_syspref function to check that the date string can be
converted to a valid JavaScript Date object.

To test, apply the patch and test date input fields in a few places in
the staff interface, e.g. the patron edit form, the circulation
statistics wizard, or the overdues page. Also check these specific
pages:

- With the dateformat system preference set to something other than
  "yyyy-mm-dd" open an item for editing. The "Date acquired" date picker
  should work correctly and fill the date in "yyyy-mm-dd" format
  (overriding the Flatpickr default).
- Check Serial -> Claims to confirm that the "To" and "From" filters
  work correctly.
- Test that various dateformat preference settings work in all cases.

In the OPAC, test that date picker inputs work correctly on pages like
the "Place hold" page, the "Your personal details" page, and "Your
summary" -> "Holds" -> "Suspend."
Comment 8 Owen Leonard 2022-05-06 14:09:23 UTC
(In reply to Jonathan Druart from comment #3)
> Only at the OPAC, is that change expected? If so, why not at the intranet?

The OPAC calendar.inc shouldn't use the double underscore because it is a template, not JavaScript. In the staff interface some calendar JS has been split out into a JS file, where the double underscore is correct.
Comment 9 Katrin Fischer 2022-06-06 21:15:39 UTC
Should we only test the second patch here?
Comment 10 Jonathan Druart 2022-06-07 09:44:17 UTC
(In reply to Katrin Fischer from comment #9)
> Should we only test the second patch here?

Yes.
Comment 11 Katrin Fischer 2022-06-25 15:11:53 UTC
I've tested the second patch and could not spot any issues, but there is a mismatch with the test plan:

- With the dateformat system preference set to something other than
  "yyyy-mm-dd" open an item for editing. The "Date acquired" date picker
  should work correctly and fill the date in "yyyy-mm-dd" format
  (overriding the Flatpickr default).

I was using DD.MM.YYYY and the date showed like this in the input field. Changing and storing a different format appeared to work well tho.
Comment 12 Katrin Fischer 2022-06-25 15:12:54 UTC
Created attachment 136552 [details] [review]
Bug 30673: Improve is_valid_date function for validating date strings

This patch corrects a couple of issues with our date validation in both
the staff interface and the OPAC. The "is_valid_date" function needs to
be able to accept a custom date format when one has been defined to
override the default Flatpickr configuration.

The function also needs to do better checking of the date itself
following the transition to Flatpickr. jQueryUI's parse method had
better built-in error handling, whereas Flatpickr's simply tries its
best to convert a string into a date. We can use the existing
Date_from_syspref function to check that the date string can be
converted to a valid JavaScript Date object.

To test, apply the patch and test date input fields in a few places in
the staff interface, e.g. the patron edit form, the circulation
statistics wizard, or the overdues page. Also check these specific
pages:

- With the dateformat system preference set to something other than
  "yyyy-mm-dd" open an item for editing. The "Date acquired" date picker
  should work correctly and fill the date in "yyyy-mm-dd" format
  (overriding the Flatpickr default).
- Check Serial -> Claims to confirm that the "To" and "From" filters
  work correctly.
- Test that various dateformat preference settings work in all cases.

In the OPAC, test that date picker inputs work correctly on pages like
the "Place hold" page, the "Your personal details" page, and "Your
summary" -> "Holds" -> "Suspend."

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Jonathan Druart 2022-07-13 07:05:12 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Jonathan Druart from comment #5)
> > Created attachment 134735 [details] [review] [review] [review]
> > Bug 30673: Use moment.js to improve our date parsing
> 
> That's not working well enough but I think we should go into this direction.

Moved to bug 31149.
Comment 14 Jonathan Druart 2022-07-13 07:06:22 UTC
IMO we should go with bug 31149, not this patch.
Comment 15 Katrin Fischer 2022-07-14 18:46:59 UTC
Should we mark this WONTFIX then or would this still be good for backports/older versions?
Comment 16 Jonathan Druart 2022-08-09 08:52:45 UTC
(In reply to Katrin Fischer from comment #15)
> Should we mark this WONTFIX then or would this still be good for
> backports/older versions?

I think so.
Comment 17 Jonathan Druart 2022-08-09 09:10:58 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Katrin Fischer from comment #15)
> > Should we mark this WONTFIX then or would this still be good for
> > backports/older versions?
> 
> I think so.

I think that we should go with bug 31149 and don't spend more time in this mess.