Add labels to the 'Note on MR' webhook payload

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

app/models/concerns/issuable.rb

@@ -524,6 +524,10 @@ module Issuable
     labels.order('title ASC').pluck(:title)
   end
 
+  def labels_hook_attrs
+    labels.map(&:hook_attrs)
+  end
+
   # Convert this Issuable class name to a format usable by Ability definitions
   #
   # Examples:

app/models/issue.rb

@@ -386,10 +386,6 @@ class Issue < ApplicationRecord
     resource_parent.root_namespace&.issue_repositioning_disabled?
   end
 
-  def hook_attrs
-    Gitlab::HookData::IssueBuilder.new(self).build
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
@@ -530,10 +526,6 @@ class Issue < ApplicationRecord
     ::MergeRequestsClosingIssues.count_for_issue(self.id, user)
   end
 
-  def labels_hook_attrs
-    labels.map(&:hook_attrs)
-  end
-
   def previous_updated_at
     previous_changes['updated_at']&.first || updated_at
   end

doc/user/project/integrations/webhook_events.md

@@ -538,6 +538,32 @@ Payload example:
     "iid": 1,
     "description": "Et voluptas corrupti assumenda temporibus. Architecto cum animi eveniet amet asperiores. Vitae numquam voluptate est natus sit et ad id.",
     "position": 0,
+    "labels": [
+      {
+        "id": 25,
+        "title": "Afterpod",
+        "color": "#3e8068",
+        "project_id": null,
+        "created_at": "2019-06-05T14:32:20.211Z",
+        "updated_at": "2019-06-05T14:32:20.211Z",
+        "template": false,
+        "description": null,
+        "type": "GroupLabel",
+        "group_id": 4
+      },
+      {
+        "id": 86,
+        "title": "Element",
+        "color": "#231afe",
+        "project_id": 4,
+        "created_at": "2019-06-05T14:32:20.637Z",
+        "updated_at": "2019-06-05T14:32:20.637Z",
+        "template": false,
+        "description": null,
+        "type": "ProjectLabel",
+        "group_id": null
+      }
+    ],
     "source":{
       "name":"Gitlab Test",
       "description":"Aut reprehenderit ut est.",

lib/gitlab/data_builder/note.rb

@@ -43,10 +43,9 @@ module Gitlab
         if note.for_commit?
           data[:commit] = build_data_for_commit(project, user, note)
         elsif note.for_issue?
-          data[:issue] = note.noteable.hook_attrs
-          data[:issue][:labels] = note.noteable.labels_hook_attrs
+          data[:issue] = Gitlab::HookData::IssueBuilder.new(note.noteable).build
         elsif note.for_merge_request?
-          data[:merge_request] = note.noteable.hook_attrs
+          data[:merge_request] = Gitlab::HookData::MergeRequestBuilder.new(note.noteable).build
         elsif note.for_snippet?
           data[:snippet] = note.noteable.hook_attrs
         end

lib/gitlab/hook_data/issuable_builder.rb

@@ -13,7 +13,7 @@ module Gitlab
           event_type: event_type,
           user: user.hook_attrs,
           project: issuable.project.hook_attrs,
-          object_attributes: issuable.hook_attrs,
+          object_attributes: issuable_builder.new(issuable).build,
           labels: issuable.labels.map(&:hook_attrs),
           changes: final_changes(changes.slice(*safe_keys)),
           # DEPRECATED

lib/gitlab/hook_data/merge_request_builder.rb

@@ -60,6 +60,7 @@ module Gitlab
           human_time_estimate: merge_request.human_time_estimate,
           assignee_ids: merge_request.assignee_ids,
           assignee_id: merge_request.assignee_ids.first, # This key is deprecated
+          labels: merge_request.labels_hook_attrs,
           state: merge_request.state, # This key is deprecated
           blocking_discussions_resolved: merge_request.mergeable_discussions_state?
         }

spec/lib/gitlab/data_builder/note_spec.rb

@@ -8,18 +8,22 @@ RSpec.describe Gitlab::DataBuilder::Note do
   let(:data) { described_class.build(note, user) }
   let(:fixed_time) { Time.at(1425600000) } # Avoid time precision errors
 
-  before do
-    expect(data).to have_key(:object_attributes)
-    expect(data[:object_attributes]).to have_key(:url)
-    expect(data[:object_attributes][:url])
-      .to eq(Gitlab::UrlBuilder.build(note))
-    expect(data[:object_kind]).to eq('note')
-    expect(data[:user]).to eq(user.hook_attrs)
+  shared_examples 'includes general data' do
+    specify do
+      expect(data).to have_key(:object_attributes)
+      expect(data[:object_attributes]).to have_key(:url)
+      expect(data[:object_attributes][:url])
+        .to eq(Gitlab::UrlBuilder.build(note))
+      expect(data[:object_kind]).to eq('note')
+      expect(data[:user]).to eq(user.hook_attrs)
+    end
   end
 
   describe 'When asking for a note on commit' do
     let(:note) { create(:note_on_commit, project: project) }
 
+    it_behaves_like 'includes general data'
+
     it 'returns the note and commit-specific data' do
       expect(data).to have_key(:commit)
     end
@@ -31,6 +35,8 @@ RSpec.describe Gitlab::DataBuilder::Note do
   describe 'When asking for a note on commit diff' do
     let(:note) { create(:diff_note_on_commit, project: project) }
 
+    it_behaves_like 'includes general data'
+
     it 'returns the note and commit-specific data' do
       expect(data).to have_key(:commit)
     end
@@ -51,22 +57,21 @@ RSpec.describe Gitlab::DataBuilder::Note do
       create(:note_on_issue, noteable: issue, project: project)
     end
 
+    it_behaves_like 'includes general data'
+
     it 'returns the note and issue-specific data' do
-      without_timestamps = lambda { |label| label.except('created_at', 'updated_at') }
-      hook_attrs = issue.reload.hook_attrs
+      expect_next_instance_of(Gitlab::HookData::IssueBuilder) do |issue_data_builder|
+        expect(issue_data_builder).to receive(:build).and_return('Issue data')
+      end
 
-      expect(data).to have_key(:issue)
-      expect(data[:issue].except('updated_at', 'labels'))
-        .to eq(hook_attrs.except('updated_at', 'labels'))
-      expect(data[:issue]['updated_at'])
-        .to be >= hook_attrs['updated_at']
-      expect(data[:issue]['labels'].map(&without_timestamps))
-        .to eq(hook_attrs['labels'].map(&without_timestamps))
+      expect(data[:issue]).to eq('Issue data')
     end
 
     context 'with confidential issue' do
       let(:issue) { create(:issue, project: project, confidential: true) }
 
+      it_behaves_like 'includes general data'
+
       it 'sets event_type to confidential_note' do
         expect(data[:event_type]).to eq('confidential_note')
       end
@@ -77,10 +82,12 @@ RSpec.describe Gitlab::DataBuilder::Note do
   end
 
   describe 'When asking for a note on merge request' do
+    let(:label) { create(:label, project: project) }
     let(:merge_request) do
-      create(:merge_request, created_at: fixed_time,
+      create(:labeled_merge_request, created_at: fixed_time,
                              updated_at: fixed_time,
-                             source_project: project)
+                             source_project: project,
+                             labels: [label])
     end
 
     let(:note) do
@@ -88,12 +95,14 @@ RSpec.describe Gitlab::DataBuilder::Note do
                                      project: project)
     end
 
-    it 'returns the note and merge request data' do
-      expect(data).to have_key(:merge_request)
-      expect(data[:merge_request].except('updated_at'))
-        .to eq(merge_request.reload.hook_attrs.except('updated_at'))
-      expect(data[:merge_request]['updated_at'])
-        .to be >= merge_request.hook_attrs['updated_at']
+    it_behaves_like 'includes general data'
+
+    it 'returns the merge request data' do
+      expect_next_instance_of(Gitlab::HookData::MergeRequestBuilder) do |mr_data_builder|
+        expect(mr_data_builder).to receive(:build).and_return('MR data')
+      end
+
+      expect(data[:merge_request]).to eq('MR data')
     end
 
     include_examples 'project hook data'
