Bug 10663 - Holds never block renewals
Summary: Holds never block renewals
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 10719
  Show dependency treegraph
 
Reported: 2013-07-30 08:30 UTC by Katrin Fischer
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10663 - Partial fix - blocking renewal for item level holds (1.34 KB, patch)
2013-07-30 13:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
But 10663 - Revert and use CheckReserves for CanBookBeRenewed (1.05 KB, patch)
2013-08-06 13:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10663: Holds never block renewals (2.43 KB, patch)
2013-08-06 20:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10663 - Unit tests for CanBookBeRenewed (5.64 KB, patch)
2013-08-06 21:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10633: Follow-up - Unit tests for CanBookBeRenewed (7.10 KB, patch)
2013-08-08 00:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10663 - Unit tests for CanBookBeRenewed (5.75 KB, patch)
2013-08-08 00:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10633: Follow-up - Unit tests for CanBookBeRenewed (7.10 KB, patch)
2013-08-08 00:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10663 - Revert and use CheckReserves for CanBookBeRenewed (1.30 KB, patch)
2013-08-08 00:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10663: Follow-up - Revert and use CheckReserves for CanBookBeRenewed (901 bytes, patch)
2013-08-08 00:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED_OFF] Bug 10663 - Unit tests for CanBookBeRenewed (5.79 KB, patch)
2013-08-08 04:49 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED_OFF] Bug 10663: Follow-up - Unit tests for CanBookBeRenewed (7.10 KB, patch)
2013-08-08 04:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10663 - Revert and use CheckReserves for CanBookBeRenewed (1.34 KB, patch)
2013-08-08 04:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10663: Follow-up - Revert and use CheckReserves for CanBookBeRenewed (955 bytes, patch)
2013-08-08 04:51 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED_QA] Bug 10663 - Revert and use CheckReserves for CanBookBeRenewed (1.70 KB, patch)
2013-08-15 11:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[COUNTER_PATCH] Bug 10663: Correcting GetReserveStatus so that holds block renewal (4.20 KB, patch)
2013-08-15 11:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10663: QA Followup: Typo in comment in renewscript (859 bytes, patch)
2013-08-15 11:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10663: (3.12.x followup) Rewrite some calls as unit tests rely on newer API (1.79 KB, patch)
2013-11-13 16:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-07-30 08:30:52 UTC
We had a library with 3.12.1 reporting this issue and I confirmed testing on the current master branch:

0) Allow OPAC renewals (OpacRenewalAllowed)
   Create a new record with one item.
1) Check out the item to patron A.
2) Place an item level hold for patron B.
3) Try to renew the item in the OPAC for patron A.

Problem: You can renew the item, the renewal is not blocked by the hold.
The same happens, when you have a title level hold instead of item level. 

I couldn't find any system preference affecting this behaviour. A quick test on an older 3.6.x installation shows, that renewals are blocked there.

Instead of the renewal checkbox we show a message:
 	Not renewable (On hold)
Comment 1 Katrin Fischer 2013-07-30 08:35:27 UTC
The logs are clean for renewing the item and accessing the 'my summary' tab.
Comment 2 Chris Cormack 2013-07-30 08:42:29 UTC
fad1f44d42600f440ac88c8df767cfc81c51dea9 is the commit that broke it

Bug 9367: Code optimization: CheckReserves is too often called
Comment 3 Katrin Fischer 2013-07-30 10:10:30 UTC
I tried reverting the commit, but ran into problems with conflicts. 

Further tests show that the hold is also not displayed when trying to renew in staff.
Comment 4 Katrin Fischer 2013-07-30 11:52:43 UTC
After reverting gave me trouble... I kept looking for the problem, and I think I found it... doing some more tests.
Comment 5 Katrin Fischer 2013-07-30 12:29:43 UTC
This change seems to improve things quite a bit:

     if(defined $found) {
         return 'Waiting'  if $found eq 'W' and $priority == 0;
         return 'Finished' if $found eq 'F';
-        return 'Reserved' if $priority > 0;
     }
