|
Description
Stefan Berndtsson
2018-11-27 12:57:47 UTC
Created attachment 82668 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add --library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds a "--library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Hi Stefan, please don't forget to assign the bug to yourself when providing a patch! Created attachment 86513 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add --library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds a "--library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se> There is a bug in this one where only one of two SQL queries in overdue_notices.pl are changed to handle the owning library parameter. I will try to push a fix soon, but in the meantime I've set this back to In Discussion. Created attachment 111581 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add -library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Attached a new patch correcting the merge conflict and: --owning instead of -owning in the --man content. -library instead of --library in the test description. Created attachment 111582 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add -library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library This patch fixes the SQL statement in overdue_notice.pl Created attachment 111679 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add -library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library This patch also fixes the SQL statements in Circulation.pm Stefan has kindly informed that Gothenburg University Library has this working in production, which is great. We anticipate a proper patch from the proper source. Created attachment 111711 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add -library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library I am not the author of this patch but have created a new patch rebased on latest Koha master with the code that is currently running in production. Created attachment 111828 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add -library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> I corrected the --man where it the option was presented as -owning instead of --owning in the same commit as Sign-off. Created attachment 111872 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add --library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "--library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se> Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 111873 [details] [review] Bug 21886: (QA follow-up) Fix QA script errors This patch fixes a perlcritic issue and also replaces tabs with four spaces where appropriate. This does the trick and now passes the QA scripts. I would have liked to have seen the query in GetUpcomingDueIssues adapted for the --library case as opposed to pulling back all the results only to then filter them in perl.. but this would have also required unit tests. One for a followup bug I think. Passing QA I am not sure about the term "owning", we usually use "homebranch". Maybe we should stick with this terminology as it is widely used in Koha. Katrin, Nick, what do you think? Good question.. WE could use --use-homebranch as the current implementation is a boolean.. the '-use-' would make that a little clearer.. OR Perhaps we should change it to something more descriptive entirely.. '--branchfilter=issuebranch|homebranch' to make it clear that we're applying a filter on the lookup and it can take either 'issuebranch' or 'homebranch'. (In reply to Martin Renvoize from comment #19) > Good question.. > > WE could use --use-homebranch as the current implementation is a boolean.. > the '-use-' would make that a little clearer.. > > OR > > Perhaps we should change it to something more descriptive entirely.. > '--branchfilter=issuebranch|homebranch' to make it clear that we're applying > a filter on the lookup and it can take either 'issuebranch' or 'homebranch'. That final one should more likely be `--frombranch=issuebranch|homebranch` I just wanted to say that I think filter is wrong here. I think replacing owning is a good point. But I'd suggest different parameter names if we want to stick with terminology: - checkout_library - item_home_library homebranch is a bit misleading as it could be the patron's or the item's. (In reply to Katrin Fischer from comment #21) > I just wanted to say that I think filter is wrong here. > > I think replacing owning is a good point. But I'd suggest different > parameter names if we want to stick with terminology: > > - checkout_library > - item_home_library > > homebranch is a bit misleading as it could be the patron's or the item's. +1 Created attachment 112990 [details] [review] Bug 21886: (QA follow-up) Update parameters As discussed, this patch updates the --owning parameter to more clearly represent it's function. --owning boolean => --frombranch=item-homebranch|item-issuebranch Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Be great to get a final independant QA on my followup.. setting signed off. got 2 votes for checkout and library now :) Maybe missed more discussion, why not using comment#22 suggestions? (In reply to Katrin Fischer from comment #25) > got 2 votes for checkout and library now :) Maybe missed more discussion, > why not using comment#22 suggestions? Why not using comment#20 ;). I didn't get a reply to why that wasn't a good idea.. I feel my followup makes the syntax considerably clearer personally. having two boolean options that are mutually excluding and don't clearly say what they actually do in the name is far less clear to me than having one parameter that's clearly named with two clear options available. Can we reach a decision on this? (In reply to Martin Renvoize from comment #26) > (In reply to Katrin Fischer from comment #25) > > got 2 votes for checkout and library now :) Maybe missed more discussion, > > why not using comment#22 suggestions? > > Why not using comment#20 ;). > > I didn't get a reply to why that wasn't a good idea.. I feel my followup > makes the syntax considerably clearer personally. having two boolean > options that are mutually excluding and don't clearly say what they actually > do in the name is far less clear to me than having one parameter that's > clearly named with two clear options available. Happy about the structural change - was only struggling with "issue" vs. "checkout", but can "live" with it here :) Setting back to SO. Created attachment 114068 [details] [review] Bug 21886: Add option to send notices from owning library instead of issuing library The provided patch adds the following functionality: * Add --owning to both advance_notice.pl and overdue_notice.pl * Add --library to advance_notice.pl like the way overdue_notice.pl already works When specifying "--owning" both scripts will use items.homebranch instead of issues.branchcode to determine sending library. For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices. To test: 1. Have a patron who wants advance notices as email with 2 days in advance (not digest) 2. Have the first overdue date set 3 days past due date 3. Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch 4. Have issue for that patron where date_due is 3 days old 5. Run advance_notices.pl without --owning 6. Run overdue_notices.pl without --owning 7. Confirm that two messages were created for that patron with the sender being the issuing branch 8. Delete messages or create two more issues according to (3) and (4) 9. Run advance_notices.pl with --owning 10. Run overdue_notices.pl with --owning 11. Confirm that the two messages created has the item homebranch as sender Extra feature for advance_notices.pl is that it adds "--library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well. Sponsored-by: Gothenburg University Library Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se> Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114069 [details] [review] Bug 21886: (QA follow-up) Fix QA script errors This patch fixes a perlcritic issue and also replaces tabs with four spaces where appropriate. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114070 [details] [review] Bug 21886: (QA follow-up) Update parameters As discussed, this patch updates the --owning parameter to more clearly represent it's function. --owning boolean => --frombranch=item-homebranch|item-issuebranch Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114071 [details] [review] Bug 21886: (QA follow-up) Fix indentation, prevent warns, fix parameter, simplify SQL The SQL code was duplicated, I combine them here frombranch needs to take input switch owning to frombranch in second script initialize hash as empty list, not a reference add a newline after printed output if not mailing notices Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x Please fill the release notes. |