Bug 6292 - Overdue notices have a bug when multiple overdues exist
Summary: Overdue notices have a bug when multiple overdues exist
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Target Milestone: Oldversions
Assignee: Chris Cormack
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-02 23:28 UTC by Chris Cormack
Modified: 2018-12-16 19:34 UTC (History)
5 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
Proposed Patch (3.17 KB, patch)
2011-05-03 00:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Fixed Patch (3.03 KB, patch)
2011-05-03 00:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Followup for the 6292 patch selecting correct list of items for overdue notices (1.26 KB, patch)
2011-08-12 15:03 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6292 followup (1.37 KB, patch)
2011-08-14 18:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6292 : Overdue notices not being generated when borrower had an overdue older than the max value of the notice triggers (3.14 KB, patch)
2011-08-14 18:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
avoid duplication of letters (2.49 KB, patch)
2011-08-30 15:03 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 6292 : Followup 2. several letters where generated if a borrower had overdues with different due_date triggering the same level (2.54 KB, patch)
2011-12-11 00:03 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2011-05-02 23:28:52 UTC
When using the -t switch for triggers, the overdue_notices.pl is only looking at the oldest overdue, to see if it matches the trigger.

This means if your oldest overdue is older than the longest period of the trigger, no notices are sent, even if you have new overdues that match the trigger.
Comment 1 Chris Cormack 2011-05-02 23:30:44 UTC
The piece of code is here

