Bug 7849

Summary: Instant Fine Calculation at Checkin
Product: Koha Reporter: Elliott Davis <tdavis>
Component: CirculationAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: enhancement    
Priority: P5 - low CC: chris, gmcharlt, paul.poulain, wizzyrea
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7852    
Bug Blocks:    
Attachments: Adds Instant fine calculation at checkin
[Signed-Off] Adds instant fine calculation at checkin
Add instant checkin to Circulation
Bug 7849 Followup: only charge fines on return if finesMode set to production
Patch to add instant fine calculation
Adds instant fine calculation
Fix typo in patch as applied

Description Elliott Davis 2012-03-29 17:20:04 UTC
Add the ability for fines to be calculated at checkin.  This will be helpful for hourly based items so the cronjob for fines doesn't have to be run on such a frequent interval.
Comment 1 Kyle M Hall 2012-03-29 19:48:44 UTC
fyi, I have written a module to do fines on checkin for the Crawford County Federated Library System. I will find the code and post it up.
Comment 2 Elliott Davis 2012-03-30 01:07:37 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-04-02 13:29:01 UTC
This code is far more terse then my own ( which I had written as a perl module to be an alternative to the traditional fines cron ), and thus more efficient. I won't bother submitting my own code, as Eliot Davis deserves a ++ for this.

