Bug 23172 - Holds queue should check patron category hold policies when mapping items to pending reserves
Summary: Holds queue should check patron category hold policies when mapping items to ...
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-21 04:25 UTC by Alex Buckley
Modified: 2023-08-01 23:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
The hold queue should check patron category hold policies when mapping items to pending title-level and item-level reserves. Previously the hold queue only checked the hold policies regardless of patron categories.
Version(s) released in:


Attachments
Bug 23172: When returning items check issuingrules (4.67 KB, patch)
2019-06-21 04:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: Unit tests (9.23 KB, patch)
2019-07-01 07:53 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests (13.06 KB, patch)
2019-07-04 23:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: When returning items check issuingrules (4.33 KB, patch)
2019-08-16 03:50 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 23172: Unit tests (9.31 KB, patch)
2019-08-16 03:50 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests (13.10 KB, patch)
2019-08-16 03:51 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 23172: When returning items check issuingrules (4.35 KB, patch)
2020-03-31 02:56 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23172: Unit tests (9.30 KB, patch)
2020-03-31 02:56 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests (14.55 KB, patch)
2020-03-31 02:56 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23172: (follow-up) Fixing test (28.63 KB, patch)
2020-04-01 03:47 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 23172: When returning items check issuingrules (4.32 KB, patch)
2021-03-29 05:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: Unit tests (9.26 KB, patch)
2021-03-29 05:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests (14.55 KB, patch)
2021-03-29 05:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: (follow-up) Fixing test (29.25 KB, patch)
2021-03-29 05:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: The holds queue should check if a patron category is eligible to reserve an itemtype (6.75 KB, patch)
2021-12-21 04:33 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 23172: Unit test for holds queue (18.58 KB, patch)
2021-12-22 03:21 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2019-06-21 04:25:21 UTC
Currently, when an item is returned the values in the issuingrules are not checked when determining whether to allocate that item to a bib-level hold, based on the item's itemtype and the requesting patrons patron category. 

This means hold rules defined in the issuingrules table are not being respected when allocating an item to a bib-level hold. 

For example, A biblio 'Fish' has items with two different item types 'BK' and 'CR'. 

In the circulation rules the patron category 'B' (Board) with the item type 'BK' has a reservesallowed value of 0 and a holds_per_record value of 0. 

Patron A (who has a patron category of 'B') can place a bib-level hold on 'Fish' biblio as there is an item with itemtype of 'CR' which they are permitted to reserve. 

If the 'BK' item is returned first then that will be allocated to fill patron A's bib-level hold, even though in the issuingrules 'B' patrons cannot place a hold on 'BK' items.
Comment 1 Alex Buckley 2019-06-21 04:42:28 UTC
Created attachment 90868 [details] [review]
Bug 23172: When returning items check issuingrules

When iterating over every reserve on a biblio when it is being returned
retrieve the issuingrule reserveallowed and holds_per_record values for
the reserves requester borrower category and returned item type.

If the issuingrule defines that reservesallowed or holds_per_record is
0 then skip allocating that item to the current bib-level reserve.

Test plan:
1. Find/make a biblio record with two items: one with item type of 'BK'
(book) and the other with the itemtype of 'CR' (Continuing Resource)

2. In Administration > Circulation and fine rules define 2 new rules:

Patron category: 'B' (Board)/ item type: 'BK'/ holds allowed (total): 0/
hold allowed (daily): 0 / holds per record: 0

Patron category: 'P' (Patron)/Item type: 'BK' / holds allowed (total):
1/ holds allowed (daily): 1 / holds per record 1

3. On the biblio record place a biblio-level hold for patron A (who has patron
category of 'B') and patron B (who has patron category of 'P')

4. Return the 'BK' item (item with item type of 'BK')

5. Notice a popup asking you to confirm the hold by patron A (even
though patrons of category 'B' cannot hold items of item type
'BK')

6. Apply patch and restart memcached and plack

7. Remove both holds on biblio

8. Repeat steps 3-5 but notice instead of a popup for patron A
appearing, a popup asking you to confirm patron B appears

