Merge branch '8040-fix-missing-attachments' into develop

app/controllers/attachments_controller.rb

@@ -133,7 +133,7 @@ def ensure_masked
       end
     end
 
-  rescue Timeout::Error
+  rescue Timeout::Error, ActiveRecord::RecordNotFound
     redirect_to wait_for_attachment_mask_path(
       @attachment.to_signed_global_id,
       referer: verifier.generate(request.fullpath)

app/models/foi_attachment.rb

@@ -100,20 +100,22 @@ def body=(d)
     update_display_size!
   end
 
-  # raw body, encoded as binary
   def body
     return @cached_body if @cached_body
 
-    if masked?
-      @cached_body = file.download
-    elsif persisted?
+    begin
+      return file.download if masked?
+    rescue ActiveStorage::FileNotFoundError
+      # file isn't in storage and has gone missing, rescue to allow the masking
+      # job to run and rebuild the stored file or even the whole attachment.
+    end
+
+    if persisted?
       FoiAttachmentMaskJob.perform_once_now(self)
-      reload
-      body
+      return body unless destroyed?
     end
 
-  rescue ActiveRecord::RecordNotFound
-    load_attachment_from_incoming_message!.body
+    load_attachment_from_incoming_message!.body if destroyed?
   end
 
   # body as UTF-8 text, with scrubbing of invalid chars if needed
@@ -136,10 +138,15 @@ def unmasked_body
     )
 
   rescue MailHandler::MismatchedAttachmentHexdigest
-    attributes = MailHandler.attempt_to_find_original_attachment_attributes(
-      raw_email.mail,
-      body: file.download
-    ) if file.attached?
+    begin
+      attributes = MailHandler.attempt_to_find_original_attachment_attributes(
+        raw_email.mail,
+        body: file.download
+      ) if file.attached?
+
+    rescue ActiveStorage::FileNotFoundError
+      raise MissingAttachment, "attachment missing from storage (ID=#{id})"
+    end
 
     unless attributes
       raise MissingAttachment, "attachment missing in raw email (ID=#{id})"

doc/CHANGES.md

@@ -2,6 +2,8 @@
 
 ## Highlighted Features
 
+* Update attachment processing to automatically rebuild if cached file goes
+  missing (Graeme Porteous)
 * Allow `InfoRequest` to be categorised (Graeme Porteous)
 * Replace public body categories with generalised categories (Graeme Porteous)
 * Add admin links to and from batch request show action (Graeme Porteous)

spec/controllers/attachments_controller_spec.rb

@@ -124,11 +124,7 @@ def show(params = {})
         end
       end
 
-      context 'when response times out waiting for masked attachment' do
-        before do
-          allow(Timeout).to receive(:timeout).and_raise(Timeout::Error)
-        end
-
+      shared_examples 'redirects to attachment mask route' do
         it 'queues FoiAttachmentMaskJob' do
           expect(FoiAttachmentMaskJob).to receive(:perform_later).
             with(attachment)
@@ -156,6 +152,26 @@ def show(params = {})
           )
         end
       end
+
+      context 'when response times out waiting for masked attachment' do
+        before do
+          expect(Timeout).to receive(:timeout).and_raise(Timeout::Error)
+        end
+
+        include_examples 'redirects to attachment mask route'
+      end
+
+      context 'when attachment gets rebuilt' do
+        before do
+          allow(IncomingMessage).
+            to receive(:get_attachment_by_url_part_number_and_filename!).
+            and_return(attachment)
+          allow(attachment).
+            to receive(:reload).and_raise(ActiveRecord::RecordNotFound)
+        end
+
+        include_examples 'redirects to attachment mask route'
+      end
     end
 
     context 'when request is embargoed' do

spec/models/foi_attachment_spec.rb

@@ -123,6 +123,26 @@
       end
     end
 
+    context 'when masked but stored attachment is missing' do
+      let(:foi_attachment) { FactoryBot.create(:body_text) }
+
+      before do
+        allow(foi_attachment.file).
+          to receive(:download).and_raise(ActiveStorage::FileNotFoundError)
+      end
+
+      it 'calls the FoiAttachmentMaskJob now and return the masked body' do
+        expect(FoiAttachmentMaskJob).to receive(:perform_now).
+          with(foi_attachment).
+          and_invoke(-> (_) {
+            # mock the job
+            foi_attachment.update(body: 'maskedbody', masked_at: Time.zone.now)
+          })
+
+        expect(foi_attachment.body).to eq('maskedbody')
+      end
+    end
+
     context 'when unmasked and original attachment can be found' do
       let(:incoming_message) do
         FactoryBot.create(:incoming_message, foi_attachments_factories: [
@@ -180,6 +200,21 @@
         )
       end
     end
+
+    context 'when attachment has been destroy' do
+      let(:foi_attachment) { FactoryBot.create(:foi_attachment) }
+
+      before { foi_attachment.destroy }
+
+      it 'returns load_attachment_from_incoming_message.body' do
+        allow(foi_attachment).to(
+          receive(:load_attachment_from_incoming_message).and_return(
+            double(body: 'thisisthenewtext')
+          )
+        )
+        expect(foi_attachment.body).to eq('thisisthenewtext')
+      end
+    end
   end
 
   describe '#body_as_text' do
@@ -262,6 +297,20 @@
       end
     end
 
+    context 'when unable to find original attachment in storage' do
+      before do
+        allow(foi_attachment.file).
+          to receive(:download).and_raise(ActiveStorage::FileNotFoundError)
+      end
+
+      it 'raises missing attachment exception' do
+        expect { unmasked_body }.to raise_error(
+          FoiAttachment::MissingAttachment,
+          "attachment missing from storage (ID=#{foi_attachment.id})"
+        )
+      end
+    end
+
     context 'when unable to find original attachment through other means' do
       before do
         allow(MailHandler).to receive(:attachment_body_for_hexdigest).