Bug 20985 - CanBookBeReserved and CanItemBeReserved should check 'On shelf holds allowed' policy
Summary: CanBookBeReserved and CanItemBeReserved should check 'On shelf holds allowed'...
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:
: 19294 (view as bug list)
Depends on:
Blocks: 30845
  Show dependency treegraph
 
Reported: 2018-06-22 21:06 UTC by Nick Clemens
Modified: 2023-10-11 07:15 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This bug fixes the "On shelf holds allowed" rules so that they work correctly when using ILS-DI to place holds. The expected behaviour for "On shelf holds allowed" settings for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules) are: - If set to "Yes", patrons can place holds on items currently checked in. - If set to "If any unavailable", patrons can only place holds on items that are not unavailable. - If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable. RMNOTE: Reverted!
Version(s) released in:


Attachments
Bug 20985 : Add OnShelfHoldsAllowed checks to CanItemBeReserved (1.81 KB, patch)
2019-03-22 07:08 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985 : Add OnShelfHoldsAllowed checks to CanItemBeReserved (1.89 KB, patch)
2019-05-13 23:11 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (4.83 KB, patch)
2020-06-17 23:03 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.53 KB, patch)
2020-09-15 07:29 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.90 KB, patch)
2020-10-19 07:25 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 25898: Fix indirect object notation in debian/list-deps (1.11 KB, patch)
2020-10-30 10:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.84 KB, patch)
2020-11-17 13:31 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (5.59 KB, patch)
2020-11-17 13:31 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.78 KB, patch)
2021-01-04 14:43 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.57 KB, patch)
2021-01-04 14:43 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.78 KB, patch)
2021-01-04 14:47 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.57 KB, patch)
2021-01-04 14:48 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.83 KB, patch)
2021-01-24 23:43 UTC, David Nind
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.61 KB, patch)
2021-01-24 23:44 UTC, David Nind
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.67 KB, patch)
2021-05-18 09:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.62 KB, patch)
2021-05-18 09:33 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.53 KB, patch)
2021-05-18 09:53 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.82 KB, patch)
2021-05-19 00:04 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.53 KB, patch)
2021-05-19 13:32 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.87 KB, patch)
2021-05-19 16:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.60 KB, patch)
2021-05-19 16:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 20985: Unit tests (3.94 KB, patch)
2021-05-28 14:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (7.65 KB, patch)
2021-05-28 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved (26.90 KB, patch)
2023-08-23 14:40 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-06-22 21:06:37 UTC
Via ILSDI we use the routines mentioned above to determine holdability, however, neither of these routines check on shelf hold policies
Comment 1 Arthur Suzuki 2019-03-21 08:48:53 UTC
Isn't this a duplicate of 19294 since HoldTitle and HoldItem calls CanBookBeReserved and CanItemBeReserved to check if reference can be reserved?
Or does it solves 19294? -> I guess so.
Comment 2 Arthur Suzuki 2019-03-22 07:08:17 UTC
Created attachment 86874 [details] [review]
Bug 20985 : Add OnShelfHoldsAllowed checks to CanItemBeReserved

Test plan :
1 / Check that default circulation have OnShelfHoldsAllowed to Yes
2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed.
3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table).
Comment 3 Hayley Pelham 2019-04-17 03:24:12 UTC
This patch results in a runtime error. Here are the steps I followed which led to the error.

- Checked default circulation rule - OnShelfHoldsAllowed is set to yes
- Clicked Place hold on a biblio with one available item
- Entered card number - error!

Error trace: 

Can't call method "effective_itemtype" on an undefined value at /home/vagrant/kohaclone/C4/Reserves.pm line 1181

1.    in C4::Reserves::IsAvailableForItemLevelRequest at /home/vagrant/kohaclone/C4/Reserves.pm line 1181

     1178:     #         consolidated
     1179:     my $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
     1180:     my $item_object = Koha::Items->find( $item->{itemnumber } );
     1181:     my $itemtype = $item_object->effective_itemtype;
     1182:     my $notforloan_per_itemtype
     1183:       = $dbh->selectrow_array("SELECT notforloan FROM itemtypes WHERE itemtype = ?",
     1184:                               undef, $itemtype);

