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.
I am not sure if this is still valid, certainly should be retested after over 2 years and changes made to fine calculations.
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).
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.
(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 });
(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
+1 from me too :)
(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.
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.
(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.
See: Bug 13044 - Deprecate old book drop mode
Created attachment 86611 [details] [review] Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines
Created attachment 86612 [details] [review] Bug 14591: Update other calls to AddReturn
Created attachment 86613 [details] [review] Bug 14591: Update unit tests
Created attachment 86614 [details] [review] Bug 14591: book drop / drop box mode incorrectly decrements accrued overdue fines
Created attachment 86615 [details] [review] Bug 14591: Update other calls to AddReturn
Created attachment 86616 [details] [review] Bug 14591: Update unit tests
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>
Created attachment 86619 [details] [review] Bug 14591: Update other calls to AddReturn Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86620 [details] [review] Bug 14591: Update unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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.
See bug 22516 for the removal of lastincrement.. I decided to leave that as a followup bug to keep this bug simple.
(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.
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>
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>
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>
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>
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>
Awesome work all! Pushed to master for 19.05
Created attachment 86676 [details] [review] Bug 14591: (QA follow-up) Fix call to AddReturn for SIP
(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
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
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
Bug with enhancement not in 18.11.x series.
(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..
*** Bug 13044 has been marked as a duplicate of this bug. ***