From 5859f76b2940988bca27151e1febb94686b59dcb Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Fri, 15 May 2020 15:36:36 +0100
Subject: [PATCH 1/7] Propagate instance-level integration to projects

When an admin saves an instance-level integration, they will have two
options that will propagate the settings differently.

- Apply changes to inherited integrations:
  - Updates integration for inherited projects
  - Creates integrations for projects without integrations
- Apply changes to all integrations:
  - Updates integration for all projects
  - Creates integrations for projects without integrations
---
 .../concerns/integrations_actions.rb          |   2 +
 .../admin/propagate_integration_service.rb    | 147 ++++++++++++++++++
 app/workers/all_queues.yml                    |   7 +
 app/workers/propagate_integration_worker.rb   |  16 ++
 config/sidekiq_queues.yml                     |   2 +
 .../admin/integrations_controller_spec.rb     |  12 +-
 .../propagate_integration_service_spec.rb     | 124 +++++++++++++++
 .../propagate_integration_worker_spec.rb      |  26 ++++
 8 files changed, 335 insertions(+), 1 deletion(-)
 create mode 100644 app/services/admin/propagate_integration_service.rb
 create mode 100644 app/workers/propagate_integration_worker.rb
 create mode 100644 spec/services/admin/propagate_integration_service_spec.rb
 create mode 100644 spec/workers/propagate_integration_worker_spec.rb

diff --git a/app/controllers/concerns/integrations_actions.rb b/app/controllers/concerns/integrations_actions.rb
index ff283f9bb62ea1..82c476b7961e4c 100644
--- a/app/controllers/concerns/integrations_actions.rb
+++ b/app/controllers/concerns/integrations_actions.rb
@@ -16,10 +16,12 @@ def edit
 
   def update
     saved = integration.update(service_params[:service])
+    update_only_inherited = ActiveRecord::Type::Boolean.new.cast(params[:update_only_inherited])
 
     respond_to do |format|
       format.html do
         if saved
+          PropagateIntegrationWorker.perform_async(integration.id, update_only_inherited)
           redirect_to scoped_edit_integration_path(integration), notice: success_message
         else
           render 'shared/integrations/edit'
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb
new file mode 100644
index 00000000000000..c8f26901aaf793
--- /dev/null
+++ b/app/services/admin/propagate_integration_service.rb
@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+
+module Admin
+  class PropagateIntegrationService
+    BATCH_SIZE = 100
+
+    delegate :data_fields_present?, to: :integration
+
+    def self.propagate(integration:, update_only_inherited:)
+      new(integration, update_only_inherited).propagate
+    end
+
+    def initialize(integration, update_only_inherited)
+      @integration = integration
+      @update_only_inherited = update_only_inherited
+    end
+
+    def propagate
+      if update_only_inherited
+        update_integration_for_inherited_projects
+      else
+        update_integration_for_all_projects
+      end
+
+      create_integration_for_projects_without_integration
+    end
+
+    private
+
+    attr_reader :integration, :update_only_inherited
+
+    # rubocop: disable Cop/InBatches
+    # rubocop: disable CodeReuse/ActiveRecord
+    def update_integration_for_inherited_projects
+      Service.where(type: integration.type, inherit_from_id: integration.id).in_batches(of: BATCH_SIZE) do |batch|
+        bulk_update_from_integration(batch)
+      end
+    end
+
+    def update_integration_for_all_projects
+      Service.where(type: integration.type).in_batches(of: BATCH_SIZE) do |batch|
+        bulk_update_from_integration(batch)
+      end
+    end
+    # rubocop: enable Cop/InBatches
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def bulk_update_from_integration(batch)
+      # Retrieving the IDs instantiates the ActiveRecord relation (batch)
+      # into concrete models, otherwise update_all will clear the relation.
+      # https://stackoverflow.com/q/34811646/462015
+      batch_ids = batch.pluck(:id)
+
+      Service.transaction do
+        batch.update_all(service_hash)
+
+        if data_fields_present?
+          integration.data_fields.class.where(service_id: batch_ids).update_all(data_hash)
+        end
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def create_integration_for_projects_without_integration
+      loop do
+        batch = Project.uncached { project_ids_without_integration }
+
+        bulk_create_from_integration(batch) unless batch.empty?
+
+        break if batch.size < BATCH_SIZE
+      end
+    end
+
+    def bulk_create_from_integration(batch)
+      service_list = batch.map do |project_id|
+        service_hash.values << project_id << integration.id
+      end
+
+      Project.transaction do
+        results = bulk_insert(Service, service_hash.keys << 'project_id' << 'inherit_from_id', service_list)
+
+        if data_fields_present?
+          data_list = results.map { |row| data_hash.values << row['id'] }
+
+          bulk_insert(integration.data_fields.class, data_hash.keys << 'service_id', data_list)
+        end
+
+        run_callbacks(batch)
+      end
+    end
+
+    def bulk_insert(klass, columns, values_array)
+      items_to_insert = values_array.map { |array| Hash[columns.zip(array)] }
+
+      klass.insert_all(items_to_insert, returning: [:id])
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def run_callbacks(batch)
+      if active_external_issue_tracker?
+        Project.where(id: batch).update_all(has_external_issue_tracker: true)
+      end
+
+      if active_external_wiki?
+        Project.where(id: batch).update_all(has_external_wiki: true)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def active_external_issue_tracker?
+      integration.issue_tracker? && !integration.default
+    end
+
+    def active_external_wiki?
+      integration.type == 'ExternalWikiService'
+    end
+
+    def project_ids_without_integration
+      Project.connection.select_values(
+        <<-SQL
+          SELECT id
+          FROM projects
+          WHERE NOT EXISTS (
+            SELECT true
+            FROM services
+            WHERE services.project_id = projects.id
+            AND services.type = #{ActiveRecord::Base.connection.quote(integration.type)}
+          )
+          AND projects.pending_delete = false
+          AND projects.archived = false
+          LIMIT #{BATCH_SIZE}
+        SQL
+      )
+    end
+
+    def service_hash
+      @service_hash ||= integration
+        .as_json(methods: :type, except: %w[id instance project_id])
+        .tap { |json| json['inherit_from_id'] = integration.id }
+    end
+
+    def data_hash
+      @data_hash ||= integration.data_fields.as_json(only: integration.data_fields.class.column_names).except('id', 'service_id')
+    end
+  end
+end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 1f9a53d64d9203..1454ededc047e1 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -1291,6 +1291,13 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: true
+- :name: propagate_integration
+  :feature_category: :integrations
+  :has_external_dependencies: 
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
 - :name: propagate_service_template
   :feature_category: :source_code_management
   :has_external_dependencies: 
