Bug 22563: (QA follow-up) Use issue_id in chargelostitem

C4::Accounts::chargelostitem contained a FIXME which asked if an item
should be charged for it lost, returned and then lost again. We add
handling for that case here.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

diff --git a/C4/Accounts.pm b/C4/Accounts.pm
index 4c01194..5601291 100644 (file)
--- a/C4/Accounts.pm
+++ b/C4/Accounts.pm
@@ -71,7 +71,7 @@ FIXME : if no replacement price, borrower just doesn't get charged?
 
 =cut
 
-sub chargelostitem{
+sub chargelostitem {
     my $dbh = C4::Context->dbh();
     my ($borrowernumber, $itemnumber, $amount, $description) = @_;
     my $itype = Koha::ItemTypes->find({ itemtype => Koha::Items->find($itemnumber)->effective_itemtype() });
@@ -83,6 +83,8 @@ sub chargelostitem{
     if ($usedefaultreplacementcost && $amount == 0 && $defaultreplacecost){
         $replacementprice = $defaultreplacecost;
     }
+    my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber });
+    my $issue_id = $checkout ? $checkout->issue_id : undef;
 
     my $account = Koha::Account->new({ patron_id => $borrowernumber });
     # first make sure the borrower hasn't already been charged for this item
@@ -92,13 +94,12 @@ sub chargelostitem{
         {
             itemnumber     => $itemnumber,
             accounttype    => 'LOST',
+            issue_id       => $issue_id
         }
     )->count();
 
     # OK, they haven't
     unless ($existing_charges) {
-        my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber });
-        my $issue_id = $checkout ? $checkout->issue_id : undef;
         #add processing fee
         if ($processfee && $processfee > 0){
             my $accountline = $account->add_debit(

diff --git a/t/db_dependent/Accounts.t b/t/db_dependent/Accounts.t
index 1e214fa..4a45063 100644 (file)
--- a/t/db_dependent/Accounts.t
+++ b/t/db_dependent/Accounts.t
@@ -32,6 +32,8 @@ use Koha::Notice::Messages;
 use Koha::Notice::Templates;
 use Koha::DateUtils qw( dt_from_string );
 
+use C4::Circulation qw( MarkIssueReturned );
+
 BEGIN {
     use_ok('C4::Accounts');
     use_ok('Koha::Object');
@@ -569,12 +571,13 @@ subtest "C4::Accounts::chargelostitem tests" => sub {
     my $cli_issue_id_2 = $builder->build({ source => 'Issue', value => { borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber2 } })->{issue_id};
     my $cli_issue_id_3 = $builder->build({ source => 'Issue', value => { borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber3 } })->{issue_id};
     my $cli_issue_id_4 = $builder->build({ source => 'Issue', value => { borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4 } })->{issue_id};
+    my $cli_issue_id_4X = undef;
 
     my $lostfine;
     my $procfee;
 
     subtest "fee application tests" => sub {
-        plan tests => 40;
+        plan tests => 44;
 
         t::lib::Mocks::mock_preference('item-level_itypes', '1');
         t::lib::Mocks::mock_preference('useDefaultReplacementCost', '0');
@@ -688,8 +691,20 @@ subtest "C4::Accounts::chargelostitem tests" => sub {
         is( $lostfine->amount, "6.120000", "Lost fine equals replacementcost when pref on and default set");
         is( $procfee->amount, "2.040000",  "Processing fee if processing fee");
         is( $procfee->issue_id, $cli_issue_id_4, "Processing fee issue id is correct" );
-        $lostfine->delete();
-        $procfee->delete();
+        C4::Accounts::chargelostitem( $cli_borrowernumber, $cli_itemnumber4, 6.12, "Perdedor");
+        my $lostfines = Koha::Account::Lines->search({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'LOST' });
+        my $procfees  = Koha::Account::Lines->search({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'PF' });
+        ok( $lostfines->count == 1 , "Lost fine cannot be double charged for the same issue_id");
+        ok( $procfees->count == 1,  "Processing fee cannot be double charged for the same issue_id");
+        MarkIssueReturned($cli_borrowernumber, $cli_itemnumber4);
+        $cli_issue_id_4X = $builder->build({ source => 'Issue', value => { borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4 } })->{issue_id};
+        C4::Accounts::chargelostitem( $cli_borrowernumber, $cli_itemnumber4, 6.12, "Perdedor");
+        $lostfines = Koha::Account::Lines->search({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'LOST' });
+        $procfees  = Koha::Account::Lines->search({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'PF' });
+        ok( $lostfines->count == 2 , "Lost fine can be charged twice for the same item if they are distinct issue_id's");
+        ok( $procfees->count == 2,  "Processing fee can be charged twice for the same item if they are distinct issue_id's");
+        $lostfines->delete();
+        $procfees->delete();
     };
 
     subtest "basic fields tests" => sub {
@@ -702,7 +717,7 @@ subtest "C4::Accounts::chargelostitem tests" => sub {
         $lostfine = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'LOST' });
         ok($lostfine, "Lost fine created");
         is($lostfine->manager_id, $staff_id, "Lost fine manager_id set correctly");
-        is($lostfine->issue_id, $cli_issue_id_4, "Lost fine issue_id set correctly");
+        is($lostfine->issue_id, $cli_issue_id_4X, "Lost fine issue_id set correctly");
         is($lostfine->description, "Perdedor", "Lost fine issue_id set correctly");
         is($lostfine->note, '', "Lost fine does not contain a note");
         is($lostfine->branchcode, $branchcode, "Lost fine branchcode set correctly");
@@ -711,7 +726,7 @@ subtest "C4::Accounts::chargelostitem tests" => sub {
         $procfee  = Koha::Account::Lines->find({ borrowernumber => $cli_borrowernumber, itemnumber => $cli_itemnumber4, accounttype => 'PF' });
         ok($procfee, "Processing fee created");
         is($procfee->manager_id, $staff_id, "Processing fee manager_id set correctly");
-        is($procfee->issue_id, $cli_issue_id_4, "Processing fee issue_id set correctly");
+        is($procfee->issue_id, $cli_issue_id_4X, "Processing fee issue_id set correctly");
         is($procfee->description, "Perdedor", "Processing fee issue_id set correctly");
         is($procfee->note, C4::Context->preference("ProcessingFeeNote"), "Processing fee contains note matching ProcessingFeeNote");
         is($procfee->branchcode, $branchcode, "Processing fee branchcode set correctly");