+    if (defined $priority) {
+               return 'Reserved' if $priority > 0;
+       }
     return '';

When there is a hold, but the item is not W or F priority is NULL, so those cases were never caught here.

More tests in 3.6 show that we used to not allow renewals, when one item of many items or of course all items of many items from a record where checked out and a title level hold was placed. This is no longer the case and I think related to the fact that we check by itemnumber - for title level holds the itemnumber is not set in reserves.
Comment 6 Katrin Fischer 2013-07-30 13:19:23 UTC
Ok, if I understand this correctly, GetReserveStatus is always only called with itemnumber, so we completely ignore the possibility of a title level hold.
The new GetReserveStatus was introduced in lots of places - like CanItemBeRenewed and the formerly used CheckReserves.

I think I can't fix this and would say we need to revert the changes made by bug 9367. I am not sure how to do that and if it's the best solution. I would appreciate if someone else could take a look and try to untangle this.

I am going to attach a patch for the change noted below in case someone wants to use it.
Comment 7 Katrin Fischer 2013-07-30 13:30:16 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2013-07-30 15:09:47 UTC
(In reply to Katrin Fischer from comment #6)
> I think I can't fix this and would say we need to revert the changes made by
> bug 9367. I am not sure how to do that and if it's the best solution. I
> would appreciate if someone else could take a look and try to untangle this.

The code has moved to the point where it doesn't look like a revert of the patch series for bug 9367 is nontrivial.  I'll look at this bug further.
Comment 9 Katrin Fischer 2013-07-31 07:27:47 UTC
The more I look into this the more I think that GetReserveStatus is not suitable to determine if something is "Reserved" on title level. For item level holds we have the itemnumer and all is fine, given the patch I wrote is applied. To determine if an item can fill a title level hold is much more complicated. I don't think CheckReserves is perfect for this job either - but it might be worth reverting to using this in CanBookBeRenewed and try to improve after restoring the old behaviour.
Comment 10 Tomás Cohen Arazi 2013-08-06 13:52:31 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2013-08-06 20:12:31 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2013-08-06 20:13:43 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> Created attachment 20139 [details] [review] [review]
> Bug 10663: Holds never block renewals
> 
> Based on Katrin's patch, this one fixes GetReserveStatus to look for
> biblio-level
> holds when an itemtype is passed and no item-level hold is set.

                ^^^^^^^^
Of course I meant to write itemnumber, right? :-P
Comment 13 Katrin Fischer 2013-08-06 20:48:35 UTC
I think we should go with the first patch - fixing GetReserveStatus will take a lot more work and testing all the places it is in now too.

The only reserve status interesting for renewals is if something can actually fill a hold. If it's checked out, it will not be waiting on the pickup shelf (W). Also it being filled (F) would not be blocking renewal - and GetReserveStatus is not querying the right table for that status anyway (old_reserves).

I think fixing this bug, we need to revert to using CheckReserves - it's the same routine that is used on checkin, so it makes the most sense.
Comment 14 Tomás Cohen Arazi 2013-08-06 21:51:27 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-08-08 00:03:30 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2013-08-08 00:05:46 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-08-08 00:06:04 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2013-08-08 00:06:26 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-08-08 00:06:37 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2013-08-08 00:15:14 UTC
Some more notes about testing this:

A
0) Allow OPAC renewals (OpacRenewalAllowed)
   Create a new record with one item.
1) Check out the item to patron A.
2) Place an item level hold for patron B.
3) Try to renew the item in the OPAC for patron A.
> Renewal should be blocked after patches are applied.

B
4) Cancel item level hold
5) Place title level hold for patron B.
6) Try to renew the item in the OPAC for patron A.
> Renewal should be blocked, after patches are applied

Also:
a) Apply first 2 patches - run t/db_dependent/Circulation.t
Tests related to checking if an item is reserved will fail.

