From e5f05f2634f2a0ca5e90eb38cdba20e3ab6aee62 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Mon, 22 May 2017 14:26:25 -0300
Subject: [PATCH] Bug 18651: Do not charge if the checkin failed

2. If the move fails for whatever reason (see
https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an
example), fines can be charged. It should not

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(cherry picked from commit acb7a717482c2a47f7da6a0d883784415bf3b8d5)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
---
 C4/Circulation.pm                    | 10 +++++-----
 t/db_dependent/Circulation/Returns.t | 27 ++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 41f6c70e0d..7e142ef702 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1934,16 +1934,16 @@ sub AddReturn {
         }
 
         if ($borrowernumber) {
-            if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) {
-                _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } );
-            }
-
             eval {
                 my $issue_id = MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
                     $circControlBranch, $return_date, $borrower->{'privacy'} );
                 $issue->{issue_id} = $issue_id;
             };
-            if ( $@ ) {
+            unless ( $@ ) {
+                if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) {
+                    _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } );
+                }
+            } else {
                 $messages->{'Wrongbranch'} = {
                     Wrongbranch => $branch,
                     Rightbranch => $message
diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t
index fe98452b5c..9d295e5985 100644
--- a/t/db_dependent/Circulation/Returns.t
+++ b/t/db_dependent/Circulation/Returns.t
@@ -29,6 +29,7 @@ use C4::Circulation;
 use C4::Items;
 use C4::Members;
 use Koha::Database;
+use Koha::Account::Lines;
 use Koha::DateUtils;
 use Koha::Items;
 
@@ -263,9 +264,15 @@ subtest "AddReturn logging on statistics table (item-level_itypes=0)" => sub {
 };
 
 subtest 'Handle ids duplication' => sub {
-    plan tests => 2;
+    plan tests => 4;
+
+    t::lib::Mocks::mock_preference( 'item-level_itypes', 1 );
+    t::lib::Mocks::mock_preference( 'CalculateFinesOnReturn', 1 );
+    t::lib::Mocks::mock_preference( 'finesMode', 'production' );
+    Koha::IssuingRules->search->update({ chargeperiod => 1, fine => 1, firstremind => 1, });
 
     my $biblio = $builder->build( { source => 'Biblio' } );
+    my $itemtype = $builder->build( { source => 'Itemtype', value => { rentalcharge => 5 } } );
     my $item = $builder->build(
         {
             source => 'Item',
@@ -274,19 +281,25 @@ subtest 'Handle ids duplication' => sub {
                 notforloan => 0,
                 itemlost   => 0,
                 withdrawn  => 0,
-
+                itype      => $itemtype->{itemtype},
             }
         }
     );
     my $patron = $builder->build({source => 'Borrower'});
+    $patron = Koha::Patrons->find( $patron->{borrowernumber} );
+
+    my $original_checkout = AddIssue( $patron->unblessed, $item->{barcode}, dt_from_string->subtract( days => 50 ) );
+    $builder->build({ source => 'OldIssue', value => { issue_id => $original_checkout->issue_id } });
+    my $old_checkout = Koha::Old::Checkouts->find( $original_checkout->issue_id );
+
+    AddRenewal( $patron->borrowernumber, $item->{itemnumber} );
 
-    my $checkout = AddIssue( $patron, $item->{barcode} );
-    $builder->build({ source => 'OldIssue', value => { issue_id => $checkout->issue_id } });
+    my ($doreturn, $messages, $new_checkout, $borrower) = AddReturn( $item->{barcode}, undef, undef, undef, dt_from_string );
 
-    my ($doreturn, $messages, $issue, $borrower) = AddReturn( $item->{barcode} );
-    my $old_checkout = Koha::Old::Checkouts->find( $checkout->issue_id );
+    my $account_lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber, issue_id => $new_checkout->{issue_id} });
+    is( $account_lines->count, 1, 'One account line should exist on new issue_id' );
 
-    isnt( $checkout->issue_id, $issue->{issue_id}, 'AddReturn should return the issue with the new issue_id' );
+    isnt( $original_checkout->issue_id, $new_checkout->{issue_id}, 'AddReturn should return the issue with the new issue_id' );
     isnt( $old_checkout->itemnumber, $item->{itemnumber}, 'If an item is checked-in, it should be moved to old_issues even if the issue_id already existed in the table' );
 };
 
-- 
2.39.5