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.
Created attachment 14467 [details] [review] proposed patch
Can you please add a test plan for this?
Current code looks good to me.
Current code is buggy but the proposed patch does not fix correctly the issue.
Created attachment 19505 [details] [review] Proposed new patch Test plan in commit message
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
Reply to Comment 6 : Yes I think it is the expected behavior.
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
Created attachment 19522 [details] [review] [PATCH][SIGNED OFF] Bug 9362 - Wrong query in GetUpcomingDueIssues Test described in patch comment M. Saby
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.
Pushed to master. Thanks, Fridolyn!
This patch has been pushed to 3.12.x, will be in 3.12.4. Thanks Fridolyn!
This patch is also needed for 3.8.x and 3.10.x versions. Thanks
Pushed to 3.10.x, will be in 3.10.11
Pushed to 3.8.x, will be in 3.8.18
As the advance_notice.pl script also procudes the DUE notices, the assumption that issues due today should not be selected is not correct.