If a library does not use --mark-returned when running longoverdue.pl, all those lost item checkouts are selected by overdue_notices.pl. This causes much unnecessary overhead. In addition Koha::Calendar is instantiated many times for each branchcode which is not necessary.
Created attachment 59378 [details] [review] Bug 17952 - Lost items not skipped by overdue_notices.pl If a library does not use --mark-returned when running longoverdue.pl, all those lost item checkouts are selected by overdue_notices.pl. This causes much unnecessary overhead. In addition Koha::Calendar is instantiated many times for each branchcode which is not necessary. Test Plan: 1) Run overdue_notices.pl, note output 2) Apply this patch 3) Run overdue_notices.pl again, note output is the same
Created attachment 59551 [details] [review] Bug 17952 - Lost items not skipped by overdue_notices.pl If a library does not use --mark-returned when running longoverdue.pl, all those lost item checkouts are selected by overdue_notices.pl. This causes much unnecessary overhead. In addition Koha::Calendar is instantiated many times for each branchcode which is not necessary. Test Plan: 1) Run overdue_notices.pl, note output 2) Apply this patch 3) Run overdue_notices.pl again, note output is the same Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jane Leven <jleven@camdencountylibrary.org>
Kyle, could you move the "$calendar" fix to its own bug? That sounds like a major bug as well.
*** Bug 18509 has been marked as a duplicate of this bug. ***
Created attachment 62928 [details] [review] Bug 17952 - Lost items not skipped by overdue_notices.pl If a library does not use --mark-returned when running longoverdue.pl, all those lost item checkouts are selected by overdue_notices.pl. This causes much unnecessary overhead. In addition Koha::Calendar is instantiated many times for each branchcode which is not necessary. Test Plan: 1) Run overdue_notices.pl, note output 2) Apply this patch 3) Run overdue_notices.pl again, note output is the same Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jane Leven <jleven@camdencountylibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK, So in an ideal world I'd like to have seen these as separate bugs, but as it's still a fairly trivial change and the Koha::Calendar instantiation bug is fixed in the exact same way as I was intending on bug 18509 then I'm happy to sign off as a whole. Tested, but not definitively benchmarked to prove the improvement. It 'feels' more performant on the tests I ran. Signed off
Created attachment 62984 [details] [review] Bug 17952 - Lost items not skipped by overdue_notices.pl If a library does not use --mark-returned when running longoverdue.pl, all those lost item checkouts are selected by overdue_notices.pl. This causes much unnecessary overhead. In addition Koha::Calendar is instantiated many times for each branchcode which is not necessary. Test Plan: 1) Run overdue_notices.pl, note output 2) Apply this patch 3) Run overdue_notices.pl again, note output is the same Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jane Leven <jleven@camdencountylibrary.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #3) > Kyle, could you move the "$calendar" fix to its own bug? > That sounds like a major bug as well. I do not understand my own comment. It does not sound a major bug, just a speed improvement. It would have been good to have it on its own bug anyway :)
Pushed to master for 17.05!
I am not clear on this one - did we calculate fines on lost items before this patch?
To clarify, this patch adds two performance enhancements in one patch. a) It prevents re-instantiation of Koha::Calendar objects inside the loop (no functionality change here) b) It enhances the SQL query to skip all lost items during overdue calculation. (this is a change of logic, but I don't think it's a change of functionality)
Hmm, on second read.. it is a change of functionality in so much as listitems were not skipped before. In all my own test cases we have overdue rules set at times long before itemlost marking rules.. so you would never see the change.. I imagine that is the usual case.. hmm..
Yeah, I am mostly worried about pushing a behaviour change to the stable release. We don't use the longoverdues script atm, but I know libraries are manually setting the lost status when an item is reported lost.
seems a slash snuck into the patch? @@ -466,7 +467,8 @@ SELECT biblio.*, items.*, issues.*, biblioitems.itemtype, branchname AND biblio.biblionumber = biblioitems.biblionumber AND issues.borrowernumber = ? AND issues.branchcode = ? - AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 + AND items.itemlost = 0 +/ AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 END_SQL this breaks the query
(In reply to Benjamin Rokseth from comment #14) > seems a slash snuck into the patch? > > @@ -466,7 +467,8 @@ SELECT biblio.*, items.*, issues.*, > biblioitems.itemtype, branchname > AND biblio.biblionumber = biblioitems.biblionumber > AND issues.borrowernumber = ? > AND issues.branchcode = ? > - AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 > + AND items.itemlost = 0 > +/ AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 > END_SQL > > this breaks the query This is terrible, I should have caught that. See bug 18835.