@@ -, +, @@ returns --- C4/Circulation.pm | 16 +- t/db_dependent/Circulation.t | 236 +++++++++++++++++++++------ t/db_dependent/Circulation/Returns.t | 45 +++++ 3 files changed, 239 insertions(+), 58 deletions(-) --- a/C4/Circulation.pm +++ a/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(); + } } ); --- a/t/db_dependent/Circulation.t +++ a/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; --- a/t/db_dependent/Circulation/Returns.t +++ a/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; --