Bug 22539

Summary: Fines charged on back-dated checkin when item has been renewed
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Fines and feesAssignee: Bugs List <koha-bugs>
Status: In Discussion --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P3 CC: danielle, hannah.co, jonathan.druart, katrin.fischer, kyle, m.de.rooy, marjorie.barry-vila, martin.renvoize, nick, stefan.berndtsson, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12144
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22877
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22902
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24763
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25417
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8338
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14591, 25417    
Bug Blocks: 14825    
Attachments: Bug 22539: Reset fines for backdated returns
Bug 22539: Reset fines for backdated returns
Bug 22539: Always recalculate fines when a return date is specified
Bug 22539: Reset fines for backdated returns
Bug 22539: Always recalculate fines when a return date is specified
Bug 22539: Reset fines for backdated returns
Bug 22539: Always recalculate fines when a return date is specified
Bug 22539: Add regression tests

Description Andrew Fuerste-Henry 2019-03-19 14:19:18 UTC
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.
Comment 1 Martin Renvoize 2019-03-22 16:55:09 UTC
Fun.. I'll take a deeper look at this on Monday.
Comment 2 Kyle M Hall 2019-03-22 17:24:51 UTC
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?
Comment 3 Andrew Fuerste-Henry 2019-03-22 17:39:09 UTC
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.
Comment 4 Martin Renvoize 2019-03-25 13:59:26 UTC
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!)
Comment 5 Martin Renvoize 2019-03-25 14:02:35 UTC
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.
Comment 6 Kyle M Hall 2019-03-25 17:30:33 UTC
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>
Comment 7 Kyle M Hall 2019-03-25 17:30:47 UTC
Created attachment 86986 [details] [review]
Bug 22539: Always recalculate fines when a return date is specified
Comment 8 Martin Renvoize 2019-03-26 06:10:23 UTC
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>
Comment 9 Martin Renvoize 2019-03-26 06:10:26 UTC
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>
Comment 10 Martin Renvoize 2019-03-26 06:12:30 UTC
This will need some tests before QA.. I'll try to get to those today as we seem to be happy with this path.
Comment 11 Martin Renvoize 2019-04-03 08:45:47 UTC
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.
Comment 12 Martin Renvoize 2019-04-03 09:37:29 UTC
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>
Comment 13 Martin Renvoize 2019-04-03 09:37:31 UTC
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>
Comment 14 Martin Renvoize 2019-04-03 09:37:34 UTC
Created attachment 87343 [details] [review]
Bug 22539: Add regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-04-03 09:38:24 UTC
All ready for a QA run
Comment 16 Martin Renvoize 2019-04-03 09:51:01 UTC
(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.
Comment 17 Kyle M Hall 2019-04-03 10:52:27 UTC
(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'.
Comment 18 Marcel de Rooy 2019-04-12 08:23:26 UTC
QA: Looking here
Comment 19 Marcel de Rooy 2019-04-12 09:08:47 UTC
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.
Comment 20 Katrin Fischer 2019-04-12 09:38:19 UTC
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?
Comment 21 Katrin Fischer 2019-04-12 09:39:18 UTC
asc also seems wrong here!
Comment 22 Martin Renvoize 2019-04-16 10:19:15 UTC
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?
Comment 23 Stefan Berndtsson 2019-10-09 13:46:12 UTC
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
Comment 24 Danielle Elder 2020-01-08 20:27:11 UTC
This is still affecting libraries, especially after holiday breaks.