Bug 14390 - Fine not updated from 'FU' to 'F' on renewal
Summary: Fine not updated from 'FU' to 'F' on renewal
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
: 9614 16244 16255 (view as bug list)
Depends on:
Blocks: 17135 18802
  Show dependency treegraph
 
Reported: 2015-06-15 19:31 UTC by Barton Chittenden
Modified: 2019-03-15 16:40 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Step by step sample (332.40 KB, application/pdf)
2015-08-07 13:54 UTC, Claudio Costales
Details
Bug 14390 - On renewal, the last fine is not marked as accounttype 'F' when item is checked in. (7.91 KB, patch)
2015-11-06 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 - On renewal, the last fine is not marked as accounttype 'F' when item is checked in. (7.95 KB, patch)
2015-11-06 18:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Unit Test (2.71 KB, patch)
2015-12-01 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 - Fine not updated from 'FU' to 'F' on renewal (7.92 KB, patch)
2015-12-01 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Fix warning (831 bytes, patch)
2015-12-01 17:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Allow OpacFineNoRenewals to be 0 (1.56 KB, patch)
2016-01-29 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 - Fine not updated from 'FU' to 'F' on renewal (8.71 KB, patch)
2016-05-19 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Unit Test (2.95 KB, patch)
2016-05-19 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Fix warning (825 bytes, patch)
2016-05-19 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390 - Fine not updated from 'FU' to 'F' on renewal (8.75 KB, patch)
2016-08-19 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Unit Test (2.99 KB, patch)
2016-08-19 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Fix warning (871 bytes, patch)
2016-08-19 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390: [QA Follow-up] UpdateFine should be passed a hash (2.11 KB, patch)
2016-08-19 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Never try to update closed out fines (732 bytes, patch)
2016-08-22 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14390: [Follow-up] Only update FU record in UpdateFine (1.88 KB, patch)
2016-08-23 14:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390: [Follow-up] Only update FU record in UpdateFine (1.90 KB, patch)
2016-08-23 15:23 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 14390 - Fine not updated from 'FU' to 'F' on renewal (8.80 KB, patch)
2016-08-24 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Unit Test (3.05 KB, patch)
2016-08-24 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390 [QA Followup] - Fix warning (928 bytes, patch)
2016-08-24 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390: [QA Follow-up] UpdateFine should be passed a hash (2.11 KB, patch)
2016-08-24 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14390: [Follow-up] Only update FU record in UpdateFine (1.94 KB, patch)
2016-08-24 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2015-06-15 19:31:38 UTC
When an item with a fine is renewed, the old fine is closed (i.e. accounttype is set to 'F'). A new fine is created, with an updated description. The new find is not closed when the item is checked back in.

select date, 
       amount, 
       description, 
       accounttype, 
       amountoutstanding, 
       lastincrement,
       timestamp 
from accountlines 
where itemnumber = 35540 
  and borrowernumber = 24990\G
*************************** 1. row ***************************
             date: 2015-05-05
           amount: 0.500000
      description: O'Connor's annotated criminal codes plus. 05/04/2015 11:59 PM
      accounttype: F
amountoutstanding: 0.500000
    lastincrement: 0.500000
        timestamp: 2015-06-08 12:42:27
*************************** 2. row ***************************
             date: 2015-06-08
           amount: 4.500000
      description: O'Connor's annotated criminal codes plus. 05/26/2015 11:59 PM
      accounttype: FU
amountoutstanding: 4.500000
    lastincrement: 0.500000
        timestamp: 2015-06-08 02:02:40
2 rows in set (0.00 sec)

Looking in old_issues, you can see that the item was renewed, and that date_due does match the second description... only the first accountlines entry is marked woth accounttype 'F' on return, however.

select 
    date_due, 
    returndate, 
    lastreneweddate, 
    renewals, 
    timestamp, 
    issuedate 
from old_issues 
where itemnumber = 35540 
  and borrowernumber = 24990\G
*************************** 1. row ***************************
       date_due: 2015-05-26 23:59:00
     returndate: 2015-06-08 12:42:27
lastreneweddate: 2015-05-05 00:00:00
       renewals: 1
      timestamp: 2015-06-08 12:42:27
      issuedate: 2015-04-13 09:23:20

To replicate:
1) check out an item
2) let the item go overdue and start accruing fines
3) renew the item
4) let the item go overdue again. Note that a second accountlines entry has been added
5) return the item. Note that the new fine is set with accounttype 'FU'.
Comment 1 Barton Chittenden 2015-06-15 20:00:10 UTC
Assigning to Kyle Hall, per discussion with him.

