Bug 14591 - book drop / drop box mode incorrectly decrements accrued overdue fines
Summary: book drop / drop box mode incorrectly decrements accrued overdue fines
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 22049
Blocks: 13044 22539 22516 22533
  Show dependency treegraph
 
Reported: 2015-07-22 16:14 UTC by Galen Charlton
Modified: 2019-04-03 09:52 UTC (History)
11 users (show)

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


Attachments
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines (9.11 KB, patch)
2019-03-14 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: Update other calls to AddReturn (1.98 KB, patch)
2019-03-14 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: Update unit tests (12.14 KB, patch)
2019-03-14 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines (9.61 KB, patch)
2019-03-14 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: Update other calls to AddReturn (1.98 KB, patch)
2019-03-14 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: Update unit tests (12.14 KB, patch)
2019-03-14 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines (9.66 KB, patch)
2019-03-14 16:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14591: Update other calls to AddReturn (2.03 KB, patch)
2019-03-14 16:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14591: Update unit tests (12.19 KB, patch)
2019-03-14 16:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14591: (QA follow-up) Optimize DateTime passing (4.18 KB, patch)
2019-03-14 16:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines (9.73 KB, patch)
2019-03-15 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14591: Update other calls to AddReturn (2.08 KB, patch)
2019-03-15 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14591: Update unit tests (12.25 KB, patch)
2019-03-15 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14591: (QA follow-up) Optimize DateTime passing (4.24 KB, patch)
2019-03-15 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14591: (QA follow-up) Fix AddReturn call in Circulation/issue.t (1.20 KB, patch)
2019-03-15 09:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14591: (QA follow-up) Fix call to AddReturn for SIP (1.35 KB, patch)
2019-03-15 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2015-07-22 16:14:33 UTC
Book drop mode will always subtract the lastincrement value from accrued overdue fines, even if the loan is so overdue that it reached the max fine amount prior to effective return date.

For example, if a loan was due on 1/15/2015 and is charged 0.20 per day with a maximum fine of 1.00, if it is returned on 2/1/2015 (assume no holidays), the fine should remain at 1.00.  However, currently the fine will be reduced to 0.80 if the item is checked in using book drop mode.

I believe this can also account for the behavior that Kyle reported in bug 12144

Bug 13044 is also relevant here: making book drop mode become a special case of the SpecifyReturnDate functionality (and in particular, removing the dropbox logic from _FixOverduesOnReturn) is an opportunity to avoid inappropriate reduction of max overdue fines.
Comment 1 Katrin Fischer 2017-12-05 22:04:11 UTC
I am not sure if this is still valid, certainly should be retested after over 2 years and changes made to fine calculations.
Comment 2 Tomás Cohen Arazi 2018-12-06 13:53:24 UTC
This is still valid. 'lastincrement' is not enough to determine how much to roll back.

