-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] #17 Migrate from paperclip to active storage #137
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
Conversation
| belongs_to :user | ||
|
|
||
| scope :liaisons, -> { where(position: 1) } | ||
| scope :liaisons, -> { where(position: "liaison") } |
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.
This was changed for readability with @marc during debugging an enum issue with the rails 8.1 upgrade.
| if @user.update_with_password(pass_params) | ||
| sign_in(@user, :bypass => true) | ||
| flash[:alert] = 'Your Password was updated.' | ||
| bypass_sign_in(@user) |
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.
Method change with rails/devise upgrade.
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.
These method should probably be consolidated once the storage attachments have been migrated. The stakeholders only want to upload a single file that can display correctly in different places. legacy should be removed as it was used from a previous data transfer but we need to check what files are actually affected by that.
| feature_flag_variables = File.join(Rails.root, 'config', 'feature_flag_variables.rb') | ||
| load(feature_flag_variables) if File.exist?(feature_flag_variables) |
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.
This was a straight forward way to load my ACTIVE_STORAGE env variable for dev and testing.
I'm not sure if/how we are loading other env variables for dev/test/staging but if we want to set ACTIVE_STORAGE somewhere else that we already use, this file change could be removed.
|
I ran a query on the database so see how many attachments we are actually dealing with. If I loaded the correct file(s) for the db this the what I came up with: |
|
@jmilljr24 is this PR ready to be reviewed? |
|
|
||
| has_attached_file :file | ||
| validates_attachment :file, content_type: { content_type: %w(application/pdf application/msword image/gif image/jpg image/jpeg image/png) } | ||
| ACCEPTED_CONTENT_TYPES = %w[application/pdf application/msword image/gif image/jpeg image/png].freeze |
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.
might we want to expand the list?
here's what i ended up using in another application to support additional document spreadsheet types and avoid upload errors users were having.
for workshops, there's a need to support uploading a video, so i added video types in here too
UPLOAD_TYPES = [
Documents
'application/pdf',
'application/msword',
'application/vnd.ms-word',
'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
'application/msexcel',
'application/vnd.ms-excel',
'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
'application/mspowerpoint',
'application/vnd.ms-powerpoint',
'application/vnd.openxmlformats-officedocument.presentationml.presentation',
'text/plain',
Videos
'video/mp4',
'video/quicktime',
'video/x-msvideo',
'video/x-ms-wmv',
'video/mpeg',
'video/webm',
'video/ogg',
Images (regex)
/\Aimage/.*\Z/
]
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.
That definitely makes sense! I was trying to keep this PR as close to a one for one change, at least until all the files are migrated to avoid any confusion on what actually needs to be changes vs. improvements.
Do you think we could open a draft issue for your suggested changes until the migration is complete?
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.
YES, love this separation of concerns
| :url => "/ckeditor_assets/pictures/:id/:style_:basename.:extension", | ||
| :path => "/ckeditor_assets/pictures/:id/:style_:basename.:extension", | ||
| :styles => { :content => '800>', :thumb => '118x100#' } | ||
| ACCEPTED_CONTENT_TYPES = ["image/jpeg", "image/png"].freeze |
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 we should consider changing this to:
`/\Aimage/.*\Z/``
what do you think?
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.
If these images are going to be displayed on the website, it probably makes sense to limit the content_type to web-safe image types.
app/models/ckeditor/picture.rb
Outdated
| has_attached_file :data, | ||
| url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", | ||
| path: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", | ||
| styles: {content: "800>", thumb: "118x100#"} |
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.
is there no need for listing styles with activestorage?
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.
It's not exactly the same. Active Storage uses variants. You would use something like <%= image_tag user.avatar.variant(resize_to_limit: [100, 100]) %> in a view.
From the rails guides you can also configure specific variants per attachment by calling the variant method on yielded attachable object:
class User < ApplicationRecord
has_one_attached :avatar do |attachable|
attachable.variant :thumb, resize_to_limit: [100, 100]
end
end
<%= image_tag user.avatar.variant(:thumb) %>
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 it would make sense to wait and see where we display these images decide on the variant size for each specific view.
| 'workshop_default.png' | ||
| ) | ||
| validates_attachment :file, content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] } | ||
| ACCEPTED_CONTENT_TYPES = ["image/jpeg", "image/png", "image/gif"].freeze |
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.
same comment -- how about this instead?
/\Aimage/.*\Z/
| belongs_to :workshop_log, optional: true | ||
|
|
||
| has_attached_file :file | ||
| FORM_FILE_CONTENT_TYPES = ["image/jpeg", "image/png", |
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.
did you remove image/jpg on purpose?
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.
Yes! jpg is not a valid MIME type 😃
app/models/report.rb
Outdated
| validates :form_file, content_type: FORM_FILE_CONTENT_TYPES | ||
| else | ||
| has_attached_file :form_file | ||
| before_save :set_has_attachament |
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.
the methoddef set_has_attachament should live in this portion of the conditional as well, right?
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.
Ah yes, I need to alter that method because Active Storage uses a different method to check if attachment is attached.
Do you know where in the UI I can do this flow? I can write up the code change but I remember now, I left it as a TODO because I couldn't find where in the UI this was used.
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'm still digging around to figure out the UI for Forms myself (and thus :form_file)
Likely these were only edited within RailsAdmin. I'm working through rebuilding native CRUD for important models but since this wasn't on the stakeholder list, we don't have UI for it afaik.
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.
Ok. I will add the check when active storage is being used. I will add a TODO to remind us to verify it works once available in the UI. All of my conditional checks for active storage will be revisited to rip out the the old stuff anyways so it shouldn't go unnoticed.
|
|
@maebeale It's about as far as I can take it without input from @johnpaulashenfelter on the actual migration of the files which he planned to do. I will keep up on merge conflicts/new changes to the main branch and address your feedback in the mean time. |
It looks like |
|
@jmilljr24 would you rebase this branch so @johnpaulashenfelter can give it a look soon? |
Did we come to a consensus on if we want to use a feature flag, or just go straight to active storage for staging? If we don't want the feature flag, I'd rather close this PR and start fresh. |
Some methods for params were commented out for an unknown reason. This add's them back and the update method works properly.
setup initial change for user avatar
4e02a33 to
5fb0b7f
Compare
What is the goal of this PR and why is this important?
This is the initial change from Paperclip to ActiveStorage.
How did you approach the change?
A temporary feature flag has been added
ENV['ACTIVE_STORAGE]before the full change over is deployed to production. This change allows using active storage attachments and associated methods if the flag is set while in development and staging. This change does not yet add the migration of the data.Anything else to add?
I could not do some user testing in multiple areas of the app because of preexisting issues or other blockers.
The following areas have been verified to work:
Not verified:
Additional considerations - There is code related to a legacy import. There a locations were attachments are handling depending on the legacy status or attachment variants that needs further investigation. The stakeholder stated for attachments with thumbnail and header, the thumbnail attachment is the priority. Ideally they only want to upload a single image for a workshop and have the image display correctly in different areas that show the workshop.