Attachment 27487 Details for Bug 7981 – [SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref

[patch] [SIGNED-OFF] Bug 7981 - Transfer message when returning doesn't properly respect HomeOrHoldingBranchReturn syspref

SIGNED-OFF-Bug-7981---Transfer-message-when-return.patch (text/plain), 13.01 KB, created by Kyle M Hall (khall) on 2014-04-23 14:15:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2014-04-23 14:15:01 UTC

Size: 13.01 KB

patch

obsolete

>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 ) %]
>-    
>+    
> 	<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

Actions: View | Diff | Splinter Review

Attachments on bug 7981: 26723 | 27487 | 27950 | 27992 | 28199 | 33455 | 36615 | 37724 | 37725 | 37726 | 38151 | 38174 | 38176 | 38177 | 38178 | 38179 | 38180 | 38237 | 38238 | 38241 | 38242 | 38243 | 38244 | 38245 | 39504 | 39848 | 39849 | 39850 | 39851 | 39852 | 39853