Bug 30514 - Error in date format check following datepicker removal
Summary: Error in date format check following datepicker removal
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
: 30561 (view as bug list)
Depends on: 30011
Blocks: 30673 30706
  Show dependency treegraph
 
Reported: 2022-04-12 14:20 UTC by Owen Leonard
Modified: 2023-12-28 20:42 UTC (History)
4 users (show)

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


Attachments
Bug 30514: Error in date format check following datepicker removal (5.30 KB, patch)
2022-04-12 14:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30514: Error in date format check following datepicker removal (5.36 KB, patch)
2022-04-20 03:57 UTC, David Cook
Details | Diff | Splinter Review
Bug 30514: Error in date format check following datepicker removal (5.41 KB, patch)
2022-05-06 08:36 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-04-12 14:20:27 UTC
calendar.inc still contains a reference to a variable which was removed by Bug 30011. This creates an error when using Flatpickr in some contexts.
Comment 1 Owen Leonard 2022-04-12 14:44:20 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2022-04-19 11:04:07 UTC
*** Bug 30561 has been marked as a duplicate of this bug. ***
Comment 3 David Cook 2022-04-20 03:57:36 UTC
Created attachment 133428 [details] [review]
Bug 30514: Error in date format check following datepicker removal

This patch corrects the is_valid_date function in the OPAC and staff
interface so that it work correctly with Flatpickr.

To test, apply the patch and clear your browser cache if necessary.

- In the staff client, locate a bibliographic record and edit its items.
- Test that the "Date acquired" date picker works correctly and that
  there are no errors in the browser console.
- Test other date input fields to check that they still work, e.g.
  specify due date during checkout; Catalog statistics wizard; Patron
  entry/modification, etc.
- Perform similar tests in the OPAC: Hold suspension; Update your
  personal details.

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 4 Katrin Fischer 2022-04-25 12:45:44 UTC
With different date formats, things don't appear quite right:

* Changed Dateformat DD.MM.YYYY
* Edit any patron
* Set or edit date of birth
* Verify the date is shown with / still, instead of selected dateformat
Comment 5 Owen Leonard 2022-04-26 12:45:08 UTC
In my testing today I found that it works but only after "restart_all." I don't know why this would be. Individually restarting plack, memcached, or apache doesn't help. Other system preferences, like IntranetUserCSS, are applied immediately without restart.
Comment 6 Jonathan Druart 2022-05-03 05:47:19 UTC
What Katrin describes is not related to this bug report. It's very weird and I have no idea where it's coming from.

I think there is still something wrong with this patch however. The is_valid_date does not work as we are expecting. We used to have an alert if the date was wrong.

The following change will restore the historical behaviour:

diff --git a/koha-tmpl/intranet-tmpl/prog/js/calendar.js b/koha-tmpl/intranet-tmpl/prog/js/calendar.js
index 81561cc42d2..8ea4fbadfe6 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/calendar.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/calendar.js
@@ -22,12 +22,7 @@ function is_valid_date(date) {
         if (date.search(/^\d{2}\.\d{2}\.\d{4}($|\s)/) == -1) return 0;
         dateformat = 'd.m.Y';
     }
-    try {
-        flatpickr.parseDate(date, dateformat);
-    } catch (e) {
-        return 0;
-    }
-    return 1;
+    return flatpickr.parseDate(date, dateformat) ? 1 : 0;
 }

 function get_dateformat_str(dateformat) {
Comment 7 Jonathan Druart 2022-05-03 05:55:47 UTC
(In reply to Jonathan Druart from comment #6)
> What Katrin describes is not related to this bug report. It's very weird and
> I have no idea where it's coming from.

Hum, mornings...

There is a bad space there:
  var dateformat_pref = "[% Koha.Preference('dateformat ') | html %]";

Not coming from this patch but bug 26261 (needs to be fixed separately).
Comment 8 Owen Leonard 2022-05-03 18:11:08 UTC
I think that within the scope of this bug report the patch should not be considered Failed QA.

I've posted Bug 30673 - Improve is_valid_date function for validating date strings - to address the issues raised in comment 4, comment 6, and comment 7. But none of those issues are related to the changes made by this patch.
Comment 9 Katrin Fischer 2022-05-06 08:07:36 UTC
Retesting this
Comment 10 Katrin Fischer 2022-05-06 08:18:47 UTC
Testing this again I noticed this in the console with the patch applied:

Source map error: Error: request failed with status 404
Resource URL: http://localhost:8081/intranet-tmpl/lib/flatpickr/shortcut-buttons-flatpickr.min_21.1200048.js
Source Map URL: shortcut-buttons-flatpickr.min.js.map

separate bug?
Comment 11 Katrin Fischer 2022-05-06 08:36:10 UTC
(In reply to Owen Leonard from comment #5)
> In my testing today I found that it works but only after "restart_all." I
> don't know why this would be. Individually restarting plack, memcached, or
> apache doesn't help. Other system preferences, like IntranetUserCSS, are
> applied immediately without restart.

I am seeing the same behavior, after a restart_all it's ok. I filed:

Bug 30706 - DateFormat change only takes effect after a restart of services
Comment 12 Katrin Fischer 2022-05-06 08:36:57 UTC
Created attachment 134683 [details] [review]
Bug 30514: Error in date format check following datepicker removal

This patch corrects the is_valid_date function in the OPAC and staff
interface so that it work correctly with Flatpickr.

To test, apply the patch and clear your browser cache if necessary.

- In the staff client, locate a bibliographic record and edit its items.
- Test that the "Date acquired" date picker works correctly and that
  there are no errors in the browser console.
- Test other date input fields to check that they still work, e.g.
  specify due date during checkout; Catalog statistics wizard; Patron
  entry/modification, etc.
- Perform similar tests in the OPAC: Hold suspension; Update your
  personal details.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 13 Fridolin Somers 2022-05-06 20:34:30 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