Bug 10272 - CheckReserves returns not respecting ReservesControlBranch
Summary: CheckReserves returns not respecting ReservesControlBranch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-17 12:03 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (3.96 KB, patch)
2013-05-17 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (4.03 KB, patch)
2013-05-17 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (4.02 KB, patch)
2013-05-17 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.24 KB, patch)
2013-06-12 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.41 KB, patch)
2013-06-13 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.47 KB, patch)
2013-06-14 13:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.46 KB, patch)
2013-06-17 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.51 KB, patch)
2013-07-06 08:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.42 KB, patch)
2013-09-04 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch (5.63 KB, patch)
2013-09-07 22:05 UTC, Katrin Fischer
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-05-17 12:03:11 UTC
CheckReserves is using the CircControl system preference to determine what patrons an item can fill a hold for. It should be using ReservesControlBranch instead.
Comment 1 Kyle M Hall 2013-05-17 12:10:23 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-17 12:12:55 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-05-17 15:18:21 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-05-22 09:28:08 UTC
Kyle,

The new routine GetReservesControlBranch needs a unit test.

Marked as Failed QA.
Comment 5 Kyle M Hall 2013-06-12 15:34:27 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-06-12 15:38:26 UTC
Good catch! Unit test added!

(In reply to comment #4)
> Kyle,
> 
> The new routine GetReservesControlBranch needs a unit test.
> 
> Marked as Failed QA.
Comment 7 Jonathan Druart 2013-06-13 09:58:39 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-06-13 09:59:18 UTC
Last patch changes the expected number of tests (4 to 6).
Comment 9 Kyle M Hall 2013-06-14 12:14:23 UTC
Jonathan, did you mean to sign-off on this patch, or just revise the number of tests? Either way, thanks for fixing that!

Kyle

(In reply to comment #8)
> Last patch changes the expected number of tests (4 to 6).
Comment 10 Jonathan Druart 2013-06-14 12:19:26 UTC
(In reply to comment #9)
> Jonathan, did you mean to sign-off on this patch, or just revise the number
> of tests? Either way, thanks for fixing that!

No I didn't test the patch, just fix the unit tests.
Comment 11 Julian Maurice 2013-06-14 13:39:47 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2013-06-14 13:40:19 UTC
Thanks for the test plan, Kyle :)
Tested OK.
Unit tests pass.

Signed off.
Comment 13 Julian Maurice 2013-06-14 15:08:27 UTC
In fact it seems that it don't work.
I retested it with Jonathan and at step 4, the hold returned is still the same (the one for patron at library B)
But why should the hold for library A's patron be returned whereas it has the lowest priority ?
Comment 14 Kyle M Hall 2013-06-17 16:19:09 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-06-17 16:20:33 UTC
I forgot a step in the test plan! ReservesControlBranch needs to be set to "item's home library".

(In reply to comment #13)
> In fact it seems that it don't work.
> I retested it with Jonathan and at step 4, the hold returned is still the
> same (the one for patron at library B)
> But why should the hold for library A's patron be returned whereas it has
> the lowest priority ?
Comment 16 Chris Cormack 2013-07-06 08:46:39 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-07-13 21:45:17 UTC
Kyle, can you please rebase and switch back to signed off?
Comment 18 Kyle M Hall 2013-09-04 13:14:29 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-09-07 22:05:35 UTC
Created attachment 20864 [details] [review]
[PASSED QA] Bug 10272 - CheckReserves returns not respecting ReservesControlBranch

CheckReserves is using the CircControl system preference to determine what
patrons an item can fill a hold for. It should be using ReservesControlBranch
instead.

Test Plan:
1) Set ReservesControlBranch to "item's home library".

2) Create an item at Library A, place holds for it for patrons at
   Library B, Library C, and Library A in that order,
   for pickup at the patrons home library.

3) Make sure the holds policy for Library A is set to
   Hold Policy = "From home library" and
   Return Policy = "Item returns home".

   Make sure the holds policies for the other libraries are set to
   Hold Policy = "From any library".

4) Check the item in at Library C, the hold for the patron at Library B
   should pop up, even though it's in violation of the circulation rules.
   Don't click the confirm button!

5) Apply this patch, and reload the page,
   now the hold listed should be for the last hold,
   the hold for the patron at Library A, which is correct.

This patch adds the subroutine C4::Reserves::GetReservesControlBranch as
an equivilent to C4::Circulation::_GetCircControlBranch.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Fixed POD so that arguments and explanation match (C<$item>).
Also tested opac-reserves.pl for regressions.
Passes all tests, QA script, and Reserves.t.
Comment 20 Galen Charlton 2013-09-08 01:24:45 UTC
Pushed to master, along with a follow-up that adds a regression test.  Thanks, Kyle!

As a side-note, I care a /lot/ about there being regression tests, especially for changes that touch circulation and/or hold requests.  While the unit test for the new routine was necessary, it wasn't sufficient.
Comment 21 Tomás Cohen Arazi 2013-10-08 14:13:08 UTC
This patch has been pushed to 3.12.x, will be in 3.12.6.

Thanks Kyle!
Comment 22 Bernardo Gonzalez Kriegel 2013-10-13 22:54:16 UTC
Pushed to 3.10.x, will be in 3.10.12