diff --git a/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb b/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb index d26ee25890f5..2bc95d577124 100644 --- a/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb +++ b/modules/storages/app/controllers/storages/admin/storages/project_storages_controller.rb @@ -109,10 +109,21 @@ def destroy_confirmation_dialog end def destroy - Storages::ProjectStorages::DeleteService + result = Storages::ProjectStorages::DeleteService .new(user: current_user, model: @project_storage) .call + # rubocop:disable Rails/ActionControllerFlashBeforeRender + result.on_success do + flash[:primer_banner] = { message: I18n.t(:notice_successful_delete) } + end + + result.on_failure do |failure| + error = failure.errors.map(&:message).to_sentence + flash[:primer_banner] = { message: t("project_storages.remove_project.deletion_failure_flash", error:), scheme: :danger } + end + # rubocop:enable Rails/ActionControllerFlashBeforeRender + redirect_to admin_settings_storage_project_storages_path(@storage) end diff --git a/modules/storages/config/locales/en.yml b/modules/storages/config/locales/en.yml index 85c7f39b788a..6436c13f9433 100644 --- a/modules/storages/config/locales/en.yml +++ b/modules/storages/config/locales/en.yml @@ -67,6 +67,7 @@ en: inactive: No specific folder manual: Existing folder manually managed remove_project: + deletion_failure_flash: Failed to remove the project from the storage. %{error} dialog: automatically_managed_appendix: Also, in this case this storage has an automatically managed project folder, this and its files will be deleted forever. confirmation_text: Please, confirm you understand and want to remove this file storage from this project diff --git a/modules/storages/spec/features/storages/admin/project_storages_spec.rb b/modules/storages/spec/features/storages/admin/project_storages_spec.rb index e1e4d8a6c838..d41c7941ad3b 100644 --- a/modules/storages/spec/features/storages/admin/project_storages_spec.rb +++ b/modules/storages/spec/features/storages/admin/project_storages_spec.rb @@ -55,10 +55,6 @@ current_user { admin } - before do - Storages::Peripherals::Registry - .stub("#{storage.short_provider_type}.commands.delete_folder", ->(_) { ServiceResult.success }) - end context "with insufficient permissions" do it "is not accessible" do @@ -320,6 +316,20 @@ end describe "Removal of a project from a storage" do + let(:success_delete_service) do + Class.new do + def initialize(user:, model:) + @user = user + @model = model + end + + def call + @model.destroy! + ServiceResult.success + end + end + end + it "shows a warning dialog that can be aborted" do expect(page).to have_text(project.name) project_storages_index_page.click_menu_item_of("Remove project", project) @@ -337,13 +347,25 @@ expect(page).to have_text(project.name) project_storages_index_page.click_menu_item_of("Remove project", project) + # The original DeleteService would try to remove actual files from actual storages, + # which is of course not possible in this test since no real storage is used. + expect(Storages::ProjectStorages::DeleteService) + .to receive(:new) # rubocop:disable RSpec/MessageSpies + .and_wrap_original do |_original_method, *args, &_block| + user, model = *args.first.values + success_delete_service.new(user:, model:) + end + page.within("dialog") do expect(page).to have_button("Remove", disabled: true) - check "Please, confirm you understand and want to remove this file storage from this project" - expect(page).to have_button("Remove", disabled: false, wait: 3) # ensure button is clickable - click_on "Remove" + Retryable.repeat_until_success do + check "Please, confirm you understand and want to remove this file storage from this project" + expect(page).to have_button("Remove", disabled: false) # ensure button is clickable + click_on "Remove" + end end + expect(page).to have_text("Successful deletion.") expect(page).to have_no_text(project.name) end end diff --git a/spec/support/retryable.rb b/spec/support/retryable.rb new file mode 100644 index 000000000000..8857f5af1d29 --- /dev/null +++ b/spec/support/retryable.rb @@ -0,0 +1,65 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# 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. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class Retryable + WAIT_PERIOD = 0.05 + RETRY_ERRORS = %w[ + RSpec::Expectations::ExpectationNotMetError + Capybara::ElementNotFound + ].freeze + + def self.repeat_until_success(max_seconds: RSpec.configuration.wait_timeout, &block) + repeat_started = system_time + tries = 0 + begin + tries += 1 + try_started = system_time + yield block + rescue Exception => e # rubocop:disable Lint/RescueException + raise e unless retryable_error?(e) + raise e if (try_started - repeat_started) > max_seconds && (tries >= 2) + + sleep(WAIT_PERIOD) + if system_time == repeat_started + raise "Time appears to be frozen, can't use Retryable!" + end + + retry + end + end + + # Use the system clock (i.e. seconds since boot) to calculate the time, + # because Time.now may be frozen + def self.system_time + Process.clock_gettime(Process::CLOCK_MONOTONIC) + end + + def self.retryable_error?(error) + RETRY_ERRORS.include?(error.class.name) + end +end