Merge branch 'rename_delete_services' into 'master'

Fix inconsistent naming for services that delete things

See merge request !5803

app/controllers/admin/groups_controller.rb

@@ -49,7 +49,7 @@ class Admin::GroupsController < Admin::ApplicationController
   end
 
   def destroy
-    DestroyGroupService.new(@group, current_user).async_execute
+    Groups::DestroyService.new(@group, current_user).async_execute
 
     redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion."
   end

app/controllers/groups_controller.rb

@@ -91,7 +91,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def destroy
-    DestroyGroupService.new(@group, current_user).async_execute
+    Groups::DestroyService.new(@group, current_user).async_execute
 
     redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
   end

app/controllers/projects/notes_controller.rb

@@ -51,7 +51,7 @@ class Projects::NotesController < Projects::ApplicationController
 
   def destroy
     if note.editable?
-      Notes::DeleteService.new(project, current_user).execute(note)
+      Notes::DestroyService.new(project, current_user).execute(note)
     end
 
     respond_to do |format|

app/controllers/registrations_controller.rb

@@ -24,7 +24,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def destroy
-    DeleteUserService.new(current_user).execute(current_user)
+    Users::DestroyService.new(current_user).execute(current_user)
 
     respond_to do |format|
       format.html do

app/services/delete_user_service.rb

@@ -1,31 +0,0 @@
-class DeleteUserService
-  attr_accessor :current_user
-
-  def initialize(current_user)
-    @current_user = current_user
-  end
-
-  def execute(user, options = {})
-    if !options[: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
-
-    user.solo_owned_groups.each do |group|
-      DestroyGroupService.new(group, current_user).execute
-    end
-
-    user.personal_projects.each do |project|
-      # Skip repository removal because we remove directory with namespace
-      # that contain all this repositories
-      ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
-    end
-
-    # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
-    namespace = user.namespace
-    user_data = user.destroy
-    namespace.really_destroy!
-
-    user_data
-  end
-end

app/services/destroy_group_service.rb

@@ -1,29 +0,0 @@
-class DestroyGroupService
-  attr_accessor :group, :current_user
-
-  def initialize(group, user)
-    @group, @current_user = group, user
-  end
-
-  def async_execute
-    # Soft delete via paranoia gem
-    group.destroy
-    job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
-    Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
-  end
-
-  def execute
-    group.projects.each do |project|
-      # Execute the destruction of the models immediately to ensure atomic cleanup.
-      # Skip repository removal because we remove directory with namespace
-      # that contain all these repositories
-      ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
-    end
-
-    group.children.each do |group|
-      DestroyGroupService.new(group, current_user).async_execute
-    end
-
-    group.really_destroy!
-  end
-end

app/services/groups/destroy_service.rb

@@ -0,0 +1,25 @@
+module Groups
+  class DestroyService < Groups::BaseService
+    def async_execute
+      # Soft delete via paranoia gem
+      group.destroy
+      job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
+      Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
+    end
+
+    def execute
+      group.projects.each do |project|
+        # Execute the destruction of the models immediately to ensure atomic cleanup.
+        # Skip repository removal because we remove directory with namespace
+        # that contain all these repositories
+        ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
+      end
+
+      group.children.each do |group|
+        DestroyService.new(group, current_user).async_execute
+      end
+
+      group.really_destroy!
+    end
+  end
+end

app/services/notes/destroy_service.rb

@@ -1,5 +1,5 @@
 module Notes
-  class DeleteService < BaseService
+  class DestroyService < BaseService
     def execute(note)
       note.destroy
     end

app/services/users/destroy_service.rb

@@ -0,0 +1,33 @@
+module Users
+  class DestroyService
+    attr_accessor :current_user
+
+    def initialize(current_user)
+      @current_user = current_user
+    end
+
+    def execute(user, options = {})
+      if !options[: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
+
+      user.solo_owned_groups.each do |group|
+        Groups::DestroyService.new(group, current_user).execute
+      end
+
+      user.personal_projects.each do |project|
+        # Skip repository removal because we remove directory with namespace
+        # that contain all this repositories
+        ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
+      end
+
+      # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
+      namespace = user.namespace
+      user_data = user.destroy
+      namespace.really_destroy!
+
+      user_data
+    end
+  end
+end

app/workers/delete_user_worker.rb

@@ -6,6 +6,6 @@ class DeleteUserWorker
     delete_user  = User.find(delete_user_id)
     current_user = User.find(current_user_id)
 
-    DeleteUserService.new(current_user).execute(delete_user, options.symbolize_keys)
+    Users::DestroyService.new(current_user).execute(delete_user, options.symbolize_keys)
   end
 end

app/workers/group_destroy_worker.rb

@@ -11,6 +11,6 @@ class GroupDestroyWorker
 
     user = User.find(user_id)
 
-    DestroyGroupService.new(group, user).execute
+    Groups::DestroyService.new(group, user).execute
   end
 end

changelogs/unreleased/rename_delete_services.yml

@@ -0,0 +1,4 @@
+---
+title: Fix inconsistent naming for services that delete things
+merge_request: 5803
+author: dixpac

lib/api/groups.rb

@@ -125,7 +125,7 @@ module API
       delete ":id" do
         group = find_group!(params[:id])
         authorize! :admin_group, group
-        DestroyGroupService.new(group, current_user).execute
+        ::Groups::DestroyService.new(group, current_user).execute
       end
 
       desc 'Get a list of projects in this group.' do

lib/api/notes.rb

@@ -131,7 +131,7 @@ module API
           note = user_project.notes.find(params[:note_id])
           authorize! :admin_note, note
 
-          ::Notes::DeleteService.new(user_project, current_user).execute(note)
+          ::Notes::DestroyService.new(user_project, current_user).execute(note)
 
           present note, with: Entities::Note
         end

lib/api/users.rb

@@ -293,7 +293,7 @@ module API
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        DeleteUserService.new(current_user).execute(user)
+        ::Users::DestroyService.new(current_user).execute(user)
       end
 
       desc 'Block a user. Available only for admins.'

spec/services/groups/destroy_service_spec.rb

@@ -1,13 +1,13 @@
 require 'spec_helper'
 
-describe DestroyGroupService, services: true do
+describe Groups::DestroyService, services: true do
   include DatabaseConnectionHelpers
 
-  let!(:user) { create(:user) }
-  let!(:group) { create(:group) }
-  let!(:project) { create(:project, namespace: group) }
+  let!(:user)         { create(:user) }
+  let!(:group)        { create(:group) }
+  let!(:project)      { create(:project, namespace: group) }
   let!(:gitlab_shell) { Gitlab::Shell.new }
-  let!(:remove_path) { group.path + "+#{group.id}+deleted" }
+  let!(:remove_path)  { group.path + "+#{group.id}+deleted" }
 
   shared_examples 'group destruction' do |async|
     context 'database records' do
@@ -43,9 +43,9 @@ describe DestroyGroupService, services: true do
 
     def destroy_group(group, user, async)
       if async
-        DestroyGroupService.new(group, user).async_execute
+        Groups::DestroyService.new(group, user).async_execute
       else
-        DestroyGroupService.new(group, user).execute
+        Groups::DestroyService.new(group, user).execute
       end
     end
   end
@@ -80,7 +80,7 @@ describe DestroyGroupService, services: true do
 
           # Kick off the initial group destroy in a new thread, so that
           # it doesn't share this spec's database transaction.
-          Thread.new { DestroyGroupService.new(group, user).async_execute }.join(5)
+          Thread.new { Groups::DestroyService.new(group, user).async_execute }.join(5)
 
           group_record = run_with_new_database_connection do |conn|
             conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first

spec/services/notes/destroy_service_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Notes::DeleteService, services: true do
+describe Notes::DestroyService, services: true do
   describe '#execute' do
     it 'deletes a note' do
       project = create(:empty_project)

spec/services/users/destroy_spec.rb

@@ -1,15 +1,16 @@
 require 'spec_helper'
 
-describe DeleteUserService, services: true do
+describe Users::DestroyService, services: true do
   describe "Deletes a user and all their personal projects" do
     let!(:user)         { create(:user) }
     let!(:current_user) { create(:user) }
     let!(:namespace)    { create(:namespace, owner: user) }
     let!(:project)      { create(:project, namespace: namespace) }
+    let(:service)       { described_class.new(current_user) }
 
     context 'no options are given' do
       it 'deletes the user' do
-        user_data = DeleteUserService.new(current_user).execute(user)
+        user_data = service.execute(user)
 
         expect { user_data['email'].to eq(user.email) }
         expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
@@ -19,7 +20,7 @@ describe DeleteUserService, services: true do
       it 'will delete the project in the near future' do
         expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once
 
-        DeleteUserService.new(current_user).execute(user)
+        service.execute(user)
       end
     end
 
@@ -30,7 +31,7 @@ describe DeleteUserService, services: true do
 
       before do
         solo_owned.group_members = [member]
-        DeleteUserService.new(current_user).execute(user)
+        service.execute(user)
       end
 
       it 'does not delete the user' do
@@ -45,7 +46,7 @@ describe DeleteUserService, services: true do
 
       before do
         solo_owned.group_members = [member]
-        DeleteUserService.new(current_user).execute(user, delete_solo_owned_groups: true)
+        service.execute(user, delete_solo_owned_groups: true)
       end
 
       it 'deletes solo owned groups' do

spec/workers/delete_user_worker_spec.rb

@@ -5,14 +5,14 @@ describe DeleteUserWorker do
   let!(:current_user) { create(:user) }
 
   it "calls the DeleteUserWorker with the params it was given" do
-    expect_any_instance_of(DeleteUserService).to receive(:execute).
+    expect_any_instance_of(Users::DestroyService).to receive(:execute).
                                                       with(user, {})
 
     DeleteUserWorker.new.perform(current_user.id, user.id)
   end
 
   it "uses symbolized keys" do
-    expect_any_instance_of(DeleteUserService).to receive(:execute).
+    expect_any_instance_of(Users::DestroyService).to receive(:execute).
                                                       with(user, test: "test")
 
     DeleteUserWorker.new.perform(current_user.id, user.id, "test" => "test")