Bug 6151 - IndependantBranches and HomeOrHoldingBranchReturn can prevent items from being checked in
Summary: IndependantBranches and HomeOrHoldingBranchReturn can prevent items from bein...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P3 major (vote)
Assignee: Jonathan Druart
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 19:42 UTC by Ian Walls
Modified: 2013-12-05 20:05 UTC (History)
6 users (show)

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


Attachments
Proposed Patch (10.34 KB, patch)
2011-12-01 21:36 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 6151: Add AllowReturnToBranch system preference (10.43 KB, patch)
2011-12-11 00:13 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6151: Add AllowReturnToBranch system preference (10.54 KB, patch)
2012-08-01 12:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6151: Add AllowReturnToBranch system preference (10.59 KB, patch)
2012-08-02 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table. (3.42 KB, patch)
2012-08-02 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table. (3.51 KB, patch)
2012-08-07 10:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6151 - Followup - Change 'itemnumber' to 'barcode' (988 bytes, patch)
2012-08-13 20:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6151 - Followup - Change 'itemnumber' to 'barcode' (1.03 KB, patch)
2012-08-14 07:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6151: Add AllowReturnToBranch system preference (10.66 KB, patch)
2012-09-14 08:48 UTC, Mason James
Details | Diff | Splinter Review
Bug 6151 - Followup - Display message for failed returns on circulation.pl when returning via issues table. (3.10 KB, patch)
2012-09-14 08:53 UTC, Mason James
Details | Diff | Splinter Review
Bug 6151 - Followup - Change 'itemnumber' to 'barcode' (1.09 KB, patch)
2012-09-14 08:57 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2011-04-08 19:42:25 UTC
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.
Comment 1 D Ruth Holloway 2011-06-26 18:29:43 UTC
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")} )
           )) {
Comment 2 Ian Walls 2011-12-01 19:43:54 UTC
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.
Comment 3 Ian Walls 2011-12-01 21:36:04 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-12-11 00:13:19 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2011-12-14 15:26:25 UTC
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.
Comment 6 Ian Walls 2011-12-14 22:18:12 UTC
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?
Comment 7 Jonathan Druart 2011-12-15 08:54:58 UTC
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 :-/
Comment 8 Kyle M Hall 2012-08-01 12:40:05 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-08-02 12:27:01 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-08-02 12:27:49 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2012-08-07 10:19:19 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-08-07 12:23:39 UTC
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.
Comment 13 Kyle M Hall 2012-08-13 20:02:12 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2012-08-14 07:13:22 UTC Comment hidden (obsolete)
Comment 15 Mason James 2012-09-14 08:48:39 UTC
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>
Comment 16 Mason James 2012-09-14 08:53:48 UTC
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>
Comment 17 Mason James 2012-09-14 08:57:12 UTC
Created attachment 12210 [details] [review]
Bug 6151 - Followup - Change 'itemnumber' to 'barcode'

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Mason James 2012-09-14 09:04:19 UTC
(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
Comment 19 Paul Poulain 2012-09-14 14:35:17 UTC
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)
Comment 20 Kyle M Hall 2012-09-14 17:32:50 UTC
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)
Comment 21 Paul Poulain 2012-09-17 17:25:41 UTC
Patch pushed to master.
I've revised my judgement, it's a very handy feature !
Comment 22 Chris Cormack 2013-04-20 22:37:21 UTC
Released in 3.10.0