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
Created attachment 126259 [details] [review] Bug 29240: Remove error in console when date is incorrect
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.
(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.
Created attachment 126286 [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.
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.
Created attachment 126293 [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
Created attachment 126294 [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. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
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
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)
(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.
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.
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.
(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?
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.
(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.
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>
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>
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>
OK, I've dug into this a little and I think the hidden fields are entirely legacy stuff.. they're not referenced anywhere and don't have an input name at all so aren't even passed properly to the controller. I reckon they're safe to go, so I'm signing off.. all the functionality seems to work still.
Humm, ignore that last comment.. digging further..
Created attachment 126472 [details] [review] Bug 29240: Rename start_for with date_to
(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.
I agree with the rename :)
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
I agree with the rename!
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>
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>
Created attachment 126495 [details] [review] Bug 29240: Rename start_for with date_to Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(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.
Pushed to master for 21.11, thanks to everybody involved!