Bug 34924 - Add ability to send 'final auto renewal notice'
Summary: Add ability to send 'final auto renewal notice'
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Emily Lamancusa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-26 19:07 UTC by Nick Clemens
Modified: 2024-01-25 16:34 UTC (History)
10 users (show)

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


Attachments
Bug 34924: Add Koha::Checkout->attempt_auto_renew (8.77 KB, patch)
2023-09-26 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed (6.17 KB, patch)
2023-09-26 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Handle final renewal errors (9.61 KB, patch)
2023-09-26 19:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add Koha::Checkout->attempt_auto_renew (8.79 KB, patch)
2023-10-17 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed (6.17 KB, patch)
2023-10-17 20:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Handle final renewal errors (9.61 KB, patch)
2023-10-17 20:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add Koha::Checkout->attempt_auto_renew (9.68 KB, patch)
2023-10-23 17:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed (9.59 KB, patch)
2023-10-23 17:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Handle final renewal errors (8.89 KB, patch)
2023-10-23 17:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Adjust auto renewal email to reflect success for final renewals (2.89 KB, patch)
2023-10-23 17:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34924: Add Koha::Checkout->attempt_auto_renew (9.66 KB, patch)
2023-10-26 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed (9.66 KB, patch)
2023-10-26 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34924: Handle final renewal errors (8.96 KB, patch)
2023-10-26 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34924: Adjust auto renewal email to reflect success for final renewals (2.96 KB, patch)
2023-10-26 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34924: Add Koha::Checkout->attempt_auto_renew (9.72 KB, patch)
2023-10-26 15:02 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed (9.72 KB, patch)
2023-10-26 15:02 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34924: Handle final renewal errors (9.02 KB, patch)
2023-10-26 15:02 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34924: Adjust auto renewal email to reflect success for final renewals (3.03 KB, patch)
2023-10-26 15:02 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34924: (QA follow-up) correctly handle digest data compilation for successful renewal (3.41 KB, patch)
2023-10-26 15:02 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-09-26 19:07:32 UTC
See bugs 19014 and 31427 - users can end up receiving an auto renewal failure immediately after a success.  31427 updates to send this notice at the end of a circ period, but then the user doesn't know the book won't renew until it is nearly due

We should have a way to mark that an issue has reached the end of the allowed renewals and warn the user that the item won't be renewed when it becomes due.
Comment 1 Nick Clemens 2023-09-26 19:15:42 UTC
Created attachment 156239 [details] [review]
Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed
Comment 2 Nick Clemens 2023-09-26 19:15:44 UTC
Created attachment 156240 [details] [review]
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests.

To test:
prove -v t/db_dependent/Circulation.t
Comment 3 Nick Clemens 2023-09-26 19:15:46 UTC
Created attachment 156241 [details] [review]
Bug 34924: Handle final renewal errors

This patch updates Koha::Checkout->attempt_auto_renew to renew when the 'final' errors are passed, and to pass the error value back to the cronjob for processing

The sample notice for AUTO_RENEW and AUTO_RENEW_DGST are both updated to handle the new error

On the next cron the error will be updated to too_many or too_unseen, but a notice will not be sent.

To test:
 0 - Run reset_all to install the updated sample notices or copy the text
 1 - Set system preference UnseenRenewals to 'Allow'
 2 - Setup a circ rule to allow 2 renewals, 2 unseen renewals
 3 - Checkout an item to a patron who has an email defined and auto renewals selected in messaging preferences
 4 - Update the issue to be due/overdue:
    UPDATE issues SET date_due=DATE_SUB(NOW(), INTERVAL 1 DAYS);
 5 - perl misc/cronjobs/automatic_renewals.pl -v -c
 6 - Confirm patron notified and issue renewed
 7 - Set issue due/overdue again
 8 - perl misc/cronjobs/automatic_renewals.pl -v -c
 9 - Confirm patron notified of final unseen renewal
