From 5270e48dcd7f6141a367474b3dcd6b4ab6457568 Mon Sep 17 00:00:00 2001 From: Tricia Jenkins Date: Tue, 2 Jun 2020 16:10:59 -0600 Subject: [PATCH 1/2] centralize abstration for thumbnail generation Similar logic appeared in Depositable, DraftProperties, Community and the thumbnail/feature_image partials. By refactoring the thumbnail_path into a helper and being able to call thumbnail_file on each instance we can centralize the logic and have each class only deal with the things that concern them. In investigating this I learned a few new things. ImageMagick is required for variant on images, but a pdf utility like Poppler is required to preview PDFs and ffmpeg is required to preview video. All three a requirements for our thumbnails to work correctly. I tried a few different things in order to test the helper. I tried creating a dummy @logo which implemented only the surface area that the thumbnail_path touched. With this approach I got stuck on the rails_representation_path with the pdf and images case. Then I tried creating fixtures (turns out that active_storage fixtures are supported out of the box) for these objects. I ran into a similar road block because the fixtures wouldn't respond to variant and preview. Finally I tried a hybrid approach using real objects for the pdf and image and fixtures for the error cases. Success! I would have liked to also test that a useful warning was logged in the error cases but couldn't find a approach that worked for me short of using the minitest-logger gem. Maybe that's the best way forward. --- CHANGELOG.md | 2 +- README.md | 2 +- app/helpers/page_layout_helper.rb | 25 +++++- app/models/community.rb | 7 -- app/models/concerns/draft_properties.rb | 13 --- app/models/jupiter_core/depositable.rb | 13 --- .../admin/theses/draft/_thumbnail.html.erb | 24 ++--- app/views/application/_feature_image.html.erb | 4 +- app/views/application/_thumbnail.html.erb | 4 +- app/views/items/draft/_thumbnail.html.erb | 24 ++--- test/fixtures/active_storage/attachments.yml | 5 ++ test/fixtures/active_storage/blobs.yml | 6 ++ test/helpers/page_layout_helper_test.rb | 88 ++++++++++++++++++- 13 files changed, 141 insertions(+), 76 deletions(-) create mode 100644 test/fixtures/active_storage/attachments.yml create mode 100644 test/fixtures/active_storage/blobs.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dd618e8c..34be90227 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/README.md b/README.md index 29cc243fb..e18f68631 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ - **PostgreSQL** - **Redis** - **Solr** -- **ImageMagick** +- **ImageMagick, Poppler, FFMpeg** - **Node.js** 10.13.0+ - **Yarn** diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index 729234a82..f045aea27 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -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 diff --git a/app/models/community.rb b/app/models/community.rb index 56eee69c0..a7b2c30a7 100644 --- a/app/models/community.rb +++ b/app/models/community.rb @@ -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 diff --git a/app/models/concerns/draft_properties.rb b/app/models/concerns/draft_properties.rb index 741bc697e..5f67c5694 100644 --- a/app/models/concerns/draft_properties.rb +++ b/app/models/concerns/draft_properties.rb @@ -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 diff --git a/app/models/jupiter_core/depositable.rb b/app/models/jupiter_core/depositable.rb index 6ccdaa992..24b97342e 100644 --- a/app/models/jupiter_core/depositable.rb +++ b/app/models/jupiter_core/depositable.rb @@ -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 diff --git a/app/views/admin/theses/draft/_thumbnail.html.erb b/app/views/admin/theses/draft/_thumbnail.html.erb index e94b3741d..6d7b27857 100644 --- a/app/views/admin/theses/draft/_thumbnail.html.erb +++ b/app/views/admin/theses/draft/_thumbnail.html.erb @@ -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 %> -
- <%= icon('far', file_icon(file.content_type), class: 'fa-5x') %> - <%= file.filename %> -
- <% end %> +<% if thumbnail_path(file).present? %> + <%= safe_thumbnail_tag(thumbnail_path(file), alt: '', title: file.filename, size: '100x100') %> +<% else %> +
+ <%= icon('far', file_icon(file.content_type), class: 'fa-5x') %> + <%= file.filename %> +
<% end %> diff --git a/app/views/application/_feature_image.html.erb b/app/views/application/_feature_image.html.erb index dd0a3213f..2ec3bea20 100644 --- a/app/views/application/_feature_image.html.erb +++ b/app/views/application/_feature_image.html.erb @@ -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 %>
<%= icon('far', file_icon(object.thumbnail_file&.content_type), class: 'fa-5x text-muted') %> diff --git a/app/views/application/_thumbnail.html.erb b/app/views/application/_thumbnail.html.erb index 903f0b9e8..745a04799 100644 --- a/app/views/application/_thumbnail.html.erb +++ b/app/views/application/_thumbnail.html.erb @@ -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') %> diff --git a/app/views/items/draft/_thumbnail.html.erb b/app/views/items/draft/_thumbnail.html.erb index e94b3741d..6d7b27857 100644 --- a/app/views/items/draft/_thumbnail.html.erb +++ b/app/views/items/draft/_thumbnail.html.erb @@ -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 %> -
- <%= icon('far', file_icon(file.content_type), class: 'fa-5x') %> - <%= file.filename %> -
- <% end %> +<% if thumbnail_path(file).present? %> + <%= safe_thumbnail_tag(thumbnail_path(file), alt: '', title: file.filename, size: '100x100') %> +<% else %> +
+ <%= icon('far', file_icon(file.content_type), class: 'fa-5x') %> + <%= file.filename %> +
<% end %> diff --git a/test/fixtures/active_storage/attachments.yml b/test/fixtures/active_storage/attachments.yml new file mode 100644 index 000000000..3102c4bcb --- /dev/null +++ b/test/fixtures/active_storage/attachments.yml @@ -0,0 +1,5 @@ +logo: + name: 'files' + record_type: 'Item' + record_id: <%= ActiveRecord::FixtureSet.identify(:fancy) %> + blob_id: <%= ActiveRecord::FixtureSet.identify(:jpeg) %> \ No newline at end of file diff --git a/test/fixtures/active_storage/blobs.yml b/test/fixtures/active_storage/blobs.yml new file mode 100644 index 000000000..e39e6f066 --- /dev/null +++ b/test/fixtures/active_storage/blobs.yml @@ -0,0 +1,6 @@ +jpeg: + key: '12345' + filename: 'image-sample.jpeg' + content_type: 'image/jpeg' + byte_size: 12086 + checksum: "GxpIjJsC4KnRoBKNjWnkJA==" \ No newline at end of file diff --git a/test/helpers/page_layout_helper_test.rb b/test/helpers/page_layout_helper_test.rb index 165d5211a..f599d3062 100644 --- a/test/helpers/page_layout_helper_test.rb +++ b/test/helpers/page_layout_helper_test.rb @@ -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 @@ -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 + community = communities(:books) + collection = collections(:fantasy_books) + @item = Item.new.tap do |uo| + 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| + 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('/') From 6fa836685f42ebd0a5306035443aec87b031e775 Mon Sep 17 00:00:00 2001 From: Tricia Jenkins Date: Wed, 3 Jun 2020 16:56:21 -0600 Subject: [PATCH 2/2] simplify tests and skip pdf if dependency not found --- test/helpers/page_layout_helper_test.rb | 64 +++++++------------------ 1 file changed, 18 insertions(+), 46 deletions(-) diff --git a/test/helpers/page_layout_helper_test.rb b/test/helpers/page_layout_helper_test.rb index f599d3062..972277633 100644 --- a/test/helpers/page_layout_helper_test.rb +++ b/test/helpers/page_layout_helper_test.rb @@ -97,32 +97,23 @@ def base_url # thumbnail_path + def mupdf_exists? + ActiveStorage::Previewer::MuPDFPreviewer.mutool_exists? + end + + def poppler_exists? + ActiveStorage::Previewer::PopplerPDFPreviewer.pdftoppm_exists? + end + test 'thumbnail_path should return preview for pdf (Invariable but Previewable)' do - community = communities(:books) - collection = collections(:fantasy_books) - @item = Item.new.tap do |uo| - 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 + skip 'this test requires the dependency MuPDF or Poppler to be installed' unless mupdf_exists? || poppler_exists? + + item = items(:fancy) + File.open(file_fixture('pdf-sample.pdf'), 'r') do |file| + item.add_and_ingest_files([file]) end - logo = @item.files.first + logo = item.reload.files.first expected = Rails.application.routes.url_helpers.rails_representation_path( logo.preview(resize: '100x100', auto_orient: true).processed ) @@ -130,31 +121,12 @@ def base_url end test 'thumbnail_path should return preview for image (Variable)' do - community = communities(:books) - collection = collections(:fantasy_books) - @item = Item.new.tap do |uo| - 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 + item = items(:fancy) + File.open(file_fixture('image-sample.jpeg'), 'r') do |file| + item.add_and_ingest_files([file]) end - logo = @item.files.first + logo = item.reload.files.first expected = Rails.application.routes.url_helpers.rails_representation_path( logo.variant(resize: '100x100', auto_orient: true).processed )