Bug 15532 - Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold
Summary: Add ability to allow only items whose home/holding branch matches the hold's ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 15708 15533
  Show dependency treegraph
 
Reported: 2016-01-09 11:00 UTC by Kyle M Hall
Modified: 2016-12-05 21:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15532 - Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold (43.44 KB, patch)
2016-01-09 11:05 UTC, Kyle M Hall
Details | Diff | Splinter 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 (43.53 KB, patch)
2016-01-17 01:52 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold (44.24 KB, patch)
2016-03-16 13:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15532 [QA Followup] - Remove unecessary lines from unit test (1.57 KB, patch)
2016-04-18 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15532 [QA Followup] - Remove unused variable (935 bytes, patch)
2016-04-27 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15532: Add ability to allow only items whose home/holding branch matches the hold's pickup branch to fill a given hold (44.34 KB, patch)
2016-04-28 12:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15532 [QA Followup] - Remove unecessary lines from unit test (1.67 KB, patch)
2016-04-28 12:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15532 [QA Followup] - Remove unused variable (1.01 KB, patch)
2016-04-28 12:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-01-09 11:00:25 UTC
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.
Comment 1 Kyle M Hall 2016-01-09 11:05:38 UTC Comment hidden (obsolete)
Comment 2 Héctor Eduardo Castro Avalos 2016-01-17 01:52:25 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2016-03-16 13:28:11 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-04-06 12:36:42 UTC
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
Comment 5 Kyle M Hall 2016-04-18 16:50:54 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2016-04-27 17:03:18 UTC
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
Comment 7 Kyle M Hall 2016-04-27 17:19:20 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2016-04-28 09:45:48 UTC
QA: Looking here now. Brr Holds
Comment 9 Marcel de Rooy 2016-04-28 12:08:48 UTC
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>
Comment 10 Marcel de Rooy 2016-04-28 12:08:52 UTC
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>
Comment 11 Marcel de Rooy 2016-04-28 12:08:56 UTC
Created attachment 50903 [details] [review]
Bug 15532 [QA Followup] - Remove unused variable

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2016-04-28 12:11:12 UTC
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
Comment 13 Kyle M Hall 2016-04-29 10:25:39 UTC
Pushed to Master - should be in the May 2016 - Release.   Thanks!