Move exception handling to execute29289-project-destroy-clean-up-after-failure

7 files changed, 32 insertions, 40 deletions

diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 7e38aacc91a..243f51737d4 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -31,8 +31,10 @@ module Projects
 
       log_info("Project \"#{project.full_path}\" was removed")
       true
-    rescue Projects::DestroyService::DestroyError => error
-      Rails.logger.error("Deletion failed on #{project.full_path} with the following message: #{error.message}")
+    rescue Exception => error # rubocop:disable Lint/RescueException
+      project.update_attributes(delete_error: error.message, pending_delete: false)
+      log_error("Deletion failed on #{project.full_path} with the following message: #{error.message}")
+
       false
     end
 
@@ -76,9 +78,6 @@ module Projects
         project.team.truncate
         project.destroy!
       end
-    rescue Exception => error # rubocop:disable Lint/RescueException
-      project.update_attributes(delete_error: error.message, pending_delete: false)
-      raise
     end
 
     ##
diff --git a/app/views/projects/_deletion_failed.html.haml b/app/views/projects/_deletion_failed.html.haml
index 028510b5671..85e2af48e25 100644
--- a/app/views/projects/_deletion_failed.html.haml
+++ b/app/views/projects/_deletion_failed.html.haml
@@ -1,9 +1,11 @@
-- if @project.delete_error.present?
-  .project-deletion-failed-message.alert.alert-warning
-    This project was scheduled for deletion, but failed with the following message:
-    = @project.delete_error
+- project = local_assigns.fetch(:project)
+- return unless project.delete_error.present?
 
-    .alert-link-group
-      = link_to "Don't show again", profile_path(user: { hide_no_ssh_key: true }), method: :put, class: 'alert-link'
-      |
-      = link_to 'Remind later', '#', class: 'hide-no-ssh-message alert-link'
+.project-deletion-failed-message.alert.alert-warning
+  This project was scheduled for deletion, but failed with the following message:
+  = project.delete_error
+
+  .alert-link-group
+    = link_to "Don't show again", profile_path(user: { hide_no_ssh_key: true }), method: :put, class: 'alert-link'
+    |
+    = link_to 'Remind later', '#', class: 'hide-no-ssh-message alert-link'
diff --git a/app/views/projects/_flash_messages.html.haml b/app/views/projects/_flash_messages.html.haml
index 80b658e3061..0175b519867 100644
--- a/app/views/projects/_flash_messages.html.haml
+++ b/app/views/projects/_flash_messages.html.haml
@@ -1,5 +1,7 @@
+- project = local_assigns.fetch(:project)
+
 = content_for :flash_message do
-  = render 'deletion_failed'
+  = render partial: 'deletion_failed', locals: { project: project }
   - if current_user && can?(current_user, :download_code, project)
     = render 'shared/no_ssh'
     = render 'shared/no_password'
diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb
index e695ec060f0..2738ef0a8e9 100644
--- a/app/workers/project_destroy_worker.rb
+++ b/app/workers/project_destroy_worker.rb
@@ -8,6 +8,6 @@ class ProjectDestroyWorker
 
     ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
   rescue ActiveRecord::RecordNotFound => error
-    logger.error("Failed to delete project #{project.path_with_namespace} (#{project.id}): #{error.message}")
+    logger.error("Failed to delete project #{project.path_with_namespace} (#{project.id}): #{error.message}") if project
   end
 end
diff --git a/spec/features/projects/show_project_spec.rb b/spec/features/projects/show_project_spec.rb
index 5aa0d8f0026..943d9454fc9 100644
--- a/spec/features/projects/show_project_spec.rb
+++ b/spec/features/projects/show_project_spec.rb
@@ -13,18 +13,10 @@ describe 'Project show page', feature: true do
       error_message = "some error message"
       project.update_attributes(delete_error: error_message, pending_delete: false)
 
-      visit namespace_project_path(project.namespace, project)
+      visit project_path(project)
 
       expect(page).to have_selector('.project-deletion-failed-message')
       expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{error_message}")
     end
-
-    it 'renders 404 if project was successfully deleted' do
-      worker.perform(project.id, project.owner.id, {})
-
-      visit namespace_project_path(project.namespace, project)
-
-      expect(page).to have_http_status(404)
-    end
   end
 end
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 857661938b7..a7da3d61a11 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -132,8 +132,8 @@ describe Projects::DestroyService, services: true do
 
       context 'when `execute` raises any other error' do
         before do
-          expect_any_instance_of(Projects::DestroyService)
-            .to receive(:execute).and_raise(ArgumentError.new("Other error message"))
+          expect_any_instance_of(Project)
+            .to receive(:destroy!).and_raise(StandardError.new("Other error message"))
         end
 
         it_behaves_like 'handles errors thrown during async destroy', "Other error message"
@@ -182,8 +182,7 @@ describe Projects::DestroyService, services: true do
           expect_any_instance_of(ContainerRepository)
             .to receive(:delete_tags!).and_return(false)
 
-          expect{ destroy_project(project, user) }
-            .to raise_error(ActiveRecord::RecordNotDestroyed)
+          expect(destroy_project(project, user)).to be false
         end
       end
     end
@@ -208,8 +207,7 @@ describe Projects::DestroyService, services: true do
           expect_any_instance_of(ContainerRepository)
             .to receive(:delete_tags!).and_return(false)
 
-          expect { destroy_project(project, user) }
-            .to raise_error(Projects::DestroyService::DestroyError)
+          expect(destroy_project(project, user)).to be false
         end
       end
     end
diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb
index 29f0295de42..f19c9dff941 100644
--- a/spec/workers/project_destroy_worker_spec.rb
+++ b/spec/workers/project_destroy_worker_spec.rb
@@ -21,17 +21,16 @@ describe ProjectDestroyWorker do
       expect(Dir.exist?(path)).to be_truthy
     end
 
-    describe 'when StandardError is raised' do
-      it 'reverts pending_delete attribute with a error message' do
-        allow_any_instance_of(::Projects::DestroyService).to receive(:execute).and_raise(StandardError, "some error message")
-
-        expect do
-          subject.perform(project.id, project.owner.id, {})
-        end.to change { project.reload.pending_delete }.from(true).to(false)
+    it 'does not raise error when project could not be found' do
+      expect do
+        subject.perform(-1, project.owner.id, {})
+      end.not_to raise_error
+    end
 
-        expect(Project.all).to include(project)
-        expect(project.delete_error).to eq("some error message")
-      end
+    it 'does not raise error when user could not be found' do
+      expect do
+        subject.perform(project.id, -1, {})
+      end.not_to raise_error
     end
   end
 end