Bug 34498 - Default autorenewal notices do not account for item_denied_renewal
Summary: Default autorenewal notices do not account for item_denied_renewal
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-08 16:36 UTC by Lucas Gass (lukeg)
Modified: 2025-04-04 13:54 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 34498: Make autorenewal notices consider ItemsDeniedRenewal (2.16 KB, patch)
2025-03-18 20:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 34498: Make autorenewal notices consider ItemsDeniedRenewal (2.20 KB, patch)
2025-04-02 11:53 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2023-08-08 16:36:10 UTC
To recreate:

1. Have the default notices set for AUTO_RENEWALS and AUTO_RENEWALS_DGST.
2. Set the ItemsDeniedRenewal system preference. ( itemlost: [1,2,3,4,5] )
3. Set the AutoRenewalNotices system preference to 'According to messaging preferences'.
4. Don't mark lost items as returned (  MarkLostItemsAsReturned ).
5. Set up some auto renewals and then mark the items as lost. 
6. Run automatic_renewals.pl
7. You get a notice telling you the item cannot be renewed but no reason is specified.
Comment 1 Juliet Heltibridle 2024-12-06 19:01:19 UTC
This is causing our patrons to receive an auto renewal digest notice the day after their items are marked as long overdue, stating that the item can't be renewed. We don't want to send this notice, as it is could be confusing to patrons who just received a bill for those lost items.
Comment 2 Aleisha Amohia 2025-03-18 20:40:22 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2025-04-02 11:53:16 UTC
Created attachment 180304 [details] [review]
Bug 34498: Make autorenewal notices consider ItemsDeniedRenewal

This fix ensures that autorenewals (and corresponding notices) are not generated for items that are configured to be denied renewal in the ItemsDeniedRenewal system preference.

To test:

1. Have the default notices set for AUTO_RENEWALS and AUTO_RENEWALS_DGST.
2. Set the ItemsDeniedRenewal system preference. ( itemlost: [1,2,3,4,5] )
3. Set the AutoRenewalNotices system preference to 'According to messaging preferences'.
4. Don't mark lost items as returned (  MarkLostItemsAsReturned ).
5. Set up some auto renewals and then mark the items as lost.
6. Run automatic_renewals.pl
7. You get a notice telling you the item cannot be renewed but no reason is specified.

8. Apply the patch and restart services

9. Run through the test plan again and confirm no notices are generated.

Sponsored-by: New Zealand Parliamentary Library
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 4 Emily Lamancusa (emlam) 2025-04-03 13:20:56 UTC
I don't think skipping the item completely in the auto-renewal cron job is the way to go here. If the item is denied renewal based on a permanent attribute, like item type, then it's either a bug or a configuration issue if that item is getting flagged for auto-renewal in the first place. On the other hand, if it's denied renewal based on a temporary status such as Lost status, that means it was eligible for auto-renewal at some point, and libraries might want to notify patrons rather than have it silently fail to renew. We should adjust the default notice template as the bug title suggests to better handle items_denied_renewal, and leave libraries the choice to make their own revisions based on whether they want to notify of that failure reason or not.
Comment 5 Aleisha Amohia 2025-04-03 21:08:43 UTC
(In reply to Emily Lamancusa (emlam) from comment #4)
> I don't think skipping the item completely in the auto-renewal cron job is
> the way to go here. If the item is denied renewal based on a permanent
> attribute, like item type, then it's either a bug or a configuration issue
> if that item is getting flagged for auto-renewal in the first place. On the
> other hand, if it's denied renewal based on a temporary status such as Lost
> status, that means it was eligible for auto-renewal at some point, and
> libraries might want to notify patrons rather than have it silently fail to
> renew. We should adjust the default notice template as the bug title
> suggests to better handle items_denied_renewal, and leave libraries the
> choice to make their own revisions based on whether they want to notify of
> that failure reason or not.

Hey Emily, thanks for your thoughts here. The particular use case for our patch was that the library is using the ItemsDeniedRenewal system preference to exclude items from renewal based on collectioncode. So it's more permanent than a lost status, and I'm sure they had a reason for not using an item type which would've allowed them to use the circulation rules. It means that patrons are getting empty autorenewal notices if they have items from this collection checked out, which aren't eligible for auto-renew. Does that explain the purpose of this better?
Comment 6 Emily Lamancusa (emlam) 2025-04-04 13:54:39 UTC
Thanks for the explanation, Aleisha!

I think there are two pieces here:
One is that the checkout code should be checking ItemsDeniedRenewal, and not setting issues.auto-renew in the first place if its renewal is denied by the syspref. Then the cron wouldn't pick up those items, and we wouldn't need a separate loop to exclude them.

For the notices issue, we have something similar - it's not specific to ItemsDeniedRenewal, but in our system we generally don't want an auto-renewal notice going out at all if nothing actually renewed. To prevent that, we wrapped our notice template in:
[%- UNLESS success == 0 %]
<notice template contents>
[%- END %]

Would something like that work for this library's use case?