Bug 10694 - Allow arbitrary backdating of returns
Summary: Allow arbitrary backdating of returns
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 8769 12959 20626
  Show dependency treegraph
 
Reported: 2013-08-07 15:02 UTC by Kyle M Hall
Modified: 2018-04-21 11:44 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10694 - Allow arbitrary backdating of returns (9.87 KB, patch)
2013-08-07 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (12.76 KB, patch)
2013-08-07 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (15.36 KB, patch)
2013-09-13 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (14.30 KB, patch)
2013-09-13 15:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (14.36 KB, patch)
2013-09-13 15:39 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup (3.71 KB, patch)
2013-10-20 22:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup (3.68 KB, patch)
2013-10-20 22:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (14.35 KB, patch)
2013-12-19 12:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup (3.68 KB, patch)
2013-12-19 12:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 (2.73 KB, patch)
2013-12-19 17:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (14.39 KB, patch)
2014-02-26 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup (3.68 KB, patch)
2014-02-26 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup 2 (2.73 KB, patch)
2014-02-26 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup 3 (5.10 KB, patch)
2014-02-26 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns (14.41 KB, patch)
2014-03-12 11:35 UTC, Petter Goksøyr Åsen
Details | Diff | Splinter Review
Bug 10694 - Allow arbitrary backdating of returns - QA Followup (8.98 KB, patch)
2014-03-12 11:35 UTC, Petter Goksøyr Åsen
Details | Diff | Splinter Review
[PASSED QA] Bug 10694 - Allow arbitrary backdating of returns (14.53 KB, patch)
2014-03-31 17:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10694 - Allow arbitrary backdating of returns - QA Followup (9.05 KB, patch)
2014-03-31 17:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10694 [QA Followup] - Unit Tests, improve AddReturn POD (2.74 KB, patch)
2014-04-23 11:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] [Bug 10694 [QA Followup] - Unit Tests, improve AddReturn POD (2.82 KB, patch)
2014-04-23 13:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-08-07 15:02:01 UTC
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.
Comment 1 Kyle M Hall 2013-08-07 15:08:16 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-08-07 15:19:10 UTC Comment hidden (obsolete)
Comment 3 Barbara Knibbs 2013-09-13 15:18:22 UTC
Barbara Knibbs from Farmington Library has tested this patch. It works great, and I would like to sign off on it.
Comment 4 Kyle M Hall 2013-09-13 15:30:07 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-09-13 15:32:48 UTC Comment hidden (obsolete)
Comment 6 Melissa Lefebvre 2013-09-13 15:39:48 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-09-19 09:58:50 UTC
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.
Comment 8 Kyle M Hall 2013-09-26 11:23:38 UTC
(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.
Comment 9 Jonathan Druart 2013-09-26 14:17:09 UTC
(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.
Comment 10 Katrin Fischer 2013-09-26 15:00:05 UTC
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?
Comment 11 Kyle M Hall 2013-10-02 17:00:48 UTC
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.
Comment 12 Jonathan Druart 2013-10-04 11:52:09 UTC
(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.
Comment 13 Kyle M Hall 2013-10-20 18:02:36 UTC
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.
Comment 14 Paul Poulain 2013-10-20 18:08:17 UTC
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)
Comment 15 Kyle M Hall 2013-10-20 22:40:10 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-10-20 22:41:20 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-11-21 21:47:57 UTC
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.
Comment 18 Katrin Fischer 2013-12-06 15:32:15 UTC
Failing QA to get Kyle's attention.
Comment 19 Kyle M Hall 2013-12-19 12:48:07 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-12-19 12:48:22 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-12-19 17:34:49 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-12-19 17:37:12 UTC
> 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.
Comment 23 Katrin Fischer 2014-01-12 10:50:21 UTC
Working on this now...
Comment 24 Katrin Fischer 2014-01-12 11:18:10 UTC
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.
Comment 25 Kyle M Hall 2014-02-26 15:49:30 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2014-02-26 15:49:46 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-02-26 15:49:50 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-02-26 15:49:55 UTC Comment hidden (obsolete)
Comment 29 Petter Goksøyr Åsen 2014-03-12 11:35:05 UTC Comment hidden (obsolete)
Comment 30 Petter Goksøyr Åsen 2014-03-12 11:35:10 UTC Comment hidden (obsolete)
Comment 31 Petter Goksøyr Åsen 2014-03-12 11:39:48 UTC
Tested, works according to the test plan - it deletes the fine when backdating.
Comment 32 Petter Goksøyr Åsen 2014-03-12 13:24:29 UTC
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.
Comment 33 Katrin Fischer 2014-03-31 17:29:58 UTC
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.
Comment 34 Katrin Fischer 2014-03-31 17:41:32 UTC
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>
Comment 35 Katrin Fischer 2014-03-31 17:43:05 UTC
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.
Comment 36 Katrin Fischer 2014-03-31 17:47:03 UTC
Hm, I think I missed something here - we should have some unit tests for the changes to AddReturn
Comment 37 Katrin Fischer 2014-04-15 08:43:29 UTC
*** Bug 8338 has been marked as a duplicate of this bug. ***
Comment 38 Colin Campbell 2014-04-22 14:02:26 UTC
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
Comment 39 Kyle M Hall 2014-04-23 10:37:39 UTC
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
Comment 40 Kyle M Hall 2014-04-23 11:39:08 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2014-04-23 11:41:24 UTC
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.
Comment 42 Martin Renvoize 2014-04-23 13:17:52 UTC
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>
Comment 43 Colin Campbell 2014-04-23 13:39:05 UTC
(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?
Comment 44 Galen Charlton 2014-05-02 22:56:38 UTC
Pushed to master.  Thanks, Kyle!