From 3afcdc7d21d9aa31240ed00e61cb4c9c9c530958 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 19 Aug 2022 07:54:55 +0000 Subject: [PATCH] Bug 27421: (QA follow-up) Polishing the backgroundjob modules Content-Type: text/plain; charset=utf-8 StageMARCForImport: - Rollback in catch - Setting progress, size or status after BatchStageMarcRecords in both try and catch block ImportCommitBatch: - Move setting size back to enqueue moment - Rollback in catch - Setting progress, size or status after BatchStageMarcRecords in both try and catch block ImportRevertBatch: - Move setting size back to enqueue moment - Adding transaction/rollback to module since import routine does not support it. Could be moved later. - Setting progress, size or status after BatchStageMarcRecords in both try and catch block Test plan: Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t Test staging file Bonus: Put a die statement in BatchStageMarcRecords. Test importing batch Bonus: Include some records with an invalid library code; this will trigger an FK exception. Test reverting batch. Bonus: Put a die in BatchRevertRecords. Signed-off-by: Marcel de Rooy --- Koha/BackgroundJob/MARCImportCommitBatch.pm | 25 +++++++++++-------- Koha/BackgroundJob/MARCImportRevertBatch.pm | 27 ++++++++++++++------- Koha/BackgroundJob/StageMARCForImport.pm | 22 +++++++++++------ 3 files changed, 47 insertions(+), 27 deletions(-) diff --git a/Koha/BackgroundJob/MARCImportCommitBatch.pm b/Koha/BackgroundJob/MARCImportCommitBatch.pm index 57faa99c51..6bec9ff189 100644 --- a/Koha/BackgroundJob/MARCImportCommitBatch.pm +++ b/Koha/BackgroundJob/MARCImportCommitBatch.pm @@ -65,19 +65,24 @@ sub process { my ( $num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored ); try { - my $size = Koha::Import::Records->search({ import_batch_id => $import_batch_id })->count; - $self->size($size)->store; - ( - $num_added, $num_updated, $num_items_added, - $num_items_replaced, $num_items_errored, $num_ignored - ) - = BatchCommitRecords( $import_batch_id, $frameworkcode, 50, - sub { my $job_progress = shift; $self->progress( $job_progress )->store } ); + ( $num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored ) = + BatchCommitRecords( + $import_batch_id, $frameworkcode, 50, + sub { my $job_progress = shift; $self->progress( $job_progress )->store } + ); + my $count = $num_added + $num_updated; + if( $count ) { + $self->set({ progress => $count, size => $count }); + } else { # TODO Refine later + $self->set({ progress => 0, status => 'failed' }); + } } catch { warn $_; + Koha::Database->schema->storage->txn_rollback; # TODO BatchCommitRecords started a transaction die "Something terrible has happened!" if ( $_ =~ /Rollback failed/ ); # Rollback failed + $self->set({ progress => 0, status => 'failed' }); }; my $report = { @@ -106,8 +111,8 @@ sub enqueue { my ( $self, $args) = @_; $self->SUPER::enqueue({ - job_size => 0, # unknown for now - job_args => $args + job_size => Koha::Import::Records->search({ import_batch_id => $args->{import_batch_id} })->count, + job_args => $args, }); } diff --git a/Koha/BackgroundJob/MARCImportRevertBatch.pm b/Koha/BackgroundJob/MARCImportRevertBatch.pm index bd125a07e8..2f294aa58f 100644 --- a/Koha/BackgroundJob/MARCImportRevertBatch.pm +++ b/Koha/BackgroundJob/MARCImportRevertBatch.pm @@ -23,6 +23,8 @@ use base 'Koha::BackgroundJob'; use C4::ImportBatch qw( BatchRevertRecords ); +use Koha::Database; +use Koha::Import::Records; =head1 NAME @@ -64,17 +66,24 @@ sub process { $num_items_deleted, $num_ignored ); + my $schema = Koha::Database->new->schema; try { - ( - $num_deleted, $num_errors, $num_reverted, - $num_items_deleted, $num_ignored - ) = BatchRevertRecords( $import_batch_id, 50, - sub { my $job_progress = shift; $self->progress( $job_progress )->store } ); + $schema->storage->txn_begin; + ( $num_deleted, $num_errors, $num_reverted, $num_items_deleted, $num_ignored ) = + BatchRevertRecords( $import_batch_id ); # TODO BatchRevertRecords still needs a progress_callback + $schema->storage->txn_commit; + + my $count = $num_deleted + $num_reverted; + if( $count ) { + $self->set({ progress => $count, size => $count }); + } else { # TODO Nothing happened? Refine later + $self->set({ progress => 0, status => 'failed' }); + } } catch { warn $_; - die "Something terrible has happened!" - if ( $_ =~ /Rollback failed/ ); # Rollback failed + $schema->storage->txn_rollback; + $self->set({ progress => 0, status => 'failed' }); }; my $report = { @@ -103,8 +112,8 @@ sub enqueue { my ( $self, $args) = @_; $self->SUPER::enqueue({ - job_size => 0, # unknown for now - job_args => $args + job_size => Koha::Import::Records->search({ import_batch_id => $args->{import_batch_id} })->count, + job_args => $args, }); } diff --git a/Koha/BackgroundJob/StageMARCForImport.pm b/Koha/BackgroundJob/StageMARCForImport.pm index a2c12a78fd..869b6c4e52 100644 --- a/Koha/BackgroundJob/StageMARCForImport.pm +++ b/Koha/BackgroundJob/StageMARCForImport.pm @@ -87,8 +87,8 @@ sub process { my $matcher_failed = 0; my $matcher_code = ""; + my $schema = Koha::Database->new->schema; try { - my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; my ( $errors, $marcrecords ); @@ -110,8 +110,7 @@ sub process { $self->size(scalar @$marcrecords)->store; - ( $batch_id, $num_valid, $num_items, @import_errors ) = - BatchStageMarcRecords( + ( $batch_id, $num_valid, $num_items, @import_errors ) = BatchStageMarcRecords( $record_type, $encoding, $marcrecords, $filename, $marc_modification_template, $comments, @@ -123,8 +122,13 @@ sub process { $job_progress /= 2; } $self->progress( int($job_progress) )->store; - } - ); + } + ); + if( $num_valid ) { + $self->set({ progress => $num_valid, size => $num_valid }); + } else { # We must assume that something went wrong here + $self->set({ progress => 0, status => 'failed' }); + } if ($profile_id) { my $ibatch = Koha::ImportBatches->find($batch_id); @@ -155,8 +159,10 @@ sub process { } catch { warn $_; + $schema->storage->txn_rollback; die "Something terrible has happened!" - if ( $_ =~ /Rollback failed/ ); # Rollback failed + if ( $_ =~ /Rollback failed/ ); # TODO Check test: Rollback failed + $self->set({ progress => 0, status => 'failed' }); }; my $report = { @@ -190,8 +196,8 @@ sub enqueue { my ( $self, $args) = @_; $self->SUPER::enqueue({ - job_size => 0, # unknown for now - job_args => $args + job_size => 0, # TODO Unknown for now? + job_args => $args, }); } -- 2.20.1