Commit fa160c26 authored by Michael Kozono's avatar Michael Kozono

Merge branch 'sh-add-delete-confirmation' into 'master'

Make it harder to delete issuables accidentally

Closes #62387

See merge request gitlab-org/gitlab-ce!32376
parents 873a344e f6c7e380
......@@ -300,9 +300,9 @@ export default {
this.closeRecaptcha();
},
deleteIssuable() {
deleteIssuable(payload) {
this.service
.deleteIssuable()
.deleteIssuable(payload)
.then(res => res.data)
.then(data => {
// Stop the poll so we don't get 404's with the issuable not existing
......
......@@ -55,7 +55,7 @@ export default {
if (window.confirm(confirmMessage)) {
this.deleteLoading = true;
eventHub.$emit('delete.issuable');
eventHub.$emit('delete.issuable', { destroy_confirm: true });
}
},
},
......
......@@ -10,8 +10,8 @@ export default class Service {
return axios.get(this.realtimeEndpoint);
}
deleteIssuable() {
return axios.delete(this.endpoint);
deleteIssuable(payload) {
return axios.delete(this.endpoint, { params: payload });
}
updateIssuable(data) {
......
......@@ -6,6 +6,7 @@ module IssuableActions
included do
before_action :authorize_destroy_issuable!, only: :destroy
before_action :check_destroy_confirmation!, only: :destroy
before_action :authorize_admin_issuable!, only: :bulk_update
before_action only: :show do
push_frontend_feature_flag(:scoped_labels, default_enabled: true)
......@@ -91,6 +92,33 @@ module IssuableActions
end
end
def check_destroy_confirmation!
return true if params[:destroy_confirm]
error_message = "Destroy confirmation not provided for #{issuable.human_class_name}"
exception = RuntimeError.new(error_message)
Gitlab::Sentry.track_acceptable_exception(
exception,
extra: {
project_path: issuable.project.full_path,
issuable_type: issuable.class.name,
issuable_id: issuable.id
}
)
index_path = polymorphic_path([parent, issuable.class])
respond_to do |format|
format.html do
flash[:notice] = error_message
redirect_to index_path
end
format.json do
render json: { errors: error_message }, status: :unprocessable_entity
end
end
end
def bulk_update
result = Issuable::BulkUpdateService.new(current_user, bulk_update_params).execute(resource_name)
quantity = result[:count]
......
......@@ -66,7 +66,7 @@
= link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class]), class: 'btn btn-cancel'
- else
- if can?(current_user, :"destroy_#{issuable.to_ability_name}", @project)
= link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.human_class_name} will be removed! Are you sure?" }, method: :delete, class: 'btn btn-danger btn-grouped'
= link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable], params: { destroy_confirm: true }), data: { confirm: "#{issuable.human_class_name} will be removed! Are you sure?" }, method: :delete, class: 'btn btn-danger btn-grouped'
= link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
%span.append-right-10
......
---
title: Make it harder to delete issuables accidentally
merge_request: 32376
author:
type: fixed
......@@ -1084,16 +1084,41 @@ describe Projects::IssuesController do
end
it "deletes the issue" do
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
expect(response).to have_gitlab_http_status(302)
expect(controller).to set_flash[:notice].to(/The issue was successfully deleted\./)
end
it "deletes the issue" do
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
expect(response).to have_gitlab_http_status(302)
expect(controller).to set_flash[:notice].to(/The issue was successfully deleted\./)
end
it "prevents deletion if destroy_confirm is not set" do
expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
expect(response).to have_gitlab_http_status(302)
expect(controller).to set_flash[:notice].to('Destroy confirmation not provided for issue')
end
it "prevents deletion in JSON format if destroy_confirm is not set" do
expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, format: 'json' }
expect(response).to have_gitlab_http_status(422)
expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for issue' })
end
it 'delegates the update of the todos count cache to TodoService' do
expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
end
end
end
......
......@@ -573,16 +573,34 @@ describe Projects::MergeRequestsController do
end
it "deletes the merge request" do
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
expect(response).to have_gitlab_http_status(302)
expect(controller).to set_flash[:notice].to(/The merge request was successfully deleted\./)
end
it "prevents deletion if destroy_confirm is not set" do
expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
expect(response).to have_gitlab_http_status(302)
expect(controller).to set_flash[:notice].to('Destroy confirmation not provided for merge request')
end
it "prevents deletion in JSON format if destroy_confirm is not set" do
expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, format: 'json' }
expect(response).to have_gitlab_http_status(422)
expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for merge request' })
end
it 'delegates the update of the todos count cache to TodoService' do
expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
end
end
end
......
......@@ -104,7 +104,7 @@ describe('Edit Actions components', () => {
spyOn(window, 'confirm').and.returnValue(true);
vm.$el.querySelector('.btn-danger').click();
expect(eventHub.$emit).toHaveBeenCalledWith('delete.issuable');
expect(eventHub.$emit).toHaveBeenCalledWith('delete.issuable', { destroy_confirm: true });
});
it('shows loading icon after clicking delete button', done => {
......
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