Merge branch 'pl-tracing-core-4-operations-controller' into 'master'

Move Tracing operations controller to Core  [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!44323

app/controllers/projects/settings/operations_controller.rb

@@ -9,6 +9,7 @@ module Projects
       respond_to :json, only: [:reset_alerting_token, :reset_pagerduty_token]
 
       helper_method :error_tracking_setting
+      helper_method :tracing_setting
 
       def update
         result = ::Projects::Operations::UpdateService.new(project, current_user, update_params).execute
@@ -17,15 +18,6 @@ module Projects
         render_update_response(result)
       end
 
-      # overridden in EE
-      def track_events(result)
-        if result[:status] == :success
-          ::Gitlab::Tracking::IncidentManagement.track_from_params(
-            update_params[:incident_management_setting_attributes]
-          )
-        end
-      end
-
       def reset_alerting_token
         result = ::Projects::Operations::UpdateService
           .new(project, current_user, alerting_params)
@@ -55,6 +47,24 @@ module Projects
 
       private
 
+      def track_events(result)
+        if result[:status] == :success
+          ::Gitlab::Tracking::IncidentManagement.track_from_params(
+            update_params[:incident_management_setting_attributes]
+          )
+          track_tracing_external_url
+        end
+      end
+
+      def track_tracing_external_url
+        external_url_previous_change = project&.tracing_setting&.external_url_previous_change
+
+        return unless external_url_previous_change
+        return unless external_url_previous_change[0].blank? && external_url_previous_change[1].present?
+
+        ::Gitlab::Tracking.event('project:operations:tracing', 'external_url_populated')
+      end
+
       def alerting_params
         { alerting_setting_attributes: { regenerate_token: true } }
       end
@@ -106,6 +116,10 @@ module Projects
           project.build_error_tracking_setting
       end
 
+      def tracing_setting
+        @tracing_setting ||= project.tracing_setting || project.build_tracing_setting
+      end
+
       def update_params
         params.require(:project).permit(permitted_project_params)
       end
@@ -124,7 +138,8 @@ module Projects
             project: [:slug, :name, :organization_slug, :organization_name]
           ],
 
-          grafana_integration_attributes: [:token, :grafana_url, :enabled]
+          grafana_integration_attributes: [:token, :grafana_url, :enabled],
+          tracing_setting_attributes: [:external_url]
         }
 
         if Feature.enabled?(:settings_operations_prometheus_service, project)

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

@@ -8,43 +8,23 @@ module EE
         extend ActiveSupport::Concern
 
         prepended do
-          helper_method :tracing_setting, :status_page_setting
+          helper_method :status_page_setting
 
           private
 
-          def tracing_setting
-            @tracing_setting ||= project.tracing_setting || project.build_tracing_setting
-          end
-
           def status_page_setting
             @status_page_setting ||= project.status_page_setting || project.build_status_page_setting
           end
 
-          def has_tracing_license?
-            project.feature_available?(:tracing, current_user)
-          end
-
           def has_status_page_license?
             project.feature_available?(:status_page, current_user)
           end
-
-          def track_tracing_external_url
-            external_url_previous_change = project&.tracing_setting&.external_url_previous_change
-            return unless external_url_previous_change
-            return unless external_url_previous_change[0].blank? && external_url_previous_change[1].present?
-
-            ::Gitlab::Tracking.event('project:operations:tracing', "external_url_populated")
-          end
         end
 
         override :permitted_project_params
         def permitted_project_params
           permitted_params = super
 
-          if has_tracing_license?
-            permitted_params[:tracing_setting_attributes] = [:external_url]
-          end
-
           if has_status_page_license?
             permitted_params.merge!(status_page_setting_params)
           end
@@ -55,13 +35,6 @@ module EE
         def status_page_setting_params
           { status_page_setting_attributes: [:status_page_url, :aws_s3_bucket_name, :aws_region, :aws_access_key, :aws_secret_key, :enabled] }
         end
-
-        override :track_events
-        def track_events(result)
-          super
-
-          track_tracing_external_url if result[:status] == :success
-        end
       end
     end
   end

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

@@ -3,17 +3,21 @@
 require 'spec_helper'
 
 RSpec.describe Projects::Settings::OperationsController do
+  let_it_be_with_reload(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
 
   before do
     sign_in(user)
   end
 
+  before_all do
+    project.add_maintainer(user)
+  end
+
   describe 'GET show' do
     shared_examples 'user without read access' do |project_visibility, project_role|
-      let(:project) { create(:project, project_visibility) }
-
       before do
+        project.update!(visibility: project_visibility.to_s)
         project.add_role(user, project_role)
       end
 
@@ -25,9 +29,8 @@ RSpec.describe Projects::Settings::OperationsController do
     end
 
     shared_examples 'user with read access' do |project_visibility|
-      let(:project) { create(:project, project_visibility) }
-
       before do
+        project.update!(visibility: project_visibility.to_s)
         project.add_maintainer(user)
       end
 
@@ -40,9 +43,11 @@ RSpec.describe Projects::Settings::OperationsController do
     end
 
     shared_examples 'user needs to login' do |project_visibility|
-      it 'redirects for private project' do
-        project = create(:project, project_visibility)
+      before do
+        project.update!(visibility: project_visibility.to_s)
+      end
 
+      it 'redirects for private project' do
         get :show, params: project_params(project)
 
         expect(response).to redirect_to(new_user_session_path)
@@ -51,7 +56,7 @@ RSpec.describe Projects::Settings::OperationsController do
 
     context 'with a license' do
       before do
-        stub_licensed_features(tracing: true, incident_management: true)
+        stub_licensed_features(status_page: true)
       end
 
       context 'with maintainer role' do
@@ -82,7 +87,7 @@ RSpec.describe Projects::Settings::OperationsController do
 
     context 'without license' do
       before do
-        stub_licensed_features(tracing: false, incident_management: false)
+        stub_licensed_features(status_page: false)
       end
 
       it_behaves_like 'user with read access', :public
@@ -92,32 +97,12 @@ RSpec.describe Projects::Settings::OperationsController do
   end
 
   describe 'PATCH update' do
-    let(:public_project) { create(:project, :public) }
-    let(:private_project) { create(:project, :private) }
-    let(:internal_project) { create(:project, :internal) }
-
-    before do
-      public_project.add_maintainer(user)
-      private_project.add_maintainer(user)
-      internal_project.add_maintainer(user)
-    end
-
     shared_examples 'user without write access' do |project_visibility, project_role|
-      let(:project) { create(:project, project_visibility) }
-
       before do
+        project.update!(visibility: project_visibility.to_s)
         project.add_role(user, project_role)
       end
 
-      it 'does not create tracing_setting' do
-        update_project(
-          project,
-          tracing_params: { external_url: 'https://gitlab.com' }
-        )
-
-        expect(project.tracing_setting).to be_nil
-      end
-
       it 'does not create status_page_setting' do
         update_project(
           project,
@@ -128,70 +113,9 @@ RSpec.describe Projects::Settings::OperationsController do
       end
     end
 
-    context 'format html' do
-      let(:project) { create(:project) }
-      let(:operations_update_service) { spy(:operations_update_service) }
-
-      before do
-        stub_licensed_features(tracing: true)
-
-        project.add_maintainer(user)
-      end
-
-      context 'when update succeeds' do
-        before do
-          stub_operations_update_service_returning(status: :success)
-        end
-
-        it 'shows a notice' do
-          update_project(project, tracing_params: { external_url: 'http://gitlab.com' })
-
-          expect(response).to redirect_to(project_settings_operations_url(project))
-          expect(flash[:notice]).to eq _('Your changes have been saved')
-        end
-      end
-
-      context 'when update fails' do
-        before do
-          stub_operations_update_service_returning(status: :error)
-        end
-
-        it 'renders show page' do
-          update_project(project, tracing_params: { external_url: 'http://gitlab.com' })
-
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(response).to render_template(:show)
-        end
-      end
-    end
-
     context 'with a license' do
       before do
-        stub_licensed_features(tracing: true, incident_management: true, status_page: true)
-      end
-
-      shared_examples 'user with write access' do |project_visibility|
-        let(:project) { create(:project, project_visibility) }
-        let(:tracing_url) { 'https://gitlab.com' }
-
-        before do
-          project.add_maintainer(user)
-        end
-
-        it 'creates tracing setting' do
-          update_project(
-            project,
-            tracing_params: { external_url: tracing_url }
-          )
-
-          expect(project.tracing_setting.external_url).to eq(tracing_url)
-        end
-      end
-
-      context 'with maintainer role' do
-        it_behaves_like 'user with write access', :public
-        it_behaves_like 'user with write access', :private
-        it_behaves_like 'user with write access', :internal
+        stub_licensed_features(status_page: true)
       end
 
       context 'with non maintainer roles' do
@@ -214,80 +138,7 @@ RSpec.describe Projects::Settings::OperationsController do
         it_behaves_like 'user without write access', :internal, :maintainer
       end
 
-      context 'with existing tracing_setting' do
-        let(:project) { create(:project) }
-
-        before do
-          project.create_tracing_setting!(external_url: 'https://gitlab.com')
-          project.add_maintainer(user)
-        end
-
-        it 'unsets external_url with nil' do
-          update_project(project, tracing_params: { external_url: nil })
-
-          expect(project.tracing_setting).to be_nil
-        end
-
-        it 'unsets external_url with empty string' do
-          update_project(project, tracing_params: { external_url: '' })
-
-          expect(project.tracing_setting).to be_nil
-        end
-
-        it 'fails validation with invalid url' do
-          expect do
-            update_project(project, tracing_params: { external_url: "invalid" })
-          end.not_to change(project.tracing_setting, :external_url)
-        end
-
-        it 'does not set external_url if not present in params' do
-          expect do
-            update_project(project, tracing_params: { some_param: 'some_value' })
-          end.not_to change(project.tracing_setting, :external_url)
-        end
-      end
-
-      context 'without existing tracing_setting' do
-        let(:project) { create(:project) }
-
-        before do
-          project.add_maintainer(user)
-        end
-
-        it 'fails validation with invalid url' do
-          update_project(project, tracing_params: { external_url: "invalid" })
-
-          expect(project.tracing_setting).to be_nil
-        end
-
-        it 'does not set external_url if not present in params' do
-          update_project(project, tracing_params: { some_param: 'some_value' })
-
-          expect(project.tracing_setting).to be_nil
-        end
-      end
-
-      context 'updating tracing settings' do
-        let(:project) { create(:project) }
-        let(:new_tracing_settings) { {} }
-
-        before do
-          project.add_maintainer(user)
-        end
-
-        it 'creates a gitlab tracking event' do
-          expect(Gitlab::Tracking).to receive(:event).with('project:operations:tracing', 'external_url_populated')
-          update_project(project, tracing_params: { external_url: "http://example.com" } )
-        end
-      end
-
       context 'without existing status page setting' do
-        let(:project) { create(:project) }
-
-        before do
-          project.add_maintainer(user)
-        end
-
         subject(:status_page_setting) do
           valid_attributes = attributes_for(:status_page_setting).except(:enabled)
           update_project(project, status_page_params: valid_attributes )
@@ -299,11 +150,9 @@ RSpec.describe Projects::Settings::OperationsController do
       end
 
       context 'with existing status page setting' do
-        let(:project) { create(:project) }
         let(:status_page_attributes) { attributes_for(:status_page_setting) }
 
         before do
-          project.add_maintainer(user)
           project.create_status_page_setting!(status_page_attributes)
         end
 
@@ -336,7 +185,7 @@ RSpec.describe Projects::Settings::OperationsController do
 
     context 'without a license' do
       before do
-        stub_licensed_features(tracing: false, incident_management: false, status_page: false)
+        stub_licensed_features(status_page: false)
       end
 
       it_behaves_like 'user without write access', :public, :maintainer
@@ -346,11 +195,9 @@ RSpec.describe Projects::Settings::OperationsController do
 
     private
 
-    def update_project(project, tracing_params: nil, incident_management_params: nil, status_page_params: nil)
+    def update_project(project, status_page_params: nil)
       patch :update, params: project_params(
         project,
-        tracing_params: tracing_params,
-        incident_management_params: incident_management_params,
         status_page_params: status_page_params
       )
 
@@ -360,13 +207,11 @@ RSpec.describe Projects::Settings::OperationsController do
 
   private
 
-  def project_params(project, tracing_params: nil, incident_management_params: nil, status_page_params: nil)
+  def project_params(project, status_page_params: nil)
     {
       namespace_id: project.namespace,
       project_id: project,
       project: {
-        tracing_setting_attributes: tracing_params,
-        incident_management_setting_attributes: incident_management_params,
         status_page_setting_attributes: status_page_params
       }
     }

spec/controllers/projects/settings/operations_controller_spec.rb

@@ -6,9 +6,12 @@ RSpec.describe Projects::Settings::OperationsController do
   let_it_be(:user) { create(:user) }
   let_it_be(:project, reload: true) { create(:project) }
 
+  before_all do
+    project.add_maintainer(user)
+  end
+
   before do
     sign_in(user)
-    project.add_maintainer(user)
   end
 
   shared_examples 'PATCHable' do
@@ -163,10 +166,6 @@ RSpec.describe Projects::Settings::OperationsController do
       context 'updating each incident management setting' do
         let(:new_incident_management_settings) { {} }
 
-        before do
-          project.add_maintainer(user)
-        end
-
         shared_examples 'a gitlab tracking event' do |params, event_key|
           it "creates a gitlab tracking event #{event_key}" do
             new_incident_management_settings = params
@@ -194,10 +193,6 @@ RSpec.describe Projects::Settings::OperationsController do
     end
 
     describe 'POST #reset_pagerduty_token' do
-      before do
-        project.add_maintainer(user)
-      end
-
       context 'with existing incident management setting has active PagerDuty webhook' do
         let!(:incident_management_setting) do
           create(:project_incident_management_setting, project: project, pagerduty_active: true)
@@ -392,10 +387,6 @@ RSpec.describe Projects::Settings::OperationsController do
     end
 
     describe 'POST #reset_alerting_token' do
-      before do
-        project.add_maintainer(user)
-      end
-
       context 'with existing alerting setting' do
         let!(:alerting_setting) do
           create(:project_alerting_setting, project: project)
@@ -478,6 +469,104 @@ RSpec.describe Projects::Settings::OperationsController do
     end
   end
 
+  context 'tracing integration' do
+    describe 'GET #show' do
+      context 'with existing setting' do
+        let_it_be(:setting) do
+          create(:project_tracing_setting, project: project)
+        end
+
+        it 'loads existing setting' do
+          get :show, params: project_params(project)
+
+          expect(controller.helpers.tracing_setting).to eq(setting)
+        end
+      end
+
+      context 'without an existing setting' do
+        it 'builds a new setting' do
+          get :show, params: project_params(project)
+
+          expect(controller.helpers.tracing_setting).to be_new_record
+        end
+      end
+    end
+
+    describe 'PATCH #update' do
+      let_it_be(:external_url) { 'https://gitlab.com' }
+      let(:params) do
+        {
+          tracing_setting_attributes: {
+            external_url: external_url
+          }
+        }
+      end
+
+      it_behaves_like 'PATCHable'
+
+      describe 'gitlab tracking', :snowplow, quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/259282#note_425243784' do
+        shared_examples 'event tracking' do
+          it 'tracks an event' do
+            expect_snowplow_event(
+              category: 'project:operations:tracing',
+              action: 'external_url_populated'
+            )
+          end
+        end
+
+        shared_examples 'no event tracking' do
+          it 'does not track an event' do
+            expect_no_snowplow_event
+          end
+        end
+
+        before do
+          make_request
+        end
+
+        subject(:make_request) do
+          patch :update, params: project_params(project, params), format: :json
+        end
+
+        context 'without existing setting' do
+          context 'when creating a new setting' do
+            it_behaves_like 'event tracking'
+          end
+
+          context 'with invalid external_url' do
+            let_it_be(:external_url) { nil }
+
+            it_behaves_like 'no event tracking'
+          end
+        end
+
+        context 'with existing setting' do
+          let_it_be(:existing_setting) do
+            create(:project_tracing_setting,
+                   project: project,
+                   external_url: external_url)
+          end
+
+          context 'when changing external_url' do
+            let_it_be(:external_url) { 'https://example.com' }
+
+            it_behaves_like 'no event tracking'
+          end
+
+          context 'with unchanged external_url' do
+            it_behaves_like 'no event tracking'
+          end
+
+          context 'with invalid external_url' do
+            let_it_be(:external_url) { nil }
+
+            it_behaves_like 'no event tracking'
+          end
+        end
+      end
+    end
+  end
+
   private
 
   def project_params(project, params = {})