From b8662ce83d790dde07f93818004ed6e4f4a3af95 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 13 May 2020 14:11:16 +0100 Subject: [PATCH] Bug 8338: [19.11] Remove zero amount overdues on backdated returns [19.11.x] This is a squashed patchset ready for application to 19.11 * Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. * Fix test This patch moves the previous test introduced with bug 24075 into the same block as the rest of the AddReturn tests and updates it to test for the new 'remove accountline' behaviour as well as the reduce and refund behaviour. --- C4/Circulation.pm | 16 +- t/db_dependent/Circulation.t | 236 +++++++++++++++++++++------ t/db_dependent/Circulation/Returns.t | 45 +++++ 3 files changed, 239 insertions(+), 58 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 0eaf6f3a7a..ba783a71e5 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2034,7 +2034,7 @@ sub AddReturn { # fix up the overdues in accounts... if ($borrowernumber) { my $fix = _FixOverduesOnReturn( $borrowernumber, $item->itemnumber, $exemptfine, 'RETURNED' ); - defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $item->itemnumber...) failed!"; # zero is OK, check defined + defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, ".$item->itemnumber."...) failed!"; # zero is OK, check defined if ( $issue and $issue->is_overdue($return_date) ) { # fix fine days @@ -2406,9 +2406,12 @@ sub _FixOverduesOnReturn { return 0 unless $accountlines->count; # no warning, there's just nothing to fix my $accountline = $accountlines->next; + my $payments = $accountline->credits; my $amountoutstanding = $accountline->amountoutstanding; - if ($exemptfine && ($amountoutstanding != 0)) { + if ( $accountline->amount == 0 && $payments->count == 0 ) { + $accountline->delete; + } elsif ($exemptfine && ($amountoutstanding != 0)) { my $account = Koha::Account->new({patron_id => $borrowernumber}); my $credit = $account->add_credit( { @@ -2423,16 +2426,17 @@ sub _FixOverduesOnReturn { $credit->apply({ debits => [ $accountline ], offset_type => 'Forgiven' }); - $accountline->status('FORGIVEN'); - if (C4::Context->preference("FinesLog")) { &logaction("FINES", 'MODIFY',$borrowernumber,"Overdue forgiven: item $item"); } + + $accountline->status('FORGIVEN'); + $accountline->store(); } else { $accountline->status($status); - } + $accountline->store(); - return $accountline->store(); + } } ); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 9b53952774..0144de6b4c 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 47; +use Test::More tests => 46; use Test::MockModule; use Data::Dumper; @@ -2034,7 +2034,7 @@ subtest 'CanBookBeIssued + AutoReturnCheckedOutItems' => sub { subtest 'AddReturn | is_overdue' => sub { - plan tests => 6; + plan tests => 8; t::lib::Mocks::mock_preference('CalculateFinesOnReturn', 1); t::lib::Mocks::mock_preference('finesMode', 'production'); @@ -2068,6 +2068,7 @@ subtest 'AddReturn | is_overdue' => sub { my $now = dt_from_string; my $one_day_ago = $now->clone->subtract( days => 1 ); + my $two_days_ago = $now->clone->subtract( days => 2 ); my $five_days_ago = $now->clone->subtract( days => 5 ); my $ten_days_ago = $now->clone->subtract( days => 10 ); $patron = Koha::Patrons->find( $patron->{borrowernumber} ); @@ -2132,9 +2133,9 @@ subtest 'AddReturn | is_overdue' => sub { Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete; }; - subtest 'bug 25417 | backdated return + exemptfine' => sub { + subtest 'bug 8338 | backdated return resulting in zero amount fine' => sub { - plan tests => 6; + plan tests => 17; t::lib::Mocks::mock_preference('CalculateFinesOnBackdate', 1); @@ -2156,20 +2157,194 @@ subtest 'AddReturn | is_overdue' => sub { is( int( $patron->account->balance() ), 1, "Overdue fine of 1 day overdue" ); - # Backdated return (dropbox mode example - charge should exist but be zero) + # Backdated return (dropbox mode example - charge should be removed) AddReturn( $item->{barcode}, $library->{branchcode}, 1, $one_day_ago ); is( int( $patron->account->balance() ), 0, "Overdue fine should be annulled" ); my $lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber }); - is( $lines->count, 1, "Overdue fine accountlines still exists"); + is( $lines->count, 0, "Overdue fine accountline has been removed"); + + $issue = AddIssue( $patron->unblessed, $item->{barcode}, $two_days_ago ); # date due was 2d ago + + # Fake fines cronjob on this checkout + ($fine) = + CalcFine( $item, $patron->categorycode, $library->{branchcode}, + $two_days_ago, $now ); + UpdateFine( + { + issue_id => $issue->issue_id, + itemnumber => $item->{itemnumber}, + borrowernumber => $patron->borrowernumber, + amount => $fine, + due => output_pref($one_day_ago) + } + ); + is( int( $patron->account->balance() ), + 2, "Overdue fine of 2 days overdue" ); + + # Payment made against fine + $lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber }); + my $debit = $lines->next; + my $credit = $patron->account->add_credit( + { + amount => 2, + type => 'PAYMENT', + interface => 'test', + } + ); + $credit->apply( + { debits => [ $debit ], offset_type => 'Payment' } ); + + is( int( $patron->account->balance() ), + 0, "Overdue fine should be paid off" ); + $lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber }); + is ( $lines->count, 2, "Overdue (debit) and Payment (credit) present"); my $line = $lines->next; - is($line->amount+0,0, "Overdue fine amount has been reduced to 0"); - is($line->amountoutstanding+0,0, "Overdue fine amount outstanding has been reduced to 0"); - is($line->status,'RETURNED', "Overdue fine was fixed"); + is( $line->amount+0, 2, "Overdue fine amount remains as 2 days"); + is( $line->amountoutstanding+0, 0, "Overdue fine amountoutstanding reduced to 0"); + + # Backdated return (dropbox mode example - charge should be removed) + AddReturn( $item->{barcode}, $library->{branchcode}, undef, $one_day_ago ); + is( int( $patron->account->balance() ), + -1, "Refund credit has been applied" ); + $lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber }, { order_by => { '-asc' => 'accountlines_id' }}); + is( $lines->count, 3, "Overdue (debit), Payment (credit) and Refund (credit) are all present"); + + $line = $lines->next; + is($line->amount+0,1, "Overdue fine amount has been reduced to 1"); + is($line->amountoutstanding+0,0, "Overdue fine amount outstanding remains at 0"); + is($line->status,'RETURNED', "Overdue fine is fixed"); + $line = $lines->next; + is($line->amount+0,-2, "Original payment amount remains as 2"); + is($line->amountoutstanding+0,0, "Original payment remains applied"); + $line = $lines->next; + is($line->amount+0,-1, "Refund amount correctly set to 1"); + is($line->amountoutstanding+0,-1, "Refund amount outstanding unspent"); + + # Cleanup + Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete; + }; + + subtest 'bug 25417 | backdated return + exemptfine' => sub { + + plan tests => 2; + + t::lib::Mocks::mock_preference('CalculateFinesOnBackdate', 1); + + my $issue = AddIssue( $patron->unblessed, $item->{barcode}, $one_day_ago ); # date due was 1d ago + + # Fake fines cronjob on this checkout + my ($fine) = + CalcFine( $item, $patron->categorycode, $library->{branchcode}, + $one_day_ago, $now ); + UpdateFine( + { + issue_id => $issue->issue_id, + itemnumber => $item->{itemnumber}, + borrowernumber => $patron->borrowernumber, + amount => $fine, + due => output_pref($one_day_ago) + } + ); + is( int( $patron->account->balance() ), + 1, "Overdue fine of 1 day overdue" ); + + # Backdated return (dropbox mode example - charge should no longer exist) + AddReturn( $item->{barcode}, $library->{branchcode}, 1, $one_day_ago ); + is( int( $patron->account->balance() ), + 0, "Overdue fine should be annulled" ); # Cleanup Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber })->delete; }; + + subtest 'bug 24075 | backdated return with return datetime matching due datetime' => sub { + plan tests => 7; + + t::lib::Mocks::mock_preference( 'CalculateFinesOnBackdate', 1 ); + + my $due_date = dt_from_string; + my $issue = AddIssue( $patron->unblessed, $item->{barcode}, $due_date ); + + # Add fine + UpdateFine( + { + issue_id => $issue->issue_id, + itemnumber => $item->{itemnumber}, + borrowernumber => $patron->borrowernumber, + amount => 0.25, + due => output_pref($due_date) + } + ); + is( $patron->account->balance(), + 0.25, 'Overdue fine of $0.25 recorded' ); + + # Backdate return to exact due date and time + my ( undef, $message ) = + AddReturn( $item->{barcode}, $library->{branchcode}, + undef, $due_date ); + + my $accountline = + Koha::Account::Lines->find( { issue_id => $issue->id } ); + ok( !$accountline, 'accountline removed as expected' ); + + # Re-issue + $issue = AddIssue( $patron->unblessed, $item->{barcode}, $due_date ); + + # Add fine + UpdateFine( + { + issue_id => $issue->issue_id, + itemnumber => $item->{itemnumber}, + borrowernumber => $patron->borrowernumber, + amount => .25, + due => output_pref($due_date) + } + ); + is( $patron->account->balance(), + 0.25, 'Overdue fine of $0.25 recorded' ); + + # Partial pay accruing fine + my $lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->borrowernumber, + issue_id => $issue->id + } + ); + my $debit = $lines->next; + my $credit = $patron->account->add_credit( + { + amount => .20, + type => 'PAYMENT', + interface => 'test', + } + ); + $credit->apply( { debits => [$debit], offset_type => 'Payment' } ); + + is( $patron->account->balance(), .05, 'Overdue fine reduced to $0.05' ); + + # Backdate return to exact due date and time + ( undef, $message ) = + AddReturn( $item->{barcode}, $library->{branchcode}, + undef, $due_date ); + + $lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->borrowernumber, + issue_id => $issue->id + } + ); + $accountline = $lines->next; + is( $accountline->amountoutstanding + 0, + 0, 'Partially paid fee amount outstanding was reduced to 0' ); + is( $accountline->amount + 0, + 0, 'Partially paid fee amount was reduced to 0' ); + is( $patron->account->balance(), -0.20, 'Patron refund recorded' ); + + # Cleanup + Koha::Account::Lines->search( + { borrowernumber => $patron->borrowernumber } )->delete; + }; }; subtest '_FixAccountForLostAndReturned' => sub { @@ -3461,49 +3636,6 @@ subtest 'CanBookBeIssued & RentalFeesCheckoutConfirmation' => sub { $itemtype->rentalcharge_daily('0')->store; }; -subtest "Test Backdating of Returns" => sub { - plan tests => 2; - - my $branch = $library2->{branchcode}; - my $biblio = $builder->build_sample_biblio(); - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $branch, - itype => $itemtype, - } - ); - - my %a_borrower_data = ( - firstname => 'Kyle', - surname => 'Hall', - categorycode => $patron_category->{categorycode}, - branchcode => $branch, - ); - my $borrowernumber = Koha::Patron->new(\%a_borrower_data)->store->borrowernumber; - my $borrower = Koha::Patrons->find( $borrowernumber )->unblessed; - - my $due_date = dt_from_string; - my $issue = AddIssue( $borrower, $item->barcode, $due_date ); - UpdateFine( - { - issue_id => $issue->id(), - itemnumber => $item->itemnumber, - borrowernumber => $borrowernumber, - amount => .25, - amountoutstanding => .25, - type => q{} - } - ); - - - my ( undef, $message ) = AddReturn( $item->barcode, $branch, undef, $due_date ); - - my $accountline = Koha::Account::Lines->find( { issue_id => $issue->id } ); - is( $accountline->amountoutstanding+0, 0, 'Fee amount outstanding was reduced to 0' ); - is( $accountline->amount+0, 0, 'Fee amount was reduced to 0' ); -}; - subtest 'Filling a hold should cancel existing transfer' => sub { plan tests => 4; diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t index 5def587366..345c06e107 100644 --- a/t/db_dependent/Circulation/Returns.t +++ b/t/db_dependent/Circulation/Returns.t @@ -394,4 +394,49 @@ subtest 'BranchTransferLimitsType' => sub { is( $doreturn, 1, 'AddReturn should have checkin the item if BranchTransferLimitsType=itemtype'); }; +subtest 'Backdated returns should reduce fine if needed' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference( "CalculateFinesOnReturn", 0 ); + t::lib::Mocks::mock_preference( "CalculateFinesOnBackdate", 1 ); + + my $biblio = $builder->build_object( { class => 'Koha::Biblios' } ); + my $item = $builder->build_object( + { + class => 'Koha::Items', + value => { + biblionumber => $biblio->biblionumber, + notforloan => 0, + itemlost => 0, + withdrawn => 0, + } + } + ); + my $patron = $builder->build_object({class => 'Koha::Patrons'}); + my $checkout = AddIssue( $patron->unblessed, $item->barcode ); + my $fine = Koha::Account::Line->new({ + issue_id => $checkout->id, + borrowernumber => $patron->id, + itemnumber => $item->id, + date => dt_from_string(), + amount => 100, + amountoutstanding => 100, + debit_type_code => 'OVERDUE', + status => 'UNRETURNED', + timestamp => dt_from_string(), + manager_id => undef, + interface => 'cli', + branchcode => $patron->branchcode, + })->store(); + + my $account = $patron->account; + is( $account->balance+0, 100, "Account balance before return is 100"); + + my ( $doreturn, $messages, $issue ) = AddReturn($item->barcode, undef, undef, dt_from_string('1999-01-01') ); + is( $account->balance+0, 0, "Account balance after return is 0"); + + $fine = $fine->get_from_storage; + is( $fine, undef, "Fine was removed correctly with a backdated return" ); +}; + $schema->storage->txn_rollback; -- 2.20.1