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.
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
Unit test patch for testing the new function: Koha::Hold->check_if_existing_hold_matches_issuingrules() is my next priority for this patch.
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
Ready to be signed off
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
Signed off by Andrew Fuerste-Henry
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>
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>
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>
Have attached signed off patches based on Andrew's comment #6
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
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>
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>
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>
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
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
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>
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>
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>
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
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
Thanks for testing David, looking now
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.
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
Patchset attached and unit tests to come
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
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
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.
(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
I would love to see this issue be picked back up. Looks like it has been around a while and has gone stale. Our libraries are now affected by this issue as our boards/laws have made things a little more complex. We would like to see this move towards a resolution.