Bug 29240

Summary: Flatpickr - error in the console when a date is selected
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Staff interfaceAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: gmcharlt, kyle, martin.renvoize, nugged
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29270
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on:    
Bug Blocks: 29212, 29241, 30183    
Attachments: Bug 29240: Remove error in console when date is incorrect
Bug 29240: Centralise from/to handling
Bug 29240: Centralise from/to handling
Bug 29240: Centralise from/to handling
Bug 29240: Centralise from/to handling
Bug 29240: (follow-up) Correct more instances
Bug 29240: Fix request.tt
Bug 29240: Centralise from/to handling
Bug 29240: (follow-up) Correct more instances
Bug 29240: Fix request.tt
Bug 29240: Rename start_for with date_to
Bug 29240: Centralise from/to handling
Bug 29240: (follow-up) Correct more instances
Bug 29240: Rename start_for with date_to

Description Jonathan Druart 2021-10-14 08:22:49 UTC
It seems that there is no date validation. When a date is selected there is a worrying error in the console:

Error: Invalid date provided: Tue Oct 19 2021 00:00:00 GMT+0200 (Central European Summer Time)
    C http://kohadev-intra.mydnsname.org:8081/intranet-tmpl/lib/flatpickr/flatpickr.min_21.0600032.js:2
    is_valid_date http://kohadev-intra.mydnsname.org:8081/intranet-tmpl/prog/js/calendar_21.0600032.js:26
    validate_date http://kohadev-intra.mydnsname.org:8081/intranet-tmpl/prog/js/calendar_21.0600032.js:48
    onClose http://kohadev-intra.mydnsname.org:8081/cgi-bin/koha/reserve/request.pl?biblionumbers=14/:864
Comment 1 Jonathan Druart 2021-10-14 09:15:42 UTC
Created attachment 126259 [details] [review]
Bug 29240: Remove error in console when date is incorrect
Comment 2 Jonathan Druart 2021-10-14 09:17:06 UTC
Maybe this is the way, I am not sure.
But for sure, we must remove the onClose duplication code. All the flackpikr logic must be in calendar.inc.

