Merge branch...

Merge branch '290290-refactoring-generate-webhook-data-for-models-from-its-own-data-builder-classes-for-group' into 'master'

Expose webhook data for Group via its own data-builder class

See merge request gitlab-org/gitlab!52060

app/services/system_hooks_service.rb

 # frozen_string_literal: true
 
 class SystemHooksService
-  BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember].freeze
+  BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember, Group].freeze
 
   def execute_hooks_for(model, event)
     data = build_event_data(model, event)
@@ -58,15 +58,6 @@ class SystemHooksService
       end
     when ProjectMember
       data.merge!(project_member_data(model))
-    when Group
-      data.merge!(group_data(model))
-
-      if event == :rename
-        data.merge!(
-          old_path: model.path_before_last_save,
-          old_full_path: model.full_path_before_last_save
-        )
-      end
     end
 
     data
@@ -114,19 +105,6 @@ class SystemHooksService
     }
   end
 
-  def group_data(model)
-    owner = model.owner
-
-    {
-      name: model.name,
-      path: model.path,
-      full_path: model.full_path,
-      group_id: model.id,
-      owner_name: owner.try(:name),
-      owner_email: owner.try(:email)
-    }
-  end
-
   def user_data(model)
     {
       name: model.name,
@@ -141,10 +119,14 @@ class SystemHooksService
   end
 
   def builder_driven_event_data(model, event)
-    case model
-    when GroupMember
-      Gitlab::HookData::GroupMemberBuilder.new(model).build(event)
-    end
+    builder_class = case model
+                    when GroupMember
+                      Gitlab::HookData::GroupMemberBuilder
+                    when Group
+                      Gitlab::HookData::GroupBuilder
+                    end
+
+    builder_class.new(model).build(event)
   end
 end
 

lib/gitlab/hook_data/base_builder.rb

@@ -21,6 +21,12 @@ module Gitlab
 
       private
 
+      def event_data(event)
+        event_name =  "#{object.class.name.downcase}_#{event}"
+
+        { event_name: event_name }
+      end
+
       def timestamps_data
         {
           created_at: object.created_at&.xmlschema,

lib/gitlab/hook_data/group_builder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HookData
+    class GroupBuilder < BaseBuilder
+      alias_method :group, :object
+
+      # Sample data
+      # {
+      #  :created_at=>"2021-01-20T09:40:12Z",
+      #  :updated_at=>"2021-01-20T09:40:12Z",
+      #  :event_name=>"group_rename",
+      #  :name=>"group1",
+      #  :path=>"group1",
+      #  :full_path=>"group1",
+      #  :group_id=>1,
+      #  :owner_name=>nil,
+      #  :owner_email=>nil,
+      #  :old_path=>"old-path",
+      #  :old_full_path=>"old-path"
+      # }
+
+      def build(event)
+        [
+          timestamps_data,
+          event_data(event),
+          group_data,
+          event_specific_group_data(event)
+        ].reduce(:merge)
+      end
+
+      private
+
+      def group_data
+        owner = group.owner
+
+        {
+          name: group.name,
+          path: group.path,
+          full_path: group.full_path,
+          group_id: group.id,
+          owner_name: owner.try(:name),
+          owner_email: owner.try(:email)
+        }
+      end
+
+      def event_specific_group_data(event)
+        return {} unless event == :rename
+
+        {
+          old_path: group.path_before_last_save,
+          old_full_path: group.full_path_before_last_save
+        }
+      end
+    end
+  end
+end

spec/lib/gitlab/hook_data/group_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::GroupBuilder do
+  let_it_be(:group) { create(:group) }
+
+  describe '#build' do
+    let(:data) { described_class.new(group).build(event) }
+    let(:event_name) { data[:event_name] }
+    let(:attributes) do
+      [
+        :event_name, :created_at, :updated_at, :name, :path, :full_path, :group_id,
+        :owner_name, :owner_email
+      ]
+    end
+
+    context 'data' do
+      shared_examples_for 'includes the required attributes' do
+        it 'includes the required attributes' do
+          expect(data).to include(*attributes)
+
+          expect(data[:name]).to eq(group.name)
+          expect(data[:path]).to eq(group.path)
+          expect(data[:full_path]).to eq(group.full_path)
+          expect(data[:group_id]).to eq(group.id)
+          expect(data[:owner_name]).to eq(nil)
+          expect(data[:owner_email]).to eq(nil)
+          expect(data[:created_at]).to eq(group.created_at.xmlschema)
+          expect(data[:updated_at]).to eq(group.updated_at.xmlschema)
+        end
+      end
+
+      shared_examples_for 'does not include old path attributes' do
+        it 'does not include old path attributes' do
+          expect(data).not_to include(:old_path, :old_full_path)
+        end
+      end
+
+      context 'on create' do
+        let(:event) { :create }
+
+        it { expect(event_name).to eq('group_create') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include old path attributes'
+      end
+
+      context 'on destroy' do
+        let(:event) { :destroy }
+
+        it { expect(event_name).to eq('group_destroy') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include old path attributes'
+      end
+
+      context 'on rename' do
+        let(:event) { :rename }
+
+        it { expect(event_name).to eq('group_rename') }
+        it_behaves_like 'includes the required attributes'
+
+        it 'includes old path details' do
+          allow(group).to receive(:path_before_last_save).and_return('old-path')
+
+          expect(data[:old_path]).to eq(group.path_before_last_save)
+          expect(data[:old_full_path]).to eq(group.path_before_last_save)
+        end
+      end
+    end
+  end
+end

spec/services/system_hooks_service_spec.rb

@@ -156,9 +156,6 @@ RSpec.describe SystemHooksService do
     it { expect(event_name(project_member, :update)).to eq "user_update_for_team" }
     it { expect(event_name(key, :create)).to eq 'key_create' }
     it { expect(event_name(key, :destroy)).to eq 'key_destroy' }
-    it { expect(event_name(group, :create)).to eq 'group_create' }
-    it { expect(event_name(group, :destroy)).to eq 'group_destroy' }
-    it { expect(event_name(group, :rename)).to eq 'group_rename' }
   end
 
   def event_data(*args)