Merge branch '260347-group-webhooks-data-refactor' into 'master'

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

See merge request gitlab-org/gitlab!48867

app/services/system_hooks_service.rb

 # frozen_string_literal: true
 
 class SystemHooksService
+  BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember].freeze
+
   def execute_hooks_for(model, event)
     data = build_event_data(model, event)
 
@@ -20,6 +22,9 @@ class SystemHooksService
   private
 
   def build_event_data(model, event)
+    # return entire event data from its builder class, if available.
+    return builder_driven_event_data(model, event) if builder_driven_event_data_available?(model)
+
     data = {
       event_name: build_event_name(model, event),
       created_at: model.created_at&.xmlschema,
@@ -62,8 +67,6 @@ class SystemHooksService
           old_full_path: model.full_path_before_last_save
         )
       end
-    when GroupMember
-      data.merge!(group_member_data(model))
     end
 
     data
@@ -75,10 +78,6 @@ class SystemHooksService
       return "user_add_to_team"      if event == :create
       return "user_remove_from_team" if event == :destroy
       return "user_update_for_team"  if event == :update
-    when GroupMember
-      return 'user_add_to_group'      if event == :create
-      return 'user_remove_from_group' if event == :destroy
-      return 'user_update_for_group'  if event == :update
     else
       "#{model.class.name.downcase}_#{event}"
     end
@@ -128,19 +127,6 @@ class SystemHooksService
     }
   end
 
-  def group_member_data(model)
-    {
-      group_name: model.group.name,
-      group_path: model.group.path,
-      group_id: model.group.id,
-      user_username: model.user.username,
-      user_name: model.user.name,
-      user_email: model.user.email,
-      user_id: model.user.id,
-      group_access: model.human_access
-    }
-  end
-
   def user_data(model)
     {
       name: model.name,
@@ -149,6 +135,17 @@ class SystemHooksService
       username: model.username
     }
   end
+
+  def builder_driven_event_data_available?(model)
+    model.class.in?(BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES)
+  end
+
+  def builder_driven_event_data(model, event)
+    case model
+    when GroupMember
+      Gitlab::HookData::GroupMemberBuilder.new(model).build(event)
+    end
+  end
 end
 
 SystemHooksService.prepend_if_ee('EE::SystemHooksService')

ee/app/services/ee/system_hooks_service.rb

@@ -6,13 +6,6 @@ module EE
 
     private
 
-    override :group_member_data
-    def group_member_data(model)
-      super.tap do |data|
-        data[:group_plan] = model.group.gitlab_subscription&.plan_name
-      end
-    end
-
     override :user_data
     def user_data(model)
       super.tap do |data|

ee/lib/ee/gitlab/hook_data/group_member_builder.rb

+# frozen_string_literal: true
+module EE
+  module Gitlab
+    module HookData
+      module GroupMemberBuilder
+        extend ::Gitlab::Utils::Override
+
+        private
+
+        override :group_member_data
+        def group_member_data
+          super.tap do |data|
+            data[:group_plan] = group_member.group.gitlab_subscription&.plan_name
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/hook_data/group_member_builder_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::GroupMemberBuilder do
+  let(:group_member) { create(:group_member, :developer, group: group) }
+
+  describe '#build' do
+    let(:event) { :create }
+    let(:data) { described_class.new(group_member).build(event) }
+
+    context 'data' do
+      context 'group_plan attribute' do
+        let(:group) { create(:group_with_plan, plan: :gold_plan) }
+
+        it 'returns correct group_plan' do
+          expect(data).to include(:group_plan)
+          expect(data[:group_plan]).to eq('gold')
+        end
+      end
+    end
+  end
+end

lib/gitlab/hook_data/base_builder.rb

@@ -21,6 +21,13 @@ module Gitlab
 
       private
 
+      def timestamps_data
+        {
+          created_at: object.created_at&.xmlschema,
+          updated_at: object.updated_at&.xmlschema
+        }
+      end
+
       def absolute_image_urls(markdown_text)
         return markdown_text unless markdown_text.present?
 

