From 58b5437b66efdf5cbf4bccbb8bd3262b4e638ab7 Mon Sep 17 00:00:00 2001 From: Jens Kraemer Date: Tue, 22 Feb 2022 12:20:56 +0800 Subject: [PATCH] background job for project deletion Due to the deletion of dependent objects (issues etc), project deletion may take a long time. This patch moves the actual project deletion into an ActiveJob job. It also introduces a new project status (SCHEDULED_FOR_DELETION) that is used to effectively hide the project that is about to be deleted (and any potential descendant projects) from the system immediately. A security notification is sent out to the user that deleted the project, informing about success / failure. The projects list is extended to be able to filter for the new status, so in case of a failure, the project can still be accessed for examination. --- app/controllers/projects_controller.rb | 2 +- app/helpers/admin_helper.rb | 3 +- app/jobs/application_job.rb | 4 ++ app/jobs/destroy_project_job.rb | 77 +++++++++++++++++++++ app/models/project.rb | 7 +- app/views/admin/projects.html.erb | 2 + config/locales/en.yml | 5 ++ test/functional/projects_controller_test.rb | 20 ++++++ test/integration/api_test/projects_test.rb | 10 ++- test/unit/jobs/destroy_project_job_test.rb | 63 +++++++++++++++++ 10 files changed, 187 insertions(+), 6 deletions(-) create mode 100644 app/jobs/application_job.rb create mode 100644 app/jobs/destroy_project_job.rb create mode 100644 test/unit/jobs/destroy_project_job_test.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index c2e2c70ea..84e79c807 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -300,7 +300,7 @@ class ProjectsController < ApplicationController @project_to_destroy = @project if api_request? || params[:confirm] == @project_to_destroy.identifier - @project_to_destroy.destroy + DestroyProjectJob.schedule(@project_to_destroy) respond_to do |format| format.html do redirect_to( diff --git a/app/helpers/admin_helper.rb b/app/helpers/admin_helper.rb index 7b4ef9b1b..68cb31277 100644 --- a/app/helpers/admin_helper.rb +++ b/app/helpers/admin_helper.rb @@ -22,7 +22,8 @@ module AdminHelper options_for_select([[l(:label_all), ''], [l(:project_status_active), '1'], [l(:project_status_closed), '5'], - [l(:project_status_archived), '9']], selected.to_s) + [l(:project_status_archived), '9'], + [l(:project_status_scheduled_for_deletion), '10']], selected.to_s) end def plugin_data_for_updates(plugins) diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb new file mode 100644 index 000000000..d92ffddcb --- /dev/null +++ b/app/jobs/application_job.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class ApplicationJob < ActiveJob::Base +end diff --git a/app/jobs/destroy_project_job.rb b/app/jobs/destroy_project_job.rb new file mode 100644 index 000000000..3094bf1ad --- /dev/null +++ b/app/jobs/destroy_project_job.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +class DestroyProjectJob < ApplicationJob + include Redmine::I18n + + def self.schedule(project, user: User.current) + # make the project (and any children) disappear immediately + project.self_and_descendants.update_all status: Project::STATUS_SCHEDULED_FOR_DELETION + perform_later(project.id, user.id, user.remote_ip) + end + + def perform(project_id, user_id, remote_ip) + user_current_was = User.current + + unless @user = User.active.find_by_id(user_id) + info "User check failed: User #{user_id} triggering project destroy does not exist anymore or isn't active." + return + end + @user.remote_ip = remote_ip + User.current = @user + set_language_if_valid @user.language || Setting.default_language + + unless @project = Project.find_by_id(project_id) + info "Project check failed: Project has already been deleted." + return + end + + unless @project.deletable? + info "Project check failed: User #{user_id} lacks permissions." + return + end + + message = if @project.descendants.any? + :mail_destroy_project_with_subprojects_successful + else + :mail_destroy_project_successful + end + delete_project ? success(message) : failure + ensure + User.current = user_current_was + info "End destroy project" + end + + private + + def delete_project + info "Starting with project deletion" + return !!@project.destroy + rescue + info "Error while deleting project: #{$!}" + false + end + + def success(message) + Mailer.deliver_security_notification( + @user, @user, + message: message, + value: @project.name, + url: {controller: 'admin', action: 'projects'}, + title: :label_project_plural + ) + end + + def failure + Mailer.deliver_security_notification( + @user, @user, + message: :mail_destroy_project_failed, + value: @project.name, + url: {controller: 'admin', action: 'projects'}, + title: :label_project_plural + ) + end + + def info(msg) + Rails.logger.info("[DestroyProjectJob] --- #{msg}") + end +end diff --git a/app/models/project.rb b/app/models/project.rb index fa910fdfe..d0db00364 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -25,6 +25,7 @@ class Project < ActiveRecord::Base STATUS_ACTIVE = 1 STATUS_CLOSED = 5 STATUS_ARCHIVED = 9 + STATUS_SCHEDULED_FOR_DELETION = 10 # Maximum length for project identifiers IDENTIFIER_MAX_LENGTH = 100 @@ -182,7 +183,7 @@ class Project < ActiveRecord::Base perm = Redmine::AccessControl.permission(permission) base_statement = if perm && perm.read? - "#{Project.table_name}.status <> #{Project::STATUS_ARCHIVED}" + "#{Project.table_name}.status <> #{Project::STATUS_ARCHIVED} AND #{Project.table_name}.status <> #{Project::STATUS_SCHEDULED_FOR_DELETION}" else "#{Project.table_name}.status = #{Project::STATUS_ACTIVE}" end @@ -399,6 +400,10 @@ class Project < ActiveRecord::Base self.status == STATUS_ARCHIVED end + def scheduled_for_deletion? + self.status == STATUS_SCHEDULED_FOR_DELETION + end + # Archives the project and its descendants def archive # Check that there is no issue of a non descendant project that is assigned diff --git a/app/views/admin/projects.html.erb b/app/views/admin/projects.html.erb index aa462741d..122039f99 100644 --- a/app/views/admin/projects.html.erb +++ b/app/views/admin/projects.html.erb @@ -32,10 +32,12 @@ <%= checked_image project.is_public? %> <%= format_date(project.created_on) %> + <% unless project.scheduled_for_deletion? %> <%= link_to(l(:button_archive), archive_project_path(project, :status => params[:status]), :data => {:confirm => l(:text_are_you_sure)}, :method => :post, :class => 'icon icon-lock') unless project.archived? %> <%= link_to(l(:button_unarchive), unarchive_project_path(project, :status => params[:status]), :method => :post, :class => 'icon icon-unlock') if project.archived? %> <%= link_to(l(:button_copy), copy_project_path(project), :class => 'icon icon-copy') %> <%= link_to(l(:button_delete), project_path(project), :method => :delete, :class => 'icon icon-del') %> + <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 2378e56d5..168dcfa95 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -270,6 +270,10 @@ en: mail_body_security_notification_notify_disabled: "Email address %{value} no longer receives notifications." mail_body_settings_updated: "The following settings were changed:" mail_body_password_updated: "Your password has been changed." + mail_destroy_project_failed: Project %{value} could not be deleted. + mail_destroy_project_successful: Project %{value} was deleted successfully. + mail_destroy_project_with_subprojects_successful: Project %{value} and its subprojects were deleted successfully. + field_name: Name field_description: Description @@ -1195,6 +1199,7 @@ en: project_status_active: active project_status_closed: closed project_status_archived: archived + project_status_scheduled_for_deletion: scheduled for deletion version_status_open: open version_status_locked: locked diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 837de343a..411496f0d 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -33,6 +33,7 @@ class ProjectsControllerTest < Redmine::ControllerTest def setup @request.session[:user_id] = nil Setting.default_language = 'en' + ActiveJob::Base.queue_adapter = :inline end def test_index_by_anonymous_should_not_show_private_projects @@ -1118,6 +1119,25 @@ class ProjectsControllerTest < Redmine::ControllerTest 'eCookbook Subproject 2'].join(', ') end + def test_destroy_should_mark_project_and_subprojects_for_deletion + queue_adapter_was = ActiveJob::Base.queue_adapter + ActiveJob::Base.queue_adapter = :test + set_tmp_attachments_directory + @request.session[:user_id] = 1 # admin + + assert_no_difference 'Project.count' do + delete(:destroy, :params => {:id => 1, :confirm => 'ecookbook'}) + assert_redirected_to '/admin/projects' + end + assert p = Project.find_by_id(1) + assert_equal Project::STATUS_SCHEDULED_FOR_DELETION, p.status + p.descendants.each do |child| + assert_equal Project::STATUS_SCHEDULED_FOR_DELETION, child.status + end + ensure + ActiveJob::Base.queue_adapter = queue_adapter_was + end + def test_destroy_with_confirmation_should_destroy_the_project_and_subprojects set_tmp_attachments_directory @request.session[:user_id] = 1 # admin diff --git a/test/integration/api_test/projects_test.rb b/test/integration/api_test/projects_test.rb index 9ac0e52f6..e4047c523 100644 --- a/test/integration/api_test/projects_test.rb +++ b/test/integration/api_test/projects_test.rb @@ -20,6 +20,7 @@ require File.expand_path('../../../test_helper', __FILE__) class Redmine::ApiTest::ProjectsTest < Redmine::ApiTest::Base + include ActiveJob::TestHelper fixtures :projects, :versions, :users, :roles, :members, :member_roles, :issues, :journals, :journal_details, :trackers, :projects_trackers, :issue_statuses, :enabled_modules, :enumerations, :boards, :messages, :attachments, :custom_fields, :custom_values, :custom_fields_projects, :time_entries, :issue_categories, @@ -361,13 +362,16 @@ class Redmine::ApiTest::ProjectsTest < Redmine::ApiTest::Base assert_select 'errors error', :text => "Name cannot be blank" end - test "DELETE /projects/:id.xml should delete the project" do - assert_difference('Project.count', -1) do + test "DELETE /projects/:id.xml should schedule deletion of the project" do + assert_no_difference('Project.count') do delete '/projects/2.xml', :headers => credentials('admin') end + assert_enqueued_with(job: DestroyProjectJob, + args: ->(job_args){ job_args[0] == 2}) assert_response :no_content assert_equal '', @response.body - assert_nil Project.find_by_id(2) + assert p = Project.find_by_id(2) + assert_equal Project::STATUS_SCHEDULED_FOR_DELETION, p.status end test "PUT /projects/:id/archive.xml should archive project" do diff --git a/test/unit/jobs/destroy_project_job_test.rb b/test/unit/jobs/destroy_project_job_test.rb new file mode 100644 index 000000000..c4e165043 --- /dev/null +++ b/test/unit/jobs/destroy_project_job_test.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +# Redmine - project management software +# Copyright (C) 2006-2022 Jean-Philippe Lang +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +require File.expand_path('../../../test_helper', __FILE__) + +class DestroyProjectJobTest < ActiveJob::TestCase + fixtures :users, :projects, :email_addresses + + setup do + @project = Project.find 1 + @user = User.find_by_admin true + end + + test "schedule should mark project and children for deletion" do + assert @project.descendants.any? + DestroyProjectJob.schedule @project, user: @user + @project.reload + assert_equal Project::STATUS_SCHEDULED_FOR_DELETION, @project.status + @project.descendants.each do |child| + assert_equal Project::STATUS_SCHEDULED_FOR_DELETION, child.status + end + end + + test "schedule should enqueue job" do + DestroyProjectJob.schedule @project, user: @user + assert_enqueued_with( + job: DestroyProjectJob, + args: ->(job_args){ + job_args[0] == @project.id && + job_args[1] == @user.id + } + ) + end + + test "should destroy project and send email" do + assert_difference 'Project.count', -5 do + DestroyProjectJob.perform_now @project.id, @user.id, '127.0.0.1' + end + assert_enqueued_with( + job: ActionMailer::MailDeliveryJob, + args: ->(job_args){ + job_args[1] == 'security_notification' && + job_args[3].to_s.include?("mail_destroy_project_with_subprojects_successful") + } + ) + end +end -- 2.20.1