From 67d5ca9f9220e5572f3fa6d0d8290cd7b802f02f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Sep 2017 19:08:27 +0200 Subject: Include the changes in issuable webhook payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/issuable.rb | 8 ++- app/models/issue.rb | 1 + app/models/merge_request.rb | 1 + app/services/issuable_base_service.rb | 2 +- app/services/issues/base_service.rb | 14 +++--- app/services/merge_requests/base_service.rb | 14 +++--- app/services/merge_requests/refresh_service.rb | 2 +- .../34284-add-changes-to-issuable-webhook-data.yml | 5 ++ doc/user/project/integrations/webhooks.md | 54 ++++++++++++++++++-- spec/models/concerns/issuable_spec.rb | 35 +++++-------- .../merge_requests/refresh_service_spec.rb | 8 +-- .../services/merge_requests/update_service_spec.rb | 2 +- spec/support/project_hook_data_shared_example.rb | 42 ---------------- .../models/issuable_hook_data_shared_examples.rb | 58 ++++++++++++++++++++++ .../models/project_hook_data_shared_examples.rb | 42 ++++++++++++++++ 15 files changed, 196 insertions(+), 92 deletions(-) create mode 100644 changelogs/unreleased/34284-add-changes-to-issuable-webhook-data.yml delete mode 100644 spec/support/project_hook_data_shared_example.rb create mode 100644 spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb create mode 100644 spec/support/shared_examples/models/project_hook_data_shared_examples.rb 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/project_hook_data_shared_example.rb b/spec/support/project_hook_data_shared_example.rb deleted file mode 100644 index 1eb405d4be8..00000000000 --- a/spec/support/project_hook_data_shared_example.rb +++ /dev/null @@ -1,42 +0,0 @@ -RSpec.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) - expect(data[project_key][:web_url]).to eq(project.web_url) - expect(data[project_key][:avatar_url]).to eq(project.avatar_url) - expect(data[project_key][:git_http_url]).to eq(project.http_url_to_repo) - expect(data[project_key][:git_ssh_url]).to eq(project.ssh_url_to_repo) - expect(data[project_key][:namespace]).to eq(project.namespace.name) - expect(data[project_key][:visibility_level]).to eq(project.visibility_level) - expect(data[project_key][:path_with_namespace]).to eq(project.full_path) - expect(data[project_key][:default_branch]).to eq(project.default_branch) - expect(data[project_key][:homepage]).to eq(project.web_url) - expect(data[project_key][:url]).to eq(project.url_to_repo) - expect(data[project_key][:ssh_url]).to eq(project.ssh_url_to_repo) - expect(data[project_key][:http_url]).to eq(project.http_url_to_repo) - end -end - -RSpec.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) - expect(data[project_key][:web_url]).to eq(project.web_url) - expect(data[project_key][:avatar_url]).to eq(project.avatar_url) - expect(data[project_key][:git_http_url]).to eq(project.http_url_to_repo) - expect(data[project_key][:git_ssh_url]).to eq(project.ssh_url_to_repo) - expect(data[project_key][:namespace]).to eq(project.namespace.name) - expect(data[project_key][:visibility_level]).to eq(project.visibility_level) - expect(data[project_key][:path_with_namespace]).to eq(project.full_path) - expect(data[project_key][:default_branch]).to eq(project.default_branch) - end -end - -RSpec.shared_examples 'deprecated repository hook data' do |project_key: :project| - it 'contains deprecated repository data' do - expect(data[:repository][:name]).to eq(project.name) - expect(data[:repository][:description]).to eq(project.description) - expect(data[:repository][:url]).to eq(project.url_to_repo) - expect(data[:repository][:homepage]).to eq(project.web_url) - end -end 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/shared_examples/models/project_hook_data_shared_examples.rb b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb new file mode 100644 index 00000000000..f0264878811 --- /dev/null +++ b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb @@ -0,0 +1,42 @@ +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) + expect(data[project_key][:web_url]).to eq(project.web_url) + expect(data[project_key][:avatar_url]).to eq(project.avatar_url) + expect(data[project_key][:git_http_url]).to eq(project.http_url_to_repo) + expect(data[project_key][:git_ssh_url]).to eq(project.ssh_url_to_repo) + expect(data[project_key][:namespace]).to eq(project.namespace.name) + expect(data[project_key][:visibility_level]).to eq(project.visibility_level) + expect(data[project_key][:path_with_namespace]).to eq(project.full_path) + expect(data[project_key][:default_branch]).to eq(project.default_branch) + expect(data[project_key][:homepage]).to eq(project.web_url) + expect(data[project_key][:url]).to eq(project.url_to_repo) + expect(data[project_key][:ssh_url]).to eq(project.ssh_url_to_repo) + expect(data[project_key][:http_url]).to eq(project.http_url_to_repo) + end +end + +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) + expect(data[project_key][:web_url]).to eq(project.web_url) + expect(data[project_key][:avatar_url]).to eq(project.avatar_url) + expect(data[project_key][:git_http_url]).to eq(project.http_url_to_repo) + expect(data[project_key][:git_ssh_url]).to eq(project.ssh_url_to_repo) + expect(data[project_key][:namespace]).to eq(project.namespace.name) + expect(data[project_key][:visibility_level]).to eq(project.visibility_level) + expect(data[project_key][:path_with_namespace]).to eq(project.full_path) + expect(data[project_key][:default_branch]).to eq(project.default_branch) + end +end + +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) + expect(data[:repository][:url]).to eq(project.url_to_repo) + expect(data[:repository][:homepage]).to eq(project.web_url) + end +end -- cgit v1.2.1 From 075d6516047d899746d22b5323d3b74558e200d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 19 Sep 2017 19:23:15 +0200 Subject: Start adding Gitlab::HookData::IssuableBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/issuable.rb | 27 +++--- app/models/issue.rb | 69 +++++++++++---- app/models/merge_request.rb | 78 ++++++++++++----- app/services/issuable_base_service.rb | 2 +- app/services/issues/base_service.rb | 8 +- app/services/merge_requests/base_service.rb | 8 +- lib/gitlab/hook_data/issuable_builder.rb | 36 ++++++++ spec/lib/gitlab/hook_data/issuable_builder_spec.rb | 97 ++++++++++++++++++++++ spec/models/concerns/issuable_spec.rb | 52 +++++++----- spec/models/issue_spec.rb | 36 ++++++-- spec/models/merge_request_spec.rb | 43 ++++++++-- .../services/merge_requests/update_service_spec.rb | 5 +- .../models/issuable_hook_data_shared_examples.rb | 58 ------------- 13 files changed, 369 insertions(+), 150 deletions(-) create mode 100644 lib/gitlab/hook_data/issuable_builder.rb create mode 100644 spec/lib/gitlab/hook_data/issuable_builder_spec.rb delete mode 100644 spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e8a6c37d0b9..27f4dedffd3 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -256,29 +256,22 @@ module Issuable participants(user).include?(user) end - def to_hook_data(user, old_labels: []) + def to_hook_data(user, old_labels: [], old_assignees: []) changes = previous_changes + if old_labels != labels - changes[:labels] = [old_labels.map(&:name), labels.map(&:name)] + changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)] 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) - } - if self.is_a?(Issue) - hook_data[:assignees] = assignees.map(&:hook_attrs) if assignees.any? - else - hook_data[:assignee] = assignee.hook_attrs if assignee + 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 diff --git a/app/models/issue.rb b/app/models/issue.rb index 058ee144ee4..2527622a989 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -18,6 +18,36 @@ class Issue < ActiveRecord::Base DueThisWeek = DueDateStruct.new('Due This Week', 'week').freeze DueThisMonth = DueDateStruct.new('Due This Month', 'month').freeze + SAFE_HOOK_ATTRIBUTES = %i[ + assignee_id + author_id + branch_name + closed_at + confidential + created_at + deleted_at + description + due_date + id + iid + last_edited_at + last_edited_by_id + milestone_id + moved_to_id + project_id + relative_position + state + time_estimate + title + updated_at + updated_by_id + ].freeze + + SAFE_HOOK_RELATIONS = %i[ + assignees + labels + ].freeze + belongs_to :project belongs_to :moved_to, class_name: 'Issue' @@ -74,21 +104,6 @@ class Issue < ActiveRecord::Base end end - def hook_attrs - assignee_ids = self.assignee_ids - - attrs = { - url: Gitlab::UrlBuilder.build(self), - total_time_spent: total_time_spent, - human_total_time_spent: human_total_time_spent, - human_time_estimate: human_time_estimate, - assignee_ids: assignee_ids, - assignee_id: assignee_ids.first # This key is deprecated - } - - attributes.merge!(attrs) - end - def self.reference_prefix '#' end @@ -132,6 +147,30 @@ class Issue < ActiveRecord::Base "id DESC") end + def self.safe_hook_attributes + SAFE_HOOK_ATTRIBUTES + end + + def self.safe_hook_relations + SAFE_HOOK_RELATIONS + end + + def hook_attrs + assignee_ids = self.assignee_ids + + attrs = { + url: Gitlab::UrlBuilder.build(self), + total_time_spent: total_time_spent, + human_total_time_spent: human_total_time_spent, + human_time_estimate: human_time_estimate, + assignee_ids: assignee_ids, + assignee_id: assignee_ids.first # This key is deprecated + } + + attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) + .merge!(attrs) + end + # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 52a6c31503a..ce32b7cb05e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -7,6 +7,41 @@ class MergeRequest < ActiveRecord::Base include IgnorableColumn include CreatedAtFilterable + SAFE_HOOK_ATTRIBUTES = %i[ + assignee_id + author_id + created_at + deleted_at + description + head_pipeline_id + id + iid + last_edited_at + last_edited_by_id + merge_commit_sha + merge_error + merge_params + merge_status + merge_user_id + merge_when_pipeline_succeeds + milestone_id + ref_fetched + source_branch + source_project_id + state + target_branch + target_project_id + time_estimate + title + updated_at + updated_by_id + ].freeze + + SAFE_HOOK_RELATIONS = %i[ + assignee + labels + ].freeze + ignore_column :locked_at belongs_to :target_project, class_name: "Project" @@ -179,6 +214,30 @@ class MergeRequest < ActiveRecord::Base work_in_progress?(title) ? title : "WIP: #{title}" end + def self.safe_hook_attributes + SAFE_HOOK_ATTRIBUTES + end + + def self.safe_hook_relations + SAFE_HOOK_RELATIONS + end + + def hook_attrs + attrs = { + url: Gitlab::UrlBuilder.build(self), + source: source_project.try(:hook_attrs), + target: target_project.hook_attrs, + last_commit: diff_head_commit&.hook_attrs, + work_in_progress: work_in_progress?, + total_time_spent: total_time_spent, + human_total_time_spent: human_total_time_spent, + human_time_estimate: human_time_estimate + } + + attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) + .merge!(attrs) + end + # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { @@ -587,25 +646,6 @@ class MergeRequest < ActiveRecord::Base !discussions_to_be_resolved? end - def hook_attrs - attrs = { - url: Gitlab::UrlBuilder.build(self), - 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 diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 1d6b48ccf56..d61a342ebad 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', old_labels: old_labels) + execute_hooks(issuable, 'update', old_labels: old_labels, old_assignees: old_assignees) 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 f4fff4bf646..735257c4779 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -1,7 +1,7 @@ module Issues class BaseService < ::IssuableBaseService - def hook_data(issue, action, old_labels: []) - hook_data = issue.to_hook_data(current_user, old_labels: old_labels) + 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 @@ -22,8 +22,8 @@ module Issues issue, issue.project, current_user, old_assignees) end - def execute_hooks(issue, action = 'open', old_labels: []) - issue_data = hook_data(issue, action, old_labels: old_labels) + 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) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7cdb45ca552..112606a82d7 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -18,8 +18,8 @@ module MergeRequests super if changed_title end - def hook_data(merge_request, action, old_rev: nil, old_labels: []) - hook_data = merge_request.to_hook_data(current_user, old_labels: old_labels) + 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 old_rev && !Gitlab::Git.blank_ref?(old_rev) hook_data[:object_attributes][:oldrev] = old_rev @@ -28,9 +28,9 @@ module MergeRequests hook_data end - def execute_hooks(merge_request, action = 'open', old_rev: nil, old_labels: []) + 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, old_rev: old_rev, old_labels: old_labels) + 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 diff --git a/lib/gitlab/hook_data/issuable_builder.rb b/lib/gitlab/hook_data/issuable_builder.rb new file mode 100644 index 00000000000..e817c6af94a --- /dev/null +++ b/lib/gitlab/hook_data/issuable_builder.rb @@ -0,0 +1,36 @@ +module Gitlab + module HookData + class IssuableBuilder + 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: 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.class.safe_hook_attributes + issuable.class.safe_hook_relations + end + end + end +end diff --git a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb new file mode 100644 index 00000000000..31cc5eaea88 --- /dev/null +++ b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb @@ -0,0 +1,97 @@ +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] + } + end + let(:data) { builder.build(user: user, changes: changes) } + + it 'populates the :changes hash' do + expect(data[:changes]).to match(hash_including({ + title: ['A title', 'Hello World'], + description: ['A description', 'A cool description'] + })) + 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).tap { |i| i.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).tap { |mr| mr.update(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 diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 5763e1ba6d4..1f8541a3262 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -264,41 +264,55 @@ describe Issuable do end end - describe "#to_hook_data" do - it_behaves_like 'issuable hook data', 'issue' do - let(:issuable) { create(:issue, description: 'A description') } - end + describe '#to_hook_data' do + context 'labels are updated' do + let(:labels) { create_list(:label, 2) } + let(:data) { issue.to_hook_data(user, old_labels: [labels[0]]) } - it_behaves_like 'issuable hook data', 'merge_request' do - let(:issuable) { create(:merge_request, description: 'A description') } + before do + issue.update(labels: [labels[1]]) + end + + it 'includes labels in the hook data' do + expect(data[:labels]).to eq([labels[1].hook_attrs]) + expect(data[:changes]).to match(hash_including({ + labels: [[labels[0].hook_attrs], [labels[1].hook_attrs]] + })) + end end - context "issue is assigned" do - let(:data) { issue.to_hook_data(user) } + context 'issue is assigned' do + let(:user2) { create(:user) } + let(:data) { issue.to_hook_data(user, old_assignees: [user]) } before do - issue.assignees << user + issue.assignees << user << user2 end - it "returns correct hook data" do - expect(data[:assignees].first).to eq(user.hook_attrs) + it 'returns correct hook data' do + expect(data[:assignees]).to eq([user.hook_attrs, user2.hook_attrs]) + expect(data[:changes]).to match(hash_including({ + assignees: [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]] + })) end end - context "merge_request is assigned" do + context 'merge_request is assigned' do let(:merge_request) { create(:merge_request) } - let(:data) { merge_request.to_hook_data(user) } + let(:user2) { create(:user) } + let(:data) { merge_request.to_hook_data(user, old_assignees: [user]) } before do - merge_request.update_attribute(:assignee, user) + merge_request.update(assignee: user) + merge_request.update(assignee: user2) end - it "returns correct hook data" do - expect(data[:object_attributes]['assignee_id']).to eq(user.id) - expect(data[:assignee]).to eq(user.hook_attrs) + it 'returns correct hook data' do + expect(data[:object_attributes]['assignee_id']).to eq(user2.id) + expect(data[:assignee]).to eq(user2.hook_attrs) expect(data[:changes]).to match(hash_including({ - 'assignee_id' => [nil, user.id], - 'updated_at' => [a_kind_of(ActiveSupport::TimeWithZone), a_kind_of(ActiveSupport::TimeWithZone)] + assignee_id: [user.id, user2.id], + assignee: [user.hook_attrs, user2.hook_attrs] })) end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index e547da0cfbe..bd1ae3c4945 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -702,15 +702,39 @@ describe Issue do describe '#hook_attrs' do let(:attrs_hash) { subject.hook_attrs } - it 'includes time tracking attrs' do + it 'includes safe attribute' do + %w[ + assignee_id + author_id + branch_name + closed_at + confidential + created_at + deleted_at + description + due_date + id + iid + last_edited_at + last_edited_by_id + milestone_id + moved_to_id + project_id + relative_position + state + time_estimate + title + updated_at + updated_by_id + ].each do |key| + expect(attrs_hash).to include(key) + end + end + + it 'includes additional attrs' do expect(attrs_hash).to include(:total_time_spent) expect(attrs_hash).to include(:human_time_estimate) expect(attrs_hash).to include(:human_total_time_spent) - expect(attrs_hash).to include('time_estimate') - end - - it 'includes assignee_ids and deprecated assignee_id' do - expect(attrs_hash).to include(:assignee_id) expect(attrs_hash).to include(:assignee_ids) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 950af653c80..844ada57e22 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -660,19 +660,53 @@ describe MergeRequest do end end - describe "#hook_attrs" do + describe '#hook_attrs' do let(:attrs_hash) { subject.hook_attrs } - [:source, :target].each do |key| + it 'includes safe attribute' do + %w[ + assignee_id + author_id + created_at + deleted_at + description + head_pipeline_id + id + iid + last_edited_at + last_edited_by_id + merge_commit_sha + merge_error + merge_params + merge_status + merge_user_id + merge_when_pipeline_succeeds + milestone_id + ref_fetched + source_branch + source_project_id + state + target_branch + target_project_id + time_estimate + title + updated_at + updated_by_id + ].each do |key| + expect(attrs_hash).to include(key) + end + end + + %i[source target].each do |key| describe "#{key} key" do include_examples 'project hook data', project_key: key do let(:data) { attrs_hash } - let(:project) { subject.send("#{key}_project") } + let(:project) { subject.public_send("#{key}_project") } end end end - it "has all the required keys" do + it 'includes additional attrs' do expect(attrs_hash).to include(:source) expect(attrs_hash).to include(:target) expect(attrs_hash).to include(:last_commit) @@ -680,7 +714,6 @@ describe MergeRequest 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 end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 3a8cb4ce83d..7257c359a7e 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/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', old_labels: []) + 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 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 deleted file mode 100644 index ed1d2f41eae..00000000000 --- a/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb +++ /dev/null @@ -1,58 +0,0 @@ -# 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 -- cgit v1.2.1 From f070265a6ddd0173c8924bfcd7791ecafa15ab7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 5 Oct 2017 19:02:50 +0200 Subject: Introduce new hook data builders for Issue and MergeRequest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/issue.rb | 52 +----------------- app/models/merge_request.rb | 57 +------------------- doc/user/project/integrations/webhooks.md | 54 ++++++++++++++++++- lib/gitlab/hook_data/issuable_builder.rb | 24 ++++++++- lib/gitlab/hook_data/issue_builder.rb | 55 +++++++++++++++++++ lib/gitlab/hook_data/merge_request_builder.rb | 62 ++++++++++++++++++++++ spec/lib/gitlab/hook_data/issuable_builder_spec.rb | 18 +++++-- spec/lib/gitlab/hook_data/issue_builder_spec.rb | 46 ++++++++++++++++ .../gitlab/hook_data/merge_request_builder_spec.rb | 62 ++++++++++++++++++++++ spec/models/concerns/issuable_spec.rb | 57 +++++++++++++------- spec/models/issue_spec.rb | 44 +++------------ spec/models/merge_request_spec.rb | 59 +++----------------- .../models/issuable_hook_data_shared_examples.rb | 57 ++++++++++++++++++++ 13 files changed, 423 insertions(+), 224 deletions(-) create mode 100644 lib/gitlab/hook_data/issue_builder.rb create mode 100644 lib/gitlab/hook_data/merge_request_builder.rb create mode 100644 spec/lib/gitlab/hook_data/issue_builder_spec.rb create mode 100644 spec/lib/gitlab/hook_data/merge_request_builder_spec.rb create mode 100644 spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb diff --git a/app/models/issue.rb b/app/models/issue.rb index 2527622a989..36e4108b9d6 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -18,36 +18,6 @@ class Issue < ActiveRecord::Base DueThisWeek = DueDateStruct.new('Due This Week', 'week').freeze DueThisMonth = DueDateStruct.new('Due This Month', 'month').freeze - SAFE_HOOK_ATTRIBUTES = %i[ - assignee_id - author_id - branch_name - closed_at - confidential - created_at - deleted_at - description - due_date - id - iid - last_edited_at - last_edited_by_id - milestone_id - moved_to_id - project_id - relative_position - state - time_estimate - title - updated_at - updated_by_id - ].freeze - - SAFE_HOOK_RELATIONS = %i[ - assignees - labels - ].freeze - belongs_to :project belongs_to :moved_to, class_name: 'Issue' @@ -147,28 +117,8 @@ class Issue < ActiveRecord::Base "id DESC") end - def self.safe_hook_attributes - SAFE_HOOK_ATTRIBUTES - end - - def self.safe_hook_relations - SAFE_HOOK_RELATIONS - end - def hook_attrs - assignee_ids = self.assignee_ids - - attrs = { - url: Gitlab::UrlBuilder.build(self), - total_time_spent: total_time_spent, - human_total_time_spent: human_total_time_spent, - human_time_estimate: human_time_estimate, - assignee_ids: assignee_ids, - assignee_id: assignee_ids.first # This key is deprecated - } - - attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) - .merge!(attrs) + Gitlab::HookData::IssueBuilder.new(self).build end # Returns a Hash of attributes to be used for Twitter card metadata diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ce32b7cb05e..75e9bdaaa45 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -7,41 +7,6 @@ class MergeRequest < ActiveRecord::Base include IgnorableColumn include CreatedAtFilterable - SAFE_HOOK_ATTRIBUTES = %i[ - assignee_id - author_id - created_at - deleted_at - description - head_pipeline_id - id - iid - last_edited_at - last_edited_by_id - merge_commit_sha - merge_error - merge_params - merge_status - merge_user_id - merge_when_pipeline_succeeds - milestone_id - ref_fetched - source_branch - source_project_id - state - target_branch - target_project_id - time_estimate - title - updated_at - updated_by_id - ].freeze - - SAFE_HOOK_RELATIONS = %i[ - assignee - labels - ].freeze - ignore_column :locked_at belongs_to :target_project, class_name: "Project" @@ -214,28 +179,8 @@ class MergeRequest < ActiveRecord::Base work_in_progress?(title) ? title : "WIP: #{title}" end - def self.safe_hook_attributes - SAFE_HOOK_ATTRIBUTES - end - - def self.safe_hook_relations - SAFE_HOOK_RELATIONS - end - def hook_attrs - attrs = { - url: Gitlab::UrlBuilder.build(self), - source: source_project.try(:hook_attrs), - target: target_project.hook_attrs, - last_commit: diff_head_commit&.hook_attrs, - work_in_progress: work_in_progress?, - total_time_spent: total_time_spent, - human_total_time_spent: human_total_time_spent, - human_time_estimate: human_time_estimate - } - - attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes) - .merge!(attrs) + Gitlab::HookData::MergeRequestBuilder.new(self).build end # Returns a Hash of attributes to be used for Twitter card metadata diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index 3b2ce8d722c..7abc600a680 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -270,7 +270,32 @@ X-Gitlab-Event: Issue Hook "changes": { "updated_by_id": [null, 1], "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"], - "labels": [["Platform", "bug"], ["API"]] + "labels": { + "previous": [{ + "id": 206, + "title": "API", + "color": "#ffffff", + "project_id": 14, + "created_at": "2013-12-03T17:15:43Z", + "updated_at": "2013-12-03T17:15:43Z", + "template": false, + "description": "API related issues", + "type": "ProjectLabel", + "group_id": 41 + }], + "current": [{ + "id": 205, + "title": "Platform", + "color": "#123123", + "project_id": 14, + "created_at": "2013-12-03T17:15:43Z", + "updated_at": "2013-12-03T17:15:43Z", + "template": false, + "description": "Platform related issues", + "type": "ProjectLabel", + "group_id": 41 + }] + } } } ``` @@ -772,7 +797,32 @@ X-Gitlab-Event: Merge Request Hook "changes": { "updated_by_id": [null, 1], "updated_at": ["2017-09-15 16:50:55 UTC", "2017-09-15 16:52:00 UTC"], - "labels": [["Platform", "bug"], ["API"]] + "labels": { + "previous": [{ + "id": 206, + "title": "API", + "color": "#ffffff", + "project_id": 14, + "created_at": "2013-12-03T17:15:43Z", + "updated_at": "2013-12-03T17:15:43Z", + "template": false, + "description": "API related issues", + "type": "ProjectLabel", + "group_id": 41 + }], + "current": [{ + "id": 205, + "title": "Platform", + "color": "#123123", + "project_id": 14, + "created_at": "2013-12-03T17:15:43Z", + "updated_at": "2013-12-03T17:15:43Z", + "template": false, + "description": "Platform related issues", + "type": "ProjectLabel", + "group_id": 41 + }] + } } } ``` diff --git a/lib/gitlab/hook_data/issuable_builder.rb b/lib/gitlab/hook_data/issuable_builder.rb index e817c6af94a..4febb0ab430 100644 --- a/lib/gitlab/hook_data/issuable_builder.rb +++ b/lib/gitlab/hook_data/issuable_builder.rb @@ -1,6 +1,8 @@ module Gitlab module HookData class IssuableBuilder + CHANGES_KEYS = %i[previous current].freeze + attr_accessor :issuable def initialize(issuable) @@ -14,7 +16,7 @@ module Gitlab project: issuable.project.hook_attrs, object_attributes: issuable.hook_attrs, labels: issuable.labels.map(&:hook_attrs), - changes: changes.slice(*safe_keys), + changes: final_changes(changes.slice(*safe_keys)), # DEPRECATED repository: issuable.project.hook_attrs.slice(:name, :url, :description, :homepage) } @@ -29,7 +31,25 @@ module Gitlab end def safe_keys - issuable.class.safe_hook_attributes + issuable.class.safe_hook_relations + issuable_builder::SAFE_HOOK_ATTRIBUTES + issuable_builder::SAFE_HOOK_RELATIONS + end + + private + + def issuable_builder + case issuable + when Issue + Gitlab::HookData::IssueBuilder + when MergeRequest + Gitlab::HookData::MergeRequestBuilder + end + end + + def final_changes(changes_hash) + changes_hash.reduce({}) do |hash, (key, changes_array)| + hash[key] = Hash[CHANGES_KEYS.zip(changes_array)] + hash + end end end end diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb new file mode 100644 index 00000000000..de9cab80a02 --- /dev/null +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -0,0 +1,55 @@ +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 diff --git a/lib/gitlab/hook_data/merge_request_builder.rb b/lib/gitlab/hook_data/merge_request_builder.rb new file mode 100644 index 00000000000..eaef19c9d04 --- /dev/null +++ b/lib/gitlab/hook_data/merge_request_builder.rb @@ -0,0 +1,62 @@ +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 diff --git a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb index 31cc5eaea88..30da56bec16 100644 --- a/spec/lib/gitlab/hook_data/issuable_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb @@ -37,15 +37,23 @@ describe Gitlab::HookData::IssuableBuilder do lock_version: %w[foo bar], merge_jid: %w[foo bar], title: ['A title', 'Hello World'], - title_html: %w[foo bar] + title_html: %w[foo bar], + labels: [ + [{ id: 1, title: 'foo' }], + [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }] + ] } end let(:data) { builder.build(user: user, changes: changes) } it 'populates the :changes hash' do expect(data[:changes]).to match(hash_including({ - title: ['A title', 'Hello World'], - description: ['A description', 'A cool description'] + title: { previous: 'A title', current: 'Hello World' }, + description: { previous: 'A description', current: 'A cool description' }, + labels: { + previous: [{ id: 1, title: 'foo' }], + current: [{ id: 1, title: 'foo' }, { id: 2, title: 'bar' }] + } })) end @@ -73,7 +81,7 @@ describe Gitlab::HookData::IssuableBuilder do end context 'issue is assigned' do - let(:issue) { create(:issue).tap { |i| i.assignees << user } } + let(:issue) { create(:issue, assignees: [user]) } let(:data) { described_class.new(issue).build(user: user) } it 'returns correct hook data' do @@ -84,7 +92,7 @@ describe Gitlab::HookData::IssuableBuilder do end context 'merge_request is assigned' do - let(:merge_request) { create(:merge_request).tap { |mr| mr.update(assignee: user) } } + let(:merge_request) { create(:merge_request, assignee: user) } let(:data) { described_class.new(merge_request).build(user: user) } it 'returns correct hook data' do diff --git a/spec/lib/gitlab/hook_data/issue_builder_spec.rb b/spec/lib/gitlab/hook_data/issue_builder_spec.rb new file mode 100644 index 00000000000..6c529cdd051 --- /dev/null +++ b/spec/lib/gitlab/hook_data/issue_builder_spec.rb @@ -0,0 +1,46 @@ +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 diff --git a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb new file mode 100644 index 00000000000..92bf87bbad4 --- /dev/null +++ b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb @@ -0,0 +1,62 @@ +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 diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 1f8541a3262..ba57301a3c9 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -267,53 +267,70 @@ describe Issuable do describe '#to_hook_data' do context 'labels are updated' do let(:labels) { create_list(:label, 2) } - let(:data) { issue.to_hook_data(user, old_labels: [labels[0]]) } before do issue.update(labels: [labels[1]]) end - it 'includes labels in the hook data' do - expect(data[:labels]).to eq([labels[1].hook_attrs]) - expect(data[:changes]).to match(hash_including({ - labels: [[labels[0].hook_attrs], [labels[1].hook_attrs]] - })) + it 'delegates to Gitlab::HookData::IssuableBuilder#build' do + builder = double + + expect(Gitlab::HookData::IssuableBuilder) + .to receive(:new).with(issue).and_return(builder) + expect(builder).to receive(:build).with( + user: user, + changes: hash_including( + 'labels' => [[labels[0].hook_attrs], [labels[1].hook_attrs]] + )) + + issue.to_hook_data(user, old_labels: [labels[0]]) end end context 'issue is assigned' do let(:user2) { create(:user) } - let(:data) { issue.to_hook_data(user, old_assignees: [user]) } before do issue.assignees << user << user2 end - it 'returns correct hook data' do - expect(data[:assignees]).to eq([user.hook_attrs, user2.hook_attrs]) - expect(data[:changes]).to match(hash_including({ - assignees: [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]] - })) + it 'delegates to Gitlab::HookData::IssuableBuilder#build' do + builder = double + + expect(Gitlab::HookData::IssuableBuilder) + .to receive(:new).with(issue).and_return(builder) + expect(builder).to receive(:build).with( + user: user, + changes: hash_including( + 'assignees' => [[user.hook_attrs], [user.hook_attrs, user2.hook_attrs]] + )) + + issue.to_hook_data(user, old_assignees: [user]) end end context 'merge_request is assigned' do let(:merge_request) { create(:merge_request) } let(:user2) { create(:user) } - let(:data) { merge_request.to_hook_data(user, old_assignees: [user]) } before do merge_request.update(assignee: user) merge_request.update(assignee: user2) end - it 'returns correct hook data' do - expect(data[:object_attributes]['assignee_id']).to eq(user2.id) - expect(data[:assignee]).to eq(user2.hook_attrs) - expect(data[:changes]).to match(hash_including({ - assignee_id: [user.id, user2.id], - assignee: [user.hook_attrs, user2.hook_attrs] - })) + it 'delegates to Gitlab::HookData::IssuableBuilder#build' do + builder = double + + expect(Gitlab::HookData::IssuableBuilder) + .to receive(:new).with(merge_request).and_return(builder) + expect(builder).to receive(:build).with( + user: user, + changes: hash_including( + 'assignee_id' => [user.id, user2.id], + 'assignee' => [user.hook_attrs, user2.hook_attrs] + )) + + merge_request.to_hook_data(user, old_assignees: [user]) end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index bd1ae3c4945..bb5033c1628 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -700,42 +700,14 @@ describe Issue do end describe '#hook_attrs' do - let(:attrs_hash) { subject.hook_attrs } - - it 'includes safe attribute' do - %w[ - assignee_id - author_id - branch_name - closed_at - confidential - created_at - deleted_at - description - due_date - id - iid - last_edited_at - last_edited_by_id - milestone_id - moved_to_id - project_id - relative_position - state - time_estimate - title - updated_at - updated_by_id - ].each do |key| - expect(attrs_hash).to include(key) - end - end - - it 'includes additional attrs' do - expect(attrs_hash).to include(:total_time_spent) - expect(attrs_hash).to include(:human_time_estimate) - expect(attrs_hash).to include(:human_total_time_spent) - expect(attrs_hash).to include(:assignee_ids) + it 'delegates to Gitlab::HookData::IssueBuilder#build' do + builder = double + + expect(Gitlab::HookData::IssueBuilder) + .to receive(:new).with(subject).and_return(builder) + expect(builder).to receive(:build) + + subject.hook_attrs end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 844ada57e22..17c9f15b021 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -661,59 +661,14 @@ describe MergeRequest do end describe '#hook_attrs' do - let(:attrs_hash) { subject.hook_attrs } - - it 'includes safe attribute' do - %w[ - assignee_id - author_id - created_at - deleted_at - description - head_pipeline_id - id - iid - last_edited_at - last_edited_by_id - merge_commit_sha - merge_error - merge_params - merge_status - merge_user_id - merge_when_pipeline_succeeds - milestone_id - ref_fetched - source_branch - source_project_id - state - target_branch - target_project_id - time_estimate - title - updated_at - updated_by_id - ].each do |key| - expect(attrs_hash).to include(key) - end - end - - %i[source target].each do |key| - describe "#{key} key" do - include_examples 'project hook data', project_key: key do - let(:data) { attrs_hash } - let(:project) { subject.public_send("#{key}_project") } - end - end - end + it '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 'includes additional attrs' do - expect(attrs_hash).to include(:source) - expect(attrs_hash).to include(:target) - expect(attrs_hash).to include(:last_commit) - expect(attrs_hash).to include(:work_in_progress) - expect(attrs_hash).to include(:total_time_spent) - expect(attrs_hash).to include(:human_time_estimate) - expect(attrs_hash).to include(:human_total_time_spent) + subject.hook_attrs end end 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..a4762b68858 --- /dev/null +++ b/spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb @@ -0,0 +1,57 @@ +# 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 -- cgit v1.2.1