my $borrower_sql = <<'END_SQL';
SELECT COUNT(*), issues.borrowernumber, firstname, surname, address, address2, city, zipcode, country, email, MIN(date_due) as longest_issue
FROM   issues,borrowers,categories
WHERE  issues.borrowernumber=borrowers.borrowernumber
AND    borrowers.categorycode=categories.categorycode
END_SQL
            my @borrower_parameters;
            if ($branchcode) {
                $borrower_sql .= ' AND issues.branchcode=? ';
                push @borrower_parameters, $branchcode;
            }
            if ( $overdue_rules->{categorycode} ) {
                $borrower_sql .= ' AND borrowers.categorycode=? ';
                push @borrower_parameters, $overdue_rules->{categorycode};
            }
            $borrower_sql .= '  AND categories.overduenoticerequired=1
                                GROUP BY issues.borrowernumber ';
            if($triggered) {
                $borrower_sql .= ' HAVING TO_DAYS(NOW())-TO_DAYS(longest_issue) = ?';
                push @borrower_parameters, $mindays;
Comment 2 Chris Cormack 2011-05-02 23:39:36 UTC
The code is contained in a loop, so it is tested for each of the 3 triggers, but the bug still exists, if the oldest overdue is too old, no triggers will ever match.
Comment 3 dev_patches 2011-05-02 23:49:03 UTC
we came into that bug too.
It is quite severe indeed.
And in the case of more than 2 levels, the problem is to decide which items should be reported missing by the library. And which letter to send.
First assumption (and demand) was to take the more severe report and not to split the email into many pieces in order to send recalls for the appropriate level for each.
Do one report by library by user, whatever the level, and send the more severe details could be another one. 
unless decision is done via parameter or system preferences, which would lead to quite a big refactoring, flexibility cannot be achieved.
Comment 4 Chris Cormack 2011-05-03 00:21:27 UTC
I have just submitted a patch that while it doesnt change the functionality (still results in overdues being generated for each trigger) This is how it has always worked, it loops through generating notices for each trigger period, so a borrower may get multiple, I havent been able to fix that yet but what this patch changes is it will now not miss generating them when it should. Before if you had an overdue that was sufficiently old, you never ever got an overdue notice.

Rewriting the overdues system is a bug for another day, this patch will now stop it from missing sending them though.
Comment 5 Chris Cormack 2011-05-03 00:21:50 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-05-03 00:24:42 UTC
Actually ignore this patch, a better one will follow
Comment 7 Chris Cormack 2011-05-03 00:30:28 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2011-05-15 21:14:09 UTC
I tested this patch on current master:

overdue notice triggers: 
adult: 1, 2 and 3 days delays with different notices for each level
student: 2, 4 and 6 days delays with different notices for each level

I created 2 borrowers for these category codes and checked out 3 items to each with due dates in the past:
1,2,3 days in the past
2,4,6 days in the past

I started overdue_notices with -v and -t.

Without the patch only one 3rd notice for each listing the oldest item is created.

With the patch 3 notices are generated for each borrower. But the items listes arenot correct:

1st notice lists the items 1 and 2 (or 2 and 4) days due.
2nd notice lists the items 2 and 3 (4 and 6I days due.
3rd notice lists only the items 3 or 6 days due.

I think the notices should only list the items that are due the delay specified for the notice as long as the -list-all switch is not used.
Comment 9 Sophie MEYNIEUX 2011-08-12 15:03:11 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2011-08-14 17:57:49 UTC
I repeated my test plan with both patches applied:

Results: Delay 1,2,3, User has email address
- This looks ok now, the items listed in each notice are ok.
- I added a second item for the 3rd notice. The list was printed correctly.

Results: Delay 2,4,6, User has NO email address
- This looks ok too.

I have found no problems with this and I am signing off.
Comment 11 Katrin Fischer 2011-08-14 18:11:39 UTC
Created attachment 5000 [details] [review]
[SIGNED-OFF] Bug 6292 followup

selection of items to be listed in an overdue notice included
both limits (upper and lower). So items with an overdue equal
to a limit appeared on both notices. This patch fix this,
including lower limit and excluding upper limit for the selection.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 12 Katrin Fischer 2011-08-14 18:12:35 UTC
Created attachment 5001 [details] [review]
[SIGNED-OFF] Bug 6292 : Overdue notices not being generated when borrower had an overdue older than the max value of the notice triggers

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 13 Sophie MEYNIEUX 2011-08-30 15:03:30 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2011-09-01 20:39:06 UTC
All three patches are still applying to current master.

Sophie: Sorry, looking at it I don't understand the problem this is trying to fix and how to create a test case for it.

When using notice triggers the range for each notice level is exactly one day, so there can't be items with different due dates on the same level.

Do I have to use a different option from the overdue_notices script? Can you explain how to reproduce the problem? Thx!
Comment 15 Chris Nighswonger 2011-09-07 18:27:03 UTC
I cannot find that this work is in master. Please take a moment to update the status of this bug. Pushing to master.
Comment 16 Katrin Fischer 2011-09-09 11:34:25 UTC
First 2 patches are signed-off and tested on master. Problem also exists in 3.4.x and 3.2.x 

I don't know how to test the 3rd patch and need help here (see comment 14)
Comment 17 Katrin Fischer 2011-09-24 17:17:54 UTC
Can someone please provide a test plan for the 3rd patch? I am not sure if the problem blocks the other 2 patches or not?
Comment 18 Chris Cormack 2011-10-10 21:08:30 UTC
Hi Sophie

We really need a test case for your last patch, or at least how to construct a situation that will call the duplicate so we can test it
Comment 19 Sophie MEYNIEUX 2011-10-19 06:59:27 UTC
The 3rd patch should be tested without -t parameter.
If a borrower has 2 overdue items, with different due date, but both triggering the same level, 2 letters are sent.
For exemple if level 1 triggers after 10 days and level 2 after 20, if a borrower has one item with 12 days overdue and one with 17, 2 level1 letters are generated listing both items.
With the patch, only one letter is generated.

Hope this help
Comment 20 Chris Cormack 2011-12-11 00:03:36 UTC
Created attachment 6700 [details] [review]
Bug 6292 : Followup 2. several letters where generated if a borrower had overdues with different due_date triggering the same level

This patch fixes the SQL request giving the list of borrowers

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 21 Paul Poulain 2011-12-16 16:56:53 UTC
QA comment: clean code, nothing to say about it.

perlcritic unchanged by those patches (has 2 errors reported, the same before and after the patch)

test plan: see comment 3, comment 9 and comment 13 !

passed QA

patch pushed, please test