From: Joe Atzberger Date: Thu, 18 Jun 2009 16:31:33 +0000 (-0500) Subject: Returns reworking to handle empty GetItemIssue X-Git-Tag: v3.00.04~217 X-Git-Url: http://git.rot13.org/?a=commitdiff_plain;h=1e244d83e3c93c0af786419d484b0fc674ca05ff;p=koha.git Returns reworking to handle empty GetItemIssue Code cannot rely on issueinformation being populated. Note there is room for better efficiency to have AddReturn also provide the itemnumber (where existing) so that GetItemnumberFromBarcode is not called at both levels. Unfortunately there is discrepancy between this idea (for efficiency) and the stated purpose of the $iteminformation object returned, since $iteminformation is specifically the info from the issues table and MUST be empty when the item was not in fact issued. Signed-off-by: Galen Charlton --- diff --git a/C4/Circulation.pm b/C4/Circulation.pm index f7e3776739..f899089267 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1305,6 +1305,9 @@ either C, C, or 0. =back +C<$iteminformation> is a reference-to-hash, giving information about the +returned item from the issues table. + C<$borrower> is a reference-to-hash, giving information about the patron who last borrowed the book. @@ -1312,59 +1315,62 @@ patron who last borrowed the book. sub AddReturn { my ( $barcode, $branch, $exemptfine, $dropbox ) = @_; - my $dbh = C4::Context->dbh; + if ($branch and not GetBranchDetail($branch)) { + warn "AddReturn error: branch '$branch' not found. Reverting to " . C4::Context->userenv->{'branch'}; + undef $branch; + } + $branch = C4::Context->userenv->{'branch'} unless $branch; # we trust userenv to be a safe fallback/default my $messages; - my $doreturn = 1; my $borrower; + my $doreturn = 1; my $validTransfert = 0; - my $reserveDone = 0; + my $reserveDone = 0; # get information on item - my $iteminformation = GetItemIssue( GetItemnumberFromBarcode($barcode)); - my $biblio = GetBiblioItemData($iteminformation->{'biblioitemnumber'}); + my $itemnumber = GetItemnumberFromBarcode( $barcode ); + my $iteminformation = GetItemIssue($itemnumber); + my $biblio = GetBiblioItemData($iteminformation->{'biblioitemnumber'}); # use Data::Dumper;warn Data::Dumper::Dumper($iteminformation); - unless ($iteminformation->{'itemnumber'} ) { + unless ($itemnumber) { $messages->{'BadBarcode'} = $barcode; $doreturn = 0; } else { - # find the borrower - if ( ( not $iteminformation->{borrowernumber} ) && $doreturn ) { + if ( not $iteminformation ) { $messages->{'NotIssued'} = $barcode; # even though item is not on loan, it may still # be transferred; therefore, get current branch information my $curr_iteminfo = GetItem($iteminformation->{'itemnumber'}); - $iteminformation->{'homebranch'} = $curr_iteminfo->{'homebranch'}; + $iteminformation->{'homebranch'} = $curr_iteminfo->{'homebranch'}; $iteminformation->{'holdingbranch'} = $curr_iteminfo->{'holdingbranch'}; - $iteminformation->{'itemlost'} = $curr_iteminfo->{'itemlost'}; + $iteminformation->{'itemlost'} = $curr_iteminfo->{'itemlost'}; + # These lines patch up $iteminformation enough so it can be used below for other messages $doreturn = 0; } - + + my $hbr = $iteminformation->{C4::Context->preference("HomeOrHoldingBranch")} || ''; # check if the book is in a permanent collection.... - my $hbr = $iteminformation->{C4::Context->preference("HomeOrHoldingBranch")}; - my $branches = GetBranches(); - # FIXME -- This 'PE' attribute is largely undocumented. afaict, there's no user interface that reflects this functionality. - if ( $hbr && $branches->{$hbr}->{'PE'} ) { - $messages->{'IsPermanent'} = $hbr; + # FIXME -- This 'PE' attribute is largely undocumented. afaict, there's no user interface that reflects this functionality. + if ( $hbr ) { + my $branches = GetBranches(); # a potentially expensive call for a non-feature. + $branches->{$hbr}->{PE} and $messages->{'IsPermanent'} = $hbr; } - - # if independent branches are on and returning to different branch, refuse the return - if ($hbr ne C4::Context->userenv->{'branch'} && C4::Context->preference("IndependantBranches")){ - $messages->{'Wrongbranch'} = 1; - $doreturn=0; - } - - # check that the book has been cancelled - if ( $iteminformation->{'wthdrawn'} ) { + + # if indy branches and returning to different branch, refuse the return + if ($hbr ne $branch && C4::Context->preference("IndependantBranches")){ + $messages->{'Wrongbranch'} = 1; + $doreturn = 0; + } + + if ( $iteminformation->{'wthdrawn'} ) { # book has been cancelled $messages->{'wthdrawn'} = 1; $doreturn = 0; } - - # new op dev : if the book returned in an other branch update the holding branch - - # update issues, thereby returning book (should push this out into another subroutine + + # if the book returned in an other branch, update the holding branch + # update issues, thereby returning book (should push this out into another subroutine $borrower = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 ); - - # case of a return of document (deal with issues and holdingbranch) + + # case of a return of document (deal with issues and holdingbranch) if ($doreturn) { my $circControlBranch = GetCirculationBranch($iteminformation,$borrower); @@ -1376,60 +1382,54 @@ sub AddReturn { $messages->{'WasReturned'} = 1; # FIXME is the "= 1" right? # continue to deal with returns cases, but not only if we have an issue - - # We update the holdingbranch from circControlBranch variable - UpdateHoldingbranch($circControlBranch,$iteminformation->{'itemnumber'}); - $iteminformation->{'holdingbranch'} = $circControlBranch; - - + # the holdingbranch is updated if the document is returned in an other location . + if ( $iteminformation->{'holdingbranch'} ne $branch ) { + UpdateHoldingbranch($branch, $iteminformation->{'itemnumber'}); + $iteminformation->{'holdingbranch'} = $branch; # update iteminformation holdingbranch too + } ModDateLastSeen( $iteminformation->{'itemnumber'} ); ModItem({ onloan => undef }, $biblio->{'biblionumber'}, $iteminformation->{'itemnumber'}); - + if ($iteminformation->{borrowernumber}){ - ($borrower) = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 ); + $borrower = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 ); # FIXME: we shouldn't need to make the same call twice } } - # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # - # check if we have a transfer for this document + # check if we have a transfer for this document my ($datesent,$frombranch,$tobranch) = GetTransfers( $iteminformation->{'itemnumber'} ); - # if we have a transfer to do, we update the line of transfers with the datearrived + # if we have a transfer to do, we update the line of transfers with the datearrived if ($datesent) { - if ( $tobranch eq C4::Context->userenv->{'branch'} ) { - my $sth = - $dbh->prepare( - "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL" - ); - $sth->execute( $iteminformation->{'itemnumber'} ); - $sth->finish; - # now we check if there is a reservation with the validate of transfer if we have one, we can set it with the status 'W' - C4::Reserves::ModReserveStatus( $iteminformation->{'itemnumber'},'W' ); + if ( $tobranch eq $branch ) { + my $sth = C4::Context->dbh->prepare( + "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL" + ); + $sth->execute( $iteminformation->{'itemnumber'} ); + # if we have a reservation with the validate of transfer, we can set it's status to 'W' + C4::Reserves::ModReserveStatus($iteminformation->{'itemnumber'}, 'W'); + } else { + $messages->{'WrongTransfer'} = $tobranch; + $messages->{'WrongTransferItem'} = $iteminformation->{'itemnumber'}; } - else { - $messages->{'WrongTransfer'} = $tobranch; - $messages->{'WrongTransferItem'} = $iteminformation->{'itemnumber'}; - } - $validTransfert = 1; + $validTransfert = 1; } - # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # fix up the accounts..... if ($iteminformation->{'itemlost'}) { FixAccountForLostAndReturned($iteminformation, $borrower); ModItem({ itemlost => '0' }, $biblio->{'biblionumber'}, $iteminformation->{'itemnumber'}); $messages->{'WasLost'} = 1; } + # fix up the overdues in accounts... FixOverduesOnReturn( $borrower->{'borrowernumber'}, $iteminformation->{'itemnumber'}, $exemptfine, $dropbox ); - # find reserves..... - # if we don't have a reserve with the status W, we launch the Checkreserves routine - my ( $resfound, $resrec ) = - C4::Reserves::CheckReserves( $iteminformation->{'itemnumber'} ); + # find reserves..... + # if we don't have a reserve with the status W, we launch the Checkreserves routine + my ( $resfound, $resrec ) = C4::Reserves::CheckReserves( $iteminformation->{'itemnumber'} ); if ($resfound) { - $resrec->{'ResFound'} = $resfound; + $resrec->{'ResFound'} = $resfound; $messages->{'ResFound'} = $resrec; $reserveDone = 1; } @@ -1453,8 +1453,12 @@ sub AddReturn { if (C4::Context->preference("AutomaticItemReturn") == 1) { ModItemTransfer($iteminformation->{'itemnumber'}, C4::Context->userenv->{'branch'}, $iteminformation->{$hbr}); $messages->{'WasTransfered'} = 1; - } - else { + } elsif ( C4::Context->preference("UseBranchTransferLimits") == 1 + && ! IsBranchTransferAllowed( $branch, $iteminformation->{'homebranch'}, $iteminformation->{ C4::Context->preference("BranchTransferLimitsType") } ) + ) { + ModItemTransfer($iteminformation->{'itemnumber'}, C4::Context->userenv->{'branch'}, $iteminformation->{'homebranch'}); + $messages->{'WasTransfered'} = 1; + } else { $messages->{'NeedsTransfer'} = 1; } } diff --git a/circ/returns.pl b/circ/returns.pl index 81222eb8e5..5a5b4a314c 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -96,7 +96,7 @@ foreach ( $query->param ) { my $borrowernumber = $query->param("bn-$counter"); $counter++; - # decode barcode + # decode barcode ## Didn't we already decode them before passing them back last time?? $barcode = barcodedecode($barcode) if(C4::Context->preference('itemBarcodeInputFilter')); ###################### @@ -157,6 +157,7 @@ my $borrower; my $returned = 0; my $messages; my $issueinformation; +my $itemnumber; my $barcode = $query->param('barcode'); my $exemptfine = $query->param('exemptfine'); my $dropboxmode = $query->param('dropboxmode'); @@ -167,7 +168,7 @@ my $today = C4::Dates->new(); my $today_iso = $today->output('iso'); my $dropboxdate = $calendar->addDate($today, -1); if ($dotransfer){ - # An item has been returned to a branch other than the homebranch, and the librarian has choosen to initiate a transfer + # An item has been returned to a branch other than the homebranch, and the librarian has chosen to initiate a transfer my $transferitem = $query->param('transferitem'); my $tobranch = $query->param('tobranch'); ModItemTransfer($transferitem, $userenv_branch, $tobranch); @@ -176,13 +177,13 @@ if ($dotransfer){ # actually return book and prepare item table..... if ($barcode) { $barcode = barcodedecode($barcode) if C4::Context->preference('itemBarcodeInputFilter'); -# -# save the return -# + $itemnumber = GetItemnumberFromBarcode($barcode); + ( $returned, $messages, $issueinformation, $borrower ) = - AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode); + AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode); # do the return + # get biblio description - my $biblio = GetBiblioFromItemNumber($issueinformation->{'itemnumber'}); + my $biblio = GetBiblioFromItemNumber($itemnumber); # fix up item type for display $biblio->{'itemtype'} = C4::Context->preference('item-level_itypes') ? $biblio->{'itype'} : $biblio->{'itemtype'}; @@ -203,19 +204,14 @@ if ($barcode) { ); if ($returned) { - $returneditems{0} = $barcode; - $riborrowernumber{0} = $borrower->{'borrowernumber'}; - $riduedate{0} = $issueinformation->{'date_due'}; + my $duedate = $issueinformation->{'date_due'}; + $returneditems{0} = $barcode; + $riborrowernumber{0} = $borrower->{'borrowernumber'}; + $riduedate{0} = $duedate; $input{borrowernumber} = $borrower->{'borrowernumber'}; - $input{duedate} = $issueinformation->{'date_due'}; - $input{return_overdue} = 1 if ($issueinformation->{'date_due'} lt $today->output('iso')); + $input{duedate} = $duedate; + $input{return_overdue} = 1 if ($duedate and $duedate lt $today->output('iso')); push( @inputloop, \%input ); - - # check if the branch is the same as homebranch - # if not, we want to put a message - if ( $biblio->{'homebranch'} ne $userenv_branch ) { - $template->param( homebranch => $biblio->{'homebranch'} ); - } } elsif ( !$messages->{'BadBarcode'} ) { $input{duedate} = 0; @@ -227,7 +223,7 @@ if ($barcode) { $riborrowernumber{0} = 'Item Cancelled'; } else { - $input{borrowernumber} = ' '; + $input{borrowernumber} = ' '; # This seems clearly bogus. $riborrowernumber{0} = ' '; } push( @inputloop, \%input ); @@ -253,7 +249,7 @@ if ( $messages->{'NeedsTransfer'} ){ $template->param( found => 1, needstransfer => 1, - itemnumber => $issueinformation->{'itemnumber'} + itemnumber => $itemnumber, ); } @@ -263,7 +259,7 @@ if ( $messages->{'Wrongbranch'} ){ ); } -# adding a case of wrong transfert, if the document wasn't transfered in the good library (according to branchtransfer (tobranch) BDD) +# case of wrong transfert, if the document wasn't transfered to the right library (according to branchtransfer (tobranch) BDD) if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { $template->param( @@ -293,7 +289,6 @@ if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { ); } - # # reserve found and item arrived at the expected branch # @@ -345,8 +340,6 @@ if ( $messages->{'ResFound'}) { # Error Messages my @errmsgloop; foreach my $code ( keys %$messages ) { - - # warn $code; my %err; my $exit_required_p = 0; if ( $code eq 'BadBarcode' ) { @@ -392,7 +385,8 @@ foreach my $code ( keys %$messages ) { } else { - die "Unknown error code $code"; # XXX + die "Unknown error code $code"; # note we need all the (empty) elsif's above, or we die. + # This forces the issue of staying in sync w/ Circulation.pm } if (%err) { push( @errmsgloop, \%err ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl index a06dc49470..f3bb36d697 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl @@ -158,14 +158,13 @@ function Dopop(link) { - - +
-

Please return "> to

+

Please return "> to

- +

This item needs to be transfered to

Transfer Now?