10 - perl misc/cronjobs/automatic_renewals.pl -v -c
11 - Confirm issue not renewed, patron not notified
12 - Update circ rules t all 4 renewals total
13 - Force renewal on staff side to clear unseen
14 - perl misc/cronjobs/automatic_renewals.pl -v -c
15 - Confirm patron notified of final renewal (allowed 4, 2 unseen from cron, 1 manual, this unseen from cron)
16 - perl misc/cronjobs/automatic_renewals.pl -v -c
17 - Confirm patron not notified, issue not renewed
Comment 4 Emily Lamancusa 2023-10-03 13:26:37 UTC
The script output looks right in terms of which items should renew, but the renewal itself doesn't seem to be happening. After running the cron with the --confirm switch, the item's due date doesn't change, and the patron's checkouts table line for that item still says "Scheduled for automatic renewal (2 of 2 renewals remaining and 2 of 2 unseen renewals remaining)". The patron does have a queued automatic renewal notice saying that the item renewed and is due on the original due date.

I tried again with only the first patch applied and got the same result, so I'm guessing the problem is there somewhere.
Comment 5 Nick Clemens 2023-10-17 20:50:59 UTC
Created attachment 157290 [details] [review]
Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed
Comment 6 Nick Clemens 2023-10-17 20:51:01 UTC
Created attachment 157291 [details] [review]
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests.

To test:
prove -v t/db_dependent/Circulation.t
Comment 7 Nick Clemens 2023-10-17 20:51:03 UTC
Created attachment 157292 [details] [review]
Bug 34924: Handle final renewal errors

This patch updates Koha::Checkout->attempt_auto_renew to renew when the 'final' errors are passed, and to pass the error value back to the cronjob for processing

The sample notice for AUTO_RENEW and AUTO_RENEW_DGST are both updated to handle the new error

On the next cron the error will be updated to too_many or too_unseen, but a notice will not be sent.

To test:
 0 - Run reset_all to install the updated sample notices or copy the text
 1 - Set system preference UnseenRenewals to 'Allow'
 2 - Setup a circ rule to allow 2 renewals, 2 unseen renewals
 3 - Checkout an item to a patron who has an email defined and auto renewals selected in messaging preferences
 4 - Update the issue to be due/overdue:
    UPDATE issues SET date_due=DATE_SUB(NOW(), INTERVAL 1 DAYS);
 5 - perl misc/cronjobs/automatic_renewals.pl -v -c
 6 - Confirm patron notified and issue renewed
 7 - Set issue due/overdue again
 8 - perl misc/cronjobs/automatic_renewals.pl -v -c
 9 - Confirm patron notified of final unseen renewal
