Fix trying to write non-existent image remote URL attribute on preview cards (#14181)

Regression from #14145

app/models/concerns/remotable.rb

@@ -7,8 +7,8 @@ module Remotable
     def remotable_attachment(attachment_name, limit, suppress_errors: true, download_on_assign: true, attribute_name: nil)
       attribute_name ||= "#{attachment_name}_remote_url".to_sym
 
-      define_method("download_#{attachment_name}!") do
-        url = self[attribute_name]
+      define_method("download_#{attachment_name}!") do |url = nil|
+        url ||= self[attribute_name]
 
         return if url.blank?
 
@@ -51,9 +51,9 @@ module Remotable
       define_method("#{attribute_name}=") do |url|
         return if self[attribute_name] == url && public_send("#{attachment_name}_file_name").present?
 
-        self[attribute_name] = url
+        self[attribute_name] = url if has_attribute?(attribute_name)
 
-        public_send("download_#{attachment_name}!") if download_on_assign
+        public_send("download_#{attachment_name}!", url) if download_on_assign
       end
 
       alias_method("reset_#{attachment_name}!", "download_#{attachment_name}!")

spec/models/concerns/remotable_spec.rb

@@ -29,170 +29,177 @@ RSpec.describe Remotable do
     end
   end
 
-  context 'Remotable module is included' do
-    before do
-      class Foo
-        include Remotable
-        remotable_attachment :hoge, 1.kilobyte
-      end
-    end
+  before do
+    class Foo
+      include Remotable
 
-    let(:attribute_name) { "#{hoge}_remote_url".to_sym }
-    let(:code)           { 200 }
-    let(:file)           { 'filename="foo.txt"' }
-    let(:foo)            { Foo.new }
-    let(:headers)        { { 'content-disposition' => file } }
-    let(:hoge)           { :hoge }
-    let(:url)            { 'https://google.com' }
-
-    let(:request) do
-      stub_request(:get, url)
-        .to_return(status: code, headers: headers)
+      remotable_attachment :hoge, 1.kilobyte
     end
+  end
 
-    it 'defines a method #hoge_remote_url=' do
-      expect(foo).to respond_to(:hoge_remote_url=)
-    end
+  let(:attribute_name) { "#{hoge}_remote_url".to_sym }
+  let(:code)           { 200 }
+  let(:file)           { 'filename="foo.txt"' }
+  let(:foo)            { Foo.new }
+  let(:headers)        { { 'content-disposition' => file } }
+  let(:hoge)           { :hoge }
+  let(:url)            { 'https://google.com' }
+
+  it 'defines a method #hoge_remote_url=' do
+    expect(foo).to respond_to(:hoge_remote_url=)
+  end
 
-    it 'defines a method #reset_hoge!' do
-      expect(foo).to respond_to(:reset_hoge!)
+  it 'defines a method #reset_hoge!' do
+    expect(foo).to respond_to(:reset_hoge!)
+  end
+
+  it 'defines a method #download_hoge!' do
+    expect(foo).to respond_to(:download_hoge!)
+  end
+
+  describe '#hoge_remote_url=' do
+    before do
+      stub_request(:get, url).to_return(status: code, headers: headers)
     end
 
-    it 'defines a method #download_hoge!' do
-      expect(foo).to respond_to(:download_hoge!)
+    it 'always returns its argument' do
+      [nil, '', [], {}].each do |arg|
+        expect(foo.hoge_remote_url = arg).to be arg
+      end
     end
 
-    describe '#hoge_remote_url=' do
+    context 'with an invalid URL' do
       before do
-        request
+        allow(Addressable::URI).to receive_message_chain(:parse, :normalize).with(url).with(no_args).and_raise(Addressable::URI::InvalidURIError)
       end
 
-      it 'always returns arg' do
-        [nil, '', [], {}].each do |arg|
-          expect(foo.hoge_remote_url = arg).to be arg
-        end
+      it 'makes no request' do
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to_not have_been_made
       end
+    end
 
-      context 'Addressable::URI::InvalidURIError raised' do
-        it 'makes no request' do
-          allow(Addressable::URI).to receive_message_chain(:parse, :normalize)
-            .with(url).with(no_args).and_raise(Addressable::URI::InvalidURIError)
+    context 'with scheme that is neither http nor https' do
+      let(:url) { 'ftp://google.com' }
 
-          foo.hoge_remote_url = url
-          expect(request).not_to have_been_requested
-        end
+      it 'makes no request' do
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to_not have_been_made
       end
+    end
 
-      context 'scheme is neither http nor https' do
-        let(:url) { 'ftp://google.com' }
+    context 'with relative URL' do
+      let(:url) { 'https:///path' }
 
-        it 'makes no request' do
-          foo.hoge_remote_url = url
-          expect(request).not_to have_been_requested
-        end
+      it 'makes no request' do
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to_not have_been_made
       end
+    end
 
-      context 'parsed_url.host is empty' do
-        it 'makes no request' do
-          parsed_url = double(scheme: 'https', host: double(blank?: true))
-          allow(Addressable::URI).to receive_message_chain(:parse, :normalize)
-            .with(url).with(no_args).and_return(parsed_url)
+    context 'when URL has not changed' do
+      it 'makes no request if file is already 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
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to_not have_been_made
       end
 
-      context 'parsed_url.host is nil' do
-        it 'makes no request' do
-          parsed_url = Addressable::URI.parse('https:https://example.com/path/file.png')
-          allow(Addressable::URI).to receive_message_chain(:parse, :normalize)
-            .with(url).with(no_args).and_return(parsed_url)
+      it 'makes request if file is not already 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).not_to have_been_requested
-        end
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to have_been_made
       end
+    end
 
-      context 'foo[attribute_name] == url' 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')
+    context 'when instance has no attribute for URL' do
+      before do
+        allow(foo).to receive(:has_attribute?).with(attribute_name).and_return(false)
+      end
 
-          foo.hoge_remote_url = url
-          expect(request).not_to have_been_requested
-        end
+      it 'does not try to write attribute' do
+        expect(foo).to_not receive('[]=').with(attribute_name, url)
+        foo.hoge_remote_url = url
+      end
+    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)
+    context 'when instance has an attribute for URL' do
+      before do
+        allow(foo).to receive(:has_attribute?).with(attribute_name).and_return(true)
+      end
 
