Bug 25758 - Items scheduled for automatic renewal do not show that they will not renew due to a hold
Summary: Items scheduled for automatic renewal do not show that they will not renew du...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 40 votes (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 19014
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-15 15:08 UTC by Andrew Fuerste-Henry
Modified: 2020-11-24 00:05 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Bug 19014 prioritized the 'too soon' message for renewals to prevent sending too many notifications. When displaying information about the hold elsewhere it is desired to see the 'on hold' status even when the renewal is too soon. This patch add a switch to the CanBookBeRenewed routine to decide which status has priority (i.e. whether we are checking from the renewal cron or elsewhere)
Version(s) released in:
20.11.00, 20.05.06, 19.11.12


Attachments
Bug 25758: Display the different causes of non renewability (41.58 KB, patch)
2020-06-24 09:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25758: Fix handle of checkouts without holds (3.51 KB, patch)
2020-07-27 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
renew hold (64.84 KB, image/png)
2020-07-29 12:59 UTC, Kelly McElligott
Details
opac renew (47.41 KB, image/png)
2020-07-29 13:02 UTC, Kelly McElligott
Details
Bug 25758: Display the different causes of non renewability (42.38 KB, patch)
2020-07-29 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25758: Display the different causes of non renewability (42.44 KB, patch)
2020-08-06 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25758: Display the different causes of non renewability (42.50 KB, patch)
2020-08-06 14:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25758: Fix renewal at the OPAC (4.39 KB, patch)
2020-08-26 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (7.02 KB, patch)
2020-09-04 12:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (7.09 KB, patch)
2020-09-04 15:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (6.81 KB, patch)
2020-09-04 15:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (6.91 KB, patch)
2020-09-04 16:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (7.20 KB, patch)
2020-09-13 08:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (6.96 KB, patch)
2020-09-13 08:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron (6.97 KB, patch)
2020-09-13 08:47 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-06-15 15:08:27 UTC
In the issues table on circulation.pl, we're no longer showing a message of "On Hold" when an item is scheduled for automatic renewal, is outside of the No Renewal Before window, but has a hold that will block renewal.

This is a regression, most likely caused by bug 19014.

To recreate:
1- set NoRenewalBeforePrecision to date only
2- set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
3- check item out
4- confirm item shows Scheduled For Automatic Renewal in issues table
5- place a hold on the item for another patron
6- reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
7- change No Renewal Before value to 7
8- reload issues table for patron 1, confirm checkout now shows "on hold"
Comment 1 Joy Nelson 2020-06-15 15:20:07 UTC
Holds are a 'flux state' that change frequently.  Is it realistic to state that hold have been placed on a title when those holds may or may not impact the autorenewal of that item?  Perhaps the hold is cancelled prior to the autorenewal check or the hold was filled with another item prior to autorenewal check?
Comment 2 Ed Veal 2020-06-15 16:23:49 UTC
Joy, I understand your thinking and don't disagree. However, when staff is looking at a patrons record to renew things beyond the auto renew limit there is no indication that the specific item can't be renewed and generates confusion. 

Ed
Comment 3 Barbara Johnson 2020-06-15 23:28:16 UTC
I remember following bug 19014 and liked that patrons would no longer receive notices of non-renewal almost as soon as they checked out a popular title with lots of holds on it.  I don't think we understood that staff and patrons would no longer see the 'On Hold' message.   

I think an unintended consequence of the change is that a patron will think their item will renew since there is no indication it might not.  The patron doesn't know until they receive a notification on the day the item is due.  I also agree with Ed's statement regarding confusion on the staff side.
Comment 4 Jonathan Druart 2020-06-16 09:04:52 UTC
Should not we actually want to see both messages?
Comment 5 Andrew Fuerste-Henry 2020-06-16 16:38:38 UTC
(In reply to Jonathan Druart from comment #4)
> Should not we actually want to see both messages?

Both messages would be good, though that hasn't been the behavior before. How would we best convey, within our limited space, that the best we can give here is "If this renewal were to happen right now, it would/wouldn't be successful due to X"?
Comment 6 Donna 2020-06-16 18:51:56 UTC
The current behavior for non-auto renewable items that have a hold on them where the only available copy/copies is checked out is that the renew status changes to On Hold as soon as that hold is placed.  While the hold may be canceled before the due date, the patron and staff are both immediately able to see the current non-renewable status.  

For libraries that only use auto-renew for some item types, this change will cause even more confusion.

As a library user, I want to be able to see which of the 5 books I have checked out that I need to read first, since it potentially will not be renewable.  This is the behavior we have established in Koha.

I like the idea of showing that something is not potentially renewable due to a hold, but even setting it back to saying No renewal (and then getting a surprise renewal) would be better than the current situation.
Comment 7 Barbara Johnson 2020-06-16 19:57:59 UTC
I agree with Donna's assessment. I think it is more valuable to the patron to see the 'On Hold' info. We discovered additional confusion on the staff side.  We decided to turn on the system preference for AllowRenewalOnHoldOverride because we sometimes do need to be able to do this.  However, because the 'On Hold' info no longer displays staff found out later that they had unknowingly renewed something that had a hold on it.  We ended up having to turn that sys pref off.

I actually think going back to the 'On Hold' wording would work just fine.  But if we want to look at changing it maybe it could be something like:

Possible Hold
Potential Hold
Has a Hold
Holds Exist

1     Scheduled for automatic renewal
Possible Hold  (3 of 4 renewals remaining)
Comment 8 Peggy Thrasher 2020-06-18 19:57:15 UTC
We've been closed so this hasn't been a big deal, but now that we are opening holds are really important.  It is currently letting us renew items that are on hold.  This is going to make for some very angry patrons.  I hope that this can get fixed really soon!
Peggy
Comment 9 Barbara Johnson 2020-06-18 21:43:32 UTC
Peggy, We ran into the same problem.  If you have the AllowRenewalOnHoldOverride sys pref set to ‘Allow’ try changing it to ‘Don’t Allow’.  I agree with needing a fix soon.  Seeing the ‘On Hold’ information is essential to both patrons and staff.
Comment 10 Katrin Fischer 2020-06-22 20:36:17 UTC
(In reply to Joy Nelson from comment #1)
> Holds are a 'flux state' that change frequently.  Is it realistic to state
> that hold have been placed on a title when those holds may or may not impact
> the autorenewal of that item?  Perhaps the hold is cancelled prior to the
> autorenewal check or the hold was filled with another item prior to
> autorenewal check?

I think yes, when a hold blocks a renewal or auto renewal it should be stated as such (probably depends on AllowRenewalIfOtherItemsAvailable setting). It could be cancelled, but if not you'd want to plan for the case that you can no longer keep the item.
Comment 11 Daniel Gaghan 2020-06-22 20:51:00 UTC
I concur with Katrin, we have users who determine what material they read or view next based on if there is a hold on the material. Not having that notice will throw them off, especially when the "cannot autorenew due to hold" message appears the day the item is due.
Comment 12 Jonathan Druart 2020-06-24 09:14:21 UTC
Created attachment 106229 [details] [review]
Bug 25758: Display the different causes of non renewability
Comment 13 Jonathan Druart 2020-06-24 09:14:37 UTC
I have been trying to correct the unexpected behaviour, and I ended up with this patch.
It modifies the way CanBookBeReserved work to make it return the different reason of the non renewability.

I am not happy with the patch in the way that it will make thinks a bit more fragile.

Testing and feedback welcome.
Comment 14 Jonathan Druart 2020-06-24 09:15:59 UTC
This is what I see now: https://snipboard.io/dS1rsL.jpg
Comment 15 Barbara Johnson 2020-07-16 20:18:30 UTC
The test plan worked.  However, our issues table displays the number of renewals used and how many are remaining.  We wouldn't want to lose that information.  Here's a link to what we would prefer to see: https://snipboard.io/2lYXd6.jpg
Comment 16 Andrew Fuerste-Henry 2020-07-17 12:30:52 UTC
With your patch applied, checkouts don't show in the checkouts table at all if they don't have a hold on them and are not scheduled for auto-renew.

I don't believe we haven't historically shown the number of renewals remaining if there is a hold, because at that point the number of renewals is irrelevant. On master right now without this patch, if you have an checkout not scheduled for auto-renew and there is a hold, it just says "On hold." As far as I remember, that's been the case through recent versions.
Comment 17 Barbara Johnson 2020-07-17 21:26:16 UTC
When I checked out an item that did not have a hold on it and was not scheduled for auto renew the checkout screen did not load and display the item in the issues table. It only showed the 'Loading... you may continue scanning' button on the screen.  Subsequent checkouts did not display either and the 'Loading' button remained on the screen. The checkouts tab showed that 4 items had been checked out but they don't display in the issues table.

Here's a screenshot - https://snipboard.io/XgDbR7.jpg
Comment 18 Katrin Fischer 2020-07-18 13:04:39 UTC
From the comments I get that the patch attached is not working, but it seems agreeable that we should show the information about an existing hold.

Could you rework your patch?
Comment 19 Jonathan Druart 2020-07-27 10:38:14 UTC
Created attachment 107409 [details] [review]
Bug 25758: Fix handle of checkouts without holds
Comment 20 Jonathan Druart 2020-07-27 10:38:50 UTC
Sorry Barbara and Andrew, there was a silly error in the patch. Can you retest please?
Comment 21 Barbara Johnson 2020-07-28 16:15:11 UTC
Still running into a problem with this. I set the sys pref and created the loan rule.  I checked out an item to a patron but it is not displaying in the issues table.  The item does show that it is checked out to the patron when looking at the holdings table. Checking the ‘Always show checkouts immediately’ checkbox does not display the checked out title.  There is also a new ‘Show checkouts’ button which also does not display the checkouts.

Here are screenshots:
Checked out item is not displaying in issues table - https://snipboard.io/J063vL.jpg
Holdings table shows that item is actually checked out to the patron - https://snipboard.io/t5NwCX.jpg
Comment 22 Jonathan Druart 2020-07-29 07:48:57 UTC
Hi Barbara, I don't recreate.
How did you test? On a sandbox or locally? If locally, did you restart all the services (plack & memcached)?
Comment 23 Kelly McElligott 2020-07-29 12:59:37 UTC
Created attachment 107495 [details]
renew hold

This is what I am seeing while testing. I believe we may want to remove one of the "Scheduled for Automatic Renewal" message - as there are two.  Also, I had added a 1 to the no renewal column in the circulation rules, and the message here is saying "No Renewal before Null" - which I am not sure if that is correct.
Comment 24 Kelly McElligott 2020-07-29 13:02:56 UTC
Created attachment 107496 [details]
opac renew

Also, the OPAC is not showing that there is a hold - and looks like from the patron's perspective that the item will auto renew.  Is this in the scope of the bug? Or should it be a new one?
Comment 25 Jonathan Druart 2020-07-29 14:19:30 UTC
Created attachment 107504 [details] [review]
Bug 25758: Display the different causes of non renewability

Bug 25758: Fix handle of checkouts without holds
Comment 26 Jonathan Druart 2020-07-29 14:22:38 UTC
Thanks Kelly. What you reported should now be fixed by this last patch. Can you retest please?

You will note that the "no renewal before" will display the time part 00:00. It should certainly be removed, but it appears to be the same behaviour in master (should be fixed separately then).

About the OPAC, I'd like to confirm it works correctly staff-side before considering porting the code for the OPAC.
Comment 27 Barbara Johnson 2020-07-31 19:35:39 UTC
Jonathan - I retested with a new sandbox and the test plan worked.  
https://snipboard.io/vzET9N.jpg
Comment 28 Jonathan Druart 2020-08-06 08:23:01 UTC
Hi Barbara, thanks for testing!
Are you able to provide a signed-off patch (you can do that with the sandboxes)?
Otherwise I can do it for you.
Comment 29 Owen Leonard 2020-08-06 12:24:17 UTC
Created attachment 107879 [details] [review]
Bug 25758: Display the different causes of non renewability

Bug 25758: Fix handle of checkouts without holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 30 ByWater Sandboxes 2020-08-06 14:16:14 UTC
Created attachment 107894 [details] [review]
Bug 25758: Display the different causes of non renewability

Bug 25758: Fix handle of checkouts without holds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 31 Katrin Fischer 2020-08-23 11:05:28 UTC
I am not quite sure if that works alright, so adding my test plan:

1) Check out an item to any patron with checkbox "auto-renewal" selected
   The circulation rule is NOT set up for auto-renewal.
2) Place a hold on the record
3) Check out an item with circulation rule auto-renewal
4) Place a hold on the record too

