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.
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.
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.
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') %]
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>
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." ?
(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';
(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!
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>
I am wondering about this one. Won't this create an empty notice if all items are "auto_too_soon"?
(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.
Thx for the reply :)
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.
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.02
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!
Pushed to 23.11.x for 23.11.07