2.    in C4::Reserves::CanItemBeReserved at /home/vagrant/kohaclone/C4/Reserves.pm line 332

      329:         && !C4::Context->preference('AllowHoldsOnDamagedItems') );
      330: 
      331:     # Check for item on shelves and OnShelfHoldsAllowed
      332:     return { status => 'onShelfHoldsNotAllowed' }
      333:       unless ( IsAvailableForItemLevelRequest($item,$borrower) );
      334: 
      335:     # Check for the age restriction

3.    in C4::Reserves::CanBookBeReserved at /home/vagrant/kohaclone/C4/Reserves.pm line 286

      283: 
      284:     my $canReserve = { status => '' };
      285:     foreach my $itemnumber (@itemnumbers) {
      286:         $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode );
      287:         return { status => 'OK' } if $canReserve->{status} eq 'OK';
      288:     }
      289:     return $canReserve;

Setting to Failed QA
Comment 4 Nick Clemens 2019-04-22 13:08:24 UTC
*** Bug 19294 has been marked as a duplicate of this bug. ***
Comment 5 Arthur Suzuki 2019-05-13 23:11:10 UTC
Created attachment 89723 [details] [review]
Bug 20985 : Add OnShelfHoldsAllowed checks to CanItemBeReserved

Test plan :
1 / Check that default circulation have OnShelfHoldsAllowed to Yes
2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed.
3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table).
Comment 6 Arthur Suzuki 2019-05-13 23:13:09 UTC
(In reply to Hayley Mapley from comment #3)
> This patch results in a runtime error. Here are the steps I followed which
> led to the error.
> 
> - Checked default circulation rule - OnShelfHoldsAllowed is set to yes
> - Clicked Place hold on a biblio with one available item
> - Entered card number - error!
> 
> Error trace: 
> 
> Can't call method "effective_itemtype" on an undefined value at
> /home/vagrant/kohaclone/C4/Reserves.pm line 1181
> 
> 1.    in C4::Reserves::IsAvailableForItemLevelRequest at
> /home/vagrant/kohaclone/C4/Reserves.pm line 1181
> 
>      1178:     #         consolidated
>      1179:     my $patron = Koha::Patrons->find( $borrower->{borrowernumber}
> );
>      1180:     my $item_object = Koha::Items->find( $item->{itemnumber } );
>      1181:     my $itemtype = $item_object->effective_itemtype;
>      1182:     my $notforloan_per_itemtype
>      1183:       = $dbh->selectrow_array("SELECT notforloan FROM itemtypes
> WHERE itemtype = ?",
>      1184:                               undef, $itemtype);
> 
> 2.    in C4::Reserves::CanItemBeReserved at
> /home/vagrant/kohaclone/C4/Reserves.pm line 332
> 
>       329:         && !C4::Context->preference('AllowHoldsOnDamagedItems') );
>       330: 
>       331:     # Check for item on shelves and OnShelfHoldsAllowed
>       332:     return { status => 'onShelfHoldsNotAllowed' }
>       333:       unless ( IsAvailableForItemLevelRequest($item,$borrower) );
>       334: 
>       335:     # Check for the age restriction
> 
> 3.    in C4::Reserves::CanBookBeReserved at
> /home/vagrant/kohaclone/C4/Reserves.pm line 286
> 
>       283: 
>       284:     my $canReserve = { status => '' };
>       285:     foreach my $itemnumber (@itemnumbers) {
>       286:         $canReserve = CanItemBeReserved( $borrowernumber,
> $itemnumber, $pickup_branchcode );
>       287:         return { status => 'OK' } if $canReserve->{status} eq
> 'OK';
>       288:     }
>       289:     return $canReserve;
> 
> Setting to Failed QA

Hi Hayley,
Thanks for testing, updated my patch in consequence :)
Comment 7 Hayley Pelham 2019-05-14 04:34:43 UTC
Hi Arthur,

Looks good, only the description is not displayed in the Hold column of the item table when the 'On shelf holds allowed' preference is set to 'If all unavailable'. It is shown though when the preference is set to 'If any unavailable'
Comment 8 Owen Leonard 2019-08-06 16:33:17 UTC
Marking "Failed QA" based on comment 7
Comment 9 Arthur Suzuki 2020-06-17 23:03:02 UTC
Created attachment 105991 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

Test plan :
1 / Check that default circulation have OnShelfHoldsAllowed to Yes
2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed.
3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table).
Comment 10 Arthur Suzuki 2020-09-15 07:29:26 UTC
Created attachment 110095 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

Test plan :
1 / Check that default circulation have OnShelfHoldsAllowed to Yes
2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed.
3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table).
Comment 11 David Nind 2020-09-16 11:28:18 UTC
Sorry Arthur, I found this one very confusing to test as I am not very familiar with setting circulation rules and the "On shelf holds allowed". I wasn't sure what this bug fixes or what we are testing for (no explanation before the test). So probably better if someone else more familiar tests this.

1. At first I thought these were system preferences, but these are the Perl code names variable/routine used to check the rules.

2. Are you supposed to test with the staff interface or using ILS-DI, or both? I only tested using the staff interface.

3. I wasn't really sure what was different between before the patch was applied and after - the only behaviour change I could see was the text appearing in the hold column (red X vs red "X onShelfHoldsNotAllowed"), which didn't seem to be useful to me as a "normal" user.

My notes
--------

Expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules): 
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable. 
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record  are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

Testing using staff interface:
1. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
2. Set "On shelf holds allowed" to "Yes" for all libraries/patron categories/item types.
3. Place a hold on a record with a single item (where the item is available for loan) 
  . I placed a hold for an item and patron both in/belonging to the Centerville library.
  . Information column in the item table displays "Not on hold".
4. The hold is placed.
5. Cancel the hold.
6. Set "On shelf holds allowed" to either "If any unavailable" or "If all unavailable" for all patrons/patron categories/item types.
7. The hold is not placed:
   . Message is "Cannot place hold. No items are available to be placed on hold."
   . Item table: Hold column displays in red "X onShelfHoldsNotAllowed" and the information column displays "Not on hold".
8. Run the tests prove t/db_dependent/Reserves.t - these should pass

Tests fail
----------