(In reply to comment #2)
> Created attachment 8703 [details] [review]
> Adds Instant fine calculation at checkin
> 
> To Test:
> 
> Check out an Item, hourly items are best but any will do.  Make sure the
> item is overdue, and when you check the item in you should see a fine
> reflected that is correct for the amount of time it is overdue according to
> your smart rules.
Comment 4 Kyle M Hall 2012-04-02 13:56:32 UTC
Unfortunately, I'm having some problems with testing this code. Using $duration->days() only returns the number of days, not the length of time in days. So an item that is overdue 2 months will show a days of 0. I had thought in_units might work, but it does not appear to work either.

From CPAN:
Returns the length of the duration in the units (any of those that can be passed to new) given as arguments. All lengths are integral, but may be negative. Smaller units are computed from what remains after taking away the larger units given, so for example:

  $dur->in_units( 'years' );            # 2
  $dur->in_units( 'months' );           # 27
  $dur->in_units( 'years', 'months' );  # (2, 3)
  $dur->in_units( 'weeks', 'days' );    # (0, 0) !

The last example demonstrates that there *will not be any conversion between units which don't have a fixed conversion rate*.
Comment 5 Elliott Davis 2012-04-02 15:57:08 UTC
Kyle I'm sorry but I forgot to mark bug 7852 as a blocker for this patch. No fine calculation will be working 100% until that gets pushed.

I'm going to flip this back to needs sign off.
Comment 6 Kyle M Hall 2012-04-02 17:01:03 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2012-04-04 15:49:45 UTC
QA comment: eliott, there are many tabs in your patch. You must use 4 spaces.
I have a git that update tabs to replace them by 4 spaces, but it seems that your tab is set to 8 spaces, to the code look really ugly.

Failed QA, please resubmit with spaces.

2nd comment: a clear test plan would be usefull, I'm not sure I will be able to easily test the patch
Comment 8 Elliott Davis 2012-04-04 16:51:33 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-04-04 17:12:25 UTC
DateTime objects checkout.  Print to STDERR is removed.  $today variable is altered by 1 day if dropbox mode, but it's not used in it's original context again, so that's just fine.  UpdateFine call is consistent with the way it's called in misc/cronjob/fines.pl

Checked that double-fines do not occur; running this at checkin just updates the existing fine, not charging it again.

Marking Passed QA.
Comment 10 Ian Walls 2012-04-04 17:28:44 UTC
Hmmm, in hindsight, one concern:

If the library has finesMode set to anything other than 'production', the patron should not be charged.  Ideally, if the setting is 'test', the fines value should appear on the template, with a confirmation box or some kind of dialogue indicating what they WOULD be charged.  I'd settle for just respecting 'production' or !'production' at this point, though.

Needs followup.
Comment 11 Ian Walls 2012-04-05 10:53:17 UTC Comment hidden (obsolete)
Comment 12 Ian Walls 2012-04-05 10:57:53 UTC
Paul, the first patch is Passed QA now, but the followup is Needs Signoff.  It's a one line change, just adding a conditional check to finesMode before calling UpdateFine, but it needs to be tested.

Leaving this as Passed QA status until the first patch is addressed, then we can reset to Needs Signoff.
Comment 13 Paul Poulain 2012-04-06 15:43:34 UTC
mmm... I think there is a small mistake in this patch, so, to avoid addin one by pushing something wrong, I prefer to ask:
The attachment http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=8879 says
From: Elliott Davis <tdavis@uttyler.edu>
Date: Thu, 29 Mar 2012 20:05:10 +0000
Subject: [PATCH 2/2] updateing whitespace changes

But I don't see the [PATCH 1/2]

Looking at (obsoleted) attachment 8781 [details] [review], the commit message is:
From: Elliott Davis <tdavis@uttyler.edu>
Date: Thu, 29 Mar 2012 20:05:10 +0000
Subject: [PATCH] [Signed-Off] Adds instant fine calculation at checkin

http://bugs.koha-community.org/show_bug.cgi?id=7849
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

I feel it's just the commit message that is wrong, and the patch could be pushed with an updated message.

Elliott, could you confirm ? (and, eventually, re-upload a patch with the proper commit message)
Comment 14 Elliott Davis 2012-04-08 15:21:33 UTC Comment hidden (obsolete)
Comment 15 Paul Poulain 2012-04-12 16:18:00 UTC
mmm... something goes wrong:

When I apply those patches:
* the 1st does not apply, there's a tiny conflict in use, that I think I can fix easily
* the 2nd one apply smoothly, BUT,

circ/circulation-home.pl gives me a bad:

syntax error at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1620, near ") ) "
Global symbol "$amount" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1624.
Global symbol "$type" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1624.
Global symbol "$borrowernumber" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1628.
Global symbol "$item" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1628.
Global symbol "$circControlBranch" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1628.
Global symbol "$borrower" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1628.
Global symbol "$messages" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1629.
syntax error at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1630, near "}"
Global symbol "$issue" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1632.
Global symbol "$item" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1632.
Global symbol "$item" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1637.
Global symbol "$branch" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1637.
Global symbol "$branch" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1638.
Global symbol "$item" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1638.
Global symbol "$item" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1639.
Global symbol "$branch" requires explicit package name at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1639.
syntax error at /home/paul/koha.dev/koha-community/C4/Circulation.pm line 1640, near "}"
/home/paul/koha.dev/koha-community/C4/Circulation.pm has too many errors.
Compilation failed in require at /home/paul/koha.dev/koha-community/C4/Overdues.pm line 26.
BEGIN failed--compilation aborted at /home/paul/koha.dev/koha-community/C4/Overdues.pm line 26.
Compilation failed in require at /home/paul/koha.dev/koha-community/C4/Members.pm line 30.
BEGIN failed--compilation aborted at /home/paul/koha.dev/koha-community/C4/Members.pm line 30.

Looking at the patch, I don't understand what's happening :'(

You've submitted the patch with tabs, and i've set a git rule to transform them to spaces. My supposition is that something went wrong with this.
Could you resubmit your patch with spaces instead of tab please ?
(switch back to passed QA once you've submitted)
Comment 16 Elliott Davis 2012-04-17 15:38:57 UTC
Created attachment 9231 [details] [review]
Adds instant fine calculation

This should (hopefully) resolve the spacing issue Paul was seeing.  Also rebased to master.
Comment 17 Paul Poulain 2012-04-19 16:14:26 UTC
OK, the patch applies ! I made some tests, and even if I couldn't check the patch work well because my configuration has no hourly rules nor fines, I could do check-ins without any problem.

I also checked the code, and it looks logical.

patch pushed, please double check, i'm a little bit uncomfortable with pushing such a patch 3 days before a release...
Comment 18 Colin Campbell 2012-04-24 13:32:27 UTC
Created attachment 9284 [details] [review]
Fix typo in patch as applied

There is a typo in the patch that causes a number of runtime warnings in make test and will have an effect as the returned value of type is always overwritten
Comment 19 Paul Poulain 2012-04-30 12:15:03 UTC
QA comment for the typo patch provided by Colin: 1 line patch, fixes an evident mistake that could/should have been detected before !

passed QA & pushed on branch 7949