Bugzilla – Attachment 189429 Details for
Bug 40445
Point of Sale reconciliation input during daily summaries
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40445: Improve error handling and validation
Bug-40445-Improve-error-handling-and-validation.patch (text/plain), 9.57 KB, created by
Martin Renvoize (ashimema)
on 2025-11-10 17:04:19 UTC
(
hide
)
Description:
Bug 40445: Improve error handling and validation
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-11-10 17:04:19 UTC
Size:
9.57 KB
patch
obsolete
>From c3aa858119206387b91ea79db37cd318f9e27925 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 @@ > <div id="error_message" class="alert alert-warning"> A cashup is already in progress for this register. </div> > [% END %] > >+ [% IF ( error_cashup_no_transactions ) %] >+ <div id="error_message" class="alert alert-info"> Cannot start cashup - there are no cash transactions in this register since the last cashup. </div> >+ [% END %] >+ > [% IF ( error_no_cashup_start ) %] > <div id="error_message" class="alert alert-warning"> No cashup session has been started. Please start a cashup before attempting to complete it. </div> > [% END %] >@@ -68,8 +72,21 @@ > <div id="error_message" class="alert alert-warning"> Failed to start cashup. Please try again. </div> > [% END %] > >+ [% IF ( error_cashup_missing_param ) %] >+ <div id="error_message" class="alert alert-warning"> Missing required parameter for cashup: [% error_message | html %] </div> >+ [% END %] >+ >+ [% IF ( error_cashup_amount_invalid ) %] >+ <div id="error_message" class="alert alert-warning"> The cashup amount must be a positive number greater than zero. </div> >+ [% END %] >+ > [% IF ( error_cashup_complete ) %] >- <div id="error_message" class="alert alert-warning"> Failed to complete cashup. Please try again. </div> >+ <div id="error_message" class="alert alert-warning"> >+ Failed to complete cashup. Please try again. >+ [% IF error_details %] >+ <br /><strong>Error details:</strong> [% error_details | html %] >+ [% END %] >+ </div> > [% 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 b103b03375b..98dac5fc135 100755 >--- a/t/db_dependent/Koha/Cash/Register.t >+++ b/t/db_dependent/Koha/Cash/Register.t >@@ -1249,8 +1249,10 @@ subtest 'start_cashup_parameter_validation' => sub { > } > ); > >- 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 >@@ -1570,9 +1572,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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40445
:
185289
|
185290
|
185291
|
185292
|
185809
|
186636
|
186637
|
186638
|
186639
|
186640
|
186641
|
186642
|
186643
|
189333
|
189334
|
189335
|
189336
|
189337
|
189338
|
189339
|
189340
|
189341
|
189353
|
189354
|
189355
|
189356
|
189357
|
189358
|
189359
|
189360
|
189361
|
189362
|
189363
|
189364
|
189410
|
189411
|
189412
|
189413
|
189414
|
189415
|
189420
|
189421
|
189422
|
189423
|
189424
|
189425
|
189426
|
189427
|
189428
|
189429
|
189430
|
189476
|
189477
|
189478
|
189479
|
189480
|
189481
|
189482