Bug 10120 - Fine recalculation on return needs to be a system preference
Summary: Fine recalculation on return needs to be a system preference
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 10262
  Show dependency treegraph
 
Reported: 2013-04-25 19:54 UTC by Katrin Fischer
Modified: 2015-10-27 15:11 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10120 : Adding a syspref to control if overdue charges are updated /on return (4.68 KB, patch)
2013-04-29 21:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10120 : Tidy up the code so we can see what is going on (2.60 KB, patch)
2013-04-29 21:11 UTC, Chris Cormack
Details | Diff | Splinter Review
[Signed off] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return (4.71 KB, patch)
2013-05-02 09:12 UTC, David Cook
Details | Diff | Splinter Review
[Signed off] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return (4.71 KB, patch)
2013-05-02 09:15 UTC, David Cook
Details | Diff | Splinter Review
[Signed off] Bug 10120 : Tidy up the code so we can see what is going on (2.67 KB, patch)
2013-05-02 09:15 UTC, David Cook
Details | Diff | Splinter Review
[PASSED QA] Bug 10120 : Tidy up the code so we can see what is going on (3.42 KB, patch)
2013-05-15 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return (5.03 KB, patch)
2013-05-15 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10120 : Tidy up the code so we can see what is going on (3.42 KB, patch)
2013-05-15 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10120: (QA followup) avoid raising warnings on upgrade (1.44 KB, patch)
2015-10-27 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-04-25 19:54:52 UTC
As agreed on IRC today, recalculation of fines on return needs to be tied to a system preference as it changes the fines workflow.

Ideally the system preference would have 3 settings:
- recalculate always
- recalculate only for hourly loans
- don't recalculate
Comment 1 Chris Cormack 2013-04-29 20:19:55 UTC
There is currently no way to know something is an hourly loan or not.
So the first implementation of this will only be on or off. If someone wants to change circulation to store if something was an hourly loan or not on the issue row then it could be expanded to 3 choices.
Comment 2 Chris Cormack 2013-04-29 21:10:32 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2013-04-29 21:11:11 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2013-04-29 21:12:11 UTC
Only briefly tested, please test thoroughly
Comment 5 David Cook 2013-05-02 09:12:16 UTC Comment hidden (obsolete)
Comment 6 David Cook 2013-05-02 09:15:05 UTC Comment hidden (obsolete)
Comment 7 David Cook 2013-05-02 09:15:15 UTC Comment hidden (obsolete)
Comment 8 David Cook 2013-05-02 09:17:20 UTC
Works as described. 

I doubt a library would want to have this system preference on for any case except hourly loans, but since there is no way to tell if a loan is hourly or not, this binary method is the best we've got.
Comment 9 Katrin Fischer 2013-05-15 06:30:04 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-05-15 06:30:18 UTC
Created attachment 18144 [details] [review]
[PASSED QA] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return

On by default,
To Test

1/ Create an overdue item, that should get fines
2/ Return the item
3/ Check the borrowers record to see if the fine has been added/updated

Apply patch
1/ Make sure preference is set to do
Repeat steps 1-3 above

2/ Switch the preference to don't
Repeat stes 1-2
3/ Check the fine hasn't been added/updated

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass, works as described.
I would categorize this a bug fix for libraries that don't want
the new changed  behaviour that was introduced by recalculating
fines on return.
Comment 11 Katrin Fischer 2013-05-15 06:30:26 UTC
Created attachment 18145 [details] [review]
[PASSED QA] Bug 10120 : Tidy up the code so we can see what is going on

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Amended test plan used for testing:

Note: Use a new item for each test as we determine
fines by item number. If you have the same item twice
on the same day on the same account with the same
due date, fine calculation is going to fail.

1/ Create an overdue item, that should get fines
2/ Return the item from the patron account checkout tab
3/ Check the borrowers record to see if the fine has been added/updated

Repeat, but for 2/ return the item from the check in page

Apply patch

1/ Make sure preference is set to do
Repeat steps 1-3 from above

2/ Switch the preference to don't
Repeat steps 1-2
3/ Check the fine hasn't been added/updated

Again, check for returns from patron account and check in page.
Comment 12 Jared Camins-Esakov 2013-05-16 13:37:44 UTC
String changes, so this will be held for 3.14/3.12.1.
Comment 13 Marcel de Rooy 2013-05-23 08:26:22 UTC
Back to master.
Comment 14 Galen Charlton 2013-05-24 14:55:38 UTC
For future reference, the IRC discussion in question starts at

http://irc.koha-community.org/koha/2013-04-25#i_1239245
Comment 15 Galen Charlton 2013-05-24 15:10:02 UTC
And a general comment: perhaps we should consider having loan records either store a copy of the circ policy that applies to the loan (e.g., issuelength, lengthunit, fine, finedays, etc.) or a reference to it (e.g,. by first giving issuingrules an opaque PK, then adding that as a FK to issues).
Comment 16 Galen Charlton 2013-05-24 15:11:03 UTC
And of course, Chris already suggested that. :)
Comment 17 Galen Charlton 2013-05-24 15:52:06 UTC
I have pushed the main patch to master, though with a bit of reluctance as I suspect the need for this syspref will go away if/when the underpinnings of fine accounting are improved.

I have *not* pushed the follow-up whitespace patch; it might just be me, but I found that it made that section of code *less* readable, not more.  Among other reasons, I really prefer that comments be indented at the same level as the block they apply to.

I suggest that we just plan on running C4/Circulation.pm through perltidy at some point in the near future.

Thanks, Chris!

RMaint 3.12 please take note: I believed that this is considered a blocker by at least one library to upgrading.
Comment 18 Tomás Cohen Arazi 2013-06-03 15:54:20 UTC
This patch has been pushed to 3.12.x, will be in 3.12.1.
Comment 19 Bernardo Gonzalez Kriegel 2013-06-10 20:02:37 UTC
Pushed to 3.10.x, will be in 3.10.7
Comment 20 Tomás Cohen Arazi 2015-10-27 15:11:50 UTC
Created attachment 44078 [details] [review]
Bug 10120: (QA followup) avoid raising warnings on upgrade

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>