diff --git a/app/workers/propagate_integration_worker.rb b/app/workers/propagate_integration_worker.rb
new file mode 100644
index 00000000000000..0d35082fa79d09
--- /dev/null
+++ b/app/workers/propagate_integration_worker.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class PropagateIntegrationWorker
+  include ApplicationWorker
+
+  feature_category :integrations
+
+  idempotent!
+
+  def perform(integration_id, update_only_inherited)
+    Admin::PropagateIntegrationService.propagate(
+      integration: Service.find(integration_id),
+      update_only_inherited: update_only_inherited
+    )
+  end
+end
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index e6e0b4b4409fed..2079aad01700f9 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -210,6 +210,8 @@
   - 1
 - - prometheus_create_default_alerts
   - 1
+- - propagate_integration
+  - 1
 - - propagate_service_template
   - 1
 - - reactive_caching
diff --git a/spec/controllers/admin/integrations_controller_spec.rb b/spec/controllers/admin/integrations_controller_spec.rb
index 817223bd91acfc..0276255ddd2520 100644
--- a/spec/controllers/admin/integrations_controller_spec.rb
+++ b/spec/controllers/admin/integrations_controller_spec.rb
@@ -36,7 +36,9 @@
     let(:integration) { create(:jira_service, :instance) }
 
     before do
-      put :update, params: { id: integration.class.to_param, service: { url: url } }
+      allow(PropagateIntegrationWorker).to receive(:perform_async)
+
+      put :update, params: { id: integration.class.to_param, update_only_inherited: true, service: { url: url } }
     end
 
     context 'valid params' do
@@ -46,6 +48,10 @@
         expect(response).to have_gitlab_http_status(:found)
         expect(integration.reload.url).to eq(url)
       end
+
+      it 'calls to PropagateIntegrationWorker' do
+        expect(PropagateIntegrationWorker).to have_received(:perform_async).with(integration.id, true)
+      end
     end
 
     context 'invalid params' do
