If IndependantBranches is on, and an item is checked into a branch that is not it's current HomeOrHoldBranchReturn branch, a "Wrong Branch" message is given, and the return is denied. This is not always desirable behaviour. For example, some libraries may want an item owned by Branch A but issued from Branch B to be returnable to either A or B (the patron returning the material to A saves the library transit costs), or even to be returnable to Library C if the transit network is robust. More heinous a problem, if an item is issued from Library B, and HomeOrHoldingBranchReturn is set to Homebranch, the patron cannot return the item to Library B. And further, when AutomaticItemReturn is turned on and HomeOrHoldingBranchReturn is HoldingBranch, an item issued from Library B and returned to Library B will not trigger a transfer message to return it to Library A. So, with the two above, if HomeOrHoldingBranchReturn is considered the only adjustable variable (since changing IndependantBranches has many, many consequences, and AutomaticItemReturn is a library policy decision), neither setting will allow for desirable behaviour.
Here's a fix that is working for two sites that are on IndependantBranches with AutomaticItemReturn: In C4/Circulation.pm: # FIXME: make this comment intelligible. #adding message if holdingbranch is non equal a userenv branch to return the document to homebranch #we check, if we don't have reserv or transfert for this document, if not, return it to homebranch . + $hbr = $item->{homebranch}; # since that's what all the following code does if (($doreturn or $messages->{'NotIssued'}) and !$resfound and ($branch ne $hbr) and not $messages->{'WrongTransfer'}){ if ( C4::Context->preference("AutomaticItemReturn" ) or (C4::Context->preference("UseBranchTransferLimits") and ! IsBranchTransferAllowed($branch, $hbr, $item->{C4::Context->preference("BranchTransferLimitsType")} ) )) {
Better solution: A new system preference, AllowReturnToBranch, and a subroutine in C4/Circulation to leverage it. Possible syspref values: homebranch holdingbranch (which would correspond to the issuing branch) homebranch OR holdingbranch <== default if IndependantBranches is ON anywhere <== default if IndependantBranches is OFF (and for fresh installs) New C4::Circulation::CanReturnToBranch subroutine Takes in $item object and current branch Returns two values: 1 if return is allowed or 0 if return is forbidden, and the allowed return branch (defaults to homebranch is option is 'homebranch or holdingbranch') Change the conditional in AddReturn to consult this subroutine instead of comparing $branch to $hbr and looking for IndependantBranches syspref.
Created attachment 6497 [details] [review] Proposed Patch In order to solve the issue of IndependantBranches being incompatible with HomeOrHoldingBranchReturn, this patch changes the mechanism by which the question "can I return this material here?" is answered. Before, the conditions were "if IndependantBranches is on, and this branch isn't HomeOrHoldingBranchReturn for the item, then no, otherwise yes". Now, the question is answered by consulting CanBookBeReturned (new subroutine) New system preference: AllowReturnToBranch Possible values: - anywhere (default for new installs, and for existing systems with IndependantBranches turned off) - homebranch - holdingbranch (which is also the issuing branch in all normal circumstances) - homeorholdingbranch (default for existing systems with IndependantBranches turned on) New subroutine: CanBookBeReturned Input: $item hash (from GetItems), and $branchcode Output: 0 or 1 to indicate "allowed" or not, and an optional message if not allowed. Message is the 'correct' branchcode to return the material to To Test: 1. Install patch and new syspref 2. Check that default value of the preference: - if IndependantBranches was OFF at install time, should be 'anywhere' - if IndependantBranches was ON at install time, should be 'homeorholdingbranch' Case: 'anywhere' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should be successful 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B and Library C Case: 'homebranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to Library A should be successful 4. Repeat step 3 with Library B and Library C. Both should FAIL (returning message to return at A) Case: 'holdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should FAIL (returning message to return at B) 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at B) Case: 'homeorholdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at A)
Created attachment 6701 [details] [review] Bug 6151: Add AllowReturnToBranch system preference In order to solve the issue of IndependantBranches being incompatible with HomeOrHoldingBranchReturn, this patch changes the mechanism by which the question "can I return this material here?" is answered. Before, the conditions were "if IndependantBranches is on, and this branch isn't HomeOrHoldingBranchReturn for the item, then no, otherwise yes". Now, the question is answered by consulting CanBookBeReturned (new subroutine) New system preference: AllowReturnToBranch Possible values: - anywhere (default for new installs, and for existing systems with IndependantBranches turned off) - homebranch - holdingbranch (which is also the issuing branch in all normal circumstances) - homeorholdingbranch (default for existing systems with IndependantBranches turned on) New subroutine: CanBookBeReturned Input: $item hash (from GetItems), and $branchcode Output: 0 or 1 to indicate "allowed" or not, and an optional message if not allowed. Message is the 'correct' branchcode to return the material to To Test: 1. Install patch and new syspref 2. Check that default value of the preference: - if IndependantBranches was OFF at install time, should be 'anywhere' - if IndependantBranches was ON at install time, should be 'homeorholdingbranch' Case: 'anywhere' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should be successful 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B and Library C Case: 'homebranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to Library A should be successful 4. Repeat step 3 with Library B and Library C. Both should FAIL (returning message to return at A) Case: 'holdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should FAIL (returning message to return at B) 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at B) Case: 'homeorholdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at A) Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
I have tested and reviewed this patch. It works but, isn't it better to have a message in the interface when the return fails ? Currently, this patch does not notify the user when an error occurs.
Jonathan, The message is in the WrongBranch param, and displays which branch they SHOULD be returning it to.... are you saying this isn't showing up? Or are you thinking we need an additional error message?
Hi Ian, I think I found the problem :) I fact, I check in from the circ/circulation.pl table, and this script call circ/renewscript and not circ/returns.pl. In this script, error messages are not caught and sent to the template. But renewscript.pl redirects to circulation.pl. Thus to transmit the message, it's not easy nor clean :-/
Created attachment 11250 [details] [review] Bug 6151: Add AllowReturnToBranch system preference In order to solve the issue of IndependantBranches being incompatible with HomeOrHoldingBranchReturn, this patch changes the mechanism by which the question "can I return this material here?" is answered. Before, the conditions were "if IndependantBranches is on, and this branch isn't HomeOrHoldingBranchReturn for the item, then no, otherwise yes". Now, the question is answered by consulting CanBookBeReturned (new subroutine) New system preference: AllowReturnToBranch Possible values: - anywhere (default for new installs, and for existing systems with IndependantBranches turned off) - homebranch - holdingbranch (which is also the issuing branch in all normal circumstances) - homeorholdingbranch (default for existing systems with IndependantBranches turned on) New subroutine: CanBookBeReturned Input: $item hash (from GetItems), and $branchcode Output: 0 or 1 to indicate "allowed" or not, and an optional message if not allowed. Message is the 'correct' branchcode to return the material to To Test: 1. Install patch and new syspref 2. Check that default value of the preference: - if IndependantBranches was OFF at install time, should be 'anywhere' - if IndependantBranches was ON at install time, should be 'homeorholdingbranch' Case: 'anywhere' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should be successful 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B and Library C Case: 'homebranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to Library A should be successful 4. Repeat step 3 with Library B and Library C. Both should FAIL (returning message to return at A) Case: 'holdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should FAIL (returning message to return at B) 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at B) Case: 'homeorholdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at A) Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 11294 [details] [review] Bug 6151: Add AllowReturnToBranch system preference In order to solve the issue of IndependantBranches being incompatible with HomeOrHoldingBranchReturn, this patch changes the mechanism by which the question "can I return this material here?" is answered. Before, the conditions were "if IndependantBranches is on, and this branch isn't HomeOrHoldingBranchReturn for the item, then no, otherwise yes". Now, the question is answered by consulting CanBookBeReturned (new subroutine) New system preference: AllowReturnToBranch Possible values: - anywhere (default for new installs, and for existing systems with IndependantBranches turned off) - homebranch - holdingbranch (which is also the issuing branch in all normal circumstances) - homeorholdingbranch (default for existing systems with IndependantBranches turned on) New subroutine: CanBookBeReturned Input: $item hash (from GetItems), and $branchcode Output: 0 or 1 to indicate "allowed" or not, and an optional message if not allowed. Message is the 'correct' branchcode to return the material to To Test: 1. Install patch and new syspref 2. Check that default value of the preference: - if IndependantBranches was OFF at install time, should be 'anywhere' - if IndependantBranches was ON at install time, should be 'homeorholdingbranch' Case: 'anywhere' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should be successful 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B and Library C Case: 'homebranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to Library A should be successful 4. Repeat step 3 with Library B and Library C. Both should FAIL (returning message to return at A) Case: 'holdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should FAIL (returning message to return at B) 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at B) Case: 'homeorholdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at A) Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 11295 [details] [review] Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table.
Created attachment 11410 [details] [review] Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
My previous test still does not work. Maybe you should replace l.470 (circ/circulation.pl) with: $it->{'return_failed'} = $return_failed{$it->{'barcode'}}; the failedreturn param contains a barcode, not an itemnumber.
Created attachment 11585 [details] [review] Bug 6151 - Followup - Change 'itemnumber' to 'barcode'
Created attachment 11595 [details] [review] Bug 6151 - Followup - Change 'itemnumber' to 'barcode' Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 12208 [details] [review] Bug 6151: Add AllowReturnToBranch system preference In order to solve the issue of IndependantBranches being incompatible with HomeOrHoldingBranchReturn, this patch changes the mechanism by which the question "can I return this material here?" is answered. Before, the conditions were "if IndependantBranches is on, and this branch isn't HomeOrHoldingBranchReturn for the item, then no, otherwise yes". Now, the question is answered by consulting CanBookBeReturned (new subroutine) New system preference: AllowReturnToBranch Possible values: - anywhere (default for new installs, and for existing systems with IndependantBranches turned off) - homebranch - holdingbranch (which is also the issuing branch in all normal circumstances) - homeorholdingbranch (default for existing systems with IndependantBranches turned on) New subroutine: CanBookBeReturned Input: $item hash (from GetItems), and $branchcode Output: 0 or 1 to indicate "allowed" or not, and an optional message if not allowed. Message is the 'correct' branchcode to return the material to To Test: 1. Install patch and new syspref 2. Check that default value of the preference: - if IndependantBranches was OFF at install time, should be 'anywhere' - if IndependantBranches was ON at install time, should be 'homeorholdingbranch' Case: 'anywhere' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should be successful 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B and Library C Case: 'homebranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to Library A should be successful 4. Repeat step 3 with Library B and Library C. Both should FAIL (returning message to return at A) Case: 'holdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should FAIL (returning message to return at B) 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at B) Case: 'homeorholdingbranch' 1. Checkout a Library A book at Library A. Return at Library A should be successful 2. Repeat step 1, returning to Library B. Return should FAIL (returning message to return at A) 3. Checkout a Library A book at Library B. Return to A should be successful 4. Repeat step 3 with Library B. Return should be successful 5. Repeat step 3 with Library C. Return should FAIL (returning message to return at A) Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12209 [details] [review] Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 12210 [details] [review] Bug 6151 - Followup - Change 'itemnumber' to 'barcode' Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
(In reply to comment #17) > Created attachment 12210 [details] [review] > Bug 6151 - Followup - Change 'itemnumber' to 'barcode' > > Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> rebased 3 patches against master '60790d0' all patches apply clean, passing QA... $ koha-qa.pl -c 3 testing 3 commit(s) (applied to commit 60790d0) * 41e46a0 Bug 6151 - Followup - Change 'itemnumber' to 'barcode' circ/circulation.pl * 0b479be Bug 6151 - Followup - Display message for failed returns on circu circ/circulation.pl koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt * 32c3ceb Bug 6151: Add AllowReturnToBranch system preference C4/Circulation.pm admin/systempreferences.pl installer/data/mysql/sysprefs.sql installer/data/mysql/updatedatabase.pl koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref * circ/circulation.pl OK * C4/Circulation.pm OK * admin/systempreferences.pl OK * installer/data/mysql/updatedatabase.pl OK * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt OK
Can't this feature be achieved already with branch transfer limits ? I thought that, with this feature activated, one can do much precise return rules than with just a syspref. Looking at the code, I realize I'm wrong: C4/Circulation.pm, sub AddReturn says: if ($hbr ne $branch && C4::Context->preference("IndependantBranches") && !(C4::Context->preference("canreservefromotherbranches"))){ $messages->{'Wrongbranch'} = { Wrongbranch => $branch, Rightbranch => $hbr, }; $doreturn = 0; # bailing out here - in this case, current desired behavior # is to act as if no return ever happened at all. # FIXME - even in an indy branches situation, there should # still be an option for the library to accept the item # and transfer it to its owning library. return ( $doreturn, $messages, $issue, $borrower ); But shouldn't we use the branch_transfer_limit anyway (and not this new syspref) (just throwing asking the question, not rejecting the patch)
Paul, this is a bug fix, rather than a feature. This patch is to correct a problem that exists in Koha, where using IndependantBranches can result in items not being returnable to a library, even when they should be. This is a problem with returning items, rather than transferring them. Kyle (In reply to comment #19) > Can't this feature be achieved already with branch transfer limits ? > I thought that, with this feature activated, one can do much precise return > rules than with just a syspref. > > Looking at the code, I realize I'm wrong: > C4/Circulation.pm, sub AddReturn says: > > if ($hbr ne $branch && C4::Context->preference("IndependantBranches") && > !(C4::Context->preference("canreservefromotherbranches"))){ > $messages->{'Wrongbranch'} = { > Wrongbranch => $branch, > Rightbranch => $hbr, > }; > $doreturn = 0; > # bailing out here - in this case, current desired behavior > # is to act as if no return ever happened at all. > # FIXME - even in an indy branches situation, there should > # still be an option for the library to accept the item > # and transfer it to its owning library. > return ( $doreturn, $messages, $issue, $borrower ); > > But shouldn't we use the branch_transfer_limit anyway (and not this new > syspref) > > (just throwing asking the question, not rejecting the patch)
Patch pushed to master. I've revised my judgement, it's a very handy feature !
Released in 3.10.0