i.e. patron A's bib-level hold is skipped as the issuingrules has been
checked when the item was returned and it has been found that they
cannot reserve an item with that specific itemtype

Sponsored-By: Brimbank Library, Australia
Comment 2 Alex Buckley 2019-06-21 04:43:44 UTC
Unit test patch for testing the new function: Koha::Hold->check_if_existing_hold_matches_issuingrules() is my next priority for this patch.
Comment 3 Alex Buckley 2019-07-01 07:53:24 UTC
Created attachment 91133 [details] [review]
Bug 23172: Unit tests

Adding unit tests to check the output of the
Koha::Hold->check_if_existing_hold_matches_issuingrules()

Test plan:
1. Run test plan in first patch

2. Enter Koha shell:
sudo koha-shell <instancename>

3. Run: prove t/db_dependent/Hold.t

4. All tests should pass

Sponsored-By: Brimbank Library, Australia
Comment 4 Hayley Pelham 2019-07-01 20:59:33 UTC
Ready to be signed off
Comment 5 Alex Buckley 2019-07-04 23:57:22 UTC
Created attachment 91301 [details] [review]
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

This patch adds to the unit test coverage of the new
Koha::Hold->check_if_existing_hold_matches_issuingrules().

In patch two on bug 23172 I added unit test for directly testing the new
subroutine Koha::Hold->check_if_existing_hold_matches_issuingrules().

In this patch I fix the test failures in t/db_dependent/Circulation.t
and t/db_dependent/Reserves.t and add a test to confirm
C4::Circulation->AddReturn() works correctly with
Koha::Hold->check_if_existing_hold_matches_issuingrules().

This is because when an item is returned C4::Circulation->AddReturn() calls C4::Reserves->CheckReserves()
which in turn calls the aforementioned new subroutine.

So the additional unit tests in this patch check all these subroutines
work correctly together.

Test plan:
1. Apply first two patches and follow their respective test plans

2. Enter Koha shell: sudo koha-shell <instancename>

3. Run (from your koha home directory):
prove t/db_dependent/Circulation.t

Confirm it fails

4. Run:
prove t/db_dependent/Reserves.t

Confirm it fails

5. Apply this patch

6. Repeat steps 3 and 4 and confirm the tests pass

6. Confirm t/db_dependent/Holds.t still passes by repeating step 2 and
then running:
prove t/db_dependent/Holds.t

Sponsored-By: Brimbank Library, Australia
Comment 6 Andrew Fuerste-Henry 2019-08-15 21:01:51 UTC
Signed off by Andrew Fuerste-Henry
Comment 7 Hayley Pelham 2019-08-16 03:50:15 UTC
Created attachment 92250 [details] [review]
Bug 23172: When returning items check issuingrules

When iterating over every reserve on a biblio when it is being returned
retrieve the issuingrule reserveallowed and holds_per_record values for
the reserves requester borrower category and returned item type.

If the issuingrule defines that reservesallowed or holds_per_record is
0 then skip allocating that item to the current bib-level reserve.

Test plan:
1. Find/make a biblio record with two items: one with item type of 'BK'
(book) and the other with the itemtype of 'CR' (Continuing Resource)

2. In Administration > Circulation and fine rules define 2 new rules:

Patron category: 'B' (Board)/ item type: 'BK'/ holds allowed (total): 0/
hold allowed (daily): 0 / holds per record: 0

Patron category: 'P' (Patron)/Item type: 'BK' / holds allowed (total):
1/ holds allowed (daily): 1 / holds per record 1

3. On the biblio record place a biblio-level hold for patron A (who has patron
category of 'B') and patron B (who has patron category of 'P')

4. Return the 'BK' item (item with item type of 'BK')

5. Notice a popup asking you to confirm the hold by patron A (even
though patrons of category 'B' cannot hold items of item type
'BK')

6. Apply patch and restart memcached and plack

7. Remove both holds on biblio

8. Repeat steps 3-5 but notice instead of a popup for patron A
appearing, a popup asking you to confirm patron B appears

i.e. patron A's bib-level hold is skipped as the issuingrules has been
checked when the item was returned and it has been found that they
cannot reserve an item with that specific itemtype

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 8 Hayley Pelham 2019-08-16 03:50:51 UTC
Created attachment 92251 [details] [review]
Bug 23172: Unit tests

