From ec1360d5c981e317cbdec93fca5216b353b3ad1c Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth 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 --- 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. It also has the key C, whose value is either C, C, or 0. +=item C + +Value 1 if return is successful. + +=item C + +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 ) %] - +
-

Please return [% title or "item" |html %] to [% homebranchname %]
( Print slip )

+

Please return [% title or "item" |html %] to [% returnbranchname %]
( Print slip )

[% IF ( soundon ) %] @@ -215,15 +215,15 @@ $(document).ready(function () { [% IF ( soundon ) %] [% END %] -

This item needs to be transferred to [% homebranchname %]

+

This item needs to be transferred to [% returnbranchname %]

Transfer now?
[% IF itemnumber %] - + [% END %] - + -- 1.7.2.5