Bug 30222 - Auto_renew_digest still sends every day when renewals are not allowed
Summary: Auto_renew_digest still sends every day when renewals are not allowed
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 29381
Blocks: 30278 30755
  Show dependency treegraph
 
Reported: 2022-03-03 16:23 UTC by Andrew Fuerste-Henry
Modified: 2022-05-23 00:31 UTC (History)
13 users (show)

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


Attachments
Bug 30222: Only send auto renewal digest wehn there is an update (5.90 KB, patch)
2022-03-11 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Simplify logic for reporting (4.77 KB, patch)
2022-03-11 13:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Only send auto renewal digest wehn there is an update (5.90 KB, patch)
2022-03-14 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Simplify logic for reporting (4.77 KB, patch)
2022-03-14 14:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Only send auto renewal digest wehn there is an update (5.90 KB, patch)
2022-03-14 14:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Simplify logic for reporting (4.77 KB, patch)
2022-03-14 14:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Further simplify logic (3.28 KB, patch)
2022-03-15 13:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30222: Only send auto renewal digest wehn there is an update (5.97 KB, patch)
2022-03-15 15:06 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30222: Simplify logic for reporting (4.83 KB, patch)
2022-03-15 15:06 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30222: Further simplify logic (3.35 KB, patch)
2022-03-15 15:06 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30222: Only send auto renewal digest wehn there is an update (6.04 KB, patch)
2022-03-25 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30222: Simplify logic for reporting (4.91 KB, patch)
2022-03-25 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30222: Further simplify logic (3.42 KB, patch)
2022-03-25 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30222: Remove warning (1.34 KB, patch)
2022-03-25 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30222: Remove warning (1.49 KB, patch)
2022-03-25 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-03-03 16:23:21 UTC
The non-digest auto-renew notice checks to see whether or not the auto-renew error on the issue has changed since the previous day. If the error is not new, the notice doesn't generate. 

In the auto-renew-digest, we don't do that check. For any error other than auto_too_soon, the patron will be notified every day. 

Bug 29381 tried to address this, but did not correct the whole issue.
Comment 1 Nick Clemens 2022-03-11 12:45:18 UTC
Created attachment 131586 [details] [review]
Bug 30222: Only send auto renewal digest wehn there is an update

Currently, the code sends an email to the patron when there is any error that is not
'too_soon' - we also don't include issues that are 'too_soon', so emials may be missing information

This patch changes the code to:
1 - push all issues that were checked to the report when a borrower wants a digest
2 - adds a variable to track when any of the issues has been updated, and checkes this when
    sending digests
3 - if nothing has been updated, no report is sent to the patron

To test:
 1 - Checkout an item, marked for autorenewal, backdated to be overdue to a patron
 2 - Set preferences
    AutoRenewalNotices = 'according to patron messaging preferences'
    EnhancedMessagingPreferences = 'Allow'
 3 - Set the patrons preference for auto renewal to email+digest
 4 - Ensure the patron has an email
 5 - Place an item level hold for another patron on the issued item
 6 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
 7 - Check patron record/notices tab
 8 - Note item not renewed, patron notified
 9 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
10 - Note item not renewed, status not changed, patron notified again
11 - Apply patch
12 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
13 - Item not renewed, patron no notified
14 - Checkout another item marked for auto renewal to the patron, but due in the future
     ensure it is outside of 'no renewal before'
15 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
16 - Patron not notified, items not renewd
     Change to status of 'auto_too_soon' is not notified
17 - Checkout a third item, marked for auto renew, overdue
18 - place an item level hold for another patorn on third item
19 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
20 - patron is notified, nothing renewed, all statuses reported
21 - cancel the item level hold on third item
22 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
23 - item renewed, patron notified, all statuses reported
24 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
25 - nothing renewed, patron not notified, only change is one issue 'auto_too_soon'
Comment 2 Nick Clemens 2022-03-11 13:29:57 UTC
Created attachment 131587 [details] [review]
Bug 30222: Simplify logic for reporting

This patch adds two new variables:
$wants_email and $wants digest

These are used to simplify checks on whether notices should be sent

To test:
1 - Apply patch
2 - Confirm notices are not sent of pref is 'cron' and send_notcies flag not set
3 - Confirm notices are sent if pref is cron and send_notices flag is set and borrower
    does not have preference set for auto renewals
4 - Confirm notices not sent if pref is set to follow messaging preference and borrower
    does not have preferences set for auto_renewals
