Introduce new hook data builders for Issue and MergeRequest34284-add-changes-to-issuable-webhook-data

Signed-off-by: Rémy Coutable <remy@rymai.me>

3 files changed, 52 insertions, 108 deletions

diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 1f8541a3262..ba57301a3c9 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -267,53 +267,70 @@ describe Issuable do
   describe '#to_hook_data' do
     context 'labels are updated' do
       let(:labels) { create_list(:label, 2) }
-      let(:data) { issue.to_hook_data(user, old_labels: [labels[0]]) }
 
       before do
         issue.update(labels: [labels[1]])
       end
 
-      it 'includes labels in the hook data' do
-        expect(data[:labels]).to eq([labels[1].hook_attrs])
-        expect(data[:changes]).to match(hash_including({
-          labels: [[labels[0].hook_attrs], [labels[1].hook_attrs]]
-        }))
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        builder = double
+
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(issue).and_return(builder)
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'labels' => [[labels[0].hook_attrs], [labels[1].hook_attrs]]
+          ))
+
+        issue.to_hook_data(user, old_labels: [labels[0]])
       end
     end
 
     context 'issue is assigned' do
       let(:user2) { create(:user) }
-      let(:data) { issue.to_hook_data(user, old_assignees: [user]) }
 
       before do
         issue.assignees << user << user2
       end
 
-      it 'returns correct hook data' do
-        expect(data[:assignees]).to eq([user.hook_attrs, user2.hook_attrs])
-        expect(data[:changes]).to match(hash_including({
-          assignees: [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]]
-        }))
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        builder = double
+
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(issue).and_return(builder)
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'assignees' => [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]]
+          ))
+
+        issue.to_hook_data(user, old_assignees: [user])
       end
     end
 
     context 'merge_request is assigned' do
       let(:merge_request) { create(:merge_request) }
       let(:user2) { create(:user) }
-      let(:data) { merge_request.to_hook_data(user, old_assignees: [user]) }
 
       before do
         merge_request.update(assignee: user)
         merge_request.update(assignee: user2)
       end
 
-      it 'returns correct hook data' do
-        expect(data[:object_attributes]['assignee_id']).to eq(user2.id)
-        expect(data[:assignee]).to eq(user2.hook_attrs)
-        expect(data[:changes]).to match(hash_including({
-          assignee_id: [user.id, user2.id],
-          assignee: [user.hook_attrs, user2.hook_attrs]
-        }))
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        builder = double
+
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(merge_request).and_return(builder)
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'assignee_id' => [user.id, user2.id],
+            'assignee' => [user.hook_attrs, user2.hook_attrs]
+          ))
+
+        merge_request.to_hook_data(user, old_assignees: [user])
       end
     end
   end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index bd1ae3c4945..bb5033c1628 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -700,42 +700,14 @@ describe Issue do
   end
 
   describe '#hook_attrs' do
-    let(:attrs_hash) { subject.hook_attrs }
-
-    it 'includes safe attribute' do
-      %w[
-        assignee_id
-        author_id
-        branch_name
-        closed_at
-        confidential
-        created_at
-        deleted_at
-        description
-        due_date
-        id
-        iid
-        last_edited_at
-        last_edited_by_id
-        milestone_id
-        moved_to_id
-        project_id
-        relative_position
-        state
-        time_estimate
-        title
-        updated_at
-        updated_by_id
-      ].each do |key|
-        expect(attrs_hash).to include(key)
-      end
-    end
-
-    it 'includes additional attrs' do
-      expect(attrs_hash).to include(:total_time_spent)
-      expect(attrs_hash).to include(:human_time_estimate)
-      expect(attrs_hash).to include(:human_total_time_spent)
-      expect(attrs_hash).to include(:assignee_ids)
+    it 'delegates to Gitlab::HookData::IssueBuilder#build' do
+      builder = double
+
+      expect(Gitlab::HookData::IssueBuilder)
+        .to receive(:new).with(subject).and_return(builder)
+      expect(builder).to receive(:build)
+
+      subject.hook_attrs
     end
   end
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 844ada57e22..17c9f15b021 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -661,59 +661,14 @@ describe MergeRequest do
   end
 
   describe '#hook_attrs' do
-    let(:attrs_hash) { subject.hook_attrs }
-
-    it 'includes safe attribute' do
-      %w[
-        assignee_id
-        author_id
-        created_at
-        deleted_at
-        description
-        head_pipeline_id
-        id
-        iid
-        last_edited_at
-        last_edited_by_id
-        merge_commit_sha
-        merge_error
-        merge_params
-        merge_status
-        merge_user_id
-        merge_when_pipeline_succeeds
-        milestone_id
-        ref_fetched
-        source_branch
-        source_project_id
-        state
-        target_branch
-        target_project_id
-        time_estimate
-        title
-        updated_at
-        updated_by_id
-      ].each do |key|
-        expect(attrs_hash).to include(key)
-      end
-    end
-
-    %i[source target].each do |key|
-      describe "#{key} key" do
-        include_examples 'project hook data', project_key: key do
-          let(:data)    { attrs_hash }
-          let(:project) { subject.public_send("#{key}_project") }
-        end
-      end
-    end
+    it 'delegates to Gitlab::HookData::MergeRequestBuilder#build' do
+      builder = double
+
+      expect(Gitlab::HookData::MergeRequestBuilder)
+        .to receive(:new).with(subject).and_return(builder)
+      expect(builder).to receive(:build)
 
-    it 'includes additional attrs' do
-      expect(attrs_hash).to include(:source)
-      expect(attrs_hash).to include(:target)
-      expect(attrs_hash).to include(:last_commit)
-      expect(attrs_hash).to include(:work_in_progress)
-      expect(attrs_hash).to include(:total_time_spent)
-      expect(attrs_hash).to include(:human_time_estimate)
-      expect(attrs_hash).to include(:human_total_time_spent)
+      subject.hook_attrs
     end
   end