Adding unit tests to check the output of the
Koha::Hold->check_if_existing_hold_matches_issuingrules()

Test plan:
1. Run test plan in first patch

2. Enter Koha shell:
sudo koha-shell <instancename>

3. Run: prove t/db_dependent/Hold.t

4. All tests should pass

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 9 Hayley Pelham 2019-08-16 03:51:25 UTC
Created attachment 92252 [details] [review]
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

This patch adds to the unit test coverage of the new
Koha::Hold->check_if_existing_hold_matches_issuingrules().

In patch two on bug 23172 I added unit test for directly testing the new
subroutine Koha::Hold->check_if_existing_hold_matches_issuingrules().

In this patch I fix the test failures in t/db_dependent/Circulation.t
and t/db_dependent/Reserves.t and add a test to confirm
C4::Circulation->AddReturn() works correctly with
Koha::Hold->check_if_existing_hold_matches_issuingrules().

This is because when an item is returned C4::Circulation->AddReturn() calls C4::Reserves->CheckReserves()
which in turn calls the aforementioned new subroutine.

So the additional unit tests in this patch check all these subroutines
work correctly together.

Test plan:
1. Apply first two patches and follow their respective test plans

2. Enter Koha shell: sudo koha-shell <instancename>

3. Run (from your koha home directory):
prove t/db_dependent/Circulation.t

Confirm it fails

4. Run:
prove t/db_dependent/Reserves.t

Confirm it fails

5. Apply this patch

6. Repeat steps 3 and 4 and confirm the tests pass

6. Confirm t/db_dependent/Holds.t still passes by repeating step 2 and
then running:
prove t/db_dependent/Holds.t

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 10 Hayley Pelham 2019-08-16 03:52:15 UTC
Have attached signed off patches based on Andrew's comment #6
Comment 11 Nick Clemens 2019-09-03 16:34:33 UTC
I didn't fully test, but a few notes:
1 - Reserves.t fails after applying the patches
2 - The tests should use specific branches rather than deleting all rules/reserves - not a blocker
3 - check_if_existing_hold_matches_issuingrules should use the item and borrower methods rather than find - this will let us use table relationships and prefetch in the future and save db calls/hits
4 - Why do you unbless the item?
5 - the ReservesControlBranch code could be its own function  - Koha::Hold::get_control_branch - not a blocker, but the tests should mock the pref and ensure behaviour works as expected with both settings
Comment 12 Aleisha Amohia 2020-03-31 02:56:10 UTC
Created attachment 102099 [details] [review]
Bug 23172: When returning items check issuingrules

When iterating over every reserve on a biblio when it is being returned
retrieve the issuingrule reserveallowed and holds_per_record values for
the reserves requester borrower category and returned item type.

If the issuingrule defines that reservesallowed or holds_per_record is
0 then skip allocating that item to the current bib-level reserve.

Test plan:
1. Find/make a biblio record with two items: one with item type of 'BK'
(book) and the other with the itemtype of 'CR' (Continuing Resource)

2. In Administration > Circulation and fine rules define 2 new rules:

Patron category: 'B' (Board)/ item type: 'BK'/ holds allowed (total): 0/
hold allowed (daily): 0 / holds per record: 0

Patron category: 'P' (Patron)/Item type: 'BK' / holds allowed (total):
1/ holds allowed (daily): 1 / holds per record 1

3. On the biblio record place a biblio-level hold for patron A (who has patron
category of 'B') and patron B (who has patron category of 'P')

4. Return the 'BK' item (item with item type of 'BK')

5. Notice a popup asking you to confirm the hold by patron A (even
though patrons of category 'B' cannot hold items of item type
'BK')

6. Apply patch and restart memcached and plack

7. Remove both holds on biblio

8. Repeat steps 3-5 but notice instead of a popup for patron A
appearing, a popup asking you to confirm patron B appears

i.e. patron A's bib-level hold is skipped as the issuingrules has been
checked when the item was returned and it has been found that they
cannot reserve an item with that specific itemtype

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 13 Aleisha Amohia 2020-03-31 02:56:13 UTC
Created attachment 102100 [details] [review]
Bug 23172: Unit tests

