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

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