Summary: | Instant Fine Calculation at Checkin | ||
---|---|---|---|
Product: | Koha | Reporter: | Elliott Davis <tdavis> |
Component: | Circulation | Assignee: | 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 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
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
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. 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. 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. 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*. 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. Created attachment 8781 [details] [review] [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> 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 Created attachment 8879 [details] [review] Add instant checkin to Circulation To Test: 1) Check out an Item, hourly items are best but any will do. 2) Make sure the item is overdue 3) Check the item in 4) You should see a fine reflected that is correct for the amount of time it is overdue according to your smart rules. 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. 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. Created attachment 8907 [details] [review] Bug 7849 Followup: only charge fines on return if finesMode set to production If the finesMode system preference is anything but 'production', patrons shouldn't actually be charged any fines, even if the rules say they should accrue them. This followup adds that conditional check, in order to prevent surprise fines at libraries with such a configuration. To test: 1. identify 2 overdue materials with associated fines rules 2. set fines mode to 'production' 3. return the first item: patron should be charged 4. set fines mode to 'test' or 'none' 5. return the second item: the patron should not be charged Followup idea: if either 'test' or 'production', return the fine amt that was/would have been charged, so it can be displayed on screen. 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. 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) Created attachment 9012 [details] [review] Patch to add instant fine calculation This patch has the updated commit message. Sorry for the confusion, I had a slight git snafu. 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) Created attachment 9231 [details] [review] Adds instant fine calculation This should (hopefully) resolve the spacing issue Paul was seeing. Also rebased to master. 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... 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 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 |