Merge branch 'adam-build-missing-services-when-necessary' into 'master'

Defer saving project services to the database if there are no user changes

## What does this MR do?

It defers saving project services to the database as long as it is possible. It creates a project service when creating a project only if this project service has an active template. After that project services are saved on the first edit.

## Are there points in the code the reviewer needs to double check?

- tests that used `build_missing_services` before the change
- number of queries executed

## Why was this MR needed?

Motivation in #22281

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes #22281

See merge request !6958

11 files changed, 49 insertions, 54 deletions

diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index 97e6e9471e0..40a23a6f806 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -10,8 +10,7 @@ class Projects::ServicesController < Projects::ApplicationController
   layout "project_settings"
 
   def index
-    @project.build_missing_services
-    @services = @project.services.visible.reload
+    @services = @project.find_or_initialize_services
   end
 
   def edit
@@ -46,6 +45,6 @@ class Projects::ServicesController < Projects::ApplicationController
   private
 
   def service
-    @service ||= @project.services.find { |service| service.to_param == params[:id] }
+    @service ||= @project.find_or_initialize_service(params[:id])
   end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 327f81412f8..4aedc91dc34 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -77,7 +77,6 @@ class Project < ActiveRecord::Base
   has_many :boards, before_add: :validate_board_limit, dependent: :destroy
 
   # Project services
-  has_many :services
   has_one :campfire_service, dependent: :destroy
   has_one :drone_ci_service, dependent: :destroy
   has_one :emails_on_push_service, dependent: :destroy
@@ -749,27 +748,32 @@ class Project < ActiveRecord::Base
     update_column(:has_external_wiki, services.external_wikis.any?)
   end
 
-  def build_missing_services
+  def find_or_initialize_services
     services_templates = Service.where(template: true)
 
-    Service.available_services_names.each do |service_name|
+    Service.available_services_names.map do |service_name|
       service = find_service(services, service_name)
 
-      # If service is available but missing in db
-      if service.nil?
+      if service
+        service
+      else
         # We should check if template for the service exists
         template = find_service(services_templates, service_name)
 
         if template.nil?
-          # If no template, we should create an instance. Ex `create_gitlab_ci_service`
-          public_send("create_#{service_name}_service")
+          # If no template, we should create an instance. Ex `build_gitlab_ci_service`
+          public_send("build_#{service_name}_service")
         else
-          Service.create_from_template(self.id, template)
+          Service.build_from_template(id, template)
         end
       end
     end
   end
 
+  def find_or_initialize_service(name)
+    find_or_initialize_services.find { |service| service.to_param == name }
+  end
+
   def create_labels
     Label.templates.each do |label|
       params = label.attributes.except('id', 'template', 'created_at', 'updated_at')
diff --git a/app/models/service.rb b/app/models/service.rb
index 625fbc48302..9d6ff190cdf 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -222,11 +222,11 @@ class Service < ActiveRecord::Base
     ]
   end
 
-  def self.create_from_template(project_id, template)
+  def self.build_from_template(project_id, template)
     service = template.dup
     service.template = false
     service.project_id = project_id
-    service if service.save
+    service
   end
 
   private
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 15d7918e7fd..28db145a1f4 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -95,7 +95,7 @@ module Projects
 
       unless @project.gitlab_project_import?
         @project.create_wiki unless skip_wiki?
-        @project.build_missing_services
+        create_services_from_active_templates(@project)
 
         @project.create_labels
       end
@@ -135,5 +135,12 @@ module Projects
 
       @project
     end
+
+    def create_services_from_active_templates(project)
+      Service.where(template: true, active: true).each do |template|
+        service = Service.build_from_template(project.id, template)
+        service.save!
+      end
+    end
   end
 end
diff --git a/app/views/projects/services/index.html.haml b/app/views/projects/services/index.html.haml
index 4a33a5bc6f6..66fd3029dc9 100644
--- a/app/views/projects/services/index.html.haml
+++ b/app/views/projects/services/index.html.haml
@@ -28,5 +28,6 @@
           %td.hidden-xs
             = service.description
           %td.light
-            = time_ago_in_words service.updated_at
-            ago
+            - if service.updated_at.present?
+              = time_ago_in_words service.updated_at
+              ago
diff --git a/changelogs/unreleased/adam-build-missing-services-when-necessary.yml b/changelogs/unreleased/adam-build-missing-services-when-necessary.yml
new file mode 100644
index 00000000000..8b157e31e99
--- /dev/null
+++ b/changelogs/unreleased/adam-build-missing-services-when-necessary.yml
@@ -0,0 +1,4 @@
+---
+title: Defer saving project services to the database if there are no user changes
+merge_request: 6958
+author:
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 6998b6dc039..84cc9200d1b 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -86,25 +86,10 @@ module API
     end
 
     def project_service
-      @project_service ||= begin
-        underscored_service = params[:service_slug].underscore
-
-        if Service.available_services_names.include?(underscored_service)
-          user_project.build_missing_services
-
-          service_method = "#{underscored_service}_service"
-
-          send_service(service_method)
-        end
-      end
-
+      @project_service ||= user_project.find_or_initialize_service(params[:service_slug].underscore)
       @project_service || not_found!("Service")
     end
 
-    def send_service(service_method)
-      user_project.send(service_method)
-    end
-
     def service_attributes
       @service_attributes ||= project_service.fields.inject([]) do |arr, hash|
         arr << hash[:name].to_sym
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3a0c3ec21cd..c74d9c282cf 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -496,9 +496,6 @@ describe Project, models: true do
     end
 
     it 'returns nil and does not query services when there is no external issue tracker' do
-      project.build_missing_services
-      project.reload
-
       expect(project).not_to receive(:services)
 
       expect(project.external_issue_tracker).to eq(nil)
@@ -506,9 +503,6 @@ describe Project, models: true do
 
     it 'retrieves external_issue_tracker querying services and cache it when there is external issue tracker' do
       ext_project.reload # Factory returns a project with changed attributes
-      ext_project.build_missing_services
-      ext_project.reload
-
       expect(ext_project).to receive(:services).once.and_call_original
 
       2.times { expect(ext_project.external_issue_tracker).to be_a_kind_of(RedmineService) }
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 43937a54b2c..b1615a95004 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -53,7 +53,7 @@ describe Service, models: true do
 
   describe "Template" do
     describe "for pushover service" do
-      let(:service_template) do
+      let!(:service_template) do
         PushoverService.create(
           template: true,
           properties: {
@@ -66,13 +66,9 @@ describe Service, models: true do
       let(:project) { create(:project) }
 
       describe 'is prefilled for projects pushover service' do
-        before do
-          service_template
-          project.build_missing_services
-        end
-
         it "has all fields prefilled" do
-          service = project.pushover_service
+          service = project.find_or_initialize_service('pushover')
+
           expect(service.template).to eq(false)
           expect(service.device).to eq('MyDevice')
           expect(service.sound).to eq('mic')
diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb
index 375671bca4c..2aadab3cbe1 100644
--- a/spec/requests/api/services_spec.rb
+++ b/spec/requests/api/services_spec.rb
@@ -56,8 +56,7 @@ describe API::API, api: true  do
 
       # inject some properties into the service
       before do
-        project.build_missing_services
-        service_object = project.send(service_method)
+        service_object = project.find_or_initialize_service(service)
         service_object.properties = service_attrs
         service_object.save
       end
diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb
index 876bfaf085c..2cf9883113c 100644
--- a/spec/services/projects/create_service_spec.rb
+++ b/spec/services/projects/create_service_spec.rb
@@ -10,13 +10,6 @@ describe Projects::CreateService, services: true do
       }
     end
 
-    it 'creates services on Project creation' do
-      project = create_project(@user, @opts)
-      project.reload
-
-      expect(project.services).not_to be_empty
-    end
-
     it 'creates labels on Project creation if there are templates' do
       Label.create(title: "bug", template: true)
       project = create_project(@user, @opts)
@@ -137,6 +130,19 @@ describe Projects::CreateService, services: true do
         expect(project.namespace).to eq(@user.namespace)
       end
     end
+
+    context 'when there is an active service template' do
+      before do
+        create(:service, project: nil, template: true, active: true)
+      end
+
+      it 'creates a service from this template' do
+        project = create_project(@user, @opts)
+        project.reload
+
+        expect(project.services.count).to eq 1
+      end
+    end
   end
 
   def create_project(user, opts)