We should start with centralizing the code.
Comment 3 Jonathan Druart 2021-10-14 09:42:10 UTC
(In reply to Jonathan Druart from comment #2)
> Maybe this is the way, I am not sure.
> But for sure, we must remove the onClose duplication code. All the flackpikr
> logic must be in calendar.inc.
> 
> We should start with centralizing the code.

See suggestion on bug 28942 comment 5.
Comment 4 Martin Renvoize 2021-10-14 14:07:42 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2021-10-14 14:09:00 UTC
I caught 90% of the cases here.. there are still a few lurking in the codebase..  What it highlighted to me is our inconsistency in places already.. futuredate isn't used consistently and is sometimes done locally instead of using a class for example.
Comment 6 Martin Renvoize 2021-10-14 14:33:36 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2021-10-14 15:16:34 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2021-10-14 15:33:36 UTC
Created attachment 126295 [details] [review]
Bug 29240: Centralise from/to handling

This patch moves to using a data-start_for attribute to point the 'from'
flatpickr to the 'to' flatpickr.

We also fix the date validation issue in the onClose handler inline.

Test plan.
1. Check that the from/to datpicker combinations work on each of the
   changed pages.
2. Look at the console for errors, there should be none

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2021-10-14 17:09:34 UTC
Created attachment 126301 [details] [review]
Bug 29240: (follow-up) Correct more instances

This patch fixes linked date fields in these pages:

 - Reports -> Patrons with the most checkouts
 - Reports -> Patrons statistics wizard
 - Reports -> Most-circulated items
 - Reports -> Catalog statistics wizard
 - Reports -> Guided reports
 - Reports -> Average loan time
 - Reports -> Holds statistics wizard
Comment 10 Jonathan Druart 2021-10-15 07:16:19 UTC
Thanks! That's much better and will be way easier to maintain!

1. Those 2 are missing:
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                    validate_date( selectedDates, instance );
koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt:                    validate_date( selectedDates, instance );

2. data-start_for reads weird to me, why not data-date_from_id? (I will amend the patches if we agree)
Comment 11 Jonathan Druart 2021-10-15 07:19:56 UTC
(In reply to Jonathan Druart from comment #10)
> 2. data-start_for reads weird to me, why not data-date_from_id? (I will
> amend the patches if we agree)

It's the reverse, data-date_to_id. Ok, nevermind.
Comment 12 Martin Renvoize 2021-10-15 07:25:26 UTC
I agree start_for is a bit weird.. but I struggled to come up with anything better.. I wanted to keep it fairly short but also clear.. totally open to suggestions.
Comment 13 Jonathan Druart 2021-10-15 07:50:45 UTC
Created attachment 126309 [details] [review]
Bug 29240: Fix request.tt

I have a doubt here, I was going to make the JS part retrieve the date
in ymd format, but realized that those inputs are not displayed when
trying to place holds for patron's clubs.
Comment 14 Jonathan Druart 2021-10-15 07:50:58 UTC
(In reply to Jonathan Druart from comment #13)
> Created attachment 126309 [details] [review] [review]
> Bug 29240: Fix request.tt
> 
> I have a doubt here, I was going to make the JS part retrieve the date
> in ymd format, but realized that those inputs are not displayed when
> trying to place holds for patron's clubs.

Can you confirm this?
Comment 15 Owen Leonard 2021-10-15 10:31:12 UTC
I deliberately omitted any change to request.tt because the Flatpickr instantiation was already being modified with an onChange event. I didn't see any advantage to changing it.
Comment 16 Jonathan Druart 2021-10-15 11:56:50 UTC
(In reply to Owen Leonard from comment #15)
> I deliberately omitted any change to request.tt because the Flatpickr
> instantiation was already being modified with an onChange event. I didn't
> see any advantage to changing it.

We can leave the hard ones for a separate bug reports, but I think we should aim removing them all for 21.11.
Comment 17 Martin Renvoize 2021-10-15 14:20:44 UTC
Created attachment 126341 [details] [review]
Bug 29240: Centralise from/to handling

This patch moves to using a data-start_for attribute to point the 'from'
flatpickr to the 'to' flatpickr.

We also fix the date validation issue in the onClose handler inline.

Test plan.
1. Check that the from/to datpicker combinations work on each of the
   changed pages.
2. Look at the console for errors, there should be none

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-10-15 14:20:48 UTC
Created attachment 126342 [details] [review]
Bug 29240: (follow-up) Correct more instances

This patch fixes linked date fields in these pages:

 - Reports -> Patrons with the most checkouts
 - Reports -> Patrons statistics wizard
 - Reports -> Most-circulated items
 - Reports -> Catalog statistics wizard
 - Reports -> Guided reports
 - Reports -> Average loan time
 - Reports -> Holds statistics wizard

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-10-15 14:20:52 UTC
Created attachment 126343 [details] [review]
Bug 29240: Fix request.tt

I have a doubt here, I was going to make the JS part retrieve the date
in ymd format, but realized that those inputs are not displayed when
trying to place holds for patron's clubs.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-10-15 14:22:16 UTC Comment hidden (obsolete)
Comment 21 Martin Renvoize 2021-10-15 14:50:06 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2021-10-19 07:57:49 UTC
Created attachment 126472 [details] [review]
Bug 29240: Rename start_for with date_to
Comment 23 Jonathan Druart 2021-10-19 07:58:28 UTC
(In reply to Jonathan Druart from comment #22)
> Created attachment 126472 [details] [review] [review]
> Bug 29240: Rename start_for with date_to

Martin, Owen, if we agree on that I will PQA and push these patches.
Comment 24 Martin Renvoize 2021-10-19 10:04:13 UTC
I agree with the rename :)
Comment 25 Martin Renvoize 2021-10-19 10:05:20 UTC
But I think I decided we should move the request.tt change to another bug for testing as I'm not 100% confident in it at last look
Comment 26 Owen Leonard 2021-10-19 10:22:32 UTC
I agree with the rename!
Comment 27 Jonathan Druart 2021-10-19 12:08:56 UTC
Created attachment 126493 [details] [review]
Bug 29240: Centralise from/to handling

This patch moves to using a data-start_for attribute to point the 'from'
flatpickr to the 'to' flatpickr.

We also fix the date validation issue in the onClose handler inline.

Test plan.
1. Check that the from/to datpicker combinations work on each of the
   changed pages.
2. Look at the console for errors, there should be none

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2021-10-19 12:09:01 UTC
Created attachment 126494 [details] [review]
Bug 29240: (follow-up) Correct more instances

This patch fixes linked date fields in these pages:

 - Reports -> Patrons with the most checkouts
 - Reports -> Patrons statistics wizard
 - Reports -> Most-circulated items
 - Reports -> Catalog statistics wizard
 - Reports -> Guided reports
 - Reports -> Average loan time
 - Reports -> Holds statistics wizard

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2021-10-19 12:09:06 UTC
Created attachment 126495 [details] [review]
Bug 29240: Rename start_for with date_to

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2021-10-19 12:12:25 UTC
(In reply to Martin Renvoize from comment #25)
> But I think I decided we should move the request.tt change to another bug
> for testing as I'm not 100% confident in it at last look

Will be moved to bug 29270.
Comment 31 Jonathan Druart 2021-10-19 13:31:22 UTC
Pushed to master for 21.11, thanks to everybody involved!