Merge pull request #19406 from opf/fix-folder-create

Show error message when creating folder fails

app/services/service_result.rb

@@ -143,6 +143,8 @@ class ServiceResult
     merge_success!(other) unless without_success
     merge_errors!(other)
     merge_dependent!(other)
+
+    self
   end
 
   ##

modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/file_info_query.rb

@@ -58,12 +58,12 @@ module Storages
           private
 
           def validate_input(file_id)
-            if file_id.nil?
+            if file_id.blank?
               ServiceResult.failure(
                 result: :error,
                 errors: StorageError.new(code: :error,
                                          data: StorageErrorData.new(source: self.class),
-                                         log_message: "File ID can not be nil")
+                                         log_message: "File ID can not be blank")
               )
             else
               ServiceResult.success

modules/storages/app/controllers/storages/project_storages_controller.rb

@@ -130,7 +130,7 @@ class Storages::ProjectStoragesController < ApplicationController
   end
 
   def show_error(message)
-    flash[:error] = message
+    flash[:error] = Array(message) + [I18n.t("project_storages.open.contact_admin")]
     redirect_back(fallback_location: project_path(id: @project_storage.project_id))
   end
 end

modules/storages/app/services/storages/base_service.rb

@@ -67,6 +67,7 @@ module Storages
         @result.errors.add(attribute, storage_error.code, **options)
       end
 
+      @result.success = false
       @result
     end
   end

modules/storages/app/services/storages/managed_folder_sync_service.rb

@@ -46,7 +46,7 @@ module Storages
     def call
       with_tagged_logger([self.class.name, "storage-#{@storage.id}"]) do
         info "Starting AMPF Sync for Storage #{@storage.id}"
-        prepare_remote_folders.on_failure { return epilogue }
+        prepare_remote_folders
         apply_permissions_to_folders
         epilogue
       end

modules/storages/app/services/storages/nextcloud_managed_folder_create_service.rb

@@ -111,7 +111,7 @@ module Storages
       info "Ensuring that automatically managed project folders exist and are correctly named."
       id_folder_map = remote_folders.to_h { |path, file| [file.id, path] }
 
-      @project_storages.includes(:project).map do |project_storage|
+      @project_storages.includes(:project).find_each do |project_storage|
         unless id_folder_map.key?(project_storage.project_folder_id)
           info "#{project_storage.managed_project_folder_path} does not exist. Creating..."
           next create_remote_folder(project_storage)
@@ -138,7 +138,7 @@ module Storages
         log_storage_error(service_result.errors, folder_id: file_id, folder_name: name)
 
         add_error(:rename_project_folder, service_result.errors,
-                  options: { current_path:, project_folder_name: name, project_folder_id: file_id }).fail!
+                  options: { current_path:, project_folder_name: name, project_folder_id: file_id })
       end
     end
 
@@ -153,14 +153,14 @@ module Storages
         return add_error(:create_folder, service_result.errors, options: { folder_name:, parent_location: })
       end.result
 
+      audit_last_project_folder(project_storage, created_folder.id)
+    end
+
+    def audit_last_project_folder(project_storage, project_folder_id)
       last_project_folder = LastProjectFolder.find_or_initialize_by(
         project_storage_id: project_storage.id, mode: project_storage.project_folder_mode
       )
 
-      audit_last_project_folder(last_project_folder, created_folder.id)
-    end
-
-    def audit_last_project_folder(last_project_folder, project_folder_id)
       ApplicationRecord.transaction do
         success = last_project_folder.update(origin_folder_id: project_folder_id) &&
           last_project_folder.project_storage.update(project_folder_id:)
@@ -188,7 +188,7 @@ module Storages
 
       set_permissions.call(storage: @storage, auth_strategy:, input_data:).on_failure do |service_result|
         log_storage_error(service_result.errors, folder: "root", root_folder_id:)
-        add_error(:ensure_root_folder_permissions, service_result.errors, options: { group:, username: }).fail!
+        add_error(:ensure_root_folder_permissions, service_result.errors, options: { group:, username: })
       end
     end
 
@@ -202,7 +202,7 @@ module Storages
                                depth: 1)
                          .on_failure do |service_result|
         log_storage_error(service_result.errors, { folder: group_folder })
-        add_error(:remote_folders, service_result.errors, options: { group_folder:, username: @storage.username }).fail!
+        add_error(:remote_folders, service_result.errors, options: { group_folder:, username: @storage.username })
       end
     end
 

modules/storages/app/services/storages/nextcloud_managed_folder_permissions_service.rb

@@ -83,7 +83,7 @@ module Storages
     def add_remove_users_to_group(group, username)
       remote_users = remote_group_users.result_or do |error|
         log_storage_error(error, group:)
-        return add_error(:remote_group_users, error, options: { group: }).fail!
+        return add_error(:remote_group_users, error, options: { group: })
       end
 
       local_users = remote_identities.order(:id).pluck(:origin_user_id)

modules/storages/app/services/storages/one_drive_managed_folder_create_service.rb

@@ -125,12 +125,11 @@ module Storages
         return add_error(:create_folder, service_result.errors, options: { folder_name:, parent_location: root_folder })
       end.result
 
-      last_project_folder = ::Storages::LastProjectFolder.find_by(project_storage_id:, mode: :automatic)
-
-      audit_last_project_folder(last_project_folder, folder_info.id)
+      audit_last_project_folder(project_storage_id, folder_info.id)
     end
 
-    def audit_last_project_folder(last_project_folder, project_folder_id)
+    def audit_last_project_folder(project_storage_id, project_folder_id)
+      last_project_folder = ::Storages::LastProjectFolder.find_by(project_storage_id:, mode: :automatic)
       ApplicationRecord.transaction do
         success =
           last_project_folder.update(origin_folder_id: project_folder_id) &&
@@ -145,7 +144,7 @@ module Storages
 
       file_list = files.call(storage: @storage, auth_strategy:, folder: root_folder).on_failure do |failed|
         log_storage_error(failed.errors, { drive_id: })
-        return add_error(:remote_folders, failed.errors, options: { drive_id: }).fail!
+        return add_error(:remote_folders, failed.errors, options: { drive_id: })
       end.result
 
       ServiceResult.success(result: filter_folders_from(file_list.files))

modules/storages/app/services/storages/project_storages/copy_project_folders_service.rb

@@ -80,7 +80,7 @@ module Storages
                 source_path:,
                 destination_path:).on_failure do |failed|
           log_storage_error(failed.errors)
-          add_error(:base, failed.errors, options: { destination_path:, source_path: }).fail!
+          add_error(:base, failed.errors, options: { destination_path:, source_path: })
         end
       end
 

modules/storages/app/services/storages/upload_link_service.rb

@@ -61,7 +61,6 @@ module Storages
                            .on_failure do |error|
         add_error(:base, error.errors, options: { storage_name: @storage.name, folder: upload_data.folder_id })
         log_storage_error(error.errors)
-        @result.success = false
       end
     end
 

modules/storages/config/locales/en.yml

@@ -82,6 +82,7 @@ en:
     edit_project_folder:
       label: Edit project folder
     open:
+      contact_admin: Please contact your administrator to resolve this error.
       remote_identity_error: An unexpected error occurred while connecting to the storage.
     project_folder_mode:
       automatic: Automatically managed
@@ -129,7 +130,7 @@ en:
               conflict: 'The user %{user} could not be added to the %{group} group for the following reason: %{reason}'
               failed_to_add: 'The user %{user} could not be added to the %{group} group for the following reason: %{reason}'
             create_folder:
-              conflict: The %{folder_name} already exists on %{parent_location}.
+              conflict: The folder %{folder_name} already exists on %{parent_location}.
               not_found: "%{parent_location} wasn't found."
             ensure_root_folder_permissions:
               not_found: "%{group_folder} wasn't found. Please check your Nextcloud Group Folder setup."

modules/storages/spec/requests/project_storages_open_spec.rb

