Commit 4ccba6bf authored by Robert Speicher's avatar Robert Speicher

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
parents ae63f152 4955a47c
...@@ -125,7 +125,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -125,7 +125,7 @@ class ProjectsController < Projects::ApplicationController
def destroy def destroy
return access_denied! unless can?(current_user, :remove_project, @project) 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." flash[:alert] = "Project '#{@project.name}' will be deleted."
redirect_to dashboard_projects_path redirect_to dashboard_projects_path
......
...@@ -1161,16 +1161,6 @@ class Project < ActiveRecord::Base ...@@ -1161,16 +1161,6 @@ class Project < ActiveRecord::Base
@wiki ||= ProjectWiki.new(self, self.owner) @wiki ||= ProjectWiki.new(self, self.owner)
end 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) def running_or_pending_build_count(force: false)
Rails.cache.fetch(['projects', id, 'running_or_pending_build_count'], force: force) do Rails.cache.fetch(['projects', id, 'running_or_pending_build_count'], force: force) do
builds.running_or_pending.count(:all) builds.running_or_pending.count(:all)
......
...@@ -18,7 +18,7 @@ class DeleteUserService ...@@ -18,7 +18,7 @@ class DeleteUserService
user.personal_projects.each do |project| user.personal_projects.each do |project|
# Skip repository removal because we remove directory with namespace # Skip repository removal because we remove directory with namespace
# that contain all this repositories # 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 end
user.destroy user.destroy
......
...@@ -9,7 +9,7 @@ class DestroyGroupService ...@@ -9,7 +9,7 @@ class DestroyGroupService
group.projects.each do |project| group.projects.each do |project|
# Skip repository removal because we remove directory with namespace # Skip repository removal because we remove directory with namespace
# that contain all this repositories # 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 end
group.destroy group.destroy
......
...@@ -6,8 +6,12 @@ module Projects ...@@ -6,8 +6,12 @@ module Projects
DELETED_FLAG = '+deleted' DELETED_FLAG = '+deleted'
def pending_delete! def async_execute
project.schedule_delete!(current_user.id, params) 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 end
def execute def execute
......
...@@ -323,7 +323,7 @@ module API ...@@ -323,7 +323,7 @@ module API
# DELETE /projects/:id # DELETE /projects/:id
delete ":id" do delete ":id" do
authorize! :remove_project, user_project authorize! :remove_project, user_project
::Projects::DestroyService.new(user_project, current_user, {}).pending_delete! ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
end end
# Mark this project as forked from another # Mark this project as forked from another
......
...@@ -38,7 +38,7 @@ describe SystemHook, models: true do ...@@ -38,7 +38,7 @@ describe SystemHook, models: true do
end end
it "project_destroy hook" do 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( expect(WebMock).to have_requested(:post, system_hook.url).with(
body: /project_destroy/, body: /project_destroy/,
......
...@@ -15,7 +15,7 @@ describe DeleteUserService, services: true do ...@@ -15,7 +15,7 @@ describe DeleteUserService, services: true do
end end
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(:pending_delete!).once expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once
DeleteUserService.new(current_user).execute(user) DeleteUserService.new(current_user).execute(user)
end end
......
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