Bug 17135 - Fine for the previous overdue may get overwritten by the next one
Summary: Fine for the previous overdue may get overwritten by the next one
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jacek Ablewicz
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 14390 15675
Blocks: 18666
  Show dependency treegraph
 
Reported: 2016-08-16 15:09 UTC by Jacek Ablewicz
Modified: 2017-05-24 16:19 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17135 - Fine for the previous overdue may get overwritten by the next one (8.68 KB, patch)
2016-08-29 10:58 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17135 - Some minor changes & fixes in the script (5.00 KB, patch)
2016-08-31 14:39 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17135 - Some minor changes & fixes in the script (4.98 KB, patch)
2016-08-31 14:41 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade (2.25 KB, patch)
2016-08-31 18:01 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 17135 - Fine for the previous overdue may get overwritten by the next one (8.73 KB, patch)
2016-08-31 22:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17135 - Some minor changes & fixes in the script (5.03 KB, patch)
2016-08-31 22:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade (2.30 KB, patch)
2016-08-31 22:49 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17135: [QA Follow-up] Change location of new script to fix fines (3.77 KB, patch)
2016-09-09 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135: [QA Follow-up] Changes as to reporting (3.29 KB, patch)
2016-09-09 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135 - Fine for the previous overdue may get overwritten by the next one (8.83 KB, patch)
2016-09-16 06:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135 - Some minor changes & fixes in the script (5.12 KB, patch)
2016-09-16 06:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade (2.39 KB, patch)
2016-09-16 06:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135: [QA Follow-up] Change location of new script to fix fines (3.77 KB, patch)
2016-09-16 06:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17135: [QA Follow-up] Changes as to reporting (3.55 KB, patch)
2016-09-16 06:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Follow up] Bug 17135 - Cleaner message for updatedatabase (2.34 KB, patch)
2016-09-18 23:27 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-08-16 15:09:54 UTC
When patron renews an item which has overdue fines accruing, fine amount updating stops, but fine record remains associated with a given issue (by issue_id in accountlines). If this item becomes due again, fines.pl doesn't create new fine record for 2nd+ overdue of the same issue - it just overwrites 'amount' and 'amountoutstanding' in the existing record instead, and the previous fine for this issue gets effectively lost.

This used to work correctly in 3.18-3.22 (more or less - historically implementation was pretty convoluted, it used due date in the fine description for matching issue record with the fine record), but it's broken since 16.05; probably a regression from Bug 15675 (?).
Comment 1 Jacek Ablewicz 2016-08-17 14:15:36 UTC
Dunno how to resolve this problem properly

- reintroduce due date to the issue record <-> fine record matching in UpdateFine()
- unlink fine record from the issue record on renewal (set issue_id to NULL in fine record)
- set fine type to 'F' on renewal, and ignore records of such type in the matching code in UpdateFine()

or maybe some combination of the above? I think this problem can happen only when there is at least 1 renewal of due item involved (any kind of renewal), but I'm not entirely sure. All that legacy fine calculation code works in truly mysterious ways.
Comment 2 Marcel de Rooy 2016-08-19 08:39:10 UTC
A fix on bug 14390 might address this too.
Comment 3 Katrin Fischer 2016-08-19 09:13:14 UTC
I think the cleanest way without losing information (issue_id) would be your third option:

> - set fine type to 'F' on renewal, and ignore records of such type in the 
> matching code in UpdateFine()

As Marcel points out, Kyle had a go on fixing this on bug 14390, but it needs more work now.
Comment 4 Jacek Ablewicz 2016-08-19 09:52:39 UTC
(In reply to Katrin Fischer from comment #3)
> I think the cleanest way without losing information (issue_id) would be your
> third option:
> 
> > - set fine type to 'F' on renewal, and ignore records of such type in the 
> > matching code in UpdateFine()

That would work OK for the newly created fine records, but the ones existing in the database may need some cleaning up. This F/FU/O/.. is a long standing problem, and I'm a bit worried what kind of fine 'types' people may have in their databases (probably all kinds imaginable); not sure if this approach alone would be safe enough.

Removing issue_id on renewal would be a simplest way, question is if it's even all that useful for closed fines? I wonder what happens with this field on returns (when issue record is moved to old_issues).
Comment 5 Katrin Fischer 2016-08-19 10:04:21 UTC
issue_id ... Hm, I think you could probably use it for statistics... finding out about... not sure. Might be I just like things linked :)

