Add specs for ProjectDestroyWorker

app/services/projects/destroy_service.rb

@@ -44,7 +44,7 @@ module Projects
       system_hook_service.execute_hooks_for(project, :destroy)
-      log_info("Project \"#{project.path_with_namespace}\" was removed")
+      log_info("Project \"#{project.full_path}\" was removed")
       true
     end

app/views/projects/_deletion_failed.html.haml

 - if @project.delete_error.present?
   .project-deletion-failed-message.alert.alert-warning
-    This project was scheduled for deletion, but failed with the message:
+    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 "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'

app/views/projects/_flash_messages.html.haml

+= content_for flash_message_container do
+  = render 'deletion_failed'
+  - if current_user && can?(current_user, :download_code, project)
+    = render 'shared/no_ssh'
+    = render 'shared/no_password'

app/views/projects/empty.html.haml

 - @no_container = true
 - flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
-= content_for flash_message_container do
-  - if current_user && can?(current_user, :download_code, @project)
-    = render 'shared/no_ssh'
-    = render 'shared/no_password'
+= render partial: 'flash_messages', locals: { project: @project }
 = render "projects/head"
 = render "home_panel"

app/views/projects/show.html.haml

@@ -6,11 +6,7 @@
 = content_for :meta_tags do
   = auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity")
-= content_for flash_message_container do
-  = render 'deletion_failed'
-  - if current_user && can?(current_user, :download_code, @project)
-    = render 'shared/no_ssh'
-    = render 'shared/no_password'
+= render partial: 'flash_messages', locals: { project: @project }
 = render "projects/head"
 = render "projects/last_push"

app/workers/project_destroy_worker.rb

@@ -3,17 +3,14 @@ class ProjectDestroyWorker
   include DedicatedSidekiqQueue
   def perform(project_id, user_id, params)
-    begin
-      project = Project.unscoped.find(project_id)
-    rescue ActiveRecord::RecordNotFound
-      return
-    end
-
+    project = Project.find(project_id)
     user = User.find(user_id)
     ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
-  rescue StandardError => error
-    project.assign_attributes(delete_error: error.message, pending_delete: false)
-    project.save!(validate: false)
+  rescue Exception => error # rubocop:disable Lint/RescueException
+    project&.update_attributes(delete_error: error.message, pending_delete: false)
+    Rails.logger.error("Deletion failed on #{project&.full_path} with the following message: #{error.message}")
+
+    raise
   end
 end

db/migrate/20170428064307_add_column_delete_error_to_projects.rb

-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
 class AddColumnDeleteErrorToProjects < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
-  # When a migration requires downtime you **must** uncomment the following
-  # constant and define a short and easy to understand explanation as to why the
-  # migration requires downtime.
-  # DOWNTIME_REASON = ''
-
-  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
-  # "add_column_with_default" you must disable the use of transactions
-  # as these methods can not run in an existing transaction.
-  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
-  # that either of them is the _only_ method called in the migration,
-  # any other changes should go in a separate migration.
-  # This ensures that upon failure _only_ the index creation or removing fails
-  # and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
-
   def change
     add_column :projects, :delete_error, :text
   end

spec/features/projects/show_project_spec.rb

+require 'spec_helper'
+
+describe 'Project show page', feature: true do
+  context 'when project pending delete' do
+    let(:project) { create(:project, :empty_repo, pending_delete: true) }
+    let(:worker) { ProjectDestroyWorker.new }
+
+    before do
+      sign_in(project.owner)
+    end
+
+    it 'shows flash error if deletion for project fails' do
+      error_message = "some error message"
+      project.update_attributes(delete_error: error_message, pending_delete: false)
+
+      visit namespace_project_path(project.namespace, 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

spec/workers/project_destroy_worker_spec.rb

 require 'spec_helper'
 describe ProjectDestroyWorker do
-  let(:project) { create(:project, :repository) }
+  let(:project) { create(:project, :repository, pending_delete: true) }
   let(:path) { project.repository.path_to_repo }
   subject { described_class.new }
-  describe "#perform" do
-    it "deletes the project" do
+  describe '#perform' do
+    it 'deletes the project' do
       subject.perform(project.id, project.owner.id, {})
       expect(Project.all).not_to include(project)
       expect(Dir.exist?(path)).to be_falsey
     end
-    it "deletes the project but skips repo deletion" do
+    it 'deletes the project but skips repo deletion' do
       subject.perform(project.id, project.owner.id, { "skip_repo" => true })
       expect(Project.all).not_to include(project)
       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)
+
+        expect(Project.all).to include(project)
+        expect(project.delete_error).to eq("some error message")
+      end
+    end
   end
 end