Merge pull request #16389 from opf/implementation/57038-destroying-a-…

…storage-should-trigger-the-storagesprojectstoragesdeleteservice

[#57038] Trigger all necessary hooks when a project is removed from a storage

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
 

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

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

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