|
Description
Nick Clemens (kidclamp)
2023-09-26 19:07:32 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 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 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 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. 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 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 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 (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 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 This looks exciting, but alas, I wasn't quick enough and it no longer applies. 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 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 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 Created attachment 157690 [details] [review] Bug 34924: Adjust auto renewal email to reflect success for final renewals (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. (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. 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 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> 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> 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> 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> Of course.. Emily is on the QA team.. so we can swap.. added my signoff and passing to Emily for the final QA :) 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> 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> 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> 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> 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> 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! Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x
> 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?
We would be very disappointed if changes to the existing functionality added here (as suggested in comments 9 to 16) led to a reduction in functionality. We are using the AUTO_RENEWALS_DGST as in effect the first notification of an item being overdue. We have removed the ‘success’ TT code from the notice so no emails are sent if all items renew without an error state. No pointless emails are sent, which is proving very popular here. The ‘auto_renewal_final’ situation is something the reader needs to be told about (of course), so having the ‘auto_renewal_final’ as an error is key to our 23.11 auto-renewals functionality. In dozens of tests to configure auto-renewals we have never been able to replicate point 11 in the test plan for this bug. An email is prepared for the ‘auto_renewal_final’ and one is also sent for the ‘too_many’ error, which is good as it supports our four overdue notifications workflow. (Our first ‘official’ ODUE notice is sent 7 days after the item fails to auto-renew). If we wanted to achieve test point 11 and have no notifications sent for the ‘too_many’ error I think we could simply remove that error from the TT code in the notice. But why would we do that, given the delights of no ‘success’ renewal email nags and 4 overdue notification functionality? Created attachment 168642 [details]
What we were trying to achieve
I need to update my comments. Extended testing has shown that my removing the ‘success content’ in the digest triggers a nasty side effect that we cannot tolerate. And further tests show that the renewal reminder being triggered by the too_many error is also caused by my removing the ‘success content’. Now that we have reverted to using the default digest, when we upgrade to 23.11 tomorrow, the digest will work in accordance with this bug. The attachment outlines what we were trying to achieve, and I plan to raise a separate bug to request the optional prevention of error-less renewal notifications, and further to optionally have a digest prepared as well for the too_many error. This doesn't seem to change anything on the staff side (or OPAC), so nothing to change in the manual. Please reopen with details if this is not the case. |