how to reproduce : Set an issuing rule with opacitemholds set to "Force" Make a search and select multiple records Place a hold on those records (using multi-hold records) Verify hold is placed on the record (next item available) where it shouldn't!
Created attachment 104488 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved test plan: 1) set issuingrule with opacitemholds set to "Force" 2) try to add a new hold using multi-hold 3) hold is placed on the record where it shouldn't 4) apply patch 5) retry to place same hold 6) success: hold cannot be placed since record-level hold is not available)
Created attachment 104496 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved test plan: 1) set issuingrule with opacitemholds set to "Force" 2) try to add a new hold using multi-hold 3) hold is placed on the record where it shouldn't 4) apply patch 5) retry to place same hold 6) success: hold cannot be placed since record-level hold is not available)
Please add tests to cover this change.
Aside from the QA issue: > 1) set issuingrule with opacitemholds set to "Force" > 2) try to add a new hold using multi-hold > 3) hold is placed on the record where it shouldn't Can't reproduce. To detail: - went to circ rules - there is only one general rule, for all sites, all item types, all categories - set "OPAC item level holds" for "Force" - went to the OPAC - used the search - ticked two result - "Place hold" - placed a hold without touching anything. (on record had 1 item, the other 2) - went back to the staff interface to each record's hold page - both hold are listen as "Only item" That's not expected right?
Well, seems this bug also impact holds placed with WebServices. CanItemBeReserved also miss this check... patch definitely need rework.
Created attachment 105988 [details] [review] Bug 25408: Added unit tests for opacitemholds policy
Created attachment 105989 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :)
Created attachment 105990 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :)
Created attachment 106413 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 106414 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
It works! Are the additional warnings caused by the patch? # Only test patch applied kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$ prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. 14/62 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 558. t/db_dependent/Reserves.t .. 59/62 # Failed test 'record-level holds should not be possible with opacitemholds set to "Force"' # at t/db_dependent/Reserves.t line 1013. # got: 'OK' # expected: 'RecordHoldNotAllowed' # Failed test 'item-level holds should not be possible with opacitemholds set to "No"' # at t/db_dependent/Reserves.t line 1049. # got: 'OK' # expected: 'notReservable' # Looks like you failed 2 tests of 6. # Failed test 'test opacitemholds rules' # at t/db_dependent/Reserves.t line 1079. # Looks like you failed 1 test of 3. t/db_dependent/Reserves.t .. 61/62 # Failed test 'reserves.item_level_hold' # at t/db_dependent/Reserves.t line 1080. t/db_dependent/Reserves.t .. 62/62 # Looks like you failed 1 test of 62. t/db_dependent/Reserves.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/62 subtests Test Summary Report ------------------- t/db_dependent/Reserves.t (Wstat: 256 Tests: 62 Failed: 1) Failed test: 61 Non-zero exit status: 1 Files=1, Tests=62, 7 wallclock secs ( 0.04 usr 0.01 sys + 5.90 cusr 0.83 csys = 6.78 CPU) Result: FAIL # Applied the implementation kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$ prove t/db_dependent/Reserves.t t/db_dependent/Reserves.t .. 20/62 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 577. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 577. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 425. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 329. Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 425. t/db_dependent/Reserves.t .. ok All tests successful. Files=1, Tests=62, 8 wallclock secs ( 0.05 usr 0.01 sys + 5.97 cusr 0.71 csys = 6.74 CPU) Result: PASS kohadev-koha@ea2bce8f4ac7:/kohadevbox/koha$
Created attachment 106521 [details] [review] Bug 25408: QA followup
Hi Tuxayo, Hope you are doing well, you seem to be doing fine! I've made a small qa patch to solve the warn :)
Created attachment 106535 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 106536 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 106537 [details] [review] Bug 25408: QA followup Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
(In reply to Arthur Suzuki from comment #14) > I've made a small qa patch to solve the warn :) It works and no additional warnings :) (In reply to Arthur Suzuki from comment #14) > Hi Tuxayo, > Hope you are doing well, you seem to be doing fine! Doing okay, best wishes for you :)
yay, thanks! :)
Needs a minor rebase I am not sure this is the best way to go. With these patches we fetch every item from the db and check the hold rule - but this doubles work we do in CanItemBeReserved and feels unnecessary Currently the loop in CanBookBeReserved returns as soon as it finds one 'OK' - you loop over all items to check the holds policy first - I think if we return the holds policy too from CanItemBeReserved we can return if one item is OK and item level holds not forced. I believe this would allow removal of the caller check too The code on the patches returns 'RecordHoldNotAllowed' if any items have item level holds forced, but shouldn't it only return that if all items on the biblio are forced? You add an extra fetch for the item which has already been fetched in the code: 429 $item = Koha::Items->find( $itemnumber );
Created attachment 107777 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 107778 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved test plan: 1) apply tests patch 2) run tests 3) verify not green 4) apply patch 5) run tests (again) 6) success: green :) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 107779 [details] [review] Bug 25408: (QA follow-up)
Created attachment 107780 [details] [review] Bug 25408: (QA follow-up)
(In reply to Nick Clemens from comment #20) > Needs a minor rebase done \o/ > > I am not sure this is the best way to go. With these patches we fetch every > item from the db and check the hold rule - but this doubles work we do in > CanItemBeReserved and feels unnecessary > > Currently the loop in CanBookBeReserved returns as soon as it finds one 'OK' > - you loop over all items to check the holds policy first - I think if we > return the holds policy too from CanItemBeReserved we can return if one item > is OK and item level holds not forced. I believe this would allow removal of > the caller check too Good advice, applied :) > The code on the patches returns 'RecordHoldNotAllowed' if any items have > item level holds forced, but shouldn't it only return that if all items on > the biblio are forced? > Erfff... you mean different items belonging to a single biblio could have different rules? daaaaaamn... > You add an extra fetch for the item which has already been fetched in the > code: > 429 $item = Koha::Items->find( $itemnumber ); This has been corrected in the qa patch :)
Afaik they can have different rules, but afaik the most strict is enforced in the GUI (which makes sense to me). So if you have one item that has enforced item holds, you will get that.
This seems to be affecting the staff side as well To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is not allowed 4 - Change rule to 'force' 5 - Hold can be placed I worry about this line: return { status => 'OK' } if ( $canReserve->{status} eq 'notReservable' && $opacitemholds eq 'N' ); as it short circuits some other checks, but I think the scripts end up double checking and it is not a problem. Other that that all the login in the scripts should be in the modules ;-) > afaik the most strict is enforced in the GUI (which makes sense to me) I talked this over with Andrew, there doesn't seem to be a perfect way to do this, but as long as this patch isn't changing current rules I think we are good Can we get a more detailed test plan covering the cases on opac and staff side?
Hm, do we need an interface parameter maybe to distinguish between the call coming from OPAC or staff?
(In reply to Nick Clemens from comment #27) > I worry about this line: > return { status => 'OK' } if ( $canReserve->{status} eq 'notReservable' && > $opacitemholds eq 'N' ); > as it short circuits some other checks, but I think the scripts end up > double checking and it is not a problem. Other that that all the login in > the scripts should be in the modules ;-) Checks not made in ILS-DI, at least when I first looked at the bug ;) Moving checks in this modules will helps maintaining coherence between interfaces (so i think...) but any opinion is welcome :) @Katrin : I see that there is now a "params" which might help passing data to these function, i'll try to get previous behaviour using this argument.
maybe i can try to remove double checks of this rule in other places in the code and see if tests passes as well.
Created attachment 111681 [details] [review] Bug 25408: (QA follow-up)
Created attachment 111682 [details] [review] Bug 25408: More test (with staff context)
Hi Katrin, I've added a mechanism so that the context (staff, opac, api) is passed to the Can(Item|Book)BeReserved. Nick, I couldn't see double checks in the pl's. (made a grep on opacitemholds in the pl's...) you are right and i didn't try to remove anything :)
Hi Arthur, your commit messages are a little sparse, the first commit should have a description of the problem and the test plan. Can you please add some more inofrmation? (see also comment#27?)
Created attachment 111693 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 111696 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 111716 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 111717 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Hold can be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 111718 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 111719 [details] [review] Bug 25408: (QA follow-up)
Created attachment 111720 [details] [review] Bug 25408: (QA follow-up)
Created attachment 111721 [details] [review] Bug 25408: More test (with staff context)
re-uploaded everything in the proper order to ease application of the patch with git-bz
> 5 - Hold is not allowed «There are no items that can be placed on hold.» Looks ok I guess. > 7 - Hold can be placed in all interfaces Hold in staff interface silently fails. I go back to the hold list page but no hold listed. No logged error. Hold in OPAC fails with «This title cannot be requested.»
Created attachment 115125 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Hi, I've rebased the patch and have added an error message on staff interface
Created attachment 115126 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 115127 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 115128 [details] [review] Bug 25408: (QA follow-up) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold
Created attachment 115129 [details] [review] Bug 25408: More test (with staff context)
Created attachment 115130 [details] [review] Bug 25408: (QA follow-up) Added handling of calling context (staff or else)
1. Not sure I understand, CanItemBeReserved is called from other C4::Reserves subroutines where the context is not passed. Why only passing it from direct calls from controller scripts? 2. The following change is not needed: sub CanBookBeReserved{ my ($borrowernumber, $biblionumber, $pickup_branchcode, $params) = @_; + my $patron = Koha::Patrons->find( $borrowernumber ); + my $borrower = $patron->unblessed; 3. Commit messages "(QA follow-up)" are not meaningful. Add more info or squash.
Can you please take a look at the last comment, Arthur?
Created attachment 121524 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 121525 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Hi Joubu, points 2 and 3 from last comment has been addressed. "Context" variable name is missused, I'm uploading a new version of the patch with use of C4::Context->interface.
Created attachment 121526 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 121527 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 121528 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 121530 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
> 6 - Change rule to 'force' > 7 - Item-level hold can be placed in all interfaces Odd, in staff I get «Too many holds: Jane Doe has too many holds.» So no holds at all can be placed. OPAC: «This title cannot be requested.» Also > 4 - Attempt to place hold from opac or api > 5 - Hold is not allowed I can't place at all an OPAC hold when "OPAC item level holds" : don't allow I get «There are no items that can be placed on hold.» Did it work for you?
Created attachment 128505 [details] [review] Bug 25408: (QA follow-up) This rule should not apply when placing a hold from staff interface
Salut Victor :) Sorry for the late reply... I made a follow-up patch to make sure things won't be changed on the staff interface. Can you retry the patch-set please? Thanks in advance! All the best, Arthur
Oh damn it, I see what you meant by "I can't place a hold at all from Opac". forget the "re-test please", I will re-work things (opac interface) and re-upload a patch. Things are working from staff and ils-di though.
(In reply to Arthur Suzuki from comment #64) > Oh damn it, I see what you meant by "I can't place a hold at all from Opac". > forget the "re-test please", I will re-work things (opac interface) and > re-upload a patch. > Things are working from staff and ils-di though. Ok, i got the issue while applying this patch to 20.11, but on master it seems ok, I can place a hold at the record level on a koha-testing-docker. Or maybe it's late here, buzzy day and i'm not sure about anything anymore... I guess another test from another person won't hurt. Best, Arthur Suzuki for BibLibre Support Team
Created attachment 130180 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 130181 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Main patch makes a cosmetic change in t/db_dependent/Reserves.t : diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index c4afba6424..5c4b0b64df 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1083,7 +1083,7 @@ subtest 'reserves.item_level_hold' => sub { subtest 'test opacitemholds rules in staff context' => sub { plan tests => 2; - C4::Context->interface('intranet'); + C4::Context->interface('intranet'); Avoid this to ease rebases.
Patch does not apply : > git bz apply 25408 Bug 25408 - CanBookBeReserved & CanItemBeReserved should check "opacitemholds" policy 130180 - Bug 25408: Added unit tests for opacitemholds policy 130181 - Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 25408: Added unit tests for opacitemholds policy Applying: Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved error: sha1 information is lacking or useless (t/db_dependent/Reserves.t). error: could not build fake ancestor Patch failed at 0001 Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-25408-Add-opacitemholds-checks-in-CanBookBeRes-2ODFm7.patch
Created attachment 132658 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 132660 [details] [review] Bug 25408: Added unit tests for opacitemholds policy Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 132661 [details] [review] Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved The rule "opacitemholds" seemed not to be controlled in some cases. This patch adds a control for this issuingrule in such a way it is checked across all interfaces (staff, opac, api) Added error message when trying to hold on the record and opacitemhold is set to "Force" to item-level hold To test: 0 - Apply patches 1 - Set rule to "Don't allow" item specific holds on opac 2 - Attempt to place hold on staff side 3 - Hold is still allowed 4 - Attempt to place hold from opac or api 5 - Hold is not allowed 6 - Change rule to 'force' 7 - Item-level hold can be placed in all interfaces 8 - Record-level hold cannot be placed in all interfaces Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
There has been a confusion. My last testing was from before the last changes to fix issues found afterwards. So my signoff isn't valid anymore and this patch is still waiting for me or someone else to test it again. Switching back to need SO
I'm not sure I understand exactly what is going on, but I have changed the status to Failed QA after: - working through the test plan, and - working through all the options (Allow, Don't allow, Force) for the OPAC, staff interface, and ILS-DI. If I've totally misunderstood what this is about and what is required, please feel free to change the status back! Questions: 1. Should the circulation rule "OPAC item level holds" apply for the staff interface? If it should, then the setting name in the staff interface needs changing. 2. By API I'm assuming ILS-DI. If it needs testing using the REST API, can you provide some instructions on how to do this? (I have Postman available + the RESTer addon for Firefox, but APIs are not something I'm familiar with). I only tested with a circulation rule of All All - I didn't try different combinations of libraries, patron types, and item types. I used koha-testing-docker with the default configuration provided - I didn't change any system preferences apart from enabling ILS-DI. Test plan results ~~~~~~~~~~~~~~~~~ Step 1: OPAC (setting = Don't allow) - can't place any holds either at record level or item level, message for each record is "There are no items that can be placed on hold." Step 2: Staff interface (setting = Don't allow) - can still place holds (as expected), when placing multiple holds from the search don't get to select item-level holds; when placing holds from the record can select and place item level holds Step 4: . OPAC (setting = Don't allow) - can't place ANY holds (either record level or item level), message for is "There are no items that can be placed on hold." . ILS-DI (setting = Don't allow) - record level hold placed (as expected), item level hold NOT placed (as expected), get response <code>recordHoldsOnly</code> Step 7: . OPAC (setting = Force) - can't place ANY holds (either record level or item level), message is "This title cannot be requested." . Staff interface (setting = Force) - no change in behaviour from before the patch was applied: can still pace a hold when on the record detail page (either record level hold or item level hold); for records with multiple items and from search cannot select individual items to place a hold on . ILS-DI (setting = Force) - record level hold NOT placed (as expected), get response <code>recordHoldNotAllowed</code>; item level hold place (as expected) Before patch applied ~~~~~~~~~~~~~~~~~~~~ OPAC - Before patch applied - single record: all PASS, works as expected for Allow, Don't allow, and Force - Before patch applied - multiple records from search, records with one item and multiple items: all PASS, works as expected for Allow, Don't allow, and Force Staff interface - Didn't expect to work as circulation rule setting is for OPAC - Force = doesn't work - With any of the settings for records with multiple items and from search cannot select individual items to place a hold on Record level hold using ILS_DI http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=49&bib_id=146&request_location=127.0.0.1 - OPAC item level holds = Allow = PASS (hold placed) - OPAC item level holds = Don't allow = FAIL (hold placed) - OPAC item level holds = Force = FAIL (hold placed) Item level hold using ILS_DI http://127.0.0.1:8080/:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=49&bib_id=146&item_id=315 - OPAC item level holds = Allow = PASS (item level hold placed) - OPAC item level holds = Don't allow = FAIL (item level hold placed) - OPAC item level holds = Force = PASS (item level hold placed) Tests pass: prove t/db_dependent/Reserves.t After patch applied ~~~~~~~~~~~~~~~~~~~ OPAC - After patch applied - single record: - Allow: can place a record level or item level hold - Don't allow: can't place a hold, message is "There are no items that can be placed on hold." - Force: can't place a hold, message is "This title cannot be requested" - After patch applied - multiple records from search, one record with one item and two records with multiple items: - Allow: can place record level and item level holds - Don't allow: can't place any holds, message for all records is "There are no items that can be placed on hold." - Force: can't place any holds, message for all records is "This title cannot be requested." Staff interface: - No change in behaviour from before the patch was applied - With any of the settings for records with multiple items and from search cannot select individual items to place a hold on Record level hold using ILS-DI http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=49&bib_id=146&request_location=127.0.0.1 - OPAC item level holds = Allow = PASS (hold placed) - OPAC item level holds = Don't allow = PASS (hold placed) - OPAC item level holds = Force = PASS (hold NOT placed, get response <code>recordHoldNotAllowed</code>) Item level hold using ILS-DI http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=49&bib_id=146&item_id=315 - OPAC item level holds = Allow = PASS (item level hold placed) - OPAC item level holds = Don't allow = PASS (item level hold NOT placed, get response <code>recordHoldsOnly</code>) - OPAC item level holds = Force = PASS (item level hold placed) Tests pass: prove t/db_dependent/Reserves.t