Patch to remove deprectated C4::Dates from circ/circulation.pl
Created attachment 42895 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl Patch to remove deprectated C4::Dates from circ/circulation.pl To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture')
Before applying the patch I search and found the followin lines: 169 if ($duedatespec =~ C4::Dates->regexp('syspref')) { 178 our $todaysdate = C4::Dates->new->output('iso'); I do not know if there should be these lines! If the intention in bug 14870 is to delete C4::Dates, this will launch an error or warning when the script will executed.
(In reply to Héctor Eduardo Castro Avalos from comment #2) > Before applying the patch I search and found the followin lines: > > 169 if ($duedatespec =~ C4::Dates->regexp('syspref')) { > 178 our $todaysdate = C4::Dates->new->output('iso'); > > I do not know if there should be these lines! > > If the intention in bug 14870 is to delete C4::Dates, this will launch an > error or warning when the script will executed. After applying the patch these lines continued there. The patch doesn't touch it.
Created attachment 42989 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl Patch to remove deprectated C4::Dates from circ/circulation.pl To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture') - Verify that due date works as before (with syspref 'SpecifyDueDate' on and off (Amended following comment #3)
Comment on attachment 42989 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl New patch will follow with C4/Dates.pm included.
Created attachment 43303 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) Patch to remove deprectated C4::Dates from: - circ/circulation.pl - C4/Circulation.pm - koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture') - Search for regressions - prove -v t/db_dependent/Circulation.t
*** Bug 7899 has been marked as a duplicate of this bug. ***
Still applies / auto-merging
Hi Marc, it works mostly well for me. Among other things I tried changing the date format in between and using wrong preset dates in the URL like circulation.pl?borrowernumber=51&duedatespec=10%2F30%2F2015+23%3A59 While the old version shows warnings or even breaks ("30" in the "month" position leads to a software error because it is not between 1 and 12), with your patch the wrong format passed by URL is displayed in the calendar, but no warnings or errors appear and the correct date is used when checking out. (In reply to Marc Véron from comment #6) > - Verify that data displays properly (including for users with a card > that expires in the near future, see syspref 'NotifyBorrowerDeparture') That breaks it for me: > Template process failed: undef error - KohaDates: filter not found at /home/mirko/koha/C4/Templates.pm line 121. As you seem to have tested this, is that taken care of by one of the other patches by any chance? I would sign off if it has a "depends on" that works.
There is bug 14985 labelled as "part one". Maybe C4/Templates.pm is in a part two not yet on BZ?
Hi Mirko, Thank you for the comments and for testing. Sorry, I have no time at teh moment to fined out what's wrong here; maybe I can come back to that on Sunday evening. I think the circulation files are the most sensible ones,maybe it is a good idea to fix them in one of the very last steps of this loooong journey. That said I really appreciate every sign-off and QA on the other C4::Dates Bugs. BTW: If anybody wants to take one of the C4::Dates bugs to help to get them through, I'll be very grateful. Marc
(In reply to Mirko Tietgen from comment #9) (...) > ....using wrong preset dates in the URL like > circulation.pl?borrowernumber=51&duedatespec=10%2F30%2F2015+23%3A59 > > While the old version shows warnings or even breaks ("30" in the "month" > position leads to a software error because it is not between 1 and 12), with > your patch the wrong format passed by URL is displayed in the calendar, but > no warnings or errors appear and the correct date is used when checking out. (...) MV: Found, will be fixed in amended patch: Date will not be displayed if params do not result in a correct date. > (In reply to Marc Véron from comment #6) > > > - Verify that data displays properly (including for users with a card > > that expires in the near future, see syspref 'NotifyBorrowerDeparture') > > That breaks it for me: > > > Template process failed: undef error - KohaDates: filter not found at /home/mirko/koha/C4/Templates.pm line 121. (...) MV: Found, was due to a typo, will be fixed in amended patch.
Created attachment 43958 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) Patch to remove deprectated C4::Dates from: - circ/circulation.pl - C4/Circulation.pm - koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture') - Search for regressions - prove -v t/db_dependent/Circulation.t (Amended following comment #9 / #11 25.10.2015 /mv)
Created attachment 44072 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) Patch to remove deprectated C4::Dates from: - circ/circulation.pl - C4/Circulation.pm - koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture') - Search for regressions - prove -v t/db_dependent/Circulation.t (Amended following comment #9 / #11 25.10.2015 /mv) Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment on attachment 44072 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) Review of attachment 44072 [details] [review]: ----------------------------------------------------------------- ::: circ/circulation.pl @@ +174,4 @@ > > my $stickyduedate = $query->param('stickyduedate') || $session->param('stickyduedate'); > my $duedatespec = $query->param('duedatespec') || $session->param('stickyduedate'); > +$duedatespec = eval { output_pref( { dt => dt_from_string( $duedatespec ), dateformat => 'iso' }); }; No, only if duedatespec if given. Otherwise "Automatic due date" is always filled with now. @@ +203,4 @@ > $datedue = output_pref({ dt => dt_from_string, dateonly => 1, dateformat => 'iso' }); > $datedue .= ' 23:59:00'; > } elsif( $duedatespec_allow ) { > + if ($datedue) { That's not the same code as before ($dateduespec vs $datedue), please double check.
Created attachment 44096 [details] [review] Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl Follow up to fix QA issues from comment #15
Jonathan, is it OK to switch back to Signed-off with the followup?
(In reply to Marc Véron from comment #17) > Jonathan, is it OK to switch back to Signed-off with the followup? Yes because the following is still wrong: $template->param( IMPOSSIBLE=>1, INVALID_DATE=>$datedue ) Test plan: Set and force an invalid date for the due date and confirm you get a warning with the invalid date displayed.
Created attachment 44131 [details] [review] Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl Follow up to fix QA issues from comment #15 ...and amended for comment #18
Hi Jonathan, I amended the patch. However, I can not trigger the message. The datepicker does not allow to enter wrong dates (field is readonly).
Created attachment 44137 [details] [review] Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) Patch to remove deprectated C4::Dates from: - circ/circulation.pl - C4/Circulation.pm - koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt To test: - Apply patch - Go to the checkout site (Home > Circulation > Checkouts) - Verify that data displays properly (including for users with a card that expires in the near future, see syspref 'NotifyBorrowerDeparture') - Search for regressions - prove -v t/db_dependent/Circulation.t (Amended following comment #9 / #11 25.10.2015 /mv) Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 44138 [details] [review] Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl Follow up to fix QA issues from comment #15 ...and amended for comment #18 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Patches pushed to master. Thanks Marc!