Bug 30755 - auto_too_soon should not be counted as an error in autorenewals
Summary: auto_too_soon should not be counted as an error in autorenewals
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords: release-notes-needed
Depends on: 30222
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-13 15:57 UTC by Martin Renvoize
Modified: 2022-10-31 22:14 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch alters the way errors/successes are counted in auto renewals. Prior to this patch, using [% error %] in a template would provide a count of all items not renewed, even if 'too_soon' After this patch [% error + results.auto_too_soon %] will provide the same count, or you can get a count of each error in the results variable e.g. Some items were not renewed: [% FOREACH key in results.keys %] [% results.$key %] item(s) were not renewed for reason [% key %] [% END %]
Version(s) released in:
22.11.00


Attachments
Bug 30755: Remove auto_too_soon from error count (2.05 KB, patch)
2022-05-20 07:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30755: Remove auto_too_soon from error count (2.10 KB, patch)
2022-09-02 07:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30755: (follow-up) Add a results hash to automatic renewals (3.14 KB, patch)
2022-09-02 07:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30755: Remove auto_too_soon from error count (2.17 KB, patch)
2022-09-02 10:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30755: (follow-up) Add a results hash to automatic renewals (3.20 KB, patch)
2022-09-02 10:22 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-05-13 15:57:23 UTC
When working through the notice for autorenewals digest I found that the too_soon error gets counted, and displayed, in the errors section of the notice.

I don't believe it should be, as it's just a function of the system rather than a hard un-recoverable error that the user needs to know about.
Comment 1 Martin Renvoize 2022-05-13 16:04:07 UTC
I noticed this whilst working on an improved notice that only display renewals (or failures) that actually took place on that run.

Notice included below for reference (the errors count comes out incorrect).. it's helpful to have bug 30751 to test this.

