port tootsuite/#12125 to monsterfork: Fix attachment not being re-downloaded even if file is not stored

Change the behaviour of remotable concern. Previously, it would skip
downloading an attachment if the stored remote URL is identical to
the new one. Now it would not be skipped if the attachment is not
actually currently stored by Paperclip.

4 files changed, 18 insertions, 10 deletions

diff --git a/app/models/account.rb b/app/models/account.rb
index 20b93ddc5..28ba35148 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -453,10 +453,9 @@ class Account < ApplicationRecord
   def save_with_optional_media!
     save!
   rescue ActiveRecord::RecordInvalid
-    self.avatar              = nil
-    self.header              = nil
-    self[:avatar_remote_url] = ''
-    self[:header_remote_url] = ''
+    self.avatar = nil
+    self.header = nil
+
     save!
   end
 
diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb
index 9ebb41ec2..5eec70d62 100644
--- a/config/initializers/paperclip.rb
+++ b/config/initializers/paperclip.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-Paperclip::DataUriAdapter.register
-
 Paperclip.interpolates :filename do |attachment, style|
   if style == :original
     attachment.original_filename
@@ -91,7 +89,7 @@ else
   Paperclip::Attachment.default_options.merge!(
     storage: :filesystem,
     use_timestamp: true,
-    path: File.join(ENV.fetch('PAPERCLIP_ROOT_PATH', File.join(':rails_root', 'public', 'system')), ':class', ':attachment', ':id_partition', ':style', ':filename'),
+    path: ENV.fetch('PAPERCLIP_ROOT_PATH', ':rails_root/public/system') + '/:class/:attachment/:id_partition/:style/:filename',
     url: ENV.fetch('PAPERCLIP_ROOT_URL', '/system') + '/:class/:attachment/:id_partition/:style/:filename',
   )
 end
diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index 917411ab5..f46ce6a52 100644
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@@ -126,8 +126,8 @@ RSpec.describe Account, type: :model do
       end
 
       it 'sets default avatar, header, avatar_remote_url, and header_remote_url' do
-        expect(account.avatar_remote_url).to eq ''
-        expect(account.header_remote_url).to eq ''
+        expect(account.avatar_remote_url).to eq 'https://remote.test/invalid_avatar'
+        expect(account.header_remote_url).to eq expectation.header_remote_url
         expect(account.avatar_file_name).to  eq nil
         expect(account.header_file_name).to  eq nil
       end
diff --git a/spec/models/concerns/remotable_spec.rb b/spec/models/concerns/remotable_spec.rb
index a4289cc45..99a60cbf6 100644
--- a/spec/models/concerns/remotable_spec.rb
+++ b/spec/models/concerns/remotable_spec.rb
@@ -18,6 +18,8 @@ RSpec.describe Remotable do
 
     def hoge=(arg); end
 
+    def hoge_file_name; end
+
     def hoge_file_name=(arg); end
 
     def has_attribute?(arg); end
@@ -109,12 +111,21 @@ RSpec.describe Remotable do
       end
 
       context 'foo[attribute_name] == url' do
-        it 'makes no request' do
+        it 'makes no request if file is saved' do
           allow(foo).to receive(:[]).with(attribute_name).and_return(url)
+          allow(foo).to receive(:hoge_file_name).and_return('foo.jpg')
 
           foo.hoge_remote_url = url
           expect(request).not_to have_been_requested
         end
+
+        it 'makes request if file is not saved' do
+          allow(foo).to receive(:[]).with(attribute_name).and_return(url)
+          allow(foo).to receive(:hoge_file_name).and_return(nil)
+
+          foo.hoge_remote_url = url
+          expect(request).to have_been_requested
+        end
       end
 
       context "scheme is https, parsed_url.host isn't empty, and foo[attribute_name] != url" do