From 74d471cc6ca2effd20273a1a54cd8c5a118c53ed Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Wed, 19 Aug 2020 17:07:02 +0100
Subject: [PATCH 1/6] Extract #operating spec from scopes describe block

---
 spec/models/service_spec.rb | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index d001b6ec0f129c..e60863420d4ec7 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -91,20 +91,6 @@
       end
     end
 
-    describe '#operating?' do
-      it 'is false when the service is not active' do
-        expect(build(:service).operating?).to eq(false)
-      end
-
-      it 'is false when the service is not persisted' do
-        expect(build(:service, active: true).operating?).to eq(false)
-      end
-
-      it 'is true when the service is active and persisted' do
-        expect(create(:service, active: true).operating?).to eq(true)
-      end
-    end
-
     describe '.confidential_note_hooks' do
       it 'includes services where confidential_note_events is true' do
         create(:service, active: true, confidential_note_events: true)
@@ -134,6 +120,20 @@
     end
   end
 
+  describe '#operating?' do
+    it 'is false when the service is not active' do
+      expect(build(:service).operating?).to eq(false)
+    end
+
+    it 'is false when the service is not persisted' do
+      expect(build(:service, active: true).operating?).to eq(false)
+    end
+
+    it 'is true when the service is active and persisted' do
+      expect(create(:service, active: true).operating?).to eq(true)
+    end
+  end
+
   describe "Test Button" do
     describe '#can_test?' do
       subject { service.can_test? }
-- 
GitLab


From 7172fef9973dfeaa008e18665a7fece7f5c2788e Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Wed, 19 Aug 2020 17:02:43 +0100
Subject: [PATCH 2/6] Complete CRUD for group-level integrations

After adding the CRUD for group-level integrations
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27557, we were
missing the group_id column
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38499, so an
integration cannot belong to a group.

This completes the work, having now group-level integrations.
---
 .../settings/integrations_controller.rb       |  8 +--
 app/models/service.rb                         | 11 ++--
 .../settings/integrations_controller_spec.rb  |  3 +-
 spec/models/service_spec.rb                   | 53 +++++++++++++++++++
 4 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/app/controllers/groups/settings/integrations_controller.rb b/app/controllers/groups/settings/integrations_controller.rb
index adfbe9bfa17f61..b0cc20026ab57e 100644
--- a/app/controllers/groups/settings/integrations_controller.rb
+++ b/app/controllers/groups/settings/integrations_controller.rb
@@ -8,15 +8,15 @@ class IntegrationsController < Groups::ApplicationController
       before_action :authorize_admin_group!
 
       def index
-        @integrations = []
+        @integrations = Service.find_or_initialize_by_group(group).sort_by(&:title)
       end
 
       private
 
-      # TODO: Make this compatible with group-level integration
-      # https://gitlab.com/groups/gitlab-org/-/epics/2543
       def find_or_initialize_integration(name)
-        Project.first.find_or_initialize_service(name)
+        if name.in?(Service.available_services_names)
+          "#{name}_service".camelize.constantize.find_or_initialize_by(group_id: group.id) # rubocop:disable CodeReuse/ActiveRecord
+        end
       end
 
       def integrations_enabled?
diff --git a/app/models/service.rb b/app/models/service.rb
index 40e7e5552d1757..81739275206a6a 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -64,6 +64,7 @@ class Service < ApplicationRecord
   scope :active, -> { where(active: true) }
   scope :by_type, -> (type) { where(type: type) }
   scope :by_active_flag, -> (flag) { where(active: flag) }
+  scope :by_group, -> (group) { where(group_id: group, type: available_services_types) }
   scope :templates, -> { where(template: true, type: available_services_types) }
   scope :instances, -> { where(instance: true, type: available_services_types) }
 
@@ -307,11 +308,15 @@ def self.find_or_create_templates
   end
 
   def self.find_or_initialize_instances
-    instances + build_nonexistent_instances
+    instances + build_nonexistent_services_for(instances)
   end
 
-  private_class_method def self.build_nonexistent_instances
-    list_nonexistent_services_for(instances).map do |service_type|
+  def self.find_or_initialize_by_group(group)
+    by_group(group) + build_nonexistent_services_for(by_group(group))
+  end
+
+  private_class_method def self.build_nonexistent_services_for(existing_services)
+    list_nonexistent_services_for(existing_services).map do |service_type|
       service_type.constantize.new
     end
   end
