From 0dacf3c169a85e6f3a1c70f3f5e377d47f770d19 Mon Sep 17 00:00:00 2001 From: dixpac Date: Sat, 13 Aug 2016 14:45:31 +0200 Subject: [PATCH] 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 --- app/controllers/admin/groups_controller.rb | 2 +- app/controllers/groups_controller.rb | 2 +- app/controllers/projects/notes_controller.rb | 2 +- app/controllers/registrations_controller.rb | 2 +- app/services/delete_user_service.rb | 31 ----------------- app/services/destroy_group_service.rb | 29 ---------------- app/services/groups/destroy_service.rb | 25 ++++++++++++++ .../{delete_service.rb => destroy_service.rb} | 2 +- app/services/users/destroy_service.rb | 33 +++++++++++++++++++ app/workers/delete_user_worker.rb | 2 +- app/workers/group_destroy_worker.rb | 2 +- .../unreleased/rename_delete_services.yml | 4 +++ lib/api/groups.rb | 2 +- lib/api/notes.rb | 2 +- lib/api/users.rb | 2 +- .../destroy_service_spec.rb} | 16 ++++----- ...ervice_spec.rb => destroy_service_spec.rb} | 2 +- .../destroy_spec.rb} | 11 ++++--- spec/workers/delete_user_worker_spec.rb | 4 +-- 19 files changed, 89 insertions(+), 86 deletions(-) delete mode 100644 app/services/delete_user_service.rb delete mode 100644 app/services/destroy_group_service.rb create mode 100644 app/services/groups/destroy_service.rb rename app/services/notes/{delete_service.rb => destroy_service.rb} (66%) create mode 100644 app/services/users/destroy_service.rb create mode 100644 changelogs/unreleased/rename_delete_services.yml rename spec/services/{destroy_group_service_spec.rb => groups/destroy_service_spec.rb} (87%) rename spec/services/notes/{delete_service_spec.rb => destroy_service_spec.rb} (88%) rename spec/services/{delete_user_service_spec.rb => users/destroy_spec.rb} (84%) diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index b7722a1d15d..8d0198f8797 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 264b14713fb..9b6c3dd33b8 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 c5d93ce25bc..b033f7b5ea9 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 bf27f3d4d51..cf6aa37936b 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 eaff88d6463..00000000000 --- 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 2316c57bf1e..00000000000 --- 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 00000000000..7f2d28086f5 --- /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 a673e8e9dde..b819bd17039 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 00000000000..2d11305be13 --- /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 3194c389b3d..5483bbb210b 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 a49a5fd0855..07e82767b06 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 00000000000..686a1ef3d55 --- /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 7682d286866..50dadd94b04 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 4d2a8f48267..8beccaaabd1 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 0ed468626b7..4980a90f952 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 538e85cdc89..f86189b68e9 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 1d0a747a480..f53f96e0c2b 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 418a12a83a9..46e58393218 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 14c56521280..0765573408c 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")