summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorFelipe Artur <felipefac@gmail.com>2018-08-06 16:02:52 -0300
committerFelipe Artur <felipefac@gmail.com>2018-08-06 17:31:47 -0300
commitabb50ff4710e264c0c700df88757ee3ab1cf7dfb (patch)
treea77ce62f4384becfe7b36fa1ae7fea700895812f
parentcb2e07309b4e61501a44c3568155bdb73252338f (diff)
downloadgitlab-ce-abb50ff4710e264c0c700df88757ee3ab1cf7dfb.tar.gz
Allow to delete group milestonesissue_36138
-rw-r--r--app/assets/javascripts/pages/groups/milestones/show/index.js4
-rw-r--r--app/assets/javascripts/pages/milestones/shared/components/delete_milestone_modal.vue6
-rw-r--r--app/controllers/groups/milestones_controller.rb17
-rw-r--r--app/policies/group_policy.rb2
-rw-r--r--app/services/milestones/destroy_service.rb20
-rw-r--r--app/views/groups/milestones/index.html.haml2
-rw-r--r--app/views/projects/milestones/show.html.haml13
-rw-r--r--app/views/shared/milestones/_delete_button.html.haml14
-rw-r--r--app/views/shared/milestones/_milestone.html.haml2
-rw-r--r--app/views/shared/milestones/_top.html.haml5
-rw-r--r--changelogs/unreleased/issue_36138.yml5
-rw-r--r--config/routes/group.rb2
-rw-r--r--doc/api/group_milestones.md13
-rw-r--r--lib/api/group_milestones.rb14
-rw-r--r--lib/api/project_milestones.rb3
-rw-r--r--spec/controllers/groups/milestones_controller_spec.rb11
-rw-r--r--spec/factories/milestones.rb2
-rw-r--r--spec/features/milestones/user_deletes_milestone_spec.rb44
-rw-r--r--spec/policies/group_policy_spec.rb2
-rw-r--r--spec/requests/api/project_milestones_spec.rb13
-rw-r--r--spec/services/milestones/destroy_service_spec.rb28
-rw-r--r--spec/support/api/milestones_shared_examples.rb18
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" }