From 3491b19a4e67a9f439c12afac45ef38f3fce0ef5 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 29 Jun 2017 12:43:01 +0100 Subject: Add specs for ProjectDestroyWorker --- app/services/projects/destroy_service.rb | 2 +- app/views/projects/_deletion_failed.html.haml | 4 +-- app/views/projects/_flash_messages.html.haml | 5 ++++ app/views/projects/empty.html.haml | 5 +--- app/views/projects/show.html.haml | 6 +---- app/workers/project_destroy_worker.rb | 15 +++++------ ...28064307_add_column_delete_error_to_projects.rb | 24 ----------------- spec/features/projects/show_project_spec.rb | 30 ++++++++++++++++++++++ spec/workers/project_destroy_worker_spec.rb | 21 ++++++++++++--- 9 files changed, 63 insertions(+), 49 deletions(-) create mode 100644 app/views/projects/_flash_messages.html.haml create mode 100644 spec/features/projects/show_project_spec.rb diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 682407ac896..7b0a08af290 100644 --- a/app/services/projects/destroy_service.rb +++ b/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 diff --git a/app/views/projects/_deletion_failed.html.haml b/app/views/projects/_deletion_failed.html.haml index cd717760432..028510b5671 100644 --- a/app/views/projects/_deletion_failed.html.haml +++ b/app/views/projects/_deletion_failed.html.haml @@ -1,9 +1,9 @@ - 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' diff --git a/app/views/projects/_flash_messages.html.haml b/app/views/projects/_flash_messages.html.haml new file mode 100644 index 00000000000..6c9d466c761 --- /dev/null +++ b/app/views/projects/_flash_messages.html.haml @@ -0,0 +1,5 @@ += 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' diff --git a/app/views/projects/empty.html.haml b/app/views/projects/empty.html.haml index 0f132a68ce1..3d7c72ae61a 100644 --- a/app/views/projects/empty.html.haml +++ b/app/views/projects/empty.html.haml @@ -1,10 +1,7 @@ - @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" diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 336bc694ffc..3926149e790 100644 --- a/app/views/projects/show.html.haml +++ b/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" diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb index 482e1e38cd1..209cf11e893 100644 --- a/app/workers/project_destroy_worker.rb +++ b/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 diff --git a/db/migrate/20170428064307_add_column_delete_error_to_projects.rb b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb index ef5fc2cdea5..09f9d9b5b7a 100644 --- a/db/migrate/20170428064307_add_column_delete_error_to_projects.rb +++ b/db/migrate/20170428064307_add_column_delete_error_to_projects.rb @@ -1,30 +1,6 @@ -# 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 diff --git a/spec/features/projects/show_project_spec.rb b/spec/features/projects/show_project_spec.rb new file mode 100644 index 00000000000..5aa0d8f0026 --- /dev/null +++ b/spec/features/projects/show_project_spec.rb @@ -0,0 +1,30 @@ +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 diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb index 3d135f40c1f..29f0295de42 100644 --- a/spec/workers/project_destroy_worker_spec.rb +++ b/spec/workers/project_destroy_worker_spec.rb @@ -1,24 +1,37 @@ 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 -- cgit v1.2.1