From bdb107bcec6d87649d6c38ac8bee960a321c9ab0 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Mon, 5 Sep 2022 11:57:52 +0000
Subject: [PATCH] Bug 27421: (QA follow-up) BatchCommitImportRecords needs
 param for skipping commits

When you submit a background jobs, and it fails, you do not expect
partial results in the database.
Note that when the Background feature would support a partially
completed status, things might change again.

Note that the >0 test was superfluous if you check for ^\d+$.

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t

Note: This serves to verify that it still runs as expected.
The test plan of the following patch covers the new param.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
---
 C4/ImportBatch.pm                           | 38 +++++++++------------
 Koha/BackgroundJob/MARCImportCommitBatch.pm | 13 +++----
 2 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm
index ddf10e666d..905a44aa62 100644
--- a/C4/ImportBatch.pm
+++ b/C4/ImportBatch.pm
@@ -509,28 +509,23 @@ sub BatchFindDuplicates {
 
 =head2 BatchCommitRecords
 
-  my ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored) =
-        BatchCommitRecords($batch_id, $framework,
-        $progress_interval, $progress_callback);
+    my ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored) =
+        BatchCommitRecords( $batch_id, $framework, $progress_interval, $progress_callback, $params );
+
+    Parameter skip_intermediate_commit does what is says.
 
 =cut
 
 sub BatchCommitRecords {
-    my $batch_id = shift;
-    my $framework = shift;
+    my ( $batch_id, $framework, $progress_interval, $progress_callback, $params ) = @_;
+    my $skip_intermediate_commit = $params->{skip_intermediate_commit};
+    $progress_interval = 0 unless $progress_interval && $progress_interval =~ /^\d+$/;
+    $progress_interval = 0 unless ref($progress_callback) eq 'CODE';
 
     my $schema = Koha::Database->schema;
-
-    # optional callback to monitor status 
-    # of job
-    my $progress_interval = 0;
-    my $progress_callback = undef;
-    if ($#_ == 1) {
-        $progress_interval = shift;
-        $progress_callback = shift;
-        $progress_interval = 0 unless $progress_interval =~ /^\d+$/ and $progress_interval > 0;
-        $progress_interval = 0 unless 'CODE' eq ref $progress_callback;
-    }
+    $schema->txn_begin;
+    # NOTE: Moved this transaction to the front of the routine. Note that inside the while loop below
+    # transactions may be committed and started too again. The final commit is close to the end.
 
     my $record_type;
     my $num_added = 0;
@@ -560,7 +555,6 @@ sub BatchCommitRecords {
 
     my $rec_num = 0;
     my @biblio_ids;
-    $schema->txn_begin; # We commit in a transaction
     while (my $rowref = $sth->fetchrow_hashref) {
         $record_type = $rowref->{'record_type'};
 
@@ -568,9 +562,9 @@ sub BatchCommitRecords {
 
         if ($progress_interval and (0 == ($rec_num % $progress_interval))) {
             # report progress and commit
-            $schema->txn_commit;
+            $schema->txn_commit unless $skip_intermediate_commit;
             &$progress_callback( $rec_num );
-            $schema->txn_begin;
+            $schema->txn_begin unless $skip_intermediate_commit;
         }
         if ($rowref->{'status'} eq 'error' or $rowref->{'status'} eq 'imported') {
             $num_ignored++;
@@ -704,8 +698,6 @@ sub BatchCommitRecords {
         &$progress_callback($rec_num);
     }
 
-    $schema->txn_commit; # Commit final records that may not have hit callback threshold
-
     $sth->finish();
 
     if ( @biblio_ids ) {
@@ -714,6 +706,10 @@ sub BatchCommitRecords {
     }
 
     SetImportBatchStatus($batch_id, 'imported');
+
+    # Moved final commit to the end
+    $schema->txn_commit;
+
     return ($num_added, $num_updated, $num_items_added, $num_items_replaced, $num_items_errored, $num_ignored);
 }
 
diff --git a/Koha/BackgroundJob/MARCImportCommitBatch.pm b/Koha/BackgroundJob/MARCImportCommitBatch.pm
index 57faa99c51..23a8123756 100644
--- a/Koha/BackgroundJob/MARCImportCommitBatch.pm
+++ b/Koha/BackgroundJob/MARCImportCommitBatch.pm
@@ -67,12 +67,13 @@ sub process {
     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 },
+            { skip_intermediate_commit => 1 },
+        );
     }
     catch {
         warn $_;
-- 
2.39.5