Bug 9362 - Wrong query in GetUpcomingDueIssues (Circulation.pm)
Summary: Wrong query in GetUpcomingDueIssues (Circulation.pm)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-08 10:13 UTC by Adrien SAURAT
Modified: 2014-05-26 21:04 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (756 bytes, patch)
2013-01-08 13:10 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Proposed new patch (1.98 KB, patch)
2013-07-09 15:46 UTC, Fridolin Somers
Details | Diff | Splinter Review
[PATCH][SIGNED OFF] Bug 9362 - Wrong query in GetUpcomingDueIssues (2.72 KB, patch)
2013-07-10 14:09 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9362 - Wrong query in GetUpcomingDueIssues (2.79 KB, patch)
2013-07-12 14:43 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2013-01-08 10:13:12 UTC
C4:Circulation:GetUpcomingDueIssues is used in the advance_notices.pl script.
This script waits for a "maxdays" parameter, which is used in the following request.

We have in GetUpcomingDueIssues the following query:
SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
FROM issues 
LEFT JOIN items USING (itemnumber)
LEFT OUTER JOIN branches USING (branchcode)
WhERE returndate is NULL
AND ( TO_DAYS( NOW() )-TO_DAYS( date_due ) ) < ?

Shouldn't the last line be replaced by
"AND TO_DAYS( date_due )-TO_DAYS( NOW() ) < ?"
like what we see in the select clause?
The date_due is supposed to be "bigger" (farther) than the NOW date.

--
Also, this request gives the upcoming due issues, but also the overdues. Is it supposed to be so?
If not, the last line should in fact be:
AND ( TO_DAYS( NOW() )-TO_DAYS( date_due ) ) BETWEEN 0 AND ?
--

I'm not sure about the last point, so for now I'll make a patch including only the first change.
Comment 1 Adrien SAURAT 2013-01-08 13:10:13 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-02-05 17:32:50 UTC
Can you please add a test plan for this?
Comment 3 Jonathan Druart 2013-07-09 08:22:17 UTC
Current code looks good to me.
Comment 4 Jonathan Druart 2013-07-09 10:29:22 UTC
Current code is buggy but the proposed patch does not fix correctly the issue.
Comment 5 Fridolin Somers 2013-07-09 15:46:19 UTC Comment hidden (obsolete)
Comment 6 Mathieu Saby 2013-07-09 16:11:25 UTC
I did the following test : 

- 1 book to check in 2 days
- 2 books to check in in the past

before applying the patch : 

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=2
getting upcoming due issues at ../misc/cronjobs/advance_notices.pl line 203.
found 1 issues at ../misc/cronjobs/advance_notices.pl line 205.

I changed the value of "-m" : 0, 1, 2, 3, 4
=> always 1 issue found (the book to check in in 2 days)


after applying the patch

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=2
found 0 issues
for m = 0, 1, 2 => 0 issues

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=3
found 1 issues
for m = 3,4,5 => 1 issues (the book to check in in 2 days)


Do you consider it is the expected behavior?
If it is so, I can sign off.

Mathieu
Comment 7 Fridolin Somers 2013-07-10 13:30:48 UTC
Reply to Comment 6 : 
Yes I think it is the expected behavior.
Comment 8 Mathieu Saby 2013-07-10 14:06:17 UTC
It seems logical, and can probably explain some problem we encountered in Rennes 2 :some patrons complain they receive unwanted predue notice. We thought they were wrong, but maybe they are not...

I sign off.

M. Saby
Comment 9 Mathieu Saby 2013-07-10 14:09:25 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-07-12 14:43:59 UTC
Created attachment 19599 [details] [review]
Bug 9362 - Wrong query in GetUpcomingDueIssues

C4:Circulation:GetUpcomingDueIssues is used in the advance_notices.pl script.
This script waits for a "maxdays" parameter, which is used in the following request.

We have in GetUpcomingDueIssues the following query:
SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
FROM issues
LEFT JOIN items USING (itemnumber)
LEFT OUTER JOIN branches USING (branchcode)
WhERE returndate is NULL
AND ( TO_DAYS( NOW() )-TO_DAYS( date_due ) ) < ?

The last line should be with date_due before NOW. The date_due is supposed to be "bigger" (farther) than the NOW date.
Also, this request gives the upcoming due issues, but also the overdues.

This patch corrects this by using HAVING in query.

Test plan :
- Create an issue with a date due in the paste
- Create an issue with a date due in two days
- Launch advance notices with due date in max 2 days : perl misc/cronjobs/advance_notices.pl -c -n -v -m=2
=> You get a warn "found 0 issues"
- Launch advance notices with due date in max 3 days : perl misc/cronjobs/advance_notices.pl -c -n -v -m=3
=> You get a warn "found 1 issues"

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
I did the following test :

- 1 book to check in 2 days
- 2 books to check in in the past

before applying the patch :

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=2
getting upcoming due issues at ../misc/cronjobs/advance_notices.pl line 203.
found 1 issues at ../misc/cronjobs/advance_notices.pl line 205.

I changed the value of "-m" : 0, 1, 2, 3, 4
=> always 1 issue found (the book to check in in 2 days)

after applying the patch :

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=2
found 0 issues
for m = 0, 1, 2 => 0 issues

$perl ../misc/cronjobs/advance_notices.pl -c -n -v -m=3
found 1 issues
for m = 3,4,5 => 1 issues (the book to check in in 2 days)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa.pl, works as advertised.
Comment 11 Galen Charlton 2013-07-17 15:55:03 UTC
Pushed to master.  Thanks, Fridolyn!
Comment 12 Tomás Cohen Arazi 2013-08-13 16:56:17 UTC
This patch has been pushed to 3.12.x, will be in 3.12.4.

Thanks Fridolyn!
Comment 13 Sophie MEYNIEUX 2013-09-11 07:51:52 UTC
This patch is also needed for 3.8.x and 3.10.x versions. Thanks
Comment 14 Bernardo Gonzalez Kriegel 2013-09-13 21:20:08 UTC
Pushed to 3.10.x, will be in 3.10.11
Comment 15 Chris Hall 2013-09-15 05:08:32 UTC
Pushed to 3.8.x, will be in 3.8.18
Comment 16 Katrin Fischer 2013-11-07 22:55:08 UTC
As the advance_notice.pl script also procudes the DUE notices, the assumption that issues due today should not be selected is not correct.