diff --git a/spec/controllers/groups/settings/integrations_controller_spec.rb b/spec/controllers/groups/settings/integrations_controller_spec.rb
index d079f3f077eaca..b0ea76e5523b3e 100644
--- a/spec/controllers/groups/settings/integrations_controller_spec.rb
+++ b/spec/controllers/groups/settings/integrations_controller_spec.rb
@@ -3,7 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe Groups::Settings::IntegrationsController do
-  let_it_be(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:group) { create(:group) }
 
@@ -82,7 +81,7 @@
   end
 
   describe '#update' do
-    let(:integration) { create(:jira_service, project: project) }
+    let(:integration) { create(:jira_service, project: nil, group_id: group.id) }
 
     before do
       group.add_owner(user)
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index e60863420d4ec7..91997e3864890d 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -91,6 +91,16 @@
       end
     end
 
+    describe '.by_group' do
+      let(:group) { create(:group) }
+      let!(:service1) { create(:jira_service, project_id: nil, group_id: group.id) }
+      let!(:service2) { create(:jira_service) }
+
+      it 'returns the right group service' do
+        expect(described_class.by_group(group)).to match_array([service1])
+      end
+    end
+
     describe '.confidential_note_hooks' do
       it 'includes services where confidential_note_events is true' do
         create(:service, active: true, confidential_note_events: true)
@@ -230,6 +240,49 @@
     end
   end
 
+  describe '.find_or_initialize_by_group' do
+    let(:group) { create(:group) }
+
+    shared_examples 'group services' do
+      it 'returns the available group services' do
+        expect(Service.find_or_initialize_by_group(group).pluck(:type)).to match_array(Service.available_services_types)
+      end
+
+      it 'does not create service group services' do
+        expect { Service.find_or_initialize_by_group(group) }.not_to change { Service.count }
+      end
+    end
+
+    it_behaves_like 'group services'
+
+    context 'with all existing group integrations' do
+      before do
+        Service.insert_all(
+          Service.available_services_types.map { |type| { group_id: group.id, type: type } }
+        )
+      end
+
+      it_behaves_like 'group services'
+
+      context 'with a previous existing service (Previous) and a new service (Asana)' do
+        before do
+          Service.insert(type: 'PreviousService', group_id: group.id)
+          Service.delete_by(type: 'AsanaService', group_id: group.id)
+        end
+
+        it_behaves_like 'group services'
+      end
+    end
+
+    context 'with an existing group service' do
+      before do
+        create(:jira_service, project_id: nil, group_id: group.id)
+      end
+
+      it_behaves_like 'group services'
+    end
+  end
+
   describe 'template' do
     let(:project) { create(:project) }
 
-- 
GitLab


From 7588782d947923880011036c0525229d911ce650 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Thu, 20 Aug 2020 11:15:09 +0100
Subject: [PATCH 3/6] Extract find_or_initialize_integration method

---
 app/controllers/admin/integrations_controller.rb   |  4 +---
 .../groups/settings/integrations_controller.rb     |  4 +---
 app/models/service.rb                              |  6 ++++++
 spec/models/service_spec.rb                        | 14 ++++++++++++++
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/app/controllers/admin/integrations_controller.rb b/app/controllers/admin/integrations_controller.rb
index b2d5a2d130c7f8..1e2a99f7078be2 100644
--- a/app/controllers/admin/integrations_controller.rb
+++ b/app/controllers/admin/integrations_controller.rb
@@ -6,9 +6,7 @@ class Admin::IntegrationsController < Admin::ApplicationController
   private
 
   def find_or_initialize_integration(name)
-    if name.in?(Service.available_services_names)
-      "#{name}_service".camelize.constantize.find_or_initialize_by(instance: true) # rubocop:disable CodeReuse/ActiveRecord
-    end
+    Service.find_or_initialize_integration(name, instance: true)
   end
 
   def integrations_enabled?
diff --git a/app/controllers/groups/settings/integrations_controller.rb b/app/controllers/groups/settings/integrations_controller.rb
index b0cc20026ab57e..44d97bc984e32b 100644
--- a/app/controllers/groups/settings/integrations_controller.rb
+++ b/app/controllers/groups/settings/integrations_controller.rb
@@ -14,9 +14,7 @@ def index
       private
 
       def find_or_initialize_integration(name)
-        if name.in?(Service.available_services_names)
-          "#{name}_service".camelize.constantize.find_or_initialize_by(group_id: group.id) # rubocop:disable CodeReuse/ActiveRecord
-        end
+        Service.find_or_initialize_integration(name, group_id: group.id)
       end
 
       def integrations_enabled?
