Introduce new hook data builders for Issue and MergeRequest

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

app/models/issue.rb

@@ -18,36 +18,6 @@ class Issue < ActiveRecord::Base
   DueThisWeek   = DueDateStruct.new('Due This Week', 'week').freeze
   DueThisMonth  = DueDateStruct.new('Due This Month', 'month').freeze
 
-  SAFE_HOOK_ATTRIBUTES = %i[
-    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
-  ].freeze
-
-  SAFE_HOOK_RELATIONS = %i[
-    assignees
-    labels
-  ].freeze
-
   belongs_to :project
   belongs_to :moved_to, class_name: 'Issue'
 
@@ -147,28 +117,8 @@ class Issue < ActiveRecord::Base
               "id DESC")
   end
 
-  def self.safe_hook_attributes
-    SAFE_HOOK_ATTRIBUTES
-  end
-
-  def self.safe_hook_relations
-    SAFE_HOOK_RELATIONS
-  end
-
   def hook_attrs
-    assignee_ids = self.assignee_ids
-
-    attrs = {
-      url: Gitlab::UrlBuilder.build(self),
-      total_time_spent: total_time_spent,
-      human_total_time_spent: human_total_time_spent,
-      human_time_estimate: human_time_estimate,
-      assignee_ids: assignee_ids,
-      assignee_id: assignee_ids.first # This key is deprecated
-    }
-
-    attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
-      .merge!(attrs)
+    Gitlab::HookData::IssueBuilder.new(self).build
   end
 
   # Returns a Hash of attributes to be used for Twitter card metadata

app/models/merge_request.rb

@@ -7,41 +7,6 @@ class MergeRequest < ActiveRecord::Base
   include IgnorableColumn
   include CreatedAtFilterable
 
-  SAFE_HOOK_ATTRIBUTES = %i[
-    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
-  ].freeze
-
-  SAFE_HOOK_RELATIONS = %i[
-    assignee
-    labels
-  ].freeze
-
   ignore_column :locked_at
 
   belongs_to :target_project, class_name: "Project"
@@ -214,28 +179,8 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
-  def self.safe_hook_attributes
-    SAFE_HOOK_ATTRIBUTES
-  end
-
-  def self.safe_hook_relations
-    SAFE_HOOK_RELATIONS
-  end
-
   def hook_attrs
-    attrs = {
-      url: Gitlab::UrlBuilder.build(self),
-      source: source_project.try(:hook_attrs),
-      target: target_project.hook_attrs,
-      last_commit: diff_head_commit&.hook_attrs,
-      work_in_progress: work_in_progress?,
-      total_time_spent: total_time_spent,
-      human_total_time_spent: human_total_time_spent,
-      human_time_estimate: human_time_estimate
-    }
-
-    attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
-      .merge!(attrs)
+    Gitlab::HookData::MergeRequestBuilder.new(self).build
   end
 
   # Returns a Hash of attributes to be used for Twitter card metadata

doc/user/project/integrations/webhooks.md

@@ -270,7 +270,32 @@ X-Gitlab-Event: Issue Hook
   "changes": {
     "updated_by_id": [null, 1],
     "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"],
