Merge branch '290290-refactor-user-webhook-to-generate-data-from-own-class' into 'master'

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

See merge request gitlab-org/gitlab!55608

app/services/system_hooks_service.rb

 # frozen_string_literal: true
 
 class SystemHooksService
-  BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember, Group, ProjectMember].freeze
+  BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember, Group, ProjectMember, User].freeze
 
   def execute_hooks_for(model, event)
     data = build_event_data(model, event)
@@ -47,15 +47,6 @@ class SystemHooksService
       if event == :rename || event == :transfer
         data[:old_path_with_namespace] = model.old_path_with_namespace
       end
-    when User
-      data.merge!(user_data(model))
-
-      case event
-      when :rename
-        data[:old_username] = model.username_before_last_save
-      when :failed_login
-        data[:state] = model.state
-      end
     end
 
     data
@@ -79,15 +70,6 @@ class SystemHooksService
     }
   end
 
-  def user_data(model)
-    {
-      name: model.name,
-      email: model.email,
-      user_id: model.id,
-      username: model.username
-    }
-  end
-
   def builder_driven_event_data_available?(model)
     model.class.in?(BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES)
   end
@@ -100,10 +82,10 @@ class SystemHooksService
                       Gitlab::HookData::GroupBuilder
                     when ProjectMember
                       Gitlab::HookData::ProjectMemberBuilder
+                    when User
+                      Gitlab::HookData::UserBuilder
                     end
 
     builder_class.new(model).build(event)
   end
 end
-
-SystemHooksService.prepend_if_ee('EE::SystemHooksService')

ee/app/services/ee/system_hooks_service.rb

-# frozen_string_literal: true
-
-module EE
-  module SystemHooksService
-    extend ::Gitlab::Utils::Override
-
-    private
-
-    override :user_data
-    def user_data(model)
-      super.tap do |data|
-        data.merge!(email_opted_in_data(model)) if ::Gitlab.com?
-      end
-    end
-
-    def email_opted_in_data(model)
-      {
-        email_opted_in: model.email_opted_in,
-        email_opted_in_ip: model.email_opted_in_ip,
-        email_opted_in_source: model.email_opted_in_source,
-        email_opted_in_at: model.email_opted_in_at
-      }
-    end
-  end
-end

ee/lib/ee/gitlab/hook_data/user_builder.rb

+# frozen_string_literal: true
+module EE
+  module Gitlab
+    module HookData
+      module UserBuilder
+        extend ::Gitlab::Utils::Override
+
+        # Sample data
+        # {
+        # :created_at=>"2021-04-02T09:56:49Z",
+        # :updated_at=>"2021-04-02T09:56:49Z",
+        # :event_name=>"user_create",
+        # :name=>"John Doe",
+        # :email=>"john@example.com",
+        # :user_id=>2,
+        # :username=>"johndoe",
+        # :email_opted_in=>"john@example.com",
+        # :email_opted_in_ip=>"192.168.1.1",
+        # :email_opted_in_source=>"Gitlab.com",
+        # :email_opted_in_at=>"2021-03-31T10:30:58Z"
+        # }
+
+        private
+
+        override :user_data
+        def user_data
+          super.tap do |data|
+            data.merge!(email_opted_in_data) if ::Gitlab.com?
+          end
+        end
+
+        def email_opted_in_data
+          {
+            email_opted_in: user.email_opted_in,
+            email_opted_in_ip: user.email_opted_in_ip,
+            email_opted_in_source: user.email_opted_in_source,
+            email_opted_in_at: user.email_opted_in_at
+          }
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::UserBuilder do
+  let_it_be(:user) { create(:user) }
+
+  describe '#build' do
+    let(:event) { :create }
+    let(:data) { described_class.new(user).build(event) }
+
+    context 'data for Gitlab.com' do
+      context 'contains `email_opted_in` attributes' do
+        let(:user) { create(:user, name: 'John Doe', username: 'johndoe', email: 'john@example.com') }
+
+        before do
+          expect(Gitlab).to receive(:com?).and_return(true)
+        end
+
+        it 'returns correct email_opted_in data' do
+          allow(user).to receive(:email_opted_in).and_return(user.email)
+          allow(user).to receive(:email_opted_in_ip).and_return('192.168.1.1')
+          allow(user).to receive(:email_opted_in_source).and_return('Gitlab.com')
+          allow(user).to receive(:email_opted_in_at).and_return('2021-03-31T10:30:58Z')
+
+          expect(data[:email_opted_in]).to eq('john@example.com')
+          expect(data[:email_opted_in_ip]).to eq('192.168.1.1')
+          expect(data[:email_opted_in_source]).to eq('Gitlab.com')
+          expect(data[:email_opted_in_at]).to eq('2021-03-31T10:30:58Z')
+        end
+      end
+    end
+  end
+end

