Some libraries would like to be able to limit hold filling to items that match the pickup library for a hold based on the item's home or holding library. The patron's home library should not affect whether a patron can place the hold, instead the hold will only be fillable when an item matching the pickup location becomes available.
Created attachment 46461 [details] [review] Bug 15532 - Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold Some libraries would like to be able to limit hold filling to items that match the pickup library for a hold based on the item's home or holding library. The patron's home library should not affect whether a patron can place the hold, instead the hold will only be fillable when an item matching the pickup location becomes available. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Note the new "Hold pickup library match" rules for "checkout, hold, and return policy" and for "holds policy by item type" 4) Set the policy to "item's holding library" 5) Place a hold where the item's holding branch does not match the pickup branch 6) Check in the item 7) Note it is not trapped for the hold 8) Update the item's holding branch to match the pickup branch 8) Check in the item 9) Note the item is trapped for the hold 10) Repeat steps 4-9 but for home branch instead
Created attachment 46753 [details] [review] [SIGNED-OFF]Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold Some libraries would like to be able to limit hold filling to items that match the pickup library for a hold based on the item's home or holding library. The patron's home library should not affect whether a patron can place the hold, instead the hold will only be fillable when an item matching the pickup location becomes available. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Note the new "Hold pickup library match" rules for "checkout, hold, and return policy" and for "holds policy by item type" 4) Set the policy to "item's holding library" 5) Place a hold where the item's holding branch does not match the pickup branch 6) Check in the item 7) Note it is not trapped for the hold 8) Update the item's holding branch to match the pickup branch 8) Check in the item 9) Note the item is trapped for the hold 10) Repeat steps 4-9 but for home branch instead Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as described
Created attachment 49225 [details] [review] Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold Some libraries would like to be able to limit hold filling to items that match the pickup library for a hold based on the item's home or holding library. The patron's home library should not affect whether a patron can place the hold, instead the hold will only be fillable when an item matching the pickup location becomes available. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Note the new "Hold pickup library match" rules for "checkout, hold, and return policy" and for "holds policy by item type" 4) Set the policy to "item's holding library" 5) Place a hold where the item's holding branch does not match the pickup branch 6) Check in the item 7) Note it is not trapped for the hold 8) Update the item's holding branch to match the pickup branch 8) Check in the item 9) Note the item is trapped for the hold 10) Repeat steps 4-9 but for home branch instead Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as described
Comment on attachment 49225 [details] [review] Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold Review of attachment 49225 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Holds/HoldFulfillmentPolicy.t @@ +9,5 @@ > +use t::lib::TestBuilder; > + > +BEGIN { > + use FindBin; > + use lib $FindBin::Bin; Is this useful? @@ +49,5 @@ > +$dbh->do("DELETE FROM reserves"); > +$dbh->do("DELETE FROM issues"); > +$dbh->do("DELETE FROM items"); > +$dbh->do("DELETE FROM biblio"); > +$dbh->do("DELETE FROM biblioitems"); Why do you delete the entries from these 5 tables? ::: t/db_dependent/HoldsQueue.t @@ +443,4 @@ > is( @$holds_queue, 0, "Bug 15062 - Holds queue with Transport Cost Matrix will transfer item even if transfers disabled"); > # End Bug 15062 > > +# Test hold_fulfillment_policy This looks a c/p of t/db_dependent/Holds/HoldFulfillmentPolicy.t
Created attachment 50358 [details] [review] Bug 15532 [QA Followup] - Remove unecessary lines from unit test
Small problem noted by the QA script: FAIL t/db_dependent/Holds/HoldFulfillmentPolicy.t FAIL critic "$holds_queue" is declared but not used at line 79, column 1. Unused variables clutter code and make it harder to read. OK forbidden patterns OK pod OK spelling 0 OK valid
Created attachment 50838 [details] [review] Bug 15532 [QA Followup] - Remove unused variable
QA: Looking here now. Brr Holds
Created attachment 50901 [details] [review] Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold Some libraries would like to be able to limit hold filling to items that match the pickup library for a hold based on the item's home or holding library. The patron's home library should not affect whether a patron can place the hold, instead the hold will only be fillable when an item matching the pickup location becomes available. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Note the new "Hold pickup library match" rules for "checkout, hold, and return policy" and for "holds policy by item type" 4) Set the policy to "item's holding library" 5) Place a hold where the item's holding branch does not match the pickup branch 6) Check in the item 7) Note it is not trapped for the hold 8) Update the item's holding branch to match the pickup branch 8) Check in the item 9) Note the item is trapped for the hold 10) Repeat steps 4-9 but for home branch instead Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com> Works as described Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 50902 [details] [review] Bug 15532 [QA Followup] - Remove unecessary lines from unit test Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 50903 [details] [review] Bug 15532 [QA Followup] - Remove unused variable Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Code looks good to me. No errors from qa tools. Holds tests pass (Holds.t reports two errors before this patch already.) Still I am passing QA on this patch with some hesitation. I have no good reason to block this patch, especially since not touching the circulation rules will not change behavior. Hesitation is based on: [1] Simple test plan. We have so many prefs hooking into reserves/holds. How do we make sure that everything still works? (Tests pass..) [2] opac-reserve offers pickup locations that will (or may) not allow a hold to be filled. Should we offer them in the first place in such cases? [3] Rules form: Hold pickup library match is rather cryptic. The text above the rules should be made more clear (it pertains to Hold policy only). The new field could use some text and additional documentation (online help). Passed QA
Pushed to Master - should be in the May 2016 - Release. Thanks!