Attachment 83654 Details for Bug 22049 – Bug 22049: Make MarkIssueReturned rely on returndate only

[patch] Bug 22049: Make MarkIssueReturned rely on returndate only

Bug-22049-Make-MarkIssueReturned-rely-on-returndat.patch (text/plain), 10.08 KB, created by Charles Farmer on 2019-01-04 19:33:14 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Charles Farmer

Created: 2019-01-04 19:33:14 UTC

Size: 10.08 KB

patch

obsolete

>From cbbde0d87df719fe478fd3184be94875434297e3 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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
>
>Signed-off-by: Owen Leonard <oleonard@myacpl.org>
>
>Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
>---
> C4/Circulation.pm                              | 62 ++++++++++----------------
> misc/cronjobs/longoverdue.pl                   |  2 +-
> t/db_dependent/Circulation/MarkIssueReturned.t | 10 ++---
> 3 files changed, 29 insertions(+), 45 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 0a0680e..19e7941 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -1910,8 +1910,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 ) {
>@@ -2088,30 +2094,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;
>@@ -2127,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->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
>@@ -3714,7 +3700,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)
>@@ -3785,7 +3771,6 @@ sub ProcessOfflineReturn {
>             MarkIssueReturned(
>                 $issue->{borrowernumber},
>                 $itemnumber,
>-                undef,
>                 $operation->{timestamp},
>             );
>             ModItem(
>@@ -3820,7 +3805,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
>--- 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
>--- 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' );
> 
>-- 
>2.7.4

Actions: View | Diff | Splinter Review

Attachments on bug 22049: 83542 | 83543 | 83544 | 83548 | 83549 | 83625 | 83626 | 83627 | 83654 | 83655 | 83656 | 83727 | 83728 | 83729 | 83730 | 85498