Merge branch 'pl-refactor-tracing-settings' into 'master'

Refactor tracing settings

See merge request gitlab-org/gitlab-ee!8983

ee/app/controllers/projects/settings/operations_controller.rb

@@ -7,40 +7,29 @@ module Projects
       before_action :authorize_update_environment!
 
       def show
-        @tracing_settings ||= ProjectTracingSetting.for_project(@project)
       end
 
       def update
-        result = EE::TracingSettingService.new(project, current_user, operations_params).execute
+        result = ::Projects::Operations::UpdateService.new(project, current_user, update_params).execute
 
-        render_result(result)
-      end
-
-      def create
-        result = EE::TracingSettingService.new(project, current_user, operations_params).execute
-        @tracing_setting = project.tracing_setting
-
-        render_result(result)
+        if result[:status] == :success
+          flash[:notice] = _('Your changes have been saved')
+          redirect_to project_settings_operations_path(@project)
+        else
+          render 'show'
+        end
       end
 
       private
 
-      def render_result(result)
-        respond_to do |format|
-          format.html do
-            if result[:status] == :success
-              flash[:notice] = _('Your changes have been saved')
-            else
-              flash[:alert] = _('Unable to save your changes')
-            end
-
-            redirect_to project_settings_operations_path(@project)
-          end
-        end
+      helper_method :tracing_setting
+
+      def tracing_setting
+        @tracing_setting ||= project.tracing_setting || project.build_tracing_setting
       end
 
-      def operations_params
-        params.require(:tracing_settings).permit(:external_url)
+      def update_params
+        params.require(:project).permit(tracing_setting_attributes: [:external_url])
       end
 
       def check_license

ee/app/models/ee/project.rb

@@ -111,6 +111,8 @@ module EE
       default_value_for :packages_enabled, true
 
       delegate :store_security_reports_available?, to: :namespace
+
+      accepts_nested_attributes_for :tracing_setting, update_only: true, allow_destroy: true
     end
 
     class_methods do

ee/app/models/project_tracing_setting.rb

@@ -7,19 +7,6 @@ class ProjectTracingSetting < ActiveRecord::Base
 
   before_validation :sanitize_external_url
 
-  def self.create_or_update(project, params)
-    self.transaction(requires_new: true) do
-      tracing_setting = self.for_project(project)
-      tracing_setting.update(params)
-    end
-  rescue ActiveRecord::RecordNotUnique
-    retry
-  end
-
-  def self.for_project(project)
-    self.where(project: project).first_or_initialize
-  end
-
   private
 
   def sanitize_external_url

ee/app/services/ee/tracing_setting_service.rb

-# frozen_string_literal: true
-
-module EE
-  class TracingSettingService < BaseService
-    ValidationError = Class.new(StandardError)
-
-    def execute
-      # Convert an empty string in tracing_external_url to nil
-      if params.has_key?(:external_url)
-        params[:external_url] = params[:external_url].presence
-      end
-
-      # Delete the row in project_tracing_settings table if external_row is to be
-      # set to nil since that is currently the only value in the table.
-      if params[:external_url].nil?
-        destroy
-      else
-        create_or_update
-      end
-    rescue ValidationError => e
-      error(e.message)
-    end
-
-    def create_or_update
-      if ProjectTracingSetting.create_or_update(project, params)
-        success
-      else
-        update_failed
-      end
-    end
-
-    def destroy
-      tracing_setting = ProjectTracingSetting.for_project(project)
-
-      if tracing_setting.persisted? && tracing_setting.destroy
-        success
-      else
-        destroy_failed
-      end
-    end
-
-    def update_failed
-      model_errors = project.errors.full_messages.to_sentence
-      error_message = model_errors.presence || 'Project tracing settings could not be updated!'
-
-      error(error_message)
-    end
-
-    def destroy_failed
-      model_errors = project.errors.full_messages.to_sentence
-      error_message = model_errors.presence || 'Project tracing settings could not be deleted!'
-
-      error(error_message)
-    end
-  end
-end

ee/app/services/projects/operations/update_service.rb

+# frozen_string_literal: true
+
+module Projects
+  module Operations
+    class UpdateService < BaseService
+      def execute
+        Projects::UpdateService
+          .new(project, current_user, project_update_params)
+          .execute
+      end
+
+      private
+
+      def project_update_params
+        tracing_setting_params(params)
+      end
+
+      def tracing_setting_params(params)
+        attr = params[:tracing_setting_attributes]
+        return params unless attr
+
+        destroy = attr[:external_url].blank?
+
+        { tracing_setting_attributes: attr.merge(_destroy: destroy) }
+      end
+    end
+  end
+end

ee/app/views/projects/settings/operations/_tracing.html.haml

