From e8908c2f97222dddd43474826e61d97a49e7ae9a Mon Sep 17 00:00:00 2001 From: Joe Atzberger Date: Thu, 18 Jun 2009 11:31:33 -0500 Subject: [PATCH] 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 --- C4/Circulation.pm | 130 +++++++++--------- circ/returns.pl | 44 +++--- .../prog/en/modules/circ/returns.tmpl | 7 +- 3 files changed, 86 insertions(+), 95 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index f4cea86971..9199cd9509 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1392,6 +1392,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. @@ -1399,63 +1402,66 @@ 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; - if($dropbox) { + if ($dropbox) { # don't allow dropbox mode to create an invalid entry in issues (issuedate > returndate) FIXME: actually checks eq, not gt undef($dropbox) if ( $iteminformation->{'issuedate'} eq C4::Dates->today('iso') ); if (C4::Context->preference('CircControl') eq 'ItemHomeBranch' ) { @@ -1470,20 +1476,18 @@ sub AddReturn { MarkIssueReturned($borrower->{'borrowernumber'}, $iteminformation->{'itemnumber'},$circControlBranch); $messages->{'WasReturned'} = 1; # FIXME is the "= 1" right? - # continue to deal with returns cases, but not only if we have an issue # the holdingbranch is updated if the document is returned in an other location . - if ( $iteminformation->{'holdingbranch'} ne C4::Context->userenv->{'branch'} ) { - UpdateHoldingbranch(C4::Context->userenv->{'branch'},$iteminformation->{'itemnumber'}); - # reload iteminformation holdingbranch with the userenv value - $iteminformation->{'holdingbranch'} = C4::Context->userenv->{'branch'}; + 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 ); + if ($iteminformation->{borrowernumber}){ + $borrower = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 ); # FIXME: we shouldn't need to make the same call twice } } # fix up the accounts..... @@ -1491,45 +1495,40 @@ sub AddReturn { $messages->{'WasLost'} = 1; } - # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # - # 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); - $messages->{'WasLost'} = 1; + FixAccountForLostAndReturned($iteminformation, $borrower); + $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; } @@ -1575,8 +1574,7 @@ sub AddReturn { ) { ModItemTransfer($iteminformation->{'itemnumber'}, C4::Context->userenv->{'branch'}, $iteminformation->{'homebranch'}); $messages->{'WasTransfered'} = 1; - } - else { + } else { $messages->{'NeedsTransfer'} = 1; } } diff --git a/circ/returns.pl b/circ/returns.pl index 4b91708959..4ceb2b47af 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 f445b601bd..043e74bf55 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl @@ -160,14 +160,13 @@ function Dopop(link) { - - +
-

Please return "> to

+

Please return "> to

- +

This item needs to be transfered to

Transfer Now?
-- 2.39.5