Merge branch 'clean-up-project-destroy' into 'master'

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.

See merge request !5695

5 files changed, 9 insertions, 15 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