From 2ecc8d52ba3d615e2c8984d673431c5ab0f556a2 Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth Date: Tue, 1 Apr 2014 12:00:55 +0200 Subject: [PATCH] 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 Follow-up: - Added 3 tests in t/db_dependent/Circulation_Branches.t to test AddReturn policies - Removed HomeOrHoldingBranchReturn from sysprefs.sql - Added notice on removing syspref in updatedatabase --- C4/Circulation.pm | 35 +++--- circ/returns.pl | 18 ++- installer/data/mysql/sysprefs.sql | 1 - installer/data/mysql/updatedatabase.pl | 7 ++ .../en/modules/admin/preferences/circulation.pref | 7 -- .../intranet-tmpl/prog/en/modules/circ/returns.tt | 10 +- t/db_dependent/Circulation_Branch.t | 124 ++++++++++++++++++--- 7 files changed, 151 insertions(+), 51 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 639956b..7043e0e 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: @@ -1698,6 +1699,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 @@ -1729,7 +1738,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" @@ -1749,18 +1757,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 @@ -1956,21 +1964,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 6daddc4..d98b40e 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -248,10 +248,6 @@ if ($barcode) { # # save the return # - ( $returned, $messages, $issueinformation, $borrower ) = - AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override ); - my $homeorholdingbranchreturn = C4::Context->preference('HomeOrHoldingBranchReturn'); - $homeorholdingbranchreturn ||= 'homebranch'; # get biblio description my $biblio = GetBiblioFromItemNumber($itemnumber); @@ -268,10 +264,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'}, @@ -287,6 +289,10 @@ if ($barcode) { barcode => $barcode, ); + # do the return + ( $returned, $messages, $issueinformation, $borrower ) = + AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override ); + 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'); @@ -352,7 +358,7 @@ if ( $messages->{'WasTransfered'} ) { if ( $messages->{'NeedsTransfer'} ){ $template->param( found => 1, - needstransfer => 1, + needstransfer => $messages->{'NeedsTransfer'}, itemnumber => $itemnumber, ); } diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index b06dd72..9ddc6e0 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -132,7 +132,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('HighlightOwnItemsOnOPACWhich','PatronBranch','PatronBranch|OpacURLBranch','Decides which branch\'s items to emphasize. If PatronBranch, emphasize the logged in user\'s library\'s items. If OpacURLBranch, highlight the items of the Apache var BRANCHCODE defined in Koha\'s Apache configuration file.','Choice'), ('HoldsToPullStartDate','2',NULL,'Set the default start date for the Holds to pull list to this many days ago','Integer'), ('HomeOrHoldingBranch','holdingbranch','holdingbranch|homebranch','Used by Circulation to determine which branch of an item to check with independent branches on, and by search to determine which branch to choose for availability ','Choice'), -('HomeOrHoldingBranchReturn','homebranch','holdingbranch|homebranch','Used by Circulation to determine which branch of an item to check checking-in items','Choice'), ('HTML5MediaEnabled','not','not|opac|staff|both','Show a tab with a HTML5 media player for files catalogued in field 856','Choice'), ('HTML5MediaExtensions','webm|ogg|ogv|oga|vtt','','Media file extensions','free'), ('IDreamBooksReadometer','0','','Display Readometer from IDreamBooks.com','YesNo'), diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 70adef3..d08fa94 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -8456,6 +8456,13 @@ if ( CheckVersion($DBversion) ) { "); print "Upgrade to $DBversion done (Bug 10500 - Improve isbn matching when importing records)\n"; + +$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 on return. HomeOrHoldingBranchReturn syspref removed in favour of circulation rules.)\n"; SetVersion($DBversion); } 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 ad0d78b..13b76dc 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 @@ -329,13 +329,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 2e48106..c145b1d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -253,9 +253,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 ) %] @@ -267,15 +267,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 %] - + diff --git a/t/db_dependent/Circulation_Branch.t b/t/db_dependent/Circulation_Branch.t index ee26458..2807b4e 100644 --- a/t/db_dependent/Circulation_Branch.t +++ b/t/db_dependent/Circulation_Branch.t @@ -7,17 +7,20 @@ use C4::Branch; use C4::Circulation; use C4::Items; use C4::Context; - -use Test::More tests => 10; +use Data::Dumper; +use Test::More tests => 13; BEGIN { use_ok('C4::Circulation'); } can_ok( 'C4::Circulation', qw( - GetBranchBorrowerCircRule - GetBranchItemRule - ) + AddIssue + AddReturn + GetBranchBorrowerCircRule + GetBranchItemRule + GetIssuingRule + ) ); #Start transaction @@ -160,6 +163,58 @@ $sth->execute( $sampleitemtype2->{imageurl}, $sampleitemtype2->{summary} ); +#Add biblio and item +my $record = MARC::Record->new(); +$record->append_fields( + MARC::Field->new( '952', '0', '0', a => $samplebranch1->{branchcode} ) ); +my ( $biblionumber, $biblioitemnumber ) = C4::Biblio::AddBiblio( $record, '' ); + +# item 2 has home branch and holding branch samplebranch1 +my @sampleitem1 = C4::Items::AddItem( + { + barcode => 'barcode_1', + itemcallnumber => 'callnumber1', + homebranch => $samplebranch1->{branchcode}, + holdingbranch => $samplebranch1->{branchcode} + }, + $biblionumber +); +my $item_id1 = $sampleitem1[2]; + +# item 2 has holding branch samplebranch2 +my @sampleitem2 = C4::Items::AddItem( + { + barcode => 'barcode_2', + itemcallnumber => 'callnumber2', + homebranch => $samplebranch2->{branchcode}, + holdingbranch => $samplebranch1->{branchcode} + }, + $biblionumber +); +my $item_id2 = $sampleitem2[2]; + +# item 3 has item type sampleitemtype2 with noreturn policy +my @sampleitem3 = C4::Items::AddItem( + { + barcode => 'barcode_3', + itemcallnumber => 'callnumber3', + homebranch => $samplebranch2->{branchcode}, + holdingbranch => $samplebranch2->{branchcode}, + itype => $sampleitemtype2->{itemtype} + }, + $biblionumber +); +my $item_id3 = $sampleitem3[2]; + +#Add borrower +my $borrower_id1 = C4::Members::AddMember( + firstname => 'firstname1', + surname => 'surname1 ', + categorycode => $samplecat->{categorycode}, + branchcode => $samplebranch1->{branchcode}, +); +my $borrower_1 = C4::Members::GetMember(borrowernumber => $borrower_id1); + $query = "INSERT INTO branch_borrower_circ_rules (branchcode,categorycode,maxissueqty) VALUES( ?,?,?)"; $dbh->do( @@ -167,13 +222,14 @@ $dbh->do( $samplebranch1->{branchcode}, $samplecat->{categorycode}, 5 ); -$query = -"INSERT INTO default_branch_circ_rules (branchcode,maxissueqty,holdallowed,returnbranch) VALUES( ?,?,?,?)"; -$dbh->do( $query, {}, $samplebranch2->{branchcode}, - 3, 1, $samplebranch2->{branchcode} ); + $query = "INSERT INTO default_circ_rules (singleton,maxissueqty,holdallowed,returnbranch) VALUES( ?,?,?,?)"; -$dbh->do( $query, {}, 'singleton', 4, 3, $samplebranch1->{branchcode} ); +$dbh->do( $query, {}, 'singleton', 4, 3, 'homebranch' ); + +$query = +"INSERT INTO default_branch_circ_rules (branchcode,maxissueqty,holdallowed,returnbranch) VALUES( ?,?,?,?)"; +$dbh->do( $query, {}, $samplebranch2->{branchcode}, 3, 1, 'holdingbranch' ); $query = "INSERT INTO branch_item_rules (branchcode,itemtype,holdallowed,returnbranch) VALUES( ?,?,?,?)"; @@ -181,12 +237,17 @@ $sth = $dbh->prepare($query); $sth->execute( $samplebranch1->{branchcode}, $sampleitemtype1->{itemtype}, - 5, $samplebranch1->{branchcode} + 5, 'homebranch' +); +$sth->execute( + $samplebranch2->{branchcode}, + $sampleitemtype1->{itemtype}, + 5, 'holdingbranch' ); $sth->execute( $samplebranch2->{branchcode}, $sampleitemtype2->{itemtype}, - 5, $samplebranch1->{branchcode} + 5, 'noreturn' ); #Test GetBranchBorrowerCircRule @@ -220,23 +281,52 @@ is_deeply( $samplebranch1->{branchcode}, $sampleitemtype1->{itemtype} ), - { returnbranch => $samplebranch1->{branchcode}, holdallowed => 5 }, + { returnbranch => 'homebranch', holdallowed => 5 }, "GetBranchitem returns holdallowed and return branch" ); is_deeply( GetBranchItemRule(), - { returnbranch => $samplebranch1->{branchcode}, holdallowed => 3 }, + { returnbranch => 'homebranch', holdallowed => 3 }, "Without parameters GetBranchItemRule returns the values in default_circ_rules" ); is_deeply( - GetBranchItemRule( $samplebranch1->{branchcode} ), - { returnbranch => $samplebranch1->{branchcode}, holdallowed => 3 }, + GetBranchItemRule( $samplebranch2->{branchcode} ), + { returnbranch => 'holdingbranch', holdallowed => 1 }, "With only a branchcode GetBranchItemRule returns values in default_branch_circ_rules" ); is_deeply( GetBranchItemRule( -1, -1 ), - { returnbranch => $samplebranch1->{branchcode}, holdallowed => 3 }, + { returnbranch => 'homebranch', holdallowed => 3 }, "With only one parametern GetBranchItemRule returns default values" ); +# Test return policies +C4::Context->set_preference('AutomaticItemReturn','0'); + +# item1 returned at branch2 should trigger transfer to homebranch +$query = +"INSERT INTO issues (borrowernumber,itemnumber,branchcode) VALUES( ?,?,? )"; +$dbh->do( $query, {}, $borrower_id1, $item_id1, $samplebranch1->{branchcode} ); + +my ($doreturn, $messages, $iteminformation, $borrower) = AddReturn('barcode_1', + $samplebranch2->{branchcode}); +is( $messages->{NeedsTransfer}, $samplebranch1->{branchcode}, "AddReturn respects default return policy - return to homebranch" ); + +# item2 returned at branch2 should trigger transfer to holding branch +$query = +"INSERT INTO issues (borrowernumber,itemnumber,branchcode) VALUES( ?,?,? )"; +$dbh->do( $query, {}, $borrower_id1, $item_id2, $samplebranch2->{branchcode} ); +($doreturn, $messages, $iteminformation, $borrower) = AddReturn('barcode_2', + $samplebranch2->{branchcode}); +is( $messages->{NeedsTransfer}, $samplebranch1->{branchcode}, "AddReturn respects branch return policy - item2->homebranch policy = 'holdingbranch'" ); + +# item3 should not trigger transfer - floating collection +$query = +"INSERT INTO issues (borrowernumber,itemnumber,branchcode) VALUES( ?,?,? )"; +$dbh->do( $query, {}, $borrower_id1, $item_id3, $samplebranch1->{branchcode} ); +($doreturn, $messages, $iteminformation, $borrower) = AddReturn('barcode_3', + $samplebranch1->{branchcode}); +is($messages->{NeedsTransfer},undef,"AddReturn respects branch item return policy - noreturn"); + + $dbh->rollback; -- 1.9.1