Also, the tests fail 8-( :

prove t/db_dependent/Reserves.t
t/db_dependent/Reserves.t .. 24/63 Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 597.
Use of uninitialized value in string eq at /kohadevbox/koha/C4/Reserves.pm line 597.
Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 440.
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
Use of uninitialized value $opacitemholds in string eq at /kohadevbox/koha/C4/Reserves.pm line 440.
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
t/db_dependent/Reserves.t .. 58/63 Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.

        #   Failed test 'record-level holds should not be possible with opacitemholds set to "Force"'
        #   at t/db_dependent/Reserves.t line 1012.
        #          got: 'OK'
        #     expected: 'RecordHoldNotAllowed'
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 451.
        # Looks like you failed 1 test of 6.

    #   Failed test 'test opacitemholds rules'
    #   at t/db_dependent/Reserves.t line 1080.
    # Looks like you planned 2 tests but ran 3.
    # Looks like you failed 1 test of 3 run.
t/db_dependent/Reserves.t .. 61/63 
#   Failed test 'reserves.item_level_hold'
#   at t/db_dependent/Reserves.t line 1081.
t/db_dependent/Reserves.t .. 63/63 # Looks like you failed 1 test of 63.
t/db_dependent/Reserves.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/63 subtests 

Test Summary Report
-------------------
t/db_dependent/Reserves.t (Wstat: 256 Tests: 63 Failed: 1)
  Failed test:  61
  Non-zero exit status: 1
Files=1, Tests=63,  6 wallclock secs ( 0.04 usr  0.00 sys +  5.40 cusr  0.71 csys =  6.15 CPU)
Result: FAIL
Comment 12 Fridolin Somers 2020-10-06 14:04:05 UTC
Hi,
why this addition :
+    $item = Koha::Items->find( $itemnumber );

$item object is already fetched at beginning of method.

If it is to refresh the ORM object, I think you must use ->get_from_storage()
Comment 13 Arthur Suzuki 2020-10-19 07:25:13 UTC
Created attachment 111954 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

Via ILSDI we use the routines mentioned above to determine holdability, however, neither of these routines check on shelf hold policies

Test plan :
1 / Check that default circulation have OnShelfHoldsAllowed to Yes
2 / Place a hold on a book with a single item, item being available for loan, verify that hold can be placed.
3 / Set OnShelfHoldsAllowed to any other value, verify that hold cannot be placed for reason "onShelfHoldsNotAllowed" (shown in the item table).
Comment 14 David Nind 2020-10-30 10:59:33 UTC Comment hidden (obsolete)
Comment 15 David Nind 2020-10-30 11:00:28 UTC Comment hidden (obsolete)
Comment 16 Arthur Suzuki 2020-11-17 13:31:39 UTC
Created attachment 113730 [details] [review]
Bug 20985: Unit tests

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 17 Arthur Suzuki 2020-11-17 13:31:54 UTC
Created attachment 113731 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

Expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record  are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

Testing using staff interface:
1. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
2. Set "On shelf holds allowed" to "Yes" for all libraries/patron categories/item types.
3. Place a hold on a record with a single item (where the item is available for loan)
  . I placed a hold for an item and patron both in/belonging to the Centerville library.
  . Information column in the item table displays "Not on hold".
4. The hold is placed.
5. Cancel the hold.
6. Set "On shelf holds allowed" to either "If any unavailable" or "If all unavailable" for all patrons/patron categories/item types.
7. The hold is not placed:
   . Message is "Cannot place hold. No items are available to be placed on hold."
   . Item table: Hold column displays in red "X onShelfHoldsNotAllowed" and the information column displays "Not on hold".
8. Run the tests prove t/db_dependent/Reserves.t - these should pass

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 18 David Nind 2020-12-09 18:59:14 UTC
Hi Arthur. 

Apologies for taking so long to get back to this (and work out what this was about!).

I've read the description properly now and bug 19294, so the aim is to make sure that if using ILS-DI the "On shelf holds allowed" options for circulation and fines rules are correctly applied - at the moment they aren't.

Here is an updated test plan - I've worked through it and everything works as expected. (I'm not sure I've made the test plan better, but it covers both the staff interface and using ILS-DI.)

Suggested updated test plan
===========================

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable" 
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available, 
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item, 
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Changes
=======

Tests fail
~~~~~~~~~~

The number of tests need updating from 66 to 67 (the tests pass if I update this myself):

ok 67 - MergeHolds
# Looks like you planned 66 tests but ran 67.
Dubious, test returned 255 (wstat 65280, 0xff00)
All 66 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Reserves.t (Wstat: 65280 Tests: 67 Failed: 1)
  Failed test:  67
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 66 tests but ran 67.
Files=1, Tests=67,  8 wallclock secs ( 0.05 usr  0.01 sys +  6.65 cusr  0.93 csys =  7.64 CPU)
Result: FAIL

Wording change suggestion
~~~~~~~~~~~~~~~~~~~~~~~~~

I didn't notice any change in behaviour except for Step 4 with the patch applied, red "X onShelfHoldsNotAllowed".

I would prefer to see a human readable message for the staff interface, rather than the code "onShelfHoldsNotAllowed", maybe "Holds not allowed for items on shelves" or "Holds not allowed for items available".

Happy to sign off once tests fixed.

David
Comment 19 Arthur Suzuki 2021-01-04 14:43:13 UTC
Created attachment 114801 [details] [review]
Bug 20985: Unit tests
Comment 20 Arthur Suzuki 2021-01-04 14:43:26 UTC
Created attachment 114802 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.
Comment 21 Arthur Suzuki 2021-01-04 14:47:55 UTC
Created attachment 114804 [details] [review]
Bug 20985: Unit tests
Comment 22 Arthur Suzuki 2021-01-04 14:48:27 UTC
Created attachment 114805 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.
Comment 23 Arthur Suzuki 2021-01-04 14:49:14 UTC
applied recommended changes and rebased on current master
Comment 24 David Nind 2021-01-24 23:43:56 UTC
Created attachment 115732 [details] [review]
Bug 20985: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2021-01-24 23:44:00 UTC
Created attachment 115733 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Joonas Kylmälä 2021-03-22 11:46:26 UTC
The patch doesn't apply. The patch seems to also introduce some unused code, e.g.

> +    my $on_shelf_holds = 0;   # Default to "if any unavailable"

> +    my $opacitemholds = 'Y';  # Itemlevel holds default to allowed
Comment 27 Arthur Suzuki 2021-05-18 09:33:53 UTC
Created attachment 121100 [details] [review]
Bug 20985: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Arthur Suzuki 2021-05-18 09:33:57 UTC
Created attachment 121101 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 Arthur Suzuki 2021-05-18 09:53:39 UTC
Created attachment 121102 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>
Comment 30 Arthur Suzuki 2021-05-18 09:54:43 UTC
Hi Joubu,
I've re-applied on top of master + removed opacitemholds variable.
on_shelf_holds variable is used.
Arthur
Comment 31 David Nind 2021-05-18 20:40:24 UTC
Hi Arthur.

I quickly ran through the test plan.

Everything works as expected, except the tests fail. 

If I manually update the number of tests to 68, everything passes.

David
Comment 32 Arthur Suzuki 2021-05-19 00:04:45 UTC
Created attachment 121139 [details] [review]
Bug 20985: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 Arthur Suzuki 2021-05-19 00:06:49 UTC
Hi David,
just pushed a new version of the test patch with number of tests updated.
Arthur
Comment 34 David Nind 2021-05-19 09:58:29 UTC
Path no longer applies 8-(...

error: sha1 information is lacking or useless (t/db_dependent/Reserves.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved
Comment 35 Arthur Suzuki 2021-05-19 13:32:09 UTC
Created attachment 121176 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Arthur Suzuki 2021-05-19 13:56:54 UTC
Hi David,
Can you retry? seems the patches have to be uploaded in the proper order to be applied the same...
Should work now.
Comment 37 David Nind 2021-05-19 16:14:22 UTC
> Should work now.

Everything works now - sign off on the way..
Comment 38 David Nind 2021-05-19 16:15:14 UTC
Created attachment 121185 [details] [review]
Bug 20985: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2021-05-19 16:15:18 UTC
Created attachment 121186 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 Arthur Suzuki 2021-05-20 09:20:42 UTC
Thx!
Comment 41 Kyle M Hall 2021-05-28 14:17:58 UTC
Created attachment 121485 [details] [review]
Bug 20985: Unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Kyle M Hall 2021-05-28 14:18:57 UTC
Created attachment 121486 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.

Test plan:

1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
   . "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
   . "Yes"
      ==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
   . "If any unavailable" and "If all unavailable"
      ==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
   . Query to place a hold using ILS-DI on a title that have all its items available,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
   . Query to place a hold using ILS-DI on an available item,
     example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
     ==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
   . For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
   . For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
   . Tests: should still pass.
9. Sign off.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Jonathan Druart 2021-06-15 14:28:34 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 44 Jonathan Druart 2021-06-16 08:16:48 UTC
There are tons of tests failing.

t/db_dependent/Holds.t, t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t, t/db_dependent/api/v1/holds.t, t/db_dependent/ILSDI_Services.t and t/db_dependent/Reserves/MultiplePerRecord.t.

Please fix ASAP or I will revert.
Comment 45 Arthur Suzuki 2021-06-16 09:52:47 UTC
Joubu, can you revert? otherwise I cannot apply to master (conflicts) + if solving the problem in master i'm afraid to run into issues applying the patch afterward (fake ancestor).
Thanks
Comment 46 Jonathan Druart 2021-06-16 12:45:43 UTC
Patches reverted.
Comment 47 Fridolin Somers 2021-07-02 21:23:01 UTC
+    # Check for item on shelves and OnShelfHoldsAllowed
+    return { status => 'onShelfHoldsNotAllowed' }
+    unless IsAvailableForItemLevelRequest($item, $patron, $pickup_branchcode,1);

Strange : IsAvailableForItemLevelRequest() is not the feature of on shelf holds allowed, it means hold can be on item level or forced to biblio level o_O
Comment 48 David Cook 2021-08-26 23:56:30 UTC
If I recall correctly, consideration of hold rules is a mess across the board. I keep thinking that we need to do something that covers them all otherwise all the changes will always just be Band-Aids.
Comment 49 Fridolin Somers 2021-11-22 07:57:33 UTC
(In reply to Fridolin Somers from comment #47)
> +    # Check for item on shelves and OnShelfHoldsAllowed
> +    return { status => 'onShelfHoldsNotAllowed' }
> +    unless IsAvailableForItemLevelRequest($item, $patron,
> $pickup_branchcode,1);
> 
> Strange : IsAvailableForItemLevelRequest() is not the feature of on shelf
> holds allowed, it means hold can be on item level or forced to biblio level
> o_O

Ah this method is calling :
Koha::CirculationRules->get_onshelfholds_policy()
Comment 50 Julian Maurice 2023-08-23 14:40:22 UTC
Created attachment 154711 [details] [review]
Bug 20985: Add OnShelfHoldsAllowed checks to CanItemBeReserved

The expected behaviour for "On shelf holds allowed" setting for the
circulation rules (Koha administration > Patrons and circulation >
Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise
  unavailable.
- If set to "Yes", patrons can place holds on items currently checked
  in.
- If set to "If any unavailable", patrons can only place holds on items
  that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items
  where *all* items on the record are unavailable.
(Adapted from
https://bywatersolutions.com/education/preparing-for-library-closures)

These rules should also work when using ILS-DI, but currently they
don't. This bug makes sure that the "On shelf holds allowed" rules work
correctly when using ILS-DI to place holds.

Test plan
---------

1. Enable ILS-DI (set the ILS-DI system preference to Enable).

2. Go to Koha administration > Patrons and circulation > Circulation and
   fines rules.

3. Work through steps 4-5 for each of the settings for "On shelf holds
   allowed" for all libraries/patron categories/item types:
   "Yes", "If any unavailable", and "If all unavailable"

4. Staff interface - place a hold on a record with items available for
   loan, the rules should work as expected before and after the patch is
   applied:
     "Yes":
       information column in the item table displays "Not on hold", the
       hold is placed, cancel the hold

     "If any unavailable" and "If all unavailable":
       the hold is not placed, message is "Cannot place hold. No items
       are available to be placed on hold.", red "X" in the hold column
       and the information column displays "Not on hold"

5. ILS-DI - place a hold on a record with items available for loan
   (note: without the patch, holds can be placed):
   - Query to place a hold using ILS-DI on a title that have all its
     items available, example query:

     http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1

     Without the patch the hold is placed but it shouldn't be allowed,
     cancel the hold

   - Query to place a hold using ILS-DI on an available item, example
     query:

     http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)

     Without the patch the hold is placed but it shouldn't be allowed,
     cancel the hold

6. Run the tests in t/db_dependent/Reserves.t - these should pass.

7. Apply the patch, flush the cache and restart Koha (if using
   koha-testing-docker, run flush_memcached and restart_all)

8. Run through steps 3-6 again, and note the changes when "If any
   unavailable" and "If all unavailable" options are used:
     For the staff interface:
       there should be no change in behavour and should work as
       expected, for the red "X" in the items table additional text is
       added "onShelfHoldsNotAllowed".

     For ILS-DI:
       these should now work as expected, with holds not placed, and
       this message in the results returned
       <code>onShelfHoldsNotAllowed</code> (check to confirm no holds
       place for either the patron or the item)

     Tests:
       should still pass, in particular the following tests:
       - t/db_dependent/Reserves.t
       - t/db_dependent/Holds.t
       - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
       - t/db_dependent/api/v1/holds.t
       - t/db_dependent/ILSDI_Services.t
       - t/db_dependent/Reserves/MultiplePerRecord.t
       - t/db_dependent/Reserves/IsOnShelfHoldsPolicySatisfied.t (new
         test file)
Comment 51 Julian Maurice 2023-08-23 14:45:53 UTC
Patch rebased and reworked. The original patch had a problem that caused infinite recursion (CanItemBeReserved called IsAvailableForItemLevelRequest which called ItemsAnyAvailableAndNotRestricted which called CanItemBeReserved)

Replaced the call to IsAvailableForItemLevelRequest by a call to a new subroutine IsOnShelfHoldsPolicySatisfied which does only what its name suggests

All tests mentioned in comment 44 should pass.

I removed the Signed-off-by lines because the patch changed significantly, but the test plan did not change. Please test again
Comment 52 Nick Clemens 2023-08-23 15:56:22 UTC
I believe other calls to CanItemBeRserved should be adjusted or taken into consideration

Both reserve/request.pl and opac/opac-reserve.pl currently call CanItemBeReserved and then call IsAvailableForItemLevelRequest

After this code, if I understand correctly, IsOnShelfHoldsPolicySatisfied will now be called twice for each item - once from CanItem.. and once from IsAvailable..

See bug 32702 - we really need to consolidate these two routines, or if a check is moved into CanItemBeReserved, it should be removed from the other?
Comment 53 Fridolin Somers 2023-10-10 18:07:54 UTC
+    unless ( $params->{ignore_onshelfholds_policy} ) {
+        unless ( IsOnShelfHoldsPolicySatisfied( { item => $item, patron => $patron } ) ) {
+            return _cache { status => 'onShelfHoldsNotAllowed' };
+        }
+    }

Why call _cache() ? I cant find this method
Comment 54 Julian Maurice 2023-10-11 07:15:50 UTC
(In reply to Fridolin Somers from comment #53)
> Why call _cache() ? I cant find this method
That's how it's done on master. It's in C4/Reserves.pm