@@ -56,6 +62,10 @@
         expect(response).to render_template(:edit)
         expect(integration.reload.url).not_to eq(url)
       end
+
+      it 'does not call to PropagateIntegrationWorker' do
+        expect(PropagateIntegrationWorker).not_to have_received(:perform_async)
+      end
     end
   end
 end
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb
new file mode 100644
index 00000000000000..d2ba301642bc17
--- /dev/null
+++ b/spec/services/admin/propagate_integration_service_spec.rb
@@ -0,0 +1,124 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Admin::PropagateIntegrationService do
+  describe '.propagate' do
+    let(:excluded_attributes) { %w[id project_id inherit_from_id instance created_at updated_at title description] }
+    let!(:project) { create(:project) }
+    let!(:instance_integration) do
+      JiraService.create!(
+        instance: true,
+        active: true,
+        push_events: true,
+        url: 'http://update-jira.instance.com',
+        username: 'user',
+        password: 'secret'
+      )
+    end
+
+    let!(:inherited_integration) do
+      JiraService.create!(
+        project: create(:project),
+        inherit_from_id: instance_integration.id,
+        instance: false,
+        active: true,
+        push_events: false,
+        url: 'http://jira.instance.com',
+        username: 'user',
+        password: 'secret'
+      )
+    end
+
+    let!(:not_inherited_integration) do
+      JiraService.create!(
+        project: create(:project),
+        inherit_from_id: nil,
+        instance: false,
+        active: true,
+        push_events: false,
+        url: 'http://jira.instance.com',
+        username: 'user',
+        password: 'secret'
+      )
+    end
+
+    let!(:another_inherited_integration) do
+      BambooService.create!(
+        project: create(:project),
+        inherit_from_id: instance_integration.id,
+        instance: false,
+        active: true,
+        push_events: false,
+        bamboo_url: 'http://gitlab.com',
+        username: 'mic',
+        password: 'password',
+        build_key: 'build'
+      )
+    end
+
+    shared_examples 'inherits settings from integration' do
+      it 'updates the inherited integrations' do
+        described_class.propagate(integration: instance_integration, update_only_inherited: update_only_inherited)
+
+        expect(integration.reload.inherit_from_id).to eq(instance_integration.id)
+
+        expect(integration.attributes.except(*excluded_attributes))
+          .to eq(instance_integration.attributes.except(*excluded_attributes))
+
+        excluded_attributes = %w[id service_id created_at updated_at]
+        expect(integration.data_fields.attributes.except(*excluded_attributes))
+          .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes))
+      end
+    end
+
+    shared_examples 'does not inherit settings from integration' do
+      it 'does not update the not inherited integrations' do
+        described_class.propagate(integration: instance_integration, update_only_inherited: update_only_inherited)
+
+        expect(integration.reload.attributes.except(*excluded_attributes))
+          .not_to eq(instance_integration.attributes.except(*excluded_attributes))
+      end
+    end
+
+    context 'update only inherited integrations' do
+      let(:update_only_inherited) { true }
+
+      it_behaves_like 'inherits settings from integration' do
+        let(:integration) { inherited_integration }
+      end
+
+      it_behaves_like 'does not inherit settings from integration' do
+        let(:integration) { not_inherited_integration }
+      end
+
+      it_behaves_like 'does not inherit settings from integration' do
+        let(:integration) { another_inherited_integration }
+      end
+
+      it_behaves_like 'inherits settings from integration' do
+        let(:integration) { project.jira_service }
+      end
+    end
+
+    context 'update all integrations' do
+      let(:update_only_inherited) { false }
+
+      it_behaves_like 'inherits settings from integration' do
+        let(:integration) { inherited_integration }
+      end
+
+      it_behaves_like 'inherits settings from integration' do
+        let(:integration) { not_inherited_integration }
+      end
+
+      it_behaves_like 'does not inherit settings from integration' do
+        let(:integration) { another_inherited_integration }
+      end
+
+      it_behaves_like 'inherits settings from integration' do
+        let(:integration) { project.jira_service }
+      end
+    end
+  end
+end
diff --git a/spec/workers/propagate_integration_worker_spec.rb b/spec/workers/propagate_integration_worker_spec.rb
new file mode 100644
index 00000000000000..76b1b478ef4eac
--- /dev/null
+++ b/spec/workers/propagate_integration_worker_spec.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PropagateIntegrationWorker do
+  describe '#perform' do
+    let(:integration) do
+      PushoverService.create(
+        template: true,
+        active: true,
+        device: 'MyDevice',
+        sound: 'mic',
+        priority: 4,
+        user_key: 'asdf',
+        api_key: '123456789'
+      )
+    end
+
+    it 'calls the propagate service with the integration' do
+      expect(Admin::PropagateIntegrationService).to receive(:propagate)
+        .with(integration: integration, update_only_inherited: true)
+
+      subject.perform(integration.id, true)
+    end
+  end
+end
-- 
GitLab


