Summary: | C4::Circulation::_GetCircControlBranch should not be used in opac-reserve.pl | ||
---|---|---|---|
Product: | Koha | Reporter: | Fridolin Somers <fridolin.somers> |
Component: | Hold requests | Assignee: | 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
Can you describe the problem this bug causes and how to test the proposed solution? (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. (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 > 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.
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? Created attachment 12625 [details] [review] Bug 5801 - C4::Circulation::_GetCircControlBranch doesn't work in opac-reserve.pl Note, I think there is another improper use of C4::Circulation::_GetCircControlBranch in C4::Reserves::CheckReserves (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? (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. (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? (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. Updating the bug title to more accurately reflect the problem. Created attachment 13718 [details] [review] Bug 5801 - C4::Circulation::_GetCircControlBranch wrongly used in opac-reserve.pl Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Should _GetCircControlBranch also be removed from C4::Reserves::CheckReserves ? 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 This patch has been pushed to master. Pushed to 3.10.x and 3.8.x, will be in 3.10.1 and 3.8.8 |