Validations, changed up the deletion process, and added URI saving.

git-svn-id: https://svn.thoughtbot.com/plugins/paperclip/trunk@188 7bbfaf0e-4d1d-0410-9690-a8bb5f8ef2aa

lib/paperclip.rb

@@ -45,9 +45,9 @@ module Thoughtbot #:nodoc:
     }
     }
     
     
     class PaperclipError < StandardError #:nodoc:
     class PaperclipError < StandardError #:nodoc:
-      attr_accessor :attachment, :reason, :exception
-      def initialize attachment, reason, exception = nil
-        @attachment, @reason, @exception = *args
+      attr_accessor :attachment
+      def initialize attachment
+        @attachment = attachment
       end
       end
     end
     end
 
 
@@ -60,10 +60,14 @@ module Thoughtbot #:nodoc:
       # * attachment?: Alias for _attachment_ for clarity in determining if the attachment exists.
       # * attachment?: Alias for _attachment_ for clarity in determining if the attachment exists.
       # * attachment=(file): Sets the attachment to the file and creates the thumbnails (if necessary).
       # * attachment=(file): Sets the attachment to the file and creates the thumbnails (if necessary).
       #   +file+ can be anything normally accepted as an upload (+StringIO+ or +Tempfile+) or a +File+
       #   +file+ can be anything normally accepted as an upload (+StringIO+ or +Tempfile+) or a +File+
-      #   if it has had the +Upfile+ module included.
+      #   if it has had the +Upfile+ module included. +file+ can also be a URL object pointing to a valid
+      #   resource. This resource will be downloaded using +open-uri+[http://www.ruby-doc.org/stdlib/libdoc/open-uri/rdoc/]
+      #   and processed as a regular file object would. Finally, you can set this property to +nil+ to clear
+      #   the attachment, which is the same thing as calling +destroy_attachment+.
       #   Note this does not save the attachments.
       #   Note this does not save the attachments.
       #     user.avatar = File.new("~/pictures/me.png")
       #     user.avatar = File.new("~/pictures/me.png")
       #     user.avatar = params[:user][:avatar] # When :avatar is a file_field
       #     user.avatar = params[:user][:avatar] # When :avatar is a file_field
+      #     user.avatar = URI.parse("http://www.avatars-r-us.com/spiffy.png")
       # * attachment_file_name(style): The name of the file, including path information. Pass in the
       # * attachment_file_name(style): The name of the file, including path information. Pass in the
       #   name of a thumbnail to get the path to that thumbnail.
       #   name of a thumbnail to get the path to that thumbnail.
       #     user.avatar_file_name(:thumb) # => "public/users/44/thumb/me.png"
       #     user.avatar_file_name(:thumb) # => "public/users/44/thumb/me.png"
@@ -75,9 +79,11 @@ module Thoughtbot #:nodoc:
       # * attachment_valid?: If unsaved, returns true if all thumbnails have data (that is,
       # * attachment_valid?: If unsaved, returns true if all thumbnails have data (that is,
       #   they were successfully made). If saved, returns true if all expected files exist and are
       #   they were successfully made). If saved, returns true if all expected files exist and are
       #   of nonzero size.
       #   of nonzero size.
-      # * destroy_attachment(complain = false): Deletes the attachment and all thumbnails. Sets the +attachment_file_name+
-      #   column and +attachment_content_type+ column to +nil+. Set +complain+ to true to override
-      #   the +whiny_deletes+ option.
+      # * destroy_attachment(complain = false): Flags the attachment and all thumbnails for deletion. Sets
+      #   the +attachment_file_name+ column and +attachment_content_type+ column to +nil+. Set +complain+
+      #   to true to override the +whiny_deletes+ option. Note, this does not actually delete the attachment.
+      #   You must still call +save+ on the model to actually delete the file and commit the change to the
+      #   database.
       #
       #
       # == Options
       # == Options
       # There are a number of options you can set to change the behavior of Paperclip.
       # There are a number of options you can set to change the behavior of Paperclip.
