Bug 29240 - Flatpickr - error in the console when a date is selected
Summary: Flatpickr - error in the console when a date is selected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29212 29241 30183
  Show dependency treegraph
 
Reported: 2021-10-14 08:22 UTC by Jonathan Druart
Modified: 2022-12-12 21:24 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:
21.11.00


Attachments
Bug 29240: Remove error in console when date is incorrect (1.54 KB, patch)
2021-10-14 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (42.98 KB, patch)
2021-10-14 14:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (43.20 KB, patch)
2021-10-14 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (43.03 KB, patch)
2021-10-14 15:16 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (43.25 KB, patch)
2021-10-14 15:33 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29240: (follow-up) Correct more instances (25.40 KB, patch)
2021-10-14 17:09 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 29240: Fix request.tt (4.18 KB, patch)
2021-10-15 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (43.31 KB, patch)
2021-10-15 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29240: (follow-up) Correct more instances (25.46 KB, patch)
2021-10-15 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29240: Fix request.tt (4.24 KB, patch)
2021-10-15 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29240: Rename start_for with date_to (41.57 KB, patch)
2021-10-19 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29240: Centralise from/to handling (43.46 KB, patch)
2021-10-19 12:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29240: (follow-up) Correct more instances (25.55 KB, patch)
2021-10-19 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29240: Rename start_for with date_to (40.66 KB, patch)
2021-10-19 12:09 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 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!