Bug 5801

Summary: C4::Circulation::_GetCircControlBranch should not be used in opac-reserve.pl
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: chris, gmcharlt, jcamins, katrin.fischer, koha.sekjal, kyle
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 5801 - C4::Circulation::_GetCircControlBranch doesn't work in opac-reserve.pl
Bug 5801 - C4::Circulation::_GetCircControlBranch wrongly used in opac-reserve.pl
[SIGNED-OFF] Bug 5801 - C4::Circulation::_GetCircControlBranch wrongly used in opac-reserve.pl

Description Fridolin Somers 2011-02-23 13:08:11 UTC
In opac-reserve.pl :
    my $branch = C4::Circulation::_GetCircControlBranch($itemLoopIter, $borr);

This doesn't work because _GetCircControlBranch looks for homebranch nor holdingbranch.
But $itemLoopIter doesn't contain those.

I propose : 
    my $branch = C4::Circulation::_GetCircControlBranch( $itemInfo , $borr);
Comment 1 Owen Leonard 2011-02-25 15:46:28 UTC
Can you describe the problem this bug causes and how to test the proposed solution?
Comment 2 Fridolin Somers 2011-02-25 16:09:56 UTC
(In reply to comment #1)
> Can you describe the problem this bug causes and how to test the proposed
> solution?

You define a quota of holds in the issuing rules of your branch and borrower category.
Despite this, you can't perform a hold.

With the correction, the hold is possible.
Comment 3 Fridolin Somers 2011-02-25 16:13:07 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Can you describe the problem this bug causes and how to test the proposed
> > solution?
> 
> You define a quota of holds in the issuing rules of your branch and borrower
> category.
> Despite this, you can't perform a hold.
> 
> With the correction, the hold is possible.

System pref ReservesControlBranch = ItemHomeLibrary
Comment 4 Owen Leonard 2011-02-25 16:46:31 UTC
> You define a quota of holds in the issuing rules of your branch and borrower
> category.
> Despite this, you can't perform a hold.

Can you be more specific about the conditions under which one cannot place a hold? I don't have any problems placing a hold with or without this change. 

In fact, I would expect the rule limiting [Library 1] Staff patrons to 2 holds would prevent me from placing more than 2 holds on items from [Library 1] but I'm not being prevented from doing so.
Comment 5 Ian Walls 2011-12-05 21:23:49 UTC
Just looking at the code, it's pretty clear there are some problems;

_GetCircControlBranch is an internal function (hence the _ at the beginning), so it shouldn't be called in this script.

Also, as Fridoyln points out, the hashref passed doesn't contain the necessary information, so results are inaccurate.

And shouldn't we be using ReservesControlBranch, not CircControl and HomeOrHoldingBranch, to get our rules for a hold?
Comment 6 Kyle M Hall 2012-10-01 19:16:35 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2012-10-02 11:38:10 UTC
Note, I think there is another improper use of C4::Circulation::_GetCircControlBranch in C4::Reserves::CheckReserves
Comment 8 Owen Leonard 2012-11-02 13:21:04 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Can you describe the problem this bug causes and how to test the proposed
> > solution?
> 
> You define a quota of holds in the issuing rules of your branch and borrower
> category.
> Despite this, you can't perform a hold.

I still don't understand how to reproduce this bug. I have  ReservesControlBranch set to 'ItemHomeLibrary' and I don't have any problems placing a hold without this patch.

Kyle, were you able to reproduce the problem?
Comment 9 Kyle M Hall 2012-11-02 17:35:26 UTC
(In reply to comment #8) 
> Kyle, were you able to reproduce the problem?

Yes, I was able to reproduce the problem, but that was a while back. I think this patch is still valid because we shouldn't be using _GetCircControlBranch here anyway, as this is reserves related.
Comment 10 Owen Leonard 2012-11-02 17:44:02 UTC
(In reply to comment #9)
> I think this patch is still valid because we shouldn't be using
> _GetCircControlBranch here anyway, as this is reserves related.

Does the patch solve a problem with the way holds work? If so, how does one reproduce the problem and test the patch?
Comment 11 Kyle M Hall 2012-11-05 13:17:23 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > I think this patch is still valid because we shouldn't be using
> > _GetCircControlBranch here anyway, as this is reserves related.
> 
> Does the patch solve a problem with the way holds work? If so, how does one
> reproduce the problem and test the patch?

The problem is that CircControl is meant to return the library code to be used to determine which circulation policy applies to a transaction, *not* reserves policy.

So, a library has set CircControl to use the checkout and fines rules of the library the item is from. 
The library has also set ReservesControlBranch to check the patron's home library to see if the patron can place a hold on the item.

In this case, the reserves rules used will be those of the item's home library, not those of the patron's home library. This seems like a fairly serious bug. Koha is not doing what it says it will do.
Comment 12 Chris Cormack 2012-11-28 06:34:59 UTC
Updating the bug title to more accurately reflect the problem.
Comment 13 Chris Cormack 2012-11-28 06:39:31 UTC Comment hidden (obsolete)
Comment 14 Fridolin Somers 2012-11-28 16:57:42 UTC
Should _GetCircControlBranch also be removed from C4::Reserves::CheckReserves ?
Comment 15 Katrin Fischer 2012-11-29 07:37:53 UTC
Created attachment 13755 [details] [review]
[SIGNED-OFF] Bug 5801 - C4::Circulation::_GetCircControlBranch wrongly used in opac-reserve.pl

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and perlcritic pass.

Tests done:
holds policy vs. circulation rules seemed to make the difference for me,
I am not totally sure why that is, but see no reason to fail this patch as
results are ok.

System preferences
- CircControlBranch = the library the item is from
- ReservesControlBranch = patron's home library

Circulation rules
- Library A - Holds policy: Books can not be put on hold.
- Library B - Holds policy: Books can be put on hold. (no special setting needed)

Catalog data
- A record with an item for library B, itemtype Books.

Test without patch:

1) Patron A (home library A) places a hold in OPAC on an item from library B.

Should be: hold is NOT allowed.
Before patch: hold IS allowed.
After patch: hold is NOT allowed.
OK

2) ReserveControlbranch = item's home library
Patron A (home library A) places a hold in OPAC on an item from library B.

Should be: hold is allowed.
Before patch: hold is allowed.
After patch: hold is allowed.
OK

3) ReservesControlBranch = patron's home library
Circulation rules: Remove hold policy for library B.
Library A: no holds allowed for books using 'Holds allowed (count)' = 0
Library B: 5 holds allowed for books using 'Holds allowed (count)' = 5
Patron A (home library A) places a hold in OPAC on an item from library B.

Should be: hold is NOT allowed
Before patch: hold is NOT allowed
After patch:  hold is NOT allowed
OK

4) ReserveControlbranch = item's home library

Should be: hold is allowed
Before patch: hold is allowed
After patch:  hold is allowed
OK
Comment 16 Jared Camins-Esakov 2012-11-30 03:04:14 UTC
This patch has been pushed to master.
Comment 17 Chris Cormack 2012-11-30 07:25:02 UTC
Pushed to 3.10.x and 3.8.x, will be in 3.10.1 and 3.8.8