diff --git a/app/models/service.rb b/app/models/service.rb
index 81739275206a6a..76a8aa6525da48 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -307,6 +307,12 @@ def self.find_or_create_templates
     end
   end
 
+  def self.find_or_initialize_integration(name, scope)
+    if name.in?(available_services_names)
+      "#{name}_service".camelize.constantize.find_or_initialize_by(scope)
+    end
+  end
+
   def self.find_or_initialize_instances
     instances + build_nonexistent_services_for(instances)
   end
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 91997e3864890d..39c213d3268fb0 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -199,6 +199,20 @@
     end
   end
 
+  describe '.find_or_initialize_integration' do
+    let(:group) { create(:group) }
+    let!(:service1) { create(:jira_service, project_id: nil, group_id: group.id) }
+    let!(:service2) { create(:jira_service) }
+
+    it 'returns the right service' do
+      expect(Service.find_or_initialize_integration('jira', group_id: group)).to eq(service1)
+    end
+
+    it 'does not create a new service' do
+      expect { Service.find_or_initialize_integration('redmine', group_id: group) }.not_to change { Service.count }
+    end
+  end
+
   describe '.find_or_initialize_instances' do
     shared_examples 'service instances' do
       it 'returns the available service instances' do
-- 
GitLab


From 1bc0fa50faf90f3b87015a41072850da8381d5f4 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Thu, 20 Aug 2020 18:18:00 +0100
Subject: [PATCH 4/6] Use let_it_be to reduce queries in service spec

Total number of queries saved: 4882 - 4774 = 108
---
 spec/models/service_spec.rb | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 39c213d3268fb0..9d6ac2d61e6d05 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Service do
+  let_it_be(:group) { create(:group) }
+
   describe "Associations" do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -13,7 +15,6 @@
   describe 'validations' do
     using RSpec::Parameterized::TableSyntax
 
-    let(:group) { create(:group) }
     let(:project) { create(:project) }
 
     it { is_expected.to validate_presence_of(:type) }
@@ -92,7 +93,6 @@
     end
 
     describe '.by_group' do
-      let(:group) { create(:group) }
       let!(:service1) { create(:jira_service, project_id: nil, group_id: group.id) }
       let!(:service2) { create(:jira_service) }
 
@@ -200,7 +200,6 @@
   end
 
   describe '.find_or_initialize_integration' do
-    let(:group) { create(:group) }
     let!(:service1) { create(:jira_service, project_id: nil, group_id: group.id) }
     let!(:service2) { create(:jira_service) }
 
@@ -255,8 +254,6 @@
   end
 
   describe '.find_or_initialize_by_group' do
-    let(:group) { create(:group) }
-
     shared_examples 'group services' do
       it 'returns the available group services' do
         expect(Service.find_or_initialize_by_group(group).pluck(:type)).to match_array(Service.available_services_types)
-- 
GitLab


From 062976f0fa2af6fc1d16529191778e843d2c7759 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Mon, 24 Aug 2020 12:15:01 +0100
Subject: [PATCH 5/6] Specify valid scopes

---
 app/models/service.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/service.rb b/app/models/service.rb
index 76a8aa6525da48..7440906143e257 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -307,9 +307,9 @@ def self.find_or_create_templates
     end
   end
 
-  def self.find_or_initialize_integration(name, scope)
+  def self.find_or_initialize_integration(name, instance: false, group_id: nil)
     if name.in?(available_services_names)
-      "#{name}_service".camelize.constantize.find_or_initialize_by(scope)
+      "#{name}_service".camelize.constantize.find_or_initialize_by(instance: instance, group_id: group_id)
     end
   end
 
-- 
GitLab


From 0415197708993c60b6312a745dfe565be2476429 Mon Sep 17 00:00:00 2001
From: Arturo Herrero <arturo.herrero@gmail.com>
Date: Mon, 24 Aug 2020 13:49:20 +0100
Subject: [PATCH 6/6] Extract find_or_initialize_all method

There is some duplication between find_or_initialize_instances and
find_or_initialize_by_group.
---
 .../admin/application_settings_controller.rb  |  2 +-
 .../settings/integrations_controller.rb       |  2 +-
 app/models/service.rb                         | 12 ++---
 spec/models/service_spec.rb                   | 51 ++-----------------
 4 files changed, 11 insertions(+), 56 deletions(-)

diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index 3a5b8b2862e1a2..7e92940ff309b2 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -32,7 +32,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   end
 
   def integrations
-    @integrations = Service.find_or_initialize_instances.sort_by(&:title)
+    @integrations = Service.find_or_initialize_all(Service.instances).sort_by(&:title)
   end
 
   def update
diff --git a/app/controllers/groups/settings/integrations_controller.rb b/app/controllers/groups/settings/integrations_controller.rb
index 44d97bc984e32b..844b19a43434ce 100644
--- a/app/controllers/groups/settings/integrations_controller.rb
+++ b/app/controllers/groups/settings/integrations_controller.rb
@@ -8,7 +8,7 @@ class IntegrationsController < Groups::ApplicationController
       before_action :authorize_admin_group!
 
       def index
-        @integrations = Service.find_or_initialize_by_group(group).sort_by(&:title)
+        @integrations = Service.find_or_initialize_all(Service.by_group(group)).sort_by(&:title)
       end
 
       private
diff --git a/app/models/service.rb b/app/models/service.rb
index 7440906143e257..bfa8745d8992a1 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -313,16 +313,12 @@ def self.find_or_initialize_integration(name, instance: false, group_id: nil)
     end
   end
 
-  def self.find_or_initialize_instances
-    instances + build_nonexistent_services_for(instances)
+  def self.find_or_initialize_all(scope)
+    scope + build_nonexistent_services_for(scope)
   end
 
-  def self.find_or_initialize_by_group(group)
-    by_group(group) + build_nonexistent_services_for(by_group(group))
-  end
-
-  private_class_method def self.build_nonexistent_services_for(existing_services)
-    list_nonexistent_services_for(existing_services).map do |service_type|
+  private_class_method def self.build_nonexistent_services_for(scope)
+    list_nonexistent_services_for(scope).map do |service_type|
       service_type.constantize.new
     end
   end
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 9d6ac2d61e6d05..6dddcb0eca4d50 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -212,14 +212,14 @@
     end
   end
 
-  describe '.find_or_initialize_instances' do
+  describe '.find_or_initialize_all' do
     shared_examples 'service instances' do
       it 'returns the available service instances' do
-        expect(Service.find_or_initialize_instances.pluck(:type)).to match_array(Service.available_services_types)
+        expect(Service.find_or_initialize_all(Service.instances).pluck(:type)).to match_array(Service.available_services_types)
       end
 
       it 'does not create service instances' do
-        expect { Service.find_or_initialize_instances }.not_to change { Service.count }
+        expect { Service.find_or_initialize_all(Service.instances) }.not_to change { Service.count }
       end
     end
 
@@ -234,9 +234,9 @@
 
       it_behaves_like 'service instances'
 
-      context 'with a previous existing service (Previous) and a new service (Asana)' do
+      context 'with a previous existing service (MockCiService) and a new service (Asana)' do
         before do
-          Service.insert(type: 'PreviousService', instance: true)
+          Service.insert(type: 'MockCiService', instance: true)
           Service.delete_by(type: 'AsanaService', instance: true)
         end
 
@@ -253,47 +253,6 @@
     end
   end
 
-  describe '.find_or_initialize_by_group' do
-    shared_examples 'group services' do
-      it 'returns the available group services' do
-        expect(Service.find_or_initialize_by_group(group).pluck(:type)).to match_array(Service.available_services_types)
-      end
-
-      it 'does not create service group services' do
-        expect { Service.find_or_initialize_by_group(group) }.not_to change { Service.count }
-      end
-    end
-
-    it_behaves_like 'group services'
-
-    context 'with all existing group integrations' do
-      before do
-        Service.insert_all(
-          Service.available_services_types.map { |type| { group_id: group.id, type: type } }
-        )
-      end
-
-      it_behaves_like 'group services'
-
-      context 'with a previous existing service (Previous) and a new service (Asana)' do
-        before do
-          Service.insert(type: 'PreviousService', group_id: group.id)
-          Service.delete_by(type: 'AsanaService', group_id: group.id)
-        end
-
-        it_behaves_like 'group services'
-      end
-    end
-
-    context 'with an existing group service' do
-      before do
-        create(:jira_service, project_id: nil, group_id: group.id)
-      end
-
-      it_behaves_like 'group services'
-    end
-  end
-
   describe 'template' do
     let(:project) { create(:project) }
 
-- 
GitLab