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 <galen.charlton@liblime.com>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index f4cea86..9199cd9 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1392,6 +1392,9 @@ either C<Waiting>, C<Reserved>, 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 4b91708..4ceb2b4 100755 (executable)
--- 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 f445b60..043e74b 100644 (file)
--- 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) {
                </div>
     <!-- /TMPL_IF -->
 
-    <!-- case of a return of item, but with no reservation after, if the item must be returned to its homebranch -->
     <!-- TMPL_IF Name="transfer" -->
-       <!-- transfer -->
+    <!-- transfer: item with no reservation, must be returned to its homebranch -->
        <div class="dialog message">
-         <h3>Please return <a href="/cgi-bin/koha/catalogue/detail.pl?type=intra&amp;biblionumber=<!-- TMPL_VAR NAME="itembiblionumber" -->"><!-- TMPL_VAR Name="title" escape="html" --></a> to <!-- TMPL_VAR Name="homebranch" --></h3></div><!-- /TMPL_IF -->
+         <h3>Please return <a href="/cgi-bin/koha/catalogue/detail.pl?type=intra&amp;biblionumber=<!-- TMPL_VAR NAME="itembiblionumber" -->"><!-- TMPL_VAR NAME="title" escape="html" DEFAULT="item" --></a> to <!-- TMPL_VAR NAME="homebranch" DEFAULT="homebranch" --></h3></div><!-- /TMPL_IF -->
     
     <!-- TMPL_IF Name="needstransfer" -->
-       <!-- transfer -->
+       <!-- needstransfer -->
        <div class="dialog message"><h3> This item needs to be transfered to <!-- TMPL_VAR Name="homebranch" --></h3>
        Transfer Now?<br />
     <form method="post" action="returns.pl" name="mainform" id="mainform">