[Bug 41879] New: Holds that move to a new bib can be unfillable
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Bug ID: 41879 Summary: Holds that move to a new bib can be unfillable Initiative type: --- Sponsorship --- status: Product: Koha Version: 25.11 Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: sbrown@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, lisette@bywatersolutions.com, tomascohen@gmail.com If a library's 'Holds and bookings policies by item type' allows holds by patrons from any library, but pickup at the item’s home library only, holds can be moved from bib A to bib B in a way that makes them unfillable with no indication that they won't fill. Currently, when the hold is moved from bib A to bib B, the hold pickup location will not automatically update to reflect the rule, and there is no notice that the rule has been broken. Scenario: -ReservesControlBranch is set to 'item's home library' -Main library's 'Holds and bookings policies by item type' for the itype BOOK is set to allow holds from patrons from any library, but pickup at Main only -Bib A is owned by North library, and bib B is owned by Main. Both are the item type BOOK, and each has one item on the bib. Steps: -Place a hold for a North patron on bib A for pickup at North -Move the hold to bib B -Note that the hold moves successfully, and is still set for pickup at North When the hold moves, there is no indication that the item will never trigger to send to North due to Main's 'Holds and bookings policies by item type'. The only way staff would realize that the hold will never fill is if they drop down the pickup locations for the moved hold and see the flag indicating that the current location isn't allowed. The move should either fail (as it does if the target record only allows holds from patrons matching the item’s home library per 'Holds and bookings policies by item type'), or staff should be forced to select a valid pickup location in order to complete the move. -- 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=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|25.11 |Main Assignee|koha-bugs@lists.koha-commun |lucas@bywatersolutions.com |ity.org | -- 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=41879 --- Comment #1 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198756 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198756&action=edit Bug 41879: Disallow moves to invalid pickup locations for items Patch from commit de849bb -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #2 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198768&action=edit Bug 41879: Disallow moves to invalid pickup locations for next available holds Patch from commit 8fd12db -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #3 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198772 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198772&action=edit Bug 41879: update move_hold unit tests Patch from commit 81b5ee1 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198756|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=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198768|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=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198772|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=41879 --- Comment #4 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198785 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198785&action=edit Bug 41879: Disallow moves to invalid pickup locations for items To test: 1. Set ReservesControlBranch is set to 'item's home library' 2. Set the deafult 'Holds and bookings policies by item type' for the itype BK to allow holds from patrons from any library, but pickup at CPL only 3. Have a bib owned by another library ( I used MPL ), and bib owned by CPL. Both are the item type BK, and each has one item on the bib. 4. Place a hold for a patron on bib A for pickup at MPL 5. Move the hold to bib B 6. Note that the hold moves successfully, and is still set for pickup at MPL 7. Login and try to trigger the hold, nothing happens 8. APPLY PATCH, restart_all 9. Try spets 3 - 6 again. This time on step 6 you should see a message that the hold cannot be moved. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #5 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198786 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198786&action=edit Bug 41879: Disallow moves to invalid pickup locations for next available holds Patch from commit 38e46bc -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 198787 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198787&action=edit Bug 41879: update move_hold unit tests Patch from commit f57e251 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |ephetteplace@cca.edu --- Comment #7 from Eric Phetteplace <ephetteplace@cca.edu> --- Works as advertised, signing off shortly. For step #7 what I did was check in the item on the bib, nothing happened (said "item not checked out" but not that a hold was waiting). There was other weirdness like if you try to change the pickup location to another non-CPL (invalid) location there's a 400 error and nothing happens. prove t/db_dependent/Koha/Hold.t also passed. Could the error message could be improved? Something about "Hold ID: 1 Target item/record is an invalid pickup location" feels off to be because it's saying the item/record _is_ a location. Maybe "Target item/record would have an invalid pickup location"? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198785|0 |1 is obsolete| | Attachment #198786|0 |1 is obsolete| | Attachment #198787|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=41879 --- Comment #8 from Eric Phetteplace <ephetteplace@cca.edu> --- Created attachment 199708 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199708&action=edit Bug 41879: Disallow moves to invalid pickup locations for items To test: 1. Set ReservesControlBranch is set to 'item's home library' 2. Set the deafult 'Holds and bookings policies by item type' for the itype BK to allow holds from patrons from any library, but pickup at CPL only 3. Have a bib owned by another library ( I used MPL ), and bib owned by CPL. Both are the item type BK, and each has one item on the bib. 4. Place a hold for a patron on bib A for pickup at MPL 5. Move the hold to bib B 6. Note that the hold moves successfully, and is still set for pickup at MPL 7. Login and try to trigger the hold, nothing happens 8. APPLY PATCH, restart_all 9. Try spets 3 - 6 again. This time on step 6 you should see a message that the hold cannot be moved. Signed-off-by: Eric Phetteplace <phette23@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #9 from Eric Phetteplace <ephetteplace@cca.edu> --- Created attachment 199709 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199709&action=edit Bug 41879: Disallow moves to invalid pickup locations for next available holds Signed-off-by: Eric Phetteplace <phette23@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #10 from Eric Phetteplace <ephetteplace@cca.edu> --- Created attachment 199710 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=199710&action=edit Bug 41879: update move_hold unit tests Signed-off-by: Eric Phetteplace <phette23@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #11 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> ---
Could the error message could be improved? Something about "Hold ID: 1 Target item/record is an invalid pickup location" feels off to be because it's saying the item/record _is_ a location. Maybe "Target item/record would have an invalid pickup location"?
Eric, thanks for testing! I'm happy to improve the message. Could we do so a separate bug report? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42722 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42722 [Bug 42722] Improve invalidPickupBranch error message -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #12 from Eric Phetteplace <ephetteplace@cca.edu> --- Sounds good! I created Bug 42722 and set it to "In Discussion" with some ideas on phrasing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |blawlor@clamsnet.org |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #13 from Brendan Lawlor <blawlor@clamsnet.org> --- One thing I noticed is the logic for moving an item level hold and for moving a bib level hold are duplicated, just inside a loop for the bib level move. It's worth considering refactoring into a helper function so that any future changes to the hold fulfillment policy logic would only need to be updated in one place. Not necessarily a blocker, but it's good to know. Hold.pm lines 217-226: # need to make sure the item is being moved to a valid pickup location my $branchitemrule = C4::Circulation::GetBranchItemRule( Koha::Policy::Holds->holds_control_library( $item, $patron ), $item->effective_itemtype ); my $policy = $branchitemrule->{hold_fulfillment_policy}; if ( ( $policy eq 'homebranch' || $policy eq 'holdingbranch' ) && $self->branchcode ne $item->$policy ) { return { success => 0, error => 'invalidPickupBranch' }; } Hold.pm lines 234-246 my $valid_pickup_item = 0; foreach my $candidate_item (@items) { my $branchitemrule = C4::Circulation::GetBranchItemRule( Koha::Policy::Holds->holds_control_library( $candidate_item, $patron ), $candidate_item->effective_itemtype ); my $policy = $branchitemrule->{hold_fulfillment_policy}; next if ( $policy eq 'homebranch' || $policy eq 'holdingbranch' ) && $self->branchcode ne $candidate_item->$policy; $valid_pickup_item = 1; last; } One other minor thing, and maybe it could be done on the follow up bug about the error message, but if you attempt to move a hold to a bib with no items the error message is misleading. One or more holds were not moved due to following errors: Hold ID: 2 Target item/record is an invalid pickup location In this case the real reason it can't be transferred is there are are no items on the bib record at all. Other than that everything works as advertised and the QA scripts are happy. The unit tests cover the scenario in the test plan. Not sure if it's really important, but the tests do not specifically cover the scenario of moving a hold to a bib with no items. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #14 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202128 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202128&action=edit Bug 41879: (follow-up) Add helper function to avoid repeating code Patch from commit e748632 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #15 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202129 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202129&action=edit Bug 41879: (follow-up) Bail with error code when no items on target bib Patch from commit 173d2e4 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #16 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202130&action=edit Bug 41879: (follow-up) Add a test Patch from commit 4df6115 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #17 from Brendan Lawlor <blawlor@clamsnet.org> --- Created attachment 202436 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202436&action=edit Bug 41879: (QA follow-up) Add pod for new sub _pickup_valid_for_item Patch from commit ed30afe -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #18 from Brendan Lawlor <blawlor@clamsnet.org> --- QA notes: Currently Proving /kohadevbox/koha/t/db_dependent/Koha/Hold.t FAIL but that is do to the last follow up on Bug 42391 not being pushed yet. This works as described to prevent holds from being moved to items/records where there are no items with valid pickup locations, due to using ReservesControlBranch set to item home library and a circulation rule where the hold pickup location must match the item's home library. Nice work. Passing QA! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k --- Comment #19 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- The noItemsOnBib error (moving a record-level hold to a bib with no items) has no [% CASE %] in request.tt, so it falls through to the generic fallback and shows "Error: noItemsOnBib" instead of a sentence. One-line fix: [% CASE 'noItemsOnBib' %] <span>Target record has no items</span> Not blocking, just flagging for a follow-up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #20 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.11.00 |26.11.00,26.05.03 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 --- Comment #21 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.03 release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com Version(s)|26.11.00,26.05.03 |26.11.00,26.05.03,25.11.10 released in| | Status|Pushed to stable |Pushed to oldstable --- Comment #22 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Pushed into 25.11.x for 25.11.10 nice work everyone -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41879 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|ephetteplace@cca.edu | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org