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.

Sign up for GitHub

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

Jump to bottom

lately resolve active versions when retrieving #2669

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

lately resolve active versions when retrieving #2669

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/carrierwave/uploader/versions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def version(name, options = {}, &block)

class_eval <<-RUBY, __FILE__, __LINE__ + 1
def #{name}
versions[:#{name}]
version_exists?(:#{name}) ? versions[:#{name}] : versions[:#{name}].class.new(model, mounted_as)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lately resolving the condition here. Which would make it more consistent with the current state (active or not) of the version (which fixes #2148 too).

I think it is better to return nil for non-active versions, but returning a blank uploader for backwards compatibility.

end
RUBY

Expand Down Expand Up @@ -325,11 +325,11 @@ def remove_versions!
end

def retrieve_versions_from_cache!(cache_name)
active_versions.each { |name, v| v.retrieve_from_cache!(cache_name) }
versions.each { |name, v| v.retrieve_from_cache!(cache_name) }
end

def retrieve_versions_from_store!(identifier)
active_versions.each { |name, v| v.retrieve_from_store!(identifier) }
versions.each { |name, v| v.retrieve_from_store!(identifier) }
end

Comment on lines 327 to 334
Copy link
Contributor Author

@rajyan rajyan Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to evaluate the condition here, like before the changes in #1351 .
We can keep all versions retrieved internally, evaluate the condition and return them only when users accesses.

end # Versions
Expand Down
28 changes: 28 additions & 0 deletions spec/uploader/versions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,13 @@ def move_to_cache
expect(@uploader.thumb).to be_present
expect(@uploader.preview).to be_blank
end

it "should evaluate the condition even the version is not called" do
@uploader_class.version(:preview, if: lambda{|record, args| record.false?(args[:file])})
expect(@uploader).to receive(:false?).at_least(:once).and_return(false)
@uploader.store!(@file)
expect(@uploader.thumb).to be_present
end
end

context "when there is an 'unless' option" do
Expand Down Expand Up @@ -483,6 +490,13 @@ def move_to_cache
expect(@uploader.thumb).to be_present
expect(@uploader.preview).to be_present
end

it "should evaluate the condition even the version is not called" do
@uploader_class.version(:preview, unless: lambda{|record, args| record.false?(args[:file])})
expect(@uploader).to receive(:false?).at_least(:once).and_return(false)
@uploader.store!(@file)
expect(@uploader.thumb).to be_present
end
end

it "should not cache file twice when store! called with a file" do
Expand Down Expand Up @@ -739,6 +753,13 @@ def shorten
expect(@uploader.thumb).to be_present
expect(@uploader.preview).to be_blank
end

it "should not evaluate the condition until version is called" do
@uploader_class.version(:preview, if: :false?)
expect(@uploader).not_to receive(:false?)
@uploader.retrieve_from_store!(@file)
expect(@uploader.thumb).to be_present
end
end

context "when there is an 'unless' option" do
Expand All @@ -757,6 +778,13 @@ def shorten
expect(@uploader.thumb).to be_present
expect(@uploader.preview).to be_present
end

it "should not evaluate the condition until version is called" do
@uploader_class.version(:preview, unless: :false?)
expect(@uploader).not_to receive(:false?)
@uploader.retrieve_from_store!(@file)
expect(@uploader.thumb).to be_present
end
end
end
end
Expand Down