Bug 36741 - AUTO_RENEWALS_DGST should skip auto_too_soon
Summary: AUTO_RENEWALS_DGST should skip auto_too_soon
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Lucas Gass
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-01 04:53 UTC by David Cook
Modified: 2024-07-22 10:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the default AUTO_RENEWALS_DGST notice so that items where it is too soon to renew aren't included in the notice output to patrons when the automatic renewals cron job is run (based on the circulation rules settings). These items were previously included in the notice. NOTE: This notice is only updated for new installations. Existing installations should update this notice if they only want to show the actual items automatically renewed.
Version(s) released in:
24.11.00,24.05.02,23.11.07
Circulation function:


Attachments
Bug 36741: Skip auto_too_soon issues in auto renew digest (1.51 KB, patch)
2024-05-23 00:58 UTC, David Cook
Details | Diff | Splinter Review
Bug 36741: Skip auto_too_soon issues in auto renew digest (1.56 KB, patch)
2024-05-23 20:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 36741: Skip auto_too_soon issues in auto renew digest (1.61 KB, patch)
2024-06-26 13:04 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2024-05-01 04:53:02 UTC
I think it's debateable that auto_too_soon items should show up in the AUTO_RENEWALS_DGST at all, but I can think of reasons why they would be useful to show (you might want to show which items weren't renewed because it was too soon).

But at the moment, the AUTO_RENEWALS_DGST doesn't account for them. It just prints out the title and barcode, which looks ridiculous.

We need a default. Either to skip auto_too_soon in that loop, or to say that it's too soon for it to be renewed. I'm in favour of the former.
Comment 1 David Cook 2024-05-01 07:22:10 UTC
It seems that all we need is the following:

[% FOREACH checkout IN checkouts %]
[% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %]

That's the default that makes sense to me. People could do something else if they wanted, but we *must* capture this case and do something with it. I'm getting (totally valid) complaints about this at the moment.
Comment 2 David Cook 2024-05-23 00:39:06 UTC
I don't think there's an easy way to update existing AUTO_RENEWALS_DGST notices, but we should update the mandatory sample notices at least.
Comment 3 David Cook 2024-05-23 00:58:50 UTC
Created attachment 167054 [details] [review]
Bug 36741: Skip auto_too_soon issues in auto renew digest

This change adds a line to skip auto_too_soon issues/checkouts
in the auto renew digest template.

Since auto_too_soon do not trigger notifications and don't require
any special action, let's skip them in the breakdown of checkouts in
the AUTO_RENEWALS_DGST email.

Test plan:
0. Apply the patch
1. reset_all (in koha-testing-docker)
2. Note the following line in the AUTO_RENEWALS_DGST template:
[% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %]
Comment 4 David Nind 2024-05-23 20:55:30 UTC
Created attachment 167137 [details] [review]
Bug 36741: Skip auto_too_soon issues in auto renew digest

This change adds a line to skip auto_too_soon issues/checkouts
in the auto renew digest template.

Since auto_too_soon do not trigger notifications and don't require
any special action, let's skip them in the breakdown of checkouts in
the AUTO_RENEWALS_DGST email.

Test plan:
0. Apply the patch
1. reset_all (in koha-testing-docker)
2. Note the following line in the AUTO_RENEWALS_DGST template:
[% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %]

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Lucas Gass 2024-06-25 13:35:54 UTC
If you use [% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %] will it not lead to an erroneous count here:

"There were [% error %] items that were not renewed." 

?
Comment 6 David Cook 2024-06-25 23:38:23 UTC
(In reply to Lucas Gass from comment #5)
> If you use [% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %] will
> it not lead to an erroneous count here:
> 
> "There were [% error %] items that were not renewed." 
> 
> ?

Nope. auto_too_soon isn't added to the error count, so it would actually be more accurate. 

$renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{error}++ unless $success || $error eq 'auto_too_soon';

$renew_digest->{ $auto_renew->borrowernumber }->{error}++ unless $success || $error eq 'auto_too_soon';
Comment 7 Lucas Gass 2024-06-26 13:02:10 UTC
(In reply to David Cook from comment #6)
> (In reply to Lucas Gass from comment #5)
> > If you use [% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %] will
> > it not lead to an erroneous count here:
> > 
> > "There were [% error %] items that were not renewed." 
> > 
> > ?
> 
> Nope. auto_too_soon isn't added to the error count, so it would actually be
> more accurate. 
> 
> $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber
> }->{error}++ unless $success || $error eq 'auto_too_soon';
> 
> $renew_digest->{ $auto_renew->borrowernumber }->{error}++ unless $success ||
> $error eq 'auto_too_soon';

Ah, great!
Comment 8 Lucas Gass 2024-06-26 13:04:16 UTC
Created attachment 168140 [details] [review]
Bug 36741: Skip auto_too_soon issues in auto renew digest

This change adds a line to skip auto_too_soon issues/checkouts
in the auto renew digest template.

Since auto_too_soon do not trigger notifications and don't require
any special action, let's skip them in the breakdown of checkouts in
the AUTO_RENEWALS_DGST email.

Test plan:
0. Apply the patch
1. reset_all (in koha-testing-docker)
2. Note the following line in the AUTO_RENEWALS_DGST template:
[% NEXT IF (checkout.auto_renew_error == 'auto_too_soon') %]

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 9 Katrin Fischer 2024-06-27 07:31:21 UTC
I am wondering about this one. Won't this create an empty notice if all items are "auto_too_soon"?
Comment 10 David Cook 2024-06-28 00:13:40 UTC
(In reply to Katrin Fischer from comment #9)
> I am wondering about this one. Won't this create an empty notice if all
> items are "auto_too_soon"?

No, because notices only go out if the "updated" flag in the $renew_digest hashref is set to 1, and if all items are "auto_too_soon", the "updated" flag won't be set to 1:

$renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{updated} = 1 if $updated && (!$error || $error ne 'auto_too_soon');

$renew_digest->{ $auto_renew->borrowernumber }->{updated} = 1 if $updated && (!$error || $error ne 'auto_too_soon');

Resetting to Passed QA.

--

I've had this running in production for about 2 months now, and so far so good.
Comment 11 Katrin Fischer 2024-06-28 11:22:26 UTC
Thx for the reply :)
Comment 12 Katrin Fischer 2024-06-28 11:23:25 UTC
I was wondering if we should not "remove" those items from the loop so we can do without additional code in the sample notices. The notices get a bit harder to read/understand for librarians that way, but I think it gives flexibility for those who might want to include a note about these items.
Comment 13 Katrin Fischer 2024-06-28 11:50:15 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 14 Lucas Gass 2024-07-19 16:00:51 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 15 David Nind 2024-07-20 01:11:18 UTC
I'm not sure if I described this correctly in the release notes - please update to correct my mistakes. I've never really got my head around these settings for the circulation rules!
Comment 16 Fridolin Somers 2024-07-22 10:04:24 UTC
Pushed to 23.11.x for 23.11.07