diff options
-rw-r--r-- | app/models/concerns/issuable.rb | 27 | ||||
-rw-r--r-- | app/models/issue.rb | 69 | ||||
-rw-r--r-- | app/models/merge_request.rb | 78 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 2 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 8 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/hook_data/issuable_builder.rb | 36 | ||||
-rw-r--r-- | spec/lib/gitlab/hook_data/issuable_builder_spec.rb | 97 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 52 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 36 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 43 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 5 | ||||
-rw-r--r-- | spec/support/shared_examples/models/issuable_hook_data_shared_examples.rb | 58 |
13 files changed, 369 insertions, 150 deletions
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 |