ee/spec/services/ee/system_hooks_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe EE::SystemHooksService do
-  context 'when group member' do
-    let(:group) { create(:group) }
-    let(:group_member) { create(:group_member, group: group) }
-
-    context 'event data' do
-      it { expect(event_data(group_member, :create)).to include(:event_name, :created_at, :updated_at, :group_name, :group_path, :group_plan, :group_id, :user_name, :user_username, :user_email, :user_id, :group_access) }
-      it { expect(event_data(group_member, :destroy)).to include(:event_name, :created_at, :updated_at, :group_name, :group_path, :group_plan, :group_id, :user_name, :user_username, :user_email, :user_id, :group_access) }
-    end
-
-    context 'with a Ultimate plan' do
-      let(:group) { create(:group_with_plan, plan: :ultimate_plan) }
-
-      it 'returns correct group_plan' do
-        expect(event_data(group_member, :create)[:group_plan]).to eq('ultimate')
-      end
-    end
-  end
-
-  context 'when user' do
-    let_it_be(:user) { create(:user) }
-
-    context 'event data' do
-      context 'for GitLab.com' do
-        before do
-          expect(Gitlab).to receive(:com?).and_return(true)
-        end
-
-        it { expect(event_data(user, :create)).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :email_opted_in, :email_opted_in_ip, :email_opted_in_source, :email_opted_in_at) }
-        it { expect(event_data(user, :destroy)).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :email_opted_in, :email_opted_in_ip, :email_opted_in_source, :email_opted_in_at) }
-      end
-
-      context 'for non-GitLab.com' do
-        before do
-          expect(Gitlab).to receive(:com?).and_return(false)
-        end
-
-        it { expect(event_data(user, :create)).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username) }
-        it { expect(event_data(user, :destroy)).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username) }
-      end
-    end
-  end
-
-  def event_data(*args)
-    SystemHooksService.new.send :build_event_data, *args
-  end
-end

lib/gitlab/hook_data/user_builder.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module HookData
+    class UserBuilder < BaseBuilder
+      alias_method :user, :object
+
+      # Sample data
+      # {
+      # :created_at=>"2021-04-02T10:00:26Z",
+      # :updated_at=>"2021-04-02T10:00:26Z",
+      # :event_name=>"user_create",
+      # :name=>"John Doe",
+      # :email=>"john@example.com",
+      # :user_id=>1,
+      # :username=>"johndoe"
+      # }
+
+      def build(event)
+        [
+          timestamps_data,
+          event_data(event),
+          user_data,
+          event_specific_user_data(event)
+        ].reduce(:merge)
+      end
+
+      private
+
+      def user_data
+        {
+          name: user.name,
+          email: user.email,
+          user_id: user.id,
+          username: user.username
+        }
+      end
+
+      def event_specific_user_data(event)
+        case event
+        when :rename
+          { old_username: user.username_before_last_save }
+        when :failed_login
+          { state: user.state }
+        else
+          {}
+        end
+      end
+    end
+  end
+end
+
+Gitlab::HookData::UserBuilder.prepend_if_ee('EE::Gitlab::HookData::UserBuilder')