From 4714244ca38fe9b39ed4d69697f14fc9f75edeb6 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Mon, 18 May 2020 16:51:32 +0100
Subject: [PATCH 2/7] Rename update_only_inherited to overwrite

This makes the condition more safe because in the case of an empty
params, it defaults to only update inherited services.
---
 app/controllers/concerns/integrations_actions.rb |  4 ++--
 .../admin/propagate_integration_service.rb       | 16 ++++++++--------
 app/workers/propagate_integration_worker.rb      |  4 ++--
 .../admin/integrations_controller_spec.rb        |  2 +-
 .../admin/propagate_integration_service_spec.rb  |  8 ++++----
 .../workers/propagate_integration_worker_spec.rb |  2 +-
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/app/controllers/concerns/integrations_actions.rb b/app/controllers/concerns/integrations_actions.rb
index 82c476b7961e4c..b3ad89f3227c56 100644
--- a/app/controllers/concerns/integrations_actions.rb
+++ b/app/controllers/concerns/integrations_actions.rb
@@ -16,12 +16,12 @@ def edit
 
   def update
     saved = integration.update(service_params[:service])
-    update_only_inherited = ActiveRecord::Type::Boolean.new.cast(params[:update_only_inherited])
+    overwrite = ActiveRecord::Type::Boolean.new.cast(params[:overwrite])
 
     respond_to do |format|
       format.html do
         if saved
-          PropagateIntegrationWorker.perform_async(integration.id, update_only_inherited)
+          PropagateIntegrationWorker.perform_async(integration.id, overwrite)
           redirect_to scoped_edit_integration_path(integration), notice: success_message
         else
           render 'shared/integrations/edit'
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb
index c8f26901aaf793..768f889924e83a 100644
--- a/app/services/admin/propagate_integration_service.rb
+++ b/app/services/admin/propagate_integration_service.rb
@@ -6,20 +6,20 @@ class PropagateIntegrationService
 
     delegate :data_fields_present?, to: :integration
 
-    def self.propagate(integration:, update_only_inherited:)
-      new(integration, update_only_inherited).propagate
+    def self.propagate(integration:, overwrite:)
+      new(integration, overwrite).propagate
     end
 
-    def initialize(integration, update_only_inherited)
+    def initialize(integration, overwrite)
       @integration = integration
-      @update_only_inherited = update_only_inherited
+      @overwrite = overwrite
     end
 
     def propagate
-      if update_only_inherited
-        update_integration_for_inherited_projects
-      else
+      if overwrite
         update_integration_for_all_projects
+      else
+        update_integration_for_inherited_projects
       end
 
       create_integration_for_projects_without_integration
@@ -27,7 +27,7 @@ def propagate
 
     private
 
-    attr_reader :integration, :update_only_inherited
+    attr_reader :integration, :overwrite
 
     # rubocop: disable Cop/InBatches
     # rubocop: disable CodeReuse/ActiveRecord
diff --git a/app/workers/propagate_integration_worker.rb b/app/workers/propagate_integration_worker.rb
index 0d35082fa79d09..cbab38465bc32b 100644
--- a/app/workers/propagate_integration_worker.rb
+++ b/app/workers/propagate_integration_worker.rb
@@ -7,10 +7,10 @@ class PropagateIntegrationWorker
 
   idempotent!
 
-  def perform(integration_id, update_only_inherited)
+  def perform(integration_id, overwrite)
     Admin::PropagateIntegrationService.propagate(
       integration: Service.find(integration_id),
-      update_only_inherited: update_only_inherited
+      overwrite: overwrite
     )
   end
 end
