Commit 0dacf3c1 authored by dixpac's avatar dixpac

Fix inconsistent naming for services that delete things

* Changed name of delete_user_service and worker to destroy
* Move and change delete_group_service to Groups::DestroyService
* Rename Notes::DeleteService to Notes::DestroyService
parent a965edb8
...@@ -49,7 +49,7 @@ class Admin::GroupsController < Admin::ApplicationController ...@@ -49,7 +49,7 @@ class Admin::GroupsController < Admin::ApplicationController
end end
def destroy 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." redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end end
......
...@@ -91,7 +91,7 @@ class GroupsController < Groups::ApplicationController ...@@ -91,7 +91,7 @@ class GroupsController < Groups::ApplicationController
end end
def destroy 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." redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end end
......
...@@ -51,7 +51,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -51,7 +51,7 @@ class Projects::NotesController < Projects::ApplicationController
def destroy def destroy
if note.editable? if note.editable?
Notes::DeleteService.new(project, current_user).execute(note) Notes::DestroyService.new(project, current_user).execute(note)
end end
respond_to do |format| respond_to do |format|
......
...@@ -24,7 +24,7 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -24,7 +24,7 @@ class RegistrationsController < Devise::RegistrationsController
end end
def destroy def destroy
DeleteUserService.new(current_user).execute(current_user) Users::DestroyService.new(current_user).execute(current_user)
respond_to do |format| respond_to do |format|
format.html do format.html do
......
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
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
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
module Notes module Notes
class DeleteService < BaseService class DestroyService < BaseService
def execute(note) def execute(note)
note.destroy note.destroy
end end
......
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
...@@ -6,6 +6,6 @@ class DeleteUserWorker ...@@ -6,6 +6,6 @@ class DeleteUserWorker
delete_user = User.find(delete_user_id) delete_user = User.find(delete_user_id)
current_user = User.find(current_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
end end
...@@ -11,6 +11,6 @@ class GroupDestroyWorker ...@@ -11,6 +11,6 @@ class GroupDestroyWorker
user = User.find(user_id) user = User.find(user_id)
DestroyGroupService.new(group, user).execute Groups::DestroyService.new(group, user).execute
end end
end end
---
title: Fix inconsistent naming for services that delete things
merge_request: 5803
author: dixpac
...@@ -125,7 +125,7 @@ module API ...@@ -125,7 +125,7 @@ module API
delete ":id" do delete ":id" do
group = find_group!(params[:id]) group = find_group!(params[:id])
authorize! :admin_group, group authorize! :admin_group, group
DestroyGroupService.new(group, current_user).execute ::Groups::DestroyService.new(group, current_user).execute
end end
desc 'Get a list of projects in this group.' do desc 'Get a list of projects in this group.' do
......
...@@ -131,7 +131,7 @@ module API ...@@ -131,7 +131,7 @@ module API
note = user_project.notes.find(params[:note_id]) note = user_project.notes.find(params[:note_id])
authorize! :admin_note, note 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 present note, with: Entities::Note
end end
......
...@@ -293,7 +293,7 @@ module API ...@@ -293,7 +293,7 @@ module API
user = User.find_by(id: params[:id]) user = User.find_by(id: params[:id])
not_found!('User') unless user not_found!('User') unless user
DeleteUserService.new(current_user).execute(user) ::Users::DestroyService.new(current_user).execute(user)
end end
desc 'Block a user. Available only for admins.' desc 'Block a user. Available only for admins.'
......
require 'spec_helper' require 'spec_helper'
describe DestroyGroupService, services: true do describe Groups::DestroyService, services: true do
include DatabaseConnectionHelpers include DatabaseConnectionHelpers
let!(:user) { create(:user) } let!(:user) { create(:user) }
...@@ -43,9 +43,9 @@ describe DestroyGroupService, services: true do ...@@ -43,9 +43,9 @@ describe DestroyGroupService, services: true do
def destroy_group(group, user, async) def destroy_group(group, user, async)
if async if async
DestroyGroupService.new(group, user).async_execute Groups::DestroyService.new(group, user).async_execute
else else
DestroyGroupService.new(group, user).execute Groups::DestroyService.new(group, user).execute
end end
end end
end end
...@@ -80,7 +80,7 @@ describe DestroyGroupService, services: true do ...@@ -80,7 +80,7 @@ describe DestroyGroupService, services: true do
# Kick off the initial group destroy in a new thread, so that # Kick off the initial group destroy in a new thread, so that
# it doesn't share this spec's database transaction. # 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| group_record = run_with_new_database_connection do |conn|
conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first
......
require 'spec_helper' require 'spec_helper'
describe Notes::DeleteService, services: true do describe Notes::DestroyService, services: true do
describe '#execute' do describe '#execute' do
it 'deletes a note' do it 'deletes a note' do
project = create(:empty_project) project = create(:empty_project)
......
require 'spec_helper' require 'spec_helper'
describe DeleteUserService, services: true do describe Users::DestroyService, services: true do
describe "Deletes a user and all their personal projects" do describe "Deletes a user and all their personal projects" do
let!(:user) { create(:user) } let!(:user) { create(:user) }
let!(:current_user) { create(:user) } let!(:current_user) { create(:user) }
let!(:namespace) { create(:namespace, owner: user) } let!(:namespace) { create(:namespace, owner: user) }
let!(:project) { create(:project, namespace: namespace) } let!(:project) { create(:project, namespace: namespace) }
let(:service) { described_class.new(current_user) }
context 'no options are given' do context 'no options are given' do
it 'deletes the user' 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_data['email'].to eq(user.email) }
expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
...@@ -19,7 +20,7 @@ describe DeleteUserService, services: true do ...@@ -19,7 +20,7 @@ describe DeleteUserService, services: true do
it 'will delete the project in the near future' do it 'will delete the project in the near future' do
expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once
DeleteUserService.new(current_user).execute(user) service.execute(user)
end end
end end
...@@ -30,7 +31,7 @@ describe DeleteUserService, services: true do ...@@ -30,7 +31,7 @@ describe DeleteUserService, services: true do
before do before do
solo_owned.group_members = [member] solo_owned.group_members = [member]
DeleteUserService.new(current_user).execute(user) service.execute(user)
end end
it 'does not delete the user' do it 'does not delete the user' do
...@@ -45,7 +46,7 @@ describe DeleteUserService, services: true do ...@@ -45,7 +46,7 @@ describe DeleteUserService, services: true do
before do before do
solo_owned.group_members = [member] 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 end
it 'deletes solo owned groups' do it 'deletes solo owned groups' do
......
...@@ -5,14 +5,14 @@ describe DeleteUserWorker do ...@@ -5,14 +5,14 @@ describe DeleteUserWorker do
let!(:current_user) { create(:user) } let!(:current_user) { create(:user) }
it "calls the DeleteUserWorker with the params it was given" do 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, {}) with(user, {})
DeleteUserWorker.new.perform(current_user.id, user.id) DeleteUserWorker.new.perform(current_user.id, user.id)
end end
it "uses symbolized keys" do 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") with(user, test: "test")
DeleteUserWorker.new.perform(current_user.id, user.id, "test" => "test") DeleteUserWorker.new.perform(current_user.id, user.id, "test" => "test")
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment