Include the changes in issuable webhook payloads

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

14 files changed, 157 insertions, 53 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index fc30d008dea..e8a6c37d0b9 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -256,13 +256,19 @@ module Issuable
     participants(user).include?(user)
   end
 
-  def to_hook_data(user)
+  def to_hook_data(user, old_labels: [])
+    changes = previous_changes
+    if old_labels != labels
+      changes[:labels] = [old_labels.map(&:name), labels.map(&:name)]
+    end
+
     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),
+      changes: changes,
       # DEPRECATED
       repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
     }
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 155c5d972b7..058ee144ee4 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -78,6 +78,7 @@ class Issue < ActiveRecord::Base
     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,
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 292122f779e..52a6c31503a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -589,6 +589,7 @@ class MergeRequest < ActiveRecord::Base
 
   def hook_attrs
     attrs = {
+      url: Gitlab::UrlBuilder.build(self),
       source: source_project.try(:hook_attrs),
       target: target_project.hook_attrs,
       last_commit: nil,
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index f83ece7098f..1d6b48ccf56 100644
--- a/app/services/issuable_base_service.rb
+++ b/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)
 
         issuable.update_project_counter_caches if update_project_counters
       end
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 4c198fc96ea..f4fff4bf646 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,10 +1,10 @@
 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: [])
+      hook_data = issue.to_hook_data(current_user, old_labels: old_labels)
+      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: [])
+      issue_data  = hook_data(issue, action, old_labels: old_labels)
       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)
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 35ccff26262..7cdb45ca552 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/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: [])
+      hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels)
       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: [])
       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)
         merge_request.project.execute_hooks(merge_data, :merge_request_hooks)
         merge_request.project.execute_services(merge_data, :merge_request_hooks)
       end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index bc4a13cf4bc..fc100580c4f 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/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
 
diff --git a/changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml b/changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml
new file mode 100644
index 00000000000..816e1f83111
--- /dev/null
+++ b/changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml
@@ -0,0 +1,5 @@
+---
+title: Include the changes in issuable webhook payloads
+merge_request: 14308
+author:
+type: added
diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md
index 47eb0b34f66..3b2ce8d722c 100644
--- a/doc/user/project/integrations/webhooks.md
+++ b/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,12 @@ 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": [["Platform", "bug"], ["API"]]
+  }
 }
 ```
 
@@ -661,6 +666,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 +706,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 +756,23 @@ 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": [["Platform", "bug"], ["API"]]
   }
 }
 ```
@@ -1015,7 +1059,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)
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index fb5fb7daaab..5763e1ba6d4 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/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
@@ -265,17 +265,17 @@ describe Issuable do
   end
 
   describe "#to_hook_data" do
-    let(:data) { issue.to_hook_data(user) }
-    let(:project) { issue.project }
+    it_behaves_like 'issuable hook data', 'issue' do
+      let(:issuable) { create(:issue, description: 'A description') }
+    end
 
-    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)
+    it_behaves_like 'issuable hook data', 'merge_request' do
+      let(:issuable) { create(:merge_request, description: 'A description') }
     end
 
     context "issue is assigned" do
+      let(:data) { issue.to_hook_data(user) }
+
       before do
         issue.assignees << user
       end
@@ -296,23 +296,12 @@ describe Issuable do
       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[:changes]).to match(hash_including({
+          'assignee_id' => [nil, user.id],
+          'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)]
+        }))
       end
     end
-
-    context 'issue has labels' do
-      let(:labels) { [create(:label), create(:label)] }
-
-      before do
-        issue.update_attribute(:labels, labels)
-      end
-
-      it 'includes labels in the hook data' do
-        expect(data[:labels]).to eq(labels.map(&:hook_attrs))
-      end
-    end
-
-    include_examples 'project hook data'
-    include_examples 'deprecated repository hook data'
   end
 
   describe '#labels_array' do
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 62dbe362ec8..a2c05761f6b 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/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')
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index b11a1b31f32..3a8cb4ce83d 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -79,7 +79,7 @@ describe MergeRequests::UpdateService, :mailer do
 
       it 'executes hooks with update action' do
         expect(service).to have_received(:execute_hooks)
-                               .with(@merge_request, 'update')
+                               .with(@merge_request, 'update', old_labels: [])
       end
 
       it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do
diff --git a/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb b/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb
new file mode 100644
index 00000000000..ed1d2f41eae
--- /dev/null
+++ b/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb
@@ -0,0 +1,58 @@
+# This shared example requires a `user` variable
+shared_examples 'issuable hook data' do |kind|
+  let(:data) { issuable.to_hook_data(user) }
+
+  include_examples 'project hook data' do
+    let(:project) { 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[:object_attributes]).to eq(issuable.hook_attrs)
+      expect(data[:changes]).to match(hash_including({
+        'author_id' => [nil, issuable.author_id],
+        'cached_markdown_version' => [nil, issuable.cached_markdown_version],
+        'created_at' => [nil, a_kind_of(ActiveSupport::TimeWithZone)],
+        'description' => [nil, issuable.description],
+        'description_html' => [nil, issuable.description_html],
+        'id' => [nil, issuable.id],
+        'iid' => [nil, issuable.iid],
+        'state' => [nil, issuable.state],
+        'title' => [nil, issuable.title],
+        'title_html' => [nil, issuable.title_html],
+        'updated_at' => [nil, a_kind_of(ActiveSupport::TimeWithZone)]
+      }))
+      expect(data).not_to have_key(:assignee)
+    end
+
+    describe 'simple attributes are updated' do
+      before do
+        issuable.update(title: 'Hello World', description: 'A cool description')
+      end
+
+      it 'includes an empty :changes hash' do
+        expect(data[:changes]).to match(hash_including({
+          'title' => [issuable.previous_changes['title'][0], 'Hello World'],
+          'description' => [issuable.previous_changes['description'][0], 'A cool description'],
+          'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)]
+        }))
+      end
+    end
+
+    context "#{kind} has labels" do
+      let(:labels) { [create(:label), create(:label)] }
+
+      before do
+        issuable.update_attribute(:labels, labels)
+      end
+
+      it 'includes labels in the hook data' do
+        expect(data[:labels]).to eq(labels.map(&:hook_attrs))
+        expect(data[:changes]).to eq({ 'labels' => [[], labels.map(&:name)] })
+      end
+    end
+  end
+end
diff --git a/spec/support/project_hook_data_shared_example.rb b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb
index 1eb405d4be8..f0264878811 100644
--- a/spec/support/project_hook_data_shared_example.rb
+++ b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb
@@ -1,4 +1,4 @@
-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)