[Bug 23172] New: Issuingrules not checked when returning item and allocating to bib-level holds
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Bug ID: 23172 Summary: Issuingrules not checked when returning item and allocating to bib-level holds Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: alexbuckley@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com Target Milestone: --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #1 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 90868 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90868&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #2 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Unit test patch for testing the new function: Koha::Hold->check_if_existing_hold_matches_issuingrules() is my next priority for this patch. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |alexbuckley@catalyst.net.nz |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |hayleymapley@catalyst.net.n | |z Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #3 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 91133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91133&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #4 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Ready to be signed off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #5 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 91301 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91301&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aleisha@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Andrew <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Andrew <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Who signed the| |Andrew Fuerste-Henry patch off| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Andrew <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Who signed the|Andrew Fuerste-Henry | patch off| | --- Comment #6 from Andrew <andrew@bywatersolutions.com> --- Signed off by Andrew Fuerste-Henry -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90868|0 |1 is obsolete| | --- Comment #7 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Created attachment 92250 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92250&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91133|0 |1 is obsolete| | --- Comment #8 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Created attachment 92251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92251&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91301|0 |1 is obsolete| | --- Comment #9 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Created attachment 92252 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92252&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #10 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Have attached signed off patches based on Andrew's comment #6 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Margaret <margaret@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |margaret@bywatersolutions.c | |om -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |nick@bywatersolutions.com --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Christopher Brannon <cbrannon@cdalibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cbrannon@cdalibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #92250|0 |1 is obsolete| | --- Comment #12 from Aleisha Amohia <aleisha@catalyst.net.nz> --- Created attachment 102099 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102099&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #92251|0 |1 is obsolete| | --- Comment #13 from Aleisha Amohia <aleisha@catalyst.net.nz> --- Created attachment 102100 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102100&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #92252|0 |1 is obsolete| | --- Comment #14 from Aleisha Amohia <aleisha@catalyst.net.nz> --- Created attachment 102101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102101&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #15 from Aleisha Amohia <aleisha@catalyst.net.nz> --- Created attachment 102169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102169&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=4850 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Mazen Khallaf <mazen.i.khallaf@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |mazen.i.khallaf@gmail.com --- Comment #16 from Mazen Khallaf <mazen.i.khallaf@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102099|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102100|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102101|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102169|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #17 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 118930 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118930&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #18 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 118931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118931&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #19 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 118932 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118932&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #20 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 118933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118933&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Failed QA --- Comment #21 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED --- Comment #22 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Thanks for testing David, looking now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #23 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118933|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118932|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118931|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118930|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #24 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 128817 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128817&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #25 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Patchset attached and unit tests to come -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=11350 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Lisette Scheer <lisetteslatah@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisetteslatah@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #26 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 128857 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128857&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Issuingrules not checked |Holds queue should check |when returning item and |patron category hold |allocating to bib-level |policies when mapping items |holds |to pending reserves Status|ASSIGNED |Needs Signoff Text to go in the| |The hold queue should check release notes| |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. --- Comment #27 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29750 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion Blocks| |29750 --- Comment #28 from Nick Clemens <nick@bywatersolutions.com> --- 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29750 [Bug 29750] Reserves.pm should check patron category hold policies when finding a reserve for an item -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |arthur.suzuki@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Sara Brown <sbrown@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29760 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #29 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30274 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|29750 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29750 [Bug 29750] Reserves.pm should check patron category hold policies when finding a reserve for an item -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128817|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128857|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 --- Comment #30 from Christopher Brannon <cbrannon@cdalibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 Catrina Berka <catrina@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |catrina@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23172 ayoung <ayoung@oslri.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ayoung@oslri.net --- Comment #31 from ayoung <ayoung@oslri.net> --- I would also love to see this issue picked back up. There is currently no way for a library to stop their items from triggering holds for a specific patron category (they can stop a patron category from placing holds on their items, but only the items that do not share a bib with another library that allows that patron category to place holds.) There needs to be a rule in the circ rules that bars a patron category from both placing AND triggering holds. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org