Merge branch '34284-add-changes-to-issuable-webhook-data' into 'master'

Include the changes in issuable webhook payloads

Closes #34284

See merge request gitlab-org/gitlab-ce!14308

app/models/concerns/issuable.rb

@@ -256,23 +256,22 @@ module Issuable
     participants(user).include?(user)
   end
 
-  def to_hook_data(user)
-    hook_data = {
-      object_kind: self.class.name.underscore,
-      user: user.hook_attrs,
-      project: project.hook_attrs,
-      object_attributes: hook_attrs,
-      labels: labels.map(&:hook_attrs),
-      # DEPRECATED
-      repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
-    }
-    if self.is_a?(Issue)
-      hook_data[:assignees] = assignees.map(&:hook_attrs) if assignees.any?
-    else
-      hook_data[:assignee] = assignee.hook_attrs if assignee
+  def to_hook_data(user, old_labels: [], old_assignees: [])
+    changes = previous_changes
+
+    if old_labels != labels
+      changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
+    end
+
+    if old_assignees != assignees
+      if self.is_a?(Issue)
+        changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
+      else
+        changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
+      end
     end
 
-    hook_data
+    Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
   end
 
   def labels_array

app/models/issue.rb

@@ -74,20 +74,6 @@ class Issue < ActiveRecord::Base
     end
   end
 
-  def hook_attrs
-    assignee_ids = self.assignee_ids
-
-    attrs = {
-      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.merge!(attrs)
-  end
-
   def self.reference_prefix
     '#'
   end
@@ -131,6 +117,10 @@ class Issue < ActiveRecord::Base
               "id DESC")
   end
 
+  def hook_attrs
+    Gitlab::HookData::IssueBuilder.new(self).build
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {

app/models/merge_request.rb

@@ -179,6 +179,10 @@ class MergeRequest < ActiveRecord::Base
     work_in_progress?(title) ? title : "WIP: #{title}"
   end
 
+  def hook_attrs
+    Gitlab::HookData::MergeRequestBuilder.new(self).build
+  end
+
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {
@@ -587,24 +591,6 @@ class MergeRequest < ActiveRecord::Base
     !discussions_to_be_resolved?
   end
 
-  def hook_attrs
-    attrs = {
-      source: source_project.try(:hook_attrs),
-      target: target_project.hook_attrs,
-      last_commit: nil,
-      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
-    }
-
-    if diff_head_commit
-      attrs[:last_commit] = diff_head_commit.hook_attrs
-    end
-
-    attributes.merge!(attrs)
-  end
-
   def for_fork?
     target_project != source_project
   end

app/services/issuable_base_service.rb

@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService
         invalidate_cache_counts(issuable, users: affected_assignees.compact)
         after_update(issuable)
         issuable.create_new_cross_references!(current_user)
-        execute_hooks(issuable, 'update')
+        execute_hooks(issuable, 'update', old_labels: old_labels, old_assignees: old_assignees)
 
         issuable.update_project_counter_caches if update_project_counters
       end

app/services/issues/base_service.rb

 module Issues
   class BaseService < ::IssuableBaseService
-    def hook_data(issue, action)
-      issue_data = issue.to_hook_data(current_user)
-      issue_url = Gitlab::UrlBuilder.build(issue)
-      issue_data[:object_attributes].merge!(url: issue_url, action: action)
-      issue_data
+    def hook_data(issue, action, old_labels: [], old_assignees: [])
+      hook_data = issue.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
+      hook_data[:object_attributes][:action] = action
+
+      hook_data
     end
 
     def reopen_service
@@ -22,8 +22,8 @@ module Issues
         issue, issue.project, current_user, old_assignees)
     end
 
-    def execute_hooks(issue, action = 'open')
-      issue_data  = hook_data(issue, action)
+    def execute_hooks(issue, action = 'open', old_labels: [], old_assignees: [])
+      issue_data  = hook_data(issue, action, old_labels: old_labels, old_assignees: old_assignees)
       hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
       issue.project.execute_hooks(issue_data, hooks_scope)
       issue.project.execute_services(issue_data, hooks_scope)

app/services/merge_requests/base_service.rb

@@ -18,19 +18,19 @@ module MergeRequests
       super if changed_title
     end
 
-    def hook_data(merge_request, action, oldrev = nil)
-      hook_data = merge_request.to_hook_data(current_user)
-      hook_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(merge_request)
+    def hook_data(merge_request, action, old_rev: nil, old_labels: [], old_assignees: [])
+      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels, old_assignees: old_assignees)
       hook_data[:object_attributes][:action] = action
