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

centralize abstraction for thumbnail generation #1681

Merged
merged 2 commits into from
Jun 4, 2020
Merged

centralize abstraction for thumbnail generation #1681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5270e48
centralize abstration for thumbnail generation
pgwillia Jun 2, 2020
6fa8366
simplify tests and skip pdf if dependency not found
pgwillia Jun 3, 2020
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ and releases in Jupiter project adheres to [Semantic Versioning](http://semver.o
- Update UAL Logo [#1616](https://github.com/ualbertalib/jupiter/issues/1616)
- Refactor `inactive` draft cleanup rake task to be sidekiq cron job [#1611](https://github.com/ualbertalib/jupiter/issues/1611)
- Feature Image on Item show page need to be centered align within column [#1405](https://github.com/ualbertalib/jupiter/issues/1405)

- Centralize Abstraction for Thumbnail Generation [#1343](https://github.com/ualbertalib/jupiter/issues/1343)

### Fixed
- failing tests [#1376](https://github.com/ualbertalib/jupiter/issues/1376)
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
- **PostgreSQL**
- **Redis**
- **Solr**
- **ImageMagick**
- **ImageMagick, Poppler, FFMpeg**
- **Node.js** 10.13.0+
- **Yarn**

Expand Down
25 changes: 24 additions & 1 deletion app/helpers/page_layout_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,34 @@ def page_description(description = nil)
end
# rubocop:enable Rails/HelperInstanceVariable

def thumbnail_path(logo, args = { resize: '100x100', auto_orient: true })
return nil if logo.blank?

# images have variants
Rails.application.routes.url_helpers.rails_representation_path(logo.variant(args).processed)
rescue ActiveStorage::InvariableError
begin
# pdfs and video have previews
Rails.application.routes.url_helpers.rails_representation_path(logo.preview(args).processed)
# ActiveStorage::UnpreviewableError and sometimes MiniMagick::Error gets thrown here
rescue StandardError => e
logger.warn("#{logo.record_type} with id: #{logo.record_id} and thumnail #{logo.name} \
threw an error after ActiveStorage::InvariableError.")
Rollbar.warn("#{logo.record_type} with id: #{logo.record_id} and thumnail #{logo.name} \
threw an error after ActiveStorage::InvariableError.", e)
nil
end
rescue StandardError => e
logger.warn("#{logo.record_type} with id: #{logo.record_id} and thumnail #{logo.name} threw an error.")
Rollbar.warn("#{logo.record_type} with id: #{logo.record_id} and thumnail #{logo.name} threw an error.", e)
nil
end

# rubocop:disable Rails/HelperInstanceVariable
def page_image_url
default_url = asset_pack_url('media/images/era-logo.png')
# We only have images on community and item/thesis show pages
image_path = @community&.thumbnail_path || @item&.thumbnail_path
image_path = thumbnail_path(@community&.thumbnail_file) || thumbnail_path(@item&.thumbnail_file)

image_path ? request.base_url + image_path : default_url
end
Expand Down
7 changes: 0 additions & 7 deletions app/models/community.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,6 @@ def remove_logo=(val)
logo_attachment.purge
end

# 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

def thumbnail_file
logo.attachment
end
Expand Down
13 changes: 0 additions & 13 deletions app/models/concerns/draft_properties.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,6 @@ def thumbnail
files.first
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

def thumbnail_file
thumbnail if thumbnail.present? && thumbnail.blob.present?
end
Expand Down
13 changes: 0 additions & 13 deletions app/models/jupiter_core/depositable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,6 @@ def set_thumbnail(attachment)
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

def thumbnail_file
files.find_by(id: logo_id)
end
Expand Down
24 changes: 7 additions & 17 deletions app/views/admin/theses/draft/_thumbnail.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,8 @@
<% begin %>
<%# 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 %>
<% if thumbnail_path(file).present? %>
<%= safe_thumbnail_tag(thumbnail_path(file), alt: '', title: file.filename, size: '100x100') %>
<% else %>
<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 %>
4 changes: 2 additions & 2 deletions app/views/application/_feature_image.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% if policy(object).thumbnail? %>
<% if object.thumbnail_path.present? %>
<%= safe_thumbnail_tag(object.thumbnail_path(resize: '350x350', auto_orient: true), alt: '', size: '350x350') %>
<% if thumbnail_path(object&.thumbnail_file).present? %>
<%= safe_thumbnail_tag(thumbnail_path(object&.thumbnail_file, resize: '350x350', auto_orient: true), alt: '', size: '350x350') %>
<% else %>
<div class="d-flex justify-content-center align-items-center img-thumbnail p-5">
<%= icon('far', file_icon(object.thumbnail_file&.content_type), class: 'fa-5x text-muted') %>
Expand Down
4 changes: 2 additions & 2 deletions app/views/application/_thumbnail.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% if policy(object).thumbnail? %>
<% if object.thumbnail_path.present? %>
<%= safe_thumbnail_tag(object.thumbnail_path, alt: '', title: object.title, size: '100x100') %>
<% if thumbnail_path(object&.thumbnail_file).present? %>
<%= safe_thumbnail_tag(thumbnail_path(object&.thumbnail_file), alt: '', title: object.title, size: '100x100') %>
<% else %>
<% if object.is_a? Community %>
<%= image_pack_tag('era-logo-without-text.png', class: 'j-thumbnail img-thumbnail', alt: '', title: object.title, size: '100x100') %>
Expand Down
24 changes: 7 additions & 17 deletions app/views/items/draft/_thumbnail.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,8 @@
<% begin %>
<%# 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 %>
<% if thumbnail_path(file).present? %>
<%= safe_thumbnail_tag(thumbnail_path(file), alt: '', title: file.filename, size: '100x100') %>
<% else %>
<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 %>
5 changes: 5 additions & 0 deletions test/fixtures/active_storage/attachments.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
logo:
name: 'files'
record_type: 'Item'
record_id: <%= ActiveRecord::FixtureSet.identify(:fancy) %>
blob_id: <%= ActiveRecord::FixtureSet.identify(:jpeg) %>
6 changes: 6 additions & 0 deletions test/fixtures/active_storage/blobs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
jpeg:
key: '12345'
filename: 'image-sample.jpeg'
content_type: 'image/jpeg'
byte_size: 12086
checksum: "GxpIjJsC4KnRoBKNjWnkJA=="
88 changes: 86 additions & 2 deletions test/helpers/page_layout_helper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ def base_url

# page_title

# page_title
test 'should return the page title when given one' do
assert_equal t('site_name'), page_title(t('site_name'))
end
Expand Down Expand Up @@ -93,9 +92,94 @@ def base_url
@community.logo.attach io: File.open(file_fixture('image-sample.jpeg')),
filename: 'image-sample.jpeg', content_type: 'image/jpeg'

assert_equal page_image_url, request.base_url + @community.thumbnail_path
assert_equal page_image_url, request.base_url + thumbnail_path(@community.thumbnail_file)
end

# thumbnail_path

test 'thumbnail_path should return preview for pdf (Invariable but Previewable)' do
Copy link
Contributor

@murny murny Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is installing poppler easy on CI/Dev machines? If not then maybe we should keep it as an "optional" dependency (if not installed we just fallback to file icon image which already happens as it returns nil)? Maybe this test only runs if Poppler dependency is defined 🤔 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pretty easy in Ubuntu sudo apt-get install poppler-utils I'll see if I can get this to work like Clamby.

jupiter/app/models/concerns/draft_properties.rb

Line 153 in be07bda

return unless defined?(Clamby)

Copy link
Member Author

Choose a reason for hiding this comment

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

jupiter/.github/workflows/push.yml

Line 6 in b667a36

runs-on: ubuntu-latest

https://github.com/actions/virtual-environments/blob/master/images/linux/Ubuntu2004-README.md

Has ImageMagick installed but not the PDF dependencies.

community = communities(:books)
collection = collections(:fantasy_books)
@item = Item.new.tap do |uo|
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably clean up some of this by using an Item fixture. E.g:

    @item = items(:fancy)
    File.open(file_fixture('pdf-sample.pdf'), 'r') do |file|
      @item.add_and_ingest_files([file])
    end

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was hoping that would work too. Maybe I'm using it wrong

test 'thumbnail_path should return preview for pdf (Invariable but Previewable)' do
    @item = items(:fancy)
    File.open(file_fixture('pdf-sample.pdf'), 'r') do |file|
      @item.add_and_ingest_files([file])
    end

    logo = @item.files.first
    expected = Rails.application.routes.url_helpers.rails_representation_path(
      logo.preview(resize: '100x100', auto_orient: true).processed
    )
    assert_equal expected, thumbnail_path(logo)
  end

gives me

Error:
PageLayoutHelperTest#test_thumbnail_path_should_return_preview_for_pdf_(Invariable_but_Previewable):
NoMethodError: undefined method `preview' for nil:NilClass
    test/helpers/page_layout_helper_test.rb:108:in `block in <class:PageLayoutHelperTest>'

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, my guess is you might have to .reload the item?

Take a peak at downloads controller test. Heres the relevant snippet:

   item = items(:fancy)

    File.open(file_fixture('text-sample.txt'), 'r') do |file|
      item.add_and_ingest_files([file])
    end

    @file = item.reload.files.first

Copy link
Member Author

Choose a reason for hiding this comment

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

That was exactly what I was missing. Thanks!

uo.title = 'Fantastic item'
uo.owner_id = users(:admin).id
uo.creators = ['Joe Blow', 'Smokey Chantilly-Tiffany', 'Céline Marie Claudette Dion']
uo.visibility = JupiterCore::VISIBILITY_PUBLIC
uo.created = '1999-09-09'
uo.languages = [CONTROLLED_VOCABULARIES[:language].english]
uo.license = CONTROLLED_VOCABULARIES[:license].attribution_4_0_international
uo.item_type = CONTROLLED_VOCABULARIES[:item_type].article
uo.publication_status = [CONTROLLED_VOCABULARIES[:publication_status].draft,
CONTROLLED_VOCABULARIES[:publication_status].submitted]
uo.subject = ['Items']
uo.add_to_path(community.id, collection.id)
uo.save!
end
Sidekiq::Testing.inline! do
# Attach a file to the item so that it provide preview
File.open(file_fixture('pdf-sample.pdf'), 'r') do |file|
@item.add_and_ingest_files([file])
end
end

logo = @item.files.first
expected = Rails.application.routes.url_helpers.rails_representation_path(
logo.preview(resize: '100x100', auto_orient: true).processed
)
assert_equal expected, thumbnail_path(logo)
end

test 'thumbnail_path should return preview for image (Variable)' do
community = communities(:books)
collection = collections(:fantasy_books)
@item = Item.new.tap do |uo|
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, just use an item fixture and add an attachment to it

(dont even think you need the Sidekiq::Testing.inline! anymore. Can look at admin items controller and else where for examples)

    @item = items(:fancy)
    File.open(file_fixture('text-sample.txt'), 'r') do |file|
      @item.add_and_ingest_files([file])
    end

uo.title = 'Fantastic item'
uo.owner_id = users(:admin).id
uo.creators = ['Joe Blow', 'Smokey Chantilly-Tiffany', 'Céline Marie Claudette Dion']
uo.visibility = JupiterCore::VISIBILITY_PUBLIC
uo.created = '1999-09-09'
uo.languages = [CONTROLLED_VOCABULARIES[:language].english]
uo.license = CONTROLLED_VOCABULARIES[:license].attribution_4_0_international
uo.item_type = CONTROLLED_VOCABULARIES[:item_type].article
uo.publication_status = [CONTROLLED_VOCABULARIES[:publication_status].draft,
CONTROLLED_VOCABULARIES[:publication_status].submitted]
uo.subject = ['Items']
uo.add_to_path(community.id, collection.id)
uo.save!
end
Sidekiq::Testing.inline! do
# Attach a file to the item so that it can provide variant
File.open(file_fixture('image-sample.jpeg'), 'r') do |file|
@item.add_and_ingest_files([file])
end
end

logo = @item.files.first
expected = Rails.application.routes.url_helpers.rails_representation_path(
logo.variant(resize: '100x100', auto_orient: true).processed
)
assert_equal expected, thumbnail_path(logo)
end

test 'thumbnail_path should provide nil if no thumbnail is possible (StandardError on variable)' do
logo = active_storage_attachments(:logo)
logo.define_singleton_method(:variant) { |_| throw StandardError }

assert_nil thumbnail_path(logo)
# TODO: assert that the logger.warn was written
end

test 'thumbnail_path should return nil if both the variant and preview fail' do
logo = active_storage_attachments(:logo)
logo.define_singleton_method(:variant) { |_| throw ActiveStorage::InvariableError }
logo.define_singleton_method(:preview) { |_| throw StandardError }

assert_nil thumbnail_path(logo)
# TODO: assert that the logger.warn was written
end

# canonical_href

test 'canonical_href is returning the correct canoncial url' do
assert_equal Jupiter::PRODUCTION_URL, canonical_href(nil)
assert_equal Jupiter::PRODUCTION_URL, canonical_href('/')
Expand Down