In a certain niche situation, a patron can end up with an unexpected fine due to renewing a book just prior to returning it. Here's the situation: - Patron has a book due on a Saturday and doesn't return it - On Sunday, the library is closed - Early Sunday morning, the fines cron generates an accruing fine on the item - Patron puts the item in the bookdrop on Sunday - Patron renews the item via the OPAC on Sunday - At renewal, the fine changes from FU to F - Monday morning, the item is checked in via bookdrop mode, which back-dates the checkin to Saturday According to the old_issues table, the item was checked in before any fine was charged, but the fine remains on the patron's account. If the patron had not renewed the item, the fine would have remained FU and would have been forgiven on checkin.
Fun.. I'll take a deeper look at this on Monday.
Honestly, I'm not positive I'd call this a bug. It sounds like Koha is behaving correctly and the patron is not ;) Maybe it would be possible to look at the latest fine related to a checkin since we have issue_id's for them, and see if the return date falls *after* the backdated return date, and adjust the fine accordingly?
You're not wrong, Kyle. This is Koha doing just what it's told, following the letter of the law. But I'd hate to be the librarian who has to explain to a patron that they got a fine because they renewed something :) Adjusting fines generated after the backdated return date seems like a good solution to me.
So.. are we saying the Fine should not have been added in the first place (or rather, it should wait until the next non-closed day to start the fines accrual) Or.. are we saying the Fine should be entirely removed on return. Or.. are we saying the Fine should be set to Zero on return. And.. should we taking account of 'CalculateFinesOnReturn' syspref. The more I dug into how to fix this, the more questions it raised for me.. currently I have a patch that would reset the fine to accruing if we find a backdated return date that's prior to the lastreneweddate on the issue.. this then lets all the normal handling take place if CalculateFinesOnReturn is enabled (i.e. the fine will be updated to correctly reflect either 0 or the reduced fine amount).. but it won't be deleted entirely and it won't work for systems where CalculateFinesOnReturn is not enabled (about 10% of Koha's if HEA is to be believed!)
Created attachment 86975 [details] [review] Bug 22539: Reset fines for backdated returns Fixed fines should be 'unfixed' for backdated returns that specify a return date prior to the lastreneweddate.
Created attachment 86985 [details] [review] Bug 22539: Reset fines for backdated returns Fixed fines should be 'unfixed' for backdated returns that specify a return date prior to the lastreneweddate. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86986 [details] [review] Bug 22539: Always recalculate fines when a return date is specified
Created attachment 86998 [details] [review] Bug 22539: Reset fines for backdated returns Fixed fines should be 'unfixed' for backdated returns that specify a return date prior to the lastreneweddate. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86999 [details] [review] Bug 22539: Always recalculate fines when a return date is specified Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This will need some tests before QA.. I'll try to get to those today as we seem to be happy with this path.
Question: This patch uses the existing UpdateFine (and as such Koha::Account->adjust) functionality. As such, it'll result in a fine line with an amount of '0'. Is this what we want? We have a few alternatives to consider: 1) We could leave it as is. 1) We could delete the fine entirely so it's as if it never existed 2) We could record a 'Writeoff' or something line against it so show it was removed by dropbox/backdated returns mode.
Created attachment 87341 [details] [review] Bug 22539: Reset fines for backdated returns Fixed fines should be 'unfixed' for backdated returns that specify a return date prior to the lastreneweddate. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87342 [details] [review] Bug 22539: Always recalculate fines when a return date is specified Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87343 [details] [review] Bug 22539: Add regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All ready for a QA run
(In reply to Martin Renvoize from comment #11) > Question: This patch uses the existing UpdateFine (and as such > Koha::Account->adjust) functionality. As such, it'll result in a fine line > with an amount of '0'. Is this what we want? > > We have a few alternatives to consider: > > 1) We could leave it as is. > 1) We could delete the fine entirely so it's as if it never existed > 2) We could record a 'Writeoff' or something line against it so show it was > removed by dropbox/backdated returns mode. It looks like these questions are raised and not yet dealt with fully in bug 8338.. Shall we move this discussion back over there and I can submit patches for the 'removal' or 'record of writeoff' to that bug.
(In reply to Martin Renvoize from comment #16) > (In reply to Martin Renvoize from comment #11) > > Question: This patch uses the existing UpdateFine (and as such > > Koha::Account->adjust) functionality. As such, it'll result in a fine line > > with an amount of '0'. Is this what we want? > > > > We have a few alternatives to consider: > > > > 1) We could leave it as is. > > 1) We could delete the fine entirely so it's as if it never existed > > 2) We could record a 'Writeoff' or something line against it so show it was > > removed by dropbox/backdated returns mode. > > It looks like these questions are raised and not yet dealt with fully in bug > 8338.. Shall we move this discussion back over there and I can submit > patches for the 'removal' or 'record of writeoff' to that bug. My comment from 8338: My personal preference is to leave the 0 fine as part of the 'paper trail' for Koha's accounting system. I would not be opposed to option 2. We could add new 'states' for 'Dropboxed', 'Backdated return' and 'Fine forgiven' if we build it on top of bug 22512. So, I'm in favor of 'record a writeoff'.
QA: Looking here
Back to SO. (Could have been Discussion for me too.) The solution here is bit too hacky for me. Especially the return_date_specified condition does not look good to me. If a library does not want fines recalculated on return, shouldn't we respect that? If a patron returns a book too late, shouldn't he just pay a fine? Or are we solving a bug at the wrong place? No reason to block or fail, but it does not feel good. Another QA opinion please.
I think one of the issues here is the library charging fines on days the library is closed (finesCalendar), but at the same time offering the book drop with on those days. It seems contradictory to me to waive fines from days closed when returned on a day the library is closed but charge them in the first place. As such it's probably a rare case? I also see an issue with this line: if ( $return_date_specified || ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) ) We have turned off CalculateFinesOnReturn for quite a few libraries and that would cause us trouble. We don't want to recalculate ever. my $fines = Koha::Account::Lines->search( { issue_id => $issue->issue_id }, { order_by => { '-asc' => 'date' } } ); Are we also sure that this will not cause other problems? I don't think we should be touching F fines. What happens if there are multiple F and 1 or 0 FU fines on the same issue?
asc also seems wrong here!
Alternatively, we could change 'AddRenewal' to take account of is_holiday/closed days and backdate the renewal charge to the previous open day. Any thoughts on that approach?
This issue has hit us as well. We calculate fines during weekends regardless of whether or not we're closed, but if the book was returned on saturday and sunday or monday was the first fined day, making the user pay for the book can't be correct, right? In AddReturn, there are a few conditions for recalculating fines. They used to be: "(syspref_is_set AND is_overdue) OR explicit_return_date". This has now been changed to: "(syspref_is_set AND was_overdue_on_return_date AND is_not_lost)" Why does it check if there should be a recalculation based on the backdated date (was_overdue_on_return_date)? The fine will still be calculated based on the backdated date, but the condition whether or not to recalculate shouldn't depend on the overdue status at that date. This line in AddReturn(): $is_overdue = $issue->is_overdue( $return_date ); ...used to be: $is_overdue = $issue->is_overdue; To me, the logical condition should be: 1. Syspref CalculateFinesOnReturn is set 2. Book is overdue right now 3. Book is not lost
This is still affecting libraries, especially after holiday breaks.