From 656ee8b2dd1b06541f03ea19302d3529085f5139 Mon Sep 17 00:00:00 2001
From: George Claghorn <george@basecamp.com>
Date: Tue, 29 Aug 2017 15:34:50 -0400
Subject: Fix replacing a singular attachment

---
 activestorage/lib/active_storage/attached.rb       |  6 +++---
 .../lib/active_storage/attached/macros.rb          |  4 ++--
 activestorage/lib/active_storage/attached/one.rb   | 22 +++++++++++++++++--
 activestorage/test/models/attachments_test.rb      | 25 ++++++++++++++++++++++
 4 files changed, 50 insertions(+), 7 deletions(-)

(limited to 'activestorage')

diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb
index 9c4b6d5d1d..c08fd56652 100644
--- a/activestorage/lib/active_storage/attached.rb
+++ b/activestorage/lib/active_storage/attached.rb
@@ -8,10 +8,10 @@ module ActiveStorage
   # Abstract base class for the concrete ActiveStorage::Attached::One and ActiveStorage::Attached::Many
   # classes that both provide proxy access to the blob association for a record.
   class Attached
-    attr_reader :name, :record
+    attr_reader :name, :record, :dependent
 
-    def initialize(name, record)
-      @name, @record = name, record
+    def initialize(name, record, dependent:)
+      @name, @record, @dependent = name, record, dependent
     end
 
     private
diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb
index f3879ee2e3..35a081adc4 100644
--- a/activestorage/lib/active_storage/attached/macros.rb
+++ b/activestorage/lib/active_storage/attached/macros.rb
@@ -26,7 +26,7 @@ module ActiveStorage
     def has_one_attached(name, dependent: :purge_later)
       class_eval <<-CODE, __FILE__, __LINE__ + 1
         def #{name}
-          @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self)
+          @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
         end
       CODE
 
@@ -65,7 +65,7 @@ module ActiveStorage
     def has_many_attached(name, dependent: :purge_later)
       class_eval <<-CODE, __FILE__, __LINE__ + 1
         def #{name}
-          @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self)
+          @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"})
         end
       CODE
 
diff --git a/activestorage/lib/active_storage/attached/one.rb b/activestorage/lib/active_storage/attached/one.rb
index 92a00c20a7..87c650b3d0 100644
--- a/activestorage/lib/active_storage/attached/one.rb
+++ b/activestorage/lib/active_storage/attached/one.rb
@@ -21,8 +21,9 @@ module ActiveStorage
     #   person.avatar.attach(io: File.open("~/face.jpg"), filename: "face.jpg", content_type: "image/jpg")
     #   person.avatar.attach(avatar_blob) # ActiveStorage::Blob object
     def attach(attachable)
-      write_attachment \
-        ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable))
+      purge_dependent_attachment do
+        write_attachment create_attachment_from(attachable)
+      end
     end
 
     # Returns +true+ if an attachment has been made.
@@ -53,6 +54,23 @@ module ActiveStorage
     end
 
     private
+      def purge_dependent_attachment
+        if attached? && dependent == :purge_later
+          blob.tap do
+            transaction do
+              destroy
+              yield
+            end
+          end.purge_later
+        else
+          yield
+        end
+      end
+
+      def create_attachment_from(attachable)
+        ActiveStorage::Attachment.create!(record: record, name: name, blob: create_blob_from(attachable))
+      end
+
       def write_attachment(attachment)
         record.public_send("#{name}_attachment=", attachment)
       end
diff --git a/activestorage/test/models/attachments_test.rb b/activestorage/test/models/attachments_test.rb
index 7cfd8683db..ac346c0087 100644
--- a/activestorage/test/models/attachments_test.rb
+++ b/activestorage/test/models/attachments_test.rb
@@ -31,6 +31,31 @@ class ActiveStorage::AttachmentsTest < ActiveSupport::TestCase
     assert_equal "racecar.jpg", @user.avatar.filename.to_s
   end
 
+  test "replace attached blob" do
+    @user.avatar.attach create_blob(filename: "funky.jpg")
+
+    perform_enqueued_jobs do
+      assert_no_difference -> { ActiveStorage::Blob.count } do
+        @user.avatar.attach create_blob(filename: "town.jpg")
+      end
+    end
+
+    assert_equal "town.jpg", @user.avatar.filename.to_s
+  end
+
+  test "replace attached blob unsuccessfully" do
+    @user.avatar.attach create_blob(filename: "funky.jpg")
+
+    perform_enqueued_jobs do
+      assert_raises do
+        @user.avatar.attach nil
+      end
+    end
+
+    assert_equal "funky.jpg", @user.reload.avatar.filename.to_s
+    assert ActiveStorage::Blob.service.exist?(@user.avatar.key)
+  end
+
   test "access underlying associations of new blob" do
     @user.avatar.attach create_blob(filename: "funky.jpg")
     assert_equal @user, @user.avatar_attachment.record
-- 
cgit v1.2.3