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

Centralize Abstraction for Thumbnail Generation #1343

Closed
mbarnett opened this issue Oct 29, 2019 · 4 comments
Closed

Centralize Abstraction for Thumbnail Generation #1343

mbarnett opened this issue Oct 29, 2019 · 4 comments

Comments

@mbarnett
Copy link
Contributor

Right now we call ActiveStorage's variant and preview methods in a variety of places , with inconsistent error handling. This is leading to issues where an invalid or corrupted JPEG or other unknown format issue can lead to a page or search 500ing when it attempts to display a thumbnail for the file.

Arguably, calling into ActiveStorage for variants through the items and theses and communitiers and drafts that own these files is a law of demeter violation, and it's that anti-pattern that's at the root of our inability to correctly handle these errors. We should set up a single "clearing house" method for thumbnails across models that can abstract away the variant generation and handle errors consistently and easily.

If you're going to work on this please grab @mbarnett so that we can whiteboard out approaches and come up with a good, scalable solution that will serve us well down the road.

@pgwillia
Copy link
Member

Some related tickets #1129 and #898

@pgwillia
Copy link
Member

# compatibility with item thumbnail API
def thumbnail_path(args = { resize: '100x100', auto_orient: true })
return nil if logo_attachment.blank?
Rails.application.routes.url_helpers.rails_representation_path(logo_attachment.variant(args).processed)
end

# this method came with us from LockedLdpObject, and it'll keep this name until it gets refactored
# rubocop:disable Naming/AccessorMethodName
def set_thumbnail(attachment)
self.logo_id = attachment.id
save!
end
# rubocop:enable Naming/AccessorMethodName
def thumbnail_path(args = { resize: '100x100', auto_orient: true })
logo = files.find_by(id: logo_id)
return nil if logo.blank?
Rails.application.routes.url_helpers.rails_representation_path(logo.variant(args).processed)
rescue ActiveStorage::InvariableError
begin
Rails.application.routes.url_helpers.rails_representation_path(logo.preview(args).processed)
rescue ActiveStorage::UnpreviewableError
nil
end
end

# compatibility with the thumbnail API used in Items/Theses and Communities
def thumbnail_path(args = { resize: '100x100', auto_orient: true })
return nil unless thumbnail.present? && thumbnail.blob.present?
Rails.application.routes.url_helpers.rails_representation_path(thumbnail.variant(args).processed)
rescue ActiveStorage::InvariableError
begin
Rails.application.routes.url_helpers.rails_representation_path(thumbnail.preview(args).processed)
rescue ActiveStorage::UnpreviewableError
nil
end
end

<%# NOTE: This is superficially similar to the logic in DraftItem#thumbnail, but this file is run against
every file attached to an item via a JS callback rendering files/update_files_list.js.erb calling _files_list.html.erb
whereas DraftItem#thumbnail specifically only deals with rendering the designated file for representing it as a
thumbnail and is used on various other pages, like profiles, which use the application/_thumbnail partial.%>
<% thumbnail = rails_representation_path(file.variant(resize: '100x100', auto_orient: true).processed) %>
<%= safe_thumbnail_tag(thumbnail, alt: '', title: file.filename, size: '100x100') %>
<% rescue ActiveStorage::InvariableError %>
<% begin %>
<% thumbnail = rails_representation_path(file.preview(resize: '100x100', auto_orient: true).processed) %>
<%= safe_thumbnail_tag(thumbnail, alt: '', title: file.filename, size: '100x100') %>
<% rescue ActiveStorage::UnpreviewableError %>
<div class="text-muted text-center img-thumbnail p-3">
<%= icon('far', file_icon(file.content_type), class: 'fa-5x') %>
<span class="sr-only"><%= file.filename %></span>
</div>
<% end %>

<%# NOTE: This is superficially similar to the logic in DraftItem#thumbnail, but this file is run against
every file attached to an item via a JS callback rendering files/update_files_list.js.erb calling _files_list.html.erb
whereas DraftItem#thumbnail specifically only deals with rendering the designated file for representing it as a
thumbnail and is used on various other pages, like profiles, which use the application/_thumbnail partial.%>
<% thumbnail = rails_representation_path(file.variant(resize: '100x100', auto_orient: true).processed) %>
<%= safe_thumbnail_tag(thumbnail, alt: '', title: file.filename, size: '100x100') %>
<% rescue ActiveStorage::InvariableError %>
<% begin %>
<% thumbnail = rails_representation_path(file.preview(resize: '100x100', auto_orient: true).processed) %>
<%= safe_thumbnail_tag(thumbnail, alt: '', title: file.filename, size: '100x100') %>
<% rescue ActiveStorage::UnpreviewableError %>
<div class="text-muted text-center img-thumbnail p-3">
<%= icon('far', file_icon(file.content_type), class: 'fa-5x') %>
<span class="sr-only"><%= file.filename %></span>
</div>
<% end %>

@pgwillia
Copy link
Member

Here's the rough approach:

  • Community, Depositable and DraftProperties all get an instance method #logo
  • ImagesHelper gets a new method #logo_path which does the error_handling and rollbar reporting
  • views call logo_path(item.logo)

@pgwillia
Copy link
Member

pgwillia commented Jun 4, 2020

Merged into postmigration branch.

@pgwillia pgwillia closed this as completed Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants