Bug 25408 - CanBookBeReserved & CanItemBeReserved should check "opacitemholds" policy
Summary: CanBookBeReserved & CanItemBeReserved should check "opacitemholds" policy
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Arthur Suzuki
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 30845 30851
  Show dependency treegraph
 
Reported: 2020-05-07 09:45 UTC by Arthur Suzuki
Modified: 2022-11-14 14:52 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved (2.49 KB, patch)
2020-05-07 10:38 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved (2.38 KB, patch)
2020-05-07 11:49 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.70 KB, patch)
2020-06-17 22:07 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.41 KB, patch)
2020-06-17 22:07 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.41 KB, patch)
2020-06-17 22:11 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.75 KB, patch)
2020-06-29 18:18 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.46 KB, patch)
2020-06-29 18:18 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: QA followup (764 bytes, patch)
2020-07-03 14:56 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.75 KB, patch)
2020-07-05 02:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.46 KB, patch)
2020-07-05 02:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: QA followup (822 bytes, patch)
2020-07-05 02:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.76 KB, patch)
2020-08-04 14:45 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.88 KB, patch)
2020-08-04 14:45 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (2.66 KB, patch)
2020-08-04 14:45 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (2.66 KB, patch)
2020-08-04 14:47 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (4.79 KB, patch)
2020-10-15 08:12 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: More test (with staff context) (2.11 KB, patch)
2020-10-15 08:12 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (4.04 KB, patch)
2020-10-15 09:58 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (4.04 KB, patch)
2020-10-15 10:12 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (4.22 KB, patch)
2020-10-15 12:03 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (4.22 KB, patch)
2020-10-15 12:21 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.76 KB, patch)
2020-10-15 12:22 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (2.66 KB, patch)
2020-10-15 12:22 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (4.79 KB, patch)
2020-10-15 12:22 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: More test (with staff context) (2.11 KB, patch)
2020-10-15 12:22 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.76 KB, patch)
2021-01-14 04:45 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (3.76 KB, patch)
2021-01-14 04:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.49 KB, patch)
2021-01-14 04:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (3.69 KB, patch)
2021-01-14 04:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: More test (with staff context) (2.39 KB, patch)
2021-01-14 04:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) (3.75 KB, patch)
2021-01-14 04:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (5.14 KB, patch)
2021-06-01 09:16 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.17 KB, patch)
2021-06-01 09:16 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (6.80 KB, patch)
2021-06-01 09:58 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (5.04 KB, patch)
2021-06-01 09:59 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (6.83 KB, patch)
2021-06-01 09:59 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (6.78 KB, patch)
2021-06-01 12:08 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) This rule should not apply when placing a hold from staff interface (6.03 KB, patch)
2021-12-14 12:10 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (5.04 KB, patch)
2022-02-04 21:14 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (3.23 KB, patch)
2022-02-04 21:15 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (4.93 KB, patch)
2022-03-30 19:45 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (4.93 KB, patch)
2022-03-31 07:20 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (2.74 KB, patch)
2022-03-31 07:20 UTC, Arthur Suzuki
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2020-05-07 09:45:03 UTC
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!
Comment 1 Arthur Suzuki 2020-05-07 10:38:02 UTC Comment hidden (obsolete)
Comment 2 Arthur Suzuki 2020-05-07 11:49:28 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2020-05-11 10:23:55 UTC
Please add tests to cover this change.
Comment 4 Victor Grousset/tuxayo 2020-05-31 20:59:31 UTC
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?
Comment 5 Arthur Suzuki 2020-06-12 11:49:09 UTC
Well, seems this bug also impact holds placed with WebServices.
CanItemBeReserved also miss this check...
patch definitely need rework.
Comment 6 Arthur Suzuki 2020-06-12 11:50:03 UTC
Well, seems this bug also impact holds placed with WebServices.
CanItemBeReserved also miss this check...
patch definitely need rework.
Comment 7 Arthur Suzuki 2020-06-17 22:07:50 UTC Comment hidden (obsolete)
Comment 8 Arthur Suzuki 2020-06-17 22:07:54 UTC Comment hidden (obsolete)
Comment 9 Arthur Suzuki 2020-06-17 22:11:00 UTC Comment hidden (obsolete)
Comment 10 Victor Grousset/tuxayo 2020-06-29 18:18:51 UTC Comment hidden (obsolete)
Comment 11 Victor Grousset/tuxayo 2020-06-29 18:18:54 UTC Comment hidden (obsolete)
Comment 12 Victor Grousset/tuxayo 2020-06-29 18:20:50 UTC
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$
Comment 13 Arthur Suzuki 2020-07-03 14:56:59 UTC Comment hidden (obsolete)
Comment 14 Arthur Suzuki 2020-07-03 14:59:38 UTC
Hi Tuxayo,
Hope you are doing well, you seem to be doing fine!
I've made a small qa patch to solve the warn :)
Comment 15 Victor Grousset/tuxayo 2020-07-05 02:53:15 UTC Comment hidden (obsolete)
Comment 16 Victor Grousset/tuxayo 2020-07-05 02:53:18 UTC Comment hidden (obsolete)
Comment 17 Victor Grousset/tuxayo 2020-07-05 02:53:22 UTC Comment hidden (obsolete)
Comment 18 Victor Grousset/tuxayo 2020-07-05 02:55:01 UTC
(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 :)
Comment 19 Arthur Suzuki 2020-07-09 15:11:08 UTC
yay, thanks! :)
Comment 20 Nick Clemens 2020-07-31 11:15:36 UTC
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 );
Comment 21 Arthur Suzuki 2020-08-04 14:45:13 UTC Comment hidden (obsolete)
Comment 22 Arthur Suzuki 2020-08-04 14:45:27 UTC Comment hidden (obsolete)
Comment 23 Arthur Suzuki 2020-08-04 14:45:31 UTC Comment hidden (obsolete)
Comment 24 Arthur Suzuki 2020-08-04 14:47:36 UTC Comment hidden (obsolete)
Comment 25 Arthur Suzuki 2020-08-04 14:51:15 UTC
(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 :)
Comment 26 Katrin Fischer 2020-08-05 11:22:10 UTC
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.
Comment 27 Nick Clemens 2020-08-05 14:34:14 UTC
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?
Comment 28 Katrin Fischer 2020-08-05 14:46:43 UTC
Hm, do we need an interface parameter maybe to distinguish between the call coming from OPAC or staff?
Comment 29 Arthur Suzuki 2020-10-14 14:25:36 UTC
(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.
Comment 30 Arthur Suzuki 2020-10-14 14:30:08 UTC
maybe i can try to remove double checks of this rule in other places in the code and see if tests passes as well.
Comment 31 Arthur Suzuki 2020-10-15 08:12:22 UTC Comment hidden (obsolete)
Comment 32 Arthur Suzuki 2020-10-15 08:12:41 UTC Comment hidden (obsolete)
Comment 33 Arthur Suzuki 2020-10-15 08:20:42 UTC
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 :)
Comment 34 Katrin Fischer 2020-10-15 09:43:04 UTC
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?)
Comment 35 Arthur Suzuki 2020-10-15 09:58:35 UTC Comment hidden (obsolete)
Comment 36 Arthur Suzuki 2020-10-15 10:12:54 UTC Comment hidden (obsolete)
Comment 37 Arthur Suzuki 2020-10-15 12:03:55 UTC Comment hidden (obsolete)
Comment 38 Arthur Suzuki 2020-10-15 12:21:56 UTC Comment hidden (obsolete)
Comment 39 Arthur Suzuki 2020-10-15 12:22:15 UTC Comment hidden (obsolete)
Comment 40 Arthur Suzuki 2020-10-15 12:22:19 UTC Comment hidden (obsolete)
Comment 41 Arthur Suzuki 2020-10-15 12:22:24 UTC Comment hidden (obsolete)
Comment 42 Arthur Suzuki 2020-10-15 12:22:30 UTC Comment hidden (obsolete)
Comment 43 Arthur Suzuki 2020-10-15 12:23:25 UTC
re-uploaded everything in the proper order to ease application of the patch with git-bz
Comment 44 Victor Grousset/tuxayo 2020-11-06 21:58:21 UTC
> 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.»
Comment 45 Arthur Suzuki 2021-01-14 04:45:31 UTC Comment hidden (obsolete)
Comment 46 Arthur Suzuki 2021-01-14 04:46:41 UTC
Hi,
I've rebased the patch and have added an error message on staff interface
Comment 47 Arthur Suzuki 2021-01-14 04:53:53 UTC Comment hidden (obsolete)
Comment 48 Arthur Suzuki 2021-01-14 04:54:07 UTC Comment hidden (obsolete)
Comment 49 Arthur Suzuki 2021-01-14 04:54:11 UTC Comment hidden (obsolete)
Comment 50 Arthur Suzuki 2021-01-14 04:54:15 UTC Comment hidden (obsolete)
Comment 51 Arthur Suzuki 2021-01-14 04:54:19 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2021-01-19 14:37:17 UTC
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.
Comment 53 Katrin Fischer 2021-01-21 19:56:04 UTC
Can you please take a look at the last comment, Arthur?
Comment 54 Arthur Suzuki 2021-06-01 09:16:18 UTC Comment hidden (obsolete)
Comment 55 Arthur Suzuki 2021-06-01 09:16:34 UTC Comment hidden (obsolete)
Comment 56 Arthur Suzuki 2021-06-01 09:21:04 UTC
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.
Comment 57 Arthur Suzuki 2021-06-01 09:58:50 UTC Comment hidden (obsolete)
Comment 58 Arthur Suzuki 2021-06-01 09:59:35 UTC Comment hidden (obsolete)
Comment 59 Arthur Suzuki 2021-06-01 09:59:56 UTC Comment hidden (obsolete)
Comment 60 Arthur Suzuki 2021-06-01 12:08:57 UTC Comment hidden (obsolete)
Comment 61 Victor Grousset/tuxayo 2021-06-19 11:01:11 UTC
> 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?
Comment 62 Arthur Suzuki 2021-12-14 12:10:40 UTC Comment hidden (obsolete)
Comment 63 Arthur Suzuki 2021-12-14 12:13:10 UTC
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
Comment 64 Arthur Suzuki 2021-12-14 15:10:07 UTC
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.
Comment 65 Arthur Suzuki 2021-12-14 16:41:38 UTC
(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
Comment 66 Arthur Suzuki 2022-02-04 21:14:24 UTC Comment hidden (obsolete)
Comment 67 Arthur Suzuki 2022-02-04 21:15:03 UTC Comment hidden (obsolete)
Comment 68 Fridolin Somers 2022-03-11 23:27:19 UTC
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.
Comment 69 Fridolin Somers 2022-03-11 23:33:30 UTC
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
Comment 70 Arthur Suzuki 2022-03-30 19:45:39 UTC Comment hidden (obsolete)
Comment 71 Arthur Suzuki 2022-03-31 07:20:41 UTC
Created attachment 132660 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 72 Arthur Suzuki 2022-03-31 07:20:58 UTC
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>
Comment 73 Victor Grousset/tuxayo 2022-04-23 20:53:09 UTC
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
Comment 74 David Nind 2022-05-27 22:04:09 UTC
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