From: Moritz Bunkus <m.bunkus@linet-services.de>
Date: Thu, 18 Aug 2016 08:04:24 +0000 (+0200)
Subject: »with_transaction« anstelle von »do_transaction« verwenden
X-Git-Tag: release-3.5.4~2127^2
X-Git-Url: http://wagnertech.de/git?a=commitdiff_plain;h=96670fe82a38116ac10592a6ccbd34800f8ad9f8;p=kivitendo-erp.git

»with_transaction« anstelle von »do_transaction« verwenden

Es sollte so selten wie möglich »do_transaction« verwndet werden, damit
man sich immer angewöhnt, »with_transaction« zu nutzen.

Hintergründe und Unterschiede zwischen den beiden Funktionen sind in der
Dokumentation von SL/DB.pm beschrieben.
---

diff --git a/SL/BackgroundJob/CreatePeriodicInvoices.pm b/SL/BackgroundJob/CreatePeriodicInvoices.pm
index 36dcb1ba7..22f9bf985 100644
--- a/SL/BackgroundJob/CreatePeriodicInvoices.pm
+++ b/SL/BackgroundJob/CreatePeriodicInvoices.pm
@@ -205,7 +205,7 @@ sub _create_periodic_invoice {
 
   my $order   = $config->order;
   my $invoice;
-  if (!$self->{db_obj}->db->do_transaction(sub {
+  if (!$self->{db_obj}->db->with_transaction(sub {
     1;                          # make Emacs happy
 
     $invoice = SL::DB::Invoice->new_from($order);
@@ -254,6 +254,8 @@ sub _create_periodic_invoice {
     _log_msg("_create_invoice created for period start date $period_start_date id " . $invoice->id . " number " . $invoice->invnumber . " netamount " . $invoice->netamount . " amount " . $invoice->amount);
 
     # die $invoice->transaction_description;
+
+    1;
   })) {
     $::lxdebug->message(LXDebug->WARN(), "_create_invoice failed: " . join("\n", (split(/\n/, $self->{db_obj}->db->error))[0..2]));
     return undef;
diff --git a/SL/Controller/Buchungsgruppen.pm b/SL/Controller/Buchungsgruppen.pm
index daaf94f5c..4776bd640 100644
--- a/SL/Controller/Buchungsgruppen.pm
+++ b/SL/Controller/Buchungsgruppen.pm
@@ -85,12 +85,13 @@ sub action_delete {
   # allow deletion of unused Buchungsgruppen. Will fail, due to database
   # constraint, if Buchungsgruppe is connected to a part
 
-  my $db = $self->{config}->db;
-  $db->do_transaction(sub {
-        my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ buchungsgruppen_id => $self->config->id ]);
-        foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete };
-        $self->config->delete();
-        flash_later('info',  $::locale->text('The booking group has been deleted.'));
+  $self->{config}->db->with_transaction(sub {
+    my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ buchungsgruppen_id => $self->config->id ]);
+    foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete };
+    $self->config->delete();
+    flash_later('info',  $::locale->text('The booking group has been deleted.'));
+
+    1;
   }) || flash_later('error', $::locale->text('The booking group is in use and cannot be deleted.'));
 
   $self->redirect_to(action => 'list');
@@ -133,7 +134,7 @@ sub create_or_update {
   my @errors;
 
   my $db = $self->config->db;
-  $db->do_transaction( sub {
+  if (!$db->with_transaction(sub {
 
     $self->config->assign_attributes(%{ $params }); # assign description and inventory_accno_id
 
@@ -169,9 +170,13 @@ sub create_or_update {
         $taxzone_chart->save;
       }
     }
-  } ) || die @errors ? join("\n", @errors) . "\n" : $db->error . "\n";
-         # die with rollback of taxzone save if saving of any of the taxzone_charts fails
-         # only show the $db->error if we haven't already identified the likely error ourselves
+
+    1;
+  })) {
+    die @errors ? join("\n", @errors) . "\n" : $db->error . "\n";
+    # die with rollback of taxzone save if saving of any of the taxzone_charts fails
+    # only show the $db->error if we haven't already identified the likely error ourselves
+  }
 
   flash_later('info', $is_new ? t8('The booking group has been created.') : t8('The booking group has been saved.'));
   $self->redirect_to(action => 'list');
diff --git a/SL/Controller/CustomerVendor.pm b/SL/Controller/CustomerVendor.pm
index fdccdc9d7..b2e32e6fa 100644
--- a/SL/Controller/CustomerVendor.pm
+++ b/SL/Controller/CustomerVendor.pm
@@ -152,7 +152,7 @@ sub _save {
 
   my $db = $self->{cv}->db;
 
-  $db->do_transaction(sub {
+  $db->with_transaction(sub {
     my $cvs_by_nr;
     if ( $self->is_vendor() ) {
       if ( $self->{cv}->vendornumber ) {
@@ -223,6 +223,8 @@ sub _save {
         $note->delete(cascade => 'delete');
       }
     }
+
+    1;
   }) || die($db->error);
 
 }
@@ -351,7 +353,7 @@ sub action_delete {
     $self->action_edit();
   } else {
 
-    $db->do_transaction(sub {
+    $db->with_transaction(sub {
       $self->{cv}->delete(cascade => 1);
 
       my $snumbers = $self->is_vendor() ? 'vendornumber_'. $self->{cv}->vendornumber : 'customernumber_'. $self->{cv}->customernumber;
@@ -379,7 +381,7 @@ sub action_delete_contact {
     SL::Helper::Flash::flash('error', $::locale->text('No contact selected to delete'));
   } else {
 
-    $db->do_transaction(sub {
+    $db->with_transaction(sub {
       if ( $self->{contact}->used ) {
         $self->{contact}->detach();
         $self->{contact}->save();
@@ -388,6 +390,8 @@ sub action_delete_contact {
         $self->{contact}->delete(cascade => 1);
         SL::Helper::Flash::flash('info', $::locale->text('Contact deleted.'));
       }
+
+      1;
     }) || die($db->error);
 
     $self->{contact} = $self->_new_contact_object;
@@ -405,7 +409,7 @@ sub action_delete_shipto {
     SL::Helper::Flash::flash('error', $::locale->text('No shipto selected to delete'));
   } else {
 
-    $db->do_transaction(sub {
+    $db->with_transaction(sub {
       if ( $self->{shipto}->used ) {
         $self->{shipto}->detach();
         $self->{shipto}->save(cascade => 1);
@@ -414,6 +418,8 @@ sub action_delete_shipto {
         $self->{shipto}->delete(cascade => 1);
         SL::Helper::Flash::flash('info', $::locale->text('Shipto deleted.'));
       }
+
+      1;
     }) || die($db->error);
 
     $self->{shipto} = SL::DB::Shipto->new();
diff --git a/SL/Controller/MassInvoiceCreatePrint.pm b/SL/Controller/MassInvoiceCreatePrint.pm
index c65d3b5ec..b7212c780 100644
--- a/SL/Controller/MassInvoiceCreatePrint.pm
+++ b/SL/Controller/MassInvoiceCreatePrint.pm
@@ -56,9 +56,9 @@ sub action_create_invoices {
   }
 
   my $db = SL::DB::Invoice->new->db;
+  my @invoices;
 
-  if (!$db->do_transaction(sub {
-    my @invoices;
+  if (!$db->with_transaction(sub {
     foreach my $id (@sales_delivery_order_ids) {
       my $delivery_order    = SL::DB::DeliveryOrder->new(id => $id)->load;
 
@@ -66,17 +66,17 @@ sub action_create_invoices {
       push @invoices, $invoice;
     }
 
-    my $key = sprintf('%d-%d', Time::HiRes::gettimeofday());
-    $::auth->set_session_value("MassInvoiceCreatePrint::ids-${key}" => [ map { $_->id } @invoices ]);
-
-    flash_later('info', t8('The invoices have been created. They\'re pre-selected below.'));
-    $self->redirect_to(action => 'list_invoices', ids => $key);
-
     1;
   })) {
     $::lxdebug->message(LXDebug::WARN(), "Error: " . $db->error);
     $::form->error($db->error);
   }
+
+  my $key = sprintf('%d-%d', Time::HiRes::gettimeofday());
+  $::auth->set_session_value("MassInvoiceCreatePrint::ids-${key}" => [ map { $_->id } @invoices ]);
+
+  flash_later('info', t8('The invoices have been created. They\'re pre-selected below.'));
+  $self->redirect_to(action => 'list_invoices', ids => $key);
 }
 
 sub action_list_invoices {
diff --git a/SL/Controller/Order.pm b/SL/Controller/Order.pm
index 8023aa2b9..480f8915b 100644
--- a/SL/Controller/Order.pm
+++ b/SL/Controller/Order.pm
@@ -893,9 +893,9 @@ sub _delete {
   my ($self) = @_;
 
   my $errors = [];
-  my $db = $self->order->db;
+  my $db     = $self->order->db;
 
-  $db->do_transaction(
+  $db->with_transaction(
     sub {
       my @spoolfiles = grep { $_ } map { $_->spoolfile } @{ SL::DB::Manager::Status->get_all(where => [ trans_id => $self->order->id ]) };
       $self->order->delete;
@@ -915,12 +915,11 @@ sub _save {
   my ($self) = @_;
 
   my $errors = [];
-  my $db = $self->order->db;
+  my $db     = $self->order->db;
 
-  $db->do_transaction(
-    sub {
-      SL::DB::OrderItem->new(id => $_)->delete for @{$self->item_ids_to_delete};
-      $self->order->save(cascade => 1);
+  $db->with_transaction(sub {
+    SL::DB::OrderItem->new(id => $_)->delete for @{$self->item_ids_to_delete};
+    $self->order->save(cascade => 1);
   }) || push(@{$errors}, $db->error);
 
   return $errors;
diff --git a/SL/Controller/RequirementSpec.pm b/SL/Controller/RequirementSpec.pm
index 38bb4460b..68969e61b 100644
--- a/SL/Controller/RequirementSpec.pm
+++ b/SL/Controller/RequirementSpec.pm
@@ -121,7 +121,7 @@ sub action_ajax_edit_time_and_cost_estimate {
 sub action_ajax_save_time_and_cost_estimate {
   my ($self) = @_;
 
-  $self->requirement_spec->db->do_transaction(sub {
+  $self->requirement_spec->db->with_transaction(sub {
     # Make Emacs happy
     1;
     foreach my $attributes (@{ $::form->{requirement_spec_items} || [] }) {
@@ -417,12 +417,13 @@ sub create_or_update {
   }
 
   my $db = $self->requirement_spec->db;
-  if (!$db->do_transaction(sub {
+  if (!$db->with_transaction(sub {
     if ($self->copy_source) {
       $self->requirement_spec($self->copy_source->create_copy(%{ $params }));
     } else {
       $self->requirement_spec->save(cascade => 1);
     }
+    1;
   })) {
     $::lxdebug->message(LXDebug::WARN(), "Error: " . $db->error);
     @errors = ($::locale->text('Saving failed. Error message from the database: #1', $db->error));
@@ -635,7 +636,7 @@ sub update_project_link_create {
   return $self->js->error(@errors)->render if @errors;
 
   my $db = $self->requirement_spec->db;
-  if (!$db->do_transaction(sub {
+  if (!$db->with_transaction(sub {
     $project->save;
     $self->requirement_spec->update_attributes(project_id => $project->id);
 
diff --git a/SL/Controller/RequirementSpecItem.pm b/SL/Controller/RequirementSpecItem.pm
index b7512928e..d32d28481 100644
--- a/SL/Controller/RequirementSpecItem.pm
+++ b/SL/Controller/RequirementSpecItem.pm
@@ -87,7 +87,7 @@ sub action_dragged_and_dropped {
   my $old_type            = $self->item->item_type;
   my $new_type            = !$dropped_item ? 'section' : $position =~ m/before|after/ ? $dropped_item->item_type : $dropped_item->child_type;
 
-  $self->item->db->do_transaction(sub {
+  $self->item->db->with_transaction(sub {
     $self->item->remove_from_list;
     $self->item->parent_id($position =~ m/before|after/ ? $dropped_item->parent_id : $dropped_item->id) if $dropped_item;
     $self->item->item_type($new_type);
diff --git a/SL/Controller/RequirementSpecPart.pm b/SL/Controller/RequirementSpecPart.pm
index b9df7f545..57cfb49b3 100644
--- a/SL/Controller/RequirementSpecPart.pm
+++ b/SL/Controller/RequirementSpecPart.pm
@@ -73,7 +73,7 @@ sub action_ajax_save {
   my ($self) = @_;
 
   my $db = $self->requirement_spec->db;
-  $db->do_transaction(sub {
+  $db->with_transaction(sub {
     # Make Emacs happy
     1;
     my $parts    = $::form->{additional_parts} || [];
@@ -81,8 +81,6 @@ sub action_ajax_save {
     $_->{position} = $position++ for @{ $parts };
 
     $self->requirement_spec->update_attributes(parts => $parts)->load;
-
-    1;
   }) or do {
     return $self->js->error(t8('Saving failed. Error message from the database: #1', $db->error))->render;
   };
diff --git a/SL/Controller/RequirementSpecTextBlock.pm b/SL/Controller/RequirementSpecTextBlock.pm
index 264b83b5a..055281907 100644
--- a/SL/Controller/RequirementSpecTextBlock.pm
+++ b/SL/Controller/RequirementSpecTextBlock.pm
@@ -184,7 +184,7 @@ sub action_dragged_and_dropped {
   my $dropped_type       = $position ne 'last' ? undef : $::form->{dropped_type} =~ m/^ text-blocks- (?:front|back) $/x ? $::form->{dropped_type} : die "Unknown 'dropped_type' parameter";
   my $old_where          = $self->text_block->output_position;
 
-  $self->text_block->db->do_transaction(sub {
+  $self->text_block->db->with_transaction(sub {
     1;
     $self->text_block->remove_from_list;
     $self->text_block->output_position($position =~ m/before|after/ ? $dropped_text_block->output_position : $::form->{dropped_type} eq 'text-blocks-front' ? 0 : 1);
@@ -482,7 +482,7 @@ sub show_list {
 sub paste_picture {
   my ($self, $copied) = @_;
 
-  if (!$self->text_block->db->do_transaction(sub {
+  if (!$self->text_block->db->with_transaction(sub {
     1;
     $self->picture($copied->to_object)->save;        # Create new picture from copied data and save
     $self->text_block->add_pictures($self->picture); # Add new picture to text block
diff --git a/SL/Controller/Taxzones.pm b/SL/Controller/Taxzones.pm
index 56c9f6334..1594b9b37 100644
--- a/SL/Controller/Taxzones.pm
+++ b/SL/Controller/Taxzones.pm
@@ -72,12 +72,13 @@ sub action_delete {
   # allow deletion of unused tax zones. Will fail, due to database
   # constraints, if tax zone is used anywhere
 
-  my $db = $self->{config}->db;
-  $db->do_transaction(sub {
-        my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ taxzone_id => $self->config->id ]);
-        foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete };
-        $self->config->delete();
-        flash_later('info',  $::locale->text('The tax zone has been deleted.'));
+  $self->{config}->db->with_transaction(sub {
+    my $taxzone_charts = SL::DB::Manager::TaxzoneChart->get_all(where => [ taxzone_id => $self->config->id ]);
+    foreach my $taxzonechart ( @{$taxzone_charts} ) { $taxzonechart->delete };
+    $self->config->delete();
+    flash_later('info',  $::locale->text('The tax zone has been deleted.'));
+
+    1;
   }) || flash_later('error', $::locale->text('The tax zone is in use and cannot be deleted.'));
 
   $self->redirect_to(action => 'list');
@@ -121,7 +122,7 @@ sub create_or_update {
   my @errors;
 
   my $db = $self->config->db;
-  $db->do_transaction( sub {
+  if (!$db->with_transaction(sub {
 
     # always allow editing of description and obsolete
     $self->config->assign_attributes( %{$params} ) ;
@@ -160,9 +161,13 @@ sub create_or_update {
         $taxzone_chart->save;
       }
     }
-  } ) || die @errors ? join("\n", @errors) . "\n" : $db->error . "\n";
-         # die with rollback of taxzone save if saving of any of the taxzone_charts fails
-         # only show the $db->error if we haven't already identified the likely error ourselves
+
+    1;
+  })) {
+    die @errors ? join("\n", @errors) . "\n" : $db->error . "\n";
+    # die with rollback of taxzone save if saving of any of the taxzone_charts fails
+    # only show the $db->error if we haven't already identified the likely error ourselves
+  }
 
   flash_later('info', $is_new ? t8('The taxzone has been created.') : t8('The taxzone has been saved.'));
   $self->redirect_to(action => 'list');
diff --git a/SL/DB/Helper/ActsAsList.pm b/SL/DB/Helper/ActsAsList.pm
index f48e9967a..ee49f4aed 100644
--- a/SL/DB/Helper/ActsAsList.pm
+++ b/SL/DB/Helper/ActsAsList.pm
@@ -40,7 +40,7 @@ sub move_position_down {
 sub remove_from_list {
   my ($self) = @_;
 
-  my $worker = sub {
+  return $self->db->with_transaction(sub {
     remove_position($self);
 
     # Set to -1 manually because $self->update_attributes() would
@@ -56,9 +56,7 @@ sub remove_from_list {
 SQL
     $self->db->dbh->do($sql, undef, $self->$primary_key_col);
     $self->$column(undef);
-  };
-
-  return $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker);
+  });
 }
 
 sub add_to_list {
@@ -109,12 +107,10 @@ sub add_to_list {
       ${group_by}
 SQL
 
-  my $worker = sub {
+  return $self->db->with_transaction(sub {
     $self->db->dbh->do($query, undef, $new_position - 1, @values);
     $self->update_attributes($column => $new_position);
-  };
-
-  return $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker);
+  });
 }
 
 sub get_next_in_list {
@@ -144,7 +140,7 @@ sub reorder_list {
 
   my $self   = ref($class_or_self) ? $class_or_self : $class_or_self->new;
   my $column = column_name($self);
-  my $result = $self->db->do_transaction(sub {
+  my $result = $self->db->with_transaction(sub {
     my $query = qq|UPDATE | . $self->meta->table . qq| SET ${column} = ? WHERE id = ?|;
     my $sth   = $self->db->dbh->prepare($query) || die $self->db->dbh->errstr;
 
@@ -153,6 +149,8 @@ sub reorder_list {
     }
 
     $sth->finish;
+
+    1;
   });
 
   return $result;
diff --git a/SL/DB/Invoice.pm b/SL/DB/Invoice.pm
index 121f7659c..fab4974f3 100644
--- a/SL/DB/Invoice.pm
+++ b/SL/DB/Invoice.pm
@@ -252,7 +252,7 @@ sub post {
     $params{ar_id} = $chart->id;
   }
 
-  my $worker = sub {
+  if (!$self->db->with_transaction(sub {
     my %data = $self->calculate_prices_and_taxes;
 
     $self->_post_create_assemblyitem_entries($data{assembly_items});
@@ -267,11 +267,9 @@ sub post {
     $self->_post_update_allocated($data{allocated});
 
     $self->_post_book_rounding($data{rounding});
-  };
 
-  if ($self->db->in_transaction) {
-    $worker->();
-  } elsif (!$self->db->do_transaction($worker)) {
+    1;
+  })) {
     $::lxdebug->message(LXDebug->WARN(), "convert_to_invoice failed: " . join("\n", (split(/\n/, $self->db->error))[0..2]));
     return undef;
   }
diff --git a/SL/DB/Object.pm b/SL/DB/Object.pm
index fa474643a..4c3e18439 100755
--- a/SL/DB/Object.pm
+++ b/SL/DB/Object.pm
@@ -140,21 +140,15 @@ sub load {
 sub save {
   my ($self, @args) = @_;
 
-  my ($result, $exception);
-  my $worker = sub {
-    $exception = $EVAL_ERROR unless eval {
-      SL::DB::Object::Hooks::run_hooks($self, 'before_save');
-      $result = $self->SUPER::save(@args);
-      SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result);
-      1;
-    };
+  my $result;
 
-    return $result;
-  };
+  $self->db->with_transaction(sub {
+    SL::DB::Object::Hooks::run_hooks($self, 'before_save');
+    $result = $self->SUPER::save(@args);
+    SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result);
 
-  $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker);
-
-  die $exception if $exception;
+    1;
+  }) || die $self->error;
 
   return $result;
 }
@@ -162,21 +156,15 @@ sub save {
 sub delete {
   my ($self, @args) = @_;
 
-  my ($result, $exception);
-  my $worker = sub {
-    $exception = $EVAL_ERROR unless eval {
-      SL::DB::Object::Hooks::run_hooks($self, 'before_delete');
-      $result = $self->SUPER::delete(@args);
-      SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result);
-      1;
-    };
-
-    return $result;
-  };
+  my $result;
 
-  $self->db->in_transaction ? $worker->() : $self->db->do_transaction($worker);
+  $self->db->with_transaction(sub {
+    SL::DB::Object::Hooks::run_hooks($self, 'before_delete');
+    $result = $self->SUPER::delete(@args);
+    SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result);
 
-  die $exception if $exception;
+    1;
+  }) || die $self->error;
 
   return $result;
 }
diff --git a/SL/DB/RequirementSpec.pm b/SL/DB/RequirementSpec.pm
index c35540c74..024e60d7b 100644
--- a/SL/DB/RequirementSpec.pm
+++ b/SL/DB/RequirementSpec.pm
@@ -145,7 +145,7 @@ sub create_copy {
   return $self->_create_copy(%params) if $self->db->in_transaction;
 
   my $copy;
-  if (!$self->db->do_transaction(sub { $copy = $self->_create_copy(%params) })) {
+  if (!$self->db->with_transaction(sub { $copy = $self->_create_copy(%params) })) {
     $::lxdebug->message(LXDebug->WARN(), "create_copy failed: " . join("\n", (split(/\n/, $self->db->error))[0..2]));
     return undef;
   }
diff --git a/t/ar/ar.t b/t/ar/ar.t
index 79fcc0a4a..7a97738ec 100644
--- a/t/ar/ar.t
+++ b/t/ar/ar.t
@@ -72,7 +72,7 @@ sub ar {
 
   my $db = $invoice->db;
 
-  $db->do_transaction( sub {
+  $db->with_transaction( sub {
 
   my $tax = SL::DB::Manager::Tax->find_by(taxkey => 0, rate => 0);
 
@@ -91,6 +91,8 @@ sub ar {
 
   _save_and_pay_and_check(invoice => $invoice, bank => $bank, pay => 1, check => 1);
 
+  1;
+
   }) || die "something went wrong: " . $db->error;
   return $invoice->invnumber;
 };
@@ -119,7 +121,7 @@ sub ar_with_tax {
 
   my $db = $invoice->db;
 
-  $db->do_transaction( sub {
+  $db->with_transaction( sub {
 
   # TODO: check for currency and exchange rate
 
@@ -140,6 +142,7 @@ sub ar_with_tax {
   $invoice->create_ar_row( chart => $ar_chart );
   _save_and_pay_and_check(invoice => $invoice, bank => $bank, pay => 1, check => 1);
 
+  1;
   }) || die "something went wrong: " . $db->error;
   return $invoice->invnumber;
 };