From 499154518a1555523ed5f203c9ce4bbe6317c9a5 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Thu, 28 May 2015 12:00:02 +0200
Subject: [PATCH] You can not remove user if he/she is an only owner of group
To prevent loose of group data you need to transfer or remove group
first before you can remove user
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
app/controllers/admin/users_controller.rb | 6 +----
app/controllers/registrations_controller.rb | 2 +-
app/models/user.rb | 4 ++++
app/services/delete_user_service.rb | 10 ++++++++
app/views/admin/users/index.html.haml | 9 +++----
app/views/admin/users/show.html.haml | 24 +++++++++++--------
app/views/profiles/accounts/show.html.haml | 26 ++++++++++++---------
lib/api/users.rb | 2 +-
spec/models/user_spec.rb | 18 +++++++++++++-
9 files changed, 68 insertions(+), 33 deletions(-)
create mode 100644 app/services/delete_user_service.rb
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index d36e359934c..06d6d61e907 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -86,11 +86,7 @@ class Admin::UsersController < Admin::ApplicationController
end
def destroy
- # 1. Remove groups where user is the only owner
- user.solo_owned_groups.map(&:destroy)
-
- # 2. Remove user with all authored content including personal projects
- user.destroy
+ DeleteUserService.new.execute(user)
respond_to do |format|
format.html { redirect_to admin_users_path }
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index 830751a989f..6e57fded337 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -6,7 +6,7 @@ class RegistrationsController < Devise::RegistrationsController
end
def destroy
- current_user.destroy
+ DeleteUserService.new.execute(user)
respond_to do |format|
format.html { redirect_to new_user_session_path, notice: "Account successfully removed." }
diff --git a/app/models/user.rb b/app/models/user.rb
index 50ca4bc5acc..c1bb51e86fc 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -689,4 +689,8 @@ class User < ActiveRecord::Base
true
end
+
+ def can_be_removed?
+ !solo_owned_groups.present?
+ end
end
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
new file mode 100644
index 00000000000..d259b4efca6
--- /dev/null
+++ b/app/services/delete_user_service.rb
@@ -0,0 +1,10 @@
+class DeleteUserService
+ def execute(user)
+ if user.solo_owned_groups.present?
+ user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user'
+ user
+ else
+ user.destroy
+ end
+ end
+end
diff --git a/app/views/admin/users/index.html.haml b/app/views/admin/users/index.html.haml
index fe648470233..45dee86b017 100644
--- a/app/views/admin/users/index.html.haml
+++ b/app/views/admin/users/index.html.haml
@@ -79,11 +79,12 @@
%i.fa.fa-envelope
= mail_to user.email, user.email, class: 'light'
- = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-sm"
+ = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-xs"
- unless user == current_user
- if user.blocked?
- = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-sm success"
+ = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-xs btn-success"
- else
- = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-sm btn-remove"
- = link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All tickets linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: "btn btn-sm btn-remove"
+ = link_to 'Block', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs btn-warning"
+ - if user.can_be_removed?
+ = link_to 'Destroy', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All tickets linked to this user will also be removed! Maybe block the user instead? Are you sure?" }, method: :delete, class: "btn btn-xs btn-remove"
= paginate @users, theme: "gitlab"
diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml
index 7fc85206109..f7195ac3326 100644
--- a/app/views/admin/users/show.html.haml
+++ b/app/views/admin/users/show.html.haml
@@ -140,18 +140,22 @@
.panel-heading
Remove user
.panel-body
- %p Deleting a user has the following effects:
- %ul
- %li All user content like authored issues, snippets, comments will be removed
- - rp = @user.personal_projects.count
- - unless rp.zero?
- %li #{pluralize rp, 'personal project'} will be removed and cannot be restored
+ - if @user.can_be_removed?
+ %p Deleting a user has the following effects:
+ %ul
+ %li All user content like authored issues, snippets, comments will be removed
+ - rp = @user.personal_projects.count
+ - unless rp.zero?
+ %li #{pluralize rp, 'personal project'} will be removed and cannot be restored
+ %br
+ = link_to 'Remove user', [:admin, @user], data: { confirm: "USER #{@user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-remove"
+ - else
- if @user.solo_owned_groups.present?
- %li
- Next groups with all content will be removed:
+ %p
+ This user is currently an owner in these groups:
%strong #{@user.solo_owned_groups.map(&:name).join(', ')}
- %br
- = link_to 'Remove user', [:admin, @user], data: { confirm: "USER #{@user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-remove"
+ %p
+ You must transfer ownership or delete these groups before you can delete this user.
#profile.tab-pane
.row
diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml
index 06bad7dd84a..4d1d50dcbab 100644
--- a/app/views/profiles/accounts/show.html.haml
+++ b/app/views/profiles/accounts/show.html.haml
@@ -91,15 +91,19 @@
%legend
Remove account
%div
- %p Deleting an account has the following effects:
- %ul
- %li All user content like authored issues, snippets, comments will be removed
- - rp = current_user.personal_projects.count
- - unless rp.zero?
- %li #{pluralize rp, 'personal project'} will be removed and cannot be restored
- - if current_user.solo_owned_groups.present?
- %li
- The following groups will be abandoned. You should transfer or remove them:
- %strong #{current_user.solo_owned_groups.map(&:name).join(', ')}
- = link_to 'Delete account', user_registration_path, data: { confirm: "REMOVE #{current_user.name}? Are you sure?" }, method: :delete, class: "btn btn-remove"
+ - if @user.can_be_removed?
+ %p Deleting an account has the following effects:
+ %ul
+ %li All user content like authored issues, snippets, comments will be removed
+ - rp = current_user.personal_projects.count
+ - unless rp.zero?
+ %li #{pluralize rp, 'personal project'} will be removed and cannot be restored
+ = link_to 'Delete account', user_registration_path, data: { confirm: "REMOVE #{current_user.name}? Are you sure?" }, method: :delete, class: "btn btn-remove"
+ - else
+ - if @user.solo_owned_groups.present?
+ %p
+ Your account is currently an owner in these groups:
+ %strong #{@user.solo_owned_groups.map(&:name).join(', ')}
+ %p
+ You must transfer ownership or delete these groups before you can delete yur account.
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 032a5d76e43..7d4c68c7412 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -194,7 +194,7 @@ module API
user = User.find_by(id: params[:id])
if user
- user.destroy
+ DeleteUserService.new.execute(user)
else
not_found!('User')
end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index e1205c18a85..49c7b7d99ce 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -572,7 +572,6 @@ describe User do
end
describe "#contributed_projects_ids" do
-
subject { create(:user) }
let!(:project1) { create(:project) }
let!(:project2) { create(:project, forked_from_project: project3) }
@@ -598,4 +597,21 @@ describe User do
expect(subject.contributed_projects_ids).not_to include(project2.id)
end
end
+
+ describe :can_be_removed? do
+ subject { create(:user) }
+
+ context 'no owned groups' do
+ it { expect(subject.can_be_removed?).to be_truthy }
+ end
+
+ context 'has owned groups' do
+ before do
+ group = create(:group)
+ group.add_owner(subject)
+ end
+
+ it { expect(subject.can_be_removed?).to be_falsey }
+ end
+ end
end
--
GitLab