@@ -143,6 +149,10 @@ module Thoughtbot #:nodoc:
       # Attached files are destroyed when the associated record is destroyed in a +before_destroy+ trigger. Set
       # Attached files are destroyed when the associated record is destroyed in a +before_destroy+ trigger. Set
       # the +delete_on_destroy+ option to +false+ to prevent this behavior. Also note that using the ActiveRecord's
       # the +delete_on_destroy+ option to +false+ to prevent this behavior. Also note that using the ActiveRecord's
       # +delete+ method instead of the +destroy+ method will prevent the +before_destroy+ trigger from firing.
       # +delete+ method instead of the +destroy+ method will prevent the +before_destroy+ trigger from firing.
+      #
+      # == Validation
+      # If there is a problem in the thumbnail-making process, Paperclip will add errors to your model on save. These
+      # errors appear if there is an error with +convert+ (e.g. +convert+ doesn't exist, the file wasn't an image, etc).
       def has_attached_file *attachment_names
       def has_attached_file *attachment_names
         options = attachment_names.last.is_a?(Hash) ? attachment_names.pop : {}
         options = attachment_names.last.is_a?(Hash) ? attachment_names.pop : {}
         options = DEFAULT_ATTACHMENT_OPTIONS.merge(options)
         options = DEFAULT_ATTACHMENT_OPTIONS.merge(options)
@@ -154,13 +164,17 @@ module Thoughtbot #:nodoc:
           attachments[attr] = (attachments[attr] || {:name => attr}).merge(options)
           attachments[attr] = (attachments[attr] || {:name => attr}).merge(options)
 
 
           define_method "#{attr}=" do |uploaded_file|
           define_method "#{attr}=" do |uploaded_file|
+            return send("destroy_#{attr}") if uploaded_file.nil?
+            uploaded_file = fetch_uri(uploaded_file) if uploaded_file.is_a? URI
             return unless is_a_file? uploaded_file
             return unless is_a_file? uploaded_file
+            
             attachments[attr].merge!({
             attachments[attr].merge!({
-              :dirty        => true,
-              :files        => {:original => uploaded_file},
-              :content_type => uploaded_file.content_type,
-              :filename     => sanitize_filename(uploaded_file.original_filename),
-              :errors       => []
+              :dirty          => true,
+              :files          => {:original => uploaded_file},
+              :content_type   => uploaded_file.content_type,
+              :filename       => sanitize_filename(uploaded_file.original_filename),
+              :errors         => [],
+              :delete_on_save => false
             })
             })
             write_attribute(:"#{attr}_file_name", attachments[attr][:filename])
             write_attribute(:"#{attr}_file_name", attachments[attr][:filename])
             write_attribute(:"#{attr}_content_type", attachments[attr][:content_type])
             write_attribute(:"#{attr}_content_type", attachments[attr][:content_type])
@@ -186,7 +200,7 @@ module Thoughtbot #:nodoc:
           define_method "#{attr}_attachment" do
           define_method "#{attr}_attachment" do
             attachments[attr]
             attachments[attr]
           end
           end
-          private "#{attr}_attachment"
+          private :"#{attr}_attachment"
           
           
           define_method "#{attr}_file_name" do |*args|
           define_method "#{attr}_file_name" do |*args|
             style = args.shift || attachments[attr][:default_style] # This prevents arity warnings
             style = args.shift || attachments[attr][:default_style] # This prevents arity warnings
@@ -199,31 +213,42 @@ module Thoughtbot #:nodoc:
           end
           end
           
           
           define_method "#{attr}_valid?" do
           define_method "#{attr}_valid?" do
