Attachment 18469 Details for Bug 10374 – Bug 10374 - Clean up and unify CircControl related code - Followup - Clean up AddReturn

[patch] Bug 10374 - Clean up and unify CircControl related code - Followup - Clean up AddReturn

Bug-10374---Clean-up-and-unify-CircControl-related.patch (text/plain), 15.49 KB, created by Kyle M Hall (khall) on 2013-05-29 13:16:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2013-05-29 13:16:00 UTC

Size: 15.49 KB

patch

obsolete

>From 05c9c93b7f7d9cfe5f2942d64a91d5c0f4fd7a2a Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Wed, 29 May 2013 09:11:38 -0400
>Subject: [PATCH] Bug 10374 - Clean up and unify CircControl related code - Followup - Clean up AddReturn
>
>This patch tidys up C4::Circulation::AddReturn. It rearranges some
>comments to make them more sensible, and addresses the FIXME for an
>unitelligable comment. This patch makes no functional changes to the
>code.
>---
> C4/Circulation.pm |  231 +++++++++++++++++++++++++++++++++-------------------
> 1 files changed, 146 insertions(+), 85 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index b6c6cc0..c9744a7 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -1710,61 +1710,78 @@ patron who last borrowed the book.
> sub AddReturn {
>     my ( $barcode, $branch, $exemptfine, $dropbox ) = @_;
> 
>-    if ($branch and not GetBranchDetail($branch)) {
>-        warn "AddReturn error: branch '$branch' not found.  Reverting to " . C4::Context->userenv->{'branch'};
>+    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
>+
>+    $branch = C4::Context->userenv->{'branch'}
>+      unless $branch;    # we trust userenv to be a safe fallback/default
>+
>     my $messages;
>     my $borrower;
>     my $biblio;
>     my $doreturn       = 1;
>     my $validTransfert = 0;
>-    my $stat_type = 'return';    
>+    my $stat_type      = 'return';
> 
>     # get information on item
>-    my $itemnumber = GetItemnumberFromBarcode( $barcode );
>-    unless ($itemnumber) {
>-        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"
>-                . Dumper($issue) . "\n";
>-    } else {
>+    my $itemnumber = GetItemnumberFromBarcode($barcode);
>+
>+    # no barcode means no item or borrower.  bail out.
>+    return ( 0, { BadBarcode => $barcode } ) unless ($itemnumber);
>+
>+    my $issue = GetItemIssue($itemnumber);
>+
>+    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"
>+          . Dumper($issue) . "\n";
>+    }
>+    else {
>         $messages->{'NotIssued'} = $barcode;
>+
>         # even though item is not on loan, it may still be transferred;  therefore, get current branch info
>         $doreturn = 0;
>+
>         # No issue, no borrowernumber.  ONLY if $doreturn, *might* you have a $borrower later.
>         # Record this as a local use, instead of a return, if the RecordLocalUseOnReturn is on
>-        if (C4::Context->preference("RecordLocalUseOnReturn")) {
>-           $messages->{'LocalUse'} = 1;
>-           $stat_type = 'localuse';
>+        if ( C4::Context->preference("RecordLocalUseOnReturn") ) {
>+            $messages->{'LocalUse'} = 1;
>+            $stat_type = 'localuse';
>         }
>     }
> 
>     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";
>-        # get the proper branch to which to return the item
>-    $hbr = $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
>+    # 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";
>+
>+    # get the proper branch to which to return the item
>+    # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)
>+    $hbr = $item->{$hbr} || $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 ) {
>-        my $branches = GetBranches();    # a potentially expensive call for a non-feature.
>+    if ($hbr) {
>+
>+        # a potentially expensive call for a non-feature.
>+        my $branches = GetBranches();
>         $branches->{$hbr}->{PE} and $messages->{'IsPermanent'} = $hbr;
>     }
> 
>     # check if the return is allowed at this branch
>-    my ($returnallowed, $message) = CanBookBeReturned($item, $branch);
>-    unless ($returnallowed){
>+    my ( $returnallowed, $message ) = CanBookBeReturned( $item, $branch );
>+    unless ($returnallowed) {
>         $messages->{'Wrongbranch'} = {
>             Wrongbranch => $branch,
>             Rightbranch => $message
>@@ -1773,7 +1790,7 @@ sub AddReturn {
>         return ( $doreturn, $messages, $issue, $borrower );
>     }
> 
>-    if ( $item->{'wthdrawn'} ) { # book has been cancelled
>+    if ( $item->{'wthdrawn'} ) {    # book has been cancelled
>         $messages->{'wthdrawn'} = 1;
>         $doreturn = 0 if C4::Context->preference("BlockReturnOfWithdrawnItems");
>     }
>@@ -1781,12 +1798,14 @@ sub AddReturn {
>     # case of a return of document (deal with issues and holdingbranch)
>     my $today = DateTime->now( time_zone => C4::Context->tz() );
>     if ($doreturn) {
>-    my $datedue = $issue->{date_due};
>+        my $datedue = $issue->{date_due};
>         $borrower or warn "AddReturn without current borrower";
>         if ($dropbox) {
>+
>             # don't allow dropbox mode to create an invalid entry in issues (issuedate > today)
>             # FIXME: check issuedate > returndate, factoring in holidays
>-            $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $today ) == -1 ? 1 : 0;
>+            $issue->{'overdue'} =
>+              DateTime->compare( $issue->{'date_due'}, $today ) == -1 ? 1 : 0;
>         }
> 
>         if ($borrowernumber) {
>@@ -1818,56 +1837,80 @@ sub AddReturn {
> 
>         }
> 
>-        ModItem({ onloan => undef }, $issue->{'biblionumber'}, $item->{'itemnumber'});
>+        ModItem(
>+            { onloan => undef },
>+            $issue->{'biblionumber'},
>+            $item->{'itemnumber'}
>+        );
>     }
> 
>     # the holdingbranch is updated if the document is returned to another location.
>     # this is always done regardless of whether the item was on loan or not
>-    if ($item->{'holdingbranch'} ne $branch) {
>-        UpdateHoldingbranch($branch, $item->{'itemnumber'});
>+    if ( $item->{'holdingbranch'} ne $branch ) {
>+        UpdateHoldingbranch( $branch, $item->{'itemnumber'} );
>         $item->{'holdingbranch'} = $branch; # update item data holdingbranch too
>     }
>     ModDateLastSeen( $item->{'itemnumber'} );
> 
>     # check if we have a transfer for this document
>-    my ($datesent,$frombranch,$tobranch) = GetTransfers( $item->{'itemnumber'} );
>+    my ( $datesent, $frombranch, $tobranch ) =
>+      GetTransfers( $item->{'itemnumber'} );
> 
>     # if we have a transfer to do, we update the line of transfers with the datearrived
>     if ($datesent) {
>         if ( $tobranch eq $branch ) {
>-            my $sth = C4::Context->dbh->prepare(
>-                "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL"
>-            );
>+            my $sth = C4::Context->dbh->prepare("
>+                UPDATE branchtransfers 
>+                SET datearrived = now() 
>+                WHERE itemnumber= ? 
>+                  AND datearrived IS NULL
>+            ");
>             $sth->execute( $item->{'itemnumber'} );
>+
>+            ShelfToCart( $item->{'itemnumber'} )
>+              if ( C4::Context->preference("ReturnToShelvingCart") );
>+
>             # if we have a reservation with valid transfer, we can set it's status to 'W'
>-            ShelfToCart( $item->{'itemnumber'} ) if ( C4::Context->preference("ReturnToShelvingCart") );
>-            C4::Reserves::ModReserveStatus($item->{'itemnumber'}, 'W');
>-        } else {
>+            C4::Reserves::ModReserveStatus( $item->{'itemnumber'}, 'W' );
>+        }
>+        else {
>             $messages->{'WrongTransfer'}     = $tobranch;
>             $messages->{'WrongTransferItem'} = $item->{'itemnumber'};
>         }
>         $validTransfert = 1;
>-    } else {
>-        ShelfToCart( $item->{'itemnumber'} ) if ( C4::Context->preference("ReturnToShelvingCart") );
>+    }
>+    else {
>+        ShelfToCart( $item->{'itemnumber'} )
>+          if ( C4::Context->preference("ReturnToShelvingCart") );
>     }
> 
>     # fix up the accounts.....
>     if ( $item->{'itemlost'} ) {
>         $messages->{'WasLost'} = 1;
> 
>-        if ( C4::Context->preference('RefundLostItemFeeOnReturn' ) ) {
>-            _FixAccountForLostAndReturned($item->{'itemnumber'}, $borrowernumber, $barcode);    # can tolerate undef $borrowernumber
>+        if ( C4::Context->preference('RefundLostItemFeeOnReturn') ) {
>+            _FixAccountForLostAndReturned( $item->{'itemnumber'},
>+                $borrowernumber, $barcode )
>+              ;    # can tolerate undef $borrowernumber
>             $messages->{'LostItemFeeRefunded'} = 1;
>         }
>     }
> 
>     # fix up the overdues in accounts...
>     if ($borrowernumber) {
>-        my $fix = _FixOverduesOnReturn($borrowernumber, $item->{itemnumber}, $exemptfine, $dropbox);
>-        defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $item->{itemnumber}...) failed!";  # zero is OK, check defined
>-        
>+
>+        # zero is OK, check defined
>+        my $fix =
>+          _FixOverduesOnReturn( $borrowernumber, $item->{itemnumber},
>+            $exemptfine, $dropbox );
>+
>+        warn "_FixOverduesOnReturn($borrowernumber, "
>+          . "$item->{itemnumber}...) failed!"
>+          unless defined($fix);
>+
>         if ( $issue->{overdue} && $issue->{date_due} ) {
>-# fix fine days
>+
>+            # fix fine days
>             my $debardate =
>               _debar_user_on_return( $borrower, $item, $issue->{date_due}, $today );
>             $messages->{Debarred} = $debardate if ($debardate);
>@@ -1876,57 +1919,75 @@ sub AddReturn {
> 
>     # find reserves.....
>     # if we don't have a reserve with the status W, we launch the Checkreserves routine
>-    my ($resfound, $resrec);
>-    ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ) unless ( $item->{'wthdrawn'} );
>+    my ( $resfound, $resrec );
>+    ( $resfound, $resrec, undef ) =
>+      C4::Reserves::CheckReserves( $item->{'itemnumber'} )
>+      unless ( $item->{'wthdrawn'} );
>     if ($resfound) {
>-          $resrec->{'ResFound'} = $resfound;
>+        $resrec->{'ResFound'}   = $resfound;
>         $messages->{'ResFound'} = $resrec;
>     }
> 
>-    # update stats?
>     # Record the fact that this book was returned.
>-    UpdateStats(
>-        $branch, $stat_type, '0', '',
>-        $item->{'itemnumber'},
>-        $biblio->{'itemtype'},
>-        $borrowernumber, undef, $item->{'ccode'}
>-    );
>+    UpdateStats( $branch, $stat_type, '0', '', $item->{'itemnumber'},
>+        $biblio->{'itemtype'}, $borrowernumber, undef, $item->{'ccode'} );
> 
>     # Send a check-in slip. # NOTE: borrower may be undef.  probably shouldn't try to send messages then.
>     my $circulation_alert = 'C4::ItemCirculationAlertPreference';
>-    my %conditions = (
>+    my %conditions        = (
>         branchcode   => $branch,
>         categorycode => $borrower->{categorycode},
>         item_type    => $item->{itype},
>         notification => 'CHECKIN',
>     );
>-    if ($doreturn && $circulation_alert->is_enabled_for(\%conditions)) {
>-        SendCirculationAlert({
>-            type     => 'CHECKIN',
>-            item     => $item,
>-            borrower => $borrower,
>-            branch   => $branch,
>-        });
>+    if ( $doreturn && $circulation_alert->is_enabled_for( \%conditions ) ) {
>+        SendCirculationAlert(
>+            {
>+                type     => 'CHECKIN',
>+                item     => $item,
>+                borrower => $borrower,
>+                branch   => $branch,
>+            }
>+        );
>     }
>-    
>-    logaction("CIRCULATION", "RETURN", $borrowernumber, $item->{'itemnumber'})
>-        if C4::Context->preference("ReturnLog");
>-    
>-    # 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
>-            (C4::Context->preference("UseBranchTransferLimits") and
>-             ! IsBranchTransferAllowed($branch, $hbr, $item->{C4::Context->preference("BranchTransferLimitsType")} )
>-           )) {
>-            $debug and warn sprintf "about to call ModItemTransfer(%s, %s, %s)", $item->{'itemnumber'},$branch, $hbr;
>-            $debug and warn "item: " . Dumper($item);
>-            ModItemTransfer($item->{'itemnumber'}, $branch, $hbr);
>+
>+    logaction( "CIRCULATION", "RETURN", $borrowernumber, $item->{'itemnumber'} )
>+      if C4::Context->preference("ReturnLog");
>+
>+    # If this logged in branch is not the holdingbranch, add the message WasTransferred
>+    # Then, check to see if this item has either a reserve here or a valid transfer, 
>+    # if not, then we need to send it back to the home branch.
>+    if (    ( $doreturn or $messages->{'NotIssued'} )
>+        and !$resfound
>+        and $branch ne $hbr
>+        and !$messages->{'WrongTransfer'} )
>+    {
>+        if (
>+            C4::Context->preference("AutomaticItemReturn")
>+            or (
>+                C4::Context->preference("UseBranchTransferLimits")
>+                and !IsBranchTransferAllowed(
>+                    $branch,
>+                    $hbr,
>+                    $item->{ C4::Context->preference("BranchTransferLimitsType")
>+                      }
>+                )
>+            )
>+          )
>+        {
>+            if ($debug) {
>+                warn sprintf "about to call ModItemTransfer(%s, %s, %s)",
>+                  $item->{'itemnumber'}, $branch, $hbr;
>+                warn "item: " . Dumper($item);
>+            }
>+
>+            ModItemTransfer( $item->{'itemnumber'}, $branch, $hbr );
>             $messages->{'WasTransfered'} = 1;
>-        } else {
>-            $messages->{'NeedsTransfer'} = 1;   # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
>+        }
>+        else {
>+
>+            # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
>+            $messages->{'NeedsTransfer'} = 1;
>         }
>     }
>     return ( $doreturn, $messages, $issue, $borrower );
>-- 
>1.7.2.5

Actions: View | Diff | Splinter Review

Attachments on bug 10374: 18462 | 18463 | 18467 | 18468 | 18469 | 18470 | 18471