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?

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

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.

Should not we actually want to see both messages?

(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"?

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.

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)

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

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.

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

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.

Created attachment 106229 [details] [review]
Bug 25758: Display the different causes of non renewability

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.

This is what I see now: https://snipboard.io/dS1rsL.jpg

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

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.

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

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?

Created attachment 107409 [details] [review]
Bug 25758: Fix handle of checkouts without holds

Sorry Barbara and Andrew, there was a silly error in the patch. Can you retest please?

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

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

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.

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?

Created attachment 107504 [details] [review]
Bug 25758: Display the different causes of non renewability

Bug 25758: Fix handle of checkouts without holds

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.

Jonathan - I retested with a new sandbox and the test plan worked.  
https://snipboard.io/vzET9N.jpg

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.

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>

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>

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.

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.

The behaviour on master (as well as 20.05 and 19.11) is really bad, upping severity!

https://snipboard.io/vkA3D4.jpg

Created attachment 109139 [details] [review]
Bug 25758: Fix renewal at the OPAC

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.

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.

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

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>

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.

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

Something up with that backport.. uploaded in the hopes you can correct where I've gone wrong Nick :)

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>

Thanks Nick, solution uploaded (after a chat on slack) :)

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.

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

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>

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>

Nick, is the plan to stay with this code or are we going to improve it on a separate bug report?

Pushed to master for 20.11, thanks to everybody involved!

(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

So are we dropping the previous work/idea to display the different messages?

Any chance this can be backported to 20.05 (and 19.11)? I thnk I am seing this problem on 19.11.

Yes, this appears to be a problem on 20.05 as well.

backported to 20.05.x for 20.05.06

backported to 19.11.x for 19.11.12

*** Bug 28435 has been marked as a duplicate of this bug. ***