Bug 19871: (QA follow-up) Remove safe_do, use translate_exception directly
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 <tomascohen@theke.io> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Tomás Cohen Arazi
Lucas Gass
parent
035d0fc277
commit
e7b258bdb7
71
Koha/Item.pm
71
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();
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue