diff --git a/app/models/foi_attachment.rb b/app/models/foi_attachment.rb index e55db31f653..aa8edfaa7ee 100644 --- a/app/models/foi_attachment.rb +++ b/app/models/foi_attachment.rb @@ -49,41 +49,9 @@ class FoiAttachment < ApplicationRecord BODY_MAX_TRIES = 3 BODY_MAX_DELAY = 5 - def directory - if file.attached? - warn <<~DEPRECATION.squish - [DEPRECATION] FoiAttachment#directory shouldn't be used when using - `ActiveStorage` backed file stores. This method will be removed - in 0.42. - DEPRECATION - return - end - - base_dir = File.expand_path(File.join(File.dirname(__FILE__), "../../cache", "attachments_#{Rails.env}")) - return File.join(base_dir, self.hexdigest[0..2]) - end - - def filepath - if file.attached? - warn <<~DEPRECATION.squish - [DEPRECATION] FoiAttachment#filepath shouldn't be used when using - `ActiveStorage` backed file stores. This method will be removed - in 0.42. - DEPRECATION - return - end - - File.join(self.directory, self.hexdigest) - end - def delete_cached_file! @cached_body = nil - - if file.attached? - file.purge - elsif File.exist?(filepath) - File.delete(filepath) - end + file.purge if file.attached? end def body=(d) @@ -101,33 +69,21 @@ def body=(d) end # raw body, encoded as binary - def body - if @cached_body.nil? - tries = 0 - delay = 1 - begin - if file.attached? - @cached_body = file.download - else - @cached_body = File.open(filepath, "rb" ) { |file| file.read } - end - rescue Errno::ENOENT, ActiveStorage::FileNotFoundError - # we've lost our cached attachments for some reason. Reparse them. - if tries > BODY_MAX_TRIES - raise - else - sleep delay - end - tries += 1 - delay *= 2 - delay = BODY_MAX_DELAY if delay > BODY_MAX_DELAY - force = true - self.incoming_message.parse_raw_email!(force) - reload - retry - end + def body(tries: 0, delay: 1) + return @cached_body if @cached_body + + if file.attached? + @cached_body = file.download + else + # we've lost our cached attachments for some reason. Reparse them. + raise if tries > BODY_MAX_TRIES + sleep [delay, BODY_MAX_DELAY].min + + self.incoming_message.parse_raw_email!(true) + reload + + body(tries: tries + 1, delay: delay * 2) end - return @cached_body end # body as UTF-8 text, with scrubbing of invalid chars if needed diff --git a/app/models/raw_email.rb b/app/models/raw_email.rb index 481890d4061..b1818040247 100644 --- a/app/models/raw_email.rb +++ b/app/models/raw_email.rb @@ -69,45 +69,6 @@ def empty_from_field? mail.from_addrs.nil? || mail.from_addrs.size == 0 end - def directory - if file.attached? - warn <<~DEPRECATION.squish - [DEPRECATION] RawEmail#directory shouldn't be used when using - `ActiveStorage` backed file stores. This method will be removed - in 0.42. - DEPRECATION - return - end - - if request_id.empty? - raise "Failed to find the id number of the associated request: has it been saved?" - end - - if Rails.env.test? - File.join(Rails.root, 'files/raw_email_test') - else - File.join(AlaveteliConfiguration::raw_emails_location, - request_id[0..2], request_id) - end - end - - def filepath - if file.attached? - warn <<~DEPRECATION.squish - [DEPRECATION] RawEmail#filepath shouldn't be used when using - `ActiveStorage` backed file stores. This method will be removed - in 0.42. - DEPRECATION - return - end - - if incoming_message_id.empty? - raise "Failed to find the id number of the associated incoming message: has it been saved?" - end - - File.join(directory, incoming_message_id) - end - def mail @mail ||= mail! end @@ -126,9 +87,7 @@ def data=(d) end def data - return @data ||= file.download if file.attached? - - File.open(filepath, "rb").read + @data ||= file.download if file.attached? end def data_as_text @@ -172,10 +131,6 @@ def incoming_message_id end def destroy_file_representation! - if file.attached? - file.purge - elsif File.exist?(filepath) - File.delete(filepath) - end + file.purge if file.attached? end end diff --git a/lib/configuration.rb b/lib/configuration.rb index 120e389b248..fa2c40bb684 100644 --- a/lib/configuration.rb +++ b/lib/configuration.rb @@ -134,16 +134,6 @@ module AlaveteliConfiguration # rubocop:enable Layout/LineLength end - def self.raw_emails_location - warn <<~DEPRECATION.squish - [DEPRECATION] AlaveteliConfiguration.raw_emails_location will be removed - in 0.42. You should have `ActiveStorage` configured but you still haven't - migrated all your old files. Please run `bin/rails storage:migrate` to - migrate - DEPRECATION - super - end - def self.method_missing(name) key = name.to_s.upcase if DEFAULTS.has_key?(key.to_sym) diff --git a/spec/models/raw_email_spec.rb b/spec/models/raw_email_spec.rb index 430a0b97d2e..1bfad2c6ac4 100644 --- a/spec/models/raw_email_spec.rb +++ b/spec/models/raw_email_spec.rb @@ -12,26 +12,6 @@ RSpec.describe RawEmail do - # DEPRECATION: remove for release 0.42 - class LegacyRawEmail < RawEmail - # This replicates how we used to store RawEmail before switching to - # ActiveStorage - def data=(d) - FileUtils.mkdir_p(directory) unless File.exist?(directory) - File.atomic_write(filepath) do |file| - file.binmode - file.write(d) - end - end - end - - let(:legacy_raw_email) do - LegacyRawEmail.create!( - incoming_message: FactoryBot.create(:incoming_message), - data: 'Hello world' - ) - end - def roundtrip_data(raw_email, data) raw_email.data = data raw_email.save! @@ -40,28 +20,13 @@ def roundtrip_data(raw_email, data) end describe 'before destroy callbacks' do - shared_examples :destory_file_examples do - it 'should delete the directory' do - raw_email.run_callbacks(:destroy) - expect(File.exist?(raw_email.filepath)).to eq(false) - end - - it 'should only delete the directory if it exists' do - expect(File).to receive(:delete).once.and_call_original - raw_email.run_callbacks(:destroy) - expect { raw_email.run_callbacks(:destroy) }. - not_to raise_error - end - end - - context 'with active storage' do - let(:raw_email) { FactoryBot.create(:incoming_message).raw_email } - include_examples :destory_file_examples - end + let(:raw_email) { FactoryBot.create(:incoming_message).raw_email } - context 'without active storage' do - let(:raw_email) { legacy_raw_email } - include_examples :destory_file_examples + it 'should only delete the directory if it exists' do + expect(File).to receive(:delete).once.and_call_original + raw_email.run_callbacks(:destroy) + expect { raw_email.run_callbacks(:destroy) }. + not_to raise_error end end @@ -219,33 +184,21 @@ def test_real(fixture_file, expected) end describe '#data' do + let(:raw_email) { FactoryBot.create(:incoming_message).raw_email } - shared_examples :data_unchanged_examples do - it 'roundtrips data unchanged' do - data = roundtrip_data(raw_email, "Hello, world!") - expect(data).to eq("Hello, world!") - end - - it 'returns an unchanged binary string with a valid encoding if the data is non-ascii and non-utf-8' do - data = roundtrip_data(raw_email, "\xA0") - - expect(data.encoding.to_s).to eq('ASCII-8BIT') - expect(data.valid_encoding?).to be true - data = data.force_encoding('UTF-8') - expect(data).to eq("\xA0") - end + it 'roundtrips data unchanged' do + data = roundtrip_data(raw_email, "Hello, world!") + expect(data).to eq("Hello, world!") end - context 'with active storage' do - let(:raw_email) { FactoryBot.create(:incoming_message).raw_email } - include_examples :data_unchanged_examples - end + it 'returns an unchanged binary string with a valid encoding if the data is non-ascii and non-utf-8' do + data = roundtrip_data(raw_email, "\xA0") - context 'without active storage' do - let(:raw_email) { legacy_raw_email } - include_examples :data_unchanged_examples + expect(data.encoding.to_s).to eq('ASCII-8BIT') + expect(data.valid_encoding?).to be true + data = data.force_encoding('UTF-8') + expect(data).to eq("\xA0") end - end describe '#data_as_text' do