@wip Fixed bug where processing occurs on invalid objects

- Because the processors were called on assignment, instead of during saving,
  the validations could never work correctly. This is because the built in
  validations use the values in the db columns to operate. However, since these
  are populated on assignment, the validations cannot run before the processors
  run. Moreover, any other type of validation not dependent on the db columns
  also cannot run, because the processors are called on assignment. The
  processors should be called during save which allows for validations to occur.

- Fixed tests that assert the incorrect behavior

- Closes thoughtbot#2462, Closes thoughtbot#2321, Closes
  thoughtbot#2236, Closes thoughtbot#2178,
  Closes thoughtbot#1960, Closes thoughtbot#2204

lib/paperclip/attachment.rb

@@ -109,8 +109,6 @@ def assign(uploaded_file)
           nil
         else
           assign_attributes
-          post_process_file
-          reset_file_if_original_reprocessed
         end
       else
         nil
@@ -238,6 +236,8 @@ def dirty?
     # Saves the file, if there are no errors. If there are, it flushes them to
     # the instance's errors and returns false, cancelling the save.
     def save
+      post_process_file
+      reset_file_if_original_reprocessed
       flush_deletes unless @options[:keep_old_files]
       process = only_process
       if process.any? && !process.include?(:original)

spec/paperclip/attachment_processing_spec.rb

@@ -4,49 +4,63 @@
   before { rebuild_class }
 
   context 'using validates_attachment_content_type' do
-    it 'processes attachments given a valid assignment' do
+    it "doesn't process attachments on assignment" do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment_content_type :avatar, content_type: "image/png"
       instance = Dummy.new
       attachment = instance.avatar
-      attachment.expects(:post_process_styles)
+      attachment.expects(:post_process_styles).never
 
       attachment.assign(file)
     end
 
-    it 'does not process attachments given an invalid assignment with :not' do
+    it 'processes attachments given the object is valid' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment_content_type :avatar, content_type: "image/png"
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.assign(file)
+      attachment.expects(:post_process_styles)
+
+      attachment.save
+    end
+
+    it 'does not process attachments given an invalid object with :not' do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment_content_type :avatar, not: "image/png"
       instance = Dummy.new
       attachment = instance.avatar
+      attachment.assign(file)
       attachment.expects(:post_process_styles).never
 
-      attachment.assign(file)
+      attachment.save
     end
 
-    it 'does not process attachments given an invalid assignment with :content_type' do
+    it 'does not process attachments given an invalid object with :content_type' do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment_content_type :avatar, content_type: "image/tiff"
       instance = Dummy.new
       attachment = instance.avatar
+      attachment.assign(file)
       attachment.expects(:post_process_styles).never
 
-      attachment.assign(file)
+      attachment.save
     end
 
     it 'allows what would be an invalid assignment when validation :if clause returns false' do
       invalid_assignment = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment_content_type :avatar, content_type: "image/tiff", if: lambda{false}
       instance = Dummy.new
       attachment = instance.avatar
+      attachment.assign(invalid_assignment)
       attachment.expects(:post_process_styles)
 
-      attachment.assign(invalid_assignment)
+      attachment.save
     end
   end
 
   context 'using validates_attachment' do
-    it 'processes attachments given a valid assignment' do
+    it "doesn't process attachments on assignment" do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment :avatar, content_type: {content_type: "image/png"}
       instance = Dummy.new
@@ -56,24 +70,37 @@
       attachment.assign(file)
     end
 
-    it 'does not process attachments given an invalid assignment with :not' do
+    it 'processes attachments given a valid object' do
+      file = File.new(fixture_file("5k.png"))
+      Dummy.validates_attachment :avatar, content_type: {content_type: "image/png"}
+      instance = Dummy.new
+      attachment = instance.avatar
+      attachment.assign(file)
+      attachment.expects(:post_process_styles)
+
+      attachment.save
+    end
+
+    it 'does not process attachments given an invalid object with :not' do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment :avatar, content_type: {not: "image/png"}
       instance = Dummy.new
       attachment = instance.avatar
+      attachment.assign(file)
       attachment.expects(:post_process_styles).never
 
-      attachment.assign(file)
+      attachment.save
     end
 
-    it 'does not process attachments given an invalid assignment with :content_type' do
+    it 'does not process attachments given an invalid object with :content_type' do
       file = File.new(fixture_file("5k.png"))
       Dummy.validates_attachment :avatar, content_type: {content_type: "image/tiff"}
       instance = Dummy.new
       attachment = instance.avatar
+      attachment.assign(file)
       attachment.expects(:post_process_styles).never
 
-      attachment.assign(file)
+      attachment.save
     end
   end
 end

spec/paperclip/attachment_spec.rb

@@ -711,9 +711,10 @@ class Paperclip::Test < Paperclip::Processor; end
       Paperclip::Thumbnail.stubs(:make).returns(@file)
     end
 