-            attachments[attr][:thumbnails].all? do |style, geometry|
-              attachments[attr][:dirty] ?
-                !attachments[attr][:files][style].blank? && attachments[attr][:errors].empty? :
-                File.file?( path_for(attachments[attr], style))
+            attachments[attr][:thumbnails].merge(:original => nil).all? do |style, geometry|
+              if read_attribute("#{attr}_file_name")
+                if attachments[attr][:dirty]
+                  !attachments[attr][:files][style].blank? && attachments[attr][:errors].empty?
+                else
+                  File.file?( path_for(attachments[attr], style) )
+                end
+              else
+                false
+              end
             end
             end
           end
           end
           
           
           define_method "destroy_#{attr}" do |*args|
           define_method "destroy_#{attr}" do |*args|
             complain = args.first || false
             complain = args.first || false
             if attachments[attr].keys.any?
             if attachments[attr].keys.any?
-              delete_attachment attachments[attr], complain
+              attachments[attr][:files] = nil
+              attachments[attr][:delete_on_save] = true
+              attachments[attr][:complain_on_delete] = complain
+              write_attribute("#{attr}_file_name", nil)
+              write_attribute("#{attr}_content_type", nil)
             end
             end
+            true
           end
           end
           
           
-          # alias_method_chain :save, :paperclip
-          
           validates_each attr do |r, a, v|
           validates_each attr do |r, a, v|
             attachments[attr][:errors].each{|e| r.errors.add(attr, e) } if attachments[attr][:errors]
             attachments[attr][:errors].each{|e| r.errors.add(attr, e) } if attachments[attr][:errors]
           end
           end
 
 
           define_method "#{attr}_before_save" do
           define_method "#{attr}_before_save" do
             if attachments[attr].keys.any?
             if attachments[attr].keys.any?
-              write_attachment attachments[attr] if attachments[attr][:files]
-              attachments[attr][:dirty] = false
-              attachments[attr][:files] = nil
+              write_attachment  attachments[attr] if attachments[attr][:files]
+              delete_attachment attachments[attr], attachments[attr][:complain_on_delete] if attachments[attr][:delete_on_save]
+              attachments[attr][:delete_on_save] = false
+              attachments[attr][:dirty]          = false
+              attachments[attr][:files]          = nil
             end
             end
           end
           end
           private :"#{attr}_before_save"
           private :"#{attr}_before_save"
@@ -238,18 +263,17 @@ module Thoughtbot #:nodoc:
           before_destroy :"#{attr}_before_destroy"
           before_destroy :"#{attr}_before_destroy"
         end
         end
       end
       end
-    end
-
-    module InstanceMethods #:nodoc:
       
       
-      def save_with_paperclip perform_validations = true
-        begin
-          save_without_paperclip(perform_validations)
-        rescue PaperclipError => e
-          self.errors.add(e.attachment, "could not be saved because of #{e.reason}")
-          false
+      # Adds errors if the attachments you specify are either missing or had errors on them.
+      # Essentially, acts like validates_presence_of for attachments.
+      def validates_attached_file *attachment_names
+        validates_each *attachment_names do |r, a, v|
+          r.errors.add(a, "requires a valid attachment.") unless r.send("#{a}_valid?")
         end
         end
       end
       end
+    end
+
+    module InstanceMethods #:nodoc:
       
       
       private
       private
       
       
@@ -290,6 +314,7 @@ module Thoughtbot #:nodoc:
       end
       end
       
       
       def write_attachment attachment
       def write_attachment attachment
+        return if attachment[:files].blank?
         ensure_directories_for attachment
         ensure_directories_for attachment
         attachment[:files].each do |style, atch|
         attachment[:files].each do |style, atch|
           File.open( path_for(attachment, style), "w" ) do |file|
           File.open( path_for(attachment, style), "w" ) do |file|
@@ -308,16 +333,13 @@ module Thoughtbot #:nodoc:
             raise PaperclipError(attachment[:name], e.message, e) if ::Thoughtbot::Paperclip.options[:whiny_deletes] || complain
             raise PaperclipError(attachment[:name], e.message, e) if ::Thoughtbot::Paperclip.options[:whiny_deletes] || complain
           end
           end
         end
         end
