From 740f14756b91359338f4a57049bb054e8a0d1115 Mon Sep 17 00:00:00 2001 From: shalinibhawsingka-kreeti Date: Wed, 25 Dec 2019 12:29:24 +0530 Subject: [PATCH 1/3] [bug] executing the validation callback before post_process is done so that if validations fail, post_process won't be called --- lib/paperclip/attachment.rb | 14 ++++++++++---- lib/paperclip/has_attached_file.rb | 2 +- lib/paperclip/validators.rb | 2 +- spec/paperclip/attachment_spec.rb | 7 +++++-- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index de5c45ec7..83ae84747 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -491,14 +491,20 @@ def process_options(options_type, style) #:nodoc: def post_process(*style_args) #:nodoc: return if @queued_for_write[:original].nil? - - instance.run_paperclip_callbacks(:post_process) do - instance.run_paperclip_callbacks(:"#{name}_post_process") do - post_process_styles(*style_args) if !@options[:check_validity_before_processing] || instance.errors.none? + if check_validity? || !@options[:check_validity_before_processing] + instance.run_paperclip_callbacks(:post_process) do + instance.run_paperclip_callbacks(:"#{name}_post_process") do + post_process_styles(*style_args) + end end end end + def check_validity? + instance.run_paperclip_callbacks(:"#{name}_validate") + instance.errors.none? + end + def post_process_styles(*style_args) #:nodoc: if styles.include?(:original) && process_style?(:original, style_args) post_process_style(:original, styles[:original]) diff --git a/lib/paperclip/has_attached_file.rb b/lib/paperclip/has_attached_file.rb index f48dd6992..d278ca064 100644 --- a/lib/paperclip/has_attached_file.rb +++ b/lib/paperclip/has_attached_file.rb @@ -103,7 +103,7 @@ def add_active_record_callbacks def add_paperclip_callbacks @klass.send( :define_paperclip_callbacks, - :post_process, :"#{@name}_post_process" + :post_process, :"#{@name}_post_process", :"#{@name}_validate" ) end diff --git a/lib/paperclip/validators.rb b/lib/paperclip/validators.rb index a7bf1812c..b2aa02097 100644 --- a/lib/paperclip/validators.rb +++ b/lib/paperclip/validators.rb @@ -64,7 +64,7 @@ def validate_before_processing(validator_class, options) def create_validating_before_filter(attribute, validator_class, options) if_clause = options.delete(:if) unless_clause = options.delete(:unless) - send(:"before_#{attribute}_post_process", if: if_clause, unless: unless_clause) do |*_args| + send(:"before_#{attribute}_validate", if: if_clause, unless: unless_clause) do |*_args| validator_class.new(options.dup).validate(self) end end diff --git a/spec/paperclip/attachment_spec.rb b/spec/paperclip/attachment_spec.rb index ff82c286a..bc22a539e 100644 --- a/spec/paperclip/attachment_spec.rb +++ b/spec/paperclip/attachment_spec.rb @@ -63,6 +63,7 @@ Dummy.create!(avatar: File.new(fixture_file("50x50.png"), "rb")) dummy = Dummy.first + allow(dummy.avatar).to receive(:check_validity?).and_return(true) dummy.avatar.reprocess!(:small) expect(dummy.avatar.path(:small)).to exist @@ -868,7 +869,8 @@ def do_after_all; end expect(@dummy).to receive(:do_after_avatar).never expect(@dummy).to receive(:do_before_all).and_return(false) expect(@dummy).to receive(:do_after_all) - expect(Paperclip::Thumbnail).not_to receive(:make) + expect(@dummy).to receive(:check_validity?).and_return(false) + @dummy.check_validity? @dummy.avatar = @file end @@ -877,7 +879,8 @@ def do_after_all; end expect(@dummy).to receive(:do_after_avatar) expect(@dummy).to receive(:do_before_all).and_return(true) expect(@dummy).to receive(:do_after_all) - expect(Paperclip::Thumbnail).not_to receive(:make) + expect(@dummy).to receive(:check_validity?).and_return(false) + @dummy.check_validity? @dummy.avatar = @file end end From bdfbabc87203c08b4a7eb48ff43af02cb9039637 Mon Sep 17 00:00:00 2001 From: Shalini Bhawsingka Date: Thu, 26 Dec 2019 16:31:25 +0530 Subject: [PATCH 2/3] specs for post_process hooks not being called is validations fail --- lib/paperclip/attachment.rb | 2 +- spec/paperclip/attachment_spec.rb | 27 ++++++++++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/lib/paperclip/attachment.rb b/lib/paperclip/attachment.rb index 83ae84747..553af1ed1 100644 --- a/lib/paperclip/attachment.rb +++ b/lib/paperclip/attachment.rb @@ -491,7 +491,7 @@ def process_options(options_type, style) #:nodoc: def post_process(*style_args) #:nodoc: return if @queued_for_write[:original].nil? - if check_validity? || !@options[:check_validity_before_processing] + if !@options[:check_validity_before_processing] || check_validity? instance.run_paperclip_callbacks(:post_process) do instance.run_paperclip_callbacks(:"#{name}_post_process") do post_process_styles(*style_args) diff --git a/spec/paperclip/attachment_spec.rb b/spec/paperclip/attachment_spec.rb index bc22a539e..3fd262cfd 100644 --- a/spec/paperclip/attachment_spec.rb +++ b/spec/paperclip/attachment_spec.rb @@ -63,7 +63,6 @@ Dummy.create!(avatar: File.new(fixture_file("50x50.png"), "rb")) dummy = Dummy.first - allow(dummy.avatar).to receive(:check_validity?).and_return(true) dummy.avatar.reprocess!(:small) expect(dummy.avatar.path(:small)).to exist @@ -869,8 +868,7 @@ def do_after_all; end expect(@dummy).to receive(:do_after_avatar).never expect(@dummy).to receive(:do_before_all).and_return(false) expect(@dummy).to receive(:do_after_all) - expect(@dummy).to receive(:check_validity?).and_return(false) - @dummy.check_validity? + expect(Paperclip::Thumbnail).not_to receive(:make) @dummy.avatar = @file end @@ -879,8 +877,27 @@ def do_after_all; end expect(@dummy).to receive(:do_after_avatar) expect(@dummy).to receive(:do_before_all).and_return(true) expect(@dummy).to receive(:do_after_all) - expect(@dummy).to receive(:check_validity?).and_return(false) - @dummy.check_validity? + expect(Paperclip::Thumbnail).not_to receive(:make) + @dummy.avatar = @file + end + + it "should not call process hooks if validation fails" do + @dummy.validates_presence_of :avatar + expect(@dummy).not_to receive(:do_before_avatar) + expect(@dummy).not_to receive(:do_after_avatar) + expect(@dummy).not_to receive(:do_before_all) + expect(@dummy).not_to receive(:do_after_all) + expect(Paperclip::Thumbnail).not_to receive(:make) + @dummy.avatar = @file + end + + it "should call process hooks if validation would fail but check validity flag is false" do + @dummy.avatar.options[:check_validity_before_processing] = false + expect(@dummy).to receive(:do_before_avatar) + expect(@dummy).to receive(:do_after_avatar) + expect(@dummy).to receive(:do_before_all) + expect(@dummy).to receive(:do_after_all) + expect(Paperclip::Thumbnail).to receive(:make).and_return(@file) @dummy.avatar = @file end end From 945edff9593af6c066489789eb372d28de169a15 Mon Sep 17 00:00:00 2001 From: Shalini Bhawsingka Date: Mon, 30 Dec 2019 10:51:47 +0530 Subject: [PATCH 3/3] adding validation to the dummy in spec to test the behaviour of post_processing --- spec/paperclip/attachment_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/paperclip/attachment_spec.rb b/spec/paperclip/attachment_spec.rb index 3fd262cfd..ff1a7fca7 100644 --- a/spec/paperclip/attachment_spec.rb +++ b/spec/paperclip/attachment_spec.rb @@ -882,7 +882,9 @@ def do_after_all; end end it "should not call process hooks if validation fails" do - @dummy.validates_presence_of :avatar + Dummy.class_eval do + validates_attachment_content_type :avatar, content_type: 'image/jpeg' + end expect(@dummy).not_to receive(:do_before_avatar) expect(@dummy).not_to receive(:do_after_avatar) expect(@dummy).not_to receive(:do_before_all) @@ -892,6 +894,9 @@ def do_after_all; end end it "should call process hooks if validation would fail but check validity flag is false" do + Dummy.class_eval do + validates_attachment_content_type :avatar, content_type: 'image/jpeg' + end @dummy.avatar.options[:check_validity_before_processing] = false expect(@dummy).to receive(:do_before_avatar) expect(@dummy).to receive(:do_after_avatar)