Attachment 126363 Details for Bug 28445 – Bug 28445: Don't surround the whole batch with a transaction

[patch] Bug 28445: Don't surround the whole batch with a transaction

Bug-28445-Dont-surround-the-whole-batch-with-a-tra.patch (text/plain), 10.09 KB, created by Nick Clemens (kidclamp) on 2021-10-15 16:42:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2021-10-15 16:42:21 UTC

Size: 10.09 KB

patch

obsolete

>From 0210bd96ea6569d5ae42ae902fb7d880dded0636 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Mon, 11 Oct 2021 16:26:12 +0200
>Subject: [PATCH] Bug 28445: Don't surround the whole batch with a transaction
>
>It makes more sense to commit when an item has successfully been modified and
>so move the transaction inside the loop.
>
>It also fixes the progress of the whole job.
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> Koha/BackgroundJob/BatchUpdateItem.pm |  36 +++---
> Koha/Items.pm                         | 156 +++++++++++++-------------
> 2 files changed, 96 insertions(+), 96 deletions(-)
>
>diff --git a/Koha/BackgroundJob/BatchUpdateItem.pm b/Koha/BackgroundJob/BatchUpdateItem.pm
>index 1dbc922682..8336b16e23 100644
>--- a/Koha/BackgroundJob/BatchUpdateItem.pm
>+++ b/Koha/BackgroundJob/BatchUpdateItem.pm
>@@ -111,27 +111,22 @@ sub process {
>     };
> 
>     try {
>-        my $schema = Koha::Database->new->schema;
>-        $schema->txn_do(
>-            sub {
>-                my ($results) =
>-                  Koha::Items->search( { itemnumber => \@record_ids } )
>-                  ->batch_update(
>-                    {
>-                        regex_mod  => $regex_mod,
>-                        new_values => $new_values,
>-                        exclude_from_local_holds_priority =>
>-                          $exclude_from_local_holds_priority,
>-                        callback => sub {
>-                            my ($progress) = @_;
>-                            $job->progress($progress)->store;
>-                        },
>-                    }
>-                  );
>-                $report->{modified_itemnumbers} = $results->{modified_itemnumbers};
>-                $report->{modified_fields}      = $results->{modified_fields};
>+        my ($results) =
>+          Koha::Items->search( { itemnumber => \@record_ids } )
>+          ->batch_update(
>+            {
>+                regex_mod  => $regex_mod,
>+                new_values => $new_values,
>+                exclude_from_local_holds_priority =>
>+                  $exclude_from_local_holds_priority,
>+                callback => sub {
>+                    my ($progress) = @_;
>+                    $job->progress($progress)->store;
>+                },
>             }
>-        );
>+          );
>+        $report->{modified_itemnumbers} = $results->{modified_itemnumbers};
>+        $report->{modified_fields}      = $results->{modified_fields};
>     }
>     catch {
>         warn $_;
>@@ -139,6 +134,7 @@ sub process {
>           if ( $_ =~ /Rollback failed/ );    # Rollback failed
>     };
> 
>+    $job->discard_changes;
>     my $job_data = decode_json encode_utf8 $job->data;
>     $job_data->{report} = $report;
> 
>diff --git a/Koha/Items.pm b/Koha/Items.pm
>index 8d5a01cdc7..7349bc4f38 100644
>--- a/Koha/Items.pm
>+++ b/Koha/Items.pm
>@@ -20,6 +20,7 @@ package Koha::Items;
> use Modern::Perl;
> use Array::Utils qw( array_minus );
> use List::MoreUtils qw( uniq );
>+use Try::Tiny;
> 
> use C4::Context;
> use C4::Biblio qw( GetMarcStructure GetMarcFromKohaField );
>@@ -242,98 +243,101 @@ sub batch_update {
> 
>     my (@modified_itemnumbers, $modified_fields);
>     my $i;
>+    my $schema = Koha::Database->new->schema;
>     while ( my $item = $self->next ) {
> 
>-        my $modified_holds_priority = 0;
>-        if ( defined $exclude_from_local_holds_priority ) {
>-            if(!defined $item->exclude_from_local_holds_priority || $item->exclude_from_local_holds_priority != $exclude_from_local_holds_priority) {
>-                $item->exclude_from_local_holds_priority($exclude_from_local_holds_priority)->store;
>-                $modified_holds_priority = 1;
>+        try {$schema->txn_do(sub {
>+            my $modified_holds_priority = 0;
>+            if ( defined $exclude_from_local_holds_priority ) {
>+                if(!defined $item->exclude_from_local_holds_priority || $item->exclude_from_local_holds_priority != $exclude_from_local_holds_priority) {
>+                    $item->exclude_from_local_holds_priority($exclude_from_local_holds_priority)->store;
>+                    $modified_holds_priority = 1;
>+                }
>             }
>-        }
>-
>-        my $modified = 0;
>-        my $new_values = {%$new_values};    # Don't modify the original
>-
>-        my $old_values = $item->unblessed;
>-        if ( $item->more_subfields_xml ) {
>-            $old_values = {
>-                %$old_values,
>-                %{$item->additional_attributes->to_hashref},
>-            };
>-        }
> 
>-        for my $attr ( keys %$regex_mod ) {
>-            my $old_value = $old_values->{$attr};
>+            my $modified = 0;
>+            my $new_values = {%$new_values};    # Don't modify the original
> 
>-            next unless $old_value;
>+            my $old_values = $item->unblessed;
>+            if ( $item->more_subfields_xml ) {
>+                $old_values = {
>+                    %$old_values,
>+                    %{$item->additional_attributes->to_hashref},
>+                };
>+            }
> 
>-            my $value = apply_regex(
>-                {
>-                    %{ $regex_mod->{$attr} },
>-                    value => $old_value,
>-                }
>-            );
>+            for my $attr ( keys %$regex_mod ) {
>+                my $old_value = $old_values->{$attr};
> 
>-            $new_values->{$attr} = $value;
>-        }
>+                next unless $old_value;
> 
>-        for my $attribute ( keys %$new_values ) {
>-            next if $attribute eq 'more_subfields_xml'; # Already counted before
>+                my $value = apply_regex(
>+                    {
>+                        %{ $regex_mod->{$attr} },
>+                        value => $old_value,
>+                    }
>+                );
> 
>-            my $old = $old_values->{$attribute};
>-            my $new = $new_values->{$attribute};
>-            $modified++
>-              if ( defined $old xor defined $new )
>-              || ( defined $old && defined $new && $new ne $old );
>-        }
>+                $new_values->{$attr} = $value;
>+            }
> 
>-        { # Dealing with more_subfields_xml
>-
>-            my $frameworkcode = $item->biblio->frameworkcode;
>-            my $tagslib = C4::Biblio::GetMarcStructure( 1, $frameworkcode, { unsafe => 1 });
>-            my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
>-
>-            my @more_subfield_tags = map {
>-                (
>-                         ref($_)
>-                      && %$_
>-                      && !$_->{kohafield}    # Get subfields that are not mapped
>-                  )
>-                  ? $_->{tagsubfield}
>-                  : ()
>-            } values %{ $tagslib->{$itemtag} };
>-
>-            my $more_subfields_xml = Koha::Item::Attributes->new(
>-                {
>-                    map {
>-                        exists $new_values->{$_} ? ( $_ => $new_values->{$_} )
>-                          : exists $old_values->{$_}
>-                          ? ( $_ => $old_values->{$_} )
>-                          : ()
>-                    } @more_subfield_tags
>-                }
>-            )->to_marcxml($frameworkcode);
>+            for my $attribute ( keys %$new_values ) {
>+                next if $attribute eq 'more_subfields_xml'; # Already counted before
> 
>-            $new_values->{more_subfields_xml} = $more_subfields_xml;
>+                my $old = $old_values->{$attribute};
>+                my $new = $new_values->{$attribute};
>+                $modified++
>+                  if ( defined $old xor defined $new )
>+                  || ( defined $old && defined $new && $new ne $old );
>+            }
> 
>-            delete $new_values->{$_} for @more_subfield_tags; # Clean the hash
>+            { # Dealing with more_subfields_xml
>+
>+                my $frameworkcode = $item->biblio->frameworkcode;
>+                my $tagslib = C4::Biblio::GetMarcStructure( 1, $frameworkcode, { unsafe => 1 });
>+                my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
>+
>+                my @more_subfield_tags = map {
>+                    (
>+                             ref($_)
>+                          && %$_
>+                          && !$_->{kohafield}    # Get subfields that are not mapped
>+                      )
>+                      ? $_->{tagsubfield}
>+                      : ()
>+                } values %{ $tagslib->{$itemtag} };
>+
>+                my $more_subfields_xml = Koha::Item::Attributes->new(
>+                    {
>+                        map {
>+                            exists $new_values->{$_} ? ( $_ => $new_values->{$_} )
>+                              : exists $old_values->{$_}
>+                              ? ( $_ => $old_values->{$_} )
>+                              : ()
>+                        } @more_subfield_tags
>+                    }
>+                )->to_marcxml($frameworkcode);
>+
>+                $new_values->{more_subfields_xml} = $more_subfields_xml;
>+
>+                delete $new_values->{$_} for @more_subfield_tags; # Clean the hash
> 
>-        }
>+            }
> 
>-        if ( $modified ) {
>-            my $itemlost_pre = $item->itemlost;
>-            $item->set($new_values)->store({skip_record_index => 1});
>+            if ( $modified ) {
>+                my $itemlost_pre = $item->itemlost;
>+                $item->set($new_values)->store({skip_record_index => 1});
> 
>-            LostItem(
>-                $item->itemnumber, 'batchmod', undef,
>-                { skip_record_index => 1 }
>-            ) if $item->itemlost
>-                  and not $itemlost_pre;
>+                LostItem(
>+                    $item->itemnumber, 'batchmod', undef,
>+                    { skip_record_index => 1 }
>+                ) if $item->itemlost
>+                      and not $itemlost_pre;
> 
>-            push @modified_itemnumbers, $item->itemnumber if $modified || $modified_holds_priority;
>-            $modified_fields += $modified + $modified_holds_priority;
>-        }
>+                push @modified_itemnumbers, $item->itemnumber if $modified || $modified_holds_priority;
>+                $modified_fields += $modified + $modified_holds_priority;
>+            }
>+        })};
> 
>         if ( $callback ) {
>             $callback->(++$i);
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 28445: 121372 | 123221 | 123222 | 123223 | 123224 | 123225 | 123226 | 123227 | 123228 | 123229 | 123239 | 123240 | 125801 | 125802 | 125803 | 125804 | 125805 | 125808 | 125810 | 125811 | 125812 | 125817 | 125865 | 126046 | 126047 | 126136 | 126137 | 126138 | 126139 | 126140 | 126141 | 126142 | 126217 | 126218 | 126219 | 126223 | 126328 | 126329 | 126330 | 126331 | 126332 | 126333 | 126334 | 126335 | 126336 | 126337 | 126338 | 126339 | 126352 | 126353 | 126354 | 126355 | 126356 | 126357 | 126358 | 126359 | 126360 | 126361 | 126362 | 126363 | 126364 | 126365 | 126366 | 126367 | 126374 | 126375 | 126376 | 126377 | 126378 | 126379 | 126380 | 126381 | 126382 | 126383 | 126384 | 126385 | 126386 | 126387 | 126388 | 126389 | 127033 | 127037 | 127038 | 127039 | 128007 | 128008 | 128009