From 5a78a8f55546bde755668d5a4b0eb25798fd3ae4 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sun, 21 Sep 2025 12:42:50 +0100 Subject: [PATCH] Bug 40445: Improve error handling and validation Enhances cashup error handling and prevents invalid operations. Changes: - Centralized exception handling in Cash Register methods - Detailed error messages for specific exception types: * MissingParameter: Shows parameter name * AmountNotPositive: Clarifies positive amount requirement * Unhandled: Displays debug information - Validation prevents starting cashup with zero transactions - Proper Koha::Exceptions thrown instead of raw DBIx::Class errors Test plan: 1. Apply patch and run prove t/db_dependent/Koha/Cash/Register.t 2. Attempt to start cashup with no transactions: - Verify informational error: "Cannot start cashup - no cash transactions" 3. Attempt cashup operations triggering various exceptions: - Missing parameter: Verify parameter name shown - Zero/negative amount: Verify clear validation message 4. Add cash transaction and retry: - Verify cashup starts successfully 5. Test multi-register operations: - Verify appropriate errors for registers with no transactions - Verify other registers process successfully --- Koha/Cash/Register.pm | 47 +++++++++++++------ .../prog/en/modules/pos/register.tt | 19 +++++++- pos/register.pl | 10 +++- pos/registers.pl | 2 + t/db_dependent/Koha/Cash/Register.t | 13 +++-- 5 files changed, 69 insertions(+), 22 deletions(-) diff --git a/Koha/Cash/Register.pm b/Koha/Cash/Register.pm index b54f9712143..3261ca1ff11 100644 --- a/Koha/Cash/Register.pm +++ b/Koha/Cash/Register.pm @@ -291,14 +291,28 @@ sub start_cashup { my $expected_amount = abs( $self->outstanding_accountlines->total( { payment_type => [ 'CASH', 'SIP00' ] } ) ); - # Create the CASHUP_START action - my $rs = $self->_result->add_to_cash_register_actions( - { - code => 'CASHUP_START', - manager_id => $manager_id, - amount => $expected_amount + # Prevent starting a cashup when there are no cash transactions + unless ( $expected_amount > 0 ) { + Koha::Exceptions::Object::BadValue->throw( + error => "Cannot start cashup with zero cash transactions", + type => 'amount', + value => $expected_amount + ); + } + + # Create the CASHUP_START action using centralized exception handling + my $schema = $self->_result->result_source->schema; + my $rs = $schema->safe_do( + sub { + return $self->_result->add_to_cash_register_actions( + { + code => 'CASHUP_START', + manager_id => $manager_id, + amount => $expected_amount + } + )->discard_changes; } - )->discard_changes; + ); return Koha::Cash::Register::Cashup->_new_from_dbic($rs); } @@ -387,15 +401,18 @@ sub add_cashup { $schema->txn_do( sub { - # Create the cashup action with actual amount - my $rs = $self->_result->add_to_cash_register_actions( - { - code => 'CASHUP', - manager_id => $manager_id, - amount => $amount + # Create the cashup action - safe_do handles exception translation + my $rs = $schema->safe_do( + sub { + return $self->_result->add_to_cash_register_actions( + { + code => 'CASHUP', + manager_id => $manager_id, + amount => $amount + } + )->discard_changes; } - )->discard_changes; - + ); $cashup = Koha::Cash::Register::Cashup->_new_from_dbic($rs); # Create reconciliation accountline if there's a difference diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt index 2f012023fd1..a23383c01e0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt @@ -56,6 +56,10 @@
A cashup is already in progress for this register.
[% END %] + [% IF ( error_cashup_no_transactions ) %] +
Cannot start cashup - there are no cash transactions in this register since the last cashup.
+ [% END %] + [% IF ( error_no_cashup_start ) %]
No cashup session has been started. Please start a cashup before attempting to complete it.
[% END %] @@ -68,8 +72,21 @@
Failed to start cashup. Please try again.
[% END %] + [% IF ( error_cashup_missing_param ) %] +
Missing required parameter for cashup: [% error_message | html %]
+ [% END %] + + [% IF ( error_cashup_amount_invalid ) %] +
The cashup amount must be a positive number greater than zero.
+ [% END %] + [% IF ( error_cashup_complete ) %] -
Failed to complete cashup. Please try again.
+
+ Failed to complete cashup. Please try again. + [% IF error_details %] +
Error details: [% error_details | html %] + [% END %] +
[% END %] [% IF ( error_refund_permission ) %] diff --git a/pos/register.pl b/pos/register.pl index df1faf73def..3a9524adfe4 100755 --- a/pos/register.pl +++ b/pos/register.pl @@ -117,6 +117,8 @@ if ( !$registers->count ) { if ($@) { if ( $@->isa('Koha::Exceptions::Object::DuplicateID') ) { $template->param( error_cashup_in_progress => 1 ); + } elsif ( $@->isa('Koha::Exceptions::Object::BadValue') ) { + $template->param( error_cashup_no_transactions => 1 ); } else { $template->param( error_cashup_start => 1 ); } @@ -157,8 +159,14 @@ if ( !$registers->count ) { $template->param( error_no_cashup_start => 1 ); } elsif ( $@->isa('Koha::Exceptions::Object::DuplicateID') ) { $template->param( error_cashup_already_completed => 1 ); + } elsif ( $@->isa('Koha::Exceptions::MissingParameter') ) { + $template->param( error_cashup_missing_param => 1, error_message => $@ ); + } elsif ( $@->isa('Koha::Exceptions::Account::AmountNotPositive') ) { + $template->param( error_cashup_amount_invalid => 1 ); } else { - $template->param( error_cashup_complete => 1 ); + + # Log the actual exception for debugging + $template->param( error_cashup_complete => 1, error_details => "$@" ); } } else { diff --git a/pos/registers.pl b/pos/registers.pl index 071a4a17824..2dc7e0fa441 100755 --- a/pos/registers.pl +++ b/pos/registers.pl @@ -98,6 +98,8 @@ if ( $op eq 'cud-cashup_start' ) { if ($@) { if ( $@->isa('Koha::Exceptions::Object::DuplicateID') ) { push @errors, "Register " . $register->name . ": Cashup already in progress"; + } elsif ( $@->isa('Koha::Exceptions::Object::BadValue') ) { + push @errors, "Register " . $register->name . ": No cash transactions to cashup"; } else { push @errors, "Register " . $register->name . ": Failed to start cashup"; } diff --git a/t/db_dependent/Koha/Cash/Register.t b/t/db_dependent/Koha/Cash/Register.t index 64618dfdc8b..078cb70ca7f 100755 --- a/t/db_dependent/Koha/Cash/Register.t +++ b/t/db_dependent/Koha/Cash/Register.t @@ -1071,8 +1071,10 @@ subtest 'start_cashup_parameter_validation' => sub { my $register3 = $builder->build_object( { class => 'Koha::Cash::Registers' } ); - eval { $register3->start_cashup( { manager_id => 99999999 } ); }; - ok( $@, 'start_cashup fails with invalid manager_id' ); + throws_ok { + $register3->start_cashup( { manager_id => 99999999 } ); + } + 'Koha::Exceptions::Object::FKConstraint', 'start_cashup throws FK constraint exception with invalid manager_id'; }; # Test 4: Duplicate start_cashup prevention @@ -1377,9 +1379,10 @@ subtest 'add_cashup' => sub { my $register9 = $builder->build_object( { class => 'Koha::Cash::Registers' } ); - eval { $register9->add_cashup( { manager_id => 99999999, amount => '10.00' } ); }; - ok( $@, 'add_cashup fails with invalid manager_id' ); - diag($@); + throws_ok { + $register9->add_cashup( { manager_id => 99999999, amount => '10.00' } ); + } + 'Koha::Exceptions::Object::FKConstraint', 'add_cashup throws FK constraint exception with invalid manager_id'; }; $schema->storage->txn_rollback; -- 2.51.1