I seem to remember that we talked about a clean-up script on some other bug report that I can't locate right now. I'd love if we could clean this up once and for all.

I think if we have F and FU and issue_id or no issue_id (what's with old issues before the patch?) then we will have multiple ways to say the same thing (accruing or not) and that could get quite confusing.
Comment 6 Jacek Ablewicz 2016-08-19 11:55:31 UTC
(In reply to Katrin Fischer from comment #5)
> issue_id ... Hm, I think you could probably use it for statistics... finding
> out about... not sure. Might be I just like things linked :)

Hm, issue_id is not a foreign key, it stays in fine record after check-in. I agree that it may be useful after all - for statistics, or e.g. for displaying more accurate info regarding what the given fine was for, in staff and OPAC.
Comment 7 Jacek Ablewicz 2016-08-19 12:07:56 UTC
Yet another way to fix this would be an extra column in issues table to keep an accountlines_id of the currently accruing overdue fine for this issue (to be set NULL on renewal). Good for performance, and probably for other things too in the future..
Comment 8 Kyle M Hall 2016-08-22 14:18:29 UTC
We should never remove the issue_id from an accountline. It's not a foreign key because of the way we move issues to the old_issues table. It is necessary in order to know which checkout a fine is related to.

The solution is pretty simple, we should never update closed out fines ( account type 'F' ). My latest followup on bug 14390 implements this.
Comment 9 Jacek Ablewicz 2016-08-23 10:28:02 UTC
(In reply to Kyle M Hall from comment #8)

> The solution is pretty simple, we should never update closed out fines (
> account type 'F' ). My latest followup on bug 14390 implements this.

Looks like that should solve the problem sufficiently (followup in Bug 14390 needs only a small correction so MaxFine behaviour remains unchanged). Also an idea from comment #7 turns out to be not that good after all, it would just add an unnecessary redundancy.
Comment 10 Jacek Ablewicz 2016-08-23 10:47:12 UTC
Side note: clean up of the 'F' / 'FU' fine types in existing accountlines records is already done (in Bug 15675), most likely there shouldn't be any real concerns remaining about that part any more.

Not sure if 'O' and 'M' (what is 'sundry' BTW, miscellaneous ?) may need some extra attention or not - probably a non-iussue, there are some references to those fine types remaining in the code base, but they are there from the dawn of time (pre 3.0 ?), and I don't see anything relevant in updatedatabase.
Comment 11 Marcel de Rooy 2016-08-24 12:33:16 UTC
(In reply to Jacek Ablewicz from comment #10)
> Side note: clean up of the 'F' / 'FU' fine types in existing accountlines
> records is already done (in Bug 15675), most likely there shouldn't be any
> real concerns remaining about that part any more.
> 
> Not sure if 'O' and 'M' (what is 'sundry' BTW, miscellaneous ?) may need
> some extra attention or not - probably a non-iussue, there are some
> references to those fine types remaining in the code base, but they are
> there from the dawn of time (pre 3.0 ?), and I don't see anything relevant
> in updatedatabase.

Sundry is small expenses (no issue id involved)
I think O is unused (by default at least)
If 14390 gets in, can we close this one?
Comment 12 Jacek Ablewicz 2016-08-26 09:22:41 UTC
Bug 14390 moved forward, now thanks to it if there is a clear distinction between F and FU fines in a given production database, it will stay that way in the future - for such databases problem is effectively solved.

But if F / FU distinction initially is not entirely clear - if there are some non-accruing FU fines still left in the DB - Bug 14390 is only half the solution, IMO such fine records may still get overwritten even with Bug 14390 applied.

This is not all that awfully clear - but after thinking for a while about it, I'm pretty sure that some DB clean up (at least closing the non-accruing FU fines left behind by Bug 15675) is still required for fixing an issue described by this report. Consider the following scenario (starting in some pre-Bug 15675 setup, e.g. in 3.22.x)

1) Item gets overdued, 'FU' fine record gets created, fine starts accruing
2) Patron renews this item, fine record created in step 1) stops accruing (because date due in fine description doesn't match with due date in issue record any more), fine type is still 'FU'
3) Upgrade: ... Bug 15675 gets applied ... Bug 14390 gets applied
4) Fine record from 1) is still FU and it's now linked to the issue record by issue_id
5) This item gets overdued again: instead of creating a new one, UpdateFine() will pick the same old fine record for updating (it's FU and it's linked to the existing issue), amount from the previous overdue gets overwritten.
Comment 13 Jacek Ablewicz 2016-08-26 09:32:55 UTC
Proposed solution for the issues still remaining (moved from Bug 14390):

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.
Comment 14 Jacek Ablewicz 2016-08-26 09:48:10 UTC
This part

