Allow to detach an attachment when record is not persisted

Detaching a not persisted record no longer raise an error.

Moves the `detach` logic of `ActiveStorage::Attached` to `Attached::Changes API`.

activestorage/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Allow to detach an attachment when record is not persisted
+
+    *Jacopo Beschi*
+
 *   Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips`  
 
     *Breno Gazzola*

activestorage/lib/active_storage/attached/changes.rb

@@ -12,6 +12,9 @@ module ActiveStorage
       autoload :DeleteOne
       autoload :DeleteMany
 
+      autoload :DetachOne
+      autoload :DetachMany
+
       autoload :PurgeOne
       autoload :PurgeMany
     end

activestorage/lib/active_storage/attached/changes/detach_many.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::DetachMany #:nodoc:
+    attr_reader :name, :record, :attachments
+
+    def initialize(name, record, attachments)
+      @name, @record, @attachments = name, record, attachments
+    end
+
+    def detach
+      if attachments.any?
+        attachments.delete_all if attachments.respond_to?(:delete_all)
+        record.attachment_changes.delete(name)
+      end
+    end
+  end
+end

activestorage/lib/active_storage/attached/changes/detach_one.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module ActiveStorage
+  class Attached::Changes::DetachOne #:nodoc:
+    attr_reader :name, :record, :attachment
+
+    def initialize(name, record, attachment)
+      @name, @record, @attachment = name, record, attachment
+    end
+
+    def detach
+      if attachment.present?
+        attachment.delete
+        reset
+      end
+    end
+
+    private
+      def reset
+        record.attachment_changes.delete(name)
+        record.public_send("#{name}_attachment=", nil)
+      end
+  end
+end

activestorage/lib/active_storage/attached/many.rb

@@ -16,6 +16,12 @@ module ActiveStorage
     # Purges each associated attachment through the queuing system.
     delegate :purge_later, to: :purge_many
 
+    ##
+    # :method: detach
+    #
+    # Deletes associated attachments without purging them, leaving their respective blobs in place.
+    delegate :detach, to: :detach_many
+
     delegate_missing_to :attachments
 
     # Returns all the associated attachment records.
@@ -60,14 +66,13 @@ module ActiveStorage
       attachments.any?
     end
 
-    # Deletes associated attachments without purging them, leaving their respective blobs in place.
-    def detach
-      attachments.delete_all if attached?
-    end
-
     private
       def purge_many
         Attached::Changes::PurgeMany.new(name, record, attachments)
       end
+
+      def detach_many
+        Attached::Changes::DetachMany.new(name, record, attachments)
+      end
   end
 end

activestorage/lib/active_storage/attached/one.rb

@@ -16,6 +16,12 @@ module ActiveStorage
     # Purges the attachment through the queuing system.
     delegate :purge_later, to: :purge_one
 
+    ##
+    # :method: detach
+    #
+    # Deletes the attachment without purging it, leaving its blob in place.
+    delegate :detach, to: :detach_one
+
     delegate_missing_to :attachment, allow_nil: true
 
     # Returns the associated attachment record.
@@ -67,21 +73,13 @@ module ActiveStorage
       attachment.present?
     end
 
-    # Deletes the attachment without purging it, leaving its blob in place.
-    def detach
-      if attached?
-        attachment.delete
-        write_attachment nil
-      end
-    end
-
     private
-      def write_attachment(attachment)
-        record.public_send("#{name}_attachment=", attachment)
-      end
-
       def purge_one
         Attached::Changes::PurgeOne.new(name, record, attachment)
       end
+
+      def detach_one
+        Attached::Changes::DetachOne.new(name, record, attachment)
+      end
   end
 end

activestorage/test/models/attached/many_test.rb

@@ -440,6 +440,24 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "detaching when record is not persisted" do
+    [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
+      user = User.new
+      user.highlights.attach blobs
+      assert user.highlights.attached?
+
+      perform_enqueued_jobs do
+        user.highlights.detach
+      end
+
+      assert_not user.highlights.attached?
+      assert ActiveStorage::Blob.exists?(blobs.first.id)
+      assert ActiveStorage::Blob.exists?(blobs.second.id)
+      assert ActiveStorage::Blob.service.exist?(blobs.first.key)
+      assert ActiveStorage::Blob.service.exist?(blobs.second.key)
+    end
+  end
+
   test "purging" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs

activestorage/test/models/attached/one_test.rb

@@ -451,6 +451,22 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "detaching when record is not persisted" do
+    create_blob(filename: "funky.jpg").tap do |blob|
+      user = User.new
+      user.avatar.attach blob
+      assert user.avatar.attached?
+
+      perform_enqueued_jobs do
+        user.avatar.detach
+      end
+
+      assert_not user.avatar.attached?
+      assert ActiveStorage::Blob.exists?(blob.id)
+      assert ActiveStorage::Blob.service.exist?(blob.key)
+    end
+  end
+
   test "purging" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob