Merge branch '210023-refactor-and-rename-serviceparams-to-integrations-params' into 'master'

Rename ServiceParams to Integrations::Params

See merge request gitlab-org/gitlab!60649

app/controllers/admin/services_controller.rb

 # frozen_string_literal: true
 
 class Admin::ServicesController < Admin::ApplicationController
-  include ServiceParams
+  include Integrations::Params
 
-  before_action :service, only: [:edit, :update]
+  before_action :integration, only: [:edit, :update]
   before_action :disable_query_limiting, only: [:index]
 
   feature_category :integrations
@@ -14,15 +14,15 @@ class Admin::ServicesController < Admin::ApplicationController
   end
 
   def edit
-    if service.nil? || Service.instance_exists_for?(service.type)
+    if integration.nil? || Service.instance_exists_for?(integration.type)
       redirect_to admin_application_settings_services_path,
         alert: "Service is unknown or it doesn't exist"
     end
   end
 
   def update
-    if service.update(service_params[:service])
-      PropagateServiceTemplateWorker.perform_async(service.id) if service.active? # rubocop:disable CodeReuse/Worker
+    if integration.update(integration_params[:integration])
+      PropagateServiceTemplateWorker.perform_async(integration.id) if integration.active? # rubocop:disable CodeReuse/Worker
 
       redirect_to admin_application_settings_services_path,
         notice: 'Application settings saved successfully'
@@ -34,9 +34,11 @@ class Admin::ServicesController < Admin::ApplicationController
   private
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def service
-    @service ||= Service.find_by(id: params[:id], template: true)
+  def integration
+    @integration ||= Service.find_by(id: params[:id], template: true)
+    @service ||= @integration # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/329759
   end
+  alias_method :service, :integration
   # rubocop: enable CodeReuse/ActiveRecord
 
   def disable_query_limiting

app/controllers/concerns/integrations/params.rb

+# frozen_string_literal: true
+
+module Integrations
+  module Params
+    extend ActiveSupport::Concern
+
+    ALLOWED_PARAMS_CE = [
+      :active,
+      :add_pusher,
+      :alert_events,
+      :api_key,
+      :api_url,
+      :bamboo_url,
+      :branches_to_be_notified,
+      :labels_to_be_notified,
+      :labels_to_be_notified_behavior,
+      :build_key,
+      :build_type,
+      :ca_pem,
+      :channel,
+      :channels,
+      :color,
+      :colorize_messages,
+      :comment_on_event_enabled,
+      :comment_detail,
+      :confidential_issues_events,
+      :confluence_url,
+      :datadog_site,
+      :datadog_env,
+      :datadog_service,
+      :default_irc_uri,
+      :device,
+      :disable_diffs,
+      :drone_url,
+      :enable_ssl_verification,
+      :external_wiki_url,
+      :google_iap_service_account_json,
+      :google_iap_audience_client_id,
+      :inherit_from_id,
+      # We're using `issues_events` and `merge_requests_events`
+      # in the view so we still need to explicitly state them
+      # here. `Service#event_names` would only give
+      # `issue_events` and `merge_request_events` (singular!)
+      # See app/helpers/services_helper.rb for how we
+      # make those event names plural as special case.
+      :issues_events,
+      :issues_url,
+      :jenkins_url,
+      :jira_issue_transition_automatic,
+      :jira_issue_transition_id,
+      :manual_configuration,
+      :merge_requests_events,
+      :mock_service_url,
+      :namespace,
+      :new_issue_url,
+      :notify_only_broken_pipelines,
+      :password,
+      :priority,
+      :project_key,
+      :project_name,
+      :project_url,
+      :recipients,
+      :restrict_to_branch,
+      :room,
+      :send_from_committer_email,
+      :server,
+      :server_host,
+      :server_port,
+      :sound,
+      :subdomain,
+      :teamcity_url,
+      :token,
+      :type,
+      :url,
+      :user_key,
+      :username,
+      :webhook
+    ].freeze
+
+    # Parameters to ignore if no value is specified
+    FILTER_BLANK_PARAMS = [:password].freeze
+
+    def integration_params
+      dynamic_params = @integration.event_channel_names + @integration.event_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      allowed = allowed_integration_params + dynamic_params
+      return_value = params.permit(:id, integration: allowed, service: allowed)
+      return_value[:integration] ||= return_value.delete(:service)
+      param_values = return_value[:integration]
+
+      if param_values.is_a?(ActionController::Parameters)
+        FILTER_BLANK_PARAMS.each do |param|
+          param_values.delete(param) if param_values[param].blank?
+        end
+      end
+
+      return_value
+    end
+
+    def allowed_integration_params
+      ALLOWED_PARAMS_CE
+    end
+  end
+end
+
+Integrations::Params.prepend_if_ee('EE::Integrations::Params')

app/controllers/concerns/integrations_actions.rb

@@ -4,7 +4,7 @@ module IntegrationsActions
   extend ActiveSupport::Concern
 
   included do
-    include ServiceParams
+    include Integrations::Params
 
     before_action :integration, only: [:edit, :update, :test]
   end
@@ -14,7 +14,7 @@ module IntegrationsActions
   end
 
   def update
-    saved = integration.update(service_params[:service])
+    saved = integration.update(integration_params[:integration])
 
     respond_to do |format|
       format.html do
@@ -49,9 +49,7 @@ module IntegrationsActions
   private
 
   def integration
-    # Using instance variable `@service` still required as it's used in ServiceParams.
-    # Should be removed once that is refactored to use `@integration`.
-    @integration = @service ||= find_or_initialize_non_project_specific_integration(params[:id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    @integration ||= find_or_initialize_non_project_specific_integration(params[:id])
   end
 
   def success_message

app/controllers/concerns/service_params.rb

-# frozen_string_literal: true
-
-module ServiceParams
-  extend ActiveSupport::Concern
-
-  ALLOWED_PARAMS_CE = [
-    :active,
-    :add_pusher,
-    :alert_events,
-    :api_key,
-    :api_url,
-    :bamboo_url,
-    :branches_to_be_notified,
-    :labels_to_be_notified,
-    :labels_to_be_notified_behavior,
-    :build_key,
-    :build_type,
-    :ca_pem,
-    :channel,
-    :channels,
-    :color,
-    :colorize_messages,
-    :comment_on_event_enabled,
-    :comment_detail,
-    :confidential_issues_events,
-    :confluence_url,
-    :datadog_site,
-    :datadog_env,
-    :datadog_service,
-    :default_irc_uri,
-    :device,
-    :disable_diffs,
-    :drone_url,
-    :enable_ssl_verification,
-    :external_wiki_url,
-    :google_iap_service_account_json,
-    :google_iap_audience_client_id,
-    :inherit_from_id,
-    # We're using `issues_events` and `merge_requests_events`
-    # in the view so we still need to explicitly state them
-    # here. `Service#event_names` would only give
-    # `issue_events` and `merge_request_events` (singular!)
-    # See app/helpers/services_helper.rb for how we
-    # make those event names plural as special case.
-    :issues_events,
-    :issues_url,
-    :jenkins_url,
-    :jira_issue_transition_automatic,
-    :jira_issue_transition_id,
-    :manual_configuration,
-    :merge_requests_events,
-    :mock_service_url,
-    :namespace,
-    :new_issue_url,
-    :notify_only_broken_pipelines,
-    :password,
-    :priority,
-    :project_key,
-    :project_name,
-    :project_url,
-    :recipients,
-    :restrict_to_branch,
-    :room,
-    :send_from_committer_email,
-    :server,
-    :server_host,
-    :server_port,
-    :sound,
-    :subdomain,
-    :teamcity_url,
-    :token,
-    :type,
-    :url,
-    :user_key,
-    :username,
-    :webhook
-  ].freeze
-
-  # Parameters to ignore if no value is specified
-  FILTER_BLANK_PARAMS = [:password].freeze
-
-  def service_params
-    dynamic_params = @service.event_channel_names + @service.event_names # rubocop:disable Gitlab/ModuleWithInstanceVariables
-    service_params = params.permit(:id, service: allowed_service_params + dynamic_params)
-
-    if service_params[:service].is_a?(ActionController::Parameters)
-      FILTER_BLANK_PARAMS.each do |param|
-        service_params[:service].delete(param) if service_params[:service][param].blank?
-      end
-    end
-
-    service_params
-  end
-
-  def allowed_service_params
-    ALLOWED_PARAMS_CE
-  end
-end
-
-ServiceParams.prepend_if_ee('EE::ServiceParams')

app/controllers/projects/services_controller.rb

 # frozen_string_literal: true
 
 class Projects::ServicesController < Projects::ApplicationController
-  include ServiceParams
+  include Integrations::Params
   include InternalRedirect
 
   # Authorize
   before_action :authorize_admin_project!
   before_action :ensure_service_enabled
-  before_action :service
+  before_action :integration
   before_action :web_hook_logs, only: [:edit, :update]
   before_action :set_deprecation_notice_for_prometheus_service, only: [:edit, :update]
   before_action :redirect_deprecated_prometheus_service, only: [:update]
@@ -26,16 +26,15 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def update
-    @service.attributes = service_params[:service]
-    @service.inherit_from_id = nil if service_params[:service][:inherit_from_id].blank?
+    @integration.attributes = integration_params[:integration]
+    @integration.inherit_from_id = nil if integration_params[:integration][:inherit_from_id].blank?
 
-    saved = @service.save(context: :manual_change)
+    saved = @integration.save(context: :manual_change)
 
     respond_to do |format|
       format.html do
         if saved
-          target_url = safe_redirect_path(params[:redirect_to]).presence || edit_project_service_path(@project, @service)
-          redirect_to target_url, notice: success_message
+          redirect_to redirect_path, notice: success_message
         else
           render 'edit'
         end
@@ -50,7 +49,7 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def test
-    if @service.can_test?
+    if integration.can_test?
       render json: service_test_response, status: :ok
     else
       render json: {}, status: :not_found
@@ -59,12 +58,16 @@ class Projects::ServicesController < Projects::ApplicationController
 
   private
 
+  def redirect_path
+    safe_redirect_path(params[:redirect_to]).presence || edit_project_service_path(@project, @integration)
+  end
+
   def service_test_response
-    unless @service.update(service_params[:service])
-      return { error: true, message: _('Validations failed.'), service_response: @service.errors.full_messages.join(','), test_failed: false }
+    unless @integration.update(integration_params[:integration])
+      return { error: true, message: _('Validations failed.'), service_response: @integration.errors.full_messages.join(','), test_failed: false }
     end
 
-    result = ::Integrations::Test::ProjectService.new(@service, current_user, params[:event]).execute
+    result = ::Integrations::Test::ProjectService.new(@integration, current_user, params[:event]).execute
 
     unless result[:success]
       return { error: true, message: s_('Integrations|Connection failed. Please check your settings.'), service_response: result[:message].to_s, test_failed: true }
@@ -76,16 +79,18 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def success_message
-    if @service.active?
-      s_('Integrations|%{integration} settings saved and active.') % { integration: @service.title }
+    if integration.active?
+      s_('Integrations|%{integration} settings saved and active.') % { integration: integration.title }
     else
-      s_('Integrations|%{integration} settings saved, but not active.') % { integration: @service.title }
+      s_('Integrations|%{integration} settings saved, but not active.') % { integration: integration.title }
     end
   end
 
-  def service
-    @service ||= @project.find_or_initialize_service(params[:id])
+  def integration
+    @integration ||= @project.find_or_initialize_service(params[:id])
+    @service ||= @integration # TODO: remove references to @service
   end
+  alias_method :service, :integration
 
   def web_hook_logs
     return unless @service.service_hook.present?
@@ -98,17 +103,17 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def serialize_as_json
-    @service
+    integration
       .as_json(only: @service.json_fields)
       .merge(errors: @service.errors.as_json)
   end
 
   def redirect_deprecated_prometheus_service
-    redirect_to edit_project_service_path(project, @service) if @service.is_a?(::PrometheusService) && Feature.enabled?(:settings_operations_prometheus_service, project)
+    redirect_to edit_project_service_path(project, integration) if integration.is_a?(::PrometheusService) && Feature.enabled?(:settings_operations_prometheus_service, project)
   end
 
   def set_deprecation_notice_for_prometheus_service
-    return if !@service.is_a?(::PrometheusService) || !Feature.enabled?(:settings_operations_prometheus_service, project)
+    return if !integration.is_a?(::PrometheusService) || !Feature.enabled?(:settings_operations_prometheus_service, project)
 
     operations_link_start = "<a href=\"#{project_settings_operations_path(project)}\">"
     message = s_('PrometheusService|You can now manage your Prometheus settings on the %{operations_link_start}Operations%{operations_link_end} page. Fields on this page has been deprecated.') % { operations_link_start: operations_link_start, operations_link_end: "</a>" }

ee/app/controllers/concerns/ee/integrations/params.rb

+# frozen_string_literal: true
+
+module EE
+  module Integrations
+    module Params
+      extend ::Gitlab::Utils::Override
+
+      ALLOWED_PARAMS_EE = [
+        :issues_enabled,
+        :multiproject_enabled,
+        :pass_unstable,
+        :repository_url,
+        :static_context,
+        :vulnerabilities_enabled,
+        :vulnerabilities_issuetype
+      ].freeze
+
+      override :allowed_integration_params
+      def allowed_integration_params
+        super + ALLOWED_PARAMS_EE
+      end
+    end
+  end
+end

ee/app/controllers/concerns/ee/service_params.rb

-# frozen_string_literal: true
-
-module EE
-  module ServiceParams
-    extend ::Gitlab::Utils::Override
-
-    ALLOWED_PARAMS_EE = [
-      :issues_enabled,
-      :multiproject_enabled,
-      :pass_unstable,
-      :repository_url,
-      :static_context,
-      :vulnerabilities_enabled,
-      :vulnerabilities_issuetype
-    ].freeze
-
-    override :allowed_service_params
-    def allowed_service_params
-      super + ALLOWED_PARAMS_EE
-    end
-  end
-end