-    "labels": [["Platform", "bug"], ["API"]]
+    "labels": {
+      "previous": [{
+        "id": 206,
+        "title": "API",
+        "color": "#ffffff",
+        "project_id": 14,
+        "created_at": "2013-12-03T17:15:43Z",
+        "updated_at": "2013-12-03T17:15:43Z",
+        "template": false,
+        "description": "API related issues",
+        "type": "ProjectLabel",
+        "group_id": 41
+      }],
+      "current": [{
+        "id": 205,
+        "title": "Platform",
+        "color": "#123123",
+        "project_id": 14,
+        "created_at": "2013-12-03T17:15:43Z",
+        "updated_at": "2013-12-03T17:15:43Z",
+        "template": false,
+        "description": "Platform related issues",
+        "type": "ProjectLabel",
+        "group_id": 41
+      }]
+    }
   }
 }
 ```
@@ -772,7 +797,32 @@ X-Gitlab-Event: Merge Request Hook
   "changes": {
     "updated_by_id": [null, 1],
     "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"],
-    "labels": [["Platform", "bug"], ["API"]]
+    "labels": {
+      "previous": [{
+        "id": 206,
+        "title": "API",
+        "color": "#ffffff",
+        "project_id": 14,
+        "created_at": "2013-12-03T17:15:43Z",
+        "updated_at": "2013-12-03T17:15:43Z",
+        "template": false,
+        "description": "API related issues",
+        "type": "ProjectLabel",
+        "group_id": 41
+      }],
+      "current": [{
+        "id": 205,
+        "title": "Platform",
+        "color": "#123123",
+        "project_id": 14,
+        "created_at": "2013-12-03T17:15:43Z",
+        "updated_at": "2013-12-03T17:15:43Z",
+        "template": false,
+        "description": "Platform related issues",
+        "type": "ProjectLabel",
+        "group_id": 41
+      }]
+    }
   }
 }
 ```

lib/gitlab/hook_data/issuable_builder.rb

 module Gitlab
   module HookData
     class IssuableBuilder
+      CHANGES_KEYS = %i[previous current].freeze
+
       attr_accessor :issuable
 
       def initialize(issuable)
@@ -14,7 +16,7 @@ module Gitlab
           project: issuable.project.hook_attrs,
           object_attributes: issuable.hook_attrs,
           labels: issuable.labels.map(&:hook_attrs),
-          changes: changes.slice(*safe_keys),
+          changes: final_changes(changes.slice(*safe_keys)),
           # DEPRECATED
           repository: issuable.project.hook_attrs.slice(:name, :url, :description, :homepage)
         }
@@ -29,7 +31,25 @@ module Gitlab
       end
 
       def safe_keys
-        issuable.class.safe_hook_attributes + issuable.class.safe_hook_relations
+        issuable_builder::SAFE_HOOK_ATTRIBUTES + issuable_builder::SAFE_HOOK_RELATIONS
+      end
+
+      private
+
+      def issuable_builder
+        case issuable
+        when Issue
+          Gitlab::HookData::IssueBuilder
+        when MergeRequest
+          Gitlab::HookData::MergeRequestBuilder
+        end
+      end
+
+      def final_changes(changes_hash)
+        changes_hash.reduce({}) do |hash, (key, changes_array)|
+          hash[key] = Hash[CHANGES_KEYS.zip(changes_array)]
+          hash
+        end
       end
     end
   end

lib/gitlab/hook_data/issue_builder.rb

+module Gitlab
+  module HookData
+    class IssueBuilder
+      SAFE_HOOK_ATTRIBUTES = %i[
+        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
+      ].freeze
+
+      SAFE_HOOK_RELATIONS = %i[
+        assignees
+        labels
+      ].freeze
+
+      attr_accessor :issue
+
+      def initialize(issue)
+        @issue = issue
+      end
+
+      def build
+        attrs = {
+          url: Gitlab::UrlBuilder.build(issue),
+          total_time_spent: issue.total_time_spent,
+          human_total_time_spent: issue.human_total_time_spent,
+          human_time_estimate: issue.human_time_estimate,
+          assignee_ids: issue.assignee_ids,
+          assignee_id: issue.assignee_ids.first # This key is deprecated
+        }
+
+        issue.attributes.with_indifferent_access.slice(*SAFE_HOOK_ATTRIBUTES)
+          .merge!(attrs)
+      end
+    end
+  end
+end

lib/gitlab/hook_data/merge_request_builder.rb

+module Gitlab
+  module HookData
+    class MergeRequestBuilder
+      SAFE_HOOK_ATTRIBUTES = %i[
+        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
+      ].freeze
+
+      SAFE_HOOK_RELATIONS = %i[
+        assignee
+        labels
+      ].freeze
+
+      attr_accessor :merge_request
+
+      def initialize(merge_request)
+        @merge_request = merge_request
+      end
+
+      def build
+        attrs = {
+          url: Gitlab::UrlBuilder.build(merge_request),
+          source: merge_request.source_project.try(:hook_attrs),
+          target: merge_request.target_project.hook_attrs,
+          last_commit: merge_request.diff_head_commit&.hook_attrs,
+          work_in_progress: merge_request.work_in_progress?,
+          total_time_spent: merge_request.total_time_spent,
+          human_total_time_spent: merge_request.human_total_time_spent,
+          human_time_estimate: merge_request.human_time_estimate
+        }
+
+        merge_request.attributes.with_indifferent_access.slice(*SAFE_HOOK_ATTRIBUTES)
+          .merge!(attrs)
+      end
+    end
+  end
+end

spec/lib/gitlab/hook_data/issuable_builder_spec.rb

@@ -37,15 +37,23 @@ describe Gitlab::HookData::IssuableBuilder do
             lock_version: %w[foo bar],
             merge_jid: %w[foo bar],
             title: ['A title', 'Hello World'],
-            title_html: %w[foo bar]
+            title_html: %w[foo bar],
+            labels: [
+              [{ id: 1, title: 'foo' }],
+              [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }]
+            ]
           }
         end
         let(:data) { builder.build(user: user, changes: changes) }
 
         it 'populates the :changes hash' do
           expect(data[:changes]).to match(hash_including({
-            title: ['A title', 'Hello World'],
-            description: ['A description', 'A cool description']
+            title: { previous: 'A title', current: 'Hello World' },
+            description: { previous: 'A description', current: 'A cool description' },
+            labels: {
+              previous: [{ id: 1, title: 'foo' }],
+              current: [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }]
+            }
           }))
         end
 
@@ -73,7 +81,7 @@ describe Gitlab::HookData::IssuableBuilder do
     end
 
     context 'issue is assigned' do
-      let(:issue) { create(:issue).tap { |i| i.assignees << user } }
+      let(:issue) { create(:issue, assignees: [user]) }
       let(:data) { described_class.new(issue).build(user: user) }
 
       it 'returns correct hook data' do
@@ -84,7 +92,7 @@ describe Gitlab::HookData::IssuableBuilder do
     end
 
     context 'merge_request is assigned' do
-      let(:merge_request) { create(:merge_request).tap { |mr| mr.update(assignee: user) } }
+      let(:merge_request) { create(:merge_request, assignee: user) }
       let(:data) { described_class.new(merge_request).build(user: user) }
 
       it 'returns correct hook data' do

spec/lib/gitlab/hook_data/issue_builder_spec.rb

+require 'spec_helper'
+
+describe Gitlab::HookData::IssueBuilder do
+  set(:issue) { create(:issue) }
+  let(:builder) { described_class.new(issue) }
+
+  describe '#build' do
+    let(:data) { builder.build }
+
+    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(data).to include(key)
+      end
+    end
+
+    it 'includes additional attrs' do
+      expect(data).to include(:total_time_spent)
+      expect(data).to include(:human_time_estimate)
+      expect(data).to include(:human_total_time_spent)
+      expect(data).to include(:assignee_ids)
+    end
+  end
+end

spec/lib/gitlab/hook_data/merge_request_builder_spec.rb

+require 'spec_helper'
+
+describe Gitlab::HookData::MergeRequestBuilder do
+  set(:merge_request) { create(:merge_request) }
+  let(:builder) { described_class.new(merge_request) }
+
+  describe '#build' do
+    let(:data) { builder.build }
+
+    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(data).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(:project) { merge_request.public_send("#{key}_project") }
+        end
+      end
+    end
+
+    it 'includes additional attrs' do
+      expect(data).to include(:source)
+      expect(data).to include(:target)
+      expect(data).to include(:last_commit)
+      expect(data).to include(:work_in_progress)
+      expect(data).to include(:total_time_spent)
+      expect(data).to include(:human_time_estimate)
+      expect(data).to include(:human_total_time_spent)
+    end
+  end
+end

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

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
 

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 '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)
+    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
 

spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb

+# This shared example requires a `builder` and `user` variable
+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