diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb index 9aae19e015..e81e846629 100644 --- a/app/controllers/spree/admin/products_controller.rb +++ b/app/controllers/spree/admin/products_controller.rb @@ -39,9 +39,6 @@ module Spree render :new end end - rescue Paperclip::Errors::NotIdentifiedByImageMagickError - @object.errors.add(:base, t('spree.admin.products.image_upload_error')) - respond_with(@object) end def show diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index 996067ec6f..da48229f80 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -1,103 +1,39 @@ # frozen_string_literal: true -require 'spree/core/s3_support' - module Spree class Image < Asset - include HasMigratingFile + SIZES = { + mini: { resize_to_fill: [48, 48] }, + small: { resize_to_fill: [227, 227] }, + product: { resize_to_limit: [240, 240] }, + large: { resize_to_limit: [600, 600] }, + }.freeze - validates_attachment_presence :attachment + has_one_attached :attachment + + validates :attachment, attached: true, content_type: %r{\Aimage/.*\Z} validate :no_attachment_errors - # This is where the styles are used in the app: - # - mini: used in the BackOffice: Bulk Product Edit page and Order Cycle edit page - # - small: used in the FrontOffice: Product List page - # - product: used in the BackOffice: Product Image upload modal in the Bulk Product Edit page - # and Product image edit page - # - large: used in the FrontOffice: product modal - has_one_migrating :attachment, - styles: { mini: "48x48#", small: "227x227#", - product: "240x240>", large: "600x600>" }, - default_style: :product, - url: '/spree/products/:id/:style/:basename.:extension', - path: ':rails_root/public/spree/products/:id/:style/:basename.:extension', - convert_options: { all: '-strip -auto-orient -colorspace sRGB' } - - # save the w,h of the original image (from which others can be calculated) - # we need to look at the write-queue for images which have not been saved yet - after_post_process :find_dimensions - - include Spree::Core::S3Support - supports_s3 :attachment - - # used by admin products autocomplete - def mini_url - attachment.url(:mini, false) + def variant(name) + attachment.variant(SIZES[name]) end - def find_dimensions - return if attachment.errors.present? + def url(size) + return unless attachment.variable? - geometry = Paperclip::Geometry.from_file(local_filename_of_original) - - self.attachment_width = geometry.width - self.attachment_height = geometry.height - end - - def local_filename_of_original - temporary = attachment.queued_for_write[:original] - - if temporary&.path.present? - temporary.path - else - attachment.path - end + Rails.application.routes.url_helpers.url_for(variant(size)) end # if there are errors from the plugin, then add a more meaningful message def no_attachment_errors - return if attachment.errors.empty? + return if errors[:attachment].empty? - if errors.all? { |e| e.type == "Paperclip::Errors::NotIdentifiedByImageMagickError" } + if errors.all? { |e| e.type == :content_type_invalid } attachment.errors.clear errors.add :base, I18n.t('spree.admin.products.image_upload_error') - else - errors.add :attachment, - I18n.t('spree.admin.products.paperclip_image_error', attachment_file_name: attachment_file_name) end + false end - - def self.set_attachment_attribute(attribute_name, attribute_value) - attachment_definitions[:attachment][attribute_name] = attribute_value - end - - def self.set_storage_attachment_attributes - if Spree::Config[:use_s3] - set_s3_attachment_attributes - else - attachment_definitions[:attachment].delete(:storage) - end - end - - def self.set_s3_attachment_attributes - set_attachment_attribute(:storage, :s3) - set_attachment_attribute(:s3_credentials, s3_credentials) - set_attachment_attribute(:s3_headers, - ActiveSupport::JSON.decode(Spree::Config[:s3_headers])) - set_attachment_attribute(:bucket, Spree::Config[:s3_bucket]) - - # We use :s3_alias_url (virtual host url style) and set the URL on property s3_host_alias - set_attachment_attribute(:s3_host_alias, attachment_definitions[:attachment][:url]) - set_attachment_attribute(:url, ":s3_alias_url") - end - private_class_method :set_s3_attachment_attributes - - def self.s3_credentials - { access_key_id: Spree::Config[:s3_access_key], - secret_access_key: Spree::Config[:s3_secret], - bucket: Spree::Config[:s3_bucket] } - end - private_class_method :s3_credentials end end diff --git a/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb b/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb index f5ed38c76a..fa0c9e7365 100644 --- a/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb +++ b/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb @@ -11,7 +11,7 @@ module Api end def image_url - object.images.present? ? object.images.first.attachment.url(:mini) : nil + object.images.first&.url(:mini) end def master_id diff --git a/app/serializers/api/admin/product_serializer.rb b/app/serializers/api/admin/product_serializer.rb index 6805d81d1f..d65909a521 100644 --- a/app/serializers/api/admin/product_serializer.rb +++ b/app/serializers/api/admin/product_serializer.rb @@ -13,19 +13,11 @@ module Api has_one :master, serializer: Api::Admin::VariantSerializer def image_url - if object.images.present? - object.images.first.attachment.url(:product) - else - "/noimage/product.png" - end + object.images.first&.url(:product) || "/noimage/product.png" end def thumb_url - if object.images.present? - object.images.first.attachment.url(:mini) - else - "/noimage/mini.png" - end + object.images.first&.url(:mini) || "/noimage/mini.png" end def on_hand diff --git a/app/serializers/api/admin/variant_serializer.rb b/app/serializers/api/admin/variant_serializer.rb index 1be17e1c19..062ce4ceee 100644 --- a/app/serializers/api/admin/variant_serializer.rb +++ b/app/serializers/api/admin/variant_serializer.rb @@ -33,9 +33,7 @@ module Api end def image - return if object.product.images.empty? - - object.product.images.first.mini_url + object.product.images.first&.url(:mini) end def in_stock diff --git a/app/serializers/api/image_serializer.rb b/app/serializers/api/image_serializer.rb index ead54697d6..57d44ef74e 100644 --- a/app/serializers/api/image_serializer.rb +++ b/app/serializers/api/image_serializer.rb @@ -4,18 +4,18 @@ class Api::ImageSerializer < ActiveModel::Serializer attributes :id, :alt, :thumb_url, :small_url, :image_url, :large_url def thumb_url - object.attachment.url(:mini, false) + object.url(:mini) end def small_url - object.attachment.url(:small, false) + object.url(:small) end def image_url - object.attachment.url(:product, false) + object.url(:product) end def large_url - object.attachment.url(:large, false) + object.url(:large) end end diff --git a/app/serializers/api/variant_serializer.rb b/app/serializers/api/variant_serializer.rb index 524d940656..f58810c52b 100644 --- a/app/serializers/api/variant_serializer.rb +++ b/app/serializers/api/variant_serializer.rb @@ -35,11 +35,7 @@ class Api::VariantSerializer < ActiveModel::Serializer end def thumb_url - if object.product.images.present? - object.product.images.first.attachment.url(:mini) - else - "/noimage/mini.png" - end + object.product.images.first&.url(:mini) || "/noimage/mini.png" end def unit_price_price diff --git a/app/services/image_importer.rb b/app/services/image_importer.rb index 8fc243e755..706de935a0 100644 --- a/app/services/image_importer.rb +++ b/app/services/image_importer.rb @@ -2,21 +2,12 @@ class ImageImporter def import(url, product) - attach(download(url), product) - end + valid_url = URI.parse(url) + file = open(valid_url.to_s) + filename = File.basename(valid_url.path) - private - - def download(url) - local_file = Tempfile.new - remote_file = open(url) - IO.copy_stream(remote_file, local_file) - local_file - end - - def attach(file, product) Spree::Image.create( - attachment: file, + attachment: { io: file, filename: filename }, viewable_id: product.master.id, viewable_type: Spree::Variant, ) diff --git a/app/views/spree/admin/images/edit.html.haml b/app/views/spree/admin/images/edit.html.haml index 49d1361daf..970f67d132 100644 --- a/app/views/spree/admin/images/edit.html.haml +++ b/app/views/spree/admin/images/edit.html.haml @@ -11,7 +11,9 @@ .field.alpha.three.columns.align-center = f.label t('spree.thumbnail') %br/ - = link_to image_tag(@image.attachment.url(:small)), @image.attachment.url(:product) + - # A Rails bug makes it necessary to call `main_app.url_for` here. + - # https://github.com/rails/rails/issues/31325 + = link_to image_tag(main_app.url_for(@image.variant(:small))), main_app.url_for(@image.variant(:product)) .nine.columns.omega = render partial: 'form', locals: { f: f } .clear diff --git a/app/views/spree/admin/images/index.html.haml b/app/views/spree/admin/images/index.html.haml index 7f777c573a..978de27092 100644 --- a/app/views/spree/admin/images/index.html.haml +++ b/app/views/spree/admin/images/index.html.haml @@ -32,7 +32,9 @@ %td.no-border %span.handle %td - = link_to image_tag(image.attachment.url(:mini)), image.attachment.url(:product) + - # A Rails bug makes it necessary to call `main_app.url_for` here. + - # https://github.com/rails/rails/issues/31325 + = link_to image_tag(main_app.url_for(image.variant(:mini))), main_app.url_for(image.variant(:product)) %td= options_text_for(image) %td= image.alt %td.actions diff --git a/app/views/spree/shared/_variant_thumbnail.html.haml b/app/views/spree/shared/_variant_thumbnail.html.haml index 719725c927..d92e205a0b 100644 --- a/app/views/spree/shared/_variant_thumbnail.html.haml +++ b/app/views/spree/shared/_variant_thumbnail.html.haml @@ -1,4 +1,6 @@ - if variant.product.images.length == 0 = image_tag("/noimage/mini.png") - else - = image_tag(variant.product.images.first.attachment.url(:mini)) + - # A Rails bug makes it necessary to call `main_app.url_for` here. + - # https://github.com/rails/rails/issues/31325 + = image_tag(main_app.url_for(variant.product.images.first.variant(:mini))) diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 145b585335..c3712a72f9 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -29,11 +29,6 @@ Rails.application.reloader.to_prepare do # applied correctly in Spree::Config. MailConfiguration.apply! - # Attachments settings - Spree::Image.set_attachment_attribute(:path, ENV['ATTACHMENT_PATH']) if ENV['ATTACHMENT_PATH'] - Spree::Image.set_attachment_attribute(:url, ENV['ATTACHMENT_URL']) if ENV['ATTACHMENT_URL'] - Spree::Image.set_storage_attachment_attributes - # TODO Work out why this is necessary # Seems like classes within OFN module become 'uninitialized' when server reloads # unless the empty module is explicity 'registered' here. Something to do with autoloading? diff --git a/config/locales/en.yml b/config/locales/en.yml index 82c667fb07..dc17d1d821 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3902,7 +3902,6 @@ See the %{link} to find out more about %{sitename}'s features and to start using no_payment_via_admin_backend: Paypal payments cannot be captured in the Backoffice products: image_upload_error: "The product image was not recognised. Please upload an image in PNG or JPG format." - paperclip_image_error: "Paperclip returned errors for file '%{attachment_file_name}' - check ImageMagick installation or image source file." new: title: "New Product" new_product: "New Product" diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index f4b778432f..659d2c5c6d 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -46,7 +46,7 @@ module Spree def duplicate_image(image) new_image = image.dup - new_image.assign_attributes(attachment: image.attachment.clone) + new_image.attachment.attach(image.attachment_blob) new_image end diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb index 7f2177494d..5474e5edad 100644 --- a/spec/controllers/spree/admin/products_controller_spec.rb +++ b/spec/controllers/spree/admin/products_controller_spec.rb @@ -163,9 +163,8 @@ describe Spree::Admin::ProductsController, type: :controller do } ) - expect do - spree_put :create, product: product_attrs_with_image - end.not_to raise_error Paperclip::Errors::NotIdentifiedByImageMagickError + spree_put :create, product: product_attrs_with_image + expect(response.status).to eq 200 end end diff --git a/spec/factories/order_cycle_factory.rb b/spec/factories/order_cycle_factory.rb index 2b1c50ddeb..8dbfcc6acd 100644 --- a/spec/factories/order_cycle_factory.rb +++ b/spec/factories/order_cycle_factory.rb @@ -35,7 +35,7 @@ FactoryBot.define do viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "position 1", - attachment: white_logo_file, + attachment: Rack::Test::UploadedFile.new(white_logo_path), position: 1 ) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 4026c01c8f..05813f104d 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -70,11 +70,8 @@ describe Spree::Core::ProductDuplicator do expect(new_variant).to receive(:price=).with(variant.price) expect(new_variant).to receive(:currency=).with(variant.currency) - expect(image.attachment).to receive(:clone).and_return(image.attachment) - - expect(new_image).to receive(:assign_attributes). - with(attachment: image.attachment). - and_return(new_image) + expect(image).to receive(:attachment_blob) + expect(new_image).to receive_message_chain(:attachment, :attach) expect(new_property).to receive(:created_at=).with(nil) expect(new_property).to receive(:updated_at=).with(nil) diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index f7024d0e9b..52debc535a 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -6,91 +6,54 @@ module Spree describe Image do include FileHelper + subject { + Spree::Image.create!( + attachment: black_logo_file, + viewable: product.master, + ) + } let(:product) { create(:product) } + describe "#url" do + it "returns URLs for different sizes" do + expect(subject.url(:small)).to match( + %r|^http://test\.host/rails/active_storage/representations/redirect/.+/logo-black\.png$| + ) + end + + it "returns nil for unsupported formats" do + subject.attachment_blob.update_columns( + content_type: "application/octet-stream" + ) + + expect(subject.url(:small)).to eq nil + end + end + describe "using local storage" do it "stores a new image" do - image = Spree::Image.create!( - attachment: black_logo_file, - viewable: product.master, - ) + attachment = subject.attachment + expect(attachment.attached?).to eq true - attachment = image.attachment - expect(attachment.exists?).to eq true - expect(attachment.file?).to eq true - expect(attachment.url).to match %r"^/spree/products/[0-9]+/product/logo-black\.png\?[0-9]+$" - end - - it "duplicates the image with Active Storage" do - image = Spree::Image.create!( - attachment: file, - viewable: product.master, - ) - - attachment = image.active_storage_attachment url = Rails.application.routes.url_helpers.url_for(attachment) - expect(url).to match %r|^http://test\.host/rails/active_storage/blobs/redirect/[[:alnum:]-]+/logo-black\.png$| end end describe "using AWS S3" do - let(:s3_config) { - { - url: ":s3_alias_url", - storage: :s3, - s3_credentials: { - access_key_id: "A...A", - secret_access_key: "H...H", - }, - s3_headers: { "Cache-Control" => "max-age=31557600" }, - bucket: "ofn", - s3_protocol: "https", - s3_host_alias: "ofn.s3.us-east-1.amazonaws.com", - - # This is for easier testing: - path: "/:id/:style/:basename.:extension", - } - } - before do - attachment_definition = Spree::Image.attachment_definitions[:attachment] - allow(Spree::Image).to receive(:attachment_definitions).and_return( - attachment: attachment_definition.merge(s3_config) - ) allow(Rails.application.config.active_storage). to receive(:service).and_return(:test_amazon) end it "saves a new image when none is present" do - # Paperclip requests - upload_pattern = %r"^https://ofn.s3.amazonaws.com/[0-9]+/(original|mini|small|product|large)/logo-black.png$" - download_pattern = %r"^https://ofn.s3.amazonaws.com/[0-9]+/product/logo-black.png$" - public_url_pattern = %r"^https://ofn.s3.us-east-1.amazonaws.com/[0-9]+/product/logo-black.png\?[0-9]+$" - - stub_request(:put, upload_pattern).to_return(status: 200, body: "", headers: {}) - stub_request(:head, download_pattern).to_return(status: 200, body: "", headers: {}) - # Active Storage requests as_upload_pattern = %r"^https://ofn.s3.amazonaws.com/[[:alnum:]]+$" stub_request(:put, as_upload_pattern).to_return(status: 200, body: "", headers: {}) - image = Spree::Image.create!( - attachment: black_logo_file, - viewable: product.master, - ) - - # Paperclip - attachment = image.attachment - expect(attachment.exists?).to eq true - expect(attachment.file?).to eq true - expect(attachment.url).to match public_url_pattern - - # Active Storage - attachment = image.active_storage_attachment - expect(attachment).to be_attached - expect(Rails.application.routes.url_helpers.url_for(attachment)). + expect(subject.attachment).to be_attached + expect(Rails.application.routes.url_helpers.url_for(subject.attachment)). to match %r"^http://test\.host/rails/active_storage/blobs/redirect/[[:alnum:]-]+/logo-black\.png" end end diff --git a/spec/services/image_importer_spec.rb b/spec/services/image_importer_spec.rb index 88ba329786..9ec390d25f 100644 --- a/spec/services/image_importer_spec.rb +++ b/spec/services/image_importer_spec.rb @@ -15,7 +15,7 @@ describe ImageImporter do }.by(1) expect(product.images.count).to eq 1 - expect(product.images.first.attachment.size).to eq 6274 + expect(product.images.first.attachment_blob.byte_size).to eq 6274 end end end diff --git a/spec/support/api_helper.rb b/spec/support/api_helper.rb index 37d1d6a4d1..7632c90c86 100644 --- a/spec/support/api_helper.rb +++ b/spec/support/api_helper.rb @@ -28,7 +28,7 @@ module OpenFoodNetwork end def image(filename) - File.open(Rails.root + "spec/support/fixtures" + filename) + Rack::Test::UploadedFile.new(Rails.root + "spec/support/fixtures" + filename) end end end