Kyle, please note Katrin's comments in the previous ticket:

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14380#c3
Comment 2 Galen Charlton 2015-07-22 15:56:04 UTC
I don't think that renewing a loan actually converts accrued overdue fines to committed/closed ones.  Rather, I think the sequence of events is this:

- make loan
- loan becomes overdue
- accrued fines start getting calculated (accounttype = FU)
- loan is renewed
- accrued fine record is not changed (and while the fine itself shouldn't go away, the accounttype *ought* to be changed to F and the description updated to indicate that the loan was renewed while overdue)
- loan becomes overdue again
- accrued fines start getting calculated on a second accountlines row
- loan gets returned

During the return processing, _FixOverduesOnReturn will change the accounttype of accrued fines from FU to F.  However, it currently assumes that there is exactly one such fine; if there are multiple ones, only one gets its accounttype changed.
Comment 3 Claudio Costales 2015-08-07 12:27:59 UTC
Kyle, Galen

I think it is a "major" bug... Today I will attach the test I've done.
Comment 4 Claudio Costales 2015-08-07 13:54:45 UTC
Created attachment 41428 [details]
Step by step sample

Sorry, in spanish
Comment 5 Claudio Costales 2015-08-07 14:45:02 UTC
(In reply to Galen Charlton from comment #2)
> I don't think that renewing a loan actually converts accrued overdue fines
> to committed/closed ones.  Rather, I think the sequence of events is this:
> 
> - make loan
> - loan becomes overdue
> - accrued fines start getting calculated (accounttype = FU)
> - loan is renewed
> - accrued fine record is not changed (and while the fine itself shouldn't go
> away, the accounttype *ought* to be changed to F and the description updated
> to indicate that the loan was renewed while overdue)
> - loan becomes overdue again
> - accrued fines start getting calculated on a second accountlines row
> - loan gets returned
> 
> During the return processing, _FixOverduesOnReturn will change the
> accounttype of accrued fines from FU to F.  However, it currently assumes
> that there is exactly one such fine; if there are multiple ones, only one
> gets its accounttype changed.

Fully agree with you.
Comment 6 Kyle M Hall 2015-11-06 17:22:01 UTC
*** Bug 9614 has been marked as a duplicate of this bug. ***
Comment 7 Kyle M Hall 2015-11-06 18:24:10 UTC Comment hidden (obsolete)
Comment 8 Claudio Costales 2015-11-06 18:49:43 UTC
(In reply to Galen Charlton from comment #2)
> I don't think that renewing a loan actually converts accrued overdue fines
> to committed/closed ones.  Rather, I think the sequence of events is this:
> 
> - make loan
> - loan becomes overdue
> - accrued fines start getting calculated (accounttype = FU)
> - loan is renewed
> - accrued fine record is not changed (and while the fine itself shouldn't go
> away, the accounttype *ought* to be changed to F and the description updated
> to indicate that the loan was renewed while overdue)
> - loan becomes overdue again
> - accrued fines start getting calculated on a second accountlines row
> - loan gets returned
> 
> During the return processing, _FixOverduesOnReturn will change the
> accounttype of accrued fines from FU to F.  However, it currently assumes
> that there is exactly one such fine; if there are multiple ones, only one
> gets its accounttype changed.

Galen,

It seems that the problem described by you in the last paragraph of comment 2 is not fixed in this ticket. Do you think it would be appropriate to open a new ticket to address this problem?
Comment 9 Kyle M Hall 2015-11-06 18:55:11 UTC Comment hidden (obsolete)
Comment 10 Claudio Costales 2015-11-06 19:12:29 UTC
(In reply to Galen Charlton from comment #2)
> I don't think that renewing a loan actually converts accrued overdue fines
> to committed/closed ones.  Rather, I think the sequence of events is this:
> 
> - make loan
> - loan becomes overdue
> - accrued fines start getting calculated (accounttype = FU)
> - loan is renewed
> - accrued fine record is not changed (and while the fine itself shouldn't go
> away, the accounttype *ought* to be changed to F and the description updated
> to indicate that the loan was renewed while overdue)
> - loan becomes overdue again
> - accrued fines start getting calculated on a second accountlines row
> - loan gets returned
> 
> During the return processing, _FixOverduesOnReturn will change the
> accounttype of accrued fines from FU to F.  However, it currently assumes
> that there is exactly one such fine; if there are multiple ones, only one
> gets its accounttype changed.

Galen, I will think this problem all over again. May be it works. Sorry.
Comment 11 Jonathan Druart 2015-11-13 09:48:35 UTC
Does it affect 3.18.x only?
Comment 12 Claudio Costales 2015-11-13 10:44:25 UTC
Same problem in 3.20.04 and 3.20.05
Comment 13 Katrin Fischer 2015-11-16 20:42:49 UTC
Hi Kyle and all, 

1) some problem noted by the QA script:

 FAIL	C4/Circulation.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  spelling
   FAIL	  valid
		"my" variable $borrower masks earlier declaration in same scope 

2) Also: can you please shorten the bug title a bit and explain what the consequence of the fine not changed is in the commit message?

3) I'd really like to see a regression test highlighting the changed behaviour.
Comment 14 Kyle M Hall 2015-12-01 16:55:04 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-12-01 16:55:12 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-12-01 16:57:51 UTC
> 1) some problem noted by the QA script:
> 
>  FAIL	C4/Circulation.pm
>    OK	  critic
>    OK	  forbidden patterns
>    OK	  pod
>    OK	  spelling
>    FAIL	  valid
> 		"my" variable $borrower masks earlier declaration in same scope 

