From ec1360d5c981e317cbdec93fca5216b353b3ad1c Mon Sep 17 00:00:00 2001
From: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Date: Tue, 1 Apr 2014 12:00:55 +0200
Subject: [PATCH] [SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref

This patch removes HomeOrHoldingBranchReturn syspref and makes circ/returns.pl respect branch
circulation rules from C4::Circulation::GetBranchItemRule. Also transfer slip notice should reflect this.

Default should always be to return item to home branch.

Test plan:
- make sure syspref 'AutomaticItemReturn' is set to 'false'
- unset 'Default checkout, hold and return policy' or set 'Return policy' to 'Item returns home'
- checkout an item and do a checkin from different branch than items homebranch
- verify that you're prompted with a transfer message to item's home branch and that print slip matches

- set 'Return policy' to 'Item returns to issuing library'
- do a checkout and a checkin from branch different than item's home branch
- verify that you're not prompted with a transfer message and that holding library is your current branch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 C4/Circulation.pm                                  |   35 +++++++++++--------
 circ/returns.pl                                    |   18 +++++++---
 installer/data/mysql/updatedatabase.pl             |    9 +++++
 .../en/modules/admin/preferences/circulation.pref  |    7 ----
 .../intranet-tmpl/prog/en/modules/circ/returns.tt  |   10 +++---
 5 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index bd9e1cc..1cb4986 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1580,6 +1580,7 @@ holdallowed => Hold policy for this branch and itemtype. Possible values:
 returnbranch => branch to which to return item.  Possible values:
   noreturn: do not return, let item remain where checked in (floating collections)
   homebranch: return to item's home branch
+  holdingbranch: return to issuer branch
 
 This searches branchitemrules in the following order:
 
@@ -1695,6 +1696,14 @@ fields from the reserves table of the Koha database, and
 C<biblioitemnumber>. It also has the key C<ResFound>, whose value is
 either C<Waiting>, C<Reserved>, or 0.
 
+=item C<WasReturned>
+
+Value 1 if return is successful.
+
+=item C<NeedsTransfer>
+
+If AutomaticItemReturn is disabled, return branch is given as value of NeedsTransfer.
+
 =back
 
 C<$iteminformation> is a reference-to-hash, giving information about the
@@ -1726,7 +1735,6 @@ sub AddReturn {
         return (0, { BadBarcode => $barcode }); # no barcode means no item or borrower.  bail out.
     }
     my $issue  = GetItemIssue($itemnumber);
-#   warn Dumper($iteminformation);
     if ($issue and $issue->{borrowernumber}) {
         $borrower = C4::Members::GetMemberDetails($issue->{borrowernumber})
             or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existant borrowernumber '$issue->{borrowernumber}'\n"
@@ -1746,18 +1754,18 @@ sub AddReturn {
     my $item = GetItem($itemnumber) or die "GetItem($itemnumber) failed";
         # full item data, but no borrowernumber or checkout info (no issue)
         # we know GetItem should work because GetItemnumberFromBarcode worked
-    my $hbr      = GetBranchItemRule($item->{'homebranch'}, $item->{'itype'})->{'returnbranch'} || "homebranch";
+    my $hbr = GetBranchItemRule($item->{'homebranch'}, $item->{'itype'})->{'returnbranch'} || "homebranch";
         # get the proper branch to which to return the item
-    $hbr = $item->{$hbr} || $branch ;
+    my $returnbranch = $item->{$hbr} || $branch ;
         # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)
 
     my $borrowernumber = $borrower->{'borrowernumber'} || undef;    # we don't know if we had a borrower or not
 
     # check if the book is in a permanent collection....
     # FIXME -- This 'PE' attribute is largely undocumented.  afaict, there's no user interface that reflects this functionality.
-    if ( $hbr ) {
+    if ( $returnbranch ) {
         my $branches = GetBranches();    # a potentially expensive call for a non-feature.
-        $branches->{$hbr}->{PE} and $messages->{'IsPermanent'} = $hbr;
+        $branches->{$returnbranch}->{PE} and $messages->{'IsPermanent'} = $returnbranch;
     }
 
     # check if the return is allowed at this branch
@@ -1929,21 +1937,18 @@ sub AddReturn {
         DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
     }
 
-    # 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 .
-
-    if (($doreturn or $messages->{'NotIssued'}) and !$resfound and ($branch ne $hbr) and not $messages->{'WrongTransfer'}){
-        if ( C4::Context->preference("AutomaticItemReturn"    ) or
+    # Transfer to returnbranch if Automatic transfer set or append message NeedsTransfer
+    if (($doreturn or $messages->{'NotIssued'}) and !$resfound and ($branch ne $returnbranch) and not $messages->{'WrongTransfer'}){
+        if  (C4::Context->preference("AutomaticItemReturn"    ) or
             (C4::Context->preference("UseBranchTransferLimits") and
-             ! IsBranchTransferAllowed($branch, $hbr, $item->{C4::Context->preference("BranchTransferLimitsType")} )
+             ! IsBranchTransferAllowed($branch, $returnbranch, $item->{C4::Context->preference("BranchTransferLimitsType")} )
            )) {
-            $debug and warn sprintf "about to call ModItemTransfer(%s, %s, %s)", $item->{'itemnumber'},$branch, $hbr;
+            $debug and warn sprintf "about to call ModItemTransfer(%s, %s, %s)", $item->{'itemnumber'},$branch, $returnbranch;
             $debug and warn "item: " . Dumper($item);
-            ModItemTransfer($item->{'itemnumber'}, $branch, $hbr);
+            ModItemTransfer($item->{'itemnumber'}, $branch, $returnbranch);
             $messages->{'WasTransfered'} = 1;
         } else {
-            $messages->{'NeedsTransfer'} = 1;   # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
+            $messages->{'NeedsTransfer'} = $returnbranch;
         }
     }
     return ( $doreturn, $messages, $issue, $borrower );
diff --git a/circ/returns.pl b/circ/returns.pl
index 4abac2e..9506885 100755
--- a/circ/returns.pl
+++ b/circ/returns.pl
@@ -223,10 +223,7 @@ if ($barcode) {
 #
 # save the return
 #
-    ( $returned, $messages, $issueinformation, $borrower ) =
-      AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode);     # do the return
-    my $homeorholdingbranchreturn = C4::Context->preference('HomeOrHoldingBranchReturn');
-    $homeorholdingbranchreturn ||= 'homebranch';
+
 
     # get biblio description
     my $biblio = GetBiblioFromItemNumber($itemnumber);
@@ -243,10 +240,16 @@ if ($barcode) {
         );
     }
 
+    # make sure return branch respects home branch circulation rules, default to homebranch
+    my $hbr = GetBranchItemRule($biblio->{'homebranch'}, $itemtype)->{'returnbranch'} || "homebranch";
+    my $returnbranch = $biblio->{$hbr} ;
+
     $template->param(
         title            => $biblio->{'title'},
         homebranch       => $biblio->{'homebranch'},
-        homebranchname   => GetBranchName( $biblio->{$homeorholdingbranchreturn} ),
+        homebranchname   => GetBranchName( $biblio->{'homebranch'} ),
+        returnbranch     => $returnbranch,
+        returnbranchname => GetBranchName( $returnbranch ),
         author           => $biblio->{'author'},
         itembarcode      => $biblio->{'barcode'},
         itemtype         => $biblio->{'itemtype'},
@@ -261,6 +264,9 @@ if ($barcode) {
         barcode => $barcode,
     );
 
+    ( $returned, $messages, $issueinformation, $borrower ) =
+      AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode);     # do the return
+
     if ($returned) {
         my $time_now = DateTime->now( time_zone => C4::Context->tz )->truncate( to => 'minute');
         my $duedate = $issueinformation->{date_due}->strftime('%Y-%m-%d %H:%M');
@@ -326,7 +332,7 @@ if ( $messages->{'WasTransfered'} ) {
 if ( $messages->{'NeedsTransfer'} ){
     $template->param(
         found          => 1,
-        needstransfer  => 1,
+        needstransfer  => $messages->{'NeedsTransfer'},
         itemnumber     => $itemnumber,
     );
 }
diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
index 8917081..c837cdb 100755
--- a/installer/data/mysql/updatedatabase.pl
+++ b/installer/data/mysql/updatedatabase.pl
@@ -8202,6 +8202,15 @@ if ( CheckVersion($DBversion) ) {
     SetVersion($DBversion);
 }
 
+$DBversion = "3.15.00.XXX";
+if ( CheckVersion($DBversion) ) {
+    $dbh->do(q{
+        DELETE FROM systempreferences WHERE variable = 'HomeOrHoldingBranchReturn';
+    });
+    print "Upgrade to $DBversion done (Bug 7981 - Transfer message when returning item)\n";
+    SetVersion($DBversion);
+}
+
 =head1 FUNCTIONS
 
 =head2 TableExists($table)
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
index 6282c0f..bd4f9ab 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ -323,13 +323,6 @@ Circulation:
                   confirmation: Ask for confirmation
             - when checking out to a borrower that has overdues outstanding	    
         -
-            - On checkin route the returned item to
-            - pref: HomeOrHoldingBranchReturn
-              type: choice
-              choices:
-                  homebranch: the library the item is from.
-                  holdingbranch: the library the item was checked out from.
-        -
             - "When issuing an item that has been marked as lost, "
             - pref: IssueLostItem
               choices:
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
index 14be5c8..1f011af 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
@@ -201,9 +201,9 @@ $(document).ready(function () {
     [% END %]
 
     [% IF ( transfer ) %]
-    <!-- transfer: item with no reservation, must be returned to its home library -->
+    <!-- transfer: item with no reservation, must be returned according to home library circulation rules -->
 	<div id="return1" class="dialog message">
-            <h3>Please return <a href="/cgi-bin/koha/catalogue/detail.pl?type=intra&amp;biblionumber=[% itembiblionumber %]">[% title or "item" |html %]</a> to [% homebranchname %]<br/>( <a href="#" onclick="Dopop('transfer-slip.pl?transferitem=[% itemnumber %]&amp;branchcode=[% homebranch %]&amp;op=slip'); return true;">Print slip</a> )</h3>
+            <h3>Please return <a href="/cgi-bin/koha/catalogue/detail.pl?type=intra&amp;biblionumber=[% itembiblionumber %]">[% title or "item" |html %]</a> to [% returnbranchname %]<br/>( <a href="#" onclick="Dopop('transfer-slip.pl?transferitem=[% itemnumber %]&amp;branchcode=[% returnbranch %]&amp;op=slip'); return true;">Print slip</a> )</h3>
         </div>
         [% IF ( soundon ) %]
         <audio src="[% interface %]/[% theme %]/sound/opening.ogg" autoplay="autoplay" autobuffer="autobuffer"></audio>
@@ -215,15 +215,15 @@ $(document).ready(function () {
         [% IF ( soundon ) %]
         <audio src="[% interface %]/[% theme %]/sound/opening.ogg" autoplay="autoplay" autobuffer="autobuffer"></audio>
         [% END %]
-	<div id="item-transfer" class="dialog message"><h3> This item needs to be transferred to [% homebranchname %]</h3>
+ <div id="item-transfer" class="dialog message"><h3> This item needs to be transferred to [% returnbranchname %]</h3>
     Transfer now?<br />
     <form method="post" action="returns.pl" name="mainform" id="mainform">
     [% IF itemnumber %]
-        <input type="submit" name="dotransfer" value="Yes, Print slip" class="print" onclick="Dopop('transfer-slip.pl?transferitem=[% itemnumber %]&amp;&amp;branchcode=[% homebranch %]&amp;op=slip'); return true;" />
+        <input type="submit" name="dotransfer" value="Yes, Print slip" class="print" onclick="Dopop('transfer-slip.pl?transferitem=[% itemnumber %]&amp;&amp;branchcode=[% returnbranch %]&amp;op=slip'); return true;" />
     [% END %]
 	<input type="submit" name="dotransfer" value="Yes" class="submit" />
 	<input type="submit" name="notransfer" value="No" class="submit" />
-	<input type="hidden" name="tobranch" value="[% homebranch %]" />
+    <input type="hidden" name="tobranch" value="[% returnbranch %]" />
 	<input type="hidden" name="transferitem" value="[% itemnumber %]" />
         <input type="hidden" name="exemptfine" value="[% exemptfine %]" />
         <input type="hidden" name="dropboxmode" value="[% dropboxmode %]" />
-- 
1.7.2.5