From abb50ff4710e264c0c700df88757ee3ab1cf7dfb Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 6 Aug 2018 16:02:52 -0300 Subject: Allow to delete group milestones --- .../groups/milestones_controller_spec.rb | 11 ++++++ spec/factories/milestones.rb | 2 +- .../milestones/user_deletes_milestone_spec.rb | 44 ++++++++++++++++------ spec/policies/group_policy_spec.rb | 2 +- spec/requests/api/project_milestones_spec.rb | 13 ------- spec/services/milestones/destroy_service_spec.rb | 28 +++++++++++--- spec/support/api/milestones_shared_examples.rb | 18 +++++++++ 7 files changed, 85 insertions(+), 33 deletions(-) (limited to 'spec') diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb index f7068546093..465f3499703 100644 --- a/spec/controllers/groups/milestones_controller_spec.rb +++ b/spec/controllers/groups/milestones_controller_spec.rb @@ -141,6 +141,17 @@ describe Groups::MilestonesController do end end + describe "#destroy" do + let(:milestone) { create(:milestone, group: group) } + + it "removes milestone" do + delete :destroy, group_id: group.to_param, id: milestone.iid, format: :js + + expect(response).to be_success + expect { Milestone.find(milestone.id) }.to raise_exception(ActiveRecord::RecordNotFound) + end + end + describe '#ensure_canonical_path' do before do sign_in(user) diff --git a/spec/factories/milestones.rb b/spec/factories/milestones.rb index f95632e7187..90cae4102f4 100644 --- a/spec/factories/milestones.rb +++ b/spec/factories/milestones.rb @@ -29,7 +29,7 @@ FactoryBot.define do milestone.project_id = evaluator.project_id elsif evaluator.parent id = evaluator.parent.id - evaluator.parent.is_a?(Group) ? board.group_id = id : evaluator.project_id = id + evaluator.parent.is_a?(Group) ? evaluator.group_id = id : evaluator.project_id = id else milestone.project = create(:project) end diff --git a/spec/features/milestones/user_deletes_milestone_spec.rb b/spec/features/milestones/user_deletes_milestone_spec.rb index 9d4a68239d3..a8c296b4cd2 100644 --- a/spec/features/milestones/user_deletes_milestone_spec.rb +++ b/spec/features/milestones/user_deletes_milestone_spec.rb @@ -1,26 +1,46 @@ require "rails_helper" describe "User deletes milestone", :js do - set(:user) { create(:user) } - set(:project) { create(:project) } - set(:milestone) { create(:milestone, project: project) } + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project) { create(:project, namespace: group) } before do - project.add_developer(user) sign_in(user) + end + + context "when milestone belongs to project" do + let!(:milestone) { create(:milestone, parent: project, title: "project milestone") } + + it "deletes milestone" do + project.add_developer(user) + visit(project_milestones_path(project)) + click_link(milestone.title) + click_button("Delete") + click_button("Delete milestone") + + expect(page).to have_content("No milestones to show") + + visit(activity_project_path(project)) - visit(project_milestones_path(project)) + expect(page).to have_content("#{user.name} destroyed milestone") + end end - it "deletes milestone" do - click_link(milestone.title) - click_button("Delete") - click_button("Delete milestone") + context "when milestone belongs to group" do + let!(:milestone_to_be_deleted) { create(:milestone, parent: group, title: "group milestone 1") } + let!(:milestone) { create(:milestone, parent: group, title: "group milestone 2") } - expect(page).to have_content("No milestones to show") + it "deletes milestone" do + group.add_developer(user) + visit(group_milestones_path(group)) - visit(activity_project_path(project)) + click_link(milestone_to_be_deleted.title) + click_button("Delete") + click_button("Delete milestone") - expect(page).to have_content("#{user.name} destroyed milestone") + expect(page).to have_content(milestone.title) + expect(page).not_to have_content(milestone_to_be_deleted) + end end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 35951251bc5..5ababe694c6 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -17,7 +17,7 @@ describe GroupPolicy do let(:reporter_permissions) { [:admin_label] } - let(:developer_permissions) { [:admin_milestones] } + let(:developer_permissions) { [:admin_milestone] } let(:maintainer_permissions) do [ diff --git a/spec/requests/api/project_milestones_spec.rb b/spec/requests/api/project_milestones_spec.rb index 6c05c166bd6..62613aa5938 100644 --- a/spec/requests/api/project_milestones_spec.rb +++ b/spec/requests/api/project_milestones_spec.rb @@ -39,19 +39,6 @@ describe API::ProjectMilestones do expect(response).to have_gitlab_http_status(404) end - - it "rejects a member with reporter access from deleting a milestone" do - delete api("/projects/#{project.id}/milestones/#{milestone.id}", reporter) - - expect(response).to have_gitlab_http_status(403) - end - - it 'deletes the milestone when the user has developer access to the project' do - delete api("/projects/#{project.id}/milestones/#{milestone.id}", user) - - expect(project.milestones.find_by_id(milestone.id)).to be_nil - expect(response).to have_gitlab_http_status(204) - end end describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index 6f3612501f4..8680e428517 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -4,8 +4,6 @@ describe Milestones::DestroyService do let(:user) { create(:user) } let(:project) { create(:project) } let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } - let!(:issue) { create(:issue, project: project, milestone: milestone) } - let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } before do project.add_maintainer(user) @@ -23,12 +21,23 @@ describe Milestones::DestroyService do end it 'deletes milestone id from issuables' do + issue = create(:issue, project: project, milestone: milestone) + merge_request = create(:merge_request, source_project: project, milestone: milestone) + service.execute(milestone) expect(issue.reload.milestone).to be_nil expect(merge_request.reload.milestone).to be_nil end + it 'logs destroy event' do + service.execute(milestone) + + event = Event.where(project_id: milestone.project_id, target_type: 'Milestone') + + expect(event.count).to eq(1) + end + context 'group milestones' do let(:group) { create(:group) } let(:group_milestone) { create(:milestone, group: group) } @@ -38,13 +47,20 @@ describe Milestones::DestroyService do group.add_developer(user) end - it { expect(service.execute(group_milestone)).to be_nil } + it { expect(service.execute(group_milestone)).to eq(group_milestone) } - it 'does not update milestone issuables' do - expect(MergeRequests::UpdateService).not_to receive(:new) - expect(Issues::UpdateService).not_to receive(:new) + it 'deletes milestone id from issuables' do + issue = create(:issue, project: project, milestone: group_milestone) + merge_request = create(:merge_request, source_project: project, milestone: group_milestone) service.execute(group_milestone) + + expect(issue.reload.milestone).to be_nil + expect(merge_request.reload.milestone).to be_nil + end + + it 'does not log destroy event' do + expect { service.execute(group_milestone) }.not_to change { Event.count } end end end diff --git a/spec/support/api/milestones_shared_examples.rb b/spec/support/api/milestones_shared_examples.rb index a15189db35f..8dbec499622 100644 --- a/spec/support/api/milestones_shared_examples.rb +++ b/spec/support/api/milestones_shared_examples.rb @@ -204,6 +204,24 @@ shared_examples_for 'group and project milestones' do |route_definition| end end + describe "DELETE #{route_definition}/:milestone_id" do + it "rejects a member with reporter access from deleting a milestone" do + reporter = create(:user) + milestone.parent.add_reporter(reporter) + + delete api(resource_route, reporter) + + expect(response).to have_gitlab_http_status(403) + end + + it 'deletes the milestone when the user has developer access to the project' do + delete api(resource_route, user) + + expect(project.milestones.find_by_id(milestone.id)).to be_nil + expect(response).to have_gitlab_http_status(204) + end + end + describe "GET #{route_definition}/:milestone_id/issues" do let(:issues_route) { "#{route}/#{milestone.id}/issues" } -- cgit v1.2.3