From b231ddd15ee14f155f40c35eae0e40207560a256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Busqu=C3=A9?= Date: Fri, 25 Mar 2022 13:54:35 +0100 Subject: [PATCH] Fixes defining thumbnail sizes through ActiveStorage adapter The adapter intends to make it possible to work with ActiveStorage as it was Paperclip. We had been using Paperclip until Rails 6.1 introduced ActiveStorage's public links. We moved to ActiveStorage as the default file attachment backend from that point on, but we tried to make its API compatible with Paperclip's one. See https://github.com/solidusio/solidus/pull/3501 for details. When trying to translate [Paperclip's style definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494) (based on [ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage expects (which is [image_processing syntax](https://github.com/janko/image_processing), common for ImageMagick and libvips), we were transforming everything to a [`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit) option. However, that corresponds to the `>` symbol in ImageMagick, but not to other options that can be given. Whit this commit, we add support for defining other kinds of transformations in ActiveStorage. However, we're still missing more advanced options that can be given when using [standalone Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{ geometry: '100x100', format: :jpg }`) or [standalone ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods) (example `rotate: 90`). In the long run, we should aim to completely differentiate how definitions are given for each library. --- .../active_storage_adapter/attachment.rb | 33 ++++++---- .../active_storage_adapter/attachment_spec.rb | 60 +++++++++++++++++++ 2 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 core/spec/models/spree/concerns/active_storage_adapter/attachment_spec.rb diff --git a/core/app/models/concerns/spree/active_storage_adapter/attachment.rb b/core/app/models/concerns/spree/active_storage_adapter/attachment.rb index d37d55b9e23..b2a63a6bc23 100644 --- a/core/app/models/concerns/spree/active_storage_adapter/attachment.rb +++ b/core/app/models/concerns/spree/active_storage_adapter/attachment.rb @@ -4,14 +4,16 @@ module Spree module ActiveStorageAdapter - # Decorares AtiveStorage attachment to add methods exptected by Solidus' + # Decorates ActiveStorage attachment to add methods expected by Solidus' # Paperclip-oriented attachment support. class Attachment delegate_missing_to :@attachment + attr_reader :attachment + def initialize(attachment, styles: {}) @attachment = attachment - @styles = normalize_styles(styles) + @transformations = styles_to_transformations(styles) end def exists? @@ -27,13 +29,13 @@ def url(style = nil) end def variant(style = nil) - size = style_to_size(style) - @attachment.variant( - resize_to_limit: size, + transformation = @transformations[style] || default_transformation(width, height) + + @attachment.variant({ saver: { strip: true } - ).processed + }.merge(transformation)).processed end def height @@ -59,12 +61,23 @@ def metadata @attachment.metadata end - def normalize_styles(styles) - styles.transform_values { |v| v.split('x').map(&:to_i) } + def styles_to_transformations(styles) + styles.transform_values(&method(:imagemagick_to_image_processing_definition)) + end + + def imagemagick_to_image_processing_definition(definition) + width_height = definition.split('x').map(&:to_i) + + case definition[-1].to_sym + when :^ + { resize_to_fill: width_height } + else + default_transformation(*width_height) + end end - def style_to_size(style) - @styles.fetch(style&.to_sym) { [width, height] } + def default_transformation(width, height) + { resize_to_limit: [width, height] } end end end diff --git a/core/spec/models/spree/concerns/active_storage_adapter/attachment_spec.rb b/core/spec/models/spree/concerns/active_storage_adapter/attachment_spec.rb new file mode 100644 index 00000000000..fd528073194 --- /dev/null +++ b/core/spec/models/spree/concerns/active_storage_adapter/attachment_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'rails_helper' + +unless ENV['DISABLE_ACTIVE_STORAGE'] + RSpec.describe Spree::ActiveStorageAdapter::Attachment do + describe '#variant' do + it "converts to resize_to_limit when definition doesn't contain any special symbol" do + image = create(:image) + + attachment = described_class.new(image.attachment.attachment, styles: { mini: '10x10' }) + + expect( + attachment.variant(:mini).variation.transformations + ).to include(resize_to_limit: [10, 10]) + end + + it 'converts to resize_to_limit when definition ends with >' do + image = create(:image) + + attachment = described_class.new(image.attachment.attachment, styles: { mini: '10x10>' }) + + expect( + attachment.variant(:mini).variation.transformations + ).to include(resize_to_limit: [10, 10]) + end + + it 'converts to resize_to_fill when definition ends with ^' do + image = create(:image) + + attachment = described_class.new(image.attachment.attachment, styles: { mini: '10x10^' }) + + expect( + attachment.variant(:mini).variation.transformations + ).to include(resize_to_fill: [10, 10]) + end + + it 'strips definitions' do + image = create(:image) + + attachment = described_class.new(image.attachment.attachment, styles: { mini: ' 10x10 ' }) + + expect( + attachment.variant(:mini).variation.transformations + ).to include(resize_to_limit: [10, 10]) + end + + + it "defaults to the image's width and height" do + image = create(:image) + + attachment = described_class.new(image.attachment.attachment, styles: {}) + + expect( + attachment.variant(:mini).variation.transformations + ).to include(resize_to_limit: [attachment.width, attachment.height]) + end + end + end +end