Summary: | Improve is_valid_date function for validating date strings | ||
---|---|---|---|
Product: | Koha | Reporter: | Owen Leonard <oleonard> |
Component: | Templates | Assignee: | Owen Leonard <oleonard> |
Status: | RESOLVED MOVED | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | jonathan.druart |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31149 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 30514 | ||
Bug Blocks: | |||
Attachments: |
Bug 30673: Improve is_valid_date function for validating date strings
[DO NOT APPLY TO TEST FIRST PATCH] Bug 30673: Use moment.js to improve our date parsing Bug 30673: Improve is_valid_date function for validating date strings Bug 30673: Improve is_valid_date function for validating date strings |
Description
Owen Leonard
2022-05-03 16:22:49 UTC
Created attachment 134615 [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." - 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. 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. Well, no need to wait for dayjs, momentjs has the same syntax https://momentjs.com/docs/#/parsing/string-formats/ Created attachment 134735 [details] [review] [DO NOT APPLY TO TEST FIRST PATCH] Bug 30673: Use moment.js to improve our date parsing (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. 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." (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. Should we only test the second patch here? (In reply to Katrin Fischer from comment #9) > Should we only test the second patch here? Yes. 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. 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> (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. IMO we should go with bug 31149, not this patch. Should we mark this WONTFIX then or would this still be good for backports/older versions? (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. (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. |