I cannot reproduce this qa bark. perl -c also doesn't give me any warnings.

 
> 2) Also: can you please shorten the bug title a bit and explain what the
> consequence of the fine not changed is in the commit message?

Done! There is little consequence to the bug except that lots of closed out fines appear to be accruing from a librarian's perspective.
 
> 3) I'd really like to see a regression test highlighting the changed
> behaviour.

Done!
Comment 17 Jonathan Druart 2015-12-01 17:04:15 UTC
(In reply to Kyle M Hall from comment #16)
> > 1) some problem noted by the QA script:
> > 
> >  FAIL	C4/Circulation.pm
> >    OK	  critic
> >    OK	  forbidden patterns
> >    OK	  pod
> >    OK	  spelling
> >    FAIL	  valid
> > 		"my" variable $borrower masks earlier declaration in same scope 
> 
> I cannot reproduce this qa bark. perl -c also doesn't give me any warnings.

perl -w will give you the warnings.
Comment 18 Kyle M Hall 2015-12-01 17:59:56 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2015-12-01 18:00:32 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Kyle M Hall from comment #16)
> > > 1) some problem noted by the QA script:
> > > 
> > >  FAIL	C4/Circulation.pm
> > >    OK	  critic
> > >    OK	  forbidden patterns
> > >    OK	  pod
> > >    OK	  spelling
> > >    FAIL	  valid
> > > 		"my" variable $borrower masks earlier declaration in same scope 
> > 
> > I cannot reproduce this qa bark. perl -c also doesn't give me any warnings.
> 
> perl -w will give you the warnings.

Thanks! Still not sure why koha-qa.pl didn't tell me that. Most odd.
Comment 20 Katrin Fischer 2016-01-11 21:50:25 UTC
Hm, I might have found a case where this doesn't work quite right:

Apply patch. Follow test plan:
- Make sure there is an overdue item with a fine.
- Renew overdue item with due date A.
- Observe that fine type changes from FU to F.
- Check out the item again - with a different due date B.
- Run fines again.
- Observe that fine is now F - although it should still be accrueing and FU.

