From d25d2e2a450419865c5962f5e0ff54b4ca54da72 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 18 Aug 2020 13:26:29 +0100 Subject: [PATCH] Bug 18501: [20.05.x] Automatic refunds need protection from failure Bug 18501: (QA follow-up) Add test of *_on functionality This patch adds a test for the *_on trigger functionality to ensure matching behaviour before and after the triggers change. Signed-off-by: Jonathan Druart Bug 18501: Prepare the ground This code was duplicated and we are going to need it. Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: Auto refund if mark as found from cataloguing If an item is marked as found (ie. not itemlost) we need to refund the patron. Test plan: Mark an item as lost to create a fee for the patron Mark it as found from the cataloguing module and confirm that the patron is refunded Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: but.. what was the meaning of holdingbranch here? Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: wip - need to know it has been refunded Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: set the is_refunded flag as internal var After discussion with Martin we decided that it could be the correct way Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: (QA follow-up) Remove FIXME + Track issue_id We correct some compilation errors here by updating the transfered _FixAccountForLostAndFound method to use $self references. We add issue_id tracking so that future reports can clearly see how a refund relates to the original charge and finally we remove a FIXME as I agree that the 'discard_changes' call is not required. Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: (QA follow-up) Migrate and update unit tests This patch moves the _FixAccountForLostAndFound tests from Circulation.t to Koha/Items.t and updates then to call an item store as aposed to the now removed _FixAccountForLostAndFound method. Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests The migration of unit tests in the preceeding patch highlighted a few regressions which are dealt with here. Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Bug 18501: Fix QA issues FAIL Koha/Item.pm FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). FAIL spelling Retreive ==> Retrieve appropriatly ==> appropriately somehwere ==> somewhere Bug 18501: (follow-up) Remove duplicate test The ModItem test found in t/db_dependent/Items.t was a duplciate (but slightly less functional) for the test we introduced for the *_on trigger functionality in t/db_dependent/Koha/Items.t::store The failure was a missing C4::Context mock, but the logical fix is to simply remove the duplicate test. Signed-off-by: Martin Renvoize Bug 18501: (follow-up) Test undefined userenv behaviour This patch adds a test to ensure that the 'Koha::Item->store' method will function when the userenv is not set Bug 18501: (follow-up) Do not rely on userenv->{branchcode} Bug 18501: (follow-up) Add fallback to ItemHomeBranch When 'RefundLostOnReturnControl' is set to 'CheckinLibrary' but return_branch is passed as undefined we should fallback to 'ItemHomeBranch' before falling back to the default rule. Bug 18501: (follow-up) Use t::lib::Dates::compare in tests Bug 18501: Don't use paidfor for DB fields not mapped with a MARC field On "Bug 25964: Prevent data loss when editing items from a MARC record" we picked paidfor as an example, but it has a specific behaviour and is now handled in Koha::Item->store. Let's pick another one, new_status --- C4/Circulation.pm | 137 +----- Koha/CirculationRules.pm | 4 +- Koha/Item.pm | 190 +++++++-- cataloguing/additem.pl | 5 +- t/db_dependent/Circulation.t | 435 +------------------ t/db_dependent/Items.t | 33 +- t/db_dependent/Koha/CirculationRules.t | 15 +- t/db_dependent/Koha/Items.t | 756 ++++++++++++++++++++++++++++++++- 8 files changed, 935 insertions(+), 640 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 7c2be766bb..1a9a1634cc 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1446,22 +1446,6 @@ sub AddIssue { UpdateTotalIssues( $item_object->biblionumber, 1 ); } - ## If item was lost, it has now been found, reverse any list item charges if necessary. - if ( $item_object->itemlost ) { - if ( - Koha::CirculationRules->get_lostreturn_policy( - { - return_branch => C4::Context->userenv->{branch}, - item => $item_object - } - ) - ) - { - _FixAccountForLostAndFound( $item_object->itemnumber, undef, - $item_object->barcode ); - } - } - $item_object->issues( ( $item_object->issues || 0 ) + 1); $item_object->holdingbranch(C4::Context->userenv->{'branch'}); $item_object->itemlost(0); @@ -1999,13 +1983,21 @@ sub AddReturn { # the holdingbranch is updated if the document is returned to another location. # this is always done regardless of whether the item was on loan or not - my $item_holding_branch = $item->holdingbranch; if ($item->holdingbranch ne $branch) { $item->holdingbranch($branch)->store; } + my $item_was_lost = $item->itemlost; my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0; - ModDateLastSeen( $item->itemnumber, $leave_item_lost ); + my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost ); # will unset itemlost if needed + + # fix up the accounts..... + if ( $item_was_lost ) { + $messages->{'WasLost'} = 1; + unless ( C4::Context->preference("BlockReturnOfLostItems") ) { + $messages->{'LostItemFeeRefunded'} = $updated_item->{_refunded}; + } + } # check if we have a transfer for this document my ($datesent,$frombranch,$tobranch) = GetTransfers( $item->itemnumber ); @@ -2027,26 +2019,6 @@ sub AddReturn { } } - # fix up the accounts..... - if ( $item->itemlost ) { - $messages->{'WasLost'} = 1; - unless ( C4::Context->preference("BlockReturnOfLostItems") ) { - if ( - Koha::CirculationRules->get_lostreturn_policy( - { - return_branch => C4::Context->userenv->{branch}, - item => $item, - } - ) - ) - { - _FixAccountForLostAndFound( $item->itemnumber, - $borrowernumber, $barcode ); - $messages->{'LostItemFeeRefunded'} = 1; - } - } - } - # fix up the overdues in accounts... if ($borrowernumber) { my $fix = _FixOverduesOnReturn( $borrowernumber, $item->itemnumber, $exemptfine, 'RETURNED' ); @@ -2467,95 +2439,6 @@ sub _FixOverduesOnReturn { return $result; } -=head2 _FixAccountForLostAndFound - - &_FixAccountForLostAndFound($itemnumber, [$borrowernumber, $barcode]); - -Finds the most recent lost item charge for this item and refunds the borrower -appropriatly, taking into account any payments or writeoffs already applied -against the charge. - -Internal function, not exported, called only by AddReturn. - -=cut - -sub _FixAccountForLostAndFound { - my $itemnumber = shift or return; - my $borrowernumber = @_ ? shift : undef; - my $item_id = @_ ? shift : $itemnumber; # Send the barcode if you want that logged in the description - - my $credit; - - # check for charge made for lost book - my $accountlines = Koha::Account::Lines->search( - { - itemnumber => $itemnumber, - debit_type_code => 'LOST', - status => [ undef, { '<>' => 'FOUND' } ] - }, - { - order_by => { -desc => [ 'date', 'accountlines_id' ] } - } - ); - - return unless $accountlines->count > 0; - my $accountline = $accountlines->next; - my $total_to_refund = 0; - - return unless $accountline->borrowernumber; - my $patron = Koha::Patrons->find( $accountline->borrowernumber ); - return unless $patron; # Patron has been deleted, nobody to credit the return to - - my $account = $patron->account; - - # Use cases - if ( $accountline->amount > $accountline->amountoutstanding ) { - # some amount has been cancelled. collect the offsets that are not writeoffs - # this works because the only way to subtract from this kind of a debt is - # using the UI buttons 'Pay' and 'Write off' - my $credits_offsets = Koha::Account::Offsets->search({ - debit_id => $accountline->id, - credit_id => { '!=' => undef }, # it is not the debit itself - type => { '!=' => 'Writeoff' }, - amount => { '<' => 0 } # credits are negative on the DB - }); - - $total_to_refund = ( $credits_offsets->count > 0 ) - ? $credits_offsets->total * -1 # credits are negative on the DB - : 0; - } - - my $credit_total = $accountline->amountoutstanding + $total_to_refund; - - if ( $credit_total > 0 ) { - my $branchcode = C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; - $credit = $account->add_credit( - { - amount => $credit_total, - description => 'Item found ' . $item_id, - type => 'LOST_FOUND', - interface => C4::Context->interface, - library_id => $branchcode, - item_id => $itemnumber - } - ); - - $credit->apply( { debits => [ $accountline ] } ); - } - - # Update the account status - $accountline->discard_changes->status('FOUND'); - $accountline->store; - - $accountline->item->paidfor('')->store({ log_action => 0 }); - - if ( defined $account and C4::Context->preference('AccountAutoReconcile') ) { - $account->reconcile_balance; - } - - return ($credit) ? $credit->id : undef; -} - =head2 _GetCircControlBranch my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower); diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index 0b989daffa..79c499238f 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -436,8 +436,8 @@ sub get_lostreturn_policy { my $behaviour = C4::Context->preference( 'RefundLostOnReturnControl' ) // 'CheckinLibrary'; my $behaviour_mapping = { - CheckinLibrary => $params->{'return_branch'}, - ItemHomeBranch => $item->homebranch, + CheckinLibrary => $params->{'return_branch'} // $item->homebranch, + ItemHomeBranch => $item->homebranch, ItemHoldingBranch => $item->holdingbranch }; diff --git a/Koha/Item.pm b/Koha/Item.pm index d02817b1f2..dc9c3b66cc 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -117,38 +117,46 @@ sub store { } else { # ModItem - { # Update *_on fields if needed - # Why not for AddItem as well? - my @fields = qw( itemlost withdrawn damaged ); - - # Only retrieve the item if we need to set an "on" date field - if ( $self->itemlost || $self->withdrawn || $self->damaged ) { - my $pre_mod_item = $self->get_from_storage; - for my $field (@fields) { - if ( $self->$field - and not $pre_mod_item->$field ) - { - my $field_on = "${field}_on"; - $self->$field_on( - DateTime::Format::MySQL->format_datetime( dt_from_string() ) - ); - } - } - } + my %updated_columns = $self->_result->get_dirty_columns; + return $self->SUPER::store unless %updated_columns; - # If the field is defined but empty, we are removing and, - # and thus need to clear out the 'on' field as well - for my $field (@fields) { - if ( defined( $self->$field ) && !$self->$field ) { - my $field_on = "${field}_on"; - $self->$field_on(undef); - } + # Retrieve the item for comparison if we need to + my $pre_mod_item = ( + exists $updated_columns{itemlost} + or exists $updated_columns{withdrawn} + or exists $updated_columns{damaged} + ) ? $self->get_from_storage : undef; + + # Update *_on fields if needed + # FIXME: Why not for AddItem as well? + my @fields = qw( itemlost withdrawn damaged ); + for my $field (@fields) { + + # If the field is defined but empty or 0, we are + # removing/unsetting and thus need to clear out + # the 'on' field + if ( exists $updated_columns{$field} + && defined( $self->$field ) + && !$self->$field ) + { + my $field_on = "${field}_on"; + $self->$field_on(undef); + } + # If the field has changed otherwise, we much update + # the 'on' field + elsif (exists $updated_columns{$field} + && $updated_columns{$field} + && !$pre_mod_item->$field ) + { + my $field_on = "${field}_on"; + $self->$field_on( + DateTime::Format::MySQL->format_datetime( + dt_from_string() + ) + ); } } - my %updated_columns = $self->_result->get_dirty_columns; - return $self->SUPER::store unless %updated_columns; - if ( exists $updated_columns{itemcallnumber} or exists $updated_columns{cn_source} ) { @@ -165,6 +173,16 @@ sub store { $self->permanent_location( $self->location ); } + # If item was lost and has now been found, + # reverse any list item charges if necessary. + if ( exists $updated_columns{itemlost} + and $updated_columns{itemlost} <= 0 + and $pre_mod_item->itemlost > 0 ) + { + $self->_set_found_trigger($pre_mod_item); + $self->paidfor(''); + } + $self->_after_item_action_hooks({ action => 'modify' }); logaction( "CATALOGUING", "MODIFY", $self->itemnumber, "item " . Dumper($self->unblessed) ) @@ -760,6 +778,122 @@ sub renewal_branchcode { return $branchcode; } +=head3 _set_found_trigger + + $self->_set_found_trigger + +Finds the most recent lost item charge for this item and refunds the patron +appropriately, taking into account any payments or writeoffs already applied +against the charge. + +Internal function, not exported, called only by Koha::Item->store. + +=cut + +sub _set_found_trigger { + my ( $self, $pre_mod_item ) = @_; + + ## If item was lost, it has now been found, reverse any list item charges if necessary. + my $no_refund_after_days = + C4::Context->preference('NoRefundOnLostReturnedItemsAge'); + if ($no_refund_after_days) { + my $today = dt_from_string(); + my $lost_age_in_days = + dt_from_string( $pre_mod_item->itemlost_on )->delta_days($today) + ->in_units('days'); + + return $self unless $lost_age_in_days < $no_refund_after_days; + } + + return $self + unless Koha::CirculationRules->get_lostreturn_policy( + { + item => $self, + return_branch => C4::Context->userenv + ? C4::Context->userenv->{'branch'} + : undef, + } + ); + + # check for charge made for lost book + my $accountlines = Koha::Account::Lines->search( + { + itemnumber => $self->itemnumber, + debit_type_code => 'LOST', + status => [ undef, { '<>' => 'FOUND' } ] + }, + { + order_by => { -desc => [ 'date', 'accountlines_id' ] } + } + ); + + return $self unless $accountlines->count > 0; + + my $accountline = $accountlines->next; + my $total_to_refund = 0; + + return $self unless $accountline->borrowernumber; + + my $patron = Koha::Patrons->find( $accountline->borrowernumber ); + return $self + unless $patron; # Patron has been deleted, nobody to credit the return to + # FIXME Should not we notify this somewhere + + my $account = $patron->account; + + # Use cases + if ( $accountline->amount > $accountline->amountoutstanding ) { + + # some amount has been cancelled. collect the offsets that are not writeoffs + # this works because the only way to subtract from this kind of a debt is + # using the UI buttons 'Pay' and 'Write off' + my $credits_offsets = Koha::Account::Offsets->search( + { + debit_id => $accountline->id, + credit_id => { '!=' => undef }, # it is not the debit itself + type => { '!=' => 'Writeoff' }, + amount => { '<' => 0 } # credits are negative on the DB + } + ); + + $total_to_refund = ( $credits_offsets->count > 0 ) + ? $credits_offsets->total * -1 # credits are negative on the DB + : 0; + } + + my $credit_total = $accountline->amountoutstanding + $total_to_refund; + + my $credit; + if ( $credit_total > 0 ) { + my $branchcode = + C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; + $credit = $account->add_credit( + { + amount => $credit_total, + description => 'Item found ' . $self->itemnumber, + type => 'LOST_FOUND', + interface => C4::Context->interface, + library_id => $branchcode, + item_id => $self->itemnumber, + issue_id => $accountline->issue_id + } + ); + + $credit->apply( { debits => [$accountline] } ); + $self->{_refunded} = 1; + } + + # Update the account status + $accountline->status('FOUND'); + $accountline->store(); + + if ( defined $account and C4::Context->preference('AccountAutoReconcile') ) { + $account->reconcile_balance; + } + + return $self; +} + =head3 to_api_mapping This method returns the mapping for representing a Koha::Item object diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl index aeaa7536bc..f70432c3e5 100755 --- a/cataloguing/additem.pl +++ b/cataloguing/additem.pl @@ -732,8 +732,9 @@ if ($op eq "additem") { $itemnumber = q{}; my $olditemlost = $item->itemlost; my $newitemlost = $newitem->{itemlost}; - LostItem( $item->itemnumber, 'additem' ) - if $newitemlost && $newitemlost ge '1' && !$olditemlost; + if ( $newitemlost && $newitemlost ge '1' && !$olditemlost ) { + LostItem( $item->itemnumber, 'additem' ) + } } $nextop="additem"; } elsif ($op eq "delinkitem"){ diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index bd0b9a29cc..526edb54d1 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 => 45; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -2668,397 +2668,6 @@ subtest 'AddReturn | is_overdue' => sub { }; }; -subtest '_FixAccountForLostAndFound' => sub { - - plan tests => 5; - - t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); - t::lib::Mocks::mock_preference( 'WhenLostForgiveFine', 0 ); - - my $processfee_amount = 20; - my $replacement_amount = 99.00; - my $item_type = $builder->build_object( - { class => 'Koha::ItemTypes', - value => { - notforloan => undef, - rentalcharge => 0, - defaultreplacecost => undef, - processfee => $processfee_amount, - rentalcharge_daily => 0, - } - } - ); - my $library = $builder->build_object( { class => 'Koha::Libraries' } ); - - my $biblio = $builder->build_sample_biblio({ author => 'Hall, Daria' }); - - subtest 'Full write-off tests' => sub { - - plan tests => 12; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - my $manager = $builder->build_object({ class => "Koha::Patrons" }); - t::lib::Mocks::mock_userenv({ patron => $manager,branchcode => $manager->branchcode }); - - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library->branchcode, - replacementprice => $replacement_amount, - itype => $item_type->itemtype, - } - ); - - AddIssue( $patron->unblessed, $item->barcode ); - - # Simulate item marked as lost - $item->itemlost(3)->store; - LostItem( $item->itemnumber, 1 ); - - my $processing_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'PROCESSING' } ); - is( $processing_fee_lines->count, 1, 'Only one processing fee produced' ); - my $processing_fee_line = $processing_fee_lines->next; - is( $processing_fee_line->amount + 0, - $processfee_amount, 'The right PROCESSING amount is generated' ); - is( $processing_fee_line->amountoutstanding + 0, - $processfee_amount, 'The right PROCESSING amountoutstanding is generated' ); - - my $lost_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'LOST' } ); - is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); - my $lost_fee_line = $lost_fee_lines->next; - is( $lost_fee_line->amount + 0, $replacement_amount, 'The right LOST amount is generated' ); - is( $lost_fee_line->amountoutstanding + 0, - $replacement_amount, 'The right LOST amountoutstanding is generated' ); - is( $lost_fee_line->status, - undef, 'The LOST status was not set' ); - - my $account = $patron->account; - my $debts = $account->outstanding_debits; - - # Write off the debt - my $credit = $account->add_credit( - { amount => $account->balance, - type => 'WRITEOFF', - interface => 'test', - } - ); - $credit->apply( { debits => [ $debts->as_list ], offset_type => 'Writeoff' } ); - - my $credit_return_id = C4::Circulation::_FixAccountForLostAndFound( $item->itemnumber, $patron->id ); - is( $credit_return_id, undef, 'No LOST_FOUND account line added' ); - - $lost_fee_line->discard_changes; # reload from DB - is( $lost_fee_line->amountoutstanding + 0, 0, 'Lost fee has no outstanding amount' ); - is( $lost_fee_line->debit_type_code, - 'LOST', 'Lost fee now still has account type of LOST' ); - is( $lost_fee_line->status, 'FOUND', "Lost fee now has account status of FOUND"); - - is( $patron->account->balance, -0, 'The patron balance is 0, everything was written off' ); - }; - - subtest 'Full payment tests' => sub { - - plan tests => 13; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library->branchcode, - replacementprice => $replacement_amount, - itype => $item_type->itemtype - } - ); - - AddIssue( $patron->unblessed, $item->barcode ); - - # Simulate item marked as lost - $item->itemlost(1)->store; - LostItem( $item->itemnumber, 1 ); - - my $processing_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'PROCESSING' } ); - is( $processing_fee_lines->count, 1, 'Only one processing fee produced' ); - my $processing_fee_line = $processing_fee_lines->next; - is( $processing_fee_line->amount + 0, - $processfee_amount, 'The right PROCESSING amount is generated' ); - is( $processing_fee_line->amountoutstanding + 0, - $processfee_amount, 'The right PROCESSING amountoutstanding is generated' ); - - my $lost_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'LOST' } ); - is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); - my $lost_fee_line = $lost_fee_lines->next; - is( $lost_fee_line->amount + 0, $replacement_amount, 'The right LOST amount is generated' ); - is( $lost_fee_line->amountoutstanding + 0, - $replacement_amount, 'The right LOST amountountstanding is generated' ); - - my $account = $patron->account; - my $debts = $account->outstanding_debits; - - # Write off the debt - my $credit = $account->add_credit( - { amount => $account->balance, - type => 'PAYMENT', - interface => 'test', - } - ); - $credit->apply( { debits => [ $debts->as_list ], offset_type => 'Payment' } ); - - my $credit_return_id = C4::Circulation::_FixAccountForLostAndFound( $item->itemnumber, $patron->id ); - my $credit_return = Koha::Account::Lines->find($credit_return_id); - - is( $credit_return->credit_type_code, 'LOST_FOUND', 'An account line of type LOST_FOUND is added' ); - is( $credit_return->amount + 0, - -99.00, 'The account line of type LOST_FOUND has an amount of -99' ); - is( $credit_return->amountoutstanding + 0, - -99.00, 'The account line of type LOST_FOUND has an amountoutstanding of -99' ); - - $lost_fee_line->discard_changes; - is( $lost_fee_line->amountoutstanding + 0, 0, 'Lost fee has no outstanding amount' ); - is( $lost_fee_line->debit_type_code, - 'LOST', 'Lost fee now still has account type of LOST' ); - is( $lost_fee_line->status, 'FOUND', "Lost fee now has account status of FOUND"); - - is( $patron->account->balance, - -99, 'The patron balance is -99, a credit that equals the lost fee payment' ); - }; - - subtest 'Test without payment or write off' => sub { - - plan tests => 13; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library->branchcode, - replacementprice => 23.00, - replacementprice => $replacement_amount, - itype => $item_type->itemtype - } - ); - - AddIssue( $patron->unblessed, $item->barcode ); - - # Simulate item marked as lost - $item->itemlost(3)->store; - LostItem( $item->itemnumber, 1 ); - - my $processing_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'PROCESSING' } ); - is( $processing_fee_lines->count, 1, 'Only one processing fee produced' ); - my $processing_fee_line = $processing_fee_lines->next; - is( $processing_fee_line->amount + 0, - $processfee_amount, 'The right PROCESSING amount is generated' ); - is( $processing_fee_line->amountoutstanding + 0, - $processfee_amount, 'The right PROCESSING amountoutstanding is generated' ); - - my $lost_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'LOST' } ); - is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); - my $lost_fee_line = $lost_fee_lines->next; - is( $lost_fee_line->amount + 0, $replacement_amount, 'The right LOST amount is generated' ); - is( $lost_fee_line->amountoutstanding + 0, - $replacement_amount, 'The right LOST amountountstanding is generated' ); - - my $credit_return_id = C4::Circulation::_FixAccountForLostAndFound( $item->itemnumber, $patron->id ); - my $credit_return = Koha::Account::Lines->find($credit_return_id); - - is( $credit_return->credit_type_code, 'LOST_FOUND', 'An account line of type LOST_FOUND is added' ); - is( $credit_return->amount + 0, -99.00, 'The account line of type LOST_FOUND has an amount of -99' ); - is( $credit_return->amountoutstanding + 0, 0, 'The account line of type LOST_FOUND has an amountoutstanding of 0' ); - - $lost_fee_line->discard_changes; - is( $lost_fee_line->amountoutstanding + 0, 0, 'Lost fee has no outstanding amount' ); - is( $lost_fee_line->debit_type_code, - 'LOST', 'Lost fee now still has account type of LOST' ); - is( $lost_fee_line->status, 'FOUND', "Lost fee now has account status of FOUND"); - - is( $patron->account->balance, 20, 'The patron balance is 20, still owes the processing fee' ); - }; - - subtest 'Test with partial payement and write off, and remaining debt' => sub { - - plan tests => 16; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library->branchcode, - replacementprice => $replacement_amount, - itype => $item_type->itemtype - } - ); - - AddIssue( $patron->unblessed, $item->barcode ); - - # Simulate item marked as lost - $item->itemlost(1)->store; - LostItem( $item->itemnumber, 1 ); - - my $processing_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'PROCESSING' } ); - is( $processing_fee_lines->count, 1, 'Only one processing fee produced' ); - my $processing_fee_line = $processing_fee_lines->next; - is( $processing_fee_line->amount + 0, - $processfee_amount, 'The right PROCESSING amount is generated' ); - is( $processing_fee_line->amountoutstanding + 0, - $processfee_amount, 'The right PROCESSING amountoutstanding is generated' ); - - my $lost_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'LOST' } ); - is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); - my $lost_fee_line = $lost_fee_lines->next; - is( $lost_fee_line->amount + 0, $replacement_amount, 'The right LOST amount is generated' ); - is( $lost_fee_line->amountoutstanding + 0, - $replacement_amount, 'The right LOST amountountstanding is generated' ); - - my $account = $patron->account; - is( $account->balance, $processfee_amount + $replacement_amount, 'Balance is PROCESSING + L' ); - - # Partially pay fee - my $payment_amount = 27; - my $payment = $account->add_credit( - { amount => $payment_amount, - type => 'PAYMENT', - interface => 'test', - } - ); - - $payment->apply( { debits => [ $lost_fee_line ], offset_type => 'Payment' } ); - - # Partially write off fee - my $write_off_amount = 25; - my $write_off = $account->add_credit( - { amount => $write_off_amount, - type => 'WRITEOFF', - interface => 'test', - } - ); - $write_off->apply( { debits => [ $lost_fee_line ], offset_type => 'Writeoff' } ); - - is( $account->balance, - $processfee_amount + $replacement_amount - $payment_amount - $write_off_amount, - 'Payment and write off applied' - ); - - # Store the amountoutstanding value - $lost_fee_line->discard_changes; - my $outstanding = $lost_fee_line->amountoutstanding; - - my $credit_return_id = C4::Circulation::_FixAccountForLostAndFound( $item->itemnumber, $patron->id ); - my $credit_return = Koha::Account::Lines->find($credit_return_id); - - is( $account->balance, $processfee_amount - $payment_amount, 'Balance is PROCESSING - PAYMENT (LOST_FOUND)' ); - - $lost_fee_line->discard_changes; - is( $lost_fee_line->amountoutstanding + 0, 0, 'Lost fee has no outstanding amount' ); - is( $lost_fee_line->debit_type_code, - 'LOST', 'Lost fee now still has account type of LOST' ); - is( $lost_fee_line->status, 'FOUND', "Lost fee now has account status of FOUND"); - - is( $credit_return->credit_type_code, 'LOST_FOUND', 'An account line of type LOST_FOUND is added' ); - is( $credit_return->amount + 0, - ($payment_amount + $outstanding ) * -1, - 'The account line of type LOST_FOUND has an amount equal to the payment + outstanding' - ); - is( $credit_return->amountoutstanding + 0, - $payment_amount * -1, - 'The account line of type LOST_FOUND has an amountoutstanding equal to the payment' - ); - - is( $account->balance, - $processfee_amount - $payment_amount, - 'The patron balance is the difference between the PROCESSING and the credit' - ); - }; - - subtest 'Partial payement, existing debits and AccountAutoReconcile' => sub { - - plan tests => 8; - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - my $barcode = 'KD123456793'; - my $replacement_amount = 100; - my $processfee_amount = 20; - - my $item_type = $builder->build_object( - { class => 'Koha::ItemTypes', - value => { - notforloan => undef, - rentalcharge => 0, - defaultreplacecost => undef, - processfee => 0, - rentalcharge_daily => 0, - } - } - ); - my $item = Koha::Item->new( - { - biblionumber => $biblio->biblionumber, - homebranch => $library->branchcode, - holdingbranch => $library->branchcode, - barcode => $barcode, - replacementprice => $replacement_amount, - itype => $item_type->itemtype - }, - )->store; - - AddIssue( $patron->unblessed, $barcode ); - - # Simulate item marked as lost - $item->itemlost(1)->store; - LostItem( $item->itemnumber, 1 ); - - my $lost_fee_lines = Koha::Account::Lines->search( - { borrowernumber => $patron->id, itemnumber => $item->itemnumber, debit_type_code => 'LOST' } ); - is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); - my $lost_fee_line = $lost_fee_lines->next; - is( $lost_fee_line->amount + 0, $replacement_amount, 'The right LOST amount is generated' ); - is( $lost_fee_line->amountoutstanding + 0, - $replacement_amount, 'The right LOST amountountstanding is generated' ); - - my $account = $patron->account; - is( $account->balance, $replacement_amount, 'Balance is L' ); - - # Partially pay fee - my $payment_amount = 27; - my $payment = $account->add_credit( - { amount => $payment_amount, - type => 'PAYMENT', - interface => 'test', - } - ); - $payment->apply({ debits => [ $lost_fee_line ], offset_type => 'Payment' }); - - is( $account->balance, - $replacement_amount - $payment_amount, - 'Payment applied' - ); - - my $manual_debit_amount = 80; - $account->add_debit( { amount => $manual_debit_amount, type => 'OVERDUE', interface =>'test' } ); - - is( $account->balance, $manual_debit_amount + $replacement_amount - $payment_amount, 'Manual debit applied' ); - - t::lib::Mocks::mock_preference( 'AccountAutoReconcile', 1 ); - - my $credit_return_id = C4::Circulation::_FixAccountForLostAndFound( $item->itemnumber, $patron->id ); - my $credit_return = Koha::Account::Lines->find($credit_return_id); - - is( $account->balance, $manual_debit_amount - $payment_amount, 'Balance is PROCESSING - payment (LOST_FOUND)' ); - - my $manual_debit = Koha::Account::Lines->search({ borrowernumber => $patron->id, debit_type_code => 'OVERDUE', status => 'UNRETURNED' })->next; - is( $manual_debit->amountoutstanding + 0, $manual_debit_amount - $payment_amount, 'reconcile_balance was called' ); - }; -}; - subtest '_FixOverduesOnReturn' => sub { plan tests => 14; @@ -3144,48 +2753,6 @@ subtest '_FixOverduesOnReturn' => sub { is( $accountline->status, 'RETURNED', 'Passed status has been used to set as RETURNED )'); }; -subtest '_FixAccountForLostAndFound returns undef if patron is deleted' => sub { - plan tests => 1; - - my $manager = $builder->build_object({ class => "Koha::Patrons" }); - t::lib::Mocks::mock_userenv({ patron => $manager, branchcode => $manager->branchcode }); - - my $biblio = $builder->build_sample_biblio({ author => 'Hall, Kylie' }); - - my $branchcode = $library2->{branchcode}; - - my $item = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $branchcode, - replacementprice => 99.00, - itype => $itemtype, - } - ); - - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - - ## Start with basic call, should just close out the open fine - my $accountline = Koha::Account::Line->new( - { - borrowernumber => $patron->id, - debit_type_code => 'LOST', - status => undef, - itemnumber => $item->itemnumber, - amount => 99.00, - amountoutstanding => 99.00, - interface => 'test', - } - )->store(); - - $patron->delete(); - - my $return_value = C4::Circulation::_FixAccountForLostAndFound( $patron->id, $item->itemnumber ); - - is( $return_value, undef, "_FixAccountForLostAndFound returns undef if patron is deleted" ); - -}; - subtest 'Set waiting flag' => sub { plan tests => 11; diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index 010b41e97a..4e7fecb2e3 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -33,7 +33,7 @@ use Koha::AuthorisedValues; use t::lib::Mocks; use t::lib::TestBuilder; -use Test::More tests => 15; +use Test::More tests => 14; use Test::Warn; @@ -126,31 +126,6 @@ subtest 'General Add, Get and Del tests' => sub { $schema->storage->txn_rollback; }; -subtest 'ModItem tests' => sub { - plan tests => 6; - - $schema->storage->txn_begin; - - my $builder = t::lib::TestBuilder->new; - my $item = $builder->build_sample_item(); - - my @fields = qw( itemlost withdrawn damaged ); - for my $field (@fields) { - my $field_on = $field."_on"; - - $item->$field(1)->store; - $item->discard_changes; - is( output_pref({ str => $item->$field_on, dateonly => 1 }), output_pref({ dt => dt_from_string(), dateonly => 1 }), "When updating $field, $field_on is updated" ); - - $item->$field(0)->store; - $item->discard_changes; - is( $item->$field_on, undef, "When clearing $field, $field_on is cleared" ); - } - - $schema->storage->txn_rollback; - -}; - subtest 'ModItemTransfer tests' => sub { plan tests => 8; @@ -1009,7 +984,7 @@ subtest 'ModItemFromMarc' => sub { my $item = Koha::Items->find($itemnumber); is( $item->itemlost, 1, 'itemlost picked from the item marc'); - $item->paidfor("this is something")->store; + $item->new_status("this is something")->store; my $updated_item_record = new MARC::Record; $updated_item_record->append_fields( @@ -1021,8 +996,8 @@ subtest 'ModItemFromMarc' => sub { my $updated_item = ModItemFromMarc($updated_item_record, $biblio->biblionumber, $itemnumber); is( $updated_item->{itemlost}, 0, 'itemlost should have been reset to the default value in DB' ); - is( $updated_item->{paidfor}, "this is something", "Non mapped field has not been reset" ); - is( Koha::Items->find($itemnumber)->paidfor, "this is something" ); + is( $updated_item->{new_status}, "this is something", "Non mapped field has not been reset" ); + is( Koha::Items->find($itemnumber)->new_status, "this is something" ); $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/CirculationRules.t b/t/db_dependent/Koha/CirculationRules.t index 24fc39abf9..d96aed4de0 100644 --- a/t/db_dependent/Koha/CirculationRules.t +++ b/t/db_dependent/Koha/CirculationRules.t @@ -285,7 +285,7 @@ subtest 'get_onshelfholds_policy() tests' => sub { }; subtest 'get_lostreturn_policy() tests' => sub { - plan tests => 6; + plan tests => 7; $schema->storage->txn_begin; @@ -357,8 +357,8 @@ subtest 'get_lostreturn_policy() tests' => sub { my $item = $builder->build_sample_item( { - homebranch => $specific_rule_true->{branchcode}, - holdingbranch => $specific_rule_false->{branchcode} + homebranch => $specific_rule_false->{branchcode}, + holdingbranch => $specific_rule_true->{branchcode} } ); my $params = { @@ -373,11 +373,11 @@ subtest 'get_lostreturn_policy() tests' => sub { t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHomeBranch' ); is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 1,'Specific rule for home branch is applied (true)'); + 0,'Specific rule for home branch is applied (false)'); t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'ItemHoldingBranch' ); is( Koha::CirculationRules->get_lostreturn_policy( $params ), - 0,'Specific rule for holoding branch is applied (false)'); + 1,'Specific rule for holding branch is applied (true)'); # Default rule check t::lib::Mocks::mock_preference( 'RefundLostOnReturnControl', 'CheckinLibrary' ); @@ -405,5 +405,10 @@ subtest 'get_lostreturn_policy() tests' => sub { is( Koha::CirculationRules->get_lostreturn_policy( $params ), 1,'No rule for branch, no default rule, fallback default (true)'); + # Fallback to ItemHoldBranch if CheckinLibrary is undefined + $params->{return_branch} = undef; + is( Koha::CirculationRules->get_lostreturn_policy( $params ), + 0,'return_branch undefined, fallback to ItemHomeBranch rule (false)'); + $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t index d04559a16d..a7d736c500 100644 --- a/t/db_dependent/Koha/Items.t +++ b/t/db_dependent/Koha/Items.t @@ -21,6 +21,7 @@ use Modern::Perl; use Test::More tests => 11; use Test::Exception; +use Time::Fake; use C4::Circulation; use C4::Context; @@ -64,24 +65,749 @@ my $retrieved_item_1 = Koha::Items->find( $new_item_1->itemnumber ); is( $retrieved_item_1->barcode, $new_item_1->barcode, 'Find a item by id should return the correct item' ); subtest 'store' => sub { - plan tests => 4; + plan tests => 6; + my $biblio = $builder->build_sample_biblio; - my $today = dt_from_string->set( hour => 0, minute => 0, second => 0 ); - my $item = Koha::Item->new( + my $today = dt_from_string->set( hour => 0, minute => 0, second => 0 ); + my $item = Koha::Item->new( { homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, biblionumber => $biblio->biblionumber, location => 'my_loc', } - )->store - ->get_from_storage; - - is( t::lib::Dates::compare($item->replacementpricedate, $today), 0, 'replacementpricedate must have been set to today if not given'); - is( t::lib::Dates::compare($item->datelastseen, $today), 0, 'datelastseen must have been set to today if not given'); - is( $item->itype, $biblio->biblioitem->itemtype, 'items.itype must have been set to biblioitem.itemtype is not given'); - is( $item->permanent_location, $item->location, 'permanent_location must have been set to location if not given' ); + )->store->get_from_storage; + + is( t::lib::Dates::compare( $item->replacementpricedate, $today ), + 0, 'replacementpricedate must have been set to today if not given' ); + is( t::lib::Dates::compare( $item->datelastseen, $today ), + 0, 'datelastseen must have been set to today if not given' ); + is( + $item->itype, + $biblio->biblioitem->itemtype, + 'items.itype must have been set to biblioitem.itemtype is not given' + ); + is( $item->permanent_location, $item->location, + 'permanent_location must have been set to location if not given' ); $item->delete; + + subtest '*_on updates' => sub { + plan tests => 9; + + # Once the '_on' value is set (triggered by the related field turning from false to true) + # it should not be re-set for any changes outside of the related field being 'unset'. + + my @fields = qw( itemlost withdrawn damaged ); + my $today = dt_from_string(); + my $yesterday = $today->clone()->subtract( days => 1 ); + + for my $field ( @fields ) { + my $item = $builder->build_sample_item( + { + itemlost => 0, + itemlost_on => undef, + withdrawn => 0, + withdrawn_on => undef, + damaged => 0, + damaged_on => undef + } + ); + my $field_on = $field . '_on'; + + # Set field for the first time + Time::Fake->offset( $yesterday->epoch ); + $item->$field(1)->store; + $item->get_from_storage; + is( t::lib::Dates::compare( $item->$field_on, $yesterday ), + 0, $field_on . " was set upon first truthy setting" ); + + # Update the field to a new 'true' value + Time::Fake->offset( $today->epoch ); + $item->$field(2)->store; + $item->get_from_storage; + is( t::lib::Dates::compare( $item->$field_on, $yesterday ), + 0, $field_on . " was not updated upon second truthy setting" ); + + # Update the field to a new 'false' value + $item->$field(0)->store; + $item->get_from_storage; + is($item->$field_on, undef, $field_on . " was unset upon untruthy setting"); + + Time::Fake->reset; + } + }; + + subtest '_lost_found_trigger' => sub { + plan tests => 7; + + t::lib::Mocks::mock_preference( 'WhenLostChargeReplacementFee', 1 ); + t::lib::Mocks::mock_preference( 'WhenLostForgiveFine', 0 ); + + my $processfee_amount = 20; + my $replacement_amount = 99.00; + my $item_type = $builder->build_object( + { + class => 'Koha::ItemTypes', + value => { + notforloan => undef, + rentalcharge => 0, + defaultreplacecost => undef, + processfee => $processfee_amount, + rentalcharge_daily => 0, + } + } + ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + + $biblio = $builder->build_sample_biblio( { author => 'Hall, Daria' } ); + + subtest 'Full write-off tests' => sub { + + plan tests => 12; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $manager = + $builder->build_object( { class => "Koha::Patrons" } ); + t::lib::Mocks::mock_userenv( + { patron => $manager, branchcode => $manager->branchcode } ); + + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->branchcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype, + } + ); + + C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); + + # Simulate item marked as lost + $item->itemlost(3)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + my $processing_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'PROCESSING' + } + ); + is( $processing_fee_lines->count, + 1, 'Only one processing fee produced' ); + my $processing_fee_line = $processing_fee_lines->next; + is( $processing_fee_line->amount + 0, + $processfee_amount, + 'The right PROCESSING amount is generated' ); + is( $processing_fee_line->amountoutstanding + 0, + $processfee_amount, + 'The right PROCESSING amountoutstanding is generated' ); + + my $lost_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'LOST' + } + ); + is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); + my $lost_fee_line = $lost_fee_lines->next; + is( $lost_fee_line->amount + 0, + $replacement_amount, 'The right LOST amount is generated' ); + is( $lost_fee_line->amountoutstanding + 0, + $replacement_amount, + 'The right LOST amountoutstanding is generated' ); + is( $lost_fee_line->status, undef, 'The LOST status was not set' ); + + my $account = $patron->account; + my $debts = $account->outstanding_debits; + + # Write off the debt + my $credit = $account->add_credit( + { + amount => $account->balance, + type => 'WRITEOFF', + interface => 'test', + } + ); + $credit->apply( + { debits => [ $debts->as_list ], offset_type => 'Writeoff' } ); + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, undef, 'No LOST_FOUND account line added' ); + + $lost_fee_line->discard_changes; # reload from DB + is( $lost_fee_line->amountoutstanding + 0, + 0, 'Lost fee has no outstanding amount' ); + is( $lost_fee_line->debit_type_code, + 'LOST', 'Lost fee now still has account type of LOST' ); + is( $lost_fee_line->status, 'FOUND', + "Lost fee now has account status of FOUND - No Refund" ); + + is( $patron->account->balance, + -0, 'The patron balance is 0, everything was written off' ); + }; + + subtest 'Full payment tests' => sub { + + plan tests => 14; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->branchcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + } + ); + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); + + # Simulate item marked as lost + $item->itemlost(1)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + my $processing_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'PROCESSING' + } + ); + is( $processing_fee_lines->count, + 1, 'Only one processing fee produced' ); + my $processing_fee_line = $processing_fee_lines->next; + is( $processing_fee_line->amount + 0, + $processfee_amount, + 'The right PROCESSING amount is generated' ); + is( $processing_fee_line->amountoutstanding + 0, + $processfee_amount, + 'The right PROCESSING amountoutstanding is generated' ); + + my $lost_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'LOST' + } + ); + is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); + my $lost_fee_line = $lost_fee_lines->next; + is( $lost_fee_line->amount + 0, + $replacement_amount, 'The right LOST amount is generated' ); + is( $lost_fee_line->amountoutstanding + 0, + $replacement_amount, + 'The right LOST amountountstanding is generated' ); + + my $account = $patron->account; + my $debts = $account->outstanding_debits; + + # Pay off the debt + my $credit = $account->add_credit( + { + amount => $account->balance, + type => 'PAYMENT', + interface => 'test', + } + ); + $credit->apply( + { debits => [ $debts->as_list ], offset_type => 'Payment' } ); + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, 1, 'Refund triggered' ); + + my $credit_return = Koha::Account::Lines->search( + { + itemnumber => $item->itemnumber, + credit_type_code => 'LOST_FOUND' + }, + { rows => 1 } + )->single; + + ok( $credit_return, 'An account line of type LOST_FOUND is added' ); + is( $credit_return->amount + 0, + -99.00, + 'The account line of type LOST_FOUND has an amount of -99' ); + is( + $credit_return->amountoutstanding + 0, + -99.00, +'The account line of type LOST_FOUND has an amountoutstanding of -99' + ); + + $lost_fee_line->discard_changes; + is( $lost_fee_line->amountoutstanding + 0, + 0, 'Lost fee has no outstanding amount' ); + is( $lost_fee_line->debit_type_code, + 'LOST', 'Lost fee now still has account type of LOST' ); + is( $lost_fee_line->status, 'FOUND', + "Lost fee now has account status of FOUND" ); + + is( $patron->account->balance, -99, +'The patron balance is -99, a credit that equals the lost fee payment' + ); + }; + + subtest 'Test without payment or write off' => sub { + + plan tests => 14; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->branchcode, + replacementprice => 23.00, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + } + ); + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); + + # Simulate item marked as lost + $item->itemlost(3)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + my $processing_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'PROCESSING' + } + ); + is( $processing_fee_lines->count, + 1, 'Only one processing fee produced' ); + my $processing_fee_line = $processing_fee_lines->next; + is( $processing_fee_line->amount + 0, + $processfee_amount, + 'The right PROCESSING amount is generated' ); + is( $processing_fee_line->amountoutstanding + 0, + $processfee_amount, + 'The right PROCESSING amountoutstanding is generated' ); + + my $lost_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'LOST' + } + ); + is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); + my $lost_fee_line = $lost_fee_lines->next; + is( $lost_fee_line->amount + 0, + $replacement_amount, 'The right LOST amount is generated' ); + is( $lost_fee_line->amountoutstanding + 0, + $replacement_amount, + 'The right LOST amountountstanding is generated' ); + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, 1, 'Refund triggered' ); + + my $credit_return = Koha::Account::Lines->search( + { + itemnumber => $item->itemnumber, + credit_type_code => 'LOST_FOUND' + }, + { rows => 1 } + )->single; + + ok( $credit_return, 'An account line of type LOST_FOUND is added' ); + is( $credit_return->amount + 0, + -99.00, + 'The account line of type LOST_FOUND has an amount of -99' ); + is( + $credit_return->amountoutstanding + 0, + 0, +'The account line of type LOST_FOUND has an amountoutstanding of 0' + ); + + $lost_fee_line->discard_changes; + is( $lost_fee_line->amountoutstanding + 0, + 0, 'Lost fee has no outstanding amount' ); + is( $lost_fee_line->debit_type_code, + 'LOST', 'Lost fee now still has account type of LOST' ); + is( $lost_fee_line->status, 'FOUND', + "Lost fee now has account status of FOUND" ); + + is( $patron->account->balance, + 20, 'The patron balance is 20, still owes the processing fee' ); + }; + + subtest + 'Test with partial payement and write off, and remaining debt' => + sub { + + plan tests => 17; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library->branchcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + } + ); + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); + + # Simulate item marked as lost + $item->itemlost(1)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + my $processing_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'PROCESSING' + } + ); + is( $processing_fee_lines->count, + 1, 'Only one processing fee produced' ); + my $processing_fee_line = $processing_fee_lines->next; + is( $processing_fee_line->amount + 0, + $processfee_amount, + 'The right PROCESSING amount is generated' ); + is( $processing_fee_line->amountoutstanding + 0, + $processfee_amount, + 'The right PROCESSING amountoutstanding is generated' ); + + my $lost_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'LOST' + } + ); + is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); + my $lost_fee_line = $lost_fee_lines->next; + is( $lost_fee_line->amount + 0, + $replacement_amount, 'The right LOST amount is generated' ); + is( $lost_fee_line->amountoutstanding + 0, + $replacement_amount, + 'The right LOST amountountstanding is generated' ); + + my $account = $patron->account; + is( + $account->balance, + $processfee_amount + $replacement_amount, + 'Balance is PROCESSING + L' + ); + + # Partially pay fee + my $payment_amount = 27; + my $payment = $account->add_credit( + { + amount => $payment_amount, + type => 'PAYMENT', + interface => 'test', + } + ); + + $payment->apply( + { debits => [$lost_fee_line], offset_type => 'Payment' } ); + + # Partially write off fee + my $write_off_amount = 25; + my $write_off = $account->add_credit( + { + amount => $write_off_amount, + type => 'WRITEOFF', + interface => 'test', + } + ); + $write_off->apply( + { debits => [$lost_fee_line], offset_type => 'Writeoff' } ); + + is( + $account->balance, + $processfee_amount + + $replacement_amount - + $payment_amount - + $write_off_amount, + 'Payment and write off applied' + ); + + # Store the amountoutstanding value + $lost_fee_line->discard_changes; + my $outstanding = $lost_fee_line->amountoutstanding; + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, 1, 'Refund triggered' ); + + my $credit_return = Koha::Account::Lines->search( + { + itemnumber => $item->itemnumber, + credit_type_code => 'LOST_FOUND' + }, + { rows => 1 } + )->single; + + ok( $credit_return, 'An account line of type LOST_FOUND is added' ); + + is( + $account->balance, + $processfee_amount - $payment_amount, + 'Balance is PROCESSING - PAYMENT (LOST_FOUND)' + ); + + $lost_fee_line->discard_changes; + is( $lost_fee_line->amountoutstanding + 0, + 0, 'Lost fee has no outstanding amount' ); + is( $lost_fee_line->debit_type_code, + 'LOST', 'Lost fee now still has account type of LOST' ); + is( $lost_fee_line->status, 'FOUND', + "Lost fee now has account status of FOUND" ); + + is( + $credit_return->amount + 0, + ( $payment_amount + $outstanding ) * -1, +'The account line of type LOST_FOUND has an amount equal to the payment + outstanding' + ); + is( + $credit_return->amountoutstanding + 0, + $payment_amount * -1, +'The account line of type LOST_FOUND has an amountoutstanding equal to the payment' + ); + + is( + $account->balance, + $processfee_amount - $payment_amount, +'The patron balance is the difference between the PROCESSING and the credit' + ); + }; + + subtest 'Partial payment, existing debits and AccountAutoReconcile' => + sub { + + plan tests => 10; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $barcode = 'KD123456793'; + my $replacement_amount = 100; + my $processfee_amount = 20; + + my $item_type = $builder->build_object( + { + class => 'Koha::ItemTypes', + value => { + notforloan => undef, + rentalcharge => 0, + defaultreplacecost => undef, + processfee => 0, + rentalcharge_daily => 0, + } + } + ); + my $item = Koha::Item->new( + { + biblionumber => $biblio->biblionumber, + homebranch => $library->branchcode, + holdingbranch => $library->branchcode, + barcode => $barcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + }, + )->store; + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $barcode ); + + # Simulate item marked as lost + $item->itemlost(1)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + my $lost_fee_lines = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + itemnumber => $item->itemnumber, + debit_type_code => 'LOST' + } + ); + is( $lost_fee_lines->count, 1, 'Only one lost item fee produced' ); + my $lost_fee_line = $lost_fee_lines->next; + is( $lost_fee_line->amount + 0, + $replacement_amount, 'The right LOST amount is generated' ); + is( $lost_fee_line->amountoutstanding + 0, + $replacement_amount, + 'The right LOST amountountstanding is generated' ); + + my $account = $patron->account; + is( $account->balance, $replacement_amount, 'Balance is L' ); + + # Partially pay fee + my $payment_amount = 27; + my $payment = $account->add_credit( + { + amount => $payment_amount, + type => 'PAYMENT', + interface => 'test', + } + ); + $payment->apply( + { debits => [$lost_fee_line], offset_type => 'Payment' } ); + + is( + $account->balance, + $replacement_amount - $payment_amount, + 'Payment applied' + ); + + my $manual_debit_amount = 80; + $account->add_debit( + { + amount => $manual_debit_amount, + type => 'OVERDUE', + interface => 'test' + } + ); + + is( + $account->balance, + $manual_debit_amount + $replacement_amount - $payment_amount, + 'Manual debit applied' + ); + + t::lib::Mocks::mock_preference( 'AccountAutoReconcile', 1 ); + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, 1, 'Refund triggered' ); + + my $credit_return = Koha::Account::Lines->search( + { + itemnumber => $item->itemnumber, + credit_type_code => 'LOST_FOUND' + }, + { rows => 1 } + )->single; + + ok( $credit_return, 'An account line of type LOST_FOUND is added' ); + + is( + $account->balance, + $manual_debit_amount - $payment_amount, + 'Balance is PROCESSING - payment (LOST_FOUND)' + ); + + my $manual_debit = Koha::Account::Lines->search( + { + borrowernumber => $patron->id, + debit_type_code => 'OVERDUE', + status => 'UNRETURNED' + } + )->next; + is( + $manual_debit->amountoutstanding + 0, + $manual_debit_amount - $payment_amount, + 'reconcile_balance was called' + ); + }; + + subtest 'Patron deleted' => sub { + plan tests => 1; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $barcode = 'KD123456794'; + my $replacement_amount = 100; + my $processfee_amount = 20; + + my $item_type = $builder->build_object( + { + class => 'Koha::ItemTypes', + value => { + notforloan => undef, + rentalcharge => 0, + defaultreplacecost => undef, + processfee => 0, + rentalcharge_daily => 0, + } + } + ); + my $item = Koha::Item->new( + { + biblionumber => $biblio->biblionumber, + homebranch => $library->branchcode, + holdingbranch => $library->branchcode, + barcode => $barcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + }, + )->store; + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $barcode ); + + # Simulate item marked as lost + $item->itemlost(1)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + $issue->delete(); + $patron->delete(); + + # Simulate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, undef, 'No refund triggered' ); + + }; + + subtest 'Continue when userenv is not set' => sub { + plan tests => 1; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $barcode = 'KD123456795'; + my $replacement_amount = 100; + my $processfee_amount = 20; + + my $item_type = $builder->build_object( + { + class => 'Koha::ItemTypes', + value => { + notforloan => undef, + rentalcharge => 0, + defaultreplacecost => undef, + processfee => 0, + rentalcharge_daily => 0, + } + } + ); + my $item = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + homebranch => $library->branchcode, + holdingbranch => $library->branchcode, + barcode => $barcode, + replacementprice => $replacement_amount, + itype => $item_type->itemtype + } + ); + + my $issue = + C4::Circulation::AddIssue( $patron->unblessed, $barcode ); + + # Simulate item marked as lost + $item->itemlost(1)->store; + C4::Circulation::LostItem( $item->itemnumber, 1 ); + + # Unset the userenv + C4::Context->_new_userenv(undef); + + # Simluate item marked as found + $item->itemlost(0)->store; + is( $item->{_refunded}, 1, 'No refund triggered' ); + + }; + }; }; subtest 'get_transfer' => sub { @@ -107,7 +833,6 @@ subtest 'holds' => sub { my $item = $builder->build_sample_item({ biblionumber => $biblio->biblionumber, }); - $nb_of_items++; is($item->holds->count, 0, "Nothing returned if no holds"); my $hold1 = $builder->build({ source => 'Reserve', value => { itemnumber=>$item->itemnumber, found => 'T' }}); my $hold2 = $builder->build({ source => 'Reserve', value => { itemnumber=>$item->itemnumber, found => 'W' }}); @@ -135,6 +860,8 @@ subtest 'biblioitem' => sub { is( $biblioitem->biblionumber, $retrieved_item_1->biblionumber, 'Koha::Item->biblioitem should return the correct biblioitem' ); }; +# Restore userenv +t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} }); subtest 'checkout' => sub { plan tests => 5; my $item = Koha::Items->find( $new_item_1->itemnumber ); @@ -172,7 +899,6 @@ subtest 'can_be_transferred' => sub { homebranch => $library1->branchcode, holdingbranch => $library1->branchcode, }); - $nb_of_items++; is(Koha::Item::Transfer::Limits->search({ fromBranch => $library1->branchcode, @@ -196,7 +922,11 @@ subtest 'can_be_transferred' => sub { 'We get the same result also if we pass the from-library parameter.'); }; +# Reset nb_of_items prior to testing delete +$nb_of_items = Koha::Items->search->count; + +# Test delete $retrieved_item_1->delete; -is( Koha::Items->search->count, $nb_of_items + 1, 'Delete should have deleted the item' ); +is( Koha::Items->search->count, $nb_of_items - 1, 'Delete should have deleted the item' ); $schema->storage->txn_rollback; -- 2.11.0