Adding unit tests to check the output of the
Koha::Hold->check_if_existing_hold_matches_issuingrules()

Test plan:
1. Run test plan in first patch

2. Enter Koha shell:
sudo koha-shell <instancename>

3. Run: prove t/db_dependent/Hold.t

4. All tests should pass

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 14 Aleisha Amohia 2020-03-31 02:56:17 UTC
Created attachment 102101 [details] [review]
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

This patch adds to the unit test coverage of the new
Koha::Hold->check_if_existing_hold_matches_issuingrules().

In patch two on bug 23172 I added unit test for directly testing the new
subroutine Koha::Hold->check_if_existing_hold_matches_issuingrules().

In this patch I fix the test failures in t/db_dependent/Circulation.t
and t/db_dependent/Reserves.t and add a test to confirm
C4::Circulation->AddReturn() works correctly with
Koha::Hold->check_if_existing_hold_matches_issuingrules().

This is because when an item is returned C4::Circulation->AddReturn() calls C4::Reserves->CheckReserves()
which in turn calls the aforementioned new subroutine.

So the additional unit tests in this patch check all these subroutines
work correctly together.

Test plan:
1. Apply first two patches and follow their respective test plans

2. Enter Koha shell: sudo koha-shell <instancename>

3. Run (from your koha home directory):
prove t/db_dependent/Circulation.t

Confirm it fails

4. Run:
prove t/db_dependent/Reserves.t

Confirm it fails

5. Apply this patch

6. Repeat steps 3 and 4 and confirm the tests pass

6. Confirm t/db_dependent/Holds.t still passes by repeating step 2 and
then running:
prove t/db_dependent/Holds.t

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 15 Aleisha Amohia 2020-04-01 03:47:29 UTC
Created attachment 102169 [details] [review]
Bug 23172: (follow-up) Fixing test

Ensure that the following tests pass:
t/db_dependent/Hold.t
t/db_dependent/Reserves.t

And that the original test plan works as expected
Comment 16 Mazen Khallaf 2021-01-18 23:02:03 UTC
Failed to apply 'Unit tests' patch

git bz apply 23172
Bug 23172 - Issuingrules not checked when returning item and allocating to bib-level holds