----
[%- USE String -%]
[%- now = String.new(today).truncate(10) -%]
<p>Dear [% borrower.firstname %] [% borrower.surname %],</p>

    [%- IF success -%]<p>You have [% success %] renewed item(s).</p>[%- END %]
    <ul>
    [%- FOREACH checkout IN checkouts -%]
        [%- renewaldate = String.new(checkout.lastreneweddate).truncate(10) -%]
        [%- IF !checkout.auto_renew_error && ( renewaldate == now ) %]
        <li><b>[% checkout.item.biblio.title %] : [% checkout.item.barcode %]</b> was renewed until [% checkout.date_due | $KohaDates as_due_date => 1 %]</li>
        [%- END -%]
    [%- END -%]
    </ul>
        
    [%- IF error -%]<p>Item(s) that failed to renew today:</p>[%- END %]
    <ul>
    [%- FOREACH checkout IN checkouts -%]
        [%- IF checkout.auto_renew_error && checkout.auto_renew_error != 'auto_too_soon' %]
        <li><b>[% checkout.item.biblio.title %] : [% checkout.item.barcode -%]</b>
            [%- IF checkout.auto_renew_error == 'too_many' -%]<span style="color:red;">has reached the maximum number of renewals possible</span></li>
            [%- ELSIF checkout.auto_renew_error == 'on_reserve' -%]<span style="color:red;"> is on hold for another reader.</span></li>
            [%- ELSIF checkout.auto_renew_error == 'restriction' -%]<span style="color:red;"> won't renew because your account is currently restricted.</span></li>
            [%- ELSIF checkout.auto_renew_error == 'overdue' -%]<span style="color:red;"> won't renew because you have overdue items.</span></li>
            [%- ELSIF checkout.auto_renew_error == 'auto_too_late' -%]<span style="color:red;"> won't renew because it's too late to renew this item.</span></li>
            [%- ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' -%]<span style="color:red;"> won't renew because you have outstanding charges on your account.</span></li>
            [%- ELSIF checkout.auto_renew_error == 'too_unseen' -%]<span style="color:red;"> won't renew because this item must be renewed at the library.</span></li>
            [%- END -%]
        [%- END -%]
    [%- END -%]        

<p>Thank you</p>

[% patron.homebranch.branchname %]
[% patron.homebranch.branchaddress1 %]
[% patron.homebranch.branchaddress2 %]
[% patron.homebranch.branchcity %]
[% patron.homebranch.branchzip %]

[% checkout.library.branchurl %]
Comment 2 Lucas Gass 2022-05-17 21:23:35 UTC
+1, most patrons do not want to know about the 'too_soon' error
Comment 3 David Cook 2022-05-17 23:52:01 UTC
I thought I had this problem until I applied Bug 29381.

Looking at the notice below, I don't see why it would be showing in the errors section?

Or is there a difference between "too_soon" and "auto_too_soon"?

(I've found the autorenewals in general to be such a source of headaches, although I like the feature concept overall.)
Comment 4 Jonathan Druart 2022-05-18 08:09:57 UTC
I don't think I recreate the problem. Can you provide a step-by-step plan please?
Comment 5 Nick Clemens 2022-05-19 17:32:03 UTC
The digests will receive an issue marked as 'auto_too_soon' - there should not be a case of an issue marked for auto-renewal returning 'too_soon' without auto

The default notice should be updated to handle these - if you are getting a digest I think you do want to see to:
Book A renewed
Book B not renewed (too soon)


Otherwise we should nex ton those in the loop

Beyond that - if all errors are too_soon we shouldn't trigger a notice, the updated flag should not be sent and the digest should be skipped. Can you confirm if you are seeing auto_too_soon on other notices, or notices with all auto_too_soon?
Comment 6 David Cook 2022-05-19 23:50:10 UTC
(In reply to Nick Clemens from comment #5)
> The digests will receive an issue marked as 'auto_too_soon' - there should
> not be a case of an issue marked for auto-renewal returning 'too_soon'
> without auto
> 
> The default notice should be updated to handle these - if you are getting a
> digest I think you do want to see to:
> Book A renewed
> Book B not renewed (too soon)
> 
> 
> Otherwise we should nex ton those in the loop
> 
> Beyond that - if all errors are too_soon we shouldn't trigger a notice, the
> updated flag should not be sent and the digest should be skipped. Can you
> confirm if you are seeing auto_too_soon on other notices, or notices with
> all auto_too_soon?

That's what I'm confused about. Doesn't the following mean that auto_too_soon errors aren't even being counted?

https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=29381&attachment=127947
Comment 7 Martin Renvoize 2022-05-20 07:09:35 UTC
Created attachment 135224 [details] [review]
Bug 30755: Remove auto_too_soon from error count

This patch prevents auto_too_soon errors being counted towards auto
renew errors.
Comment 8 Martin Renvoize 2022-05-20 07:13:05 UTC
Code speaks... though I'm not 100% about it vs the $updated flag.

Our libraries only want the notices to include actual auto-renewals or auto-renewal-failures and their patrons don't count 'auto_too_soon' as a failure at all.. it's just part of the function.

My notice template above does this, but it currently relies on the 'error' count that's passed by the script which currently includes auto_too_soon rather than filtering them out.

If you believe patrons do want to see these 'auto_too_soon' failures as actual failures they have to act on then the error count is already correct.. perhaps we shouldn't send an error count at all but rather count it in the notice template itself only?
Comment 9 Martin Renvoize 2022-05-20 07:34:41 UTC
In fact.. I do wonder about how much we pass in our calls to GetPreparedLetter sometimes.

In this case, I wonder if we aught to just send 'patron' and have sane accessors in the Patron object that would allow for 'Patron->checkouts->auto_renewing' or/and 'Patron->checkouts->auto_renew_failures', 'Patron->checkouts->auto_renew_sucesses'.. that would allow splitting out the types as per my notice, but in a somewhat simpler fashion without having to loop through the same set multiple times.

Thoughts?
Comment 10 Martin Renvoize 2022-05-20 07:36:20 UTC
That very much brings us back around to bug 30278.

I'll take a look at working on that :)
Comment 11 David Cook 2022-05-23 00:30:42 UTC
(In reply to Martin Renvoize from comment #7)
> Created attachment 135224 [details] [review] [review]
> Bug 30755: Remove auto_too_soon from error count
> 
> This patch prevents auto_too_soon errors being counted towards auto
> renew errors.

Oh man... I understand now... 

Bug 29381 fixed this problem, but then Bug 30222 broke it again. So now here you are trying to re-fix it again...

Yeah as you say I think that really brings us back to Bug 30278.
Comment 12 David Cook 2022-05-23 00:32:18 UTC
Changing this from enhancement to a bug, because I'd say it's definitely a bug.
Comment 13 Martin Renvoize 2022-05-23 05:44:15 UTC
Nearly, I think the updated flag works as expected and thus reduces when the notice is sent.. the issue I had was the error count being out, not matching what our patrons actually consider an error.

Putting it mathematically.. my notic filters put auto_too_soon (and splits errors and successes into two groups)

So, before my notice change we have a jumbled mess of checkouts with success + errors = all checkouts that might autorenew.

With my notice because I filter out the auto_too_soon items we have success (in a group) + actual errors (errors - auto_too_soon). The "errors" count doesn't match in this case.

But yes... This all results in the same final conclusion.. we need a clear set of object methods instead...
Comment 14 Martin Renvoize 2022-05-23 05:44:38 UTC
Nearly, I think the updated flag works as expected and thus reduces when the notice is sent.. the issue I had was the error count being out, not matching what our patrons actually consider an error.

Putting it mathematically.. my notic filters put auto_too_soon (and splits errors and successes into two groups)

So, before my notice change we have a jumbled mess of checkouts with success + errors = all checkouts that might autorenew.

With my notice because I filter out the auto_too_soon items we have success (in a group) + actual errors (errors - auto_too_soon). The "errors" count doesn't match in this case.

But yes... This all results in the same final conclusion.. we need a clear set of object methods instead...
Comment 15 Nick Clemens 2022-09-02 07:57:36 UTC
Created attachment 140078 [details] [review]
Bug 30755: Remove auto_too_soon from error count

This patch prevents auto_too_soon errors being counted towards auto
renew errors.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2022-09-02 07:57:41 UTC
Created attachment 140079 [details] [review]
Bug 30755: (follow-up) Add a results hash to automatic renewals

It's slightly debatable whether auto_too_soon is an error or not. I think
it leans far enough towards error that the other patch is correct. The change, however,
will affect notices and means we cannot count auto_too_soon anymore

This patch adds a new 'results' hash to the info - this includes a count per error
(including auto_renew i.e. success) allowing for notices to decide how to display the info.

To test:
1 - Add to Auto renew digest notice:
[% IF results %]
There were [% results.auto_too_soon %] items that were too soon.
[% END %]
2 - Check out some items to a patron that will be too soon
3 - Check out one that is not too soon and will renew
4 - Ensure patron has auto renew digest selected and that AutoRenewalNotices is set to follow patron preferences
5 - Run the auto renewals cron
6 - Confirm the notice for the patron displays 1 successful renewal, no failures, and a correct count of too_soon

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Martin Renvoize 2022-09-02 10:22:03 UTC
Created attachment 140099 [details] [review]
Bug 30755: Remove auto_too_soon from error count

This patch prevents auto_too_soon errors being counted towards auto
renew errors.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-09-02 10:22:07 UTC
Created attachment 140100 [details] [review]
Bug 30755: (follow-up) Add a results hash to automatic renewals

It's slightly debatable whether auto_too_soon is an error or not. I think
it leans far enough towards error that the other patch is correct. The change, however,
will affect notices and means we cannot count auto_too_soon anymore

This patch adds a new 'results' hash to the info - this includes a count per error
(including auto_renew i.e. success) allowing for notices to decide how to display the info.

To test:
1 - Add to Auto renew digest notice:
[% IF results %]
There were [% results.auto_too_soon %] items that were too soon.
[% END %]
2 - Check out some items to a patron that will be too soon
3 - Check out one that is not too soon and will renew
4 - Ensure patron has auto renew digest selected and that AutoRenewalNotices is set to follow patron preferences
5 - Run the auto renewals cron
6 - Confirm the notice for the patron displays 1 successful renewal, no failures, and a correct count of too_soon

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-09-02 10:23:32 UTC
I think that now covers all use cases.. I can prevent extra notices going out by comparing the count of errors and the count of issues inside the notice now too.

Signing off on Nicks follow-up and we agreed between us it should be a PQA :)
Comment 20 Tomás Cohen Arazi 2022-09-12 18:56:51 UTC
This introduces a behavior change for those relying on the error for displaying purposes. Can you please write a nice release note about it before I push?
Comment 21 Tomás Cohen Arazi 2022-09-19 18:17:49 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 22 Lucas Gass 2022-10-31 22:14:09 UTC
I like this fix but I am unsure if I want to backport it to the stable branch as it does introduce a behavior change.