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>

9 files changed, 68 insertions, 33 deletions

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'
               &nbsp;
-              = 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