Description
Kyle M Hall (khall)
2013-08-07 15:02:01 UTC
Created attachment 20146 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply the patch for bug 8769 2) Apply this patch 3) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 4) Run fines.pl to generate the fine 5) Browse to returns.pl 6) Specify a return date of the day after the specified due date 7) Check the borrowers issue history, you should see the backdated return date, rather than today's date 8) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Created attachment 20147 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply the patch for bug 8769 2) Apply this patch 3) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 4) Run fines.pl to generate the fine 5) Browse to returns.pl 6) Specify a return date of the day after the specified due date 7) Check the borrowers issue history, you should see the backdated return date, rather than today's date 8) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Barbara Knibbs from Farmington Library has tested this patch. It works great, and I would like to sign off on it. Created attachment 21094 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 21095 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 21096 [details] [review] Bug 10694 - Allow arbitrary backdating of returns QA comment: It works as described but it does not deal with the debarred date. I don't know how to do but I think the patch should manage it. Marked as Passed QA. (In reply to Jonathan Druart from comment #7) > QA comment: > > It works as described but it does not deal with the debarred date. > I don't know how to do but I think the patch should manage it. > > Marked as Passed QA. Jonathan, can you please explain what you mean when you write you think this patch should managed the debarred date? I have no idea what you might mean be that. (In reply to Kyle M Hall from comment #8) > (In reply to Jonathan Druart from comment #7) > > QA comment: > > > > It works as described but it does not deal with the debarred date. > > I don't know how to do but I think the patch should manage it. > > > > Marked as Passed QA. > > Jonathan, can you please explain what you mean when you write you think this > patch should managed the debarred date? I have no idea what you might mean > be that. You had rectified, it was not "Passed QA", but "Failed QA". In fact it is especially for discussion, I will try to explain: If I specify a hard due date with now - 1 day and I check in today, the patron is debarred until now + 3 days (in the issuing rules: Suspension in days(=3) * 1). If I specify a hard due date with now - 30 days and I specify a return date of the day after (now - 29 days), what about the suspension delay? Currently, the patron is suspended until now + 29*3 days. I am not sure it is relevant. Hm so I think the point is that when you backdate a return, not only the fine needs to be adjusted, but also the 'fines in days' that is how many days the user is debarred? Jonathan, can you point me to the code that is currently accomplishing this? I'm afraid I'm having a difficult time locating it. Thanks!
> In fact it is especially for discussion, I will try to explain:
> If I specify a hard due date with now - 1 day and I check in today, the
> patron is debarred until now + 3 days (in the issuing rules: Suspension in
> days(=3) * 1).
> If I specify a hard due date with now - 30 days and I specify a return date
> of the day after (now - 29 days), what about the suspension delay?
> Currently, the patron is suspended until now + 29*3 days.
> I am not sure it is relevant.
(In reply to Kyle M Hall from comment #11) > Jonathan, can you point me to the code that is currently accomplishing this? > I'm afraid I'm having a difficult time locating it. Thanks! I suppose it is in the C4::Circulation::_debar_user_on_return routine. We have been discussing this issue at the KohaCon 2013 hackfest, and have come to the conclusion that we cannot safely reduce the number of punitive restriction days automatically. We believe the best solution will be to display a warning to the librarian such that if a return is being backdated, and the patron is restricted, the librarian will recieve a message to check to make sure the punitive restricted days is correct and does not need to be adjusted manually. Example : I've 3 items late, i've been suspended by overdues script. I check-in only 2 => I must still be suspended. There can be other reasons. So I agree we can't unsuspend safely. Jonathan, about suspensions in days, they're calculated when the item is checked-in. Do you imply there's a problem in this case ? (I haven't checked) Created attachment 22122 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Created attachment 22123 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Hi, starting on this with the QA script and a code review: 1) QA script points out 2 small problems: FAIL Koha/Template/Plugin/Borrowers.pm OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 9) OK valid OK critic FAIL circ/returns.pl OK pod FAIL forbidden patterns forbidden pattern: tab char (line 271) 2) There is no documentation/POD for using the new TT plugin borrowers.pm 3) Would it be possible to set the date field in the datepicker not to read only? This has proven an accessiblity probem in circulationf or one of our libraries. (bug 10995) I also have started running some first tests, currently wondering a bit if the debarmentwill work with the new 2720 development and running into some problems there. Failing QA to get Kyle's attention. Created attachment 23672 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Knibbs <BKnibbs@farmingtonlibraries.org> Created attachment 23673 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Created attachment 23697 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 > 1) QA script points out 2 small problems: Fixed! > 2) There is no documentation/POD for using the new TT plugin borrowers.pm Fixed! > 3) Would it be possible to set the date field in the datepicker not to read > only? This has proven an accessiblity probem in circulationf or one of our > libraries. > (bug 10995) Fixed! > I also have started running some first tests, currently wondering a bit if > the debarment will work with the new 2720 development and running into some > problems there. I don't think there will be any issues with the new debarment system, as the borrowers.debarred field was retained for compatibility. Working on this now... Some notes: 1) The feature is activated by default on updated installations. I think it would be better to only activate it on new installations. 2) The return date defaults to "today 23:59". Wouldn't it make more sense to default to "yesterday 23:95"? And consequently, should we allow setting return dates in the future? 3) If you use the date the plugin suggests, check the checkbox for keeping the date and then submit the form with empty barcode or return a book, following error occurs: Template process failed: undef error - Can't use string ("") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/Koha/Template/Plugin/Borrowers.pm line 47. It also happens with a return date in the past, book was not checked out. X) We have the same problem here as we have on bug 10995. For accessibility reasons we should allow manual entry of the date and time, but we need to catch the errors. If you enter an invalid date now, you get a nasty perl error. I got: Can't call method "ymd" on an undefined value at /usr/share/perl5/DateTime/Format/MySQL.pm line 84. Failed for 2) and 3). X) is kind of a difficult one. I really think we need a general solution here, but not sure how to achieve it. Not going to fail for that right now, but welcoming ideas. Created attachment 25658 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Knibbs <BKnibbs@farmingtonlibraries.org> Created attachment 25659 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Created attachment 25660 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 Created attachment 25661 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup 3 Created attachment 26162 [details] [review] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Knibbs <BKnibbs@farmingtonlibraries.org> Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com> Created attachment 26163 [details] [review] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 Bug 10694 - Allow arbitrary backdating of returns - QA Followup 3 Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com> Tested, works according to the test plan - it deletes the fine when backdating. Comment: I found that the datetime in the statistics table is set to todays date, and not the backtracked one. Should it? I have no opinion about it myself. Created attachment 26707 [details] [review] [PASSED QA] Bug 10694 - Allow arbitrary backdating of returns Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly. Test Plan: 1) Apply this patch 2) Check out an item, and backdate the due date by 1 month or so * This issue needs to generate a fine 3) Run fines.pl to generate the fine 4) Browse to returns.pl 5) Specify a return date of the day after the specified due date 6) Check the borrowers issue history, you should see the backdated return date, rather than today's date 7) Check the fine, it should be reduced to a fine for a single day overdue, rather than the previous larger fine. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Knibbs <BKnibbs@farmingtonlibraries.org> Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, passes tests and QA script. Created attachment 26708 [details] [review] [PASSED QA] Bug 10694 - Allow arbitrary backdating of returns - QA Followup Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 Bug 10694 - Allow arbitrary backdating of returns - QA Followup 3 Signed-off-by: Petter Goksoyr Asen <boutrosboutrosboutros@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Couldn't find any critical problems with this, but still some ideas for possible enhancements: - It would be nice there was some feedback about a reduced fine when returning the item - Also, currently the return date is not visible in the returns table, so you can't check which date you used for returning something. I'd suggest adding the return date when the specifyreturndate feature is enabled. Hm, I think I missed something here - we should have some unit tests for the changes to AddReturn *** Bug 8338 has been marked as a duplicate of this bug. *** This changes the api of a routine used in many places so the change should be clearer. The return_date is an optional parameter but this is not indicated in the documentation of the subroutine, a comment or even explicitly in the code, being handled as a side effect of the date routines. Instead of replacing my $today = DateTime->now( time_zone => C4::Context->tz() ); by my $today = dt_from_string( $return_date, 'iso' ); can I suggest that: my $today; if ($return_date) { $today = dt_from_string( $return_date, 'iso' ); } else { $today = DateTime->now( time_zone => C4::Context->tz() ); } makes the logic a bit clearer without forcing the reader to inspect the DateUtils code to see that having a return_date is not required Colin, there is no place in these patches where I make a change like that. (In reply to Colin Campbell from comment #38) > This changes the api of a routine used in many places so the change should > be clearer. The return_date is an optional parameter but this is not > indicated in the documentation of the subroutine, a comment or even > explicitly in the code, being handled as a side effect of the date routines. > > Instead of replacing > my $today = DateTime->now( time_zone => C4::Context->tz() ); > by > my $today = dt_from_string( $return_date, 'iso' ); > > can I suggest that: > my $today; > if ($return_date) { > $today = dt_from_string( $return_date, 'iso' ); > } > else { > $today = DateTime->now( time_zone => C4::Context->tz() ); > } > > makes the logic a bit clearer without forcing the reader to inspect the > DateUtils code to see that having a return_date is not required Created attachment 27461 [details] [review] Bug 10694 [QA Followup] - Unit Tests, improve AddReturn POD Setting to passed qa on the assumption that the unit test was the final blocker for this one. If am incorrect, please update the status. Created attachment 27475 [details] [review] [SIGNED OFF] [Bug 10694 [QA Followup] - Unit Tests, improve AddReturn POD Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Kyle M Hall from comment #39) > Colin, there is no place in these patches where I make a change like that. > > (In reply to Colin Campbell from comment #38) > > This changes the api of a routine used in many places so the change should > > be clearer. The return_date is an optional parameter but this is not > > indicated in the documentation of the subroutine, a comment or even > > explicitly in the code, being handled as a side effect of the date routines. > > > > Instead of replacing > > my $today = DateTime->now( time_zone => C4::Context->tz() ); > > by > > my $today = dt_from_string( $return_date, 'iso' ); > > > > can I suggest that: > > my $today; > > if ($return_date) { > > $today = dt_from_string( $return_date, 'iso' ); > > } > > else { > > $today = DateTime->now( time_zone => C4::Context->tz() ); > > } > > > > makes the logic a bit clearer without forcing the reader to inspect the > > DateUtils code to see that having a return_date is not required Sorry its in 8769 ... but isnt it logically part of this? Pushed to master. Thanks, Kyle! |