Kyle, can you please check?
Comment 21 Katrin Fischer 2016-01-11 21:51:39 UTC
... I should add that switching back to master and repeating the process with a third due date C closer to today, the new fine added is FU. So there seems to be a difference from this patch.
Comment 22 Kyle M Hall 2016-01-12 15:14:15 UTC
I was unable to reproduce: http://screencast.com/t/LRtwBd6ax
Comment 23 Katrin Fischer 2016-01-12 15:41:12 UTC
I will retest
Comment 24 Kyle M Hall 2016-01-12 15:41:33 UTC
(In reply to Katrin Fischer from comment #23)
> I will retest

Thanks!
Comment 25 Kyle M Hall 2016-01-29 16:19:12 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2016-01-29 16:49:59 UTC
(In reply to Kyle M Hall from comment #25)
> Created attachment 47465 [details] [review] [review]
> Bug 14390 [QA Followup] - Allow OpacFineNoRenewals to be 0

Wrong bug! Patch obsoleted.
Comment 27 Katrin Fischer 2016-04-12 05:46:02 UTC
*** Bug 16244 has been marked as a duplicate of this bug. ***
Comment 28 René Salas 2016-04-13 23:31:02 UTC
*** Bug 16255 has been marked as a duplicate of this bug. ***
Comment 29 Katrin Fischer 2016-05-08 11:35:29 UTC
I am sorry, took me too long to get back to this - patch currently doesn't apply:

Applying: Bug 14390 - Fine not updated from 'FU' to 'F' on renewal
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm

I took a quick look, but not sure what caused the conflict in the first place (which change/fix in master).
Comment 30 Kyle M Hall 2016-05-19 15:14:53 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2016-05-19 15:15:03 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2016-05-19 15:15:06 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2016-08-19 06:35:48 UTC
QA: Looking here now..
Comment 34 Marcel de Rooy 2016-08-19 07:52:08 UTC
You won't believe it:
[Fri Aug 19 09:44:56 2016] [error] [client 82.173.53.17] [Fri Aug 19 09:44:56 2016] renew: Can't use string ("2144746608") as a HASH ref while "strict refs" in use at /usr/share/koha/masterclone/C4/Overdues.pm line 508., referer: https://master.rijkskoha.nl:28443/cgi-bin/koha/circ/circulation.pl?borrowernumber=1

UpdateFine is not called with a param hash. Just hope that this changed over time while this patch was waiting for some attention..
Comment 35 Jacek Ablewicz 2016-08-19 07:55:36 UTC
(In reply to Marcel de Rooy from comment #34)

> UpdateFine is not called with a param hash. Just hope that this changed over
> time while this patch was waiting for some attention..

It did.
Comment 36 Marcel de Rooy 2016-08-19 08:15:38 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2016-08-19 08:15:50 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2016-08-19 08:15:57 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2016-08-19 08:16:10 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2016-08-19 08:16:56 UTC
(In reply to Jacek Ablewicz from comment #35)
> (In reply to Marcel de Rooy from comment #34)
> 
> > UpdateFine is not called with a param hash. Just hope that this changed over
> > time while this patch was waiting for some attention..
> 
> It did.
That helps :)
Comment 41 Marcel de Rooy 2016-08-19 08:17:19 UTC
Still working on this. Please wait..
Comment 42 Jacek Ablewicz 2016-08-19 08:19:23 UTC
UpdateFine() calling convention got changed in Bug 15675. Bug 15675 has some other unpleasant side effects (see Bug 17135) which are not yet resolved, and it looks like this report may be helpful for fixing Bug 17135 (?). IMO both those reports desperately need some more attention - Bug 17135 is pretty nasty, and stable / 16.05 is affected by it as well.
Comment 43 Marcel de Rooy 2016-08-19 08:35:37 UTC
I tested by creating an overdue, backdating it, ran fines, renewed it.
The fine is set to type F. That is ok.

But we are not ready yet. What will happen if it gets overdue again?
I backdated the due date and the last renewal date.
Ran fines again. And here comes the problem:
The accountline that has type F but still has the issue id(!), got updated with the new amount and was set back to FU. (So: old fine got lost!) UpdateFine does not respect the F type here; the issue id is leading.
Note that in this case the test ..amount<>..amount in UpdateFine may be misleading too.

How to solve this? Can we remove the issue id in accountlines after switching to F? Or should UpdateFine no longer update a F record although it has the right issue id, but add a new record?

Please add a follow-up. This should be tested again. So reset to Needs Signoff.

Failed QA
Comment 44 Marcel de Rooy 2016-08-19 08:36:59 UTC
(In reply to Jacek Ablewicz from comment #42)
> UpdateFine() calling convention got changed in Bug 15675. Bug 15675 has some
> other unpleasant side effects (see Bug 17135) which are not yet resolved,
> and it looks like this report may be helpful for fixing Bug 17135 (?). IMO
> both those reports desperately need some more attention - Bug 17135 is
> pretty nasty, and stable / 16.05 is affected by it as well.

OK Our comments crossed each other. The new report describes it too.
Comment 45 Katrin Fischer 2016-08-19 09:04:25 UTC
> How to solve this? Can we remove the issue id in accountlines after
> switching to F? Or should UpdateFine no longer update a F record although it
> has the right issue id, but add a new record?

I think it should add a new line as it's a 'new fine'. If we differentiate between F and FU cleanly, we can keep the issue_id too, I think?
Comment 46 Marcel de Rooy 2016-08-19 10:43:10 UTC
Just a side note:

L2921 GetMember call (always)
L2975 GetMemberDetails call ( RenewalSendNotice==1)
Comment 47 Kyle M Hall 2016-08-22 14:14:32 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2016-08-22 14:16:09 UTC
(In reply to Katrin Fischer from comment #45)
> > How to solve this? Can we remove the issue id in accountlines after
> > switching to F? Or should UpdateFine no longer update a F record although it
> > has the right issue id, but add a new record?
> 
> I think it should add a new line as it's a 'new fine'. If we differentiate
> between F and FU cleanly, we can keep the issue_id too, I think?

Katrin has is exactly. If the same checkout goes overdue twice, the patron should have two separate fines ( and each one will show a different due date for the same checkout ).

This new followup will stop UpdateFine from updating closed out fines.
Comment 49 Marcel de Rooy 2016-08-22 19:42:27 UTC
(In reply to Kyle M Hall from comment #48)
> (In reply to Katrin Fischer from comment #45)
> > > How to solve this? Can we remove the issue id in accountlines after
> > > switching to F? Or should UpdateFine no longer update a F record although it
> > > has the right issue id, but add a new record?
> > 
> > I think it should add a new line as it's a 'new fine'. If we differentiate
> > between F and FU cleanly, we can keep the issue_id too, I think?
> 
> Katrin has is exactly. If the same checkout goes overdue twice, the patron
> should have two separate fines ( and each one will show a different due date
> for the same checkout ).
> 
> This new followup will stop UpdateFine from updating closed out fines.

We are coming closer to a solution!
But please note that we need type F (imo) in the totalamount calculation and the max fine reduction. We should focus on what happens next in UpdateFine.
The test $data->{'amount'} != $amount is not sufficient. Here we should exclude type F somehow. But take care with $type that you get from CalcFine. It may not be what you expect it to be ..
Comment 50 Katrin Fischer 2016-08-23 05:55:06 UTC
I think Jacek is right - we need the F fines for the total amount calculation in order to not break the MaxFine behaviour:

    if (my $maxfine = C4::Context->preference('MaxFine')) {
        if ($total_amount_other + $amount > $maxfine) { ...
Comment 51 Jacek Ablewicz 2016-08-23 09:16:32 UTC
(In reply to Katrin Fischer from comment #50)
> I think Jacek is right - we need the F fines for the total amount
> calculation in order to not break the MaxFine behaviour:

s/Jacek/Marcel/, but yep, the last patch changes existing MaxFine behaviour quite dramatically.

While this feature current behaviour looks a bit broken to me (hard to say, syspref description is far from clear), implementation is lacking (there is no need to fetch all that old fines from accountlines in UpdateFine() even when this syspref is not enabled), and - call me a Marxist - IMO it's socially unjust feature ;), sorting it out (if really necessary) is outside of the scope of this report.
Comment 52 Jacek Ablewicz 2016-08-23 09:47:53 UTC
(In reply to Marcel de Rooy from comment #49)
> (In reply to Kyle M Hall from comment #48)
> > (In reply to Katrin Fischer from comment #45)
> > > > How to solve this? Can we remove the issue id in accountlines after
> > > > switching to F? Or should UpdateFine no longer update a F record although it
> > > > has the right issue id, but add a new record?
> > > 
> > > I think it should add a new line as it's a 'new fine'. If we differentiate
> > > between F and FU cleanly, we can keep the issue_id too, I think?
> > 
> > Katrin has is exactly. If the same checkout goes overdue twice, the patron
> > should have two separate fines ( and each one will show a different due date
> > for the same checkout ).
> > 
> > This new followup will stop UpdateFine from updating closed out fines.
> 
> We are coming closer to a solution!
> But please note that we need type F (imo) in the totalamount calculation and
> the max fine reduction. We should focus on what happens next in UpdateFine.

IMO adding the filter (for FU, or FU|O|M ?) in

   if ( $rec->{issue_id} == $issue_id ) {

should do the trick.

> The test $data->{'amount'} != $amount is not sufficient. Here we should
> exclude type F somehow.

This test is kind of broken by itself (not always reliable, Bug 17138) but excluding F fines in there will not be sufficient IMO - it would prevent updating of the wrong fine record (in case when the one in $data is an incorrect match), but the right fine record update may get skipped too in such case.
Comment 53 Jacek Ablewicz 2016-08-23 10:14:54 UTC
(In reply to Marcel de Rooy from comment #49)

> But take care with $type that you get from CalcFine.
> It may not be what you expect it to be ..

Looks like $type argument passed to UpdateFine() is always NULL / undef, setting it to the custom string is not possible in Administration -> Circulation and fines rules, or anywhere else in the system (not implemented?). But in case it's not NULL in the database, it affects just a fine description and the log entry. 

There doesn't seem to be anything wrong with it - ?
Comment 54 Marcel de Rooy 2016-08-23 12:51:03 UTC
(In reply to Jacek Ablewicz from comment #53)
> (In reply to Marcel de Rooy from comment #49)
> 
> > But take care with $type that you get from CalcFine.
> > It may not be what you expect it to be ..
> 
> Looks like $type argument passed to UpdateFine() is always NULL / undef,
> setting it to the custom string is not possible in Administration ->
> Circulation and fines rules, or anywhere else in the system (not
> implemented?). But in case it's not NULL in the database, it affects just a
> fine description and the log entry. 
> 
> There doesn't seem to be anything wrong with it - ?

Well anything? :)
These kind of things make maintaining code only harder. They implicitly suggest something while it is not there.
Comment 55 Jacek Ablewicz 2016-08-23 13:06:44 UTC
(In reply to Marcel de Rooy from comment #54)
> (In reply to Jacek Ablewicz from comment #53)
> > (In reply to Marcel de Rooy from comment #49)
> > 
> > > But take care with $type that you get from CalcFine.
> > > It may not be what you expect it to be ..
> > 
> > Looks like $type argument passed to UpdateFine() is always NULL / undef,
> > setting it to the custom string is not possible in Administration ->
> > Circulation and fines rules, or anywhere else in the system (not
> > implemented?). But in case it's not NULL in the database, it affects just a
> > fine description and the log entry. 
> > 
> > There doesn't seem to be anything wrong with it - ?
> 
> Well anything? :)
> These kind of things make maintaining code only harder. They implicitly
> suggest something while it is not there.

Couldn't agree more.. What I meant was: anything wrong with it in UpdateFine() in particular, which may make a proposed solutions insufficient or more problematic?
Comment 56 Jacek Ablewicz 2016-08-23 13:43:13 UTC
P.S.: And sorry for the logorrhea ;). I'm just getting worried that it may miss a deadline for the upcoming 16.05 maintenance release.
Comment 57 Marcel de Rooy 2016-08-23 14:48:13 UTC Comment hidden (obsolete)
Comment 58 Marcel de Rooy 2016-08-23 14:51:17 UTC
Oops. Taking the liberty to replace your last follow-up, Kyle.
Kyle or Jacek: Could one (or both) of you test this one too ?
After that we can probably go the QA fast lane..
Comment 59 Marcel de Rooy 2016-08-23 14:53:16 UTC
(In reply to Jacek Ablewicz from comment #52)
> IMO adding the filter (for FU, or FU|O|M ?) in
> 
>    if ( $rec->{issue_id} == $issue_id ) {
> 
> should do the trick.

Followed that after all too. Thanks
Comment 60 Jacek Ablewicz 2016-08-23 15:23:24 UTC Comment hidden (obsolete)
Comment 61 Katrin Fischer 2016-08-23 15:29:51 UTC
Did we verify old cases of double FU from renewals are taken care of (cleaned)?
Comment 62 Jacek Ablewicz 2016-08-23 15:44:52 UTC
Note: for ensuring that MaxFine behaviour is unchanged, I did just:

1) set MaxFine to 100.00
2) commented out $accountline->set(...) in UpdateFine
3) (without the last patch): fines.pl 2>warnings.01 - I got over 1200 "Reducing fine for ... MaxFine reached" warnings in warnings.01 for my test database with 19k overdues
4) (with the last patch included): fines.pl 2>warnings.02
5) diff warnings.01 warnings.02: no differences

Some extra testing with step 2) excluded would be better, but hopefully the above is good enough. Also in my opinion code changes introduced by the last patch are 100% solid and regression-free.
Comment 63 Jacek Ablewicz 2016-08-23 15:58:32 UTC
(In reply to Katrin Fischer from comment #61)
> Did we verify old cases of double FU from renewals are taken care of
> (cleaned)?

Old 2+ FU records for the same item were taken care of in Bug 15675 (IMO quite effecively) - I don't think there is a need to include / repeat the same or similar procedure in this report. Double FUs created post-Bug 15675: there shouldn't be any (none such cases in my test DB, at least).
Comment 64 Katrin Fischer 2016-08-23 16:05:09 UTC
But doesn't the problem described here still exist after bug 15675? Renewing an item with a fine should recreate the problem - so maybe we should also run the clean-up script again?
Comment 65 Jacek Ablewicz 2016-08-23 16:05:26 UTC
(In reply to Jacek Ablewicz from comment #60)
> Created attachment 54779 [details] [review] [review]
> Bug 14390: [Follow-up] Only update FU record in UpdateFine
> 
> Exclude O, F and M when outstanding == 0.

Great idea, misa like! Not 100% perfect ;), but a lot better performance-wise in a typical production DB, just the right thing to do without rewriting ~half of this function from scratch.
Comment 66 Jacek Ablewicz 2016-08-23 16:19:14 UTC
(In reply to Katrin Fischer from comment #64)
> But doesn't the problem described here still exist after bug 15675? Renewing
> an item with a fine should recreate the problem - so maybe we should also
> run the clean-up script again?

Yeah, you are right! In the production DB, past bug 15675, there may be some 'FU' fines records still linked to the existing issue records, which are not currently accruing, looks like they should be dealt with somehow.

I wonder while I have no such cases in my test DB, probably because it got cloned from the production DB some time ago, and there was not a whole lot of renewals done in it since then.
Comment 67 Katrin Fischer 2016-08-23 16:20:55 UTC
It might also depend on configuraton - it's possible to block renewals of overdue items now, but it's a more recent feature.
Comment 68 Jacek Ablewicz 2016-08-23 17:50:10 UTC
(In reply to Katrin Fischer from comment #67)
> It might also depend on configuraton - it's possible to block renewals of
> overdue items now, but it's a more recent feature.

That wasn't it.. turns out I did wrong kind of search the 1st time :(. Sorry for overlooking this. If I do something like that instead:

  SELECT ac.*, b.cardnumber, iss.date_due
  FROM accountlines ac
  LEFT JOIN issues iss ON (iss.issue_id = ac.issue_id)
  LEFT JOIN borrowers b ON (iss.borrowernumber = b.borrowernumber)
  WHERE ac.accounttype = 'FU'
  AND iss.issue_id IS NOT NULL
  AND iss.date_due >= NOW();

sure enough I get some hits. Not a lot, and they are not "double FU"s per se, but indeed accounttype in such records should be changed from 'FU' to 'F'.
Comment 69 Jacek Ablewicz 2016-08-23 18:08:26 UTC
(In reply to Jacek Ablewicz from comment #68)

At the 1st glance it may look like follow-up for this should be relatively easy to write, but I have a feeling that this statement

>   AND iss.date_due >= NOW();

is not quite right / not good enough for covering all possible cases (doesn;t take into account grace periods settings etc.); I'd really appreciate any help in this matter from more experienced individuals.. In any case I pre-volunteer as a tester :)
Comment 70 Marcel de Rooy 2016-08-24 12:07:12 UTC
(In reply to Katrin Fischer from comment #64)
> But doesn't the problem described here still exist after bug 15675? Renewing
> an item with a fine should recreate the problem - so maybe we should also
> run the clean-up script again?

This bug did not create two FU records, Katrin. It changed a F record back to FU. Note that a record like type M (Sundry) from manual invoice does not have an issue id (only an optional item number); so would not change to FU. I am not sure if we can create type O records still (apart from adding them manually to authorized values).
I see no need to run the cleanup script in this regard.
I would suggest to open up a new report for cleaning up isolated FU records which should have been closed (as Jacek mentioned earlier).
Comment 71 Kyle M Hall 2016-08-24 12:10:07 UTC
For what it's worth, it's pretty trivial to clean up any checked in fines with a status of FU Katrin.

-- Close out any accruing fines with no current issue
UPDATE accountlines LEFT JOIN issues USING ( itemnumber, borrowernumber ) SET accounttype = 'F' WHERE accounttype = 'FU' and issues.issue_id IS NULL;

-- Close out any extra not really accruing fines, keep only the latest accruing fine
UPDATE accountlines SET accounttype = 'F' WHERE accountlines_id NOT IN ( SELECT accountlines_id FROM ( SELECT * FROM accountlines WHERE accounttype = 'FU' ORDER BY accountlines_id DESC ) a2 GROUP BY borrowernumber, itemnumber );
Comment 72 Marcel de Rooy 2016-08-24 12:16:44 UTC
(In reply to Jacek Ablewicz from comment #62)
> Note: for ensuring that MaxFine behaviour is unchanged, I did just:
> 
> 1) set MaxFine to 100.00
> 2) commented out $accountline->set(...) in UpdateFine
> 3) (without the last patch): fines.pl 2>warnings.01 - I got over 1200
> "Reducing fine for ... MaxFine reached" warnings in warnings.01 for my test
> database with 19k overdues
> 4) (with the last patch included): fines.pl 2>warnings.02
> 5) diff warnings.01 warnings.02: no differences
> 
> Some extra testing with step 2) excluded would be better, but hopefully the
> above is good enough. Also in my opinion code changes introduced by the last
> patch are 100% solid and regression-free.

Thx for your signoff and additional testing.
Comment 73 Katrin Fischer 2016-08-24 12:19:22 UTC
Alright :) Glad that this is easier than I thought initially. 
So all is ready for QA?
Comment 74 Marcel de Rooy 2016-08-24 12:20:37 UTC
(In reply to Marcel de Rooy from comment #70)
> This bug did not create two FU records, Katrin. It changed a F record back
> to FU. 

To be more precise: Without this patch, the FU did not become F. Without the last follow-up the corrected F became FU again. Clear enough? :)
Comment 75 Jacek Ablewicz 2016-08-24 12:23:00 UTC
IMO update database part from Bug 15675 is of no use here (and there is no need to check for duplicated FUs for the same item again, there shouldn't be any). What I think needs to be done:

1) select the FU fine records that may need correction: for that we may need to compare a due date from issue record and the date on the end of the fine description - if the dates are the same, this is a correct FU record, no need to change anything in it

2) if the dates are different, there are two possibilities:

3.a) if the fine is not accruing / item is not currently due: it's an old fine record (either left unclosed by Bug 15675 or maybe patron renewed this item after Bug 15675 was applied in the production database; maybe it got overwritten in the meantime or maybe not, there is no easy way to tell): it needs to be closed (FU -> F)

3.b) if the fine is accruing (item is due / CalcFine() returns a positive value): this is an old record which got overwritten: leave it as FU, but correct a due date in fine description.

What do you think?
Comment 76 Marcel de Rooy 2016-08-24 12:27:55 UTC
(In reply to Jacek Ablewicz from comment #75)
> IMO update database part from Bug 15675 is of no use here (and there is no
> need to check for duplicated FUs for the same item again, there shouldn't be
> any). What I think needs to be done:
> 
> 1) select the FU fine records that may need correction: for that we may need
> to compare a due date from issue record and the date on the end of the fine
> description - if the dates are the same, this is a correct FU record, no
> need to change anything in it
> 
> 2) if the dates are different, there are two possibilities:
> 
> 3.a) if the fine is not accruing / item is not currently due: it's an old
> fine record (either left unclosed by Bug 15675 or maybe patron renewed this
> item after Bug 15675 was applied in the production database; maybe it got
> overwritten in the meantime or maybe not, there is no easy way to tell): it
> needs to be closed (FU -> F)
> 
> 3.b) if the fine is accruing (item is due / CalcFine() returns a positive
> value): this is an old record which got overwritten: leave it as FU, but
> correct a due date in fine description.
> 
> What do you think?

Sounds good. But please open up a new report.
Comment 77 Marcel de Rooy 2016-08-24 12:28:15 UTC
(In reply to Katrin Fischer from comment #73)
> Alright :) Glad that this is easier than I thought initially. 
> So all is ready for QA?

Yes. We are ready now.
Since this actually was my QA session, I am happy to promote this patch set to Passed QA now.
I leave it to anyone interested to add additional cleaning up on a new report please. 
This patch set is for 16.05 too since bug 15675 was pushed to 16.05.
Comment 78 Marcel de Rooy 2016-08-24 12:39:56 UTC
Created attachment 54821 [details] [review]
Bug 14390 - Fine not updated from 'FU' to 'F' on renewal

Test Plan:
1) Find an overdue checkout with a fine
2) Renew item, note fine is not closed out (Account type F)
3) Apply this patch
4) Find another overdue checkout with a fine
5) Renew item, note fine is now correctly closed out
6) Backdate a checkout to be already overdue ( but not have a fine since
    fines.pl hasn't run yet )
7) Renew item, note a closed out fine is created

Signed-off-by: Sean Minkel <sminkel@rcplib.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2016-08-24 12:40:02 UTC
Created attachment 54822 [details] [review]
Bug 14390 [QA Followup] - Unit Test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 80 Marcel de Rooy 2016-08-24 12:40:09 UTC
Created attachment 54823 [details] [review]
Bug 14390 [QA Followup] - Fix warning

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 81 Marcel de Rooy 2016-08-24 12:40:14 UTC
Created attachment 54824 [details] [review]
Bug 14390: [QA Follow-up] UpdateFine should be passed a hash

Renewing an overdue would not work.
Log shows:
renew: Can't use string ("2144746608") as a HASH ref while "strict refs" in use at C4/Overdues.pm line 508., referer: /cgi-bin/koha/circ/circulation.pl?borrowernumber=1

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Marcel de Rooy 2016-08-24 12:40:19 UTC
Created attachment 54825 [details] [review]
Bug 14390: [Follow-up] Only update FU record in UpdateFine

Exclude O, F and M when outstanding == 0.
Check if the issue_id points to a FU record.

Note: We only warn now when we see a second FU record with this issue id.
That should be a rare exception. As before, we are just counting it in
our total. Added a FIXME.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested fine on overdue. Renewed and backdated for a second fine. The F
and FU can be seen on the Fines tab and are totaled on Check out.

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 83 Jacek Ablewicz 2016-08-24 15:31:44 UTC
(In reply to Marcel de Rooy from comment #76)

> > What do you think?
> 
> Sounds good. But please open up a new report.

Let's move it to Bug 17135 (entities must not be multiplied beyond necessity ;). After all, without the 3.a. part from comment #75 or something like that, Bug 14390 is only a half of the solution for this issue - i.e. Bug 17135 is still valid and still a blocker.
Comment 84 Kyle M Hall 2016-08-24 16:25:22 UTC
Pushed to master for 16.11, thanks for the followup and qa Marcel!
Comment 85 Frédéric Demians 2016-09-08 18:23:11 UTC
Pushed in 16.05. Will be in 16.05.04.
Comment 86 Julian Maurice 2016-09-16 06:21:44 UTC
Pushed to 3.22.x, will be in 3.22.11