b) Apply last 2 patches - run tests again.
Tests should all pass now.
Comment 21 Chris Cormack 2013-08-08 04:49:58 UTC
Created attachment 20153 [details] [review]
[SIGNED_OFF] Bug 10663 - Unit tests for CanBookBeRenewed

Initial work on unit tests for CanBookBeRenewed. More scenarios are needed.
Also made Circulation.t use autocommit=0.

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Adding some more unit tests in follow up.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 22 Chris Cormack 2013-08-08 04:50:33 UTC
Created attachment 20154 [details] [review]
[SIGNED_OFF] Bug 10663: Follow-up - Unit tests for CanBookBeRenewed

Follow-up fixing some and adding more unit tests.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 23 Chris Cormack 2013-08-08 04:51:05 UTC Comment hidden (obsolete)
Comment 24 Chris Cormack 2013-08-08 04:51:26 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2013-08-15 09:35:34 UTC
I know that more tests contain references to patron type S and home branch
MPL but if my test database does not contain them, these kind of tests will
always fail..
Comment 26 Marcel de Rooy 2013-08-15 10:06:21 UTC
I am looking at this patch now. But I have the idea that we are not completely resolving the actual problem. The error is in GetReserveStatus and is not corrected with this patch.
Comment 27 Marcel de Rooy 2013-08-15 11:08:09 UTC
The circulation.tt template also contains another problem: If I cannot renew because of a hold, it should still be possible to check in the item. But this is not possible due to the same condition used:

            [% IF ( todayissue.renew_error_on_reserve ) %]
               <td><a href="/cgi-bin/koha/reserve/request.pl?biblionumber=[% todayissue.biblionumber %]">On hold</a>
                <input type="checkbox" name="all_barcodes[]" value="[% todayissue.barcode %]" checked="checked" style="display: none;" />
                </td>
            [% ELSE %]
            <td><input type="checkbox" class="radio" name="barcodes[]"  value="[% todayissue.barcode %]" />
                <input type="checkbox" name="all_barcodes[]" value="[% todayissue.barcode %]" checked="checked" style="display: none;" />
            </td>
            [% END %]
Comment 28 Marcel de Rooy 2013-08-15 11:22:37 UTC
Created attachment 20362 [details] [review]
[PASSED_QA] Bug 10663 - Revert and use CheckReserves for CanBookBeRenewed

It makes renewal lock by holds work again.
Unit tests as follow up

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
This reverts changes made to CanBookBeRenewed by
patches from bug 9376.
GetReserveStatus is not suitable to recognize if an item
can fild a hold on return and CheckReserves is restored.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Bug 10663: Follow-up - Revert and use CheckReserves for CanBookBeRenewed

CheckReserves returns '' when no reserve is found,
so $resfound will always be defined and we need to
check if it's true.

This is part of the revert.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Squashed the above two patches. Works as advertised.
Comment 29 Marcel de Rooy 2013-08-15 11:25:10 UTC
Created attachment 20363 [details] [review]
[COUNTER_PATCH] Bug 10663: Correcting GetReserveStatus so that holds block renewal

This patch is a counterproposal for the patch already present on 10663.
Instead of replacing the call to GetReserveStatus, it repairs GetReserveStatus.

Note that the biblionumber parameter of GetReserveStatus is actually not used.
It can be removed.
The sql statement is adjusted to include title level holds. And that solves the
problem reported.

Test plan:
Issue a book.
Enter a title level hold. Check if you can renew on circulation page.
Remove the title level hold. Enter an item level hold. Check again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2013-08-15 11:25:58 UTC
Created attachment 20364 [details] [review]
Bug 10663: QA Followup: Typo in comment in renewscript

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2013-08-15 11:33:46 UTC
QA Comment:
I skipped the unit tests for now. They are not that critical. And see also my earlier comment on hardcoded assumptions.
Attachment 20362 [details] is fine for me. But it leaves the problem in GetReserveStatus.
So I do not mind pushing this patch, because it resolves the reported issue.
But I think it would be better to repair the problem in GetReserveStatus.
For that reason I submitted attachment 20363 [details] [review]. I tested it, but I leave it to Galen if he wants another signoff on that one.
The last attachment just resolves a minor typo that I stumbled about when testing this..

