From 170434dac7750410e20f0b407a19527782958d56 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 10 Jul 2020 14:49:35 +0100 Subject: [PATCH] Bug 24603: (follow-up) Update to double entry accounting This patch updates the logic to create a cancellation accountline and apply it to the charge line so we correction record the transaction in terms of double entry accounting standards. Test plan: 1. Go to a patron's accounting section 2. Create a manual invoice 3. In Transactions tab, you should see a 'Cancel charge' button. Click on it. It should now be marked as cancelled 4. A cancellation line should be associated with the original charge. 5. Create another manual invoice 6. Pay it (partially or fully) 7. Notice that the 'Cancel charge' button is not available 8. Void the payment 9. 'Cancel charge' button is available again. Click on it and verify that it still works 10. prove t/db_dependent/Koha/Account/Lines.t --- Koha/Account/Line.pm | 99 +++++++++---- installer/data/mysql/account_credit_types.sql | 3 +- installer/data/mysql/account_offset_types.sql | 3 +- .../data/mysql/atomicupdate/bug_24603.perl | 20 +++ .../prog/en/modules/members/boraccount.tt | 4 +- members/cancel-charge.pl | 9 +- t/db_dependent/Koha/Account/Line.t | 131 +++++++++++++----- 7 files changed, 202 insertions(+), 67 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_24603.perl diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index 475ec1a7b6..2f87c081f5 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -291,46 +291,90 @@ sub void { Cancel a charge. It will mark the debit as 'cancelled' by updating its status to 'CANCELLED'. + Charges that have been fully or partially paid cannot be cancelled. -Return self in case of success, undef otherwise +Returns the cancellation accountline. =cut sub cancel { - my ($self) = @_; + my ( $self, $params ) = @_; - # Make sure it is a charge we are cancelling - return unless $self->is_debit; + # Make sure it is a charge we are reducing + unless ( $self->is_debit ) { + Koha::Exceptions::Account::IsNotDebit->throw( + error => 'Account line ' . $self->id . 'is not a debit' ); + } + if ( $self->debit_type_code eq 'PAYOUT' ) { + Koha::Exceptions::Account::IsNotDebit->throw( + error => 'Account line ' . $self->id . 'is a payout' ); + } # Make sure it is not already cancelled - return if $self->status && $self->status eq 'CANCELLED'; + if ( $self->status && $self->status eq 'CANCELLED' ) { + Koha::Exceptions::Account->throw( + error => 'Account line ' . $self->id . 'is already cancelled' ); + } # Make sure it has not be paid yet - return if $self->amount != $self->amountoutstanding; - - if ( C4::Context->preference("FinesLog") ) { - logaction('FINES', 'CANCEL', $self->borrowernumber, Dumper({ - action => 'cancel_charge', - borrowernumber => $self->borrowernumber, - amount => $self->amount, - amountoutstanding => $self->amountoutstanding, - description => $self->description, - debit_type_code => $self->debit_type_code, - note => $self->note, - itemnumber => $self->itemnumber, - manager_id => $self->manager_id, - })); + if ( $self->amount != $self->amountoutstanding ) { + Koha::Exceptions::Account->throw( + error => 'Account line ' . $self->id . 'is already offset' ); } - $self->set({ - status => 'CANCELLED', - amountoutstanding => 0, - amount => 0, - }); - $self->store(); + # Check for mandatory parameters + my @mandatory = ( 'staff_id', 'branch' ); + for my $param (@mandatory) { + unless ( defined( $params->{$param} ) ) { + Koha::Exceptions::MissingParameter->throw( + error => "The $param parameter is mandatory" ); + } + } - return $self; + my $cancellation; + $self->_result->result_source->schema->txn_do( + sub { + + # A 'cancellation' is a 'credit' + $cancellation = Koha::Account::Line->new( + { + date => \'NOW()', + amount => 0 - $self->amount, + credit_type_code => 'CANCELLATION', + status => 'ADDED', + amountoutstanding => 0 - $self->amount, + manager_id => $params->{staff_id}, + borrowernumber => $self->borrowernumber, + interface => 'intranet', + branchcode => $params->{branch}, + } + )->store(); + + my $cancellation_offset = Koha::Account::Offset->new( + { + credit_id => $cancellation->accountlines_id, + type => 'CANCELLATION', + amount => $self->amount + } + )->store(); + + # Link cancellation to charge + $cancellation->apply( + { + debits => [$self], + offset_type => 'CANCELLATION' + } + ); + $cancellation->status('APPLIED')->store(); + + # Update status of original debit + $self->status('CANCELLED')->store; + } + ); + + $cancellation->discard_changes; + return $cancellation; } =head3 reduce @@ -453,7 +497,8 @@ sub reduce { } else { - # Zero amount offset used to link original 'debit' to reduction 'credit' + # Zero amount offset used to link original 'debit' to + # reduction 'credit' my $link_reduction_offset = Koha::Account::Offset->new( { credit_id => $reduction->accountlines_id, diff --git a/installer/data/mysql/account_credit_types.sql b/installer/data/mysql/account_credit_types.sql index 178b98b1c6..2c34471e90 100644 --- a/installer/data/mysql/account_credit_types.sql +++ b/installer/data/mysql/account_credit_types.sql @@ -6,6 +6,7 @@ INSERT INTO account_credit_types ( code, description, can_be_added_manually, is_ ('DISCOUNT', 'A discount applied to a patrons fine', 0, 1), ('REFUND', 'A refund applied to a patrons fine', 0, 1), ('LOST_FOUND', 'Lost item fee refund', 0, 1), -('PURCHASE', 'Purchase', 0, 1); +('PURCHASE', 'Purchase', 0, 1), +('CANCELLATION', 'Cancellation', 0, 1); INSERT INTO authorised_values (category,authorised_value,lib) VALUES ('PAYMENT_TYPE','CASH','Cash'); diff --git a/installer/data/mysql/account_offset_types.sql b/installer/data/mysql/account_offset_types.sql index c27a68996f..86ebdafb88 100644 --- a/installer/data/mysql/account_offset_types.sql +++ b/installer/data/mysql/account_offset_types.sql @@ -21,4 +21,5 @@ INSERT INTO account_offset_types ( type ) VALUES ('Credit Applied'), ('PAYOUT'), ('DISCOUNT'), -('REFUND'); +('REFUND'), +('CANCELLATION'); diff --git a/installer/data/mysql/atomicupdate/bug_24603.perl b/installer/data/mysql/atomicupdate/bug_24603.perl new file mode 100644 index 0000000000..a9c6bfe3b8 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_24603.perl @@ -0,0 +1,20 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if ( CheckVersion($DBversion) ) { + + $dbh->do( + qq{ + INSERT IGNORE INTO account_credit_types (code, description, can_be_added_manually, is_system) + VALUES + ('CANCELLATION', 'A cancellation applied to a patron charge', 0, 1) + } + ); + + $dbh->do( + qq{ + INSERT IGNORE INTO account_offset_types ( type ) VALUES ('CANCELLATION'); + } + ); + + # Always end with this (adjust the bug info) + NewVersion( $DBversion, 24603, "Add CANCELLATION credit_type_code" ); +} diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt index cc6d0f0cbd..0fb385447e 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ -87,12 +87,12 @@ [% END %] Details [% IF account.is_debit && account.amountoutstanding > 0 %] - Pay + Pay [% END %] [% IF account.is_credit && account.status != 'VOID' %] Void payment [% END %] - [% IF account.is_debit && account.amount == account.amountoutstanding && account.status != 'CANCELLED' %] + [% IF account.is_debit && account.amount == account.amountoutstanding && account.status != 'CANCELLED' && !(account.debit_type_code == 'PAYOUT') %]
diff --git a/members/cancel-charge.pl b/members/cancel-charge.pl index 4aa0b00517..91184036e9 100755 --- a/members/cancel-charge.pl +++ b/members/cancel-charge.pl @@ -42,7 +42,12 @@ unless ($csrf_token_is_valid) { my $borrowernumber = $cgi->param('borrowernumber'); my $accountlines_id = $cgi->param('accountlines_id'); -my $line = Koha::Account::Lines->find($accountlines_id); -$line->cancel(); +my $charge = Koha::Account::Lines->find($accountlines_id); +$charge->cancel( + { + branch => C4::Context->userenv->{'branch'}, + staff_id => $user + } +); print $cgi->redirect('/cgi-bin/koha/members/boraccount.pl?borrowernumber=' . $borrowernumber); diff --git a/t/db_dependent/Koha/Account/Line.t b/t/db_dependent/Koha/Account/Line.t index f757ae0f92..8d23c531d8 100755 --- a/t/db_dependent/Koha/Account/Line.t +++ b/t/db_dependent/Koha/Account/Line.t @@ -901,7 +901,7 @@ subtest "payout() tests" => sub { subtest "reduce() tests" => sub { - plan tests => 27; + plan tests => 29; $schema->storage->txn_begin; @@ -961,7 +961,7 @@ subtest "reduce() tests" => sub { my $reduce_params = { interface => 'commandline', - reduction_type => 'REFUND', + reduction_type => 'DISCOUNT', amount => 5, staff_id => $staff->borrowernumber, branch => $branchcode @@ -1009,7 +1009,7 @@ subtest "reduce() tests" => sub { '->reduce() cannot reduce more than original amount'; # Partial Reduction - # (Refund 5 on debt of 20) + # (Discount 5 on debt of 20) my $reduction = $debit1->reduce($reduce_params); is( ref($reduction), 'Koha::Account::Line', @@ -1019,6 +1019,7 @@ subtest "reduce() tests" => sub { 0, "Reduce amountoutstanding is 0" ); is( $debit1->amountoutstanding() * 1, 15, "Debit amountoutstanding reduced by 5 to 15" ); + is( $debit1->status(), 'DISCOUNTED', "Debit status updated to DISCOUNTED"); is( $account->balance() * 1, -5, "Account balance is -5" ); is( $reduction->status(), 'APPLIED', "Reduction status is 'APPLIED'" ); @@ -1028,17 +1029,19 @@ subtest "reduce() tests" => sub { my $THE_offset = $offsets->next; is( $THE_offset->amount * 1, -5, 'Correct amount was applied against debit' ); - is( $THE_offset->type, 'REFUND', "Offset type set to 'REFUND'" ); + is( $THE_offset->type, 'DISCOUNT', "Offset type set to 'DISCOUNT'" ); # Zero offset created when zero outstanding # (Refund another 5 on paid debt of 20) $credit1->apply( { debits => [$debit1] } ); is( $debit1->amountoutstanding + 0, 0, 'Debit1 amountoutstanding reduced to 0' ); + $reduce_params->{reduction_type} = 'REFUND'; $reduction = $debit1->reduce($reduce_params); is( $reduction->amount() * 1, -5, "Reduce amount is -5" ); is( $reduction->amountoutstanding() * 1, -5, "Reduce amountoutstanding is -5" ); + is( $debit1->status(), 'REFUNDED', "Debit status updated to REFUNDED"); $offsets = Koha::Account::Offsets->search( { credit_id => $reduction->id, debit_id => $debit1->id } ); @@ -1077,28 +1080,47 @@ subtest "reduce() tests" => sub { }; subtest "cancel() tests" => sub { - plan tests => 9; + plan tests => 16; $schema->storage->txn_begin; # Create a borrower - my $categorycode = $builder->build({ source => 'Category' })->{ categorycode }; - my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; + my $categorycode = + $builder->build( { source => 'Category' } )->{categorycode}; + my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $borrower = Koha::Patron->new( { - cardnumber => 'dariahall', - surname => 'Hall', - firstname => 'Daria', - } ); - $borrower->categorycode( $categorycode ); - $borrower->branchcode( $branchcode ); + my $borrower = Koha::Patron->new( + { + cardnumber => 'dariahall', + surname => 'Hall', + firstname => 'Daria', + } + ); + $borrower->categorycode($categorycode); + $borrower->branchcode($branchcode); $borrower->store; - t::lib::Mocks::mock_userenv({ branchcode => $branchcode, borrowernumber => $borrower->borrowernumber }); + my $staff = Koha::Patron->new( + { + cardnumber => 'bobby', + surname => 'Bloggs', + firstname => 'Bobby', + } + ); + $staff->categorycode($categorycode); + $staff->branchcode($branchcode); + $staff->store; + + t::lib::Mocks::mock_userenv( + { + branchcode => $branchcode, + borrowernumber => $borrower->borrowernumber + } + ); - my $account = Koha::Account->new({ patron_id => $borrower->id }); + my $account = Koha::Account->new( { patron_id => $borrower->id } ); - my $line1 = Koha::Account::Line->new( + my $debit1 = Koha::Account::Line->new( { borrowernumber => $borrower->borrowernumber, amount => 10, @@ -1107,7 +1129,7 @@ subtest "cancel() tests" => sub { debit_type_code => 'OVERDUE', } )->store(); - my $line2 = Koha::Account::Line->new( + my $debit2 = Koha::Account::Line->new( { borrowernumber => $borrower->borrowernumber, amount => 20, @@ -1117,27 +1139,68 @@ subtest "cancel() tests" => sub { } )->store(); - my $id = $account->pay({ - lines => [$line2], - amount => 5, - }); + my $ret = $account->pay( + { + lines => [$debit2], + amount => 5, + } + ); + my $credit = Koha::Account::Lines->find({ accountlines_id => $ret->{payment_id} }); is( $account->balance(), 25, "Account balance is 25" ); - is( $line1->amountoutstanding+0, 10, 'First fee has amount outstanding of 10' ); - is( $line2->amountoutstanding+0, 15, 'Second fee has amount outstanding of 15' ); + is( $debit1->amountoutstanding + 0, + 10, 'First fee has amount outstanding of 10' ); + is( $debit2->amountoutstanding + 0, + 15, 'Second fee has amount outstanding of 15' ); + throws_ok { + $credit->cancel( + { staff_id => $staff->borrowernumber, branch => $branchcode } ); + } + 'Koha::Exceptions::Account::IsNotDebit', + '->cancel() can only be used with debits'; - my $ret = $line1->cancel(); - is( ref($ret), 'Koha::Account::Line', 'Cancel returns the account line' ); - is( $account->balance(), 15, "Account balance is 15" ); - is( $line1->amount+0, 0, 'First fee has amount of 0' ); - is( $line1->amountoutstanding+0, 0, 'First fee has amount outstanding of 0' ); + throws_ok { + $debit1->reduce( { staff_id => $staff->borrowernumber } ); + } + 'Koha::Exceptions::MissingParameter', + "->cancel() requires the `branch` parameter is passed"; + throws_ok { + $debit1->reduce( { branch => $branchcode } ); + } + 'Koha::Exceptions::MissingParameter', + "->cancel() requires the `staff_id` parameter is passed"; + + throws_ok { + $debit2->cancel( + { staff_id => $staff->borrowernumber, branch => $branchcode } ); + } + 'Koha::Exceptions::Account', + '->cancel() can only be used with debits that have not been offset'; - $ret = $line2->cancel(); - is ($ret, undef, 'cancel returns undef when line cannot be cancelled'); + my $cancellation = $debit1->cancel( + { staff_id => $staff->borrowernumber, branch => $branchcode } ); + is( ref($cancellation), 'Koha::Account::Line', + 'Cancel returns an account line' ); + is( + $cancellation->amount() * 1, + $debit1->amount * -1, + "Cancellation amount is " . $debit1->amount + ); + is( $cancellation->amountoutstanding() * 1, + 0, "Cancellation amountoutstanding is 0" ); + is( $debit1->amountoutstanding() * 1, + 0, "Debit amountoutstanding reduced to 0" ); + is( $debit1->status(), 'CANCELLED', "Debit status updated to CANCELLED" ); + is( $account->balance() * 1, 15, "Account balance is 15" ); - my $account_payment = Koha::Account::Lines->find($id); - $ret = $account_payment->cancel(); - is ($ret, undef, 'payment cannot be cancelled'); + my $offsets = Koha::Account::Offsets->search( + { credit_id => $cancellation->id, debit_id => $debit1->id } ); + is( $offsets->count, 1, 'Only one offset is generated' ); + my $THE_offset = $offsets->next; + is( $THE_offset->amount * 1, + -10, 'Correct amount was applied against debit' ); + is( $THE_offset->type, 'CANCELLATION', + "Offset type set to 'CANCELLATION'" ); $schema->storage->txn_rollback; }; -- 2.20.1