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: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Arthur Suzuki
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks: 30845 30851
  Show dependency treegraph
 
Reported: 2020-05-07 09:45 UTC by Arthur Suzuki
Modified: 2024-04-26 19:36 UTC (History)
8 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
Bug 25408: Added unit tests for opacitemholds policy (5.70 KB, patch)
2024-04-11 12:41 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (7.74 KB, patch)
2024-04-11 12:41 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) using perltidy to resolve complaining QA scripts (192.96 KB, patch)
2024-04-11 12:41 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (5.70 KB, patch)
2024-04-11 12:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.47 KB, patch)
2024-04-11 12:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: (QA follow-up) using perltidy to resolve complaining QA scripts (192.96 KB, patch)
2024-04-11 12:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (7.13 KB, patch)
2024-04-11 13:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.74 KB, patch)
2024-04-11 13:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (7.18 KB, patch)
2024-04-15 11:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.80 KB, patch)
2024-04-15 11:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (4.72 KB, patch)
2024-04-18 14:16 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.15 KB, patch)
2024-04-18 14:16 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (4.77 KB, patch)
2024-04-24 00:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.20 KB, patch)
2024-04-24 00:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Added unit tests for opacitemholds policy (4.83 KB, patch)
2024-04-24 01:12 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved (8.26 KB, patch)
2024-04-24 01:12 UTC, Victor Grousset/tuxayo
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 (kidclamp) 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 (kidclamp) 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
Comment 75 Arthur Suzuki 2024-04-10 12:11:52 UTC
(In reply to David Nind from comment #74)
> 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.

It doesn't, it is only supposed to be effective for OPAC actions, hence the name should be kept.

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

It is mostly for ILS-DI, which is supposed be an interface for discovery tools so, very close or supposed to work as an OPAC.
In my opinion, API are to be used for automation as staff interface. if the api route is supposed to behave like OPAC, it is usually in the "/public" namespace.
Comment 76 Arthur Suzuki 2024-04-10 13:52:55 UTC
@David, I rebased the patch on latest master but reproduce what you've seen.
I cannot place holds from OPAC with opacitemholds rule set to "Don't Allow" : should be able to place a record-level hold.

Same with value "Force", although I should be able to place item-level holds.
This patch needs rework... working on it
Comment 77 Arthur Suzuki 2024-04-11 12:41:38 UTC
Created attachment 164680 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy
Comment 78 Arthur Suzuki 2024-04-11 12:41:42 UTC
Created attachment 164681 [details] [review]
Bug 25408: Add "opacitemholds" checks in CanBookBeReserved & CanItemBeReserved
Comment 79 Arthur Suzuki 2024-04-11 12:41:48 UTC
Created attachment 164682 [details] [review]
Bug 25408: (QA follow-up) using perltidy to resolve complaining QA scripts
Comment 80 Arthur Suzuki 2024-04-11 12:48:19 UTC
Created attachment 164684 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy
Comment 81 Arthur Suzuki 2024-04-11 12:48:22 UTC
Created attachment 164685 [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 item-level hold on staff interface
3 - Hold is still allowed (rule do not apply to staff)
4 - Attempt to place item-level hold from opac or ilsdi
5 - Hold is not allowed (rule apply from opac or ilsdi)
6 - Change rule to 'force'
7 - Item-level hold can still be placed from the staff interface
8 - Record-level hold cannot be placed from opac or ilsdi
Comment 82 Arthur Suzuki 2024-04-11 12:48:28 UTC
Created attachment 164686 [details] [review]
Bug 25408: (QA follow-up) using perltidy to resolve complaining QA scripts
Comment 83 Arthur Suzuki 2024-04-11 12:51:12 UTC
Hello Guys,
I've made a rework of that one so I thought it would be better to have it back to Need-Signoff status.
So, it's been rebased, tests provided. I made a separate patch for perltidy because it introduce a lot of cosmetic changes (but no logic changes, which is in the previous patch).
Hopefully it will pass the signoff and qa this time :)
Best,
Arthur
Comment 84 Arthur Suzuki 2024-04-11 13:33:30 UTC
Created attachment 164693 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy
Comment 85 Arthur Suzuki 2024-04-11 13:33:34 UTC
Created attachment 164694 [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 item-level hold on staff interface
3 - Hold is still allowed (rule do not apply to staff)
4 - Attempt to place item-level hold from opac or ilsdi
5 - Hold is not allowed (rule apply from opac or ilsdi)
6 - Change rule to 'force'
7 - Item-level hold can still be placed from the staff interface
8 - Record-level hold cannot be placed from opac or ilsdi
Comment 86 pierre.genty 2024-04-12 09:57:05 UTC
Patch tested for ILS-DI with two rules : 
- books : OPAC item level holds = Allow
- DVDs : OPAC item level holds = Force

Before applying patch :
- book reserve (biblionumber = 7) : /cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=11&bib_id=7&request_location=127.0.0.1 = hold allowed => OK
- DVD reserve (biblionumber = 296) : /cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=11&bib_id=296&request_location=127.0.0.1 = hold allowed => KO

After applying patch : 
- book reserve (biblionumber = 7) : /cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=11&bib_id=7&request_location=127.0.0.1 = hold allowed => OK
- DVD reserve (biblionumber = 296) : /cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=11&bib_id=296&request_location=127.0.0.1 = "recordHoldNotAllowed" => OK

Patch seems to work well with ILS-DI !
Comment 87 Pedro Amorim 2024-04-15 11:44:21 UTC
Created attachment 164878 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 88 Pedro Amorim 2024-04-15 11:44:24 UTC
Created attachment 164879 [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 item-level hold on staff interface
3 - Hold is still allowed (rule do not apply to staff)
4 - Attempt to place item-level hold from opac or ilsdi
5 - Hold is not allowed (rule apply from opac or ilsdi)
6 - Change rule to 'force'
7 - Item-level hold can still be placed from the staff interface
8 - Record-level hold cannot be placed from opac or ilsdi

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 89 Pedro Amorim 2024-04-15 11:45:58 UTC
This looks like it's a good change i.e. moving the check logic to a lower layer.
The test plan works as described and all tests pass.
I can't find a bug or issue this introduces, but others more experienced in the holds logic than me may disagree.

Also tested the following ILSDI endpoint:
/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=11&bib_id=76&item_id=167

It respects the opacitemholds config as expected, I believe, as ILSDI is an OPAC interface and context is 'opac' when using ILSDI

Some thoughts related to code specifically:
- Personally I'd prefer this to be unfolded in more commits to ease the review process, with each having a short explanation of why the changes are required:
Something like this, (only a suggestion):
-- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved
-- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC
-- Update warning labels in opac-reserve.tt accordingly.
- A new unnecessary empty new line is added in opac-reserve.pl
- Some lines that have not been changed have been tidied and included
- Unnecessary changes are made e.g. $patron->borrowernumber to $patron->id (correct me if I'm wrong here, please!)
Comment 90 Victor Grousset/tuxayo 2024-04-16 04:25:38 UTC
> - Unnecessary changes are made e.g. $patron->borrowernumber
> to $patron->id (correct me if I'm wrong here, please!)

Indeed, in bunch of lines that are already changed, it's a good opportunity to enforce terminology without adding more conflict possibilities. But here it's in lines that are not changed, so it's adding new conflicts possibilities with other submitted patches and when backporting.

> - Some lines that have not been changed have been tidied and included

Yes, also that.

> - Personally I'd prefer this to be unfolded in more commits
> to ease the review process, with each having a short
> explanation of why the changes are required:

At least having the full commit message list these points. Not sure about splitting the commit after the work is done. Maybe I missed the main point, but in this case the 3 changes look 95% likely to have been done together and not as iteration with a different logic on each step.
That's a nice thing to keep in mind for other reviews or my own patches, thanks :)
Comment 91 Arthur Suzuki 2024-04-18 14:16:11 UTC
Created attachment 165111 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy
Comment 92 Arthur Suzuki 2024-04-18 14:16:15 UTC
Created attachment 165112 [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

What this patch does :
-- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved
-- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC
-- Update warning labels in opac-reserve.tt accordingly.

To test:
0 - Apply patches
1 - Set rule to "Don't allow" item specific holds on opac
2 - Attempt to place item-level hold on staff interface
3 - Hold is still allowed (rule do not apply to staff)
4 - Attempt to place item-level hold from opac or ilsdi
5 - Hold is not allowed (rule apply from opac or ilsdi)
6 - Change rule to 'force'
7 - Item-level hold can still be placed from the staff interface
8 - Record-level hold cannot be placed from opac or ilsdi
Comment 93 Arthur Suzuki 2024-04-18 14:21:48 UTC
(In reply to Victor Grousset/tuxayo from comment #90)
> > - Unnecessary changes are made e.g. $patron->borrowernumber
> > to $patron->id (correct me if I'm wrong here, please!)
> 
> Indeed, in bunch of lines that are already changed, it's a good opportunity
> to enforce terminology without adding more conflict possibilities. But here
> it's in lines that are not changed, so it's adding new conflicts
> possibilities with other submitted patches and when backporting.

I've fixed this in the latest version of the patch :)
 
> > - Some lines that have not been changed have been tidied and included
> 
> Yes, also that.

Also fixed :)

> > - Personally I'd prefer this to be unfolded in more commits
> > to ease the review process, with each having a short
> > explanation of why the changes are required:
> 
> At least having the full commit message list these points. Not sure about
> splitting the commit after the work is done. Maybe I missed the main point,
> but in this case the 3 changes look 95% likely to have been done together
> and not as iteration with a different logic on each step.
> That's a nice thing to keep in mind for other reviews or my own patches,
> thanks :)

I've added the relevant points to the commit message, next time I will try to better split the commits.
I don't know if the patch can move to "Passed QA" or if further work is needed?
Thanks for the review anyway :)
Best,
Arthur
Comment 94 Pedro Amorim 2024-04-18 14:38:03 UTC
This looks great. You've addressed the reasons Victor FQA so it should be set back to Signed-off so that he can look again, I believe.
Comment 95 Arthur Suzuki 2024-04-18 16:48:08 UTC
(In reply to Pedro Amorim from comment #94)
> This looks great. You've addressed the reasons Victor FQA so it should be
> set back to Signed-off so that he can look again, I believe.

Great, hope that will pass QA soon :)
Comment 96 Victor Grousset/tuxayo 2024-04-19 02:50:51 UTC
> it should be set back to Signed-off

Indeed, there are no changes to the code that would invalidate the signoff. (minor ones wouldn't invalidate it, but here, it's even zero actual changes :) )

Thanks Pedro for giving a look and thanks Arthur for the fixes :)
Will look back asap see if I understand enough the code to QA.
Comment 97 Victor Grousset/tuxayo 2024-04-24 00:04:32 UTC Comment hidden (obsolete)
Comment 98 Victor Grousset/tuxayo 2024-04-24 00:04:36 UTC Comment hidden (obsolete)
Comment 99 Victor Grousset/tuxayo 2024-04-24 01:12:51 UTC
Created attachment 165444 [details] [review]
Bug 25408: Added unit tests for opacitemholds policy

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 100 Victor Grousset/tuxayo 2024-04-24 01:12:54 UTC
Created attachment 165445 [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

What this patch does :
-- Move get_opacitemholds_policy check into Reserves.pm::CanBookBeReserved
-- Update CanItemBeReserved to return recordHoldsOnly status if interface is OPAC
-- Update warning labels in opac-reserve.tt accordingly.

To test:
0 - Apply patches
1 - Set rule to "Don't allow" item specific holds on opac
2 - Attempt to place item-level hold on staff interface
3 - Hold is still allowed (rule do not apply to staff)
4 - Attempt to place item-level hold from opac or ilsdi
5 - Hold is not allowed (rule apply from opac or ilsdi)
6 - Change rule to 'force'
7 - Item-level hold can still be placed from the staff interface
8 - Record-level hold cannot be placed from opac or ilsdi

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 101 Victor Grousset/tuxayo 2024-04-24 03:14:42 UTC
It works :)
(readded Pedro's SO line that was lost in Comment 91 changes)

It functionally makes sense, found nothing out of order after some exploratory testing on opac, staff and ILS-DI. Thanks David for the earlier testing and notes :)
REST API doesn't seem to have a way to place holds so nothing to see there.

Like Pedro, I'm not confident in understanding alone enough the completeness and correctness of the many changes in C4/Reserves.pm and opac-reserve.pl to fully QA.
But let's try going with this since we both found no obvious issue in the code. The RM is free to request more review.

Works, makes sense, QA script happy, code looks good, passing QA :)

--

@Arthur: For releases notes, it would be clearer if the name of the ticket was something like:
«"OPAC item level holds" circulation rules are not followed by ILS-DI»
Do you confirm that's the fix in a nutshell?
Comment 102 Katrin Fischer 2024-04-26 11:02:19 UTC
I am sure everyone here has tested well, but I'd feel a little bit more confident if we could maybe get Kyle or another hold expert to look at this.
Comment 103 Katrin Fischer 2024-04-26 11:08:57 UTC
I don't have a status for "request additional review". Hmpf. :)
Comment 104 Nick Clemens (kidclamp) 2024-04-26 11:28:41 UTC
I am still unsure about one of the lines here:

In CanBookBeReserved, during the item loop:
    return { status => 'recordHoldNotAllowed' } if $opacitemholds eq 'F';


If one of the items is type 'new' and doesn't allow record level holds, but another is 'book' and does allow it - we will return 'recordHoldNotAllowed' if we hit the 'new' item first.

You also call 'get_opacitemholds_policy' in CanBookBeReserved and also add a call to get_effective_rules in CanItemBeReserved - these should be getting the same rule - why do we need to check at both levels? The check in CanItemBeReseerved should handle returning the necessary status
Comment 105 Nick Clemens (kidclamp) 2024-04-26 13:46:30 UTC
see comment 20 and comment 25 - I think the problem with multiple item types on a single bib is still the issue
Comment 106 Victor Grousset/tuxayo 2024-04-26 19:36:29 UTC
Nice catch, indeed that part of comment 20 wasn't addressed.