+- has_jaeger_url = setting.external_url.present?
+
+%section
+  %h4
+    = _("Jaeger tracing")
+  %p
+    - if has_jaeger_url
+      - tracing_link = link_to sanitize(@project.tracing_external_url, tags: []), target: "_blank", rel: 'noopener noreferrer' do
+        %span
+          = _('Tracing')
+          = sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle')
+    - else
+      - tracing_link = link_to project_tracing_path(@project) do
+        %span
+          = _('Tracing')
+    = _("To open Jaeger and easily view tracing from GitLab, link the %{link} page to your server").html_safe % { link: tracing_link }
+  = form_for @project, url: project_settings_operations_path(@project), method: :patch do |f|
+    = form_errors(@project)
+    .form-group
+      = f.fields_for :tracing_setting_attributes, setting do |form|
+        = form.label :external_url, _('Jaeger URL'), class: 'label-bold'
+        = form.url_field :external_url, class: 'form-control', placeholder: 'e.g. https://jaeger.mycompany.com'
+        %p.form-text.text-muted
+          - jaeger_help_url = "https://www.jaegertracing.io/docs/1.7/getting-started/"
+          - link_start_tag = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: jaeger_help_url }
+          - link_end_tag = "#{sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle')}</a>".html_safe
+          = _("For more information, please review %{link_start_tag}Jaeger's configuration doc%{link_end_tag}").html_safe % { link_start_tag: link_start_tag, link_end_tag: link_end_tag }
+
+    = f.submit _('Save changes'), class: 'btn btn-success'

ee/app/views/projects/settings/operations/show.html.haml

 - @content_class = "limit-container-width" unless fluid_layout
-- page_title _("Operations Settings")
 - page_title _("Operations")
 
-- has_jaeger_url = @project.tracing_external_url.present?
-
-%section
-  %h4
-    = _("Jaeger tracing")
-  %p
-    - if has_jaeger_url
-      - tracing_link = link_to sanitize(@project.tracing_external_url, tags: []), target: "_blank", rel: 'noopener noreferrer' do
-        %span
-          = _('Tracing')
-          = sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle')
-    - else
-      - tracing_link = link_to project_tracing_path(@project) do
-        %span
-          = _('Tracing')
-    = _("To open Jaeger and easily view tracing from GitLab, link the %{link} page to your server").html_safe % { link: tracing_link }
-  = form_for @tracing_settings, as: :tracing_settings, url: project_settings_operations_path(@project) do |f|
-    = form_errors(@tracing_settings)
-    .form-group
-      = f.label :external_url, _('Jaeger URL'), class: 'label-bold'
-      = f.url_field :external_url, class: 'form-control', placeholder: 'e.g. https://jaeger.mycompany.com'
-      %p.form-text.text-muted
-        - jaeger_help_url = "https://www.jaegertracing.io/docs/1.7/getting-started/"
-        - link_start_tag = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: jaeger_help_url }
-        - link_end_tag = "#{sprite_icon('external-link', size: 16, css_class: 'ml-1 vertical-align-middle')}</a>".html_safe
-        = _("For more information, please review %{link_start_tag}Jaeger's configuration doc%{link_end_tag}").html_safe % { link_start_tag: link_start_tag, link_end_tag: link_end_tag }
-
-    = f.submit _('Save changes'), class: 'btn btn-success'
+- if @project.feature_available?(:tracing, current_user)
+  = render_if_exists 'projects/settings/operations/tracing', setting: tracing_setting

ee/spec/controllers/projects/settings/operations_controller_spec.rb

@@ -19,7 +19,7 @@ describe Projects::Settings::OperationsController do
         end
 
         it 'returns 404' do
-          get :show, params: { namespace_id: project.namespace, project_id: project }
+          get :show, params: project_params(project)
 
           expect(response).to have_gitlab_http_status(:not_found)
         end
@@ -34,7 +34,7 @@ describe Projects::Settings::OperationsController do
       end
 
       it 'renders ok' do
-        get :show, params: { namespace_id: project.namespace, project_id: project }
+        get :show, params: project_params(project)
 
         expect(response).to have_gitlab_http_status(200)
         expect(response).to render_template(:show)
@@ -45,7 +45,7 @@ describe Projects::Settings::OperationsController do
       it 'redirects for private project' do
         project = create(:project, project_visibility)
 
-        get :show, params: { namespace_id: project.namespace, project_id: project }
+        get :show, params: project_params(project)
 
         expect(response).to redirect_to(new_user_session_path)
       end
@@ -225,14 +225,22 @@ describe Projects::Settings::OperationsController do
 
     private
 
-    def project_params(project, params = {})
-      { namespace_id: project.namespace, project_id: project, tracing_settings: params }
-    end
-
     def update_project(project, params)
       patch :update, params: project_params(project, params)
 
       project.reload
     end
   end
+
+  private
+
+  def project_params(project, params = {})
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      project: {
+        tracing_setting_attributes: params
+      }
+    }
+  end
 end

ee/spec/models/project_tracing_setting_spec.rb

@@ -4,7 +4,8 @@ require 'spec_helper'
 
 describe ProjectTracingSetting do
   describe '#external_url' do
-    let(:project) { build(:project) }
+    set(:project) { create(:project) }
+
     let(:tracing_setting) { project.build_tracing_setting }
 
     it 'accepts a valid url' do

ee/spec/services/projects/operations/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Operations::UpdateService do
+  subject { described_class.new(project, user, params) }
+
+  let(:result) { subject.execute }
+
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
+
+  describe '#execute' do
+    context 'tracing setting' do
+      context 'with valid params' do
+        let(:params) do
+          {
+            tracing_setting_attributes: {
+              external_url: 'http://some-url.com'
+            }
+          }
+        end
+
+        context 'with an existing setting' do
+          before do
+            create(:project_tracing_setting, project: project)
+          end
+
+          shared_examples 'setting deletion' do
+            let!(:original_params) { params.deep_dup }
+
+            it 'deletes the setting' do
+              expect(result[:status]).to eq(:success)
+              expect(project.reload.tracing_setting).to be_nil
+            end
+
+            it 'does not modify original params' do
+              subject.execute
+
+              expect(params).to eq(original_params)
+            end
+          end
+
+          it 'updates the setting' do
+            expect(project.tracing_setting).not_to be_nil
+
+            expect(result[:status]).to eq(:success)
+            expect(project.reload.tracing_setting.external_url)
+              .to eq('http://some-url.com')
+          end
+
+          context 'with missing external_url' do
+            before do
+              params[:tracing_setting_attributes].delete(:external_url)
+            end
+
+            it_behaves_like 'setting deletion'
+          end
+
+          context 'with empty external_url' do
+            before do
+              params[:tracing_setting_attributes][:external_url] = ''
+            end
+
+            it_behaves_like 'setting deletion'
+          end
+
+          context 'with blank external_url' do
+            before do
+              params[:tracing_setting_attributes][:external_url] = ' '
+            end
+
+            it_behaves_like 'setting deletion'
+          end
+        end
+
+        context 'without an existing setting' do
+          it 'creates a setting' do
+            expect(project.tracing_setting).to be_nil
+
+            expect(result[:status]).to eq(:success)
+            expect(project.reload.tracing_setting.external_url)
+              .to eq('http://some-url.com')
+          end
+        end
+      end
+
+      context 'with empty params' do
+        let(:params) { {} }
+
+        let!(:tracing_setting) do
+          create(:project_tracing_setting, project: project)
+        end
+
+        it 'does nothing' do
+          expect(result[:status]).to eq(:success)
+          expect(project.reload.tracing_setting).to eq(tracing_setting)
+        end
+      end
+    end
+  end
+end

ee/spec/views/projects/settings/operations/show.html.haml_spec.rb

@@ -16,12 +16,11 @@ describe 'projects/settings/operations/show' do
   describe 'Operations > Tracing' do
     context 'with project.tracing_external_url' do
       let(:tracing_url) { 'https://tracing.url' }
-      let(:tracing_settings) { create(:project_tracing_setting, project: project, external_url: tracing_url) }
+      let(:tracing_setting) { create(:project_tracing_setting, project: project, external_url: tracing_url) }
 
       before do
         allow(view).to receive(:can?).and_return(true)
-
-        assign(:tracing_settings, tracing_settings)
+        allow(view).to receive(:tracing_setting).and_return(tracing_setting)
       end
 
       it 'links to project.tracing_external_url' do
@@ -35,27 +34,26 @@ describe 'projects/settings/operations/show' do
         let(:cleaned_url) { "https://replaceme.com/'>" }
 
         before do
-          tracing_settings.update_column(:external_url, malicious_tracing_url)
+          tracing_setting.update_column(:external_url, malicious_tracing_url)
         end
 
         it 'sanitizes external_url' do
           render
 
-          expect(tracing_settings.external_url).to eq(malicious_tracing_url)
+          expect(tracing_setting.external_url).to eq(malicious_tracing_url)
           expect(rendered).to have_link('Tracing', href: cleaned_url)
         end
       end
     end
 
     context 'without project.tracing_external_url' do
-      let(:tracing_settings) { build(:project_tracing_setting, project: project) }
+      let(:tracing_setting) { build(:project_tracing_setting, project: project) }
 
       before do
         allow(view).to receive(:can?).and_return(true)
+        allow(view).to receive(:tracing_setting).and_return(tracing_setting)
 
-        tracing_settings.external_url = nil
-
-        assign(:tracing_settings, tracing_settings)
+        tracing_setting.external_url = nil
       end
 
       it 'links to Tracing page' do

locale/gitlab.pot

@@ -6138,9 +6138,6 @@ msgstr ""
 msgid "Operations Dashboard"
 msgstr ""
 
-msgid "Operations Settings"
-msgstr ""
-
 msgid "OperationsDashboard|Add a project to the dashboard"
 msgstr ""
 
@@ -9355,9 +9352,6 @@ msgstr ""
 msgid "Unable to load the diff. %{button_try_again}"
 msgstr ""
 
-msgid "Unable to save your changes"
-msgstr ""
-
 msgid "Unable to sign you in to the group with SAML due to \"%{reason}\""
 msgstr ""