-      if oldrev && !Gitlab::Git.blank_ref?(oldrev)
-        hook_data[:object_attributes][:oldrev] = oldrev
+      if old_rev && !Gitlab::Git.blank_ref?(old_rev)
+        hook_data[:object_attributes][:oldrev] = old_rev
       end
+
       hook_data
     end
 
-    def execute_hooks(merge_request, action = 'open', oldrev = nil)
+    def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: [], old_assignees: [])
       if merge_request.project
-        merge_data = hook_data(merge_request, action, oldrev)
+        merge_data = hook_data(merge_request, action, old_rev: old_rev, old_labels: old_labels, old_assignees: old_assignees)
         merge_request.project.execute_hooks(merge_data, :merge_request_hooks)
         merge_request.project.execute_services(merge_data, :merge_request_hooks)
       end

app/services/merge_requests/refresh_service.rb

@@ -166,7 +166,7 @@ module MergeRequests
     # Call merge request webhook with update branches
     def execute_mr_web_hooks
       merge_requests_for_source_branch.each do |merge_request|
-        execute_hooks(merge_request, 'update', @oldrev)
+        execute_hooks(merge_request, 'update', old_rev: @oldrev)
       end
     end
 

changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml

+---
+title: Include the changes in issuable webhook payloads
+merge_request: 14308
+author:
+type: added

doc/user/project/integrations/webhooks.md

@@ -205,7 +205,7 @@ X-Gitlab-Event: Issue Hook
     "username": "root",
     "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
   },
-  "project":{
+  "project": {
     "name":"Gitlab Test",
     "description":"Aut reprehenderit ut est.",
     "web_url":"http://example.com/gitlabhq/gitlab-test",
@@ -221,7 +221,7 @@ X-Gitlab-Event: Issue Hook
     "ssh_url":"git@example.com:gitlabhq/gitlab-test.git",
     "http_url":"http://example.com/gitlabhq/gitlab-test.git"
   },
-  "repository":{
+  "repository": {
     "name": "Gitlab Test",
     "url": "http://example.com/gitlabhq/gitlab-test.git",
     "description": "Aut reprehenderit ut est.",
@@ -266,7 +266,37 @@ X-Gitlab-Event: Issue Hook
     "description": "API related issues",
     "type": "ProjectLabel",
     "group_id": 41
-  }]
+  }],
+  "changes": {
+    "updated_by_id": [null, 1],
+    "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"],
+    "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
+      }]
+    }
+  }
 }
 ```
 
@@ -661,6 +691,28 @@ X-Gitlab-Event: Merge Request Hook
     "username": "root",
     "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
   },
+  "project": {
+    "name":"Gitlab Test",
+    "description":"Aut reprehenderit ut est.",
+    "web_url":"http://example.com/gitlabhq/gitlab-test",
+    "avatar_url":null,
+    "git_ssh_url":"git@example.com:gitlabhq/gitlab-test.git",
+    "git_http_url":"http://example.com/gitlabhq/gitlab-test.git",
+    "namespace":"GitlabHQ",
+    "visibility_level":20,
+    "path_with_namespace":"gitlabhq/gitlab-test",
+    "default_branch":"master",
+    "homepage":"http://example.com/gitlabhq/gitlab-test",
+    "url":"http://example.com/gitlabhq/gitlab-test.git",
+    "ssh_url":"git@example.com:gitlabhq/gitlab-test.git",
+    "http_url":"http://example.com/gitlabhq/gitlab-test.git"
+  },
+  "repository": {
+    "name": "Gitlab Test",
+    "url": "http://example.com/gitlabhq/gitlab-test.git",
+    "description": "Aut reprehenderit ut est.",
+    "homepage": "http://example.com/gitlabhq/gitlab-test"
+  },
   "object_attributes": {
     "id": 99,
     "target_branch": "master",
@@ -679,7 +731,7 @@ X-Gitlab-Event: Merge Request Hook
     "target_project_id": 14,
     "iid": 1,
     "description": "",
-    "source":{
+    "source": {
       "name":"Awesome Project",
       "description":"Aut reprehenderit ut est.",
       "web_url":"http://example.com/awesome_space/awesome_project",
@@ -729,6 +781,48 @@ X-Gitlab-Event: Merge Request Hook
       "username": "user1",
       "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
     }
+  },
+  "labels": [{
+    "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
+  }],
+  "changes": {
+    "updated_by_id": [null, 1],
+    "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"],
+    "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
+      }]
+    }
   }
 }
 ```
@@ -1015,7 +1109,7 @@ X-Gitlab-Event: Build Hook
 
 ## Testing webhooks
 