-        self.update_attribute "#{attachment[:name]}_file_name", nil
-        self.update_attribute "#{attachment[:name]}_content_type", nil
       end
       end
 
 
       def make_thumbnail attachment, orig_io, geometry
       def make_thumbnail attachment, orig_io, geometry
         operator = geometry[-1,1]
         operator = geometry[-1,1]
-        geometry, crop_geometry = geometry_for_crop(geometry, orig_io) if operator == '#'
         begin
         begin
-          command = "#{::Thoughtbot::Paperclip.options[:image_magick_path]}/convert - -scale '#{geometry}' #{operator == '#' ? "-crop '#{crop_geometry}'" : ""} -"
-          ActiveRecord::Base.logger.info("Thumbnail: '#{command}'")
+          geometry, crop_geometry = geometry_for_crop(geometry, orig_io) if operator == '#'
+          command = "#{::Thoughtbot::Paperclip.options[:image_magick_path]}/convert - -scale '#{geometry}' #{operator == '#' ? "-crop '#{crop_geometry}'" : ""} - 2>/dev/null"
           thumb = IO.popen(command, "w+") do |io|
           thumb = IO.popen(command, "w+") do |io|
             orig_io.rewind
             orig_io.rewind
             io.write(orig_io.read)
             io.write(orig_io.read)
@@ -325,18 +347,18 @@ module Thoughtbot #:nodoc:
             StringIO.new(io.read)
             StringIO.new(io.read)
           end
           end
         rescue Errno::EPIPE => e
         rescue Errno::EPIPE => e
-          raise PaperclipError.new(attachment, "Could not create thumbnail. Is ImageMagick or GraphicsMagick installed and available?", e)
+          raise PaperclipError.new(attachment), "Could not create thumbnail. Is ImageMagick or GraphicsMagick installed and available?"
         rescue SystemCallError => e
         rescue SystemCallError => e
-          raise PaperclipError.new(attachment, "Could not create thumbnail.", e)
+          raise PaperclipError.new(attachment), "Could not create thumbnail."
         end
         end
-        if ::Thoughtbot::Paperclip.options[:whiny_thumbnails]
-          raise PaperclipError.new(attachment, "Convert returned with result code #{$?.exitstatus}: #{thumb.read}") unless $?.success?
+        if ::Thoughtbot::Paperclip.options[:whiny_thumbnails] && !$?.success?
+          raise PaperclipError.new(attachment), "Convert returned with result code #{$?.exitstatus}: #{thumb.read}"
         end
         end
         thumb
         thumb
       end
       end
       
       
       def geometry_for_crop geometry, orig_io
       def geometry_for_crop geometry, orig_io
-        IO.popen("#{::Thoughtbot::Paperclip.options[:image_magick_path]}/identify -", "w+") do |io|
+        IO.popen("#{::Thoughtbot::Paperclip.options[:image_magick_path]}/identify - 2>/dev/null", "w+") do |io|
           orig_io.rewind
           orig_io.rewind
           io.write(orig_io.read)
           io.write(orig_io.read)
           io.close_write
           io.close_write
@@ -346,7 +368,7 @@ module Thoughtbot #:nodoc:
             dst   = geometry.match(/(\d+)x(\d+)/)[1,2].map(&:to_f)
             dst   = geometry.match(/(\d+)x(\d+)/)[1,2].map(&:to_f)
             dsth  = dst[0] > dst[1]
             dsth  = dst[0] > dst[1]
             ar    = src[0] / src[1]
             ar    = src[0] / src[1]
-            
+
             scale_geometry, scale = if dst[0] == dst[1]
             scale_geometry, scale = if dst[0] == dst[1]
               if srch
               if srch
                 [ "x#{dst[1]}", src[1] / dst[1] ]
                 [ "x#{dst[1]}", src[1] / dst[1] ]
