Bug 2394 - canreservefromotherbranches preference not working
Summary: canreservefromotherbranches preference not working
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P3 major (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-24 06:05 UTC by Chris Cormack
Modified: 2014-12-07 20:02 UTC (History)
6 users (show)

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


Attachments
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.24 KB, patch)
2013-07-09 09:57 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 2394: Use syspref canreservefromotherbranches [Follow up] (872 bytes, patch)
2013-07-10 08:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.22 KB, patch)
2013-07-11 07:19 UTC, Julian Maurice
Details | Diff | Splinter Review
[3.12.x] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.23 KB, patch)
2013-07-11 07:19 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.27 KB, patch)
2013-07-17 00:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] [3.12.x] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.26 KB, patch)
2013-07-17 00:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved (1.35 KB, patch)
2013-08-09 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved [3.12.x] (1.39 KB, patch)
2013-08-09 13:02 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:50:42 UTC


---- Reported by bchurch@ptfs.com 2008-07-24 06:05:49 ----

Patrons can place holds on titles where all attached items belong to other libraries even if the "canreservefromotherbranches" preference is set to OFF. If patrons search the catalogue before logging in they can see all titles. If they find a title where all attached items belong to other libraries, they are allowed to place a hold.

The same applies to the staff client. Staff are also able to access patron records belonging to other libraries.

The IndependentBranches preference is set to ON.



---- Additional Comments From jmf@liblime.com 2008-08-08 14:39:05 ----

I suspect what's going on is you can place title-level holds where only one item exists and it's at another branch. This is System Groups type stuff and suitable for a fix in 3.2



---- Additional Comments From joe.atzberger@liblime.com 2009-01-22 12:22:28 ----

This syspref apparently does not affect the OPAC at all.  At least the description altered to communicate it's incredibly limited area of effect:

~ INDY Branches
~ STAFF interface
~ Item-level holds.

But even that is silly, since a title level hold could end up getting the same item from the other library.  Therefore, canreservefromotherbranches should be removed and obliterated.



---- Additional Comments From jwagner@ptfs.com 2009-11-03 16:37:54 ----

This bug report doesn't show any recent activity -- has anyone been doing any work with it?  I don't agree with Joe Atzberger about removing canreservefromotherbranches.  We do need it to be active and obeyed for sites using Independent Branches -- at present even if Independent Branches is on and canreservefromotherbranches is off, people from one library can place holds (next available or item-specific) which apply to things from other libraries.




---- Additional Comments From nengard@gmail.com 2010-02-07 20:58:35 ----

I have looked at the new preference and it seems to imply that you have to use independent branches with this preference for it to work - can anyone confirm that and close this bug?



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:50 UTC  ---

This bug was previously known as _bug_ 2394 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2394

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was bchurch@ptfs.com.
CC member jwagner@ptfs.com does not have an account here
CC member nicolas.morin@biblibre.com does not have an account here

