Bug 30755

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 %]

+1, most patrons do not want to know about the 'too_soon' error

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.)

I don't think I recreate the problem. Can you provide a step-by-step plan please?

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?

(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

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.

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?

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?

That very much brings us back around to bug 30278.

I'll take a look at working on that :)

(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.

Changing this from enhancement to a bug, because I'd say it's definitely a bug.

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...

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...

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>

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>

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>

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>

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 :)

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?

Pushed to master for 22.11.

Nice work everyone, thanks!

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.