Description
Chris Cormack
2010-05-21 00:50:42 UTC
Bumping up to see if anyone can confirm my previous assumption and close this bug? Is this bug still valid? This bug is still valid in master as of 3.12 beta1. Created attachment 19498 [details] [review] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved I don't think // is in order here. Besides, maybe it should check both holdingbranch and homebranch? (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 ? Yes. But maybe we should consult whoever introduced canreservefromotherbranches. (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> (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. (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? (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. Created attachment 19513 [details] [review] Bug 2394: Use syspref canreservefromotherbranches [Follow up] Check only against item's homebranch I don't want to introduce a new behaviour, just want to make it work ;) I think it is 'IndependentBranches' Created attachment 19544 [details] [review] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved Squashed patch + fix syspref name Created attachment 19545 [details] [review] [3.12.x] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved Patch for 3.12 and before that uses IndependantBranches syspref So what do I do here? Sign off just the first one? Only the syspref name change between the two patches. Maybe you can test only one and sign off both ? Created attachment 19700 [details] [review] [SIGNED-OFF] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Created attachment 19701 [details] [review] [SIGNED-OFF] [3.12.x] Bug 2394: Use syspref canreservefromotherbranches in CanItemBeReserved Signed-off-by: Srdjan <srdjan@catalyst.net.nz> 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 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 ). I've pushed this to master, along with a regression test. Thanks, Julian! This patch has been pushed to 3.12.x, will be in 3.12.5. Thanks Julian and Galen! |