102099 - Bug 23172: When returning items check issuingrules
102100 - Bug 23172: Unit tests
102101 - Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests
102169 - Bug 23172: (follow-up) Fixing test

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23172: When returning items check issuingrules
Applying: Bug 23172: Unit tests
error: sha1 information is lacking or useless (Koha/Hold.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 23172: Unit tests
The copy of the patch that failed is found in: .git/rebase-apply/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-23172-Unit-tests-gusCAn.patch
Comment 17 Alex Buckley 2021-03-29 05:24:28 UTC
Created attachment 118930 [details] [review]
Bug 23172: When returning items check issuingrules

When iterating over every reserve on a biblio when it is being returned
retrieve the issuingrule reserveallowed and holds_per_record values for
the reserves requester borrower category and returned item type.

If the issuingrule defines that reservesallowed or holds_per_record is
0 then skip allocating that item to the current bib-level reserve.

Test plan:
1. Find/make a biblio record with two items: one with item type of 'BK'
(book) and the other with the itemtype of 'CR' (Continuing Resource)

2. In Administration > Circulation and fine rules define 2 new rules:

Patron category: 'B' (Board)/ item type: 'BK'/ holds allowed (total): 0/
hold allowed (daily): 0 / holds per record: 0

Patron category: 'P' (Patron)/Item type: 'BK' / holds allowed (total):
1/ holds allowed (daily): 1 / holds per record 1

3. On the biblio record place a biblio-level hold for patron A (who has patron
category of 'B') and patron B (who has patron category of 'P')

4. Return the 'BK' item (item with item type of 'BK')

5. Notice a popup asking you to confirm the hold by patron A (even
though patrons of category 'B' cannot hold items of item type
'BK')

6. Apply patch and restart memcached and plack

7. Remove both holds on biblio

8. Repeat steps 3-5 but notice instead of a popup for patron A
appearing, a popup asking you to confirm patron B appears

i.e. patron A's bib-level hold is skipped as the issuingrules has been
checked when the item was returned and it has been found that they
cannot reserve an item with that specific itemtype

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 18 Alex Buckley 2021-03-29 05:24:47 UTC
Created attachment 118931 [details] [review]
Bug 23172: Unit tests

Adding unit tests to check the output of the
Koha::Hold->check_if_existing_hold_matches_issuingrules()

Test plan:
1. Run test plan in first patch

2. Enter Koha shell:
sudo koha-shell <instancename>

3. Run: prove t/db_dependent/Hold.t

4. All tests should pass

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 19 Alex Buckley 2021-03-29 05:25:09 UTC
Created attachment 118932 [details] [review]
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

This patch adds to the unit test coverage of the new
Koha::Hold->check_if_existing_hold_matches_issuingrules().

In patch two on bug 23172 I added unit test for directly testing the new
subroutine Koha::Hold->check_if_existing_hold_matches_issuingrules().

In this patch I fix the test failures in t/db_dependent/Circulation.t
and t/db_dependent/Reserves.t and add a test to confirm
C4::Circulation->AddReturn() works correctly with
Koha::Hold->check_if_existing_hold_matches_issuingrules().

This is because when an item is returned C4::Circulation->AddReturn() calls C4::Reserves->CheckReserves()
which in turn calls the aforementioned new subroutine.

So the additional unit tests in this patch check all these subroutines
work correctly together.

Test plan:
1. Apply first two patches and follow their respective test plans

2. Enter Koha shell: sudo koha-shell <instancename>

3. Run (from your koha home directory):
prove t/db_dependent/Circulation.t

Confirm it fails

4. Run:
prove t/db_dependent/Reserves.t

Confirm it fails

5. Apply this patch

6. Repeat steps 3 and 4 and confirm the tests pass

6. Confirm t/db_dependent/Holds.t still passes by repeating step 2 and
then running:
prove t/db_dependent/Holds.t

Sponsored-By: Brimbank Library, Australia

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 20 Alex Buckley 2021-03-29 05:25:23 UTC
Created attachment 118933 [details] [review]
Bug 23172: (follow-up) Fixing test

Ensure that the following tests pass:
t/db_dependent/Hold.t
t/db_dependent/Reserves.t

And that the original test plan works as expected
Comment 21 David Nind 2021-03-29 11:14:42 UTC
Hi Alex.

The first patch works, but I ran into problems with the tests (it may be a sequencing problem with me apply the patches in the wrong order):

1. prove t/db_dependent/Holds.t: tests fail

prove t/db_dependent/Holds.t
t/db_dependent/Holds.t .. 66/68 Argument "" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Hold.pm line 520.

    #   Failed test 'Cannot renew'
    #   at t/db_dependent/Holds.t line 1295.

    #   Failed test 'Item is on hold'
    #   at t/db_dependent/Holds.t line 1296.
    #          got: undef
    #     expected: 'on_reserve'
Argument "" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Hold.pm line 520.
Argument "" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Hold.pm line 520.
Argument "" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Hold.pm line 520.

    #   Failed test 'Cannot renew'
    #   at t/db_dependent/Holds.t line 1328.

    #   Failed test 'Item is on hold'
    #   at t/db_dependent/Holds.t line 1329.
    #          got: undef
    #     expected: 'on_reserve'
    # Looks like you failed 4 tests of 6.

#   Failed test 'non priority holds'
#   at t/db_dependent/Holds.t line 1333.
t/db_dependent/Holds.t .. 68/68 # Looks like you failed 1 test of 68.
t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/68 subtests 

Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 256 Tests: 68 Failed: 1)
  Failed test:  67
  Non-zero exit status: 1

2 t/db_dependent/Hold.t: fails - changing to 34 tests works

  Failed test:  34
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 33 tests but ran 34.

David
Comment 22 Alex Buckley 2021-03-29 19:53:54 UTC
Thanks for testing David, looking now
Comment 23 Alex Buckley 2021-12-21 03:34:20 UTC
I've been investigating this.

