From 4955a47cb1c52168114364e45a2fccf6bc105452 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 6 Aug 2016 07:25:51 -0700
Subject: [PATCH] Clean up project destruction
Instead of redirecting from the project service to the service and back to the model,
put all destruction code in the service. Also removes a possible source of failure
where run_after_commit may not destroy the project.
---
app/controllers/projects_controller.rb | 2 +-
app/models/project.rb | 10 ----------
app/services/delete_user_service.rb | 2 +-
app/services/destroy_group_service.rb | 2 +-
app/services/projects/destroy_service.rb | 8 ++++++--
lib/api/projects.rb | 2 +-
spec/models/hooks/system_hook_spec.rb | 2 +-
spec/services/delete_user_service_spec.rb | 2 +-
8 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index a6e1aa5ccc1..207f9d6a77f 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -125,7 +125,7 @@ class ProjectsController < Projects::ApplicationController
def destroy
return access_denied! unless can?(current_user, :remove_project, @project)
- ::Projects::DestroyService.new(@project, current_user, {}).pending_delete!
+ ::Projects::DestroyService.new(@project, current_user, {}).async_execute
flash[:alert] = "Project '#{@project.name}' will be deleted."
redirect_to dashboard_projects_path
diff --git a/app/models/project.rb b/app/models/project.rb
index d306f86f783..3b1a53edc75 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1161,16 +1161,6 @@ class Project < ActiveRecord::Base
@wiki ||= ProjectWiki.new(self, self.owner)
end
- def schedule_delete!(user_id, params)
- # Queue this task for after the commit, so once we mark pending_delete it will run
- run_after_commit do
- job_id = ProjectDestroyWorker.perform_async(id, user_id, params)
- Rails.logger.info("User #{user_id} scheduled destruction of project #{path_with_namespace} with job ID #{job_id}")
- end
-
- update_attribute(:pending_delete, true)
- end
-
def running_or_pending_build_count(force: false)
Rails.cache.fetch(['projects', id, 'running_or_pending_build_count'], force: force) do
builds.running_or_pending.count(:all)
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
index ce79287e35a..2f237de813c 100644
--- a/app/services/delete_user_service.rb
+++ b/app/services/delete_user_service.rb
@@ -18,7 +18,7 @@ class DeleteUserService
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).pending_delete!
+ ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end
user.destroy
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb
index 3c42ac61be4..a4ebccb5606 100644
--- a/app/services/destroy_group_service.rb
+++ b/app/services/destroy_group_service.rb
@@ -9,7 +9,7 @@ class DestroyGroupService
group.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).pending_delete!
+ ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end
group.destroy
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 882606e38d0..8a53f65aec1 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -6,8 +6,12 @@ module Projects
DELETED_FLAG = '+deleted'
- def pending_delete!
- project.schedule_delete!(current_user.id, params)
+ def async_execute
+ project.transaction do
+ project.update_attribute(:pending_delete, true)
+ job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
+ Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}")
+ end
end
def execute
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 8fed7db8803..60cfc103afd 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -323,7 +323,7 @@ module API
# DELETE /projects/:id
delete ":id" do
authorize! :remove_project, user_project
- ::Projects::DestroyService.new(user_project, current_user, {}).pending_delete!
+ ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
end
# Mark this project as forked from another
diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb
index 4078b9e4ff5..cbdf7eec082 100644
--- a/spec/models/hooks/system_hook_spec.rb
+++ b/spec/models/hooks/system_hook_spec.rb
@@ -38,7 +38,7 @@ describe SystemHook, models: true do
end
it "project_destroy hook" do
- Projects::DestroyService.new(project, user, {}).pending_delete!
+ Projects::DestroyService.new(project, user, {}).async_execute
expect(WebMock).to have_requested(:post, system_hook.url).with(
body: /project_destroy/,
diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/delete_user_service_spec.rb
index a65938fa03b..630458f9efc 100644
--- a/spec/services/delete_user_service_spec.rb
+++ b/spec/services/delete_user_service_spec.rb
@@ -15,7 +15,7 @@ describe DeleteUserService, services: true do
end
it 'will delete the project in the near future' do
- expect_any_instance_of(Projects::DestroyService).to receive(:pending_delete!).once
+ expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once
DeleteUserService.new(current_user).execute(user)
end
--
GitLab