-You can trigger the webhook manually. Sample data from the project will be used.Sample data will take from the project. 
+You can trigger the webhook manually. Sample data from the project will be used.Sample data will take from the project.
 > For example: for triggering `Push Events` your project should have at least one commit.
 
 ![Webhook testing](img/webhook_testing.png)

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)
+        @issuable = issuable
+      end
+
+      def build(user: nil, changes: {})
+        hook_data = {
+          object_kind: issuable.class.name.underscore,
+          user: user.hook_attrs,
+          project: issuable.project.hook_attrs,
+          object_attributes: issuable.hook_attrs,
+          labels: issuable.labels.map(&:hook_attrs),
+          changes: final_changes(changes.slice(*safe_keys)),
+          # DEPRECATED
+          repository: issuable.project.hook_attrs.slice(:name, :url, :description, :homepage)
+        }
+
+        if issuable.is_a?(Issue)
+          hook_data[:assignees] = issuable.assignees.map(&:hook_attrs) if issuable.assignees.any?
+        else
+          hook_data[:assignee] = issuable.assignee.hook_attrs if issuable.assignee
+        end
+
+        hook_data
+      end
+
+      def safe_keys
+        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
+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

+require 'spec_helper'
+
+describe Gitlab::HookData::IssuableBuilder do
+  set(:user) { create(:user) }
+
+  # 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],
+            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: { 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
+
+        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
+
+  describe '#build' do
+    it_behaves_like 'issuable hook data', 'issue' do
+      let(:issuable) { create(:issue, description: 'A description') }
+      let(:builder) { described_class.new(issuable) }
+    end
+
+    it_behaves_like 'issuable hook data', 'merge_request' do
+      let(:issuable) { create(:merge_request, description: 'A description') }
+      let(:builder) { described_class.new(issuable) }
+    end
+
+    context 'issue is assigned' do
+      let(:issue) { create(:issue, assignees: [user]) }
+      let(:data) { described_class.new(issue).build(user: user) }
+
+      it 'returns correct hook data' do
+        expect(data[:object_attributes]['assignee_id']).to eq(user.id)
+        expect(data[:assignees].first).to eq(user.hook_attrs)
+        expect(data).not_to have_key(:assignee)
+      end
+    end
+
+    context 'merge_request is assigned' do
+      let(:merge_request) { create(:merge_request, assignee: user) }
+      let(:data) { described_class.new(merge_request).build(user: user) }
+
+      it 'returns correct hook data' do
+        expect(data[:object_attributes]['assignee_id']).to eq(user.id)
+        expect(data[:assignee]).to eq(user.hook_attrs)
+        expect(data).not_to have_key(:assignees)
+      end
+    end
+  end
+end

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

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Issuable do
   let(:issuable_class) { Issue }
-  let(:issue) { create(:issue) }
+  let(:issue) { create(:issue, title: 'An issue', description: 'A description') }
   let(:user) { create(:user) }
 
   describe "Associations" do
@@ -264,55 +264,75 @@ describe Issuable do
     end
   end
 
-  describe "#to_hook_data" do
-    let(:data) { issue.to_hook_data(user) }
-    let(:project) { issue.project }
-
-    it "returns correct hook data" do
-      expect(data[:object_kind]).to eq("issue")
-      expect(data[:user]).to eq(user.hook_attrs)
-      expect(data[:object_attributes]).to eq(issue.hook_attrs)
-      expect(data).not_to have_key(:assignee)
-    end
+  describe '#to_hook_data' do
+    context 'labels are updated' do
+      let(:labels) { create_list(:label, 2) }
 
-    context "issue is assigned" do
       before do
-        issue.assignees << user
+        issue.update(labels: [labels[1]])
       end
 
-      it "returns correct hook data" do
-        expect(data[:assignees].first).to eq(user.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 "merge_request is assigned" do
-      let(:merge_request) { create(:merge_request) }
-      let(:data) { merge_request.to_hook_data(user) }
+    context 'issue is assigned' do
+      let(:user2) { create(:user) }
 
       before do
-        merge_request.update_attribute(:assignee, user)
+        issue.assignees << user << user2
       end
 
-      it "returns correct hook data" do
-        expect(data[:object_attributes]['assignee_id']).to eq(user.id)
-        expect(data[:assignee]).to eq(user.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 'issue has labels' do
-      let(:labels) { [create(:label), create(:label)] }
+    context 'merge_request is assigned' do
+      let(:merge_request) { create(:merge_request) }
+      let(:user2) { create(:user) }
 
       before do
-        issue.update_attribute(:labels, labels)
+        merge_request.update(assignee: user)
+        merge_request.update(assignee: user2)
       end
 
-      it 'includes labels in the hook data' do
-        expect(data[:labels]).to eq(labels.map(&: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
-
-    include_examples 'project hook data'
-    include_examples 'deprecated repository hook data'
   end
 
   describe '#labels_array' do

spec/models/issue_spec.rb

@@ -700,18 +700,14 @@ describe Issue do
   end
 
   describe '#hook_attrs' do
-    let(:attrs_hash) { subject.hook_attrs }
+    it 'delegates to Gitlab::HookData::IssueBuilder#build' do
+      builder = double
 
-    it 'includes time tracking 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('time_estimate')
-    end
+      expect(Gitlab::HookData::IssueBuilder)
+        .to receive(:new).with(subject).and_return(builder)
+      expect(builder).to receive(:build)
 
-    it 'includes assignee_ids and deprecated assignee_id' do
-      expect(attrs_hash).to include(:assignee_id)
-      expect(attrs_hash).to include(:assignee_ids)
+      subject.hook_attrs
     end
   end
 

spec/models/merge_request_spec.rb

@@ -660,27 +660,15 @@ describe MergeRequest do
     end
   end
 
-  describe "#hook_attrs" do
-    let(:attrs_hash) { subject.hook_attrs }
-
-    [: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.send("#{key}_project") }
-        end
-      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)
 
-    it "has all the required keys" 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)
-      expect(attrs_hash).to include('time_estimate')
+      subject.hook_attrs
     end
   end
 

spec/services/merge_requests/refresh_service_spec.rb

@@ -61,7 +61,7 @@ describe MergeRequests::RefreshService do
 
       it 'executes hooks with update action' do
         expect(refresh_service).to have_received(:execute_hooks)
-          .with(@merge_request, 'update', @oldrev)
+          .with(@merge_request, 'update', old_rev: @oldrev)
 
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open
@@ -87,7 +87,7 @@ describe MergeRequests::RefreshService do
 
       it 'executes hooks with update action' do
         expect(refresh_service).to have_received(:execute_hooks)
-          .with(@merge_request, 'update', @oldrev)
+          .with(@merge_request, 'update', old_rev: @oldrev)
 
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open
@@ -182,7 +182,7 @@ describe MergeRequests::RefreshService do
 
         it 'executes hooks with update action' do
           expect(refresh_service).to have_received(:execute_hooks)
-            .with(@fork_merge_request, 'update', @oldrev)
+            .with(@fork_merge_request, 'update', old_rev: @oldrev)
 
           expect(@merge_request.notes).to be_empty
           expect(@merge_request).to be_open
@@ -264,7 +264,7 @@ describe MergeRequests::RefreshService do
 
       it 'refreshes the merge request' do
         expect(refresh_service).to receive(:execute_hooks)
-                                       .with(@fork_merge_request, 'update', Gitlab::Git::BLANK_SHA)
+                                       .with(@fork_merge_request, 'update', old_rev: Gitlab::Git::BLANK_SHA)
         allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev)
 
         refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master')

spec/services/merge_requests/update_service_spec.rb

@@ -78,8 +78,9 @@ describe MergeRequests::UpdateService, :mailer do
       end
 
       it 'executes hooks with update action' do
-        expect(service).to have_received(:execute_hooks)
-                               .with(@merge_request, 'update')
+        expect(service)
+          .to have_received(:execute_hooks)
+                .with(@merge_request, 'update', old_labels: [], old_assignees: [user3])
       end
 
       it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do

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

spec/support/project_hook_data_shared_example.rb → spec/support/shared_examples/models/project_hook_data_shared_examples.rb

-RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :project|
+shared_examples 'project hook data with deprecateds' do |project_key: :project|
   it 'contains project data' do
     expect(data[project_key][:name]).to eq(project.name)
     expect(data[project_key][:description]).to eq(project.description)
@@ -17,7 +17,7 @@ RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :pro
   end
 end
 
-RSpec.shared_examples 'project hook data' do |project_key: :project|
+shared_examples 'project hook data' do |project_key: :project|
   it 'contains project data' do
     expect(data[project_key][:name]).to eq(project.name)
     expect(data[project_key][:description]).to eq(project.description)
@@ -32,7 +32,7 @@ RSpec.shared_examples 'project hook data' do |project_key: :project|
   end
 end
 
-RSpec.shared_examples 'deprecated repository hook data' do |project_key: :project|
+shared_examples 'deprecated repository hook data' do
   it 'contains deprecated repository data' do
     expect(data[:repository][:name]).to eq(project.name)
     expect(data[:repository][:description]).to eq(project.description)