This also has the side effect of making debits (the accruing fine) be converted into a credit if it was partially payed or there was a writeoff (this could be the case for a patron having outstanding credit and it being applied automatically as proposed in bug 21915).
Comment 3 Kyle M Hall 2018-12-06 14:02:44 UTC
I think the solution is to remove all the dropbox code, and make dropbox mode just be a backdated return which backdates to the last day the library was previously open.
Comment 4 Tomás Cohen Arazi 2018-12-06 14:10:56 UTC
(In reply to Kyle M Hall from comment #3)
> I think the solution is to remove all the dropbox code, and make dropbox
> mode just be a backdated return which backdates to the last day the library
> was previously open.

I think we now have the account offsets, and can rewrite _FixOverduesOnReturn as I did with _FixAccountForLostAndReturned, to take them into account. We need to:

- Make sure each 'increment' actually creates an offset (this of course should be possible with $line->adjust from bug 21727)
- Replace the current code so it doesn't touch 'amount'. It should calculate the amount to substract, and create a special type of credit. If there was a writeoff, skip that writeoff amount, if there was a (maybe partial) payment, refund it with a CR account line. So:

my $amount = calaculate_amount_to_backtrack_skipping_writeoffs({ $fine_line });
$account->add_credit({ amount => $amount, type => 'credit_return' })->apply({ debit => $fine_line });
Comment 5 Tomás Cohen Arazi 2018-12-09 19:48:58 UTC
(In reply to Kyle M Hall from comment #3)
> I think the solution is to remove all the dropbox code, and make dropbox
> mode just be a backdated return which backdates to the last day the library
> was previously open.

I agree with this :-D
Comment 6 Martin Renvoize 2018-12-10 12:17:35 UTC
+1 from me too :)
Comment 7 Benjamin Daeuber 2019-01-31 18:25:34 UTC
(In reply to Kyle M Hall from comment #3)
> I think the solution is to remove all the dropbox code, and make dropbox
> mode just be a backdated return which backdates to the last day the library
> was previously open.

I disagree with this. We actually don't use bookdrop mode since it confines us to 24 hours. We manually set the backdate because there are times that we might be days behind checking in items and times we are caught up. We try to match the date to when the item was returned.
Comment 8 Katrin Fischer 2019-01-31 18:36:07 UTC
Hi Margo, this will still work. What was removed was the checkbox on the circulation page that only allowed to set for last day library open in favor of the newer more flexible specifyreturndate feature.
Comment 9 Benjamin Daeuber 2019-01-31 18:37:55 UTC
(In reply to Katrin Fischer from comment #8)
> Hi Margo, this will still work. What was removed was the checkbox on the
> circulation page that only allowed to set for last day library open in favor
> of the newer more flexible specifyreturndate feature.

Yes, I just read through 13044. I'm generally ok with the solution proposed, though I do have another comment I'll put in that ticket.
Comment 10 Katrin Fischer 2019-02-10 12:47:34 UTC
See: Bug 13044 - Deprecate old book drop mode
Comment 11 Kyle M Hall 2019-03-14 15:29:02 UTC
Created attachment 86611 [details] [review]
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines
Comment 12 Kyle M Hall 2019-03-14 15:29:13 UTC
Created attachment 86612 [details] [review]
Bug 14591: Update other calls to AddReturn
Comment 13 Kyle M Hall 2019-03-14 15:29:16 UTC
Created attachment 86613 [details] [review]
Bug 14591: Update unit tests
Comment 14 Kyle M Hall 2019-03-14 15:39:21 UTC
Created attachment 86614 [details] [review]
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines
Comment 15 Kyle M Hall 2019-03-14 15:39:33 UTC
Created attachment 86615 [details] [review]
Bug 14591: Update other calls to AddReturn
Comment 16 Kyle M Hall 2019-03-14 15:39:36 UTC
Created attachment 86616 [details] [review]
Bug 14591: Update unit tests
Comment 17 Martin Renvoize 2019-03-14 16:57:07 UTC
Created attachment 86618 [details] [review]
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-03-14 16:57:11 UTC
Created attachment 86619 [details] [review]
Bug 14591: Update other calls to AddReturn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2019-03-14 16:57:15 UTC
Created attachment 86620 [details] [review]
Bug 14591: Update unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-03-14 16:57:19 UTC
Created attachment 86621 [details] [review]
Bug 14591: (QA follow-up) Optimize DateTime passing

We were passing around possibly undefined $return_date variables from
AddReturn and then instantiating a new DateTime object as a default for
each routine. This followup sets the default higher up the stack within
AddReturn which provider clearer logic and a small performance
improvment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2019-03-14 17:04:31 UTC
I really like this solution! It tidies up the code significantly and corrects the logic considerably!

Signing off as it all works in testing as described...

Some QA comments for the next in line however.

* Can we/Should we remove the 'lastincrement' field here as I believe this was the only use case for it?
* The logic here puts 'dropboxmode/dropboxdate' in precidence to any specified return date.. I think we perhaps need a bit more in this regard, but perhaps that can be done within bug 13044 as there's a fair bit of contextual conversation over there around this issue.
Comment 22 Martin Renvoize 2019-03-14 17:12:01 UTC
See bug 22516 for the removal of lastincrement.. I decided to leave that as a followup bug to keep this bug simple.
Comment 23 Josef Moravec 2019-03-15 09:45:44 UTC
(In reply to Martin Renvoize from comment #21)
> I really like this solution! It tidies up the code significantly and
> corrects the logic considerably!
> 
> Signing off as it all works in testing as described...
> 
> Some QA comments for the next in line however.
> 
> * Can we/Should we remove the 'lastincrement' field here as I believe this
> was the only use case for it?
> * The logic here puts 'dropboxmode/dropboxdate' in precidence to any
> specified return date.. I think we perhaps need a bit more in this regard,
> but perhaps that can be done within bug 13044 as there's a fair bit of
> contextual conversation over there around this issue.

I agree to continue on these two points on other reports, as this patchset is ok and fixes the bug.
Comment 24 Josef Moravec 2019-03-15 09:46:17 UTC
Created attachment 86639 [details] [review]
Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Josef Moravec 2019-03-15 09:46:21 UTC
Created attachment 86640 [details] [review]
Bug 14591: Update other calls to AddReturn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Josef Moravec 2019-03-15 09:46:25 UTC
Created attachment 86641 [details] [review]
Bug 14591: Update unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Josef Moravec 2019-03-15 09:46:28 UTC
Created attachment 86642 [details] [review]
Bug 14591: (QA follow-up) Optimize DateTime passing

We were passing around possibly undefined $return_date variables from
AddReturn and then instantiating a new DateTime object as a default for
each routine. This followup sets the default higher up the stack within
AddReturn which provider clearer logic and a small performance
improvment.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2019-03-15 09:46:32 UTC
Created attachment 86643 [details] [review]
Bug 14591: (QA follow-up) Fix AddReturn call in Circulation/issue.t

Test plan:
prove t/db_dependent/Circulation/issue.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Nick Clemens 2019-03-15 12:46:49 UTC
Awesome work all!

Pushed to master for 19.05
Comment 30 Kyle M Hall 2019-03-15 14:32:28 UTC
Created attachment 86676 [details] [review]
Bug 14591: (QA follow-up) Fix call to AddReturn for SIP
Comment 31 Nick Clemens 2019-03-15 14:36:28 UTC
(In reply to Kyle M Hall from comment #30)
> Created attachment 86676 [details] [review] [review]
> Bug 14591: (QA follow-up) Fix call to AddReturn for SIP

Followup pushed to master
Comment 32 Katrin Fischer 2019-03-16 11:32:07 UTC
Where is the test plan here?

I am trying to determine if this will also help fixing:

Bug 12144 - Book drop mode mode erasing fine when CalculateFinesOnReturn is enabled 
Bug 8338 - fines not cleared with dropbox mode
Comment 33 Kyle M Hall 2019-03-18 11:56:11 UTC
Yes, I do believe both of those can be closed in some form ( duplicates? ). This bug fixes the issues related to those two bugs.

(In reply to Katrin Fischer from comment #32)
> Where is the test plan here?
> 
> I am trying to determine if this will also help fixing:
> 
> Bug 12144 - Book drop mode mode erasing fine when CalculateFinesOnReturn is
> enabled 
> Bug 8338 - fines not cleared with dropbox mode
Comment 34 Martin Renvoize 2019-03-19 09:51:55 UTC
Bug with enhancement not in 18.11.x series.
Comment 35 Martin Renvoize 2019-04-03 09:52:39 UTC
(In reply to Katrin Fischer from comment #32)
> Where is the test plan here?
> 
> I am trying to determine if this will also help fixing:
> 
> Bug 12144 - Book drop mode mode erasing fine when CalculateFinesOnReturn is
> enabled 
> Bug 8338 - fines not cleared with dropbox mode

Bug 12144 has been effectively replaced with bug 22539 which is currently seeking QA.

I encountered the though process that I believe has led to bug 8338 whilst coding bug 22539 and I asked the same questions there..