diff --git a/spec/controllers/admin/integrations_controller_spec.rb b/spec/controllers/admin/integrations_controller_spec.rb
index 0276255ddd2520..94d38353189308 100644
--- a/spec/controllers/admin/integrations_controller_spec.rb
+++ b/spec/controllers/admin/integrations_controller_spec.rb
@@ -38,7 +38,7 @@
     before do
       allow(PropagateIntegrationWorker).to receive(:perform_async)
 
-      put :update, params: { id: integration.class.to_param, update_only_inherited: true, service: { url: url } }
+      put :update, params: { id: integration.class.to_param, overwrite: true, service: { url: url } }
     end
 
     context 'valid params' do
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb
index d2ba301642bc17..9a7f7dd6a3f2cf 100644
--- a/spec/services/admin/propagate_integration_service_spec.rb
+++ b/spec/services/admin/propagate_integration_service_spec.rb
@@ -59,7 +59,7 @@
 
     shared_examples 'inherits settings from integration' do
       it 'updates the inherited integrations' do
-        described_class.propagate(integration: instance_integration, update_only_inherited: update_only_inherited)
+        described_class.propagate(integration: instance_integration, overwrite: overwrite)
 
         expect(integration.reload.inherit_from_id).to eq(instance_integration.id)
 
@@ -74,7 +74,7 @@
 
     shared_examples 'does not inherit settings from integration' do
       it 'does not update the not inherited integrations' do
-        described_class.propagate(integration: instance_integration, update_only_inherited: update_only_inherited)
+        described_class.propagate(integration: instance_integration, overwrite: overwrite)
 
         expect(integration.reload.attributes.except(*excluded_attributes))
           .not_to eq(instance_integration.attributes.except(*excluded_attributes))
@@ -82,7 +82,7 @@
     end
 
     context 'update only inherited integrations' do
-      let(:update_only_inherited) { true }
+      let(:overwrite) { false }
 
       it_behaves_like 'inherits settings from integration' do
         let(:integration) { inherited_integration }
@@ -102,7 +102,7 @@
     end
 
     context 'update all integrations' do
-      let(:update_only_inherited) { false }
+      let(:overwrite) { true }
 
       it_behaves_like 'inherits settings from integration' do
         let(:integration) { inherited_integration }
diff --git a/spec/workers/propagate_integration_worker_spec.rb b/spec/workers/propagate_integration_worker_spec.rb
index 76b1b478ef4eac..e49869a38e90bb 100644
--- a/spec/workers/propagate_integration_worker_spec.rb
+++ b/spec/workers/propagate_integration_worker_spec.rb
@@ -18,7 +18,7 @@
 
     it 'calls the propagate service with the integration' do
       expect(Admin::PropagateIntegrationService).to receive(:propagate)
-        .with(integration: integration, update_only_inherited: true)
+        .with(integration: integration, overwrite: true)
 
       subject.perform(integration.id, true)
     end
-- 
GitLab


From d56b58481b9f72d1ad5cc996961abe3111cdc718 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Mon, 18 May 2020 17:30:47 +0100
Subject: [PATCH 3/7] Propagation: Spec to cover project updates

This covers: project#has_external_wiki and project#has_external_wiki
---
 .../propagate_integration_service_spec.rb     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb
index 9a7f7dd6a3f2cf..3bbdf9d87bba19 100644
--- a/spec/services/admin/propagate_integration_service_spec.rb
+++ b/spec/services/admin/propagate_integration_service_spec.rb
@@ -120,5 +120,24 @@
         let(:integration) { project.jira_service }
       end
     end
+
+    it 'updates project#has_external_issue_tracker for issue tracker services' do
+      described_class.propagate(integration: instance_integration, overwrite: true)
+
+      expect(project.reload.has_external_issue_tracker).to eq(true)
+    end
+
+    it 'updates project#has_external_wiki for external wiki services' do
+      instance_integration = ExternalWikiService.create!(
+        instance: true,
+        active: true,
+        push_events: false,
+        external_wiki_url: 'http://external-wiki-url.com'
+      )
+
+      described_class.propagate(integration: instance_integration, overwrite: true)
+
+      expect(project.reload.has_external_wiki).to eq(true)
+    end
   end
 end
-- 
GitLab


From 6339be98be3b0696c4171143e02025e5678c50bb Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Tue, 19 May 2020 12:45:15 +0100
Subject: [PATCH 4/7] Extract spec context for better test phases

---
 .../admin/propagate_integration_service_spec.rb    | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb
index 3bbdf9d87bba19..843b78a41e9d17 100644
--- a/spec/services/admin/propagate_integration_service_spec.rb
+++ b/spec/services/admin/propagate_integration_service_spec.rb
@@ -62,13 +62,19 @@
         described_class.propagate(integration: instance_integration, overwrite: overwrite)
 
         expect(integration.reload.inherit_from_id).to eq(instance_integration.id)
-
         expect(integration.attributes.except(*excluded_attributes))
           .to eq(instance_integration.attributes.except(*excluded_attributes))
+      end
+
+      context 'integration with data fields' do
+        let(:excluded_attributes) { %w[id service_id created_at updated_at] }
+
+        it 'updates the data fields from inherited integrations' do
+          described_class.propagate(integration: instance_integration, overwrite: overwrite)
 
-        excluded_attributes = %w[id service_id created_at updated_at]
-        expect(integration.data_fields.attributes.except(*excluded_attributes))
-          .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes))
+          expect(integration.reload.data_fields.attributes.except(*excluded_attributes))
+            .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes))
+        end
       end
     end
 
-- 
GitLab


From caa492ab12bc627ceefe4a063b74d301be540fc7 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Tue, 19 May 2020 11:37:44 +0100
Subject: [PATCH 5/7] Service: Extract service hash and data_fields hash

---
 app/models/service.rb                               |  8 ++++++++
 app/services/admin/propagate_integration_service.rb | 13 ++++++-------
 app/services/projects/propagate_service_template.rb | 10 +++++-----
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/app/models/service.rb b/app/models/service.rb
index fb4d9a77077b76..58bae93267a6aa 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -134,6 +134,14 @@ def json_fields
     %w(active)
   end
 
+  def to_service_hash
+    as_json(methods: :type, except: %w[id template instance project_id])
+  end
+
+  def to_data_fields_hash
+    data_fields.as_json(only: data_fields.class.column_names).except('id', 'service_id')
+  end
+
   def test_data(project, user)
     Gitlab::DataBuilder::Push.build_sample(project, user)
   end
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb
index 768f889924e83a..5d72350868f7e0 100644
--- a/app/services/admin/propagate_integration_service.rb
+++ b/app/services/admin/propagate_integration_service.rb
@@ -56,7 +56,7 @@ def bulk_update_from_integration(batch)
         batch.update_all(service_hash)
 
         if data_fields_present?
-          integration.data_fields.class.where(service_id: batch_ids).update_all(data_hash)
+          integration.data_fields.class.where(service_id: batch_ids).update_all(data_fields_hash)
         end
       end
     end
@@ -81,9 +81,9 @@ def bulk_create_from_integration(batch)
         results = bulk_insert(Service, service_hash.keys << 'project_id' << 'inherit_from_id', service_list)
 
         if data_fields_present?
-          data_list = results.map { |row| data_hash.values << row['id'] }
+          data_list = results.map { |row| data_fields_hash.values << row['id'] }
 
-          bulk_insert(integration.data_fields.class, data_hash.keys << 'service_id', data_list)
+          bulk_insert(integration.data_fields.class, data_fields_hash.keys << 'service_id', data_list)
         end
 
         run_callbacks(batch)
@@ -135,13 +135,12 @@ def project_ids_without_integration
     end
 
     def service_hash
-      @service_hash ||= integration
-        .as_json(methods: :type, except: %w[id instance project_id])
+      @service_hash ||= integration.to_service_hash
         .tap { |json| json['inherit_from_id'] = integration.id }
     end
 
-    def data_hash
-      @data_hash ||= integration.data_fields.as_json(only: integration.data_fields.class.column_names).except('id', 'service_id')
+    def data_fields_hash
+      @data_fields_hash ||= integration.to_data_fields_hash
     end
   end
 end
diff --git a/app/services/projects/propagate_service_template.rb b/app/services/projects/propagate_service_template.rb
index 0483c951f1e07d..fb79174ad1137b 100644
--- a/app/services/projects/propagate_service_template.rb
+++ b/app/services/projects/propagate_service_template.rb
@@ -43,9 +43,9 @@ def bulk_create_from_template(batch)
         results = bulk_insert(Service, service_hash.keys << 'project_id', service_list)
 
         if data_fields_present?
-          data_list = results.map { |row| data_hash.values << row['id'] }
+          data_list = results.map { |row| data_fields_hash.values << row['id'] }
 
