-
Notifications
You must be signed in to change notification settings - Fork 27
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
[#161102] Adds active_storage_for_images_only
feature flag
#3348
Changes from 4 commits
310fa4c
a2b51e2
6c45c9b
25c6f3c
d12a30e
61024a5
235d3b8
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 |
---|---|---|
|
@@ -9,7 +9,7 @@ module ActiveStorageFile | |
has_one_attached :file | ||
|
||
def assign_attributes(new_attributes) | ||
if SettingsHelper.feature_on?(:active_storage) | ||
if SettingsHelper.active_storage_enabled? | ||
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. Do we need this feature flag here? I can't recall if we need it for some reason or perhaps it was left over from a previous iteration of the code, before we separated things into modules 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. I also can't recall. It passes CI with it removed though, so I suppose it's fine to leave it out. I recall this method causing a lot of problems, so likely extra stuff was left in it while working up to a solution. |
||
# Ensure the file name and file_type are set first when attaching a StringIO | ||
# see #file= method below | ||
assign_first = new_attributes.extract!(:name, :file_type) | ||
|
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.
I think I'm leaning towards letting each feature flag do its own thing - what's your thinking about intertwining here?
If we keep this logic I'd vote for renaming to
active_storage_for_images_enabled?
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.
Because of how the flags are named, it seemed like
active_storage
could/would be a superset ofactive_storage_for_images_only
. Also doing it this way would mean no change in OSU's settings.yml. Neither of these may be interesting enough reasons, don't have a strong opinion.