From 9c709b871f5f5c2887eefb81a3f93a8ad76fb8d0 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 Signed-off-by: David Nind Signed-off-by: Katrin Fischer https://bugs.koha-community.org/show_bug.cgi?id=24063 Bug 24603: Fix number of unit tests Signed-off-by: Katrin Fischer https://bugs.koha-community.org/show_bug.cgi?id=24063 Signed-off-by: Jonathan Druart --- Koha/Account/Line.pm | 99 +++++++++---- .../data/mysql/atomicupdate/bug_24603.perl | 20 +++ .../mysql/mandatory/account_credit_types.sql | 3 +- .../mysql/mandatory/account_offset_types.sql | 3 +- .../prog/en/modules/members/boraccount.tt | 4 +- members/cancel-charge.pl | 9 +- t/db_dependent/Koha/Account/Line.t | 133 +++++++++++++----- 7 files changed, 203 insertions(+), 68 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_24603.perl diff --git a/Koha/Account/Line.pm b/Koha/Account/Line.pm index 9f6723f6e5..2cc81993ef 100644 --- a/Koha/Account/Line.pm +++ b/Koha/Account/Line.pm @@ -293,46 +293,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 @@ -455,7 +499,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/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/installer/data/mysql/mandatory/account_credit_types.sql b/installer/data/mysql/mandatory/account_credit_types.sql index d7d8a5b047..5ed88fdd2b 100644 --- a/installer/data/mysql/mandatory/account_credit_types.sql +++ b/installer/data/mysql/mandatory/account_credit_types.sql @@ -7,6 +7,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/mandatory/account_offset_types.sql b/installer/data/mysql/mandatory/account_offset_types.sql index 1f4f24cf34..523300b003 100644 --- a/installer/data/mysql/mandatory/account_offset_types.sql +++ b/installer/data/mysql/mandatory/account_offset_types.sql @@ -22,4 +22,5 @@ INSERT INTO account_offset_types ( type ) VALUES ('Credit Applied'), ('PAYOUT'), ('DISCOUNT'), -('REFUND'); +('REFUND'), +('CANCELLATION'); 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 1df48e58e8..84cde2f3f0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt @@ -95,12 +95,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 d767056972..e4504e5114 100755 --- a/t/db_dependent/Koha/Account/Line.t +++ b/t/db_dependent/Koha/Account/Line.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 13; +use Test::More tests => 14; use Test::Exception; use Test::MockModule; @@ -912,7 +912,7 @@ subtest "payout() tests" => sub { subtest "reduce() tests" => sub { - plan tests => 27; + plan tests => 29; $schema->storage->txn_begin; @@ -972,7 +972,7 @@ subtest "reduce() tests" => sub { my $reduce_params = { interface => 'commandline', - reduction_type => 'REFUND', + reduction_type => 'DISCOUNT', amount => 5, staff_id => $staff->borrowernumber, branch => $branchcode @@ -1020,7 +1020,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', @@ -1030,6 +1030,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'" ); @@ -1039,17 +1040,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 } ); @@ -1088,28 +1091,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, @@ -1118,7 +1140,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, @@ -1128,27 +1150,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.39.5