Commit 722631a9 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'issue_36138' into 'master'

Allow to delete group milestones

Closes #36138

See merge request gitlab-org/gitlab-ce!21057
parents 881ba3f5 abb50ff4
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();
});
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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')
......
- 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
......@@ -52,7 +52,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
......
......@@ -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
......
---
title: Allow to delete group milestones
merge_request:
author:
type: added
......@@ -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
......
......@@ -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.
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -34,7 +34,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
......
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
......@@ -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
[
......
......@@ -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
......
......@@ -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
......
......@@ -196,6 +196,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" }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment