diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-10-09 15:56:42 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-10-09 15:56:42 +0000 |
commit | bc7c6dde9ae3dcf0e2a0d7fe8f6afc912cacc87c (patch) | |
tree | 3e77c6d08786700ccb50d2f15686d49cb3aa5d98 /spec | |
parent | 11c67e7c2f992299ff5918ce67995b73d1e0be6d (diff) | |
parent | f070265a6ddd0173c8924bfcd7791ecafa15ab7e (diff) | |
download | gitlab-ce-bc7c6dde9ae3dcf0e2a0d7fe8f6afc912cacc87c.tar.gz |
Merge branch '34284-add-changes-to-issuable-webhook-data' into 'master'
Include the changes in issuable webhook payloads
Closes #34284
See merge request gitlab-org/gitlab-ce!14308
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/hook_data/issuable_builder_spec.rb | 105 | ||||
-rw-r--r-- | spec/lib/gitlab/hook_data/issue_builder_spec.rb | 46 | ||||
-rw-r--r-- | spec/lib/gitlab/hook_data/merge_request_builder_spec.rb | 62 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 80 | ||||
-rw-r--r-- | spec/models/issue_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 28 | ||||
-rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 8 | ||||
-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 | 57 | ||||
-rw-r--r-- | spec/support/shared_examples/models/project_hook_data_shared_examples.rb (renamed from spec/support/project_hook_data_shared_example.rb) | 6 |
10 files changed, 344 insertions, 69 deletions
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..30da56bec16 --- /dev/null +++ b/spec/lib/gitlab/hook_data/issuable_builder_spec.rb @@ -0,0 +1,105 @@ +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], + 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: { 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 + + 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, 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, 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/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 fb5fb7daaab..ba57301a3c9 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 @@ -264,55 +264,75 @@ describe Issuable do end end - describe "#to_hook_data" do - let(:data) { issue.to_hook_data(user) } - let(:project) { issue.project } - - 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) - end + describe '#to_hook_data' do + context 'labels are updated' do + let(:labels) { create_list(:label, 2) } - context "issue is assigned" do before do - issue.assignees << user + issue.update(labels: [labels[1]]) end - it "returns correct hook data" do - expect(data[:assignees].first).to eq(user.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 "merge_request is assigned" do - let(:merge_request) { create(:merge_request) } - let(:data) { merge_request.to_hook_data(user) } + context 'issue is assigned' do + let(:user2) { create(:user) } before do - merge_request.update_attribute(:assignee, user) + issue.assignees << user << 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 '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 'issue has labels' do - let(:labels) { [create(:label), create(:label)] } + context 'merge_request is assigned' do + let(:merge_request) { create(:merge_request) } + let(:user2) { create(:user) } before do - issue.update_attribute(:labels, labels) + merge_request.update(assignee: user) + merge_request.update(assignee: user2) end - it 'includes labels in the hook data' do - expect(data[:labels]).to eq(labels.map(&: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 - - include_examples 'project hook data' - include_examples 'deprecated repository hook data' end describe '#labels_array' do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index e547da0cfbe..bb5033c1628 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -700,18 +700,14 @@ describe Issue do end describe '#hook_attrs' do - let(:attrs_hash) { subject.hook_attrs } + it 'delegates to Gitlab::HookData::IssueBuilder#build' do + builder = double - it 'includes time tracking 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 + expect(Gitlab::HookData::IssueBuilder) + .to receive(:new).with(subject).and_return(builder) + expect(builder).to receive(:build) - it 'includes assignee_ids and deprecated assignee_id' do - expect(attrs_hash).to include(:assignee_id) - expect(attrs_hash).to include(:assignee_ids) + subject.hook_attrs end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 950af653c80..17c9f15b021 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -660,27 +660,15 @@ describe MergeRequest do end end - describe "#hook_attrs" do - let(:attrs_hash) { subject.hook_attrs } - - [: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") } - end - end - end + describe '#hook_attrs' do + 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 "has all the required keys" 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) - expect(attrs_hash).to include('time_estimate') + subject.hook_attrs end end 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..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') + 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 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 diff --git a/spec/support/project_hook_data_shared_example.rb b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb index 1eb405d4be8..f0264878811 100644 --- a/spec/support/project_hook_data_shared_example.rb +++ b/spec/support/shared_examples/models/project_hook_data_shared_examples.rb @@ -1,4 +1,4 @@ -RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :project| +shared_examples 'project hook data with deprecateds' do |project_key: :project| it 'contains project data' do expect(data[project_key][:name]).to eq(project.name) expect(data[project_key][:description]).to eq(project.description) @@ -17,7 +17,7 @@ RSpec.shared_examples 'project hook data with deprecateds' do |project_key: :pro end end -RSpec.shared_examples 'project hook data' do |project_key: :project| +shared_examples 'project hook data' do |project_key: :project| it 'contains project data' do expect(data[project_key][:name]).to eq(project.name) expect(data[project_key][:description]).to eq(project.description) @@ -32,7 +32,7 @@ RSpec.shared_examples 'project hook data' do |project_key: :project| end end -RSpec.shared_examples 'deprecated repository hook data' do |project_key: :project| +shared_examples 'deprecated repository hook data' do it 'contains deprecated repository data' do expect(data[:repository][:name]).to eq(project.name) expect(data[:repository][:description]).to eq(project.description) |