diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index b7722a1d15dc1e58af01edbff63c8f30eded8377..8d0198f879747127930c9ba5cf0ef54e22f1df61 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/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
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 264b14713fb85a759d291288b04e7a6d78f4a2eb..9b6c3dd33b8fa4777b0e640449b26796a3eb0809 100644
--- a/app/controllers/groups_controller.rb
+++ b/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
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index c5d93ce25bcb1a65c0ba8fbdb3b7834ccbd878ad..b033f7b5ea94908f05889a6fdec9fb5ee95d8c9d 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/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|
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index bf27f3d4d514547b5e530e41efcc6d8d732843c3..cf6aa37936b56559ce734d813c3deb118af1f1d9 100644
--- a/app/controllers/registrations_controller.rb
+++ b/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
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
deleted file mode 100644
index eaff88d64632caec372c6148794107b45d75c48b..0000000000000000000000000000000000000000
--- a/app/services/delete_user_service.rb
+++ /dev/null
@@ -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
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb
deleted file mode 100644
index 2316c57bf1e0d81c7919d68ad5e054724333dcba..0000000000000000000000000000000000000000
--- a/app/services/destroy_group_service.rb
+++ /dev/null
@@ -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
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..7f2d28086f539a6a7adc3e4b24bfa142b8f56209
--- /dev/null
+++ b/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
diff --git a/app/services/notes/delete_service.rb b/app/services/notes/destroy_service.rb
similarity index 66%
rename from app/services/notes/delete_service.rb
rename to app/services/notes/destroy_service.rb
index a673e8e9dde2ac568dfeeca01d6f3be77105dbf4..b819bd1703946e8df00fc48389f6757ccdcc6d97 100644
--- a/app/services/notes/delete_service.rb
+++ b/app/services/notes/destroy_service.rb
@@ -1,5 +1,5 @@
module Notes
- class DeleteService < BaseService
+ class DestroyService < BaseService
def execute(note)
note.destroy
end
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..2d11305be138a2fdf19a5f71def2ccf4857da069
--- /dev/null
+++ b/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
diff --git a/app/workers/delete_user_worker.rb b/app/workers/delete_user_worker.rb
index 3194c389b3d40bcee73d4285d1eaf25c9d5ecc31..5483bbb210bae1581a8f6713a9ae4badf5e16f4d 100644
--- a/app/workers/delete_user_worker.rb
+++ b/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
diff --git a/app/workers/group_destroy_worker.rb b/app/workers/group_destroy_worker.rb
index a49a5fd08557ff877a1fa34051d040ad19d0d711..07e82767b062e10b1bbfd44de49a9fc9b15b58d3 100644
--- a/app/workers/group_destroy_worker.rb
+++ b/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
diff --git a/changelogs/unreleased/rename_delete_services.yml b/changelogs/unreleased/rename_delete_services.yml
new file mode 100644
index 0000000000000000000000000000000000000000..686a1ef3d5557c8aa2e96e1a3df398f20bffcc67
--- /dev/null
+++ b/changelogs/unreleased/rename_delete_services.yml
@@ -0,0 +1,4 @@
+---
+title: Fix inconsistent naming for services that delete things
+merge_request: 5803
+author: dixpac
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 7682d2868665f08fd93812fe5d74cf909a9d05cb..50dadd94b044f57ef78d0e30b36dd22ddc9b0d8e 100644
--- a/lib/api/groups.rb
+++ b/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
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 4d2a8f482670c35bd5ea1a5208de01918af8a8da..8beccaaabd1d44689edee10cda84fd85aa14da49 100644
--- a/lib/api/notes.rb
+++ b/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
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 0ed468626b7c13bdbe393320bda05f1caf62ef2e..4980a90f952a3d0e15713e17b220c9323ab81013 100644
--- a/lib/api/users.rb
+++ b/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.'
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
similarity index 87%
rename from spec/services/destroy_group_service_spec.rb
rename to spec/services/groups/destroy_service_spec.rb
index 538e85cdc89b03c994b3df26ebda47a35e283dbb..f86189b68e9ab5f8b66b74a341db1535a7870c4b 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/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
diff --git a/spec/services/notes/delete_service_spec.rb b/spec/services/notes/destroy_service_spec.rb
similarity index 88%
rename from spec/services/notes/delete_service_spec.rb
rename to spec/services/notes/destroy_service_spec.rb
index 1d0a747a480f4dad2589cccaf0f652a438291893..f53f96e0c2bbd777d275a9397fd4b9da92de2845 100644
--- a/spec/services/notes/delete_service_spec.rb
+++ b/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)
diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/users/destroy_spec.rb
similarity index 84%
rename from spec/services/delete_user_service_spec.rb
rename to spec/services/users/destroy_spec.rb
index 418a12a83a94b5f3f4810e927184709c1d960578..46e58393218799fa6b144d4e410d7ffecf382d2f 100644
--- a/spec/services/delete_user_service_spec.rb
+++ b/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
diff --git a/spec/workers/delete_user_worker_spec.rb b/spec/workers/delete_user_worker_spec.rb
index 14c565212803b35ba3b345dedb87dbc53c7a4ef9..0765573408cf3cc403e77d234dba3f89703f7754 100644
--- a/spec/workers/delete_user_worker_spec.rb
+++ b/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")