Comment 1 Nicole C. Engard 2010-06-23 14:43:29 UTC
Bumping up to see if anyone can confirm my previous assumption and close this bug?
Comment 2 Katrin Fischer 2011-04-25 09:25:12 UTC
Is this bug still valid?
Comment 3 Owen Leonard 2013-03-25 15:52:37 UTC
This bug is still valid in master as of 3.12 beta1.
Comment 4 Julian Maurice 2013-07-09 09:57:55 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2013-07-10 04:14:11 UTC
I don't think // is in order here. Besides, maybe it should check both holdingbranch and homebranch?
Comment 6 Julian Maurice 2013-07-10 06:45:48 UTC
(In reply to Srdjan Jankovic from comment #5)
> I don't think // is in order here. Besides, maybe it should check both
> holdingbranch and homebranch?

And allow reserve if one of those is the user's branch ?
Comment 7 Srdjan Jankovic 2013-07-10 07:00:13 UTC
Yes. But maybe we should consult whoever introduced canreservefromotherbranches.
Comment 8 Julian Maurice 2013-07-10 07:26:18 UTC
(In reply to Srdjan Jankovic from comment #7)
> Yes. But maybe we should consult whoever introduced
> canreservefromotherbranches.

This seems to be Chris Cormack

commit e30fb761ec225a7797def0c3ab8d2d8bceea7e0c
Author:     Chris Cormack <chris@snaga.liblime.co.nz>
AuthorDate: Tue Nov 13 21:07:45 2007 -0600
Commit:     Joshua Ferraro <jmf@liblime.com>
CommitDate: Tue Nov 13 21:24:23 2007 -0600

    Adding a system pref so you can stop users from reserving items that dont belong at their branch
    
    Signed-off-by: Chris Cormack <crc@liblime.com>
    Signed-off-by: Joshua Ferraro <jmf@liblime.com>
Comment 9 Chris Cormack 2013-07-10 07:44:48 UTC
(In reply to Julian Maurice from comment #8)
> (In reply to Srdjan Jankovic from comment #7)
> > Yes. But maybe we should consult whoever introduced
> > canreservefromotherbranches.
> 
> This seems to be Chris Cormack
> 
> commit e30fb761ec225a7797def0c3ab8d2d8bceea7e0c
> Author:     Chris Cormack <chris@snaga.liblime.co.nz>
> AuthorDate: Tue Nov 13 21:07:45 2007 -0600
> Commit:     Joshua Ferraro <jmf@liblime.com>
> CommitDate: Tue Nov 13 21:24:23 2007 -0600
> 
>     Adding a system pref so you can stop users from reserving items that
> dont belong at their branch
>     
>     Signed-off-by: Chris Cormack <crc@liblime.com>
>     Signed-off-by: Joshua Ferraro <jmf@liblime.com>

Ouch, what a horrible chapter in my life :(

The original spec was you couldn't reserve things that didn't have the homebranch (where an item belongs) of your borrowers branch. It didn't care what the holdingbranch (where the item is) was. It just only cared about the homebranch.
Comment 10 Julian Maurice 2013-07-10 07:56:39 UTC
(In reply to Chris Cormack from comment #9)
> 
> Ouch, what a horrible chapter in my life :(
> 
> The original spec was you couldn't reserve things that didn't have the
> homebranch (where an item belongs) of your borrowers branch. It didn't care
> what the holdingbranch (where the item is) was. It just only cared about the
> homebranch.

Do you think we should only check against homebranch?
Doesn't IndependantBranches syspref imply that homebranch = holdingbranch anyway?
Comment 11 Chris Cormack 2013-07-10 07:59:00 UTC
(In reply to Julian Maurice from comment #10)
> (In reply to Chris Cormack from comment #9)
> > 
> > Ouch, what a horrible chapter in my life :(
> > 
> > The original spec was you couldn't reserve things that didn't have the
> > homebranch (where an item belongs) of your borrowers branch. It didn't care
> > what the holdingbranch (where the item is) was. It just only cared about the
> > homebranch.
> 
> Do you think we should only check against homebranch?
> Doesn't IndependantBranches syspref imply that homebranch = holdingbranch
> anyway?

It should but it may not be, someone may have transferred one, or manually changed the holding branch. 
The important part is that this syspref is designed to only care about where the book belongs (homebranch) not where it is (holdingbranch)

You can extend it to check the holdingbranch too if you like, but that is certainly a different use case for which it was designed.
Comment 12 Julian Maurice 2013-07-10 08:24:06 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2013-07-10 08:24:45 UTC
I don't want to introduce a new behaviour, just want to make it work ;)
Comment 14 Srdjan Jankovic 2013-07-11 01:41:54 UTC
I think it is 'IndependentBranches'
Comment 15 Julian Maurice 2013-07-11 07:19:01 UTC Comment hidden (obsolete)
Comment 16 Julian Maurice 2013-07-11 07:19:46 UTC Comment hidden (obsolete)
Comment 17 Srdjan Jankovic 2013-07-16 00:26:56 UTC
So what do I do here? Sign off just the first one?
Comment 18 Julian Maurice 2013-07-16 06:50:37 UTC
Only the syspref name change between the two patches.
Maybe you can test only one and sign off both ?
Comment 19 Srdjan Jankovic 2013-07-17 00:01:47 UTC Comment hidden (obsolete)
Comment 20 Srdjan Jankovic 2013-07-17 00:11:03 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-08-09 12:59:10 UTC
Created attachment 20222 [details] [review]
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa, works as advertised
Comment 22 Kyle M Hall 2013-08-09 13:02:32 UTC
Created attachment 20223 [details] [review]
Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved [3.12.x]

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes all valid koha-qa test ( fails for IndependantBranches test ).
Comment 23 Galen Charlton 2013-08-09 18:02:41 UTC
I've pushed this to master, along with a regression test.

Thanks, Julian!
Comment 24 Tomás Cohen Arazi 2013-09-10 16:03:25 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Julian and Galen!