Description
Andrew Fuerste-Henry
2022-03-03 16:23:21 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' 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 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. 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' 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 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' 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 > 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.
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 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> 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> 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> IMO no more change should be done to this script before its code is moved and covered by tests. (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. (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. 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. Is this a regression from bug 29381 or the problem exists since the introduction of the feature (bug 18532)? 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... > 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.
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 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> 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> 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> 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 on attachment 132208 [details] [review] Bug 30222: Remove warning Hum, not sure about this patch actually. But we should remove the warning. 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> 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. (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 Pushed to master for 22.05, thanks to everybody involved [U+1F984] Pushed to 21.11.x for 21.11.05 Pushed to 21.05.x for 21.05.14 Missing dependencies for 20.11.x, it shouldn't be affected, no backport. |