@@ -117,7 +117,10 @@ RSpec.describe "projects/:project_id/project_storages/:id/open" do
       expect(last_response.headers["Location"]).to eq("http://test.host/projects/#{project.id}")
 
       flash = Sessions::UserSession.last.data.dig("flash", "flashes")
-      expect(flash["error"]).to eq("400 | Request made outside opening hours.")
+      expect(flash["error"]).to eq([
+                                     "400 | Request made outside opening hours.",
+                                     "Please contact your administrator to resolve this error."
+                                   ])
     end
   end
 
@@ -216,7 +219,7 @@ RSpec.describe "projects/:project_id/project_storages/:id/open" do
         expect(last_response.headers["Location"]).to eq("http://test.host/projects/#{project.id}")
 
         flash = Sessions::UserSession.last.data.dig("flash", "flashes")
-        expect(flash["error"]).to eq(["Nope, sorry!"])
+        expect(flash["error"]).to eq(["Nope, sorry!", "Please contact your administrator to resolve this error."])
       end
     end
 

modules/storages/spec/services/storages/managed_folder_sync_service_spec.rb

@@ -97,9 +97,9 @@ RSpec.describe Storages::ManagedFolderSyncService do
 
     it { is_expected.to be_failure }
 
-    it "does not call the folder permissions service" do
+    it "calls the folder permissions service anyways" do
       call
-      expect(folder_permissions_service).not_to have_received(:call).with(storage:)
+      expect(folder_permissions_service).to have_received(:call).with(storage:)
     end
   end
 

modules/storages/spec/services/storages/nextcloud_managed_folder_create_service_spec.rb

@@ -348,9 +348,8 @@ module Storages
                                             folder_name: project_storage.managed_project_folder_path, data: "error body")
           end
 
-          it "is a success" do
-            # TODO: why is this a success? Bug or intention?
-            expect(service.call).to be_success
+          it "is a failure" do
+            expect(service.call).to be_failure
           end
 
           it "adds to the services errors" do

modules/storages/spec/services/storages/nextcloud_managed_folder_permissions_service_spec.rb

@@ -382,7 +382,7 @@ RSpec.describe Storages::NextcloudManagedFolderPermissionsService, :webmock do
     context "when adding users to remote group fails" do
       let(:add_user_to_group_service) { double(call: ServiceResult.failure(errors: Storages::StorageError.new(code: 418))) } # rubocop:disable RSpec/VerifiedDoubles
 
-      it { is_expected.to be_success }
+      it { is_expected.to be_failure }
 
       it "has errors" do
         expect(service_call.errors).to be_present
@@ -405,7 +405,7 @@ RSpec.describe Storages::NextcloudManagedFolderPermissionsService, :webmock do
     context "when removing users from remote group fails" do
       let(:remove_user_from_group_service) { double(call: ServiceResult.failure(errors: Storages::StorageError.new(code: 418))) } # rubocop:disable RSpec/VerifiedDoubles
 
-      it { is_expected.to be_success }
+      it { is_expected.to be_failure }
 
       it "has errors" do
         expect(service_call.errors).to be_present
@@ -428,7 +428,7 @@ RSpec.describe Storages::NextcloudManagedFolderPermissionsService, :webmock do
     context "when setting permissions fails" do
       let(:set_permissions_service) { double(call: ServiceResult.failure(errors: Storages::StorageError.new(code: 418))) } # rubocop:disable RSpec/VerifiedDoubles
 
-      it { is_expected.to be_success }
+      it { is_expected.to be_failure }
 
       it "has errors" do
         expect(service_call.errors).to be_present

modules/storages/spec/services/storages/one_drive_managed_folder_create_service_spec.rb

@@ -252,7 +252,7 @@ RSpec.describe Storages::OneDriveManagedFolderCreateService, :webmock do
 
           expect { result = service.call }.not_to change(project_storage, :project_folder_id)
 
-          expect(result).to be_success
+          expect(result).to be_failure
           expect(result.errors[:create_folder])
             .to match_array(I18n.t("#{error_key_prefix}.attributes.create_folder.conflict",
                                    folder_name: project_storage.managed_project_folder_path, parent_location: "/"))