5 - Confirm regular notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set but not digest
6 - Confirm digest notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set and wants digest
Comment 3 Andrew Fuerste-Henry 2022-03-14 14:23:19 UTC
I'm getting errors when running the cron with the patches, step 13 of the first test plan:

root@kohadevbox:koha(master)$ perl misc/cronjobs/automatic_renewals.pl -v --confirm
Scalar found where operator expected at misc/cronjobs/automatic_renewals.pl line 151, near ")
        $wants_email"
	(Missing operator before $wants_email?)
"my" variable $auto_renew masks earlier declaration in same statement at misc/cronjobs/automatic_renewals.pl line 165.
syntax error at misc/cronjobs/automatic_renewals.pl line 151, near ")
        $wants_email "
Global symbol "$wants_digest" requires explicit package name (did you forget to declare "my $wants_digest"?) at misc/cronjobs/automatic_renewals.pl line 155.
Global symbol "$wants_email" requires explicit package name (did you forget to declare "my $wants_email"?) at misc/cronjobs/automatic_renewals.pl line 157.
Global symbol "$borrower_preferences" requires explicit package name (did you forget to declare "my $borrower_preferences"?) at misc/cronjobs/automatic_renewals.pl line 157.
syntax error at misc/cronjobs/automatic_renewals.pl line 158, near "}"
Global symbol "$wants_email" requires explicit package name (did you forget to declare "my $wants_email"?) at misc/cronjobs/automatic_renewals.pl line 165.
Can't redeclare "my" in "my" at misc/cronjobs/automatic_renewals.pl line 166, near "my"
Global symbol "$wants_digest" requires explicit package name (did you forget to declare "my $wants_digest"?) at misc/cronjobs/automatic_renewals.pl line 178.
Global symbol "$wants_digest" requires explicit package name (did you forget to declare "my $wants_digest"?) at misc/cronjobs/automatic_renewals.pl line 197.
Global symbol "$wants_digest" requires explicit package name (did you forget to declare "my $wants_digest"?) at misc/cronjobs/automatic_renewals.pl line 201.
syntax error at misc/cronjobs/automatic_renewals.pl line 216, near "}"
misc/cronjobs/automatic_renewals.pl has too many errors.
Comment 4 Nick Clemens 2022-03-14 14:30:52 UTC
Created attachment 131659 [details] [review]
Bug 30222: Only send auto renewal digest wehn there is an update

Currently, the code sends an email to the patron when there is any error that is not
'too_soon' - we also don't include issues that are 'too_soon', so emials may be missing information

This patch changes the code to:
1 - push all issues that were checked to the report when a borrower wants a digest
2 - adds a variable to track when any of the issues has been updated, and checkes this when
    sending digests
3 - if nothing has been updated, no report is sent to the patron

To test:
 1 - Checkout an item, marked for autorenewal, backdated to be overdue to a patron
 2 - Set preferences
    AutoRenewalNotices = 'according to patron messaging preferences'
    EnhancedMessagingPreferences = 'Allow'
 3 - Set the patrons preference for auto renewal to email+digest
 4 - Ensure the patron has an email
 5 - Place an item level hold for another patron on the issued item
 6 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
 7 - Check patron record/notices tab
 8 - Note item not renewed, patron notified
 9 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
10 - Note item not renewed, status not changed, patron notified again
11 - Apply patch
12 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
13 - Item not renewed, patron no notified
14 - Checkout another item marked for auto renewal to the patron, but due in the future
     ensure it is outside of 'no renewal before'
15 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
16 - Patron not notified, items not renewd
     Change to status of 'auto_too_soon' is not notified
17 - Checkout a third item, marked for auto renew, overdue
18 - place an item level hold for another patorn on third item
19 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
20 - patron is notified, nothing renewed, all statuses reported
21 - cancel the item level hold on third item
22 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
23 - item renewed, patron notified, all statuses reported
24 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
25 - nothing renewed, patron not notified, only change is one issue 'auto_too_soon'
Comment 5 Nick Clemens 2022-03-14 14:30:56 UTC
Created attachment 131660 [details] [review]
Bug 30222: Simplify logic for reporting

This patch adds two new variables:
$wants_email and $wants digest

These are used to simplify checks on whether notices should be sent

To test:
1 - Apply patch
2 - Confirm notices are not sent of pref is 'cron' and send_notcies flag not set
3 - Confirm notices are sent if pref is cron and send_notices flag is set and borrower
    does not have preference set for auto renewals
4 - Confirm notices not sent if pref is set to follow messaging preference and borrower
    does not have preferences set for auto_renewals
5 - Confirm regular notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set but not digest
6 - Confirm digest notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set and wants digest
Comment 6 Nick Clemens 2022-03-14 14:34:11 UTC
Created attachment 131662 [details] [review]
Bug 30222: Only send auto renewal digest wehn there is an update

Currently, the code sends an email to the patron when there is any error that is not
'too_soon' - we also don't include issues that are 'too_soon', so emials may be missing information

This patch changes the code to:
1 - push all issues that were checked to the report when a borrower wants a digest
2 - adds a variable to track when any of the issues has been updated, and checkes this when
    sending digests
3 - if nothing has been updated, no report is sent to the patron

To test:
 1 - Checkout an item, marked for autorenewal, backdated to be overdue to a patron
 2 - Set preferences
    AutoRenewalNotices = 'according to patron messaging preferences'
    EnhancedMessagingPreferences = 'Allow'
 3 - Set the patrons preference for auto renewal to email+digest
 4 - Ensure the patron has an email
 5 - Place an item level hold for another patron on the issued item
 6 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
 7 - Check patron record/notices tab
 8 - Note item not renewed, patron notified
 9 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
10 - Note item not renewed, status not changed, patron notified again
11 - Apply patch
12 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
13 - Item not renewed, patron no notified
14 - Checkout another item marked for auto renewal to the patron, but due in the future
     ensure it is outside of 'no renewal before'
15 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
16 - Patron not notified, items not renewd
     Change to status of 'auto_too_soon' is not notified
17 - Checkout a third item, marked for auto renew, overdue
18 - place an item level hold for another patorn on third item
19 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
20 - patron is notified, nothing renewed, all statuses reported
21 - cancel the item level hold on third item
22 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
23 - item renewed, patron notified, all statuses reported
24 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
25 - nothing renewed, patron not notified, only change is one issue 'auto_too_soon'
Comment 7 Nick Clemens 2022-03-14 14:34:16 UTC
Created attachment 131663 [details] [review]
Bug 30222: Simplify logic for reporting

This patch adds two new variables:
$wants_email and $wants digest

These are used to simplify checks on whether notices should be sent

To test:
1 - Apply patch
2 - Confirm notices are not sent of pref is 'cron' and send_notcies flag not set
3 - Confirm notices are sent if pref is cron and send_notices flag is set and borrower
    does not have preference set for auto renewals
4 - Confirm notices not sent if pref is set to follow messaging preference and borrower
    does not have preferences set for auto_renewals
5 - Confirm regular notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set but not digest
6 - Confirm digest notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set and wants digest
Comment 8 Andrew Fuerste-Henry 2022-03-14 15:01:27 UTC
> 4 - Confirm notices not sent if pref is set to follow messaging preference
> and borrower
>     does not have preferences set for auto_renewals


My patron got a non-digest auto-renew notice via email at this step, with the syspref set to follow patron messaging prefs and the patron's messaging prefs set to receive no auto-renew notice.
Comment 9 Nick Clemens 2022-03-15 13:38:07 UTC
Created attachment 131721 [details] [review]
Bug 30222: Further simplify logic

For any borrower, we are either:
1 - following their preference, and that should set 'wants_email'
OR
2 - Never sending / following the cron, and can determine 'wants_email'
    from the 'send_notices' variable

This patch adds an 'else' if not using preferences to accomplish this

Follow test plan on previous patches
Comment 10 Andrew Fuerste-Henry 2022-03-15 15:06:47 UTC
Created attachment 131728 [details] [review]
Bug 30222: Only send auto renewal digest wehn there is an update

Currently, the code sends an email to the patron when there is any error that is not
'too_soon' - we also don't include issues that are 'too_soon', so emials may be missing information

This patch changes the code to:
1 - push all issues that were checked to the report when a borrower wants a digest
2 - adds a variable to track when any of the issues has been updated, and checkes this when
    sending digests
3 - if nothing has been updated, no report is sent to the patron

To test:
 1 - Checkout an item, marked for autorenewal, backdated to be overdue to a patron
 2 - Set preferences
    AutoRenewalNotices = 'according to patron messaging preferences'
    EnhancedMessagingPreferences = 'Allow'
 3 - Set the patrons preference for auto renewal to email+digest
 4 - Ensure the patron has an email
 5 - Place an item level hold for another patron on the issued item
 6 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
 7 - Check patron record/notices tab
 8 - Note item not renewed, patron notified
 9 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
10 - Note item not renewed, status not changed, patron notified again
11 - Apply patch
12 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
13 - Item not renewed, patron no notified
14 - Checkout another item marked for auto renewal to the patron, but due in the future
     ensure it is outside of 'no renewal before'
15 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
16 - Patron not notified, items not renewd
     Change to status of 'auto_too_soon' is not notified
17 - Checkout a third item, marked for auto renew, overdue
18 - place an item level hold for another patorn on third item
19 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
20 - patron is notified, nothing renewed, all statuses reported
21 - cancel the item level hold on third item
22 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
23 - item renewed, patron notified, all statuses reported
24 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
25 - nothing renewed, patron not notified, only change is one issue 'auto_too_soon'

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2022-03-15 15:06:52 UTC
Created attachment 131729 [details] [review]
Bug 30222: Simplify logic for reporting

This patch adds two new variables:
$wants_email and $wants digest

These are used to simplify checks on whether notices should be sent

To test:
1 - Apply patch
2 - Confirm notices are not sent of pref is 'cron' and send_notcies flag not set
3 - Confirm notices are sent if pref is cron and send_notices flag is set and borrower
    does not have preference set for auto renewals
4 - Confirm notices not sent if pref is set to follow messaging preference and borrower
    does not have preferences set for auto_renewals
5 - Confirm regular notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set but not digest
6 - Confirm digest notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set and wants digest

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Andrew Fuerste-Henry 2022-03-15 15:06:56 UTC
Created attachment 131730 [details] [review]
Bug 30222: Further simplify logic

For any borrower, we are either:
1 - following their preference, and that should set 'wants_email'
OR
2 - Never sending / following the cron, and can determine 'wants_email'
    from the 'send_notices' variable

This patch adds an 'else' if not using preferences to accomplish this

Follow test plan on previous patches

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Jonathan Druart 2022-03-18 09:33:23 UTC
IMO no more change should be done to this script before its code is moved and covered by tests.
Comment 14 Andrew Fuerste-Henry 2022-03-18 19:28:35 UTC
(In reply to Jonathan Druart from comment #13)
> IMO no more change should be done to this script before its code is moved
> and covered by tests.

I understand the need to move this code and add tests for it, but at the same time this is a very real and immediate pain point for users and I would not like to see this fix delayed.
Comment 15 Lucas Gass 2022-03-21 15:53:06 UTC
(In reply to Andrew Fuerste-Henry from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > IMO no more change should be done to this script before its code is moved
> > and covered by tests.
> 
> I understand the need to move this code and add tests for it, but at the
> same time this is a very real and immediate pain point for users and I would
> not like to see this fix delayed.

I agree with Andrew, this bug is causing patrons to receive unnecessary emails every day and becoming very painful for libraries.
Comment 16 Lucas Gass 2022-03-24 15:11:27 UTC
This bug is causing hundreds, if not thousands, of erroneous emails to be sent each day for some libraries. This leaves them with no choice but to turn off the auto-renewal feature altogether until this fix arrives.
Comment 17 Jonathan Druart 2022-03-24 15:52:08 UTC
Is this a regression from bug 29381 or the problem exists since the introduction of the feature (bug 18532)?
Comment 18 Jonathan Druart 2022-03-24 15:52:49 UTC
Quoting myself:
Bug 18532 comment 97
"""
This script would have benefit *a lot* from having the code in a module, covered by tests. Any additional enhancements to this file must take into account this move.
"""

Bug 29381 comment 12
"""
There is a terrible lack of tests in this area :-/
"""

I know I am grumpy and complaining for everything, by maybe sometimes what I am saying makes sense...
Comment 19 Lucas Gass 2022-03-24 16:09:10 UTC
> I know I am grumpy and complaining for everything, by maybe sometimes what I
> am saying makes sense...

You are not wrong or grumpy! Having tests in this area would have likely prevented this kind of regression from happening in the first place.
Comment 20 AspenCat Team 2022-03-24 17:21:06 UTC
Just wanted to comment that this is a big deal for our patrons.  The goal is to get as few emails as possible, which is why we switched to the digest.  Due to this bug the digest is now sending these extra emails and patrons are forced to decide which way they receive unwanted emails, by receiving them for each individual item or getting this unable to renew message in digest format over and over again.  Is there anything we can do to get this pushed through the community? - Bob Bennhoff
Comment 21 Jonathan Druart 2022-03-25 08:09:02 UTC
Created attachment 132205 [details] [review]
Bug 30222: Only send auto renewal digest wehn there is an update

Currently, the code sends an email to the patron when there is any error that is not
'too_soon' - we also don't include issues that are 'too_soon', so emials may be missing information

This patch changes the code to:
1 - push all issues that were checked to the report when a borrower wants a digest
2 - adds a variable to track when any of the issues has been updated, and checkes this when
    sending digests
3 - if nothing has been updated, no report is sent to the patron

To test:
 1 - Checkout an item, marked for autorenewal, backdated to be overdue to a patron
 2 - Set preferences
    AutoRenewalNotices = 'according to patron messaging preferences'
    EnhancedMessagingPreferences = 'Allow'
 3 - Set the patrons preference for auto renewal to email+digest
 4 - Ensure the patron has an email
 5 - Place an item level hold for another patron on the issued item
 6 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
 7 - Check patron record/notices tab
 8 - Note item not renewed, patron notified
 9 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
10 - Note item not renewed, status not changed, patron notified again
11 - Apply patch
12 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
13 - Item not renewed, patron no notified
14 - Checkout another item marked for auto renewal to the patron, but due in the future
     ensure it is outside of 'no renewal before'
15 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
16 - Patron not notified, items not renewd
     Change to status of 'auto_too_soon' is not notified
17 - Checkout a third item, marked for auto renew, overdue
18 - place an item level hold for another patorn on third item
19 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
20 - patron is notified, nothing renewed, all statuses reported
21 - cancel the item level hold on third item
22 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
23 - item renewed, patron notified, all statuses reported
24 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
25 - nothing renewed, patron not notified, only change is one issue 'auto_too_soon'

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2022-03-25 08:09:07 UTC
Created attachment 132206 [details] [review]
Bug 30222: Simplify logic for reporting

This patch adds two new variables:
$wants_email and $wants digest

These are used to simplify checks on whether notices should be sent

To test:
1 - Apply patch
2 - Confirm notices are not sent of pref is 'cron' and send_notcies flag not set
3 - Confirm notices are sent if pref is cron and send_notices flag is set and borrower
    does not have preference set for auto renewals
4 - Confirm notices not sent if pref is set to follow messaging preference and borrower
    does not have preferences set for auto_renewals
5 - Confirm regular notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set but not digest
6 - Confirm digest notices sent if pref is set to follow messaging preferences and borrower
    does have the preference set and wants digest

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2022-03-25 08:09:12 UTC
Created attachment 132207 [details] [review]
Bug 30222: Further simplify logic

For any borrower, we are either:
1 - following their preference, and that should set 'wants_email'
OR
2 - Never sending / following the cron, and can determine 'wants_email'
    from the 'send_notices' variable

This patch adds an 'else' if not using preferences to accomplish this

Follow test plan on previous patches

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2022-03-25 08:09:16 UTC
Created attachment 132208 [details] [review]
Bug 30222: Remove warning

Use of uninitialized value in string ne at misc/cronjobs/automatic_renewals.pl line 193.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2022-03-25 08:12:19 UTC
Comment on attachment 132208 [details] [review]
Bug 30222: Remove warning

Hum, not sure about this patch actually. But we should remove the warning.
Comment 26 Jonathan Druart 2022-03-25 08:16:25 UTC
Created attachment 132209 [details] [review]
Bug 30222: Remove warning

Use of uninitialized value in string ne at misc/cronjobs/automatic_renewals.pl line 193.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2022-03-25 08:17:49 UTC
Nick, this patch looks correct, but I may be missing why you didn't keep it like it was before.

Can you double check it please? In doubt, obsolete it.
Comment 28 Nick Clemens 2022-03-25 12:37:00 UTC
(In reply to Jonathan Druart from comment #27)
> Nick, this patch looks correct, but I may be missing why you didn't keep it
> like it was before.
> 
> Can you double check it please? In doubt, obsolete it.


$auto_renew->auto_renew_error is the result of last run, or renewal has not been attempted previously. In either case, it means that current run either:
1 - renewed it, so remains unset 
2 - didn't renew it and we need to update it

Either case means the issue was update and belongs in the report, and avoids checking an undef value against $error

if it has value we should compare to $error
Comment 29 Fridolin Somers 2022-04-05 15:29:08 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 30 Kyle M Hall 2022-04-08 14:13:31 UTC
Pushed to 21.11.x for 21.11.05
Comment 31 Andrew Fuerste-Henry 2022-04-15 18:21:15 UTC
Pushed to 21.05.x for 21.05.14
Comment 32 Victor Grousset/tuxayo 2022-04-17 20:25:25 UTC
Missing dependencies for 20.11.x, it shouldn't be affected, no backport.