The items show as scheduled for auto-renewal, but it only shows (on hold) in staff, not in the OPAC.
Comment 32 Barbara Johnson 2020-08-25 14:33:43 UTC
Using Katrin's test plan I got the same results.  The item shows that is scheduled for auto renewal and that it is 'on hold' in staff but none of that information displays to the patron in the OPAC.
Comment 33 Jonathan Druart 2020-08-26 08:59:09 UTC
The behaviour on master (as well as 20.05 and 19.11) is really bad, upping severity!

https://snipboard.io/vkA3D4.jpg
Comment 34 Jonathan Druart 2020-08-26 09:15:53 UTC
Created attachment 109139 [details] [review]
Bug 25758: Fix renewal at the OPAC
Comment 35 Jonathan Druart 2020-08-26 09:17:32 UTC
This patch is not enough.

Basically there are way more to fix, all the occurrences of

  $error = $error->[0] if @$error;

must be adjusted.

I am putting this in discussion to get feedback. I'd like to know if it's really the direction we want to take, it's not a trivial change.
Comment 36 Jonathan Druart 2020-08-26 09:20:54 UTC
Nick, I need you to review those 2 patches and give your opinion.

The problem is that, prior to bug 19014, we used to return on_reserve. Now we get auto_too_soon, which causes problem everywhere we check if the hold is possible.
Comment 37 Nick Clemens 2020-09-04 12:54:45 UTC
Created attachment 109658 [details] [review]
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t
Comment 38 Martin Renvoize 2020-09-04 15:05:28 UTC
Created attachment 109668 [details] [review]
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-09-04 15:06:39 UTC
Tested as per plan against master.. all appears to work well, thanks Nick.

