Bug 28581 updated send_digest in automatic_renewals.pl to use $library->from_email_address, however, there is no branchcode parameter passed when not doing 'digest_as_branch' and so the call will fail
Created attachment 126222 [details] [review] Bug 29223: Use patron's branchcode for digest if not sending digest per branch To test: 1 - Set system preference AutoRenewalNotices to 'according to patron messaging preferences' 2 - Set circ rule for auto renewal - 100 renewals allowed - no renewal before 100 This shoud make a checkout eligible for many auto renewals for testing 3 - Checkout an item to a patron, ensure item is from a different branch than patron 4 - Ensure the patron has an email, and set an email for patron's branch and items branch 5 - Set the patron messaging preferences for auto-renewal to 'email' and 'wants digest' 6 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm 7 - Job dies: Can't call method "from_email_address" on an undefined value at /usr/share/koha/bin/cronjobs/automatic_renewals.pl line 286. 8 - Apply patch 9 - Repeat 10 - NO errors 11 - Check patron notices tab, should be a notice enqueued, check DB to see from address is patron's branch 12 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm --digest-per-branch 13 - No errors 14 - Check patron notices tab, should be a notice enqueued, check DB to see from address is item's branch
Created attachment 126228 [details] [review] Bug 29223: Use patron's branchcode for digest if not sending digest per branch To test: 1 - Set system preference AutoRenewalNotices to 'according to patron messaging preferences' 2 - Set circ rule for auto renewal - 100 renewals allowed - no renewal before 100 This shoud make a checkout eligible for many auto renewals for testing 3 - Checkout an item to a patron, ensure item is from a different branch than patron 4 - Ensure the patron has an email, and set an email for patron's branch and items branch 5 - Set the patron messaging preferences for auto-renewal to 'email' and 'wants digest' 6 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm 7 - Job dies: Can't call method "from_email_address" on an undefined value at /usr/share/koha/bin/cronjobs/automatic_renewals.pl line 286. 8 - Apply patch 9 - Repeat 10 - NO errors 11 - Check patron notices tab, should be a notice enqueued, check DB to see from address is patron's branch 12 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm --digest-per-branch 13 - No errors 14 - Check patron notices tab, should be a notice enqueued, check DB to see from address is item's branch Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
This worked as described. In step 14, the from_address matches the item's *holding branch.* I tried it with both possible values for HomeOrHolding, in case it was following that, but it didn't make a difference. I wasn't clear if that was the intended behavior, from the test plan.
(In reply to Andrew Fuerste-Henry from comment #3) > This worked as described. > In step 14, the from_address matches the item's *holding branch.* I tried it > with both possible values for HomeOrHolding, in case it was following that, > but it didn't make a difference. I wasn't clear if that was the intended > behavior, from the test plan. The step 14 just just describing that the --digest-per-branch behaviour is the same before and after this patch, the patch here doesn't change that at all (so if it is buggy you need to open a new bug report for that). I actually noticed also two other bugs in this script as well: 1) The individual messages are sent also in addition to the digest messages (only digest should be sent if enabled) 2) The individual messages don't use the branch specific letter templates, only global But those are unrelated to this bug, just mentioning them here...
Created attachment 127137 [details] [review] Bug 29223: Use patron's branchcode for digest if not sending digest per branch To test: 1 - Set system preference AutoRenewalNotices to 'according to patron messaging preferences' 2 - Set circ rule for auto renewal - 100 renewals allowed - no renewal before 100 This shoud make a checkout eligible for many auto renewals for testing 3 - Checkout an item to a patron, ensure item is from a different branch than patron 4 - Ensure the patron has an email, and set an email for patron's branch and items branch 5 - Set the patron messaging preferences for auto-renewal to 'email' and 'wants digest' 6 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm 7 - Job dies: Can't call method "from_email_address" on an undefined value at /usr/share/koha/bin/cronjobs/automatic_renewals.pl line 286. 8 - Apply patch 9 - Repeat 10 - NO errors 11 - Check patron notices tab, should be a notice enqueued, check DB to see from address is patron's branch 12 - perl misc/cronjobs/automatic_renewals.pl --send-notices --confirm --digest-per-branch 13 - No errors 14 - Check patron notices tab, should be a notice enqueued, check DB to see from address is item's branch Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Passing QA. And btw: (In reply to Joonas Kylmälä from comment #4) > 1) The individual messages are sent also in addition to the digest messages > (only digest should be sent if enabled) This was not true, I glanced over the code too quickly. Will file new bug reports for those other bugs in this script discussed here (also found yet another bug, the digest messages are sent every day / cron run if the reason is auto_too_soon)
(In reply to Joonas Kylmälä from comment #6) > Passing QA. Have to take back the PQA. The letter template is changed from global to the patron's library specific one. For the individual messages we use currently the global letter template. The line > branchcode => $params->{branchcode}, should be restored basically so we will use the default template when $params->{branchcode} is undef. As discussed in the bug 23296 we should probably use the branch defined by CircControl but that development can be continued in bug 23296.
(In reply to Joonas Kylmälä from comment #7) > (In reply to Joonas Kylmälä from comment #6) > > Passing QA. > > Have to take back the PQA. The letter template is changed from global to the > patron's library specific one. For the individual messages we use currently > the global letter template. The line > > > branchcode => $params->{branchcode}, > > should be restored basically so we will use the default template when > $params->{branchcode} is undef. As discussed in the bug 23296 we should > probably use the branch defined by CircControl but that development can be > continued in bug 23296. I think this change is correct, defaulting to the borrowers branch for a digest and falling back to general only if one is not defined
(In reply to Nick Clemens from comment #8) > I think this change is correct, defaulting to the borrowers branch for a > digest and falling back to general only if one is not defined Hmm, I see. I guess the feature change introduced in this bug fix is not too bad: the user can just remove the branch specific digest messages and it will use the global one. So the same behaviour can be gained even after this change. I'm unobsoleting the patch I signed off and marking this as PQA.
Pushed to master for 21.11, thanks to everybody involved!
Will be in 21.05.06
Depends on Bug 28581 not in 20.11.x
This one was driving me a little crazy...