Permalink
Browse files

Removed the ability for nil to delete an attachment, sadly.

  • Loading branch information...
1 parent f44ede7 commit 2db00be2e137c1fd8d69c35dd3e56ada28c511b1 @jyurek jyurek committed Mar 12, 2009
Showing with 51 additions and 26 deletions.
  1. +21 −1 lib/paperclip/attachment.rb
  2. +22 −3 test/attachment_test.rb
  3. +8 −22 test/integration_test.rb
@@ -62,6 +62,9 @@ def initialize name, instance, options = {}
# If the file that is assigned is not valid, the processing (i.e.
# thumbnailing, etc) will NOT be run.
def assign uploaded_file
+ # This is because of changes in Rails 2.3 that cause blank fields to send nil
+ return nil if uploaded_file.nil?
+
%w(file_name).each do |field|
unless @instance.class.column_names.include?("#{name}_#{field}")
raise PaperclipError.new("#{@instance.class} model does not have required column '#{name}_#{field}'")
@@ -158,6 +161,23 @@ def save
end
end
+ # Clears out the attachment. Has the same effect as previously assigning
+ # nil to the attachment. Does NOT save. If you wish to clear AND save,
+ # use #destroy.
+ def clear
+ queue_existing_for_delete
+ @errors = {}
+ @validation_errors = nil
+ end
+
+ # Destroys the attachment. Has the same effect as previously assigning
+ # nil to the attachment *and saving*. This is permanent. If you wish to
+ # wipe out the existing attachment but not save, use #clear.
+ def destroy
+ clear
+ save
+ end
+
# Returns the name of the file as originally assigned, and lives in the
# <attachment>_file_name attribute of the model.
def original_filename
@@ -273,7 +293,7 @@ def logging? #:nodoc:
end
def valid_assignment? file #:nodoc:
- file.nil? || (file.respond_to?(:original_filename) && file.respond_to?(:content_type))
+ file.respond_to?(:original_filename) && file.respond_to?(:content_type)
end
def validate #:nodoc:
View
@@ -592,20 +592,39 @@ def do_after_all; end
file.close
end
- context "and deleted" do
+ context "and trying to delete" do
setup do
@existing_names = @attachment.styles.keys.collect do |style|
@attachment.path(style)
end
+ end
+
+ should "not delete the files saving in a deprecated manner" do
+ @attachment.expects(:instance_write).with(:file_name, nil).never
+ @attachment.expects(:instance_write).with(:content_type, nil).never
+ @attachment.expects(:instance_write).with(:file_size, nil).never
+ @attachment.expects(:instance_write).with(:updated_at, nil).never
+ @attachment.assign nil
+ @attachment.save
+ @existing_names.each{|f| assert File.exists?(f) }
+ end
+
+ should "delete the files when you call #clear and #save" do
@attachment.expects(:instance_write).with(:file_name, nil)
@attachment.expects(:instance_write).with(:content_type, nil)
@attachment.expects(:instance_write).with(:file_size, nil)
@attachment.expects(:instance_write).with(:updated_at, nil)
- @attachment.assign nil
+ @attachment.clear
@attachment.save
+ @existing_names.each{|f| assert ! File.exists?(f) }
end
- should "delete the files" do
+ should "delete the files when you call #delete" do
+ @attachment.expects(:instance_write).with(:file_name, nil)
+ @attachment.expects(:instance_write).with(:content_type, nil)
+ @attachment.expects(:instance_write).with(:file_size, nil)
+ @attachment.expects(:instance_write).with(:updated_at, nil)
+ @attachment.destroy
@existing_names.each{|f| assert ! File.exists?(f) }
end
end
View
@@ -96,7 +96,7 @@ class IntegrationTest < Test::Unit::TestCase
context "and deleted" do
setup do
- @dummy.avatar = nil
+ @dummy.avatar.clear
@dummy.save
end
@@ -235,7 +235,7 @@ class IntegrationTest < Test::Unit::TestCase
assert File.exists?(p)
end
- @dummy.avatar = nil
+ @dummy.avatar.clear
assert_nil @dummy.avatar_file_name
assert @dummy.valid?
assert @dummy.save
@@ -258,15 +258,15 @@ class IntegrationTest < Test::Unit::TestCase
saved_paths = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.path(s) }
- @d2.avatar = nil
+ @d2.avatar.clear
assert @d2.save
saved_paths.each do |p|
assert ! File.exists?(p)
end
end
- should "know the difference between good files, bad files, not files, and nil" do
+ should "know the difference between good files, bad files, and not files" do
expected = @dummy.avatar.to_file
@dummy.avatar = "not a file"
assert @dummy.valid?
@@ -275,25 +275,21 @@ class IntegrationTest < Test::Unit::TestCase
@dummy.avatar = @bad_file
assert ! @dummy.valid?
- @dummy.avatar = nil
- assert @dummy.valid?, @dummy.errors.inspect
end
- should "know the difference between good files, bad files, not files, and nil when validating" do
+ should "know the difference between good files, bad files, and not files when validating" do
Dummy.validates_attachment_presence :avatar
@d2 = Dummy.find(@dummy.id)
@d2.avatar = @file
assert @d2.valid?, @d2.errors.full_messages.inspect
@d2.avatar = @bad_file
assert ! @d2.valid?
- @d2.avatar = nil
- assert ! @d2.valid?
end
should "be able to reload without saving and not have the file disappear" do
@dummy.avatar = @file
assert @dummy.save
- @dummy.avatar = nil
+ @dummy.avatar.clear
assert_nil @dummy.avatar_file_name
@dummy.reload
assert_equal "5k.png", @dummy.avatar_file_name
@@ -316,16 +312,6 @@ class IntegrationTest < Test::Unit::TestCase
assert_equal `identify -format "%wx%h" "#{@dummy.avatar.path(:original)}"`,
`identify -format "%wx%h" "#{@dummy2.avatar.path(:original)}"`
end
-
- should "work when assigned a nil file" do
- @dummy2.avatar = nil
- @dummy2.save
-
- @dummy.avatar = @dummy2.avatar
- @dummy.save
-
- assert !@dummy.avatar?
- end
end
end
@@ -423,7 +409,7 @@ def s3_headers_for attachment, style
assert key.exists?
end
- @dummy.avatar = nil
+ @dummy.avatar.clear
assert_nil @dummy.avatar_file_name
assert @dummy.valid?
assert @dummy.save
@@ -446,7 +432,7 @@ def s3_headers_for attachment, style
saved_keys = [:thumb, :medium, :large, :original].collect{|s| @dummy.avatar.to_file(s) }
- @d2.avatar = nil
+ @d2.avatar.clear
assert @d2.save
saved_keys.each do |key|

2 comments on commit 2db00be

This will make change some of our applications but it’s nicer to “clear” or “destroy” the assets than to assign nil. Happy for the change, so don’t feel sad. ;)

Yeh, it was a nice feature but I think this is easier to understand – I like having #clear and #destroy separately, too. :-)

Please sign in to comment.