Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regard :from_version option on calling #recreate_versions! with args #1879

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
20 changes: 20 additions & 0 deletions README.md
Expand Up @@ -518,6 +518,26 @@ User.find_each do |user|
end
```

### Recreating versions with `:from_version` dependency
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to the wiki, the Readme is already too big for a very specific user case


Imagine you have a version with `:from_version`:

```ruby
class MyUploader < CarrierWave::Uploader::Base

version :thumb do
process resize_to_fill: [280, 280]
end

version :small_thumb, from_version: :thumb do
process resize_to_fill: [20, 20]
end

end
```

In this case, when you call `recreate_versions!(:small_thumb)`, both `:small_thumb` and `:thumb` versions will be recreated.

## Configuring CarrierWave

CarrierWave has a broad range of configuration options, which you can configure,
Expand Down
35 changes: 31 additions & 4 deletions lib/carrierwave/uploader/versions.rb
Expand Up @@ -213,23 +213,49 @@ def url(*args)
# Recreate versions and reprocess them. This can be used to recreate
# versions if their parameters somehow have changed.
#
def recreate_versions!(*versions)
def recreate_versions!(*names)
# Some files could possibly not be stored on the local disk. This
# doesn't play nicely with processing. Make sure that we're only
# processing a cached file
#
# The call to store! will trigger the necessary callbacks to both
# process this version and all sub-versions
if versions.any?
file = sanitized_file if !cached?
store_versions!(file, versions)

if names.any?
file = sanitized_file
# We should somehow filter versions to cache in #cache_versions!.
# Passing arguments through all of those callbacks is difficult, so @versions_to_cache
# instance variable is introduced. It's used only here and in #cache_versions!.
@versions_to_cache = substitute_dependent_with_parents(names)
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid code like this at the current moment. I would find a way to extract this or improve the current method. I know that there might be others places where we are instantiating variables outside of the constructor of the class, but as of now, I see this as a bad smell and should be avoided at all costs.

versions_to_store = (@versions_to_cache + names).uniq
cache!(file)
@versions_to_cache = nil
store_versions!(file, versions_to_store)
else
cache! if !cached?
store!
end
end

private

# To correctly cache dependent version we need to be sure that its parent is cached, too.
# But caching parent version automatically cache its descendents.
# So, while recreating certain versions in #cache_versions! we should skip dependent versions
# as soon as their caching will be automatically started by parent.
def substitute_dependent_with_parents(names)
with_dependencies = []
Copy link
Member

Choose a reason for hiding this comment

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

You can convert this to a inject

versions.each do |name, uploader|
next unless names.include?(name)
if uploader.class.version_options[:from_version]
with_dependencies << uploader.class.version_options[:from_version]
else
with_dependencies << name
end
end
with_dependencies.uniq
end

def assign_parent_cache_id(file)
active_versions.each do |name, uploader|
uploader.parent_cache_id = @cache_id
Expand Down Expand Up @@ -264,6 +290,7 @@ def full_original_filename

def cache_versions!(new_file)
dependent_versions.each do |name, v|
next if @versions_to_cache && !@versions_to_cache.include?(name)
v.send(:cache_id=, @cache_id)
v.cache!(new_file)
end
Expand Down
53 changes: 53 additions & 0 deletions spec/uploader/versions_spec.rb
Expand Up @@ -629,5 +629,58 @@ def upcase
expect(File.read(public_path(@uploader.thumb.to_s))).to eq(File.read(public_path(@uploader.small_thumb.to_s)))
end
end

describe "#recreate_versions!" do
let(:bork_file) { File.open(file_path('bork.txt')) }
let(:original_contents) { File.read(public_path(@uploader.to_s)) }
let(:thumb_contents) { File.read(public_path(@uploader.thumb.to_s)) }
let(:small_thumb_contents) { File.read(public_path(@uploader.small_thumb.to_s)) }

context "when no versions are given" do
it "should process file based on the version" do
@uploader.store!(bork_file)
@uploader.recreate_versions!
expect(thumb_contents).to eq(small_thumb_contents)
end
end

context "when version is given" do
it "should process file based on the version" do
@uploader.store!(bork_file)
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:small_thumb)
expect(small_thumb_contents).to eq(thumb_contents)
expect(small_thumb_contents).not_to eq(original_contents)
end

it "reprocess parent version, too" do
@uploader.store!(bork_file)
FileUtils.rm(@uploader.thumb.path)
@uploader.recreate_versions!(:small_thumb)
end

it "works fine when recreating both dependent and parent versions" do
@uploader.store!(bork_file)
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:small_thumb, :thumb)
expect(File.exists?(public_path(@uploader.small_thumb.to_s))).to be true
expect(File.exists?(public_path(@uploader.thumb.to_s))).to be true

# doesn't depend on arguments order
FileUtils.rm([@uploader.small_thumb.path, @uploader.thumb.path])
@uploader.recreate_versions!(:thumb, :small_thumb)
expect(File.exists?(public_path(@uploader.small_thumb.to_s))).to be true
expect(File.exists?(public_path(@uploader.thumb.to_s))).to be true
end

it "doesn't touch other versions" do
@uploader_class.version(:another)
@uploader.store!(bork_file)
FileUtils.rm(@uploader.another.path)
@uploader.recreate_versions!(:small_thumb)
expect(File.exists?(public_path(@uploader.another.to_s))).to be false
end
end
end
end
end