Bug 11126 - Make the holds system optionally give precedence to local holds
Summary: Make the holds system optionally give precedence to local holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 12004 20747
  Show dependency treegraph
 
Reported: 2013-10-23 15:53 UTC by Kyle M Hall
Modified: 2018-05-24 19:42 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11126 - Make the holds system optionally give precedence to local holds (14.37 KB, patch)
2013-10-23 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.33 KB, patch)
2013-10-23 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.30 KB, patch)
2013-10-30 08:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.32 KB, patch)
2013-11-07 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.38 KB, patch)
2013-11-07 16:09 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
test case home library/pickup library (148.09 KB, application/octetstream)
2014-01-11 16:45 UTC, Katrin Fischer
Details
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.40 KB, patch)
2014-02-24 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.41 KB, patch)
2014-05-09 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.40 KB, patch)
2014-05-09 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.53 KB, patch)
2014-05-12 17:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.40 KB, patch)
2014-05-12 17:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.53 KB, patch)
2014-07-28 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.40 KB, patch)
2014-07-28 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11126: qa follow-up (2.15 KB, patch)
2014-10-10 18:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.53 KB, patch)
2014-10-10 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.38 KB, patch)
2014-10-10 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126: qa follow-up (2.15 KB, patch)
2014-10-10 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126 - Make the holds system optionally give precedence to local holds (14.59 KB, patch)
2014-10-10 19:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority (1.44 KB, patch)
2014-10-10 19:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11126: qa follow-up (2.21 KB, patch)
2014-10-10 19:13 UTC, Jonathan Druart
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 2013-10-23 15:53:34 UTC
This feature would allow libraries to specify that, when items a returned, a local hold may be given priority for fulfillment even though it is of lower priority in the list of unfilled holds.
Comment 1 Kyle M Hall 2013-10-23 16:19:25 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-10-23 16:21:42 UTC Comment hidden (obsolete)
Comment 3 I'm just a bot 2013-10-30 07:57:09 UTC
Applying: Bug 11126 - Make the holds system optionally give precedence to local holds
Using index info to reconstruct a base tree...
M	installer/data/mysql/updatedatabase.pl
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Patch failed at 0001 Bug 11126 - Make the holds system optionally give precedence to local holds
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 4 Chris Cormack 2013-10-30 08:00:20 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-11-07 13:47:27 UTC Comment hidden (obsolete)
Comment 6 Biblibre Sandboxes 2013-11-07 16:09:15 UTC
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 7 Biblibre Sandboxes 2013-11-07 16:09:36 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2014-01-06 14:19:58 UTC
Comment on attachment 22784 [details] [review]
Bug 11126 - Make the holds system optionally give precedence to local holds

Review of attachment 22784 [details] [review]:
-----------------------------------------------------------------

Ok, starting with a code review :)

Looks good so far:
- system preferences are added correctly
- new feature is off by default
- unit tests are provided

Only one small issue with one of the comments:

::: C4/Reserves.pm
@@ +915,5 @@
>                  return ( "Waiting", $res, \@reserves ); # Found it
>              } else {
> +                # Lazy fetch for borrower and item. We only need to know about the patron and item
> +                # each and every time if we are using LocalHoldsPriority. This is a great place to
> +                # leverage the inherent lazy fetching of DBIx::Class.

I think DBIx::Class is currently not yet used here, so the comment could be fixed in a follow up.
Comment 9 Katrin Fischer 2014-01-06 14:21:49 UTC
Coninuing testing...
Comment 10 Katrin Fischer 2014-01-11 16:45:18 UTC
Created attachment 24195 [details]
test case home library/pickup library

Kyle, I have double checked everything, but the test case described in the attached PDF doesn't seem to give the correct result to me. Could you please take a look?

When I turn off LocalHoldsPriority the hold for R is triggered.
Comment 11 Kyle M Hall 2014-02-20 18:16:53 UTC
(In reply to Katrin Fischer from comment #10)
> Created attachment 24195 [details]
> test case home library/pickup library
> 
> Kyle, I have double checked everything, but the test case described in the
> attached PDF doesn't seem to give the correct result to me. Could you please
> take a look?
> 
> When I turn off LocalHoldsPriority the hold for R is triggered.

I think I made the assumption that the holds would be iterated over in order of priority. I'll try to reproduce your results and see what I can do!
Comment 12 Kyle M Hall 2014-02-24 16:34:10 UTC Comment hidden (obsolete)
Comment 13 Joel Sasse 2014-04-10 17:50:48 UTC
Katrin, can you take a look at this again?

Thanks.
Comment 14 Katrin Fischer 2014-04-21 06:11:37 UTC
There are lots of conflicts in Reserves.pm now - with the recent changes to Reserves.pm. Please fix and test.
Comment 15 Kyle M Hall 2014-05-09 16:43:06 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-05-09 16:43:20 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-05-12 17:53:14 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-05-12 17:53:24 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-07-28 15:35:27 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2014-07-28 15:35:36 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-07-30 05:58:35 UTC
Planning to work on this in the next few days.
Comment 22 Jonathan Druart 2014-10-10 18:54:46 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-10-10 18:58:58 UTC
I am not sure about all the consequences of this enhancement. The C4::CheckReserves subroutine is called at several places.

It would have been good to have a better test plan :)

Maybe should we highlight the fact that if will be applied for new holds, not existing ones.

All looks good to me and I pass QA on it.
Comment 24 Jonathan Druart 2014-10-10 18:59:18 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-10-10 18:59:23 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-10-10 18:59:28 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2014-10-10 19:13:29 UTC
Created attachment 32195 [details] [review]
Bug 11126 - Make the holds system optionally give precedence to local holds

This feature will allow libraries to specify that, when an item is returned,
a local hold may be given priority for fulfillment even though it is
of lower priority in the list of unfilled holds.

This feature has three settings:
* LocalHoldsPriority, which enables the feature
* LocalHoldsPriorityPatronControl, which selects for either tha patron's
  home library, or the patron's pickup library for the hold
* LocalHoldsPriorityItemControl, which selects for either the item's
  holding library, or home library.

So, this feature can "give priority for filling holds to
patrons whose (home library|pickup library) matches the item's
(home library|holding library)"

Test Plan:
1) Apply this patch
2) Run t/db_dependent/Holds/LocalHoldsPriority.t

Signed-off-by: Joel Sasse <jsasse@plumcreeklibrary.net>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 28 Jonathan Druart 2014-10-10 19:13:34 UTC
Created attachment 32196 [details] [review]
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 29 Jonathan Druart 2014-10-10 19:13:41 UTC
Created attachment 32197 [details] [review]
Bug 11126: qa follow-up

- use Modern::Perl;
- fix a typo
- remove an old comment

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 30 Tomás Cohen Arazi 2014-11-05 11:35:31 UTC
Patches pushed to master.

Thanks Kyle!