-          foo.hoge_remote_url = url
-          expect(request).to have_been_requested
-        end
+      it 'does not try to write attribute' do
+        expect(foo).to receive('[]=').with(attribute_name, url)
+        foo.hoge_remote_url = url
+      end
+    end
+
+    context 'with a valid URL' do
+      it 'makes a request' do
+        foo.hoge_remote_url = url
+        expect(a_request(:get, url)).to have_been_made
       end
 
-      context "scheme is https, parsed_url.host isn't empty, and foo[attribute_name] != url" do
-        it 'makes a request' do
-          foo.hoge_remote_url = url
-          expect(request).to have_been_requested
-        end
+      context 'when the response is not successful' do
+        let(:code) { 500 }
 
-        context 'response.code != 200' do
-          let(:code) { 500 }
+        it 'does not assign file' do
+          expect(foo).not_to receive(:public_send).with("#{hoge}=", any_args)
+          expect(foo).not_to receive(:public_send).with("#{hoge}_file_name=", any_args)
 
-          it 'calls not send' do
-            expect(foo).not_to receive(:public_send).with("#{hoge}=", any_args)
-            expect(foo).not_to receive(:public_send).with("#{hoge}_file_name=", any_args)
-            foo.hoge_remote_url = url
-          end
+          foo.hoge_remote_url = url
         end
+      end
 
-        context 'response.code == 200' do
-          let(:code) { 200 }
+      context 'when the response is successful' do
+        let(:code) { 200 }
 
-          context 'response contains headers["content-disposition"]' do
-            let(:file)      { 'filename="foo.txt"' }
-            let(:headers)   { { 'content-disposition' => file } }
+        context 'and contains Content-Disposition header' do
+          let(:file)      { 'filename="foo.txt"' }
+          let(:headers)   { { 'content-disposition' => file } }
 
-            it 'calls send' do
-              string_io = StringIO.new('')
-              extname   = '.txt'
-              basename  = '0123456789abcdef'
+          it 'assigns file' do
+            string_io = StringIO.new('')
+            extname   = '.txt'
+            basename  = '0123456789abcdef'
 
-              allow(SecureRandom).to receive(:hex).and_return(basename)
-              allow(StringIO).to receive(:new).with(anything).and_return(string_io)
+            allow(SecureRandom).to receive(:hex).and_return(basename)
+            allow(StringIO).to receive(:new).with(anything).and_return(string_io)
 
-              expect(foo).to receive(:public_send).with("download_#{hoge}!")
+            expect(foo).to receive(:public_send).with("download_#{hoge}!", url)
 
-              foo.hoge_remote_url = url
+            foo.hoge_remote_url = url
 
-              expect(foo).to receive(:public_send).with("#{hoge}=", string_io)
-              expect(foo).to receive(:public_send).with("#{hoge}_file_name=", basename + extname)
+            expect(foo).to receive(:public_send).with("#{hoge}=", string_io)
+            expect(foo).to receive(:public_send).with("#{hoge}_file_name=", basename + extname)
 
-              foo.download_hoge!
-            end
+            foo.download_hoge!(url)
           end
         end
+      end
 
-        context 'an error raised during the request' do
-          let(:request) { stub_request(:get, url).to_raise(error_class) }
+      context 'when an error is raised during the request' do
+        before do
+          stub_request(:get, url).to_raise(error_class)
+        end
 
-          error_classes = [
-            HTTP::TimeoutError,
-            HTTP::ConnectionError,
-            OpenSSL::SSL::SSLError,
-            Paperclip::Errors::NotIdentifiedByImageMagickError,
-            Addressable::URI::InvalidURIError,
-          ]
+        error_classes = [
+          HTTP::TimeoutError,
+          HTTP::ConnectionError,
+          OpenSSL::SSL::SSLError,
+          Paperclip::Errors::NotIdentifiedByImageMagickError,
+          Addressable::URI::InvalidURIError,
+        ]
 
-          error_classes.each do |error_class|
-            let(:error_class) { error_class }
+        error_classes.each do |error_class|
+          let(:error_class) { error_class }
 
-            it 'calls Rails.logger.debug' do
-              expect(Rails.logger).to receive(:debug).with(/^Error fetching remote #{hoge}: /)
-              foo.hoge_remote_url = url
-            end
+          it 'calls Rails.logger.debug' do
+            expect(Rails.logger).to receive(:debug).with(/^Error fetching remote #{hoge}: /)
+            foo.hoge_remote_url = url
           end
         end
       end