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

Commit

Permalink
Don't write original file if it wasn't reprocessed
Browse files Browse the repository at this point in the history
If `only_process` list is not empty, but it doesn't contain `:original`
style, original file hasn't been reprocessed and it's not needed to
rewrite/reupload it.

[fixes #1993]
[fixes #2046]
[fixes #1804]
  • Loading branch information
sashazykov authored and tute committed Aug 19, 2016
1 parent ce22381 commit 1c7d7f6
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 1 deletion.
5 changes: 4 additions & 1 deletion lib/paperclip/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def self.default_options
# +url+ - a relative URL of the attachment. This is interpolated using +interpolator+
# +path+ - where on the filesystem to store the attachment. This is interpolated using +interpolator+
# +styles+ - a hash of options for processing the attachment. See +has_attached_file+ for the details
# +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is
# +only_process+ - style args to be run through the post-processor. This defaults to the empty list (which is
# a special case that indicates all styles should be processed)
# +default_url+ - a URL for the missing image
# +default_style+ - the style to use when an argument is not specified e.g. #url, #path
Expand Down Expand Up @@ -238,6 +238,9 @@ def dirty?
# the instance's errors and returns false, cancelling the save.
def save
flush_deletes unless @options[:keep_old_files]
if @options[:only_process].any? && !@options[:only_process].include?(:original)
@queued_for_write.except!(:original)
end
flush_writes
@dirty = false
true
Expand Down
52 changes: 52 additions & 0 deletions spec/paperclip/storage/s3_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,58 @@ def counter
end
end

context "An attachment that uses S3 for storage and has styles" do
before do
rebuild_model(
(aws2_add_region).merge(
storage: :s3,
styles: { thumb: ["90x90#", :jpg] },
bucket: "bucket",
s3_credentials: {
"access_key_id" => "12345",
"secret_access_key" => "54321" }
)
)

@file = File.new(fixture_file("5k.png"), "rb")
@dummy = Dummy.new
@dummy.avatar = @file
@dummy.save
end

context "reprocess" do
before do
@object = stub
@dummy.avatar.stubs(:s3_object).with(:original).returns(@object)
@dummy.avatar.stubs(:s3_object).with(:thumb).returns(@object)
@object.stubs(:get).yields(@file.read)
@object.stubs(:exists?).returns(true)
end

it "uploads original" do
@object.expects(:upload_file).with(
anything,
content_type: "image/png",
acl: :"public-read").returns(true)
@object.expects(:upload_file).with(
anything,
content_type: "image/jpeg",
acl: :"public-read").returns(true)
@dummy.avatar.reprocess!
end

it "doesn't upload original" do
@object.expects(:upload_file).with(
anything,
content_type: "image/jpeg",
acl: :"public-read").returns(true)
@dummy.avatar.reprocess!(:thumb)
end
end

after { @file.close }
end

context "An attachment that uses S3 for storage and has spaces in file name" do
before do
rebuild_model(
Expand Down

5 comments on commit 1c7d7f6

@morgoth
Copy link
Contributor

Choose a reason for hiding this comment

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

@tute This commit broke support for passing proc inside only_process option.
I'm not sure if it is officially supported, but here it is

only_process = only_process.call(self) if only_process.respond_to?(:call)

@morgoth
Copy link
Contributor

Choose a reason for hiding this comment

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

@tute
Copy link
Contributor

@tute tute commented on 1c7d7f6 Aug 24, 2016

Choose a reason for hiding this comment

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

aha! Thank you. Why didn't that spec fail?

@morgoth
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's buggy inside save method which is not called in this test

@tute
Copy link
Contributor

@tute tute commented on 1c7d7f6 Aug 24, 2016

Choose a reason for hiding this comment

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

Would you be willing to modify or add a regression test, and fix the bug? Thanks so much!

Please sign in to comment.