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
This commit is contained in:
commit
4e63411daa
|
@ -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
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
title: Defer saving project services to the database if there are no user changes
|
||||
merge_request: 6958
|
||||
author:
|
|
@ -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
|
||||
|
|
|
@ -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) }
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
Loading…
Reference in New Issue