Signing off.

I attempted to backport to 19.11.x but haven't succeeded yet.
Comment 40 Martin Renvoize 2020-09-04 15:42:51 UTC
Created attachment 109672 [details] [review]
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t
Comment 41 Martin Renvoize 2020-09-04 15:43:40 UTC
Something up with that backport.. uploaded in the hopes you can correct where I've gone wrong Nick :)
Comment 42 Martin Renvoize 2020-09-04 16:31:03 UTC
Created attachment 109680 [details] [review]
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2020-09-04 16:31:35 UTC
Thanks Nick, solution uploaded (after a chat on slack) :)
Comment 44 Katrin Fischer 2020-09-13 08:21:09 UTC
I know that we need this for stable releases, but instead of returning all errors, returning different errors depending on cron/GUI... why not make a new combined error code for auto_renew_too_soon and on_hold? No reason to block this, just something I wanted to note.
Comment 45 Katrin Fischer 2020-09-13 08:32:54 UTC
Created attachment 110008 [details] [review]
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

https://bugs.koha-community.org/show_bug.cgi?id=22758
Comment 46 Katrin Fischer 2020-09-13 08:44:48 UTC
Created attachment 110009 [details] [review]
Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Katrin Fischer 2020-09-13 08:47:38 UTC
Created attachment 110010 [details] [review]
[19.11] Bug 25758: Return on_reserve over too_soon when not calling from automatic_renewals cron

