From 5b6d1e99a24f7834d4c3e8469a729a56bd69cf10 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 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. (Reduce the progress from 50 to 1. If your first record would be fine, check if it is NOT imported when the job fails.) Test reverting batch. Bonus: Put a die in BatchRevertRecords. Signed-off-by: Marcel de Rooy Signed-off-by: Tomas Cohen Arazi --- Koha/BackgroundJob/MARCImportCommitBatch.pm | 13 ++++++++-- Koha/BackgroundJob/MARCImportRevertBatch.pm | 27 ++++++++++++++------- Koha/BackgroundJob/StageMARCForImport.pm | 23 ++++++++++++------ 3 files changed, 44 insertions(+), 19 deletions(-) diff --git a/Koha/BackgroundJob/MARCImportCommitBatch.pm b/Koha/BackgroundJob/MARCImportCommitBatch.pm index 23a8123756..9b8dd510bd 100644 --- a/Koha/BackgroundJob/MARCImportCommitBatch.pm +++ b/Koha/BackgroundJob/MARCImportCommitBatch.pm @@ -20,6 +20,7 @@ use Try::Tiny; use base 'Koha::BackgroundJob'; +use Koha::Database; use Koha::Import::Records; use C4::ImportBatch qw( BatchCommitRecords @@ -74,11 +75,19 @@ sub process { sub { my $job_progress = shift; $self->progress( $job_progress )->store }, { skip_intermediate_commit => 1 }, ); + 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 = { @@ -107,8 +116,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..4cd59392fa 100644 --- a/Koha/BackgroundJob/StageMARCForImport.pm +++ b/Koha/BackgroundJob/StageMARCForImport.pm @@ -20,6 +20,7 @@ use Try::Tiny; use base 'Koha::BackgroundJob'; +use Koha::Database; use C4::Matcher; use C4::ImportBatch qw( RecordsFromMARCXMLFile @@ -87,8 +88,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 +111,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 +123,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 +160,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 +197,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.39.2