Description
Barton Chittenden
2015-06-15 19:31:38 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 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. Kyle, Galen I think it is a "major" bug... Today I will attach the test I've done. Created attachment 41428 [details]
Step by step sample
Sorry, in spanish
(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. *** Bug 9614 has been marked as a duplicate of this bug. *** Created attachment 44600 [details] [review] Bug 14390 - On renewal, the last fine is not marked as accounttype 'F' when item is checked in. 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 (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? Created attachment 44603 [details] [review] Bug 14390 - On renewal, the last fine is not marked as accounttype 'F' when item is checked in. 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> (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. Does it affect 3.18.x only? Same problem in 3.20.04 and 3.20.05 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. Created attachment 45280 [details] [review] Bug 14390 [QA Followup] - Unit Test Created attachment 45281 [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> > 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! (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. Created attachment 45285 [details] [review] Bug 14390 [QA Followup] - Fix warning (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. 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? ... 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. I was unable to reproduce: http://screencast.com/t/LRtwBd6ax I will retest (In reply to Katrin Fischer from comment #23) > I will retest Thanks! Created attachment 47465 [details] [review] Bug 14390 [QA Followup] - Allow OpacFineNoRenewals to be 0 (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. *** Bug 16244 has been marked as a duplicate of this bug. *** *** Bug 16255 has been marked as a duplicate of this bug. *** 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). Created attachment 51627 [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> Created attachment 51628 [details] [review] Bug 14390 [QA Followup] - Unit Test Created attachment 51629 [details] [review] Bug 14390 [QA Followup] - Fix warning QA: Looking here now.. 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.. (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. Created attachment 54593 [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> Created attachment 54594 [details] [review] Bug 14390 [QA Followup] - Unit Test Created attachment 54595 [details] [review] Bug 14390 [QA Followup] - Fix warning Created attachment 54596 [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> (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 :) Still working on this. Please wait.. 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. 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 (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.
> 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?
Just a side note: L2921 GetMember call (always) L2975 GetMemberDetails call ( RenewalSendNotice==1) Created attachment 54704 [details] [review] Bug 14390 [QA Followup] - Never try to update closed out fines (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. (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 .. 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) { ... (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. (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. (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 - ? (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. (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? 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. Created attachment 54778 [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. 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.. (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 Created attachment 54779 [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> Did we verify old cases of double FU from renewals are taken care of (cleaned)? 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. (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). 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? (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. (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. It might also depend on configuraton - it's possible to block renewals of overdue items now, but it's a more recent feature. (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'. (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 :) (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). 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 ); (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. Alright :) Glad that this is easier than I thought initially. So all is ready for QA? (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? :) 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? (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. (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. 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> Created attachment 54822 [details] [review] Bug 14390 [QA Followup] - Unit Test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 54823 [details] [review] Bug 14390 [QA Followup] - Fix warning Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> (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. Pushed to master for 16.11, thanks for the followup and qa Marcel! Pushed in 16.05. Will be in 16.05.04. Pushed to 3.22.x, will be in 3.22.11 |