From 64ed7069c709949c0839aebe62d58191f75fa050 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Mon, 18 Feb 2019 13:30:09 +0100 Subject: [PATCH] Incorporate review feedback Use shared examples for issues and merge requests rather than a loop creating common specs. --- doc/api/issues.md | 2 +- doc/api/merge_requests.md | 2 +- lib/api/issuable_bulk_update.rb | 4 +- .../requests/api/issuable_bulk_update_spec.rb | 250 +++++++++--------- 4 files changed, 130 insertions(+), 128 deletions(-) diff --git a/doc/api/issues.md b/doc/api/issues.md index ccd28d21e98..81553746834 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -637,7 +637,7 @@ Example response: ## Bulk update issues -> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21368) in GitLab 11.8. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21368) in GitLab 11.9. Update multiple issues using a single API call. Returns the number of successfully updated issues. diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 05bc3d098c5..78cb4db74e1 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -958,7 +958,7 @@ Must include at least one non-required attribute from above. ## Bulk update merge requests -> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21368) in GitLab 11.8. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21368) in GitLab 11.9. Update multiple merge requests using a single API call. Returns the number of successfully updated merge requests. diff --git a/lib/api/issuable_bulk_update.rb b/lib/api/issuable_bulk_update.rb index bcda8b32976..5ac6c252d96 100644 --- a/lib/api/issuable_bulk_update.rb +++ b/lib/api/issuable_bulk_update.rb @@ -11,7 +11,7 @@ module API detail 'This feature was introduced in 11.9' end params do - requires :issuable_ids, type: Array[Integer], desc: "Array or #{issuable.pluralize} IDs to be updates" + requires :issuable_ids, type: Array[Integer], desc: "Array of #{issuable.pluralize} IDs to be updated" optional :state_event, type: String, values: %w(reopen close), desc: 'Reopens or closes a resource' optional :milestone_id, type: Integer, desc: 'The milestone ID number' optional :add_label_ids, type: Array[Integer], desc: 'IDs of labels to be added' @@ -20,7 +20,7 @@ module API desc: 'Subscribes or unsubscribes from a resource' if issuable == 'issue' - optional :assignee_ids, type: Array[Integer], desc: 'List of assignees IDs' + optional :assignee_ids, type: Array[Integer], desc: 'List of assignee IDs' at_least_one_of :state_event, :milestone_id, :add_label_ids, :remove_label_ids, :subscription_event, :assignee_ids else diff --git a/spec/requests/api/issuable_bulk_update_spec.rb b/spec/requests/api/issuable_bulk_update_spec.rb index 810fa8dc529..6463f3f5d35 100644 --- a/spec/requests/api/issuable_bulk_update_spec.rb +++ b/spec/requests/api/issuable_bulk_update_spec.rb @@ -3,150 +3,152 @@ require 'spec_helper' describe API::IssuableBulkUpdate do - set(:user) { create(:user) } - set(:project) do - create(:project, :public, creator_id: user.id, namespace: user.namespace) - end + set(:project) { create(:project) } + set(:user) { project.creator } - %w(issue merge_request).each do |issuable| - describe "PUT /projects/:id/#{issuable.pluralize}/bulk_update" do - let(:merge_request_1) { create(:merge_request, source_project: project) } - let(:merge_request_2) { create(:merge_request, :simple, source_project: project) } - let!(:issuables) { issuable == 'issue' ? create_list(:issue, 2, project: project) : [merge_request_1, merge_request_2] } + shared_examples "PUT /projects/:id/:issuable/bulk_update" do |issuable| + def bulk_update(issuable, issuables, params, update_user = user) + put api("/projects/#{project.id}/#{issuable.pluralize}/bulk_update", update_user), + params: { issuable_ids: Array(issuables).map(&:id) }.merge(params) + end - def bulk_update(issuable, issuables, params, update_user = user) - put api("/projects/#{project.id}/#{issuable.pluralize}/bulk_update", update_user), - params: { issuable_ids: Array(issuables).map(&:id) }.merge(params) + context 'with not enough permissions' do + it 'returns 403 for guest users' do + guest = create(:user) + project.add_guest(guest) + + bulk_update(issuable, issuables, { state_event: 'close' }, guest) + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'when modifying the state' do + it "closes #{issuable}" do + bulk_update(issuable, issuables, { state_event: 'close' }) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['message']).to eq("#{issuables.count} #{issuable.pluralize(issuables.count)} updated") + expect(project.public_send(issuable.pluralize).opened).to be_empty + expect(project.public_send(issuable.pluralize).closed).not_to be_empty end - context 'with not enough permissions' do - it 'returns 403 for guest users' do - guest = create(:user) - project.add_guest(guest) + it "opens #{issuable}" do + closed_issuables = create_list("closed_#{issuable}".to_sym, 2) - bulk_update(issuable, issuables, { state_event: 'close' }, guest) + bulk_update(issuable, closed_issuables, { state_event: 'reopen' }) - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(200) + expect(project.public_send(issuable.pluralize).closed).to be_empty + end + end + + context 'when modifying the milestone' do + let(:milestone) { create(:milestone, project: project) } + + it "adds a milestone #{issuable}" do + bulk_update(issuable, issuables, { milestone_id: milestone.id }) + + expect(response).to have_gitlab_http_status(200) + issuables.each do |issuable| + expect(issuable.reload.milestone).to eq(milestone) end end - context 'when modifying the state' do - it "closes #{issuable}" do - bulk_update(issuable, issuables, { state_event: 'close' }) + it 'removes a milestone' do + issuables.first.milestone = milestone + milestone_issuable = issuables.first - expect(response).to have_gitlab_http_status(200) - expect(json_response['message']).to eq("#{issuables.count} #{issuable.pluralize(issuables.count)} updated") - expect(project.public_send(issuable.pluralize).opened).to be_empty - expect(project.public_send(issuable.pluralize).closed).not_to be_empty - end + bulk_update(issuable, [milestone_issuable], { milestone_id: 0 }) - it "opens #{issuable}" do - closed_issuables = create_list("closed_#{issuable}".to_sym, 2) + expect(response).to have_gitlab_http_status(200) + expect(milestone_issuable.reload.milestone).to eq(nil) + end + end - bulk_update(issuable, closed_issuables, { state_event: 'reopen' }) + context 'when modifying the subscription state' do + it "subscribes to #{issuable}" do + bulk_update(issuable, issuables, { subscription_event: 'subscribe' }) - expect(response).to have_gitlab_http_status(200) - expect(project.public_send(issuable.pluralize).closed).to be_empty - end + expect(response).to have_gitlab_http_status(200) + expect(issuables).to all(be_subscribed(user, project)) end - context 'when modifying the milestone' do - let(:milestone) { create(:milestone, project: project) } - - it "adds a milestone #{issuable}" do - bulk_update(issuable, issuables, { milestone_id: milestone.id }) - - expect(response).to have_gitlab_http_status(200) - issuables.each do |issuable| - expect(issuable.reload.milestone).to eq(milestone) - end + it 'unsubscribes from issues' do + issuables.each do |issuable| + issuable.subscriptions.create(user: user, project: project, subscribed: true) end - it 'removes a milestone' do - issuables.first.milestone = milestone - milestone_issuable = issuables.first + bulk_update(issuable, issuables, { subscription_event: 'unsubscribe' }) - bulk_update(issuable, [milestone_issuable], { milestone_id: 0 }) - - expect(response).to have_gitlab_http_status(200) - expect(milestone_issuable.reload.milestone).to eq(nil) - end - end - - context 'when modifying the subscription state' do - it "subscribes to #{issuable}" do - bulk_update(issuable, issuables, { subscription_event: 'subscribe' }) - - expect(response).to have_gitlab_http_status(200) - expect(issuables).to all(be_subscribed(user, project)) - end - - it 'unsubscribes from issues' do - issuables.each do |issuable| - issuable.subscriptions.create(user: user, project: project, subscribed: true) - end - - bulk_update(issuable, issuables, { subscription_event: 'unsubscribe' }) - - expect(response).to have_gitlab_http_status(200) - issuables.each do |issuable| - expect(issuable).not_to be_subscribed(user, project) - end - end - end - - context 'when modifying the assignee' do - it 'adds assignee to issues' do - params = issuable == 'issue' ? { assignee_ids: [user.id] } : { assignee_id: user.id } - - bulk_update(issuable, issuables, params) - - expect(response).to have_gitlab_http_status(200) - issuables.each do |issuable| - expect(issuable.reload.assignees).to eq([user]) - end - end - - it 'removes assignee' do - assigned_issuable = issuables.first - - if issuable == 'issue' - params = { assignee_ids: 0 } - assigned_issuable.assignees << user - else - params = { assignee_id: 0 } - assigned_issuable.update_attribute(:assignee, user) - end - - bulk_update(issuable, [assigned_issuable], params) - expect(assigned_issuable.reload.assignees).to eq([]) - end - end - - context 'when modifying labels' do - let(:bug) { create(:label, project: project) } - let(:regression) { create(:label, project: project) } - let(:feature) { create(:label, project: project) } - - it 'adds new labels' do - bulk_update(issuable, issuables, { add_label_ids: [bug.id, regression.id, feature.id] }) - - issuables.each do |issusable| - expect(issusable.reload.label_ids).to contain_exactly(bug.id, regression.id, feature.id) - end - end - - it 'removes labels' do - labled_issuable = issuables.first - labled_issuable.labels << bug - labled_issuable.labels << regression - labled_issuable.labels << feature - - bulk_update(issuable, [labled_issuable], { remove_label_ids: [bug.id, regression.id] }) - - expect(labled_issuable.reload.label_ids).to contain_exactly(feature.id) + expect(response).to have_gitlab_http_status(200) + issuables.each do |issuable| + expect(issuable).not_to be_subscribed(user, project) end end end + + context 'when modifying the assignee' do + it 'adds assignee to issues' do + params = issuable == 'issue' ? { assignee_ids: [user.id] } : { assignee_id: user.id } + + bulk_update(issuable, issuables, params) + + expect(response).to have_gitlab_http_status(200) + issuables.each do |issuable| + expect(issuable.reload.assignees).to eq([user]) + end + end + + it 'removes assignee' do + assigned_issuable = issuables.first + + if issuable == 'issue' + params = { assignee_ids: 0 } + assigned_issuable.assignees << user + else + params = { assignee_id: 0 } + assigned_issuable.update_attribute(:assignee, user) + end + + bulk_update(issuable, [assigned_issuable], params) + expect(assigned_issuable.reload.assignees).to eq([]) + end + end + + context 'when modifying labels' do + let(:bug) { create(:label, project: project) } + let(:regression) { create(:label, project: project) } + let(:feature) { create(:label, project: project) } + + it 'adds new labels' do + bulk_update(issuable, issuables, { add_label_ids: [bug.id, regression.id, feature.id] }) + + issuables.each do |issusable| + expect(issusable.reload.label_ids).to contain_exactly(bug.id, regression.id, feature.id) + end + end + + it 'removes labels' do + labled_issuable = issuables.first + labled_issuable.labels << bug + labled_issuable.labels << regression + labled_issuable.labels << feature + + bulk_update(issuable, [labled_issuable], { remove_label_ids: [bug.id, regression.id] }) + + expect(labled_issuable.reload.label_ids).to contain_exactly(feature.id) + end + end + end + + it_behaves_like 'PUT /projects/:id/:issuable/bulk_update', 'issue' do + let(:issuables) { create_list(:issue, 2, project: project) } + end + + it_behaves_like 'PUT /projects/:id/:issuable/bulk_update', 'merge_request' do + let(:merge_request_1) { create(:merge_request, source_project: project) } + let(:merge_request_2) { create(:merge_request, :simple, source_project: project) } + let(:issuables) { [merge_request_1, merge_request_2] } end end