[Bug 10663] New: Holds don't block renewal in OPAC
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Bug ID: 10663 Summary: Holds don't block renewal in OPAC Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: katrin.fischer@bsz-bw.de QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- The logs are clean for renewing the item and accessing the 'my summary' tab. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chris@bigballofwax.co.nz --- Comment #2 from Chris Cormack <chris@bigballofwax.co.nz> --- fad1f44d42600f440ac88c8df767cfc81c51dea9 is the commit that broke it Bug 9367: Code optimization: CheckReserves is too often called -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |critical --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- After reverting gave me trouble... I kept looking for the problem, and I think I found it... doing some more tests. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Holds don't block renewal |Holds never block renewals |in OPAC | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 19987 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19987&action=edit Bug 10663 - Partial fix - blocking renewal for item level holds Item level holds should prevent renewal of the item in the OPAC and in staff. For item level holds that are not yet W (=Waiting) GetReserveStatus didn't return the item as reserved, because reserves.found is NULL in this case. To test: - Add a record with an item - Check the item out - Check that you can renew the issue in OPAC and staff - Place an item level hold on the item - Check that you can no longer renew in OPAC and staff This is probably only a partial fix to a bigger problem, see notes on Bug 10663. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #8 from Galen Charlton <gmcharlt@gmail.com> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 20129 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20129&action=edit But 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 Córdoba -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20129|0 |1 is obsolete| | CC| |tomascohen@gmail.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19987|0 |1 is obsolete| | --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 20139 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20139&action=edit 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. Please test using Katrin's test plan. Regards To+ Sponsored-by: Universidad Nacional de Cordoba -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #11)
Created attachment 20139 [details] [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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20129|1 |0 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20139|0 |1 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 20140 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20140&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20129|0 |1 is obsolete| | Attachment #20140|0 |1 is obsolete| | --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20148 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20148&action=edit Bug 10633: Follow-up - Unit tests for CanBookBeRenewed Follow-up fixing some and adding more unit tests. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20149 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20149&action=edit [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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20148|0 |1 is obsolete| | --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20150 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20150&action=edit Bug 10633: Follow-up - Unit tests for CanBookBeRenewed Follow-up fixing some and adding more unit tests. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20151 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20151&action=edit [SIGNED OFF] 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 Córdoba 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 20152 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20152&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #21 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 20153 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20153&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20149|0 |1 is obsolete| | Attachment #20150|0 |1 is obsolete| | --- Comment #22 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 20154 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20154&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #23 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 20155 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20155&action=edit 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 Córdoba 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20151|0 |1 is obsolete| | Attachment #20152|0 |1 is obsolete| | --- Comment #24 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 20156 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20156&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=10697 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |10719 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #25 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |katrin.fischer@bsz-bw.de |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #26 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #27 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 %] -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20153|Bug 10663 - Unit tests for |[SIGNED_OFF] Bug 10663 - description|CanBookBeRenewed |Unit tests for | |CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20154|Bug 10663: Follow-up - Unit |[SIGNED_OFF] Bug 10663: description|tests for CanBookBeRenewed |Follow-up - Unit tests for | |CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20155|0 |1 is obsolete| | Attachment #20156|0 |1 is obsolete| | --- Comment #28 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 20362 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20362&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20362|Bug 10663 - Revert and use |[PASSED_QA] Bug 10663 - description|CheckReserves for |Revert and use |CanBookBeRenewed |CheckReserves for | |CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20362|[PASSED_QA] Bug 10663 - |[PASSED_QA_OPTION_1] Bug description|Revert and use |10663 - Revert and use |CheckReserves for |CheckReserves for |CanBookBeRenewed |CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #29 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 20363 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20363&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #30 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 20364 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20364&action=edit Bug 10663: QA Followup: Typo in comment in renewscript Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #31 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 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. 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20363|Bug 10663: Correcting |[COUNTER_PATCH] Bug 10663: description|GetReserveStatus so that |Correcting GetReserveStatus |holds block renewal |so that holds block renewal -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20362|[PASSED_QA_OPTION_1] Bug |[PASSED_QA] Bug 10663 - description|10663 - Revert and use |Revert and use |CheckReserves for |CheckReserves for |CanBookBeRenewed |CanBookBeRenewed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #32 from Chris Cormack <chris@bigballofwax.co.nz> --- (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]. 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #33 from Liz Rea <liz@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #34 from Galen Charlton <gmcharlt@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #35 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #36 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #37 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #38 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9367 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #39 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.4. Thanks Katrin and everyone involved! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #40 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #41 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #42 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I remembered the bug report, you coudl take a look at git blame for those lines. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10663 --- Comment #43 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 22913 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22913&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org