Merge branch '28694-hard-delete-user-from-admin-panel' into 'master'

Allow users to be hard-deleted from the admin panel

Closes #28694

See merge request !11874

app/controllers/admin/users_controller.rb

@@ -138,7 +138,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def destroy
-    DeleteUserWorker.perform_async(current_user.id, user.id)
+    user.delete_async(deleted_by: current_user, params: params.permit(:hard_delete))
 
     respond_to do |format|
       format.html { redirect_to admin_users_path, notice: "The user is being deleted." }

app/controllers/registrations_controller.rb

@@ -25,7 +25,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def destroy
-    DeleteUserWorker.perform_async(current_user.id, current_user.id)
+    current_user.delete_async(deleted_by: current_user)
 
     respond_to do |format|
       format.html do

app/models/abuse_report.rb

@@ -15,8 +15,7 @@ class AbuseReport < ActiveRecord::Base
   alias_method :author, :reporter
 
   def remove_user(deleted_by:)
-    user.block
-    DeleteUserWorker.perform_async(deleted_by.id, user.id, delete_solo_owned_groups: true, hard_delete: true)
+    user.delete_async(deleted_by: deleted_by, params: { hard_delete: true })
   end
 
   def notify

app/models/spam_log.rb

@@ -4,8 +4,7 @@ class SpamLog < ActiveRecord::Base
   validates :user, presence: true
 
   def remove_user(deleted_by:)
-    user.block
-    DeleteUserWorker.perform_async(deleted_by.id, user.id, delete_solo_owned_groups: true, hard_delete: true)
+    user.delete_async(deleted_by: deleted_by, params: { hard_delete: true })
   end
 
   def text

app/models/user.rb

@@ -809,6 +809,11 @@ class User < ActiveRecord::Base
     system_hook_service.execute_hooks_for(self, :destroy)
   end
 
+  def delete_async(deleted_by:, params: {})
+    block if params[:hard_delete]
+    DeleteUserWorker.perform_async(deleted_by.id, id, params)
+  end
+
   def notification_service
     NotificationService.new
   end

app/services/users/destroy_service.rb

@@ -6,12 +6,27 @@ module Users
       @current_user = current_user
     end
 
+    # Synchronously destroys +user+
+    #
+    # The operation will fail if the user is the sole owner of any groups. To
+    # force the groups to be destroyed, pass `delete_solo_owned_groups: true` in
+    # +options+.
+    #
+    # The user's contributions will be migrated to a global ghost user. To
+    # force the contributions to be destroyed, pass `hard_delete: true` in
+    # +options+.
+    #
+    # `hard_delete: true` implies `delete_solo_owned_groups: true`.  To perform
+    # a hard deletion without destroying solo-owned groups, pass
+    # `delete_solo_owned_groups: false, hard_delete: true` in +options+.
     def execute(user, options = {})
+      delete_solo_owned_groups = options.fetch(:delete_solo_owned_groups, options[:hard_delete])
+
       unless Ability.allowed?(current_user, :destroy_user, user)
         raise Gitlab::Access::AccessDeniedError, "#{current_user} tried to destroy user #{user}!"
       end
 
-      if !options[:delete_solo_owned_groups] && user.solo_owned_groups.present?
+      if !delete_solo_owned_groups && user.solo_owned_groups.present?
         user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user'
         return user
       end

app/views/admin/users/_user.html.haml

@@ -34,9 +34,15 @@
             - if user.access_locked?
               %li
                 = link_to 'Unlock', unlock_admin_user_path(user), method: :put, class: 'btn-grouped btn btn-xs btn-success', data: { confirm: 'Are you sure?' }
-          - if user.can_be_removed? && can?(current_user, :destroy_user, user)
+          - if can?(current_user, :destroy_user, user)
             %li.divider
+            - if user.can_be_removed?
               %li
-              = link_to 'Delete user', [:admin, user], data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and groups linked to this user will also be removed! Consider cancelling this deletion and blocking the user instead. Are you sure?" },
+                = link_to 'Remove user', admin_user_path(user),
+                    data: { confirm: "USER #{user.name} WILL BE REMOVED! Are you sure?" },
+                    method: :delete
+            %li
+              = link_to 'Remove user and contributions', admin_user_path(user, hard_delete: true),
+                  data: { confirm: "USER #{user.name} WILL BE REMOVED! All issues, merge requests and comments authored by this user, and groups owned solely by them, will also be removed! Are you sure?" },
                   class: 'btn btn-remove btn-block',
                   method: :delete

app/views/admin/users/show.html.haml

@@ -177,7 +177,7 @@
             %p Deleting a user has the following effects:
             = render 'users/deletion_guidance', user: @user
             %br