-          bulk_insert(template.data_fields.class, data_hash.keys << 'service_id', data_list)
+          bulk_insert(template.data_fields.class, data_fields_hash.keys << 'service_id', data_list)
         end
 
         run_callbacks(batch)
@@ -77,11 +77,11 @@ def bulk_insert(klass, columns, values_array)
     end
 
     def service_hash
-      @service_hash ||= template.as_json(methods: :type, except: %w[id template project_id])
+      @service_hash ||= template.to_service_hash
     end
 
-    def data_hash
-      @data_hash ||= template.data_fields.as_json(only: template.data_fields.class.column_names).except('id', 'service_id')
+    def data_fields_hash
+      @data_fields_hash ||= template.to_data_fields_hash
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-- 
GitLab


From c49ef6fb3a675cee9c4cb7b5f3487608863f02df Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Tue, 19 May 2020 12:31:53 +0100
Subject: [PATCH 6/7] Extract ServiceList and DataList classes

This creates service objects to encapsulate the logic.
---
 app/models/data_list.rb                       | 28 +++++++++++++++++
 app/models/service_list.rb                    | 31 +++++++++++++++++++
 .../admin/propagate_integration_service.rb    | 10 +++---
 3 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100644 app/models/data_list.rb
 create mode 100644 app/models/service_list.rb

diff --git a/app/models/data_list.rb b/app/models/data_list.rb
new file mode 100644
index 00000000000000..d708606a350226
--- /dev/null
+++ b/app/models/data_list.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class DataList
+  def initialize(batch, integration)
+    @batch = batch
+    @integration = integration
+  end
+
+  def to_array
+    [integration.data_fields.class, columns, values]
+  end
+
+  private
+
+  attr_reader :batch, :integration
+
+  def columns
+    data_fields_hash.keys << 'service_id'
+  end
+
+  def values
+    batch.map { |row| data_fields_hash.values << row['id'] }
+  end
+
+  def data_fields_hash
+    @data_fields_hash ||= integration.to_data_fields_hash
+  end
+end
diff --git a/app/models/service_list.rb b/app/models/service_list.rb
new file mode 100644
index 00000000000000..c965c0b7772d0e
--- /dev/null
+++ b/app/models/service_list.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class ServiceList
+  def initialize(batch, integration)
+    @batch = batch
+    @integration = integration
+  end
+
+  def to_array
+    [Service, columns, values]
+  end
+
+  private
+
+  attr_reader :batch, :integration
+
+  def columns
+    service_hash.keys << 'project_id' << 'inherit_from_id'
+  end
+
+  def values
+    batch.map do |project_id|
+      service_hash.values << project_id << integration.id
+    end
+  end
+
+  def service_hash
+    @service_hash ||= integration.to_service_hash
+      .tap { |json| json['inherit_from_id'] = integration.id }
+  end
+end
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb
index 5d72350868f7e0..378950ecd9f831 100644
--- a/app/services/admin/propagate_integration_service.rb
+++ b/app/services/admin/propagate_integration_service.rb
@@ -73,17 +73,15 @@ def create_integration_for_projects_without_integration
     end
 
     def bulk_create_from_integration(batch)
-      service_list = batch.map do |project_id|
-        service_hash.values << project_id << integration.id
-      end
+      service_list = ServiceList.new(batch, integration).to_array
 
       Project.transaction do
-        results = bulk_insert(Service, service_hash.keys << 'project_id' << 'inherit_from_id', service_list)
+        results = bulk_insert(*service_list)
 
         if data_fields_present?
-          data_list = results.map { |row| data_fields_hash.values << row['id'] }
+          data_list = DataList.new(results, integration).to_array
 
-          bulk_insert(integration.data_fields.class, data_fields_hash.keys << 'service_id', data_list)
+          bulk_insert(*data_list)
         end
 
         run_callbacks(batch)
-- 
GitLab


From 39f7d9c3858ef59d491fc297fa634840ec8aa9ff Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Wed, 20 May 2020 09:19:50 +0100
Subject: [PATCH 7/7] Reuse ServiceList and DataList

This allows ServiceList and DataList to work with
PropagateServiceTemplate. It also passes service_hash and
data_fields_hash as parameters avoiding an extra call when propagating
the settings.
---
 app/models/data_list.rb                          | 13 +++++--------
 app/models/service_list.rb                       | 16 ++++++----------
 .../admin/propagate_integration_service.rb       |  4 ++--
 .../projects/propagate_service_template.rb       | 10 ++++------
 4 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/app/models/data_list.rb b/app/models/data_list.rb
index d708606a350226..12011cb17f718b 100644
--- a/app/models/data_list.rb
+++ b/app/models/data_list.rb
@@ -1,18 +1,19 @@
 # frozen_string_literal: true
 
 class DataList
-  def initialize(batch, integration)
+  def initialize(batch, data_fields_hash, klass)
     @batch = batch
-    @integration = integration
+    @data_fields_hash = data_fields_hash
+    @klass = klass
   end
 
   def to_array
-    [integration.data_fields.class, columns, values]
+    [klass, columns, values]
   end
 
   private
 
-  attr_reader :batch, :integration
+  attr_reader :batch, :data_fields_hash, :klass
 
   def columns
     data_fields_hash.keys << 'service_id'
@@ -21,8 +22,4 @@ def columns
   def values
     batch.map { |row| data_fields_hash.values << row['id'] }
   end
-
-  def data_fields_hash
-    @data_fields_hash ||= integration.to_data_fields_hash
-  end
 end
diff --git a/app/models/service_list.rb b/app/models/service_list.rb
index c965c0b7772d0e..fa3760f0c561aa 100644
--- a/app/models/service_list.rb
+++ b/app/models/service_list.rb
@@ -1,9 +1,10 @@
 # frozen_string_literal: true
 
 class ServiceList
-  def initialize(batch, integration)
+  def initialize(batch, service_hash, extra_hash = {})
     @batch = batch
-    @integration = integration
+    @service_hash = service_hash
+    @extra_hash = extra_hash
   end
 
   def to_array
@@ -12,20 +13,15 @@ def to_array
 
   private
 
-  attr_reader :batch, :integration
+  attr_reader :batch, :service_hash, :extra_hash
 
   def columns
-    service_hash.keys << 'project_id' << 'inherit_from_id'
+    (service_hash.keys << 'project_id') + extra_hash.keys
   end
 
   def values
     batch.map do |project_id|
-      service_hash.values << project_id << integration.id
+      (service_hash.values << project_id) + extra_hash.values
     end
   end
-
-  def service_hash
-    @service_hash ||= integration.to_service_hash
-      .tap { |json| json['inherit_from_id'] = integration.id }
-  end
 end
diff --git a/app/services/admin/propagate_integration_service.rb b/app/services/admin/propagate_integration_service.rb
index 378950ecd9f831..0a3c61816f8534 100644
--- a/app/services/admin/propagate_integration_service.rb
+++ b/app/services/admin/propagate_integration_service.rb
@@ -73,13 +73,13 @@ def create_integration_for_projects_without_integration
     end
 
     def bulk_create_from_integration(batch)
-      service_list = ServiceList.new(batch, integration).to_array
+      service_list = ServiceList.new(batch, service_hash, { 'inherit_from_id' => integration.id }).to_array
 
       Project.transaction do
         results = bulk_insert(*service_list)
 
         if data_fields_present?
-          data_list = DataList.new(results, integration).to_array
+          data_list = DataList.new(results, data_fields_hash, integration.data_fields.class).to_array
 
           bulk_insert(*data_list)
         end
diff --git a/app/services/projects/propagate_service_template.rb b/app/services/projects/propagate_service_template.rb
index fb79174ad1137b..ecca971594014f 100644
--- a/app/services/projects/propagate_service_template.rb
+++ b/app/services/projects/propagate_service_template.rb
@@ -35,17 +35,15 @@ def propagate_projects_with_template
     end
 
     def bulk_create_from_template(batch)
-      service_list = batch.map do |project_id|
-        service_hash.values << project_id
-      end
+      service_list = ServiceList.new(batch, service_hash).to_array
 
       Project.transaction do
-        results = bulk_insert(Service, service_hash.keys << 'project_id', service_list)
+        results = bulk_insert(*service_list)
 
         if data_fields_present?
-          data_list = results.map { |row| data_fields_hash.values << row['id'] }
+          data_list = DataList.new(results, data_fields_hash, template.data_fields.class).to_array
 
-          bulk_insert(template.data_fields.class, data_fields_hash.keys << 'service_id', data_list)
+          bulk_insert(*data_list)
         end
 
         run_callbacks(batch)
-- 
GitLab