From e7b258bdb753497677a4b5b7310c49707aa2a89e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Wed, 15 Apr 2026 15:43:32 -0300 Subject: [PATCH] Bug 19871: (QA follow-up) Remove safe_do, use translate_exception directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The safe_do wrapper adds indirection without value: - In Patron::Modification::approve, every operation inside safe_do goes through Object::store or Object::delete, which already call translate_exception internally. The wrapper was translating already-translated exceptions. - In Item::add_to_bundle, the outer try/catch just called rethrow, making the safe_do + try/catch nesting a no-op. This follow-up removes safe_do and aligns both callers with the pattern already used by Object::store and Object::delete: call translate_exception directly in the catch block when raw DBIC operations are involved. Changes: - Remove safe_do from Koha::Schema (and its Try::Tiny import) - Flatten add_to_bundle to use translate_exception in its catch - Flatten Patron::Modification::approve to rely on Object::store - Remove safe_do test, adjust test count Signed-off-by: Tomás Cohen Arazi Signed-off-by: Lucas Gass --- Koha/Item.pm | 71 +++++++++---------- Koha/Patron/Modification.pm | 54 +++++++------- Koha/Schema.pm | 48 +------------ .../Koha/Schema/Util/ExceptionTranslator.t | 30 +------- 4 files changed, 62 insertions(+), 141 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index 50a23012af..eab6d4e370 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -2415,46 +2415,45 @@ sub add_to_bundle { $schema->txn_do( sub { try { - $schema->safe_do( - sub { + Koha::Exceptions::Item::Bundle::BundleIsCheckedOut->throw if $self->checkout; - Koha::Exceptions::Item::Bundle::BundleIsCheckedOut->throw if $self->checkout; - - my $checkout = $bundle_item->checkout; - if ($checkout) { - unless ( $options->{force_checkin} ) { - Koha::Exceptions::Item::Bundle::ItemIsCheckedOut->throw(); - } - - my $branchcode = C4::Context->userenv->{'branch'}; - my ($success) = C4::Circulation::AddReturn( $bundle_item->barcode, $branchcode ); - - if ($success) { - - # HoldsQueue doesn't seem to mind bundles, not sure if this is correct, but rebuild for now - Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( - { biblio_ids => [ $self->biblionumber ] } ) - if C4::Context->preference('RealTimeHoldsQueue'); - } else { - Koha::Exceptions::Checkin::FailedCheckin->throw(); - } - } - - my $holds = $bundle_item->current_holds; - if ( $holds->count ) { - unless ( $options->{ignore_holds} ) { - Koha::Exceptions::Item::Bundle::ItemHasHolds->throw(); - } - } - - $self->_result->add_to_item_bundles_hosts( { item => $bundle_item->itemnumber } ); - - $bundle_item->notforloan($BundleNotLoanValue)->store(); + my $checkout = $bundle_item->checkout; + if ($checkout) { + unless ( $options->{force_checkin} ) { + Koha::Exceptions::Item::Bundle::ItemIsCheckedOut->throw(); } - ); + + my $branchcode = C4::Context->userenv->{'branch'}; + my ($success) = C4::Circulation::AddReturn( $bundle_item->barcode, $branchcode ); + + if ($success) { + + # HoldsQueue doesn't seem to mind bundles, not sure if this is correct, but rebuild for now + Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( + { biblio_ids => [ $self->biblionumber ] } ) + if C4::Context->preference('RealTimeHoldsQueue'); + } else { + Koha::Exceptions::Checkin::FailedCheckin->throw(); + } + } + + my $holds = $bundle_item->current_holds; + if ( $holds->count ) { + unless ( $options->{ignore_holds} ) { + Koha::Exceptions::Item::Bundle::ItemHasHolds->throw(); + } + } + + $self->_result->add_to_item_bundles_hosts( { item => $bundle_item->itemnumber } ); + + $bundle_item->notforloan($BundleNotLoanValue)->store(); } catch { - # Propagate any exception (already translated by safe_do if needed) + # Translate raw DBIC exceptions (from add_to_item_bundles_hosts) + $self->_result->result_source->schema->translate_exception($_) + if ref($_) eq 'DBIx::Class::Exception'; + + # Propagate Koha exceptions as-is $_->rethrow(); }; } diff --git a/Koha/Patron/Modification.pm b/Koha/Patron/Modification.pm index 2e2b9b79f6..11292f4086 100644 --- a/Koha/Patron/Modification.pm +++ b/Koha/Patron/Modification.pm @@ -115,38 +115,34 @@ sub approve { $schema->txn_do( sub { try { - $schema->safe_do( - sub { - $patron->store(); + $patron->store(); - # Deal with attributes - my @codes = uniq( map { $_->{code} } @{$extended_attributes} ); - foreach my $code (@codes) { - map { $_->delete } Koha::Patron::Attributes->search( - { - borrowernumber => $patron->borrowernumber, - code => $code - } - )->as_list; + # Deal with attributes + my @codes = uniq( map { $_->{code} } @{$extended_attributes} ); + foreach my $code (@codes) { + map { $_->delete } Koha::Patron::Attributes->search( + { + borrowernumber => $patron->borrowernumber, + code => $code } - foreach my $attr ( @{$extended_attributes} ) { - $attr->{attribute} = exists $attr->{attribute} ? $attr->{attribute} : $attr->{value}; - Koha::Patron::Attribute->new( - { - borrowernumber => $patron->borrowernumber, - code => $attr->{code}, - attribute => $attr->{attribute}, - } - )->store - if $attr->{attribute} # there's a value - or ( - defined $attr->{attribute} # there's a value that is 0, and not - && $attr->{attribute} ne "" # the empty string which means delete - && $attr->{attribute} == 0 - ); + )->as_list; + } + foreach my $attr ( @{$extended_attributes} ) { + $attr->{attribute} = exists $attr->{attribute} ? $attr->{attribute} : $attr->{value}; + Koha::Patron::Attribute->new( + { + borrowernumber => $patron->borrowernumber, + code => $attr->{code}, + attribute => $attr->{attribute}, } - } - ); + )->store + if $attr->{attribute} # there's a value + or ( + defined $attr->{attribute} # there's a value that is 0, and not + && $attr->{attribute} ne "" # the empty string which means delete + && $attr->{attribute} == 0 + ); + } } catch { # Convert any exception to domain-specific exception diff --git a/Koha/Schema.pm b/Koha/Schema.pm index f3036efd0f..1f7f6ad373 100644 --- a/Koha/Schema.pm +++ b/Koha/Schema.pm @@ -15,59 +15,13 @@ __PACKAGE__->load_namespaces; # Created by DBIx::Class::Schema::Loader v0.07025 @ 2013-10-14 20:56:21 # DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:oDUxXckmfk6H9YCjW8PZTw -use Try::Tiny qw( catch try ); use Koha::Schema::Util::ExceptionTranslator; =head1 UTILITY METHODS -=head2 safe_do - - $schema->safe_do(sub { - # DBIx::Class operations that might throw exceptions - $schema->resultset('SomeTable')->create(\%data); - }, $columns_info); - -Execute a code block with automatic DBIx::Class exception translation. -This provides a centralized way to handle database exceptions throughout the application. - -=head3 Parameters - -=over 4 - -=item * C<$code_ref> - Code reference to execute - -=item * C<$columns_info> (optional) - Hash reference of column information for enhanced error reporting - -=back - -=head3 Example Usage - - # Basic usage - $schema->safe_do(sub { - $register->_result->add_to_cash_register_actions(\%action_data); - }); - - # With column info for enhanced error reporting - my $columns_info = $register->_result->result_source->columns_info; - $schema->safe_do(sub { - $register->_result->create_related('some_relation', \%data); - }, $columns_info); - -=cut - -sub safe_do { - my ( $self, $code_ref, $columns_info, $object ) = @_; - - try { - return $code_ref->(); - } catch { - Koha::Schema::Util::ExceptionTranslator->translate_exception($_, $columns_info, $object); - }; -} - =head2 translate_exception - $schema->translate_exception($exception, $columns_info); + $schema->translate_exception($exception, $columns_info, $object); Convenience method that delegates to the ExceptionTranslator utility. This allows the schema to act as a central point for exception handling. diff --git a/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t b/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t index 56d572cd72..b30c9fdd79 100755 --- a/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t +++ b/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t @@ -19,7 +19,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 9; +use Test::More tests => 8; use Test::Exception; use Koha::Database; @@ -192,32 +192,4 @@ subtest 'enum_truncation_with_object_value' => sub { $schema->storage->txn_rollback; }; -subtest 'schema_safe_do_method' => sub { - plan tests => 2; - - $schema->storage->txn_begin; - - # Test successful operation - my $result = $schema->safe_do( - sub { - return "success"; - } - ); - is( $result, "success", 'safe_do returns result on success' ); - - # Test exception translation - throws_ok { - $schema->safe_do( - sub { - # Create a mock DBIx::Class::Exception - my $exception = bless { msg => "Duplicate entry 'test' for key 'primary'" }, 'DBIx::Class::Exception'; - die $exception; - } - ); - } - 'Koha::Exceptions::Object::DuplicateID', 'safe_do translates exceptions properly'; - - $schema->storage->txn_rollback; -}; - 1;