lib/gitlab/hook_data/group_member_builder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HookData
+    class GroupMemberBuilder < BaseBuilder
+      alias_method :group_member, :object
+
+      # Sample data
+
+      # {
+      #   :event_name=>"user_add_to_group",
+      #   :group_name=>"GitLab group",
+      #   :group_path=>"gitlab",
+      #   :group_id=>1,
+      #   :user_username=>"robert",
+      #   :user_name=>"Robert Mills",
+      #   :user_email=>"robert@example.com",
+      #   :user_id=>14,
+      #   :group_access=>"Guest",
+      #   :created_at=>"2020-11-04T10:12:10Z",
+      #   :updated_at=>"2020-11-04T10:12:10Z",
+      # }
+
+      def build(event)
+        [
+          timestamps_data,
+          group_member_data,
+          event_data(event)
+        ].reduce(:merge)
+      end
+
+      private
+
+      def group_member_data
+        {
+          group_name: group_member.group.name,
+          group_path: group_member.group.path,
+          group_id: group_member.group.id,
+          user_username: group_member.user.username,
+          user_name: group_member.user.name,
+          user_email: group_member.user.email,
+          user_id: group_member.user.id,
+          group_access: group_member.human_access
+        }
+      end
+
+      def event_data(event)
+        event_name =  case event
+                      when :create
+                        'user_add_to_group'
+                      when :destroy
+                        'user_remove_from_group'
+                      when :update
+                        'user_update_for_group'
+                      end
+
+        { event_name: event_name }
+      end
+    end
+  end
+end
+
+Gitlab::HookData::GroupMemberBuilder.prepend_if_ee('EE::Gitlab::HookData::GroupMemberBuilder')

spec/lib/gitlab/hook_data/group_member_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::GroupMemberBuilder do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:group_member) { create(:group_member, :developer, group: group) }
+
+  describe '#build' do
+    let(:data) { described_class.new(group_member).build(event) }
+    let(:event_name) { data[:event_name] }
+    let(:attributes) do
+      [
+        :event_name, :created_at, :updated_at, :group_name, :group_path,
+        :group_id, :user_id, :user_username, :user_name, :user_email, :group_access
+      ]
+    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[:group_name]).to eq(group.name)
+          expect(data[:group_path]).to eq(group.path)
+          expect(data[:group_id]).to eq(group.id)
+          expect(data[:user_username]).to eq(group_member.user.username)
+          expect(data[:user_name]).to eq(group_member.user.name)
+          expect(data[:user_email]).to eq(group_member.user.email)
+          expect(data[:user_id]).to eq(group_member.user.id)
+          expect(data[:group_access]).to eq('Developer')
+          expect(data[:created_at]).to eq(group_member.created_at&.xmlschema)
+          expect(data[:updated_at]).to eq(group_member.updated_at&.xmlschema)
+        end
+      end
+
+      context 'on create' do
+        let(:event) { :create }
+
+        it { expect(event_name).to eq('user_add_to_group') }
+        it_behaves_like 'includes the required attributes'
+      end
+
+      context 'on update' do
+        let(:event) { :update }
+
+        it { expect(event_name).to eq('user_update_for_group') }
+        it_behaves_like 'includes the required attributes'
+      end
+
+      context 'on destroy' do
+        let(:event) { :destroy }
+
+        it { expect(event_name).to eq('user_remove_from_group') }
+        it_behaves_like 'includes the required attributes'
+      end
+    end
+  end
+end

spec/services/system_hooks_service_spec.rb

@@ -159,9 +159,6 @@ RSpec.describe SystemHooksService do
     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' }
-    it { expect(event_name(group_member, :create)).to eq 'user_add_to_group' }
-    it { expect(event_name(group_member, :destroy)).to eq 'user_remove_from_group' }
-    it { expect(event_name(group_member, :update)).to eq 'user_update_for_group' }
   end
 
   def event_data(*args)