Bug 19014 altered CanBookBeRenewed to return (auto_)too_soon over on_reserve

For cron purposes this is the correct behaviour.

For display purposes we wish to see on_reserve over too_soon

This patchset adds a switch to 'CanBookBeRenewed' to alter the priority of these statuses

To test:
 1 - set NoRenewalBeforePrecision to date only
 2 - set a circ rule to auto-renewal=yes, no renewal before=0, checkout period to 7 days
 3 - check item out
 4 - confirm item shows Scheduled For Automatic Renewal in issues table
 5 - place a hold on the item for another patron
 6 - reload issues table for patron 1, confirm checkout still shows "scheduled for automatic renewal" rather than "on hold"
 7 - change No Renewal Before value to 7
 8 - reload issues table for patron 1, confirm checkout now shows "on hold"
 9 - Apply patch
10 - restart_all
11 - Reload the issues table - confirm 'on_hold' still shows
12 - Change No Renewal Before to 0
13 - Refresh issues table, still shows 'On hold'
14 - perl misc/cronjobs/automatic_renewals.pl -v
15 - Result shows 'auto_too_soon'
16 - prove -v t/db_dependent/Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Jonathan Druart 2020-09-28 08:53:41 UTC
Nick, is the plan to stay with this code or are we going to improve it on a separate bug report?
Comment 49 Jonathan Druart 2020-09-28 09:10:36 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 50 Nick Clemens 2020-09-28 12:25:28 UTC
(In reply to Katrin Fischer from comment #44)
> I know that we need this for stable releases, but instead of returning all
> errors, returning different errors depending on cron/GUI... why not make a
> new combined error code for auto_renew_too_soon and on_hold? No reason to
> block this, just something I wanted to note.(In reply to Jonathan Druart from comment #48)
> Nick, is the plan to stay with this code or are we going to improve it on a
> separate bug report?

I don't have a problem with this code, we could make the parameter 'always_return_holds' or something, rather than cron, to be a bit more clear

If we want to change things I think a new bug should be filed
Comment 51 Jonathan Druart 2020-09-28 12:59:18 UTC
So are we dropping the previous work/idea to display the different messages?
Comment 52 Magnus Enger 2020-11-13 12:48:47 UTC
Any chance this can be backported to 20.05 (and 19.11)? I thnk I am seing this problem on 19.11.
Comment 53 Rickard Lindfors 2020-11-20 08:55:49 UTC
Yes, this appears to be a problem on 20.05 as well.
Comment 54 Lucas Gass 2020-11-20 20:16:13 UTC
backported to 20.05.x for 20.05.06
Comment 55 Aleisha Amohia 2020-11-24 00:05:34 UTC
backported to 19.11.x for 19.11.12