[Bug 30755] New: too_soon should not be counted as an error in autorenewals
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Bug ID: 30755 Summary: too_soon should not be counted as an error in autorenewals Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Depends on: 30222 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30222 [Bug 30222] Auto_renew_digest still sends every day when renewals are not allowed -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30751 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 %] -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #2 from Lucas Gass <lucas@bywatersolutions.com> --- +1, most patrons do not want to know about the 'too_soon' error -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- 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.) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I don't think I recreate the problem. Can you provide a step-by-step plan please? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 135224 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135224&action=edit Bug 30755: Remove auto_too_soon from error count This patch prevents auto_too_soon errors being counted towards auto renew errors. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- That very much brings us back around to bug 30278. I'll take a look at working on that :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #7)
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.
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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30222, | |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29381 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Changing this from enhancement to a bug, because I'd say it's definitely a bug. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|too_soon should not be |auto_too_soon should not be |counted as an error in |counted as an error in |autorenewals |autorenewals -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135224|0 |1 is obsolete| | --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 140078 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140078&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 140079 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140079&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #140078|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 140099 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140099&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #140079|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 140100 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140100&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #19 from 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Keywords| |release-notes-needed --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This patch alters the way release notes| |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 | |%] -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Janet McGowan <janet.mcgowan@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |janet.mcgowan@ptfs-europe.c | |om -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 --- Comment #22 from Lucas Gass <lucas@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30755 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org