From e5f05f2634f2a0ca5e90eb38cdba20e3ab6aee62 Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 Signed-off-by: Chris Cormack Signed-off-by: Marcel de Rooy (cherry picked from commit acb7a717482c2a47f7da6a0d883784415bf3b8d5) Signed-off-by: Fridolin Somers --- 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