@@ -101,9 +110,11 @@ RSpec.describe Gitlab::DataBuilder::Note do
   end
 
   describe 'When asking for a note on merge request diff' do
+    let(:label) { create(:label, project: project) }
     let(:merge_request) do
-      create(:merge_request, created_at: fixed_time, updated_at: fixed_time,
-                             source_project: project)
+      create(:labeled_merge_request, created_at: fixed_time, updated_at: fixed_time,
+                             source_project: project,
+                             labels: [label])
     end
 
     let(:note) do
@@ -111,12 +122,14 @@ RSpec.describe Gitlab::DataBuilder::Note do
                                           project: project)
     end
 
-    it 'returns the note and merge request diff data' do
-      expect(data).to have_key(:merge_request)
-      expect(data[:merge_request].except('updated_at'))
-        .to eq(merge_request.reload.hook_attrs.except('updated_at'))
-      expect(data[:merge_request]['updated_at'])
-        .to be >= merge_request.hook_attrs['updated_at']
+    it_behaves_like 'includes general data'
+
+    it 'returns the merge request data' do
+      expect_next_instance_of(Gitlab::HookData::MergeRequestBuilder) do |mr_data_builder|
+        expect(mr_data_builder).to receive(:build).and_return('MR data')
+      end
+
+      expect(data[:merge_request]).to eq('MR data')
     end
 
     include_examples 'project hook data'
@@ -134,6 +147,8 @@ RSpec.describe Gitlab::DataBuilder::Note do
                                        project: project)
     end
 
+    it_behaves_like 'includes general data'
+
     it 'returns the note and project snippet data' do
       expect(data).to have_key(:snippet)
       expect(data[:snippet].except('updated_at'))

spec/lib/gitlab/hook_data/issuable_builder_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Gitlab::HookData::IssuableBuilder do
   let_it_be(:user) { create(:user) }
 
   # This shared example requires a `builder` and `user` variable
-  shared_examples 'issuable hook data' do |kind|
+  shared_examples 'issuable hook data' do |kind, hook_data_issuable_builder_class|
     let(:data) { builder.build(user: user) }
 
     include_examples 'project hook data' do
@@ -20,7 +20,7 @@ RSpec.describe Gitlab::HookData::IssuableBuilder do
         expect(data[:object_kind]).to eq(kind)
         expect(data[:user]).to eq(user.hook_attrs)
         expect(data[:project]).to eq(builder.issuable.project.hook_attrs)
-        expect(data[:object_attributes]).to eq(builder.issuable.hook_attrs)
+        expect(data[:object_attributes]).to eq(hook_data_issuable_builder_class.new(issuable).build)
         expect(data[:changes]).to eq({})
         expect(data[:repository]).to eq(builder.issuable.project.hook_attrs.slice(:name, :url, :description, :homepage))
       end
@@ -95,12 +95,12 @@ RSpec.describe Gitlab::HookData::IssuableBuilder do
   end
 
   describe '#build' do
-    it_behaves_like 'issuable hook data', 'issue' do
+    it_behaves_like 'issuable hook data', 'issue', Gitlab::HookData::IssueBuilder do
       let(:issuable) { create(:issue, description: 'A description') }
       let(:builder) { described_class.new(issuable) }
     end
 
-    it_behaves_like 'issuable hook data', 'merge_request' do
+    it_behaves_like 'issuable hook data', 'merge_request', Gitlab::HookData::MergeRequestBuilder do
       let(:issuable) { create(:merge_request, description: 'A description') }
       let(:builder) { described_class.new(issuable) }
     end

spec/lib/gitlab/hook_data/merge_request_builder_spec.rb

@@ -62,6 +62,7 @@ RSpec.describe Gitlab::HookData::MergeRequestBuilder do
       expect(data).to include(:human_time_estimate)
       expect(data).to include(:human_total_time_spent)
       expect(data).to include(:human_time_change)
+      expect(data).to include(:labels)
     end
 
     context 'when the MR has an image in the description' do

spec/models/concerns/issuable_spec.rb

@@ -625,6 +625,16 @@ RSpec.describe Issuable do
     end
   end
 