spec/lib/gitlab/hook_data/user_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::HookData::UserBuilder do
+  let_it_be(:user) { create(:user, name: 'John Doe', username: 'johndoe', email: 'john@example.com') }
+
+  describe '#build' do
+    let(:data) { described_class.new(user).build(event) }
+    let(:event_name) { data[:event_name] }
+    let(:attributes) do
+      [
+        :event_name, :created_at, :updated_at, :name, :email, :user_id, :username
+      ]
+    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('John Doe')
+          expect(data[:email]).to eq('john@example.com')
+          expect(data[:user_id]).to eq(user.id)
+          expect(data[:username]).to eq('johndoe')
+          expect(data[:created_at]).to eq(user.created_at.xmlschema)
+          expect(data[:updated_at]).to eq(user.updated_at.xmlschema)
+        end
+      end
+
+      shared_examples_for 'does not include old username attributes' do
+        it 'does not include old username attributes' do
+          expect(data).not_to include(:old_username)
+        end
+      end
+
+      shared_examples_for 'does not include state attributes' do
+        it 'does not include state attributes' do
+          expect(data).not_to include(:state)
+        end
+      end
+
+      context 'on create' do
+        let(:event) { :create }
+
+        it { expect(event_name).to eq('user_create') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include old username attributes'
+        it_behaves_like 'does not include state attributes'
+      end
+
+      context 'on destroy' do
+        let(:event) { :destroy }
+
+        it { expect(event_name).to eq('user_destroy') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include old username attributes'
+        it_behaves_like 'does not include state attributes'
+      end
+
+      context 'on rename' do
+        let(:event) { :rename }
+
+        it { expect(event_name).to eq('user_rename') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include state attributes'
+
+        it 'includes old username details' do
+          allow(user).to receive(:username_before_last_save).and_return('old-username')
+
+          expect(data[:old_username]).to eq(user.username_before_last_save)
+        end
+      end
+
+      context 'on failed_login' do
+        let(:event) { :failed_login }
+
+        it { expect(event_name).to eq('user_failed_login') }
+        it_behaves_like 'includes the required attributes'
+        it_behaves_like 'does not include old username attributes'
+
+        it 'includes state details' do
+          user.ldap_block!
+
+          expect(data[:state]).to eq('ldap_blocked')
+        end
+      end
+    end
+  end
+end

spec/services/system_hooks_service_spec.rb

@@ -113,37 +113,9 @@ RSpec.describe SystemHooksService do
         expect(data[:old_path]).to eq('old-path')
       end
     end
-
-    context 'user_rename' do
-      it 'contains old and new username' do
-        allow(user).to receive(:username_before_last_save).and_return('old-username')
-
-        data = event_data(user, :rename)
-
-        expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :old_username)
-        expect(data[:username]).to eq(user.username)
-        expect(data[:old_username]).to eq(user.username_before_last_save)
-      end
-    end
-
-    context 'user_failed_login' do
-      it 'contains state of user' do
-        user.ldap_block!
-
-        data = event_data(user, :failed_login)
-
-        expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :state)
-        expect(data[:username]).to eq(user.username)
-        expect(data[:state]).to eq('ldap_blocked')
-      end
-    end
   end
 
   context 'event names' do
-    it { expect(event_name(user, :create)).to eq "user_create" }
-    it { expect(event_name(user, :destroy)).to eq "user_destroy" }
-    it { expect(event_name(user, :rename)).to eq 'user_rename' }
-    it { expect(event_name(user, :failed_login)).to eq 'user_failed_login' }
     it { expect(event_name(project, :create)).to eq "project_create" }
     it { expect(event_name(project, :destroy)).to eq "project_destroy" }
     it { expect(event_name(project, :rename)).to eq "project_rename" }