-            = link_to 'Remove user', [:admin, @user], data: { confirm: "USER #{@user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-remove"
+            = link_to 'Remove user', admin_user_path(@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?
               %p
@@ -188,3 +188,22 @@
             - else
               %p
                 You don't have access to delete this user.
+
+      .panel.panel-danger
+        .panel-heading
+          Remove user and contributions
+        .panel-body
+          - if can?(current_user, :destroy_user, @user)
+            %p
+              This option deletes the user and any contributions that
+              would usually be moved to the
+              = succeed "." do
+                = link_to "system ghost user", help_page_path("user/profile/account/delete_account")
+              As well as the user's personal projects, groups owned solely by
+              the user, and projects in them, will also be removed. Commits
+              to other projects are unaffected.
+            %br
+            = link_to 'Remove user and contributions', admin_user_path(@user, hard_delete: true), data: { confirm: "USER #{@user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-remove"
+          - else
+            %p
+              You don't have access to delete this user.

changelogs/unreleased/28694-hard-delete-user-from-admin-panel.yml

+---
+title: Allow users to be hard-deleted from the admin panel
+merge_request: 11874
+author:

doc/user/profile/account/delete_account.md

@@ -25,7 +25,8 @@ Instead of being deleted, these records will be moved to a system-wide
 When a user is deleted from an abuse report or spam log, these associated
 records are not ghosted and will be removed, along with any groups the user
 is a sole owner of. Administrators can also request this behaviour when
-deleting users from the [API](../../../api/users.md#user-deletion)
+deleting users from the [API](../../../api/users.md#user-deletion) or the
+admin area.
 
 [ce-7393]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7393
 [ce-10273]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10273

lib/api/users.rb

@@ -293,7 +293,7 @@ module API
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        DeleteUserWorker.perform_async(current_user.id, user.id, hard_delete: params[:hard_delete])
+        user.delete_async(deleted_by: current_user, params: params)
       end
 
       desc 'Block a user. Available only for admins.'

spec/controllers/admin/users_controller_spec.rb

@@ -10,15 +10,26 @@ describe Admin::UsersController do
 
   describe 'DELETE #user with projects' do
     let(:project) { create(:empty_project, namespace: user.namespace) }
+    let!(:issue) { create(:issue, author: user) }
 
     before do
       project.team << [user, :developer]
     end
 
-    it 'deletes user' do
+    it 'deletes user and ghosts their contributions' do
       delete :destroy, id: user.username, format: :json
+
+      expect(response).to have_http_status(200)
+      expect(User.exists?(user.id)).to be_falsy
+      expect(issue.reload.author).to be_ghost
+    end
+
+    it 'deletes the user and their contributions when hard delete is specified' do
+      delete :destroy, id: user.username, hard_delete: true, format: :json
+
       expect(response).to have_http_status(200)
-      expect { User.find(user.id) }.to raise_exception(ActiveRecord::RecordNotFound)
+      expect(User.exists?(user.id)).to be_falsy
+      expect(Issue.exists?(issue.id)).to be_falsy
     end
   end
 

spec/controllers/registrations_controller_spec.rb

@@ -77,7 +77,7 @@ describe RegistrationsController do
     end
 
     it 'schedules the user for destruction' do
-      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id)
+      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id, {})
 
       post(:destroy)
 

spec/features/admin/admin_users_spec.rb

@@ -22,7 +22,8 @@ describe "Admin::Users", feature: true do
       expect(page).to have_content(user.email)
       expect(page).to have_content(user.name)
       expect(page).to have_link('Block', href: block_admin_user_path(user))
-      expect(page).to have_link('Delete', href: admin_user_path(user))
+      expect(page).to have_link('Remove user', href: admin_user_path(user))
+      expect(page).to have_link('Remove user and contributions', href: admin_user_path(user, hard_delete: true))
     end
 
     describe 'Two-factor Authentication filters' do
@@ -116,6 +117,9 @@ describe "Admin::Users", feature: true do
 
       expect(page).to have_content(user.email)
       expect(page).to have_content(user.name)
+      expect(page).to have_link('Block user', href: block_admin_user_path(user))
+      expect(page).to have_link('Remove user', href: admin_user_path(user))
+      expect(page).to have_link('Remove user and contributions', href: admin_user_path(user, hard_delete: true))
     end
 
     describe 'Impersonation' do

spec/models/abuse_report_spec.rb

@@ -28,9 +28,7 @@ RSpec.describe AbuseReport, type: :model do
     end
 
     it 'lets a worker delete the user' do
-      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, subject.user.id,
-                                                              delete_solo_owned_groups: true,
-                                                              hard_delete: true)
+      expect(DeleteUserWorker).to receive(:perform_async).with(user.id, subject.user.id, hard_delete: true)
 
       subject.remove_user(deleted_by: user)
     end