From 922309b2b5ad7bab40949f03ee808d8066fa6e6e Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Thu, 28 Feb 2019 15:21:59 +0100 Subject: [PATCH] Improved after_callbacks and skip_callback behaviour on bulk changes (e. g. CSV import). --- app/models/concerns/can_csv_import.rb | 4 +--- app/models/cti/caller_id.rb | 7 ++----- app/models/transaction.rb | 6 ++++++ app/models/user.rb | 7 ++----- lib/sequencer/sequence/import/ldap/user.rb | 2 +- .../unit/import/common/model/save.rb | 7 +++++++ .../unit/import/ldap/user/model/save.rb | 20 ------------------- .../unit/import/common/model/save_spec.rb | 13 ++++++++++++ spec/support/cache.rb | 3 +++ spec/support/capybara/set_up.rb | 1 + 10 files changed, 36 insertions(+), 34 deletions(-) delete mode 100644 lib/sequencer/unit/import/ldap/user/model/save.rb diff --git a/app/models/concerns/can_csv_import.rb b/app/models/concerns/can_csv_import.rb index ce924551c..f8b17e6d0 100644 --- a/app/models/concerns/can_csv_import.rb +++ b/app/models/concerns/can_csv_import.rb @@ -214,8 +214,7 @@ returns end # create object - BulkImportInfo.enable - Transaction.execute(disable_notification: true, reset_user_id: true) do + Transaction.execute(disable_notification: true, reset_user_id: true, bulk: true) do UserInfo.current_user_id = clean_params[:updated_by_id] || clean_params[:created_by_id] if !record || delete == true stats[:created] += 1 @@ -263,7 +262,6 @@ returns end end end - BulkImportInfo.disable records.push record end diff --git a/app/models/cti/caller_id.rb b/app/models/cti/caller_id.rb index 5dd7cba3b..1d4c4a245 100644 --- a/app/models/cti/caller_id.rb +++ b/app/models/cti/caller_id.rb @@ -6,11 +6,8 @@ module Cti # adopt/orphan matching Cti::Log records # (see https://github.com/zammad/zammad/issues/2057) - after_commit :update_cti_logs, on: :destroy - after_commit :update_cti_logs_with_fg_optimization, on: :create - - skip_callback :commit, :after, :update_cti_logs, if: -> { BulkImportInfo.enabled? } - skip_callback :commit, :after, :update_cti_logs_with_fg_optimization, if: -> { BulkImportInfo.enabled? } + after_commit :update_cti_logs, on: :destroy, unless: -> { BulkImportInfo.enabled? } + after_commit :update_cti_logs_with_fg_optimization, on: :create, unless: -> { BulkImportInfo.enabled? } =begin diff --git a/app/models/transaction.rb b/app/models/transaction.rb index bbf0ea4b1..62a581e20 100644 --- a/app/models/transaction.rb +++ b/app/models/transaction.rb @@ -3,6 +3,9 @@ class Transaction if options[:reset_user_id] == true UserInfo.current_user_id = 1 end + if options[:bulk] == true + BulkImportInfo.enable + end original_interface_handle = ApplicationHandleInfo.current if options[:interface_handle] ApplicationHandleInfo.current = options[:interface_handle] @@ -16,5 +19,8 @@ class Transaction Observer::Transaction.commit(options) PushMessages.finish end + return if options[:bulk] != true + + BulkImportInfo.disable end end diff --git a/app/models/user.rb b/app/models/user.rb index 4062a83a7..419844631 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -25,14 +25,11 @@ class User < ApplicationModel before_validation :check_mail_delivery_failed, on: :update before_create :check_preferences_default, :validate_preferences, :validate_ooo, :domain_based_assignment, :set_locale before_update :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute - after_create :avatar_for_email_check - after_update :avatar_for_email_check + after_create :avatar_for_email_check, unless: -> { BulkImportInfo.enabled? } + after_update :avatar_for_email_check, unless: -> { BulkImportInfo.enabled? } after_commit :update_caller_id before_destroy :destroy_longer_required_objects - skip_callback :create, :after, :avatar_for_email_check, if: -> { BulkImportInfo.enabled? } - skip_callback :update, :after, :avatar_for_email_check, if: -> { BulkImportInfo.enabled? } - store :preferences activity_stream_permission 'admin.user' diff --git a/lib/sequencer/sequence/import/ldap/user.rb b/lib/sequencer/sequence/import/ldap/user.rb index 9a3c0ad8f..ae91219f2 100644 --- a/lib/sequencer/sequence/import/ldap/user.rb +++ b/lib/sequencer/sequence/import/ldap/user.rb @@ -29,7 +29,7 @@ class Sequencer 'Import::Common::Model::Update', 'Import::Common::Model::Create', 'Import::Common::Model::Associations::Assign', - 'Import::Ldap::User::Model::Save', + 'Import::Common::Model::Save', 'Import::Common::Model::ExternalSync::Integrity', 'Import::Ldap::User::HttpLog', 'Import::Ldap::User::Statistics::Diff', diff --git a/lib/sequencer/unit/import/common/model/save.rb b/lib/sequencer/unit/import/common/model/save.rb index 94e4d0388..0888ac0f1 100644 --- a/lib/sequencer/unit/import/common/model/save.rb +++ b/lib/sequencer/unit/import/common/model/save.rb @@ -18,12 +18,19 @@ class Sequencer return if dry_run return if instance.blank? + save! + end + + def save! + BulkImportInfo.enable instance.save! rescue => e handle_failure(e) # unset instance if something went wrong state.provide(:instance, nil) + ensure + BulkImportInfo.disable end end end diff --git a/lib/sequencer/unit/import/ldap/user/model/save.rb b/lib/sequencer/unit/import/ldap/user/model/save.rb deleted file mode 100644 index d46a26611..000000000 --- a/lib/sequencer/unit/import/ldap/user/model/save.rb +++ /dev/null @@ -1,20 +0,0 @@ -require_dependency 'sequencer/unit/import/common/model/mixin/without_callback' - -class Sequencer - class Unit - module Import - module Ldap - module User - module Model - class Save < Import::Common::Model::Save - prepend ::Sequencer::Unit::Import::Common::Model::Mixin::WithoutCallback - - without_callback :create, :after, :avatar_for_email_check - without_callback :update, :after, :avatar_for_email_check - end - end - end - end - end - end -end diff --git a/spec/lib/sequencer/unit/import/common/model/save_spec.rb b/spec/lib/sequencer/unit/import/common/model/save_spec.rb index d9894c8fe..13a71c52f 100644 --- a/spec/lib/sequencer/unit/import/common/model/save_spec.rb +++ b/spec/lib/sequencer/unit/import/common/model/save_spec.rb @@ -44,4 +44,17 @@ RSpec.describe Sequencer::Unit::Import::Common::Model::Save, sequencer: :unit do expect(user).not_to have_received(:save!) end end + + context 'for BulkImportInfo flag' do + + it 'enables BulkImportInfo' do + expect(BulkImportInfo).to receive(:enable) + process(action: :created, instance: user, dry_run: false) + end + + it 'ensures BulkImportInfo is disabled' do + expect(BulkImportInfo).to receive(:disable) + process(action: :created, instance: user, dry_run: false) + end + end end diff --git a/spec/support/cache.rb b/spec/support/cache.rb index bb1023744..ca769af4f 100644 --- a/spec/support/cache.rb +++ b/spec/support/cache.rb @@ -8,5 +8,8 @@ RSpec.configure do |config| # clear Setting cache to prevent leaking # of Setting changes from previous test examples Setting.reload + + # reset bulk import to prevent wrong base setting + BulkImportInfo.disable end end diff --git a/spec/support/capybara/set_up.rb b/spec/support/capybara/set_up.rb index f197a88c3..f9a83e03f 100644 --- a/spec/support/capybara/set_up.rb +++ b/spec/support/capybara/set_up.rb @@ -4,6 +4,7 @@ RSpec.configure do |config| # make sure system is in a fresh state Cache.clear Setting.reload + BulkImportInfo.disable # check if system is already set up next if Setting.get('system_init_done')