https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15675 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15675 [Bug 15675] Add issue_id column to accountlines and use it for updating fines -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=14390 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- A fix on bug 14390 might address this too. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #4 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #6 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|15675 |14390 CC| |kyle@bywatersolutions.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14390 [Bug 14390] Fine not updated from 'FU' to 'F' on renewal https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15675 [Bug 15675] Add issue_id column to accountlines and use it for updating fines -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #10 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #13 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #15 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #20 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 54972 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54972&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Change title of this report ? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|blocker |major --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Severity to major ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #24 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55074|0 |1 is obsolete| | --- Comment #28 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 55075 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55075&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #30 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 55077 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55077&action=edit Bug 17135 - Maintenance script execution added as part of an atomic DB upgrade (plus warnings for 16.05+ production setups possibly already affected) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55075|0 |1 is obsolete| | --- Comment #32 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 55081 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55081&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55077|0 |1 is obsolete| | --- Comment #33 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 55082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55082&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 55413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55413&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.) ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #39 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #41 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #43 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55081|0 |1 is obsolete| | --- Comment #45 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 55596 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55596&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55413|0 |1 is obsolete| | --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 55598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55598&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #50 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jacek, Marcel! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #52 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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 ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #54 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 --- Comment #56 from Liz Rea <liz@catalyst.net.nz> --- Created attachment 55671 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55671&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17135 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |18666 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18666 [Bug 18666] Update 16.06.00.028 failing because of missing no_auto_renewal_after -- You are receiving this mail because: You are watching all bug changes.