diff options
author | Felipe Artur <felipefac@gmail.com> | 2018-08-06 16:02:52 -0300 |
---|---|---|
committer | Felipe Artur <felipefac@gmail.com> | 2018-08-06 17:31:47 -0300 |
commit | abb50ff4710e264c0c700df88757ee3ab1cf7dfb (patch) | |
tree | a77ce62f4384becfe7b36fa1ae7fea700895812f | |
parent | cb2e07309b4e61501a44c3568155bdb73252338f (diff) | |
download | gitlab-ce-abb50ff4710e264c0c700df88757ee3ab1cf7dfb.tar.gz |
Allow to delete group milestonesissue_36138
22 files changed, 172 insertions, 68 deletions
diff --git a/app/assets/javascripts/pages/groups/milestones/show/index.js b/app/assets/javascripts/pages/groups/milestones/show/index.js index 74cc4ba42c1..ebaea5ef3dc 100644 --- a/app/assets/javascripts/pages/groups/milestones/show/index.js +++ b/app/assets/javascripts/pages/groups/milestones/show/index.js @@ -1,8 +1,10 @@ import initMilestonesShow from '~/pages/milestones/shared/init_milestones_show'; +import initDeleteMilestoneModal from '~/pages/milestones/shared/delete_milestone_modal_init'; + import Milestone from '~/milestone'; document.addEventListener('DOMContentLoaded', () => { initMilestonesShow(); - + initDeleteMilestoneModal(); Milestone.initDeprecationMessage(); }); diff --git a/app/assets/javascripts/pages/milestones/shared/components/delete_milestone_modal.vue b/app/assets/javascripts/pages/milestones/shared/components/delete_milestone_modal.vue index 4061c11ba8f..48668562f09 100644 --- a/app/assets/javascripts/pages/milestones/shared/components/delete_milestone_modal.vue +++ b/app/assets/javascripts/pages/milestones/shared/components/delete_milestone_modal.vue @@ -40,8 +40,8 @@ if (this.issueCount === 0 && this.mergeRequestCount === 0) { return sprintf( s__(`Milestones| -You’re about to permanently delete the milestone %{milestoneTitle} from this project. -%{milestoneTitle} is not currently used in any issues or merge requests.`), +You’re about to permanently delete the milestone %{milestoneTitle}. +This milestone is not currently used in any issues or merge requests.`), { milestoneTitle, }, @@ -51,7 +51,7 @@ You’re about to permanently delete the milestone %{milestoneTitle} from this p return sprintf( s__(`Milestones| -You’re about to permanently delete the milestone %{milestoneTitle} from this project and remove it from %{issuesWithCount} and %{mergeRequestsWithCount}. +You’re about to permanently delete the milestone %{milestoneTitle} and remove it from %{issuesWithCount} and %{mergeRequestsWithCount}. Once deleted, it cannot be undone or recovered.`), { milestoneTitle, diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index 9bd51de7e97..6bdc0f79ef2 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -2,8 +2,8 @@ class Groups::MilestonesController < Groups::ApplicationController include MilestoneActions before_action :group_projects - before_action :milestone, only: [:edit, :show, :update, :merge_requests, :participants, :labels] - before_action :authorize_admin_milestones!, only: [:edit, :new, :create, :update] + before_action :milestone, only: [:edit, :show, :update, :merge_requests, :participants, :labels, :destroy] + before_action :authorize_admin_milestones!, only: [:edit, :new, :create, :update, :destroy] def index respond_to do |format| @@ -56,10 +56,21 @@ class Groups::MilestonesController < Groups::ApplicationController redirect_to milestone_path end + def destroy + return render_404 if @milestone.legacy_group_milestone? + + Milestones::DestroyService.new(group, current_user).execute(@milestone) + + respond_to do |format| + format.html { redirect_to group_milestones_path(group), status: :see_other } + format.js { head :ok } + end + end + private def authorize_admin_milestones! - return render_404 unless can?(current_user, :admin_milestones, group) + return render_404 unless can?(current_user, :admin_milestone, group) end def milestone_params diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index a8d7a05f509..333643865e9 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -53,7 +53,7 @@ class GroupPolicy < BasePolicy rule { has_access }.enable :read_namespace - rule { developer }.enable :admin_milestones + rule { developer }.enable :admin_milestone rule { reporter }.policy do enable :admin_label diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb index 15c04525075..7cda802c120 100644 --- a/app/services/milestones/destroy_service.rb +++ b/app/services/milestones/destroy_service.rb @@ -3,8 +3,6 @@ module Milestones class DestroyService < Milestones::BaseService def execute(milestone) - return unless milestone.project_milestone? - Milestone.transaction do update_params = { milestone: nil } @@ -16,15 +14,21 @@ module Milestones MergeRequests::UpdateService.new(parent, current_user, update_params).execute(merge_request) end - event_service.destroy_milestone(milestone, current_user) - - Event.for_milestone_id(milestone.id).each do |event| - event.target_id = nil - event.save - end + log_destroy_event_for(milestone) milestone.destroy end end + + def log_destroy_event_for(milestone) + return if milestone.group_milestone? + + event_service.destroy_milestone(milestone, current_user) + + Event.for_milestone_id(milestone.id).each do |event| + event.target_id = nil + event.save + end + end end end diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml index f5f621507b8..b6424df55cd 100644 --- a/app/views/groups/milestones/index.html.haml +++ b/app/views/groups/milestones/index.html.haml @@ -5,7 +5,7 @@ .nav-controls = render 'shared/milestones_sort_dropdown' - - if can?(current_user, :admin_milestones, @group) + - if can?(current_user, :admin_milestone, @group) = link_to "New milestone", new_group_milestone_path(@group), class: "btn btn-new" .milestones diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 2a9e20c2caa..0a684f9016a 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -43,18 +43,7 @@ - else = link_to 'Reopen milestone', project_milestone_path(@project, @milestone, milestone: {state_event: :activate }), method: :put, class: "btn btn-reopen btn-nr btn-grouped" - %button.js-delete-milestone-button.btn.btn-grouped.btn-danger{ data: { toggle: 'modal', - target: '#delete-milestone-modal', - milestone_id: @milestone.id, - milestone_title: markdown_field(@milestone, :title), - milestone_url: project_milestone_path(@project, @milestone), - milestone_issue_count: @milestone.issues.count, - milestone_merge_request_count: @milestone.merge_requests.count }, - disabled: true } - = _('Delete') - = icon('spin spinner', class: 'js-loading-icon hidden' ) - - #delete-milestone-modal + = render 'shared/milestones/delete_button' %a.btn.btn-default.btn-grouped.float-right.d-block.d-sm-none.js-sidebar-toggle{ href: "#" } = icon('angle-double-left') diff --git a/app/views/shared/milestones/_delete_button.html.haml b/app/views/shared/milestones/_delete_button.html.haml new file mode 100644 index 00000000000..e236c24b088 --- /dev/null +++ b/app/views/shared/milestones/_delete_button.html.haml @@ -0,0 +1,14 @@ +- milestone_url = @milestone.project_milestone? ? project_milestone_path(@project, @milestone) : group_milestone_path(@group, @milestone) + +%button.js-delete-milestone-button.btn.btn-grouped.btn-danger{ data: { toggle: 'modal', + target: '#delete-milestone-modal', + milestone_id: @milestone.id, + milestone_title: markdown_field(@milestone, :title), + milestone_url: milestone_url, + milestone_issue_count: @milestone.issues.count, + milestone_merge_request_count: @milestone.merge_requests.count }, + disabled: true } + = _('Delete') + = icon('spin spinner', class: 'js-loading-icon hidden' ) + +#delete-milestone-modal diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index c559945a9c9..b33bf5df707 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -49,7 +49,7 @@ - unless milestone.active? = link_to 'Reopen Milestone', project_milestone_path(@project, milestone, {state_event: :activate }), method: :put, class: "btn btn-grouped btn-reopen" - if @group - - if can?(current_user, :admin_milestones, @group) + - if can?(current_user, :admin_milestone, @group) - if milestone.closed? = link_to 'Reopen Milestone', group_milestone_route(milestone, {state_event: :activate }), method: :put, class: "btn btn-sm btn-grouped btn-reopen" - else diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 320e3788a0f..0499b04a482 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -23,7 +23,7 @@ = milestone_date_range(milestone) - if group .float-right - - if can?(current_user, :admin_milestones, group) + - if can?(current_user, :admin_milestone, group) - if milestone.group_milestone? = link_to edit_group_milestone_path(group, milestone), class: "btn btn btn-grouped" do Edit @@ -32,6 +32,9 @@ - else = link_to 'Reopen Milestone', group_milestone_route(milestone, {state_event: :activate }), method: :put, class: "btn btn-grouped btn-reopen" + - unless is_dynamic_milestone + = render 'shared/milestones/delete_button' + = render 'shared/milestones/deprecation_message' if is_dynamic_milestone .detail-page-description.milestone-detail diff --git a/changelogs/unreleased/issue_36138.yml b/changelogs/unreleased/issue_36138.yml new file mode 100644 index 00000000000..2fb2eea65f5 --- /dev/null +++ b/changelogs/unreleased/issue_36138.yml @@ -0,0 +1,5 @@ +--- +title: Allow to delete group milestones +merge_request: +author: +type: added diff --git a/config/routes/group.rb b/config/routes/group.rb index 25fbb38ba87..d7313e43786 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -37,7 +37,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do post :toggle_subscription, on: :member end - resources :milestones, constraints: { id: %r{[^/]+} }, only: [:index, :show, :edit, :update, :new, :create] do + resources :milestones, constraints: { id: %r{[^/]+} } do member do get :merge_requests get :participants diff --git a/doc/api/group_milestones.md b/doc/api/group_milestones.md index 152929b7614..e396f4411e6 100644 --- a/doc/api/group_milestones.md +++ b/doc/api/group_milestones.md @@ -96,6 +96,19 @@ Parameters: - `start_date` (optional) - The start date of the milestone - `state_event` (optional) - The state event of the milestone (close|activate) +## Delete group milestone + +Only for user with developer access to the group. + +``` +DELETE /groups/:id/milestones/:milestone_id +``` + +Parameters: + +- `id` (required) - The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user +- `milestone_id` (required) - The ID of the group's milestone + ## Get all issues assigned to a single milestone Gets all issues assigned to a single group milestone. diff --git a/lib/api/group_milestones.rb b/lib/api/group_milestones.rb index 93fa0b95857..4b4352c2b27 100644 --- a/lib/api/group_milestones.rb +++ b/lib/api/group_milestones.rb @@ -41,7 +41,7 @@ module API use :optional_params end post ":id/milestones" do - authorize! :admin_milestones, user_group + authorize! :admin_milestone, user_group create_milestone_for(user_group) end @@ -53,11 +53,21 @@ module API use :update_params end put ":id/milestones/:milestone_id" do - authorize! :admin_milestones, user_group + authorize! :admin_milestone, user_group update_milestone_for(user_group) end + desc 'Remove a project milestone' + delete ":id/milestones/:milestone_id" do + authorize! :admin_milestone, user_group + + milestone = user_group.milestones.find(params[:milestone_id]) + Milestones::DestroyService.new(user_group, current_user).execute(milestone) + + status(204) + end + desc 'Get all issues for a single group milestone' do success Entities::IssueBasic end diff --git a/lib/api/project_milestones.rb b/lib/api/project_milestones.rb index 306dc0e63d7..72cf32d7717 100644 --- a/lib/api/project_milestones.rb +++ b/lib/api/project_milestones.rb @@ -64,7 +64,8 @@ module API delete ":id/milestones/:milestone_id" do authorize! :admin_milestone, user_project - user_project.milestones.find(params[:milestone_id]).destroy + milestone = user_project.milestones.find(params[:milestone_id]) + Milestones::DestroyService.new(user_project, current_user).execute(milestone) status(204) end 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" } |