Fixes #5363 - Moving/Verifying stored files crashes if any file is missing.

app/assets/javascripts/app/views/settings/storage_provider.jst.eco

@@ -9,20 +9,20 @@
     <dt><%- @T('Simple Storage Service (S3)') %><code>S3</code></dt>
     <dd><%- @T('Attachments are stored in a Simple Storage Service.') %><br /><%- @T('For more details, please check out our online documentation %l.', 'https://admin-docs.zammad.org/en/latest/settings/system/storage.html') %></dd>
   </dl>
-  <p class="help-text"><%- @T('If you want to move already stored attachments from one backend to another, you need to execute the following via console.') %></p>
+  <p class="help-text"><%- @T('If you want to move already stored attachments from one backend to another, you need to execute the following rails/rake task.') %></p>
   <code>
-  rails&gt; Store::File.move(SOURCE_STORAGE, TARGET_STORAGE)
+  rake zammad:store:move_files SOURCE_STORAGE TARGET_STORAGE
   </code>
   <p class="help-text"><%- @T('Examples:') %></p>
   <p class="help-text"><%- @T('Move all from "%s" to "%s":', 'filesystem', 'database') %></p>
   </p>
   <code>
-  rails&gt; Store::File.move('File', 'DB')
+  rake zammad:store:move_files File DB
   </code>
   <p class="help-text"><%- @T('Move all from "%s" to "%s":', 'database', 'simple storage') %></p>
   </p>
   <code>
-  rails&gt; Store::File.move('DB', 'S3')
+  rake zammad:store:move_files DB S3
   </code>
   <br>
   <br>

app/models/store/file.rb

@@ -76,19 +76,26 @@ in case of fixing sha hash use:
 
 =end
 
-    def self.verify(fix_it = nil)
+    def self.verify(fix_it = false)
       success = true
       Store::File.find_each(batch_size: 10) do |item|
-        sha = checksum(item.content)
-        logger.info "CHECK: Store::File.find(#{item.id})"
-        next if sha == item.sha
-
-        success = false
-        logger.error "DIFF: sha diff of Store::File.find(#{item.id}) current:#{sha}/db:#{item.sha}/provider:#{item.provider}"
-        store = Store.find_by(store_file_id: item.id)
-        logger.error "STORE: #{store.inspect}"
-        item.update_attribute(:sha, sha) if fix_it # rubocop:disable Rails/SkipsModelValidations
+        begin
+          logger.info "CHECK: Store::File.find(#{item.id})"
+          sha = checksum(item.content)
+          next if sha == item.sha
+
+          success = false
+          logger.error "DIFF: sha diff of Store::File.find(#{item.id}) current:#{sha}/db:#{item.sha}/provider:#{item.provider}"
+          store = Store.find_by(store_file_id: item.id)
+          logger.error "STORE: #{store.inspect}"
+          item.update_attribute(:sha, sha) if fix_it # rubocop:disable Rails/SkipsModelValidations
+        rescue => e
+          success = false
+          logger.error { e.message }
+          next
+        end
       end
+
       success
     end
 
@@ -114,17 +121,25 @@ nice move to keep system responsive
       adapter_source = "Store::Provider::#{source}".constantize
       adapter_target = "Store::Provider::#{target}".constantize
 
+      succeeded = true
+
       Store::File.where(provider: source).find_each(batch_size: 10) do |item|
-        adapter_target.add(item.content, item.sha)
-        item.update_attribute(:provider, target) # rubocop:disable Rails/SkipsModelValidations
-        adapter_source.delete(item.sha)
+        begin
+          adapter_target.add(item.content, item.sha)
+          item.update_attribute(:provider, target) # rubocop:disable Rails/SkipsModelValidations
+          adapter_source.delete(item.sha)
+        rescue => e
+          succeeded = false
+          logger.error "File #{item.sha} could not be moved from #{source} to #{target}: #{e.message}"
+          next
+        end
 
         logger.info "Moved file #{item.sha} from #{source} to #{target}"
 
         sleep delay if delay
       end
 
-      true
+      succeeded
     end
 
 =begin

app/models/store/provider/s3.rb

@@ -10,7 +10,7 @@ module Store::Provider::S3
 
     def add(data, sha)
       if data.bytesize > Store::Provider::S3::Config.max_chunk_size
-        return upload(data, sha, content_type:, filename:)
+        return upload(data, sha)
       end
 
       request(:put_object, key: sha, body: data)

i18n/zammad.pot

@@ -7301,7 +7301,7 @@ msgid "If you lose your recovery codes it's possible to generate new ones. This
 msgstr ""
 
 #: app/assets/javascripts/app/views/settings/storage_provider.jst.eco:12
-msgid "If you want to move already stored attachments from one backend to another, you need to execute the following via console."
+msgid "If you want to move already stored attachments from one backend to another, you need to execute the following rails/rake task."
 msgstr ""
 
 #: app/assets/javascripts/app/views/channel/chat.jst.eco:162

