[Bug 29476] New: Earliest renewal date is displayed wrong in circ/renew.pl for issues with auto renewing
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Bug ID: 29476 Summary: Earliest renewal date is displayed wrong in circ/renew.pl for issues with auto renewing Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: joonas.kylmala@iki.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com The code in circ/renew.pl uses the code C4::Circulation::GetSoonestRenewDate to check the soonest renewal date and then displays it to the librarian. However, the code called by CanBookBeRenewed() that actually does the final check uses different calculation method for issues with auto renewal enabled. For auto renewed issues without circulation rules defined for soonest renewal date the soonest renewal date is the due date, in normal loans case the soonest renewal date is NOW(). This is not probably a too big annoyance to librarians, I just wanna clean up this so I can refactor some code in CanBookBeRenewed, will attach a patch shortly. -- 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=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |minor Assignee|koha-bugs@lists.koha-commun |joonas.kylmala@iki.fi |ity.org | -- 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=29476 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Joonas, for "normal" renewals the "not renewal x days before" is also used. Actually it was developed fro normal renewals and around before the auto renewals were added. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> 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=29476 --- Comment #2 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127618 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127618&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for display the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 --- Comment #3 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- (In reply to Katrin Fischer from comment #1)
Hi Joonas, for "normal" renewals the "not renewal x days before" is also used. Actually it was developed fro normal renewals and around before the auto renewals were added.
Yup, that's right. I think maybe you misunderstood my comment wrong and added this comment, please see the patch and maybe the bug is more clear, also a sign-off would be nice :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127618|0 |1 is obsolete| | --- Comment #4 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127619 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127619&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for display the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127619|0 |1 is obsolete| | --- Comment #5 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127620 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127620&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for displaying the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 --- Comment #6 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- I noticed a couple typos in the comments so re-attached the patch, ready for sign-off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127620|0 |1 is obsolete| | --- Comment #7 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- Created attachment 127900 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127900&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for displaying the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t Current status: Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29474 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 [Bug 29474] Automatic renewals cronjob is slow on systems with large numbers of reserves -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29537 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29537 [Bug 29537] Simplify auto-renewal code in CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|29474 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29474 [Bug 29474] Automatic renewals cronjob is slow on systems with large numbers of reserves -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 David Nind <david@davidnind.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=29476 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127900|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 128096 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128096&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for displaying the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com Status|Signed Off |Failed QA --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 1. There is a return in an assignment statement: + my $soonestrenewal = return dt_from_string( $itemissue->date_due ); 2. Wondering, should we return $soonestrenewal if $now < $soonestrenewal; ? 3. Shouldn't we finally call GetSoonestRenewDate from _CanBookBeAutoRenewed? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #9)
3. Shouldn't we finally call GetSoonestRenewDate from _CanBookBeAutoRenewed?
This is what you actually did on the patch "Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128096|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128477 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128477&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for displaying the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: David Nind <david@davidnind.com> 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=29476 --- Comment #12 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 128478 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128478&action=edit Bug 29476: (follow-up) Fix return, add precision test 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=29476 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #9)
2. Wondering, should we return $soonestrenewal if $now < $soonestrenewal; ? I don't think so
There are two cases: with undefined no renewal before: 1 - Automatic renewals wait until it is due 2 - Manual renewals are possible at any time Let me know if I missed something -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128477|0 |1 is obsolete| | Attachment #128478|0 |1 is obsolete| | --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128552 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128552&action=edit Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal If a checkout with auto-renewal enabled doesn't have a "norenewalbefore" circulation rule set the code in CanBookBeRenewed() falls back to using due date (to verify this please look for the string "auto_too_soon" in C4/Circulation.pm), the calculation result of GetSoonestRenewDate() however didn't do this, though luckily it was not used in CanBookBeRenewed so we didn't get any issues there. However, GetSoonestRenewDate() is used for displaying the soonest renewal date in the staff interface on the circ/renew.pl page so you would have gotten wrong results there. This patch moves additionally the tests made for Bug 14395 under a new subtest for GetSoonestRenewDate() as they should have been like that already before. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 128553 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128553&action=edit Bug 29476: (follow-up) Fix return, add precision test Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #16 from Fridolin Somers <fridolin.somers@biblibre.com> --- Hi, Maybe a silly question : - return $now unless $issuing_rule; I don't understand why do we remove this ? issuing rule may be undef right ? In this case the calls $issuing_rule->{xxx} will fail. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Fridolin Somers from comment #16)
Hi,
Maybe a silly question :
- return $now unless $issuing_rule; I don't understand why do we remove this ? issuing rule may be undef right ? In this case the calls $issuing_rule->{xxx} will fail.
The first check is 'defined' so it will be okay. We remove the return because there are two cases, see comment 13 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.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=29476 --- Comment #18 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00 |22.05.00,21.11.02 released in| | Status|Pushed to master |Pushed to stable CC| |kyle@bywatersolutions.com --- Comment #19 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.11.x for 21.11.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|22.05.00,21.11.02 |22.05.00,21.11.02,21.05.09 released in| | Status|Pushed to stable |Pushed to oldstable CC| |andrew@bywatersolutions.com --- Comment #20 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.09 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29476 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED CC| |victor@tuxayo.net Resolution|--- |FIXED --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org