Setting to Passed QA to attract the attention of RM.
Comment 32 Chris Cormack 2013-08-15 19:40:25 UTC
(In reply to M. de Rooy from comment #31)
> QA Comment:
> I skipped the unit tests for now. They are not that critical. And see also
> my earlier comment on hardcoded assumptions.
> Attachment 20362 [details] is fine for me. But it leaves the problem in
> GetReserveStatus.
> So I do not mind pushing this patch, because it resolves the reported issue.
> But I think it would be better to repair the problem in GetReserveStatus.

I think undoing the regression created should be first priority, ie pushing these patches because they undo the bad refactoring that created this problem

> For that reason I submitted attachment 20363 [details] [review] [review]. I tested
> it, but I leave it to Galen if he wants another signoff on that one.

I think this should go on it's own bug, and go through the proper signoff and qa process, separate to this. 

I am going to make a tshirt, REVERT REGRESSIONS !!! That should be our first policy, get it back to the behaviour that used to work. 
Then try the refactor again, this time with unit tests, so that we dont just bust things trying to be clever again.

However it is of course Galens call. 


> The last attachment just resolves a minor typo that I stumbled about when
> testing this..
> 
> Setting to Passed QA to attract the attention of RM.
Comment 33 Liz Rea 2013-08-15 21:52:07 UTC
All,

Guys, this was an embarrassing regression - this is functionality that has never been broken, and now we are "discussing" the solution to fixing it, when it really just needs to be fixed *right now*.

This is essential functionality for a library, and I don't think we can afford to be waiting on cleverness, or arguing over the solution for this one. 

Please let's go with the original patch. 

Liz
Comment 34 Galen Charlton 2013-08-15 22:52:44 UTC
I have pushed all that I intend to push for this bug, to master.  I've included a follow-up to set issuingrules explicitly for the purposes of Circulation.t.

Some observations:

- While fixing the regression is extremely important, of course, the most important part of the patch series from the POV of Koha's maintainability are the new test cases.  My thanks to Tomás and Katrin for writing them.  My particular thanks to Katrin for taking this opportunity to start learning how to write DB-dependent tests.

- This incident further strengthens my disinclination to push patches that aim to refactor or "clean up" code that are not accompanied by test cases that completely (as humanly possible) exercise the visible behavior whose implementation is being reworked.

- I acknowledge that the DB-dependent test suites currently make assumptions about which sample data is in place that are unwarranted.  I also know that many people, myself included, have been chipping away at those issues.

- Obviously there /is/ quite a bit of infelicity in the C4::Reserves API -- and it needs to be improved.  Yea, I say unto you, it even needs to be refactored. :)

But that's not going to happen without test cases.  As a starting point, I suggest that those who are interested go through C4/Reserves.pm, identify problematic routines, file bugs for each, and write test cases demonstrating what the expected behavior is.

- I am not opposed to squashing patches, particularly in cases where there's been flip-flopping or if judicious squashing would significantly improve the readability of the patch series.  I am /strenuously/ opposed to squashes that remove author attribution, even inadvertently.
Comment 35 Katrin Fischer 2013-08-15 23:48:58 UTC
Hi Galen, thx for taking a look at this, pushing the patches and improving the tests.

In addition to what you wrote, I want to try to explain, why I think fixing GetReserveStatus like Marcel suggested is not the way to go. Writing GetReserveStatus without using CheckReserves is trying to simplify something that is really complicated: Before you block renewal on an item, you want to check if it can actually fill the hold. There are lots of things that should be taken into account here - branches, item types, circulation rules, item statuses... CheckReseves is used on checking in the items to determine that - so I think there is the logic we need and we should try to not replicate this logic in the code. The counter patch will pass the unit tests probably, but only because I didn't manage to cover all use cases. There are a lot more tests to be written here.

