Commit 67d5ca9f authored by Rémy Coutable's avatar Rémy Coutable

Include the changes in issuable webhook payloads

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent f277fa14
...@@ -256,13 +256,19 @@ module Issuable ...@@ -256,13 +256,19 @@ module Issuable
participants(user).include?(user) participants(user).include?(user)
end 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 = { hook_data = {
object_kind: self.class.name.underscore, object_kind: self.class.name.underscore,
user: user.hook_attrs, user: user.hook_attrs,
project: project.hook_attrs, project: project.hook_attrs,
object_attributes: hook_attrs, object_attributes: hook_attrs,
labels: labels.map(&:hook_attrs), labels: labels.map(&:hook_attrs),
changes: changes,
# DEPRECATED # DEPRECATED
repository: project.hook_attrs.slice(:name, :url, :description, :homepage) repository: project.hook_attrs.slice(:name, :url, :description, :homepage)
} }
......
...@@ -78,6 +78,7 @@ class Issue < ActiveRecord::Base ...@@ -78,6 +78,7 @@ class Issue < ActiveRecord::Base
assignee_ids = self.assignee_ids assignee_ids = self.assignee_ids
attrs = { attrs = {
url: Gitlab::UrlBuilder.build(self),
total_time_spent: total_time_spent, total_time_spent: total_time_spent,
human_total_time_spent: human_total_time_spent, human_total_time_spent: human_total_time_spent,
human_time_estimate: human_time_estimate, human_time_estimate: human_time_estimate,
......
...@@ -589,6 +589,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -589,6 +589,7 @@ class MergeRequest < ActiveRecord::Base
def hook_attrs def hook_attrs
attrs = { attrs = {
url: Gitlab::UrlBuilder.build(self),
source: source_project.try(:hook_attrs), source: source_project.try(:hook_attrs),
target: target_project.hook_attrs, target: target_project.hook_attrs,
last_commit: nil, last_commit: nil,
......
...@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService ...@@ -255,7 +255,7 @@ class IssuableBaseService < BaseService
invalidate_cache_counts(issuable, users: affected_assignees.compact) invalidate_cache_counts(issuable, users: affected_assignees.compact)
after_update(issuable) after_update(issuable)
issuable.create_new_cross_references!(current_user) 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 issuable.update_project_counter_caches if update_project_counters
end end
......
module Issues module Issues
class BaseService < ::IssuableBaseService class BaseService < ::IssuableBaseService
def hook_data(issue, action) def hook_data(issue, action, old_labels: [])
issue_data = issue.to_hook_data(current_user) hook_data = issue.to_hook_data(current_user, old_labels: old_labels)
issue_url = Gitlab::UrlBuilder.build(issue) hook_data[:object_attributes][:action] = action
issue_data[:object_attributes].merge!(url: issue_url, action: action)
issue_data hook_data
end end
def reopen_service def reopen_service
...@@ -22,8 +22,8 @@ module Issues ...@@ -22,8 +22,8 @@ module Issues
issue, issue.project, current_user, old_assignees) issue, issue.project, current_user, old_assignees)
end end
def execute_hooks(issue, action = 'open') def execute_hooks(issue, action = 'open', old_labels: [])
issue_data = hook_data(issue, action) issue_data = hook_data(issue, action, old_labels: old_labels)
hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
issue.project.execute_hooks(issue_data, hooks_scope) issue.project.execute_hooks(issue_data, hooks_scope)
issue.project.execute_services(issue_data, hooks_scope) issue.project.execute_services(issue_data, hooks_scope)
......
...@@ -18,19 +18,19 @@ module MergeRequests ...@@ -18,19 +18,19 @@ module MergeRequests
super if changed_title super if changed_title
end end
def hook_data(merge_request, action, oldrev = nil) def hook_data(merge_request, action, old_rev: nil, old_labels: [])
hook_data = merge_request.to_hook_data(current_user) hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels)
hook_data[:object_attributes][:url] = Gitlab::UrlBuilder.build(merge_request)
hook_data[:object_attributes][:action] = action hook_data[:object_attributes][:action] = action
if oldrev && !Gitlab::Git.blank_ref?(oldrev) if old_rev && !Gitlab::Git.blank_ref?(old_rev)
hook_data[:object_attributes][:oldrev] = oldrev hook_data[:object_attributes][:oldrev] = old_rev
end end
hook_data hook_data
end 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 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_hooks(merge_data, :merge_request_hooks)
merge_request.project.execute_services(merge_data, :merge_request_hooks) merge_request.project.execute_services(merge_data, :merge_request_hooks)
end end
......
...@@ -166,7 +166,7 @@ module MergeRequests ...@@ -166,7 +166,7 @@ module MergeRequests
# Call merge request webhook with update branches # Call merge request webhook with update branches
def execute_mr_web_hooks def execute_mr_web_hooks
merge_requests_for_source_branch.each do |merge_request| 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
end end
......
---
title: Include the changes in issuable webhook payloads
merge_request: 14308
author:
type: added
...@@ -205,7 +205,7 @@ X-Gitlab-Event: Issue Hook ...@@ -205,7 +205,7 @@ X-Gitlab-Event: Issue Hook
"username": "root", "username": "root",
"avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
}, },
"project":{ "project": {
"name":"Gitlab Test", "name":"Gitlab Test",
"description":"Aut reprehenderit ut est.", "description":"Aut reprehenderit ut est.",
"web_url":"http://example.com/gitlabhq/gitlab-test", "web_url":"http://example.com/gitlabhq/gitlab-test",
...@@ -221,7 +221,7 @@ X-Gitlab-Event: Issue Hook ...@@ -221,7 +221,7 @@ X-Gitlab-Event: Issue Hook
"ssh_url":"git@example.com:gitlabhq/gitlab-test.git", "ssh_url":"git@example.com:gitlabhq/gitlab-test.git",
"http_url":"http://example.com/gitlabhq/gitlab-test.git" "http_url":"http://example.com/gitlabhq/gitlab-test.git"
}, },
"repository":{ "repository": {
"name": "Gitlab Test", "name": "Gitlab Test",
"url": "http://example.com/gitlabhq/gitlab-test.git", "url": "http://example.com/gitlabhq/gitlab-test.git",
"description": "Aut reprehenderit ut est.", "description": "Aut reprehenderit ut est.",
...@@ -266,7 +266,12 @@ X-Gitlab-Event: Issue Hook ...@@ -266,7 +266,12 @@ X-Gitlab-Event: Issue Hook
"description": "API related issues", "description": "API related issues",
"type": "ProjectLabel", "type": "ProjectLabel",
"group_id": 41 "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 ...@@ -661,6 +666,28 @@ X-Gitlab-Event: Merge Request Hook
"username": "root", "username": "root",
"avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" "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": { "object_attributes": {
"id": 99, "id": 99,
"target_branch": "master", "target_branch": "master",
...@@ -679,7 +706,7 @@ X-Gitlab-Event: Merge Request Hook ...@@ -679,7 +706,7 @@ X-Gitlab-Event: Merge Request Hook
"target_project_id": 14, "target_project_id": 14,
"iid": 1, "iid": 1,
"description": "", "description": "",
"source":{ "source": {
"name":"Awesome Project", "name":"Awesome Project",
"description":"Aut reprehenderit ut est.", "description":"Aut reprehenderit ut est.",
"web_url":"http://example.com/awesome_space/awesome_project", "web_url":"http://example.com/awesome_space/awesome_project",
...@@ -729,6 +756,23 @@ X-Gitlab-Event: Merge Request Hook ...@@ -729,6 +756,23 @@ X-Gitlab-Event: Merge Request Hook
"username": "user1", "username": "user1",
"avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon" "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 ...@@ -1015,7 +1059,7 @@ X-Gitlab-Event: Build Hook
## Testing webhooks ## 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. > For example: for triggering `Push Events` your project should have at least one commit.
![Webhook testing](img/webhook_testing.png) ![Webhook testing](img/webhook_testing.png)
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Issuable do describe Issuable do
let(:issuable_class) { Issue } let(:issuable_class) { Issue }
let(:issue) { create(:issue) } let(:issue) { create(:issue, title: 'An issue', description: 'A description') }
let(:user) { create(:user) } let(:user) { create(:user) }
describe "Associations" do describe "Associations" do
...@@ -265,17 +265,17 @@ describe Issuable do ...@@ -265,17 +265,17 @@ describe Issuable do
end end
describe "#to_hook_data" do describe "#to_hook_data" do
let(:data) { issue.to_hook_data(user) } it_behaves_like 'issuable hook data', 'issue' do
let(:project) { issue.project } let(:issuable) { create(:issue, description: 'A description') }
end
it "returns correct hook data" do it_behaves_like 'issuable hook data', 'merge_request' do
expect(data[:object_kind]).to eq("issue") let(:issuable) { create(:merge_request, description: 'A description') }
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 end
context "issue is assigned" do context "issue is assigned" do
let(:data) { issue.to_hook_data(user) }
before do before do
issue.assignees << user issue.assignees << user
end end
...@@ -296,23 +296,12 @@ describe Issuable do ...@@ -296,23 +296,12 @@ describe Issuable do
it "returns correct hook data" do it "returns correct hook data" do
expect(data[:object_attributes]['assignee_id']).to eq(user.id) expect(data[:object_attributes]['assignee_id']).to eq(user.id)
expect(data[:assignee]).to eq(user.hook_attrs) 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
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 end
describe '#labels_array' do describe '#labels_array' do
......
...@@ -61,7 +61,7 @@ describe MergeRequests::RefreshService do ...@@ -61,7 +61,7 @@ describe MergeRequests::RefreshService do
it 'executes hooks with update action' do it 'executes hooks with update action' do
expect(refresh_service).to have_received(:execute_hooks) 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.notes).not_to be_empty
expect(@merge_request).to be_open expect(@merge_request).to be_open
...@@ -87,7 +87,7 @@ describe MergeRequests::RefreshService do ...@@ -87,7 +87,7 @@ describe MergeRequests::RefreshService do
it 'executes hooks with update action' do it 'executes hooks with update action' do
expect(refresh_service).to have_received(:execute_hooks) 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.notes).not_to be_empty
expect(@merge_request).to be_open expect(@merge_request).to be_open
...@@ -182,7 +182,7 @@ describe MergeRequests::RefreshService do ...@@ -182,7 +182,7 @@ describe MergeRequests::RefreshService do
it 'executes hooks with update action' do it 'executes hooks with update action' do
expect(refresh_service).to have_received(:execute_hooks) 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.notes).to be_empty
expect(@merge_request).to be_open expect(@merge_request).to be_open
...@@ -264,7 +264,7 @@ describe MergeRequests::RefreshService do ...@@ -264,7 +264,7 @@ describe MergeRequests::RefreshService do
it 'refreshes the merge request' do it 'refreshes the merge request' do
expect(refresh_service).to receive(:execute_hooks) 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) allow_any_instance_of(Repository).to receive(:merge_base).and_return(@oldrev)
refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master') refresh_service.execute(Gitlab::Git::BLANK_SHA, @newrev, 'refs/heads/master')
......
...@@ -79,7 +79,7 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -79,7 +79,7 @@ describe MergeRequests::UpdateService, :mailer do
it 'executes hooks with update action' do it 'executes hooks with update action' do
expect(service).to have_received(:execute_hooks) expect(service).to have_received(:execute_hooks)
.with(@merge_request, 'update') .with(@merge_request, 'update', old_labels: [])
end end
it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do it 'sends email to user2 about assign of new merge request and email to user3 about merge request unassignment' do
......
# 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
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 it 'contains project data' do
expect(data[project_key][:name]).to eq(project.name) expect(data[project_key][:name]).to eq(project.name)
expect(data[project_key][:description]).to eq(project.description) 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 ...@@ -17,7 +17,7 @@ RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :pro
end end
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 it 'contains project data' do
expect(data[project_key][:name]).to eq(project.name) expect(data[project_key][:name]).to eq(project.name)
expect(data[project_key][:description]).to eq(project.description) expect(data[project_key][:description]).to eq(project.description)
...@@ -32,7 +32,7 @@ RSpec.shared_examples 'project hook data' do |project_key: :project| ...@@ -32,7 +32,7 @@ RSpec.shared_examples 'project hook data' do |project_key: :project|
end end
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 it 'contains deprecated repository data' do
expect(data[:repository][:name]).to eq(project.name) expect(data[:repository][:name]).to eq(project.name)
expect(data[:repository][:description]).to eq(project.description) expect(data[:repository][:description]).to eq(project.description)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment