Summary: | Overdue notices have a bug when multiple overdues exist | ||
---|---|---|---|
Product: | Koha | Reporter: | Chris Cormack <chris> |
Component: | Notices | Assignee: | Chris Cormack <chris> |
Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
Severity: | critical | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | cnighswonger, dev_patches, paul.poulain, sophie.meynieux |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Proposed Patch
Fixed Patch Followup for the 6292 patch selecting correct list of items for overdue notices [SIGNED-OFF] Bug 6292 followup [SIGNED-OFF] Bug 6292 : Overdue notices not being generated when borrower had an overdue older than the max value of the notice triggers avoid duplication of letters Bug 6292 : Followup 2. several letters where generated if a borrower had overdues with different due_date triggering the same level |
Description
Chris Cormack
2011-05-02 23:28:52 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; 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. 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. 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. Created attachment 4027 [details] [review] Proposed Patch Actually ignore this patch, a better one will follow Created attachment 4028 [details] [review] Fixed Patch 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. Created attachment 4982 [details] [review] Followup for the 6292 patch selecting correct list of items for overdue notices Katrin, noticed that items that where due on the limit defined for a notice were listed on both the notice itself and the previous one. This patch correct this by excluding upper limit from the selection. 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. 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> 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> Created attachment 5221 [details] [review] avoid duplication of letters Several letters where generated if a borrower had overdues with different due_date triggering the same level This patch fixes the SQL request selecting the list of borrowers avoiding duplicate entries 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! I cannot find that this work is in master. Please take a moment to update the status of this bug. Pushing to master. 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) 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? 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 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 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> 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 |