Bug 22049: Make MarkIssueReturned rely on returndate only

This patch changes the params accepted by
C4::Circulation::MarkIssueReturned by removing the $dropbox_branch
param.

This passed branchcode was only used to initialize the Koha::Calendar
object, but the date arithmetic has already taken place in a couple
places before we reach this point. This logic needs to be simplified
(bug 14591), and this is the starting point.

To test:
- Apply this patch
- Run:
  $ git grep MarkIssueReturned
=> SUCCESS: Check all the uses of the function either originally passed
undef, or now pass the same date that would've been calculated anyway,
in the returndate param.
- Run:
  $ kshell
 k$ prove t/db_dependent/Circulation/MarkIssueReturned.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 5581a84..99fa08a 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1920,8 +1920,14 @@ sub AddReturn {
 
         if ($patron) {
             eval {
-                MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
-                    $circControlBranch, $return_date, $patron->privacy );
+                if ( $dropbox ) {
+                    MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
+                        $dropboxdate, $patron->privacy );
+                }
+                else {
+                    MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
+                        $return_date, $patron->privacy );
+                }
             };
             unless ( $@ ) {
                 if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) {
@@ -2098,30 +2104,25 @@ sub AddReturn {
 
 =head2 MarkIssueReturned
 
-  MarkIssueReturned($borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy);
+  MarkIssueReturned($borrowernumber, $itemnumber, $returndate, $privacy);
 
 Unconditionally marks an issue as being returned by
 moving the C<issues> row to C<old_issues> and
-setting C<returndate> to the current date, or
-the last non-holiday date of the branccode specified in
-C<dropbox_branch> .  Assumes you've already checked that 
-it's safe to do this, i.e. last non-holiday > issuedate.
+setting C<returndate> to the current date.
 
 if C<$returndate> is specified (in iso format), it is used as the date
-of the return. It is ignored when a dropbox_branch is passed in.
+of the return.
 
 C<$privacy> contains the privacy parameter. If the patron has set privacy to 2,
 the old_issue is immediately anonymised
 
 Ideally, this function would be internal to C<C4::Circulation>,
-not exported, but it is currently needed by one 
-routine in C<C4::Accounts>.
+not exported, but it is currently used in misc/cronjobs/longoverdue.pl.
 
 =cut
 
 sub MarkIssueReturned {
-    my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy ) = @_;
-
+    my ( $borrowernumber, $itemnumber, $returndate, $privacy ) = @_;
 
     # Retrieve the issue
     my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return;
@@ -2137,41 +2138,26 @@ sub MarkIssueReturned {
         die "Fatal error: the patron ($borrowernumber) has requested their circulation history be anonymized on check-in, but the AnonymousPatron system preference is empty or not set correctly."
             unless Koha::Patrons->find( $anonymouspatron );
     }
-    my $database = Koha::Database->new();
-    my $schema   = $database->schema;
-    my $dbh   = C4::Context->dbh;
 
-    my $query = 'UPDATE issues SET returndate=';
-    my @bind;
-    if ($dropbox_branch) {
-        my $calendar = Koha::Calendar->new( branchcode => $dropbox_branch );
-        my $dropboxdate = $calendar->addDate( DateTime->now( time_zone => C4::Context->tz), -1 );
-        $query .= ' ? ';
-        push @bind, $dropboxdate->strftime('%Y-%m-%d %H:%M');
-    } elsif ($returndate) {
-        $query .= ' ? ';
-        push @bind, $returndate;
-    } else {
-        $query .= ' now() ';
-    }
-    $query .= ' WHERE issue_id = ?';
-    push @bind, $issue_id;
+    my $schema = Koha::Database->schema;
 
     # FIXME Improve the return value and handle it from callers
     $schema->txn_do(sub {
 
-        # Update the returndate
-        $dbh->do( $query, undef, @bind );
-
-        # We just updated the returndate, so we need to refetch $issue
-        $issue->discard_changes;
+        # Update the returndate value
+        if ( $returndate ) {
+            $issue->returndate( $returndate )->store->discard_changes; # update and refetch
+        }
+        else {
+            $issue->returndate( \'NOW()' )->store->discard_changes; # update and refetch
+        }
 
         # Create the old_issues entry
         my $old_checkout = Koha::Old::Checkout->new($issue->unblessed)->store;
 
         # anonymise patron checkout immediately if $privacy set to 2 and AnonymousPatron is set to a valid borrowernumber
         if ( $privacy == 2) {
-            $dbh->do(q|UPDATE old_issues SET borrowernumber=? WHERE issue_id = ?|, undef, $anonymouspatron, $old_checkout->issue_id);
+            $old_checkout->borrowernumber($anonymouspatron)->store;
         }
 
         # And finally delete the issue
@@ -3728,7 +3714,7 @@ sub LostItem{
             #warn " $issues->{'borrowernumber'}  /  $itemnumber ";
         }
 
-        MarkIssueReturned($borrowernumber,$itemnumber,undef,undef,$patron->privacy) if $mark_returned;
+        MarkIssueReturned($borrowernumber,$itemnumber,undef,$patron->privacy) if $mark_returned;
     }
 
     #When item is marked lost automatically cancel its outstanding transfers and set items holdingbranch to the transfer source branch (frombranch)
@@ -3799,7 +3785,6 @@ sub ProcessOfflineReturn {
             MarkIssueReturned(
                 $issue->{borrowernumber},
                 $itemnumber,
-                undef,
                 $operation->{timestamp},
             );
             ModItem(
@@ -3834,7 +3819,6 @@ sub ProcessOfflineIssue {
             MarkIssueReturned(
                 $issue->{borrowernumber},
                 $itemnumber,
-                undef,
                 $operation->{timestamp},
             );
         }

diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl
index f8fd871..f895f6d 100755 (executable)
--- a/misc/cronjobs/longoverdue.pl
+++ b/misc/cronjobs/longoverdue.pl
@@ -313,7 +313,7 @@ foreach my $startrange (sort keys %$lost) {
                     LostItem( $row->{'itemnumber'}, 'cronjob', $mark_returned );
                 } elsif ( $mark_returned ) {
                     my $patron = Koha::Patrons->find( $row->{borrowernumber} );
-                    MarkIssueReturned($row->{borrowernumber},$row->{itemnumber},undef,undef,$patron->privacy)
+                    MarkIssueReturned($row->{borrowernumber},$row->{itemnumber},undef,$patron->privacy)
                 }
             }
             $count++;

diff --git a/t/db_dependent/Circulation/MarkIssueReturned.t b/t/db_dependent/Circulation/MarkIssueReturned.t
index 7f2289f..5538e1b 100644 (file)
--- a/t/db_dependent/Circulation/MarkIssueReturned.t
+++ b/t/db_dependent/Circulation/MarkIssueReturned.t
@@ -61,28 +61,28 @@ subtest 'anonymous patron' => sub {
     # The next call will raise an error, because data are not correctly set
     t::lib::Mocks::mock_preference('AnonymousPatron', '');
     my $issue = C4::Circulation::AddIssue( $patron, $item->{barcode} );
-    eval { C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, 'dropbox_branch', 'returndate', 2 ) };
+    eval { C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, 2 ) };
     like ( $@, qr<Fatal error: the patron \(\d+\) .* AnonymousPatron>, 'AnonymousPatron is not set - Fatal error on anonymization' );
     Koha::Checkouts->find( $issue->issue_id )->delete;
 
     my $anonymous_borrowernumber = Koha::Patron->new({categorycode => $patron_category->{categorycode}, branchcode => $library->{branchcode} })->store->borrowernumber;
     t::lib::Mocks::mock_preference('AnonymousPatron', $anonymous_borrowernumber);
     $issue = C4::Circulation::AddIssue( $patron, $item->{barcode} );
-    eval { C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, 'dropbox_branch', 'returndate', 2 ) };
+    eval { C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, 2 ) };
     is ( $@, q||, 'AnonymousPatron is set correctly - no error expected');
 };
 
 my ( $issue_id, $issue );
 # The next call will return undef for invalid item number
-eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, 'invalid_itemnumber', 'dropbox_branch', 'returndate', 0 ) };
+eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, 'invalid_itemnumber', undef, 0 ) };
 is( $@, '', 'No die triggered by invalid itemnumber' );
 is( $issue_id, undef, 'No issue_id returned' );
 
 # In the next call we return the item and try it another time
 $issue = C4::Circulation::AddIssue( $patron, $item->{barcode} );
-eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, undef, 0 ) };
+eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, 0 ) };
 is( $issue_id, $issue->issue_id, "Item has been returned (issue $issue_id)" );
-eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, undef, 0 ) };
+eval { $issue_id = C4::Circulation::MarkIssueReturned( $patron->{borrowernumber}, $item->{itemnumber}, undef, 0 ) };
 is( $@, '', 'No crash on returning item twice' );
 is( $issue_id, undef, 'Cannot return an item twice' );