-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,23 +213,60 @@ 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 | ||
set_versions_to_cache_and_store(names) | ||
cache!(file) | ||
store!(file) | ||
reset_versions_to_cache_and_store | ||
else | ||
cache! if !cached? | ||
store! | ||
end | ||
end | ||
|
||
private | ||
|
||
def set_versions_to_cache_and_store(names) | ||
@versions_to_cache = source_versions_of(names) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those instance variables being set here makes me think that the recreate_versions could be in its own class. As of now, I guess it's good to avoid adding more code here. Do you mind giving it a try @hedgesky ? |
||
@versions_to_store = active_versions_with_names_in(@versions_to_cache + names) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, it's still an instance variable. And now even two of them instead of one 😄 . But introducing them helps to avoid conditions in |
||
end | ||
|
||
def reset_versions_to_cache_and_store | ||
@versions_to_cache, @versions_to_store = nil, nil | ||
end | ||
|
||
def versions_to_cache | ||
@versions_to_cache || dependent_versions | ||
end | ||
|
||
def versions_to_store | ||
@versions_to_store || active_versions | ||
end | ||
|
||
def source_versions_of(requested_names) | ||
versions.inject([]) do |sources, (name, uploader)| | ||
next sources unless requested_names.include?(name) | ||
next sources unless source_name = uploader.class.version_options[:from_version] | ||
|
||
sources << [source_name, versions[source_name]] | ||
end.uniq | ||
end | ||
|
||
def active_versions_with_names_in(names) | ||
active_versions.select do |pretendent_name, uploader| | ||
names.include?(pretendent_name) | ||
end | ||
end | ||
|
||
def assign_parent_cache_id(file) | ||
active_versions.each do |name, uploader| | ||
uploader.parent_cache_id = @cache_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now versions to store is part of object's state, so we don't need extra argument here. |
||
|
@@ -263,19 +300,14 @@ def full_original_filename | |
end | ||
|
||
def cache_versions!(new_file) | ||
dependent_versions.each do |name, v| | ||
versions_to_cache.each do |name, v| | ||
v.send(:cache_id=, @cache_id) | ||
v.cache!(new_file) | ||
end | ||
end | ||
|
||
def store_versions!(new_file, versions=nil) | ||
if versions | ||
active = Hash[active_versions] | ||
versions.each { |v| active[v].try(:store!, new_file) } unless active.empty? | ||
else | ||
active_versions.each { |name, v| v.store!(new_file) } | ||
end | ||
def store_versions!(new_file) | ||
versions_to_store.each { |name, v| v.store!(new_file) } | ||
end | ||
|
||
def remove_versions! | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we avoid duplication and possible errors by using similar approach to cache/store in both branches of a condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We noticed this change causes an AWS S3-backed store to trigger callbacks and send DELETES. Is this expected behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems related to #2385
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the reason -- this change caused the cache storage to match the uploader:
629afec