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 --- 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 ------------- 6 files changed, 201 insertions(+), 90 deletions(-) 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 (limited to 'spec') 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.3