@@ -358,17 +380,37 @@ module Thoughtbot #:nodoc:
             else
             else
               [ "x#{dst[1]}", src[1] / dst[1] ]
               [ "x#{dst[1]}", src[1] / dst[1] ]
             end
             end
-            
+
             crop_geometry = if dsth
             crop_geometry = if dsth
               "%dx%d+%d+%d" % [ dst[0], dst[1], 0, (src[1] / scale - dst[1]) / 2 ]
               "%dx%d+%d+%d" % [ dst[0], dst[1], 0, (src[1] / scale - dst[1]) / 2 ]
             else
             else
               "%dx%d+%d+%d" % [ dst[0], dst[1], (src[0] / scale - dst[0]) / 2, 0 ]
               "%dx%d+%d+%d" % [ dst[0], dst[1], (src[0] / scale - dst[0]) / 2, 0 ]
             end
             end
-            
+
             [ scale_geometry, crop_geometry ]
             [ scale_geometry, crop_geometry ]
           end
           end
         end
         end
       end
       end
+
+      def fetch_uri uri
+        require 'open-uri'
+        # I hate the fact that URI and open-uri can't handle file:// urls.
+        if uri.scheme == 'file'
+          path = url.gsub(%r{^file://}, '/')
+          image_data = open(path)
+        else
+          image_data = open(uri.to_s)
+        end
+
+        image = StringIO.new(image_data.read)
+        image_data.close
+
+        image.instance_eval { class << self; attr_accessor :content_type, :original_filename; end }
+        image.content_type = image_data.content_type
+        image.original_filename = File.basename(uri.path)
+
+        image
+      end
       
       
       def is_a_file? data
       def is_a_file? data
         [:size, :content_type, :original_filename, :read].map do |meth|
         [:size, :content_type, :original_filename, :read].map do |meth|

test/models.rb

@@ -25,6 +25,7 @@ end
 class Bar < ActiveRecord::Base
 class Bar < ActiveRecord::Base
   has_attached_file :document, :attachment_type => :document,
   has_attached_file :document, :attachment_type => :document,
                     :path_prefix => "./repository"
                     :path_prefix => "./repository"
+  validates_attached_file :document
 end
 end
 
 
 class NonStandard < ActiveRecord::Base
 class NonStandard < ActiveRecord::Base

test/paperclip_images_test.rb

 require 'test/unit'
 require 'test/unit'
+require 'uri'
 require File.dirname(__FILE__) + "/test_helper.rb"
 require File.dirname(__FILE__) + "/test_helper.rb"
 require File.dirname(__FILE__) + "/../init.rb"
 require File.dirname(__FILE__) + "/../init.rb"
 require File.join(File.dirname(__FILE__), "models.rb")
 require File.join(File.dirname(__FILE__), "models.rb")
 
 
 class PaperclipImagesTest < Test::Unit::TestCase
 class PaperclipImagesTest < Test::Unit::TestCase
   def setup
   def setup
-    assert @foo = Foo.new
-    assert @file = File.new(File.join(File.dirname(__FILE__), 'fixtures', 'test_image.jpg'))
+    assert @foo       = Foo.new
+    assert @file      = File.new(File.join(File.dirname(__FILE__), 'fixtures', 'test_image.jpg'))
+    assert @document  = File.new(File.join(File.dirname(__FILE__), 'fixtures', 'test_document.doc'))
     assert @foo.image = @file
     assert @foo.image = @file
   end
   end
 
 
@@ -66,6 +68,7 @@ class PaperclipImagesTest < Test::Unit::TestCase
     end
     end
     
     
     assert @foo.destroy_image
     assert @foo.destroy_image
+    assert @foo.save
     mappings.each do |style, file, url|
     mappings.each do |style, file, url|
       assert_not_equal file, @foo.image_file_name(style)
       assert_not_equal file, @foo.image_file_name(style)
       assert_equal "", @foo.image_file_name(style)
       assert_equal "", @foo.image_file_name(style)
@@ -88,4 +91,44 @@ class PaperclipImagesTest < Test::Unit::TestCase
     end
     end
   end
   end
   
   
+  def test_should_save_image_from_uri
+    uri = URI.parse("http://thoughtbot.com/assets/11/thoughtbot.jpg")
+    @foo.image = uri
+    @foo.save
+    @foo.image_valid?
+    assert File.exists?( @foo.image_file_name(:original) ), @foo.image_file_name(:original)
+    assert File.exists?( @foo.image_file_name(:medium) ), @foo.image_file_name(:medium)
+    assert File.exists?( @foo.image_file_name(:thumb) ), @foo.image_file_name(:thumb)
+    out = `identify '#{@foo.image_file_name(:original)}'`; assert out.match("350x100"); assert $?.exitstatus == 0
+    out = `identify '#{@foo.image_file_name(:medium)}'`;   assert out.match("300x86");  assert $?.exitstatus == 0
+    out = `identify '#{@foo.image_file_name(:thumb)}'`;    assert out.match("100x29");  assert $?.exitstatus == 0
+  end
+
+  def test_should_put_errors_on_object_if_convert_does_not_exist
+    old_path = Thoughtbot::Paperclip.options[:image_magick_path]
+    Thoughtbot::Paperclip.options[:image_magick_path] = "/does/not/exist"
+
+    assert_nothing_raised{ @foo.image = @file }
+    assert !@foo.save
+    assert !@foo.valid?
+    assert @foo.errors.length > 0
+    assert @foo.errors.on(:image)
+    [@foo.errors.on(:image)].flatten.each do |err|
+      assert_match /could not/, err, err
+    end
+  ensure
+    Thoughtbot::Paperclip.options[:image_magick_path] = old_path
+  end
+
+  def test_should_put_errors_on_object_if_convert_fails
+    assert_nothing_raised{ @foo.image = @document }
+    assert !@foo.save
+    assert !@foo.valid?
+    assert @foo.errors.length > 0
+    assert @foo.errors.on(:image)
+    [@foo.errors.on(:image)].flatten.each do |err|
+      assert_match /could not/, err, err
+    end
+  end
+
 end
 end
\ No newline at end of file

test/paperclip_non_standard_test.rb

@@ -19,7 +19,7 @@ class PaperclipNonStandardTest < Test::Unit::TestCase
 
 
   def test_should_save_the_created_file_to_the_final_asset_directory
   def test_should_save_the_created_file_to_the_final_asset_directory
     assert @ns.save
     assert @ns.save
-    assert File.exists?( @ns.resume_file_name )
+    assert File.exists?( @ns.resume_file_name ), @ns.resume_file_name
     assert File.exists?( @ns.avatar_file_name(:original) )
     assert File.exists?( @ns.avatar_file_name(:original) )
     assert File.exists?( @ns.avatar_file_name(:bigger) )
     assert File.exists?( @ns.avatar_file_name(:bigger) )
     assert File.exists?( @ns.avatar_file_name(:cropped) )
     assert File.exists?( @ns.avatar_file_name(:cropped) )

test/paperclip_test.rb

@@ -39,5 +39,15 @@ class PaperclipTest < Test::Unit::TestCase
     assert @bar.destroy
     assert @bar.destroy
     assert !File.exists?( document_file_name ), document_file_name
     assert !File.exists?( document_file_name ), document_file_name
   end
   end
+  
+  def test_should_put_on_errors_if_no_file_exists
+    assert @bar.save
+    @bar.document = nil
+    assert !@bar.document_valid?
+    assert !@bar.save
+    assert @bar.errors.length > 0
+    assert @bar.errors.on(:document)
+    assert_match /requires a valid/, @bar.errors.on(:document), @bar.errors.on(:document)
+  end
 
 
 end
 end
\ No newline at end of file