Merge branch...

Merge branch '342835-move-web-hooks-logic-from-featureflag-model-to-a-new-hookservice' into 'master'

Move web hooks logic from FeatureFlag model to a new HookService

See merge request gitlab-org/gitlab!72143

app/models/operations/feature_flag.rb

@@ -98,13 +98,6 @@ module Operations
       Ability.issues_readable_by_user(issues, current_user)
     end
 
-    def execute_hooks(current_user)
-      run_after_commit do
-        feature_flag_data = Gitlab::DataBuilder::FeatureFlag.build(self, current_user)
-        project.execute_hooks(feature_flag_data, :feature_flag_hooks)
-      end
-    end
-
     def hook_attrs
       {
         id: id,

app/services/feature_flags/base_service.rb

@@ -7,6 +7,8 @@ module FeatureFlags
     AUDITABLE_ATTRIBUTES = %w(name description active).freeze
 
     def success(**args)
+      audit_event = args.fetch(:audit_event) { audit_event(args[:feature_flag]) }
+      save_audit_event(audit_event)
       sync_to_jira(args[:feature_flag])
       super
     end
@@ -66,5 +68,11 @@ module FeatureFlags
         feature_flag_by_name.scopes.find_by_environment_scope(params[:environment_scope])
       end
     end
+
+    private
+
+    def audit_message(feature_flag)
+      raise NotImplementedError, "This method should be overriden by subclasses"
+    end
   end
 end

app/services/feature_flags/create_service.rb

@@ -10,8 +10,6 @@ module FeatureFlags
         feature_flag = project.operations_feature_flags.new(params)
 
         if feature_flag.save
-          save_audit_event(audit_event(feature_flag))
-
           success(feature_flag: feature_flag)
         else
           error(feature_flag.errors.full_messages, 400)

app/services/feature_flags/destroy_service.rb

@@ -13,8 +13,6 @@ module FeatureFlags
 
       ApplicationRecord.transaction do
         if feature_flag.destroy
-          save_audit_event(audit_event(feature_flag))
-
           success(feature_flag: feature_flag)
         else
           error(feature_flag.errors.full_messages)

app/services/feature_flags/hook_service.rb

+# frozen_string_literal: true
+
+module FeatureFlags
+  class HookService
+    HOOK_NAME = :feature_flag_hooks
+
+    def initialize(feature_flag, current_user)
+      @feature_flag = feature_flag
+      @current_user = current_user
+    end
+
+    def execute
+      project.execute_hooks(hook_data, HOOK_NAME)
+    end
+
+    private
+
+    attr_reader :feature_flag, :current_user
+
+    def project
+      @project ||= feature_flag.project
+    end
+
+    def hook_data
+      Gitlab::DataBuilder::FeatureFlag.build(feature_flag, current_user)
+    end
+  end
+end

app/services/feature_flags/update_service.rb

@@ -7,6 +7,11 @@ module FeatureFlags
       'parameters' => 'parameters'
     }.freeze
 
+    def success(**args)
+      execute_hooks_after_commit(args[:feature_flag])
+      super
+    end
+
     def execute(feature_flag)
       return error('Access Denied', 403) unless can_update?(feature_flag)
       return error('Not Found', 404) unless valid_user_list_ids?(feature_flag, user_list_ids(params))
@@ -20,16 +25,11 @@ module FeatureFlags
           end
         end
 
+        # We generate the audit event before the feature flag is saved as #changed_strategies_messages depends on the strategies' states before save
         audit_event = audit_event(feature_flag)
 
-        if feature_flag.active_changed?
-          feature_flag.execute_hooks(current_user)
-        end
-
         if feature_flag.save
-          save_audit_event(audit_event)
-
-          success(feature_flag: feature_flag)
+          success(feature_flag: feature_flag, audit_event: audit_event)
         else
           error(feature_flag.errors.full_messages, :bad_request)
         end
@@ -38,6 +38,16 @@ module FeatureFlags
 
     private
 
+    def execute_hooks_after_commit(feature_flag)
+      return unless feature_flag.active_previously_changed?
+
+      # The `current_user` method (defined in `BaseService`) is not available within the `run_after_commit` block
+      user = current_user
+      feature_flag.run_after_commit do
+        HookService.new(feature_flag, user).execute
+      end
+    end
+
     def audit_message(feature_flag)
       changes = changed_attributes_messages(feature_flag)
       changes += changed_strategies_messages(feature_flag)

spec/models/operations/feature_flag_spec.rb

@@ -164,26 +164,4 @@ RSpec.describe Operations::FeatureFlag do
       expect(subject.hook_attrs).to eq(hook_attrs)
     end
   end
-
-  describe "#execute_hooks" do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project) }
-    let_it_be(:feature_flag) { create(:operations_feature_flag, project: project) }
-
-    it 'does not execute the hook when feature_flag event is disabled' do
-      create(:project_hook, project: project, feature_flag_events: false)
-      expect(WebHookWorker).not_to receive(:perform_async)
-
-      feature_flag.execute_hooks(user)
-      feature_flag.touch
-    end
-
-    it 'executes hook when feature_flag event is enabled' do
-      hook = create(:project_hook, project: project, feature_flag_events: true)
-      expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks')
-
-      feature_flag.execute_hooks(user)
-      feature_flag.touch
-    end
-  end
 end

spec/services/feature_flags/hook_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe FeatureFlags::HookService do
+  describe '#execute_hooks' do
+    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:project) { create(:project, :repository, namespace: namespace) }
+    let_it_be(:feature_flag) { create(:operations_feature_flag, project: project) }
+    let_it_be(:user) { namespace.owner }
+
+    let!(:hook) { create(:project_hook, project: project) }
+    let(:hook_data) { double }
+
+    subject(:service) { described_class.new(feature_flag, user) }
+
+    describe 'HOOK_NAME' do
+      specify { expect(described_class::HOOK_NAME).to eq(:feature_flag_hooks) }
+    end
+
+    before do
+      allow(Gitlab::DataBuilder::FeatureFlag).to receive(:build).with(feature_flag, user).once.and_return(hook_data)
+    end
+
+    it 'calls feature_flag.project.execute_hooks' do
+      expect(feature_flag.project).to receive(:execute_hooks).with(hook_data, described_class::HOOK_NAME)
+
+      service.execute
+    end
+  end
+end