Bug 29223 - Auto-renewals can fail when not digested per branch and patron requests digest
Summary: Auto-renewals can fail when not digested per branch and patron requests digest
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 28581
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-13 15:18 UTC by Nick Clemens
Modified: 2022-06-06 20:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.06


Attachments
Bug 29223: Use patron's branchcode for digest if not sending digest per branch (2.66 KB, patch)
2021-10-13 15:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29223: Use patron's branchcode for digest if not sending digest per branch (2.72 KB, patch)
2021-10-13 17:22 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29223: Use patron's branchcode for digest if not sending digest per branch (2.87 KB, patch)
2021-10-31 13:38 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-10-13 15:18:38 UTC
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
Comment 1 Nick Clemens 2021-10-13 15:48:59 UTC
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
Comment 2 Andrew Fuerste-Henry 2021-10-13 17:22:47 UTC
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>
Comment 3 Andrew Fuerste-Henry 2021-10-13 17:29:32 UTC
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.
Comment 4 Joonas Kylmälä 2021-10-31 13:08:06 UTC
(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...
Comment 5 Joonas Kylmälä 2021-10-31 13:38:17 UTC
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>
Comment 6 Joonas Kylmälä 2021-10-31 13:40:27 UTC
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)
Comment 7 Joonas Kylmälä 2021-10-31 14:24:08 UTC
(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.
Comment 8 Nick Clemens 2021-11-01 17:36:40 UTC
(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
Comment 9 Joonas Kylmälä 2021-11-01 18:28:12 UTC
(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.
Comment 10 Jonathan Druart 2021-11-02 15:53:57 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-11-11 01:06:52 UTC
Will be in 21.05.06
Comment 12 Fridolin Somers 2021-11-11 01:07:09 UTC
Depends on Bug 28581 not in 20.11.x
Comment 13 David Cook 2021-11-15 04:54:26 UTC
This one was driving me a little crazy...