Attachment 140208 Details for Bug 27421 – Bug 27421: (QA follow-up) Polishing the backgroundjob modules

[patch] Bug 27421: (QA follow-up) Polishing the backgroundjob modules

Bug-27421-QA-follow-up-Polishing-the-backgroundjob.patch (text/plain), 7.48 KB, created by Marcel de Rooy on 2022-09-05 13:13:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2022-09-05 13:13:31 UTC

Size: 7.48 KB

patch

obsolete

>From 3afcdc7d21d9aa31240ed00e61cb4c9c9c530958 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 27421: 136127 | 136128 | 136129 | 136216 | 136217 | 136338 | 136339 | 138642 | 138643 | 138656 | 138657 | 138660 | 138881 | 139384 | 139385 | 139386 | 139387 | 139388 | 139448 | 139449 | 139450 | 139797 | 140205 | 140206 | 140207 | 140208 | 140223 | 140224 | 140225 | 140226 | 140227 | 140228 | 140229 | 140230 | 140231 | 140232 | 140233 | 140234 | 140759