Bug 11126

Summary: Make the holds system optionally give precedence to local holds
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: gitbot, gmcharlt, gwilliams, jonathan.druart, jsasse, katrin.fischer, lisettepalouse+koha, nengard, nick, sandboxes, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12004, 20747    
Attachments: Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 - Make the holds system optionally give precedence to local holds
test case home library/pickup library
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126: qa follow-up
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126: qa follow-up
Bug 11126 - Make the holds system optionally give precedence to local holds
Bug 11126 [QA Followup] - Make reserves returned by _Findgroupreserve sorted by priority
Bug 11126: qa follow-up

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!