> 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.

is optional, not that much important - arguably it may be even better to leave such records as is. If due date in description gets corrected, there wouldn't be a way to tell that this fine got overwritten in the past. Usefulness of such information (if it got overwritten or not) IMO is rather limited in most practical circumstances, but still this may be considered as a data loss by some users.
Comment 15 Jacek Ablewicz 2016-08-26 10:24:48 UTC
(In reply to Jacek Ablewicz from comment #13)

This part

> 3.a) if the fine is not accruing / item is not currently due:

giving me a big headache. Would be a simple sql statement like 

    issues.due_date > NOW()

good enough for selecting old FU fines which need to be closed (+ the unmatched due date in fine description), or do we need some more elaborate way for determining that? And we can't just close all old FU fines with unmatched dates, not taking into account their current status (due / not due, accruing / not accruing). Any opinions?
Comment 16 Jacek Ablewicz 2016-08-26 11:48:05 UTC
(In reply to Jacek Ablewicz from comment #15)
> Would be a simple sql statement like 
> 
>     issues.due_date > NOW()
> 
> good enough for selecting old FU fines which need to be closed (+ the

I guess not good enough after all: item may be due but the fine not accruing yet: if overdue is in the grace period, or eg. in the middle of the 1st charge period, (when charge period > 1 day). With just 'issues.due_date > NOW()' there is a possibility that some unclosed old FU fines may still get left behind.

I don't see any way to fix this without involving CalcFine() and a good part of Getoverdues(), but in that case it's starting to look more like some kind of external script instead of simple DB update. Or at least I don't see that kinds of things in updatedatabase.pl - what are the community guidelines in such case?
Comment 17 Katrin Fischer 2016-08-29 05:55:51 UTC
If we don't do 3) - would the fines still be calculated correctly? I got lost a bit at some point :) But if that's the case, maybe 3) is not worth the trouble of correcting the date as long as we can manage to close all fines not accrueing.
Comment 18 Marcel de Rooy 2016-08-29 06:43:21 UTC
(In reply to Jacek Ablewicz from comment #16)
> (In reply to Jacek Ablewicz from comment #15)
> > Would be a simple sql statement like 
> > 
> >     issues.due_date > NOW()
> > 
> > good enough for selecting old FU fines which need to be closed (+ the
> 
> I guess not good enough after all: item may be due but the fine not accruing
> yet: if overdue is in the grace period, or eg. in the middle of the 1st
> charge period, (when charge period > 1 day). With just 'issues.due_date >
> NOW()' there is a possibility that some unclosed old FU fines may still get
> left behind.
> 
> I don't see any way to fix this without involving CalcFine() and a good part
> of Getoverdues(), but in that case it's starting to look more like some kind
> of external script instead of simple DB update. Or at least I don't see that
> kinds of things in updatedatabase.pl - what are the community guidelines in
> such case?

Look at e.g. fix_accountlines_rmdupfines_bug8253.pl in misc/maintenance..
Comment 19 Marcel de Rooy 2016-08-29 07:14:03 UTC
What about:

Go thru all FU records.
Look up issue record.

If no issue record or CalcFine==0, consider as old fine. Move FU to F.
If CalcFine < accountlines.amount, new fine: move FU to F too. 
Otherwise: consider fine to be correct. Will be updated next run.

There will be some data loss. Left or right..
Comment 20 Jacek Ablewicz 2016-08-29 10:58:58 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2016-08-29 11:05:18 UTC
Jacek, Just a quick glance:
Why Warn ? We always just warn.
Add a dbrev that at least prints a NOTE: <<ATTENTION...>> that tells the sysadmin to optionally run your script when upgrading.
Comment 22 Marcel de Rooy 2016-08-29 11:08:34 UTC
Change title of this report ?
Comment 23 Marcel de Rooy 2016-08-29 11:10:04 UTC
Severity to major ?
Comment 24 Jacek Ablewicz 2016-08-29 11:46:47 UTC
(In reply to Katrin Fischer from comment #17)
> If we don't do 3) - would the fines still be calculated correctly?

You mean 3.b part (correcting the dates in description)? IMO after Bug 15675 this shouldn't affect further fines calculation (no matter if this date stays as is or is optionally corrected). Post-Bug 15675, date in the description is not used in UpdateFine() for anything important any more. It still gets inserted to the description when new FU fine is created though, and because it's not modified on the possible overwrite, it may by useful for two goals:

- pre-selecting the accountlines which may be non-accruing and still in a need of closing,
- recovering the possibly overwritten fines (if any).

If someone is inclined to do so (I'm not - we are not using 16.05 in production yet), automatic or semi-automatic recovery of the overwritten fines is quite possible IMO, with some extra time and effort (a lot of it, probably). If FinesLog is enabled: in ~100% of cases (?), if not - I guess this date can still be used at least to recreate the most recent overwrite of the given fine. 

The trouble with the optional 3.b part / date corrections is: it may make such theoretical recovery process more complicated or even (if the logs are absent) no longer possible.
Comment 25 Jacek Ablewicz 2016-08-30 06:55:45 UTC
(In reply to Marcel de Rooy from comment #21)
> Jacek, Just a quick glance:
> Why Warn ? We always just warn.

Just a copy & paste from some random local script (warn adds a line number etc).

> Add a dbrev that at least prints a NOTE: <<ATTENTION...>> that tells the
> sysadmin to optionally run your script when upgrading.

Should this go to STDERR or to the STDOUT (warn, print - I see both in updatedatabase used for such purposes, print seems to be more popular lately)?
Comment 26 Marcel de Rooy 2016-08-30 12:09:44 UTC
(In reply to Jacek Ablewicz from comment #25)
> > Add a dbrev that at least prints a NOTE: <<ATTENTION...>> that tells the
> > sysadmin to optionally run your script when upgrading.
> 
> Should this go to STDERR or to the STDOUT (warn, print - I see both in
> updatedatabase used for such purposes, print seems to be more popular
> lately)?

At least, we need a print statement in an atomicupdate to alert people.
You could also think of running the script via updatedatabase?
Comment 27 Jacek Ablewicz 2016-08-31 14:39:29 UTC Comment hidden (obsolete)
Comment 28 Jacek Ablewicz 2016-08-31 14:41:46 UTC Comment hidden (obsolete)
Comment 29 Jacek Ablewicz 2016-08-31 14:47:40 UTC
(In reply to Marcel de Rooy from comment #26)

> You could also think of running the script via updatedatabase?

Sounds like a good idea, and there are some precedences for that in updatedatabase (3.01.00.053, 3.01.00.107). I'll give it a try.
Comment 30 Jacek Ablewicz 2016-08-31 18:01:06 UTC Comment hidden (obsolete)
Comment 31 Chris Cormack 2016-08-31 22:48:30 UTC
Created attachment 55080 [details] [review]
Bug 17135 - Fine for the previous overdue may get overwritten by the next one

External maintenace script for fixing unclosed (FU), non accruing fine
records which may still need FU -> F correction post-Bug 15675.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 32 Chris Cormack 2016-08-31 22:48:54 UTC
Created attachment 55081 [details] [review]
Bug 17135 - Some minor changes & fixes in the script

- all non-fatal output redirected to STDOUT (as there is an intention
to run this script from updatedatabase.pl)

- added borrowernumber and itemnumber equality checks to the SELECT
statement in getFinesForChecking() - accountlines.issue_id alone is not
entirely trustworthy (because InnoDB forgets it's highest auto_increment
after server restart), in some rare cases it may point to some random
issue for different patron and different item

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 33 Chris Cormack 2016-08-31 22:49:35 UTC
Created attachment 55082 [details] [review]
Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade

(plus warnings for 16.05+ production setups possibly already affected)

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 34 Marcel de Rooy 2016-09-09 07:25:31 UTC
QA: Looking here now
Comment 35 Marcel de Rooy 2016-09-09 09:54:47 UTC
Created attachment 55413 [details] [review]
Bug 17135: [QA Follow-up] Change location of new script to fix fines

The location of the script in misc/maintenance would be fine for
running it from the command line. But it will be a problem for several
install types when running it from the web installer.
Files from misc/maintenance go to bin/maintenance in a package install,
not to mention other installs than a dev install.

This patch moves the script to installer/data/mysql. Already there are two
other scripts run by upgradedatabase. I would rather move these three
scripts somewhere else, but we c/should do that on another report.

Fixed a small typo in a message too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2016-09-09 09:54:51 UTC
Created attachment 55414 [details] [review]
Bug 17135: [QA Follow-up] Changes as to reporting

We should at least report some grand totals when fixing fine records.
This patch also includes some cosmetic whitespace swapping.
Turned one last into an if statement.
Comment 37 Marcel de Rooy 2016-09-09 09:58:59 UTC
Sorry, cannot finish this today. So some preliminary remarks here apart from the follow-ups: 

Tested regular overdue OK
Tested changing a description OK 
Since my testing was not yet complete, I am not convinced yet :)

Just thinking out loud:
You select all accountlines with FU with an corresponding issue record, but what about hypothetical records FU that have an issue_id that does not have a record in issues anymore? Or does have issue_id NULL ? Should we do something about those two groups?
What if the update reports 10/20 closed successfully? What should I do now as sysadmin with that result?

General
You use sub BorType in Overdues. It should actually not be there anymore (imho) with the new series of Koha Objects.
Constructions with blocks and last statements are not really nice imo. If possible, prevent them? Remembers some goto's :) Adjusted one.
Why do you use DateTime directly here: my $today = DateTime->now( time_zone => C4::Context->tz() );
accrune/ing should be accrue/accruing :)

@Chris: Could you confirm that the script actually executed in your test? Did you check its results (xx records changed etc.) ?
Comment 38 Jacek Ablewicz 2016-09-11 17:25:30 UTC
(In reply to Marcel de Rooy from comment #35)

> The location of the script in misc/maintenance would be fine for
> running it from the command line. But it will be a problem for several
> install types when running it from the web installer.
> Files from misc/maintenance go to bin/maintenance in a package install,
> not to mention other installs than a dev install.

Ouch, I was completely unaware of that ("monkey see monkey do" kind of approach is known to be prone to some unexpected disadvantages occasionally ;), thanks for the follow-up.

> This patch moves the script to installer/data/mysql. Already there are two
> other scripts run by upgradedatabase. I would rather move these three
> scripts somewhere else, but we c/should do that on another report.
Comment 39 Jacek Ablewicz 2016-09-11 18:25:19 UTC
(In reply to Marcel de Rooy from comment #37)

> You select all accountlines with FU with an corresponding issue record, but
> what about hypothetical records FU that have an issue_id that does not have
> a record in issues anymore? Or does have issue_id NULL ? Should we do
> something about those two groups?

To be honest, I haven't given too much thought to those 2 kinds of hypothetically still unclosed FU records (because - if any - they are not vulnerable to this particular overwriting issue in any case). But IMHO they are most likely purely hypothetical anyway:

- Sql script in Bug 15675 closes all FU fines sans the most recent ones still associated (by itemnumber & borrowernumber) with some existing issue records

- After that, the only way (?) to effectively unlink the fine record from the issue record (effectively == from the UpdateFine() POV, with Bug 14390 included) would be the return/check in of an item. An item return / check-in will (hopefully) always result in closing the fine (FU -> F) as well?
Comment 40 Jacek Ablewicz 2016-09-13 08:23:33 UTC
(In reply to Marcel de Rooy from comment #37)

> You use sub BorType in Overdues. It should actually not be there anymore
> (imho) with the new series of Koha Objects.

> Why do you use DateTime directly here: my $today = DateTime->now( time_zone
> => C4::Context->tz() );

Mostly because half of this script is essentially dumbed-down code copied from fines.pl (parts which are needed for calling CalcFine()), and tinkering with it too much / trying to modernize it here could be kinda counterproductive IMO (especially as it needs to be backported for 16.05 too).
Comment 41 Jacek Ablewicz 2016-09-13 08:42:14 UTC
(In reply to Marcel de Rooy from comment #37)

> Constructions with blocks and last statements are not really nice imo. If
> possible, prevent them? Remembers some goto's :) Adjusted one.

Yeah, I don't expect this script to win any beauty pageants anytime soon ;), it is a bit ugly. Those statements etc. are leftovers from the earlier evolutionary stage of this script (when I was considering pasting this code directly to the updatedatabase as one giant { } block with no helper subroutines).
Comment 42 Jacek Ablewicz 2016-09-13 08:54:25 UTC
(In reply to Marcel de Rooy from comment #35)

> running it from the command line. But it will be a problem for several
> install types when running it from the web installer.

Running this script from web installer may be problematic in some setups BTW, due to possible timeouts (300 seconds in apache, AFAIRC ?). CalcFine() can be deadly slow sometimes, when it takes calendar/library closed dates into account (finesCalendar syspref set to 'noFinesWhenClosed').
Comment 43 Marcel de Rooy 2016-09-16 06:00:33 UTC
(In reply to Jacek Ablewicz from comment #42)
> (In reply to Marcel de Rooy from comment #35)
> 
> > running it from the command line. But it will be a problem for several
> > install types when running it from the web installer.
> 
> Running this script from web installer may be problematic in some setups
> BTW, due to possible timeouts (300 seconds in apache, AFAIRC ?). CalcFine()
> can be deadly slow sometimes, when it takes calendar/library closed dates
> into account (finesCalendar syspref set to 'noFinesWhenClosed').

I would not consider that as a problem for this script specifically. Some upgrades will timeout in the web installer. If needed, we should solve that on its own.
Comment 44 Marcel de Rooy 2016-09-16 06:51:38 UTC
Created attachment 55595 [details] [review]
Bug 17135 - Fine for the previous overdue may get overwritten by the next one

External maintenace script for fixing unclosed (FU), non accruing fine
records which may still need FU -> F correction post-Bug 15675.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2016-09-16 06:51:43 UTC
Created attachment 55596 [details] [review]
Bug 17135 - Some minor changes & fixes in the script

- all non-fatal output redirected to STDOUT (as there is an intention
to run this script from updatedatabase.pl)

- added borrowernumber and itemnumber equality checks to the SELECT
statement in getFinesForChecking() - accountlines.issue_id alone is not
entirely trustworthy (because InnoDB forgets it's highest auto_increment
after server restart), in some rare cases it may point to some random
issue for different patron and different item

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2016-09-16 06:51:48 UTC
Created attachment 55597 [details] [review]
Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade

(plus warnings for 16.05+ production setups possibly already affected)

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2016-09-16 06:51:52 UTC
Created attachment 55598 [details] [review]
Bug 17135: [QA Follow-up] Change location of new script to fix fines

The location of the script in misc/maintenance would be fine for
running it from the command line. But it will be a problem for several
install types when running it from the web installer.
Files from misc/maintenance go to bin/maintenance in a package install,
not to mention other installs than a dev install.

This patch moves the script to installer/data/mysql. Already there are two
other scripts run by upgradedatabase. I would rather move these three
scripts somewhere else, but we c/should do that on another report.

Fixed a small typo in a message too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2016-09-16 06:51:57 UTC
Created attachment 55599 [details] [review]
Bug 17135: [QA Follow-up] Changes as to reporting

We should at least report some grand totals when fixing fine records.
This patch also includes some cosmetic whitespace swapping.
Turned one last into an if statement.

Added a copyright statement too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2016-09-16 07:04:43 UTC
Final QA comment:
With reference to earlier comments and replies:
Looks good to me. This correction is done very carefully.

Now also tested:
Tested FU non accruing, not due OK
Tested FU non accruing, due (with grace period) OK 

The warnings on the db rev are (imvho) little bit intimidating(?), especially since they are even printed when no fines records were closed. But even then it could still be true that some fines went wrong in the past. No blocker (at all).

Passed QA
Comment 50 Kyle M Hall 2016-09-16 10:51:12 UTC
Pushed to master for 16.11, thanks Jacek, Marcel!
Comment 51 Katrin Fischer 2016-09-18 08:11:03 UTC
I just ran the db update from command line and got:
Can't open perl script "/home/katrin/kohaclone/installer/data/mysql/fix_unclosed_nonaccruing_fines_bug17135.pl": No such file or directory
Comment 52 Jacek Ablewicz 2016-09-18 19:14:52 UTC
(In reply to Katrin Fischer from comment #51)
> I just ran the db update from command line and got:
> Can't open perl script
> "/home/katrin/kohaclone/installer/data/mysql/
> fix_unclosed_nonaccruing_fines_bug17135.pl": No such file or directory

Can't fully retest it at the moment in fresh/clean install, but if I trick this database update to run again by decreasing DB version preference, it seems to work fine for me, despite the change of the script location to installer/data/mysql/.

Does this file (/home/katrin/kohaclone/installer/data/mysql/fix_unclosed_nonaccruing_fines_bug17135.pl) exists and what are its permissions?

What happens it you try to run it from CLI like this

   perl /home/katrin/kohaclone/installer/data/mysql/fix_unclosed_nonaccruing_fines_bug17135.pl --verbose

?
Comment 53 Katrin Fischer 2016-09-18 19:22:27 UTC
Hm, I think it was my fault not running the web installer from koha-shell. Running the script with the right path in the koha-shell works fine:
perl /home/vagrant/kohaclone/installer/data/mysql/fix_unclosed_nonaccruing_fines_bug17135.pl 
Sorry!
Comment 54 Katrin Fischer 2016-09-18 22:28:23 UTC
I'd like to propose rewriting the database update message a bit before release - it's not really easy to understand and would require reading through longish bugs to be reassured about how much damage or not there may be.
Comment 55 Katrin Fischer 2016-09-18 22:30:23 UTC
Currently it reads:
+            print "WARNING: There is a possibility (= just a possibility, it's configuration dependent etc.) that - due to regression introduced by Bug 15675 - some old fine records for overdued items (items which got renewed 1+ time while being overdue) may have been overwritten in your production 16.05+ database. See Bugzilla reports for Bug 14390 and Bug 17135 for more details.\n";
+            print "WARNING: Please note that this upgrade does not try to recover such overwitten old fine records (if any) - it's just an follow-up for Bug 14390, it's sole purpose is preventing eventuall further-on overwrites from happening in the future. Optional recovery of the overwritten fines (again, if any) is like, totally outside of the scope of this particular upgrade!\n";
Comment 56 Liz Rea 2016-09-18 23:27:31 UTC
Created attachment 55671 [details] [review]
[Follow up] Bug 17135 - Cleaner message for updatedatabase

To test:

Read the patch and check the message seems accurate and ok
Make sure the database updates when it is supposed to with this patch applied.
Comment 57 Frédéric Demians 2016-09-22 07:10:35 UTC
Pushed in 16.05. Will be in 16.05.04.