In master, the current patchset does not work. Not just because it doesn't apply cleanly, but also because C4::Reserves->_Findgroupreserve() - called when returning an item - checks if there is a holds-queue title-level match. Returning the first match found.

But the problem is when the holds queue sets items for title-level reserves the C4::HoldsQueue->MapItemsToHoldRequests() only checks hold policies regardless of patron category.

So I could define that students should not be able to reserve book item types, and yet the holds queue would set a book item for a student's title-levle reserve. 

I think the holds queue should check the hold policies for a given patron category.

I will work on adding that.
Comment 24 Alex Buckley 2021-12-21 04:33:12 UTC
Created attachment 128817 [details] [review]
Bug 23172: The holds queue should check if a patron category is eligible to reserve an itemtype

Currently when mapping items to pending holds the holds queue does not
check the circulation rules to determine if the reserving patron category is eligible to reserve the item.

This patch fixes that by calling C4::Reserves->CanItemBeReserved() when
mapping items to pending reserves.

Test plan:
1. Create two patrons, with the following patron categories:
- Patron A -> Patron
- Patron B -> Student
2. Create two items attached to a single biblio record:
- Book
- Continuing resource
3. Create the following circ rules:
Patron category, item type, Holds allowed (total), Holds allowed
(daily), Holds per record (count)
- Patron, Book, 0, 0, 0
- Student, Book, 1, 1, 1
- All, Continuing resources, 1, 1, 1
4. Place a title level hold on the biblio for patron A
5. Place a title level hold on the biblio for patron B
6. Run HoldsQueue.pl
7. Notice in 'Circulation' > 'Holds queue' Patron A is listed as having
a hold on the Book
8. Checkin the Book and notice you are prompted to confirm the hold by
Patron A - even though according to circ rules they cannot reserve Books
9. Apply patch and restart services
10. Run HoldsQueue.pl
11. Notice in 'Circulation' > 'Holds queue' Patron A is listed as having
a hold on the Continuing resource. Patron B is listed as having a hold
on the Book -> These holds conform to the circulation rules
12. Checkin the Book, and notice you're prompted to confirm the hold by
Patron B

Sponsored-By: Brimbank Library, Australia
Comment 25 Alex Buckley 2021-12-21 04:33:48 UTC
Patchset attached and unit tests to come
Comment 26 Alex Buckley 2021-12-22 03:21:15 UTC
Created attachment 128857 [details] [review]
Bug 23172: Unit test for holds queue

Test plan:
1. sudo koha-shell <instance>
2. prove t/db_dependent/HoldsQueue.t

Sponsored-By: Brimbank Library, Australia
Comment 27 Alex Buckley 2021-12-22 04:14:40 UTC
Hi all, 

This is ready for testing, with unit tests attached. This patchset covers adding the check of patron category hold policies when the holds queue maps items to pending reserves.

Meanwhile, I've split out to bug 29750 a check of patron category hold policies when an item is returned and Koha looks for reserves to allocate to it. bug 29750  is necessary for when an item is returned before the hold queue has been updated.

Thanks!
Alex
Comment 28 Nick Clemens 2021-12-22 13:35:24 UTC
I have some concerns with the changes in this patchset, and its dependent bug 29750.

While I agree that the current behavior does not match expectations, I think we are conflating two circ rules when we make this change.

 - reservesallowed defines how many holds a patron cna place on an item.
 - hold_allowed defines whether the patron is eligible to have a hold on an item.

When libraries have AllowHoldPolicyOverride enabled, they will setup policies such that patrons cannot place holds, but staff can place these holds and they will be filled.

After these patches, there will be new holds that can be placed by override but will not fill.

I think the two questions:
 - Can a patron place a hold?
 - Can this item fill a hold for this patron?
Are different.

I think the real solution here would be to make 'hold_allowed' part of the circ rules matrix in a way that allows it to be set at a more granular level.
Comment 29 Alex Buckley 2021-12-22 23:18:33 UTC
(In reply to Nick Clemens from comment #28)

Hi Nick, 

Thanks for your reply. We'd like to think more about this and revisit it in the new year.

Have a great Christmas holiday.

Thanks,
Alex