diff options
author | Rémy Coutable <remy@rymai.me> | 2017-09-15 19:08:27 +0200 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-10-09 15:07:10 +0200 |
commit | 67d5ca9f9220e5572f3fa6d0d8290cd7b802f02f (patch) | |
tree | d424fc71d20847c8a7c0314ff081f3d2ed9afbe4 | |
parent | f277fa14094e5515e2317d2baa1fa0bfb95966da (diff) | |
download | gitlab-ce-67d5ca9f9220e5572f3fa6d0d8290cd7b802f02f.tar.gz |
Include the changes in issuable webhook payloads
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | app/models/concerns/issuable.rb | 8 | ||||
-rw-r--r-- | app/models/issue.rb | 1 | ||||
-rw-r--r-- | app/models/merge_request.rb | 1 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 2 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 14 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 14 | ||||
-rw-r--r-- | app/services/merge_requests/refresh_service.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml | 5 | ||||
-rw-r--r-- | doc/user/project/integrations/webhooks.md | 54 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 35 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb | 58 | ||||
-rw-r--r-- | spec/support/shared_examples/models/project_hook_data_shared_examples.rb (renamed from spec/support/project_hook_data_shared_example.rb) | 6 |
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.  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) |