Bug 17952 - Lost items not skipped by overdue_notices.pl
Summary: Lost items not skipped by overdue_notices.pl
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 18509 (view as bug list)
Depends on:
Blocks: 18835
  Show dependency treegraph
 
Reported: 2017-01-20 14:37 UTC by Kyle M Hall
Modified: 2017-06-21 13:59 UTC (History)
6 users (show)

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


Attachments
Bug 17952 - Lost items not skipped by overdue_notices.pl (3.28 KB, patch)
2017-01-20 14:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17952 - Lost items not skipped by overdue_notices.pl (3.39 KB, patch)
2017-01-25 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17952 - Lost items not skipped by overdue_notices.pl (3.45 KB, patch)
2017-05-01 18:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17952 - Lost items not skipped by overdue_notices.pl (3.52 KB, patch)
2017-05-02 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-01-20 14:37:26 UTC
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.
Comment 1 Kyle M Hall 2017-01-20 14:44:01 UTC
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
Comment 2 Kyle M Hall 2017-01-25 13:54:53 UTC
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>
Comment 3 Jonathan Druart 2017-01-26 11:35:52 UTC
Kyle, could you move the "$calendar" fix to its own bug?
That sounds like a major bug as well.
Comment 4 Martin Renvoize 2017-05-01 18:31:25 UTC
*** Bug 18509 has been marked as a duplicate of this bug. ***
Comment 5 Martin Renvoize 2017-05-01 18:34:14 UTC
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>
Comment 6 Martin Renvoize 2017-05-01 18:37:29 UTC
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
Comment 7 Jonathan Druart 2017-05-02 17:31:16 UTC
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>
Comment 8 Jonathan Druart 2017-05-02 17:32:40 UTC
(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 :)
Comment 9 Kyle M Hall 2017-05-08 12:40:06 UTC
Pushed to master for 17.05!
Comment 10 Katrin Fischer 2017-05-13 12:47:02 UTC
I am not clear on this one - did we calculate fines on lost items before this patch?
Comment 11 Martin Renvoize 2017-05-13 14:45:29 UTC
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)
Comment 12 Martin Renvoize 2017-05-13 14:50:07 UTC
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..
Comment 13 Katrin Fischer 2017-05-13 22:07:46 UTC
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.
Comment 14 Benjamin Rokseth 2017-06-21 08:13:11 UTC
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
Comment 15 Jonathan Druart 2017-06-21 13:59:15 UTC
(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.