lib/tasks/zammad/command.rb

@@ -28,6 +28,11 @@ module Tasks
         raise "The required method 'description' is not implemented by #{name}"
       end
 
+      # Needs to be implemented by child classes.
+      def self.task_handler
+        raise "The required method 'task_handler' is not implemented by #{name}"
+      end
+
       def self.register_rake_task
         Rake::Task.define_task task_name => :environment do
           run_task

lib/tasks/zammad/store/move_files.rake

@@ -0,0 +1,4 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require_dependency 'tasks/zammad/store/move_files.rb'
+Tasks::Zammad::Store::MoveFiles.register_rake_task

lib/tasks/zammad/store/move_files.rb

@@ -0,0 +1,50 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require_dependency 'tasks/zammad/command.rb'
+
+module Tasks
+  module Zammad
+    module Store
+      class MoveFiles < Tasks::Zammad::Command
+        ARGUMENT_COUNT = 2
+
+        def self.description
+          'Move files/attachments from one store provider to another.'
+        end
+
+        def self.handle_argv
+          _, source, target = ArgvHelper.argv
+
+          [source, target].each do |provider|
+            begin
+              "Store::Provider::#{provider}".constantize
+            rescue NameError
+              warn "Store provider '#{provider}' not found."
+              exit 1 # rubocop:disable Rails/Exit
+            end
+          end
+
+          [source, target]
+        end
+
+        def self.usage
+          "#{super} <source> <target>"
+        end
+
+        def self.task_handler
+          source, target = handle_argv
+
+          puts "Moving files from #{source} to #{target}..."
+
+          status = ::Store::File.move(source, target)
+
+          puts 'Done.'
+          return if status
+
+          warn 'One or more files could not be moved. For further information, please check the logs.'
+          exit 1 # rubocop:disable Rails/Exit
+        end
+      end
+    end
+  end
+end

lib/tasks/zammad/store/verify_files.rake

@@ -0,0 +1,4 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require_dependency 'tasks/zammad/store/verify_files.rb'
+Tasks::Zammad::Store::VerifyFiles.register_rake_task

lib/tasks/zammad/store/verify_files.rb

@@ -0,0 +1,27 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require_dependency 'tasks/zammad/command.rb'
+
+module Tasks
+  module Zammad
+    module Store
+      class VerifyFiles < Tasks::Zammad::Command
+        def self.description
+          'Verify files/attachments checksums.'
+        end
+
+        def self.task_handler
+          puts 'Verifying files checksums...'
+
+          status = ::Store::File.verify
+
+          puts 'Done.'
+          return if status
+
+          warn 'One or more files could not be verified. For further information, please check the logs.'
+          exit 1 # rubocop:disable Rails/Exit
+        end
+      end
+    end
+  end
+end

spec/lib/tasks/zammad/store/move_files_spec.rb

@@ -0,0 +1,62 @@
+# Copyright (C) 2012-2024 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Tasks::Zammad::Store::MoveFiles do
+  describe '.description' do
+    it 'returns the description' do
+      expect(described_class.description).to eq('Move files/attachments from one store provider to another.')
+    end
+  end
+
+  describe '.usage' do
+    it 'returns the usage' do
+      expect(described_class.usage).to eq('Usage: bundle exec rails zammad:store:move_files <source> <target>')
+    end
+  end
+
+  describe '.handle_argv' do
+    it 'returns the source and target' do
+      allow(ArgvHelper).to receive(:argv).and_return(%w[zammad:store:move_files File S3])
+
+      expect(described_class.handle_argv).to eq(%w[File S3])
+    end
+
+    context 'when a provider is not found' do
+      it 'warns and exits', :aggregate_failures do
+        allow(ArgvHelper).to receive(:argv).and_return(%w[zammad:store:move_files NFS S3])
+
+        expect { described_class.handle_argv }.to raise_error(SystemExit)
+          .and output("Store provider 'NFS' not found.\n").to_stderr
+      end
+    end
+  end
+
+  describe '.task_handler' do
+    let(:source) { 'File' }
+    let(:target) { 'S3' }
+
+    before do
+      allow(described_class).to receive(:handle_argv).and_return([source, target])
+      allow(Store::File).to receive(:move).with(source, target).and_return(status)
+    end
+
+    context 'when status is true' do
+      let(:status) { true }
+
+      it 'outputs the message' do
+        expect { described_class.task_handler }.to output("Moving files from #{source} to #{target}...\nDone.\n").to_stdout
+      end
+    end
+
+    context 'when status is false' do
+      let(:status) { false }
+
+      it 'warns and exits' do
+        expect { described_class.task_handler }.to output("Moving files from #{source} to #{target}...\nDone.\n").to_stdout
+          .and raise_error(SystemExit)
+          .and output("One or more files could not be moved. For further information, please check the logs.\n").to_stderr
+      end
+    end
+  end
+end