10 - perl misc/cronjobs/automatic_renewals.pl -v -c
11 - Confirm issue not renewed, patron not notified
12 - Update circ rules t all 4 renewals total
13 - Force renewal on staff side to clear unseen
14 - perl misc/cronjobs/automatic_renewals.pl -v -c
15 - Confirm patron notified of final renewal (allowed 4, 2 unseen from cron, 1 manual, this unseen from cron)
16 - perl misc/cronjobs/automatic_renewals.pl -v -c
17 - Confirm patron not notified, issue not renewed
Comment 8 Nick Clemens 2023-10-17 20:52:03 UTC
(In reply to Emily Lamancusa from comment #4)
> The script output looks right in terms of which items should renew, but the
> renewal itself doesn't seem to be happening. After running the cron with the
> --confirm switch, the item's due date doesn't change, and the patron's
> checkouts table line for that item still says "Scheduled for automatic
> renewal (2 of 2 renewals remaining and 2 of 2 unseen renewals remaining)".
> The patron does have a queued automatic renewal notice saying that the item
> renewed and is due on the original due date.
> 
> I tried again with only the first patch applied and got the same result, so
> I'm guessing the problem is there somewhere.

Ah, I neglected to pass through the confirm to attempt_auto_renewal - this should be working better now
Comment 9 Emily Lamancusa 2023-10-20 19:28:02 UTC
Thanks for the follow-up - that fixed it!

It all works great now, except that the default text of the final renewal notice is contradictory:

"The following item, Pro JavaScript techniques , has not been renewed because:

The following item, Pro JavaScript techniques , has correctly been renewed and is now due on 10/23/2023
This item has reached the maximum number of unseen renewals and will need to be renewed at the library."


It happens because the final renewal message is passed as an error even though it's not really an error...which isn't ideal. It's easy to fix in AUTO_RENEWALS, and a bit less so in AUTO_RENEWALS_DGST, but it does make me wonder if we'd be setting ourselves up for issues with maintainability down the road. Is there a better way to pass that information?


Testing notes for 3rd patch (handle final renewal errors):

Step 1 - Also need to set the system preference AutoRenwalNotices to "according to patron messaging preferences"

Step 2 - The circ rule should set renewals to 4, unseen renewals to 2, and Automatic Renewal to "yes" (setting both to 2 won't generate the "final unseen" notice because the checkout will reach its final unseen renewal and its final overall auto-renewal at the same time, and the overall limit takes precedence in the notice)

Step 12 - already done above

Step 14 - need to update the issue to be due/overdue again before running the script
Comment 10 Martin Renvoize 2023-10-23 09:27:35 UTC
This looks exciting, but alas, I wasn't quick enough and it no longer applies.
Comment 11 Nick Clemens 2023-10-23 17:59:49 UTC
Created attachment 157687 [details] [review]
Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed
Comment 12 Nick Clemens 2023-10-23 17:59:52 UTC
Created attachment 157688 [details] [review]
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests.

To test:
prove -v t/db_dependent/Circulation.t
Comment 13 Nick Clemens 2023-10-23 17:59:54 UTC
Created attachment 157689 [details] [review]
Bug 34924: Handle final renewal errors

This patch updates Koha::Checkout->attempt_auto_renew to renew when the 'final' errors are passed, and to pass the error value back to the cronjob for processing

The sample notice for AUTO_RENEW and AUTO_RENEW_DGST are both updated to handle the new error

On the next cron the error will be updated to too_many or too_unseen, but a notice will not be sent.

To test:
 0 - Run reset_all to install the updated sample notices or copy the text
 1 - Set system preference UnseenRenewals to 'Allow'
 2 - Setup a circ rule to allow 2 renewals, 2 unseen renewals
 3 - Checkout an item to a patron who has an email defined and auto renewals selected in messaging preferences
 4 - Update the issue to be due/overdue:
    UPDATE issues SET date_due=DATE_SUB(NOW(), INTERVAL 1 DAYS);
 5 - perl misc/cronjobs/automatic_renewals.pl -v -c
 6 - Confirm patron notified and issue renewed
 7 - Set issue due/overdue again
 8 - perl misc/cronjobs/automatic_renewals.pl -v -c
 9 - Confirm patron notified of final unseen renewal
10 - perl misc/cronjobs/automatic_renewals.pl -v -c
11 - Confirm issue not renewed, patron not notified
12 - Update circ rules t all 4 renewals total
13 - Force renewal on staff side to clear unseen
14 - perl misc/cronjobs/automatic_renewals.pl -v -c
15 - Confirm patron notified of final renewal (allowed 4, 2 unseen from cron, 1 manual, this unseen from cron)
16 - perl misc/cronjobs/automatic_renewals.pl -v -c
17 - Confirm patron not notified, issue not renewed
Comment 14 Nick Clemens 2023-10-23 17:59:56 UTC
Created attachment 157690 [details] [review]
Bug 34924: Adjust auto renewal email to reflect success for final renewals
Comment 15 Nick Clemens 2023-10-23 18:03:22 UTC
(In reply to Emily Lamancusa from comment #9)

> It happens because the final renewal message is passed as an error even
> though it's not really an error...which isn't ideal. It's easy to fix in
> AUTO_RENEWALS, and a bit less so in AUTO_RENEWALS_DGST, but it does make me
> wonder if we'd be setting ourselves up for issues with maintainability down
> the road. Is there a better way to pass that information?
> 

I adjusted the single renewal notice - the digest looks to me like it will come out correctly.

I started trying to look into this, and it got much bigger. We could pass a 'success' flag for the regular notice, then check for the final in the auto_renew_error field. If we just consider it to be 'status' rather than 'error' it feels less bad, adding a new column for tracking final seems overkill to me. If acceptable, I'd like to see this work done on a new bug, rather than changing the structure and adding feature at the same time.
Comment 16 Emily Lamancusa 2023-10-25 21:11:00 UTC
(In reply to Nick Clemens from comment #15)
> I started trying to look into this, and it got much bigger. We could pass a
> 'success' flag for the regular notice, then check for the final in the
> auto_renew_error field. If we just consider it to be 'status' rather than
> 'error' it feels less bad, adding a new column for tracking final seems
> overkill to me. If acceptable, I'd like to see this work done on a new bug,
> rather than changing the structure and adding feature at the same time.

That sounds like a good solution - and I agree with making that change on a separate bug. Thanks for taking a look! Will retest (and hopefully sign off!) in the morning.
Comment 17 Martin Renvoize 2023-10-26 08:49:21 UTC
Also sounds good to me.. Looking through this and having run a few tests I'd be happy to QA this once the SO has come in ;P
Comment 18 Martin Renvoize 2023-10-26 08:52:25 UTC
Created attachment 157895 [details] [review]
Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-10-26 08:52:28 UTC
Created attachment 157896 [details] [review]
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests.

To test:
prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-10-26 08:52:30 UTC
Created attachment 157897 [details] [review]
Bug 34924: Handle final renewal errors

This patch updates Koha::Checkout->attempt_auto_renew to renew when the 'final' errors are passed, and to pass the error value back to the cronjob for processing

The sample notice for AUTO_RENEW and AUTO_RENEW_DGST are both updated to handle the new error

On the next cron the error will be updated to too_many or too_unseen, but a notice will not be sent.

To test:
 0 - Run reset_all to install the updated sample notices or copy the text
 1 - Set system preference UnseenRenewals to 'Allow'
 2 - Setup a circ rule to allow 2 renewals, 2 unseen renewals
 3 - Checkout an item to a patron who has an email defined and auto renewals selected in messaging preferences
 4 - Update the issue to be due/overdue:
    UPDATE issues SET date_due=DATE_SUB(NOW(), INTERVAL 1 DAYS);
 5 - perl misc/cronjobs/automatic_renewals.pl -v -c
 6 - Confirm patron notified and issue renewed
 7 - Set issue due/overdue again
 8 - perl misc/cronjobs/automatic_renewals.pl -v -c
 9 - Confirm patron notified of final unseen renewal
10 - perl misc/cronjobs/automatic_renewals.pl -v -c
11 - Confirm issue not renewed, patron not notified
12 - Update circ rules t all 4 renewals total
13 - Force renewal on staff side to clear unseen
14 - perl misc/cronjobs/automatic_renewals.pl -v -c
15 - Confirm patron notified of final renewal (allowed 4, 2 unseen from cron, 1 manual, this unseen from cron)
16 - perl misc/cronjobs/automatic_renewals.pl -v -c
17 - Confirm patron not notified, issue not renewed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2023-10-26 08:52:33 UTC
Created attachment 157898 [details] [review]
Bug 34924: Adjust auto renewal email to reflect success for final renewals

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-10-26 08:53:15 UTC
Of course.. Emily is on the QA team.. so we can swap.. added my signoff and passing to Emily for the final QA :)
Comment 23 Emily Lamancusa 2023-10-26 15:02:22 UTC
Created attachment 157929 [details] [review]
Bug 34924: Add Koha::Checkout->attempt_auto_renew

This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script.

To test:
1 - prove -v t/db_dependent/Koha/Checkouts.t
2 - Add a circ rule with auto_renew checked
3 - Checkout an item to a patron and set due date in the past
4 - Checkout an item to a patron and set due date in the future
5 - perl misc/cronjobs/automatic_renewals.pl -v
6 - Confirm one would be renewed and the other is too_soon
7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm
8 - Confirm the expected issue is successfully renewed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 24 Emily Lamancusa 2023-10-26 15:02:25 UTC
Created attachment 157930 [details] [review]
Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests.

To test:
prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 25 Emily Lamancusa 2023-10-26 15:02:27 UTC
Created attachment 157931 [details] [review]
Bug 34924: Handle final renewal errors

This patch updates Koha::Checkout->attempt_auto_renew to renew when the 'final' errors are passed, and to pass the error value back to the cronjob for processing

The sample notice for AUTO_RENEW and AUTO_RENEW_DGST are both updated to handle the new error

On the next cron the error will be updated to too_many or too_unseen, but a notice will not be sent.

To test:
 0 - Run reset_all to install the updated sample notices or copy the text
 1 - Set system preference UnseenRenewals to 'Allow'
 2 - Setup a circ rule to allow 2 renewals, 2 unseen renewals
 3 - Checkout an item to a patron who has an email defined and auto renewals selected in messaging preferences
 4 - Update the issue to be due/overdue:
    UPDATE issues SET date_due=DATE_SUB(NOW(), INTERVAL 1 DAYS);
 5 - perl misc/cronjobs/automatic_renewals.pl -v -c
 6 - Confirm patron notified and issue renewed
 7 - Set issue due/overdue again
 8 - perl misc/cronjobs/automatic_renewals.pl -v -c
 9 - Confirm patron notified of final unseen renewal
10 - perl misc/cronjobs/automatic_renewals.pl -v -c
11 - Confirm issue not renewed, patron not notified
12 - Update circ rules t all 4 renewals total
13 - Force renewal on staff side to clear unseen
14 - perl misc/cronjobs/automatic_renewals.pl -v -c
15 - Confirm patron notified of final renewal (allowed 4, 2 unseen from cron, 1 manual, this unseen from cron)
16 - perl misc/cronjobs/automatic_renewals.pl -v -c
17 - Confirm patron not notified, issue not renewed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 26 Emily Lamancusa 2023-10-26 15:02:29 UTC
Created attachment 157932 [details] [review]
Bug 34924: Adjust auto renewal email to reflect success for final renewals

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 27 Emily Lamancusa 2023-10-26 15:02:31 UTC
Created attachment 157933 [details] [review]
Bug 34924: (QA follow-up) correctly handle digest data compilation for successful renewal

automatic_renewals.pl does some data compilation for digest notices that
depends on the error value returned from attempt_auto_renew. However, on
successful renewal $error is undefined. Add additional checks on value
of $success and definedness of $error to compile data accurately and
avoid warns for undefined variable

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 28 Emily Lamancusa 2023-10-26 15:12:28 UTC
Looks great!

Added a follow-up to fix some checks when compiling result data for the digest notices - CanBookBeRenewed was returning an $error value of 'auto-renew' for successful autorenewals, while attempt_auto_renew returns an undefined $error value in that situation (which makes sense - the subroutine already assumes we're in an auto-renewal context after all). This was actually causing some incorrect data to be passed to the digest notice, as well as a bunch of warns for trying to access $error when it was undefined. Won't cause issues anywhere else, though, since attempt_auto_renew isn't used outside the cron.

Other than that, QA tests pass, no regressions found, and everything looks good. Thanks for the nice refactor and excellent added clarity to auto-renewal notices!
Comment 29 Tomás Cohen Arazi 2023-10-31 14:06:42 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Fridolin Somers 2023-11-08 08:24:47 UTC
Enhancement not pushed to 23.05.x
Comment 31 Lucas Gass 2024-01-24 00:05:47 UTC
> I started trying to look into this, and it got much bigger. We could pass a
> 'success' flag for the regular notice, then check for the final in the
> auto_renew_error field. If we just consider it to be 'status' rather than
> 'error' it feels less bad, adding a new column for tracking final seems
> overkill to me. If acceptable, I'd like to see this work done on a new bug,
> rather than changing the structure and adding feature at the same time.

Has this new bug been filed yet?