From f1b477d1f3d86e6176bab6da5e0c673b742d63a3 Mon Sep 17 00:00:00 2001 From: Benjamin Rokseth Date: Thu, 5 Mar 2015 16:09:00 +0100 Subject: [PATCH] Bug 7981 - Remove 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 QA edits: - removed trailing whitespace in tests - moved branchname lookup from returns.pl to template Signed-off-by: Jonathan Druart Signed-off-by: Mirko Tietgen Signed-off-by: Jonathan Druart --- 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 f991680..d825d81 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1607,6 +1607,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: @@ -1725,6 +1726,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 @@ -1756,7 +1765,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" @@ -1788,9 +1796,9 @@ sub AddReturn { # 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 @@ -1817,9 +1825,9 @@ sub AddReturn { # 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 @@ -2023,21 +2031,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 ( !$is_in_rotating_collection && ($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 (!$is_in_rotating_collection && ($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; } } diff --git a/circ/returns.pl b/circ/returns.pl index 8b70e1a..7e601a4 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -249,10 +249,6 @@ if ($barcode) { # # save the return # - ( $returned, $messages, $issueinformation, $borrower ) = - AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override, $dropboxdate ); - my $homeorholdingbranchreturn = C4::Context->preference('HomeOrHoldingBranchReturn'); - $homeorholdingbranchreturn ||= 'homebranch'; # get biblio description my $biblio = GetBiblioFromItemNumber($itemnumber); @@ -269,10 +265,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'}, @@ -289,6 +291,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'); @@ -358,7 +364,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 9620d67..78843ad 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -145,7 +145,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 fe71359..de8d97e 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -8507,6 +8507,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 a16f231..b64eeb6 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 @@ -366,13 +366,6 @@ Circulation: -
Leave this field empty if you don't want to charge the user for lost items. -
(Used when the longoverdue.pl script is called without the --charge parameter) - - - 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 211b131..1b2aec6 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -275,9 +275,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 ) %] @@ -289,15 +289,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..0f30f2e 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; -- 2.1.0