+  describe "#labels_hook_attrs" do
+    let(:project) { create(:project) }
+    let(:label) { create(:label) }
+    let(:issue) { create(:labeled_issue, project: project, labels: [label]) }
+
+    it "returns a list of label hook attributes" do
+      expect(issue.labels_hook_attrs).to match_array([label.hook_attrs])
+    end
+  end
+
   describe '.labels_hash' do
     let(:feature_label) { create(:label, title: 'Feature') }
     let(:second_label) { create(:label, title: 'Second Label') }

spec/models/issue_spec.rb

@@ -1172,18 +1172,6 @@ RSpec.describe Issue do
     end
   end
 
-  describe '#hook_attrs' do
-    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
-
   describe '#check_for_spam?' do
     let_it_be(:support_bot) { ::User.support_bot }
 
@@ -1332,15 +1320,6 @@ RSpec.describe Issue do
     subject { create(:issue, updated_at: 1.hour.ago) }
   end
 
-  describe "#labels_hook_attrs" do
-    let(:label) { create(:label) }
-    let(:issue) { create(:labeled_issue, project: reusable_project, labels: [label]) }
-
-    it "returns a list of label hook attributes" do
-      expect(issue.labels_hook_attrs).to eq([label.hook_attrs])
-    end
-  end
-
   context "relative positioning" do
     let_it_be(:group) { create(:group) }
     let_it_be(:project) { create(:project, group: group) }

spec/models/merge_request_spec.rb

@@ -1757,18 +1757,6 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
-  describe '#hook_attrs' do
-    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)
-
-      subject.hook_attrs
-    end
-  end
-
   describe '#diverged_commits_count' do
     let(:project) { create(:project, :repository) }
     let(:forked_project) { fork_project(project, nil, repository: true) }

spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb

-# frozen_string_literal: true
-
-# This shared example requires a `builder` and `user` variable
-RSpec.shared_examples 'issuable hook data' do |kind|
-  let(:data) { builder.build(user: user) }
-
-  include_examples 'project hook data' do
-    let(:project) { builder.issuable.project }
-  end
-
-  include_examples 'deprecated repository hook data'
-
-  context "with a #{kind}" do
-    it 'contains issuable data' do
-      expect(data[:object_kind]).to eq(kind)
-      expect(data[:user]).to eq(user.hook_attrs)
-      expect(data[:project]).to eq(builder.issuable.project.hook_attrs)
-      expect(data[:object_attributes]).to eq(builder.issuable.hook_attrs)
-      expect(data[:changes]).to eq({})
-      expect(data[:repository]).to eq(builder.issuable.project.hook_attrs.slice(:name, :url, :description, :homepage))
-    end
-
-    it 'does not contain certain keys' do
-      expect(data).not_to have_key(:assignees)
-      expect(data).not_to have_key(:assignee)
-    end
-
-    describe 'changes are given' do
-      let(:changes) do
-        {
-          cached_markdown_version: %w[foo bar],
-          description: ['A description', 'A cool description'],
-          description_html: %w[foo bar],
-          in_progress_merge_commit_sha: %w[foo bar],
-          lock_version: %w[foo bar],
-          merge_jid: %w[foo bar],
-          title: ['A title', 'Hello World'],
-          title_html: %w[foo bar]
-        }
-      end
-
-      let(:data) { builder.build(user: user, changes: changes) }
-
-      it 'populates the :changes hash' do
-        expect(data[:changes]).to match(hash_including({
-          title: { previous: 'A title', current: 'Hello World' },
-          description: { previous: 'A description', current: 'A cool description' }
-        }))
-      end
-
-      it 'does not contain certain keys' do
-        expect(data[:changes]).not_to have_key('cached_markdown_version')
-        expect(data[:changes]).not_to have_key('description_html')
-        expect(data[:changes]).not_to have_key('lock_version')
-        expect(data[:changes]).not_to have_key('title_html')
-        expect(data[:changes]).not_to have_key('in_progress_merge_commit_sha')
-        expect(data[:changes]).not_to have_key('merge_jid')
-      end
-    end
-  end
-end