-    context "when assigned" do
+    context "when saved" do
       it "calls #make on all specified processors" do
         @dummy.avatar = @file
+        @dummy.save
 
         expect(Paperclip::Thumbnail).to have_received(:make)
         expect(Paperclip::Test).to have_received(:make)
@@ -729,12 +730,14 @@ class Paperclip::Test < Paperclip::Processor; end
         })
 
         @dummy.avatar = @file
+        @dummy.save
 
         expect(Paperclip::Thumbnail).to have_received(:make).with(anything, expected_params, anything)
       end
 
       it "calls #make with attachment passed as third argument" do
         @dummy.avatar = @file
+        @dummy.save
 
         expect(Paperclip::Test).to have_received(:make).with(anything, anything, @dummy.avatar)
       end
@@ -745,6 +748,42 @@ class Paperclip::Test < Paperclip::Processor; end
         @dummy.avatar = @file
       end
     end
+
+    context "when assigned" do
+      it "doesn't call #make on all specified processors" do
+        @dummy.avatar = @file
+
+        expect(Paperclip::Thumbnail).not_to have_received(:make)
+        expect(Paperclip::Test).not_to have_received(:make)
+      end
+
+      it "doesn't call #make with the right parameters passed as second argument" do
+        expected_params = @style_params[:once].merge({
+          style: :once,
+          processors: [:thumbnail, :test],
+          whiny: true,
+          convert_options: "",
+          source_file_options: ""
+        })
+
+        @dummy.avatar = @file
+
+        expect(Paperclip::Thumbnail).not_to have_received(:make).with(anything, expected_params, anything)
+      end
+
+      it "doesn't call #make with attachment passed as third argument" do
+        @dummy.avatar = @file
+
+        expect(Paperclip::Test).not_to have_received(:make).with(anything, anything, @dummy.avatar)
+      end
+
+      it "doesn't call #make and unlinks intermediary files afterward" do
+        # TODO: Fix this test?
+        @dummy.avatar.expects(:unlink_files).with([@file, @file])
+
+        @dummy.avatar = @file
+      end
+    end
   end
 
   context "An attachment with a processor that returns original file" do
@@ -758,8 +797,18 @@ def make; @file; end
       @dummy = Dummy.new
     end
 
+    context "when saved" do
+      it "calls #make and doesn't unlink the original file" do
+        @dummy.avatar.expects(:unlink_files).with([])
+
+        @dummy.avatar = @file
+        @dummy.save
+      end
+    end
+
     context "when assigned" do
-      it "#calls #make and doesn't unlink the original file" do
+      it "doesn't call #make and doesn't unlink the original file" do
+        # TODO: Fix this test?
         @dummy.avatar.expects(:unlink_files).with([])
 
         @dummy.avatar = @file
@@ -822,6 +871,7 @@ def make; @file; end
   end
 
   context "Assigning an attachment with post_process hooks" do
+    # NOTE: Investigate the before_post_process hooks, as they may be intended to run on assignment
     before do
       rebuild_class styles: { something: "100x100#" }
       Dummy.class_eval do
@@ -841,13 +891,23 @@ def do_after_all; end
       @attachment = @dummy.avatar
     end
 
-    it "calls the defined callbacks when assigned" do
+    it "calls the defined callbacks when saved" do
       @dummy.expects(:do_before_avatar).with()
       @dummy.expects(:do_after_avatar).with()
       @dummy.expects(:do_before_all).with()
       @dummy.expects(:do_after_all).with()
       Paperclip::Thumbnail.expects(:make).returns(@file)
       @dummy.avatar = @file
+      @dummy.save
+    end
+
+    it "doesn't call the defined callbacks when assigned" do
+      @dummy.expects(:do_before_avatar).never
+      @dummy.expects(:do_after_avatar).never
+      @dummy.expects(:do_before_all).never
+      @dummy.expects(:do_after_all).never
+      Paperclip::Thumbnail.expects(:make).never
+      @dummy.avatar = @file
     end
 
     it "does not cancel the processing if a before_post_process returns nil" do
@@ -857,6 +917,7 @@ def do_after_all; end
       @dummy.expects(:do_after_all).with()
       Paperclip::Thumbnail.expects(:make).returns(@file)
       @dummy.avatar = @file
+      @dummy.save
     end
 
     it "cancels the processing if a before_post_process returns false" do
@@ -866,6 +927,7 @@ def do_after_all; end
       @dummy.expects(:do_after_all)
       Paperclip::Thumbnail.expects(:make).never
       @dummy.avatar = @file
+      @dummy.save
     end
 
     it "cancels the processing if a before_avatar_post_process returns false" do
@@ -875,6 +937,7 @@ def do_after_all; end
       @dummy.expects(:do_after_all)
       Paperclip::Thumbnail.expects(:make).never
       @dummy.avatar = @file
+      @dummy.save
     end
   end