From 92cc89d94acd70c696e48e9149e33d9597c72ba1 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 28 Dec 2018 14:39:07 -0300 Subject: [PATCH] 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 --- C4/Circulation.pm | 61 +++++++------------ misc/cronjobs/longoverdue.pl | 2 +- .../Circulation/MarkIssueReturned.t | 10 +-- 3 files changed, 29 insertions(+), 44 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 017e9d7df0..9dc373b05c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1909,8 +1909,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 ) { @@ -2087,29 +2093,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 row to C and -setting C to the current date, or -the last non-holiday date of the branccode specified in -C . Assumes you've already checked that -it's safe to do this, i.e. last non-holiday > issuedate. +setting C 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, -not exported, but it is currently needed by one -routine in C. +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 @@ -2126,41 +2128,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->update({ returndate => $returndate })->discard_changes; # update and refetch + } + else { + $issue->update({ returndate => \'NOW()' })->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->update({ borrowernumber => $anonymouspatron }); } # And finally delete the issue @@ -3700,7 +3687,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) @@ -3771,7 +3758,6 @@ sub ProcessOfflineReturn { MarkIssueReturned( $issue->{borrowernumber}, $itemnumber, - undef, $operation->{timestamp}, ); ModItem( @@ -3806,7 +3792,6 @@ sub ProcessOfflineIssue { MarkIssueReturned( $issue->{borrowernumber}, $itemnumber, - undef, $operation->{timestamp}, ); } diff --git a/misc/cronjobs/longoverdue.pl b/misc/cronjobs/longoverdue.pl index f8fd871b1f..f895f6d451 100755 --- 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 452707aa91..ac55fa6dc6 100644 --- a/t/db_dependent/Circulation/MarkIssueReturned.t +++ b/t/db_dependent/Circulation/MarkIssueReturned.t @@ -62,28 +62,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, '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' ); -- 2.20.1