Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

@WIP Fixed bug where processing occurs on invalid objects #2594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

@WIP Fixed bug where processing occurs on invalid objects #2594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/paperclip/attachment.rb
View file
Open in desktop
Expand Up @@ -109,8 +109,6 @@ def assign(uploaded_file)
nil
else
assign_attributes
post_process_file
reset_file_if_original_reprocessed
end
else
nil
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 39 additions & 12 deletions spec/paperclip/attachment_processing_spec.rb
View file
Open in desktop
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

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
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

file = File.new(fixture_file("5k.png"))
Dummy.validates_attachment :avatar, content_type: {content_type: "image/png"}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Metrics/LineLength: Line is too long. [83/80]
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Metrics/LineLength: Line is too long. [83/80]

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
69 changes: 66 additions & 3 deletions spec/paperclip/attachment_spec.rb
View file
Open in desktop
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [84/80]

expected_params = @style_params[:once].merge({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

style: :once,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

processors: [:thumbnail, :test],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/SymbolArray: Use %i or %I for an array of symbols.

whiny: true,
convert_options: "",
source_file_options: ""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.


@dummy.avatar = @file

expect(Paperclip::Thumbnail).not_to have_received(:make).with(anything, expected_params, anything)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [106/80]

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [99/80]

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
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [99/80]

before do
rebuild_class styles: { something: "100x100#" }
Dummy.class_eval do
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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

Expand Down