Bug 28351 - Cannot set restrictions when 'dateformat' is other than 'us'
Summary: Cannot set restrictions when 'dateformat' is other than 'us'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 26261
Blocks: 28538
  Show dependency treegraph
 
Reported: 2021-05-13 18:05 UTC by Tomás Cohen Arazi
Modified: 2021-06-09 17:59 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 28351: Update to latest jquery-ui-timepicker-addon (79.15 KB, patch)
2021-05-15 12:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28351: Add v1.6.3 CSS and update templates (5.91 KB, patch)
2021-05-17 18:45 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 28351: Update to latest jquery-ui-timepicker-addon (79.21 KB, patch)
2021-05-17 19:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 28351: Add v1.6.3 CSS and update templates (5.96 KB, patch)
2021-05-17 19:08 UTC, David Nind
Details | Diff | Splinter Review
[ALTERNATIVE] Bug 28351: (bug 26261) Fix datepicker for dateformat ne mm/dd/yyyy (976 bytes, patch)
2021-05-18 07:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28351: (bug 26261) Fix datepicker for dateformat ne mm/dd/yyyy (1.46 KB, patch)
2021-05-19 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-05-13 18:05:50 UTC
Format is not correctly applied, so any date that doesn't parse well as mm/dd/yyyy is reset to current date.

To reproduce:
0. Set dateformat to mm/dd/yyyy
1. Go to a patron detail page
2. Open the 'Restrictions' tab
3. Choose a future date in which the day is higher than 12
=> SUCCESS: The input has the date we chose
4. Set dateformat to dd/mm/yyyy
5. Repeat 3
=> FAIL: It sets the current date, the browser console shows an error.
Comment 1 Tomás Cohen Arazi 2021-05-14 19:12:16 UTC
Certainly caused by 26261.
Comment 2 Tomás Cohen Arazi 2021-05-15 12:32:43 UTC
Created attachment 121017 [details] [review]
Bug 28351: Update to latest jquery-ui-timepicker-addon

This patch updates to the latest version of the library. It fixes the
collision there seems to exist between our version of JQuery, JQuery-UI
and the datetime plugin.

Note: It feels like we should get rid of this library as this version
dates back to 2016. Our previous version (1.4.3) was dated November
2013. The author recomends moving to https://flatpickr.js.org/

Anyway, this solves our urgent issues.
To test:
1. Have dateformat set to something other than 'us' (I picked
   dd/mm/yyyy).
2. Open your browser inspector in the console
3. Find a patron on the staff interface
4. Try tadding a restriction dated at some date in which the day number
   is higher than 12
=> FAIL: It puts the current day (as a fallback), an error about the
passed date being invalid is reported in the console
5. Apply this patch and reload all the things, just in case
6. Repeat 4
=> SUCCESS: It picks the date, no noticeable problems
7. Sign off :-D
Comment 3 Tomás Cohen Arazi 2021-05-15 12:34:17 UTC
This solves the conflict, but the time selector now has milliseconds and timezone selection. I don't have the time to dig into it as the kids are up early this saturday. For that reason I don't mark it NSO.
Comment 4 David Nind 2021-05-16 09:58:53 UTC
I have tested and everything works as per the test plan.
Comment 5 Lucas Gass 2021-05-17 18:43:16 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> This solves the conflict, but the time selector now has milliseconds and
> timezone selection. I don't have the time to dig into it as the kids are up
> early this saturday. For that reason I don't mark it NSO.

If we use v1.6.3 for the timepicker than I believe we need to add the v1.6.3 minified CSS. Then it must be called on the templates that use the timepicker:

git grep 'timepicker.inc':
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
koha-tmpl/intranet-tmpl/prog/en/modules/circ/offline.tt
koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt
koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Comment 6 Lucas Gass 2021-05-17 18:45:21 UTC
Created attachment 121068 [details] [review]
Bug 28351: Add v1.6.3 CSS and update templates
Comment 7 Tomás Cohen Arazi 2021-05-17 19:07:11 UTC
(In reply to Lucas Gass from comment #5)
> (In reply to Tomás Cohen Arazi from comment #3)
> > This solves the conflict, but the time selector now has milliseconds and
> > timezone selection. I don't have the time to dig into it as the kids are up
> > early this saturday. For that reason I don't mark it NSO.
> 
> If we use v1.6.3 for the timepicker than I believe we need to add the v1.6.3
> minified CSS. Then it must be called on the templates that use the
> timepicker:
> 
> git grep 'timepicker.inc':
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/offline.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/renew.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt

Great job, Lucas. Thanks!
Comment 8 David Nind 2021-05-17 19:08:07 UTC
Created attachment 121069 [details] [review]
Bug 28351: Update to latest jquery-ui-timepicker-addon

This patch updates to the latest version of the library. It fixes the
collision there seems to exist between our version of JQuery, JQuery-UI
and the datetime plugin.

Note: It feels like we should get rid of this library as this version
dates back to 2016. Our previous version (1.4.3) was dated November
2013. The author recomends moving to https://flatpickr.js.org/

Anyway, this solves our urgent issues.
To test:
1. Have dateformat set to something other than 'us' (I picked
   dd/mm/yyyy).
2. Open your browser inspector in the console
3. Find a patron on the staff interface
4. Try tadding a restriction dated at some date in which the day number
   is higher than 12
=> FAIL: It puts the current day (as a fallback), an error about the
passed date being invalid is reported in the console
5. Apply this patch and reload all the things, just in case
6. Repeat 4
=> SUCCESS: It picks the date, no noticeable problems
7. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-05-17 19:08:10 UTC
Created attachment 121070 [details] [review]
Bug 28351: Add v1.6.3 CSS and update templates

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Jonathan Druart 2021-05-18 07:11:43 UTC
Created attachment 121080 [details] [review]
[ALTERNATIVE] Bug 28351: (bug 26261) Fix datepicker for dateformat ne mm/dd/yyyy

This patch restores the same code we have prior to bug 26261
Comment 11 Jonathan Druart 2021-05-18 07:13:12 UTC
Tomas, why did you take the hard path?

This patch uses the same code for both "on change" event and restores what was done prior to bug 26261. It fixes the problem and seems way easier for backport.

What do you think?
Comment 12 Tomás Cohen Arazi 2021-05-18 10:11:24 UTC
(In reply to Jonathan Druart from comment #11)
> Tomas, why did you take the hard path?
> 
> This patch uses the same code for both "on change" event and restores what
> was done prior to bug 26261. It fixes the problem and seems way easier for
> backport.
> 
> What do you think?

Basically, because I saw how old the library was. I'm fine with either solution.
Comment 13 Jonathan Druart 2021-05-18 10:25:49 UTC
I think we should pick the small patch for backport, then test carefully the version change (like during 21.06).
Comment 14 Tomás Cohen Arazi 2021-05-18 13:39:47 UTC
(In reply to Jonathan Druart from comment #13)
> I think we should pick the small patch for backport, then test carefully the
> version change (like during 21.06).

Yeah, if we are going to spend time testing side-effects from changing the library we should just move into flatpickr or similar.
Comment 15 Tomás Cohen Arazi 2021-05-18 16:43:37 UTC
Lucas and I have tried to SO this patch, but:


$ git bz edit 28351
Traceback (most recent call last):
  File "/usr/local/bin/git-bz", line 2705, in <module>
    do_edit(*args)
  File "/usr/local/bin/git-bz", line 2377, in do_edit
    edit_bug(bug)
  File "/usr/local/bin/git-bz", line 2181, in edit_bug
    lines = edit_template(template.getvalue())
  File "/usr/local/bin/git-bz", line 820, in edit_template
    f.write(template)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xa0' in position 703: ordinal not in range(128)

Not sure what's going on with BZ. If you can, Jonathan, please stamp our signatures on pushing.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 16 Jonathan Druart 2021-05-19 12:32:35 UTC
Created attachment 121163 [details] [review]
Bug 28351: (bug 26261) Fix datepicker for dateformat ne mm/dd/yyyy

This patch restores the same code we have prior to bug 26261

0. Set dateformat to mm/dd/yyyy
1. Go to a patron detail page
2. Open the 'Restrictions' tab
3. Choose a future date in which the day is higher than 12
=> SUCCESS: The input has the date we chose
4. Set dateformat to dd/mm/yyyy
5. Repeat 3
=> FAIL: It sets the current date, the browser console shows an error.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 17 Jonathan Druart 2021-05-19 12:33:39 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> Lucas and I have tried to SO this patch, but:
> 
> 
> $ git bz edit 28351
> Traceback (most recent call last):
>   File "/usr/local/bin/git-bz", line 2705, in <module>
>     do_edit(*args)
>   File "/usr/local/bin/git-bz", line 2377, in do_edit
>     edit_bug(bug)
>   File "/usr/local/bin/git-bz", line 2181, in edit_bug
>     lines = edit_template(template.getvalue())
>   File "/usr/local/bin/git-bz", line 820, in edit_template
>     f.write(template)
> UnicodeEncodeError: 'ascii' codec can't encode character u'\xa0' in position
> 703: ordinal not in range(128)
> 
> Not sure what's going on with BZ. If you can, Jonathan, please stamp our
> signatures on pushing.


For the record, there was a non-breaking space after "[ALTERNATIVE]". Sorry about that.
Comment 18 Jonathan Druart 2021-05-19 13:08:29 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2021-05-20 07:21:52 UTC
Pushed to 20.11.x for 20.11.06
Comment 20 Andrew Fuerste-Henry 2021-05-25 14:03:19 UTC
Pushed to 20.05.x for 20.05.12
Comment 21 Victor Grousset/tuxayo 2021-05-25 14:52:29 UTC
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.