After looking through the code trying to figure it all out I came to the conclusion that we should go back to use GetReserveStatus like it was used before the refactoring - only checking for 'waiting'. Or even better, refactor and have a properly named sub that does just this - backed up by unit tests - and remove GetReserveStatus.
Comment 36 Marcel de Rooy 2013-08-16 06:48:24 UTC
Good to see that the reported issue has been resolved now. Since we do not solve the real problem but provided a partial workaround, bug 10697 will have to finish the job.

Please note that the "being clever"-comments (imo) are not really motivating me or other people to spend more time on Koha.

About squashing two patches (one with perhaps 5 lines and the other one 1 line): if the second patch just corrects one of the other 5 lines, I think we should squash them. I just used the squash of git rebase -i for that, and did not advertently remove authorship (git apparently ignores that). (And note that this is about one trivial line, my counterpatch contains a lot of lines more, but goes to waste without notice.)
Comment 37 Katrin Fischer 2013-08-16 06:55:02 UTC
Marcel, did you take the time to read my comment? I have tried to explain why I think your counter patch won't work - it's not being ignored.
Comment 38 Marcel de Rooy 2013-08-16 08:05:56 UTC
(In reply to Katrin Fischer from comment #37)
> Marcel, did you take the time to read my comment? I have tried to explain
> why I think your counter patch won't work - it's not being ignored.

Thanks for responding. Probably it would have been helpful to have your explanation before starting, or maybe I should have read the other report better :) The information on reverting is now scattered over two reports, and that does not make it easier..
Comment 39 Tomás Cohen Arazi 2013-08-16 16:02:56 UTC
This patch has been pushed to 3.12.x, will be in 3.12.4.

Thanks Katrin and everyone involved!
Comment 40 Katrin Fischer 2013-09-10 07:13:11 UTC
(In reply to M. de Rooy from comment #27)
> The circulation.tt template also contains another problem: If I cannot renew
> because of a hold, it should still be possible to check in the item. But
> this is not possible due to the same condition used:
> 
>             [% IF ( todayissue.renew_error_on_reserve ) %]
>                <td><a href="/cgi-bin/koha/reserve/request.pl?biblionumber=[%
> todayissue.biblionumber %]">On hold</a>
>                 <input type="checkbox" name="all_barcodes[]" value="[%
> todayissue.barcode %]" checked="checked" style="display: none;" />
>                 </td>
>             [% ELSE %]
>             <td><input type="checkbox" class="radio" name="barcodes[]" 
> value="[% todayissue.barcode %]" />
>                 <input type="checkbox" name="all_barcodes[]" value="[%
> todayissue.barcode %]" checked="checked" style="display: none;" />
>             </td>
>             [% END %]

Hi Marcel,

IIRC this was done on purpose, because when checking in items from the patron account the printing of the reserve slip would not be triggered. So it forces you to properly return the items.
Comment 41 Marcel de Rooy 2013-09-16 08:43:17 UTC
(In reply to Katrin Fischer from comment #40)
> Hi Marcel,
> 
> IIRC this was done on purpose, because when checking in items from the
> patron account the printing of the reserve slip would not be triggered. So
> it forces you to properly return the items.

Thanks, Katrin.
If that is the case and it could be documented somewhere ..
But where would a developer that stumbles over this, probably look?
Comment 42 Katrin Fischer 2013-09-16 09:05:45 UTC
I remembered the bug report, you coudl take a look at git blame for those lines.
Comment 43 Tomás Cohen Arazi 2013-11-13 16:53:13 UTC
Created attachment 22913 [details] [review]
Bug 10663: (3.12.x followup) Rewrite some calls as unit tests rely on newer API

Some tests fail in t/db_dependent/Circulation.t because of API difference.
This patch is a followup for 3.12.x.

Regards
To+

Sponsored-by: Universidad Nacional de Córdoba