From f4d59305d71d288332e40e31c32553656cd368c6 Mon Sep 17 00:00:00 2001 From: Prikesh Savla Date: Mon, 8 Dec 2025 09:08:35 +0530 Subject: [PATCH 1/2] Upgraded gem active_storage_validations from 1.1.2 to 3.0.2 and fixed any upgrade related issues Changed all references of processable_image to processable_file which was a breaking change from v1 to v2 https://github.com/igorkasyanchuk/active_storage_validations/tree/3.0.2?tab=readme-ov-file#upgrading-from-1x-to-2x Also it upgraded the way of validating files from just the file name and content type, so tests also needed to change for file upload checks Refactored all the similar file image validator content type in Spree::Image::ACCEPTED_CONTENT_TYPES and Updated ImageBuilder.import method to use the url.path when getting filename. --- Gemfile.lock | 11 ++++---- app/models/enterprise.rb | 12 ++++---- app/models/enterprise_group.rb | 8 +++--- app/models/spree/image.rb | 6 ++-- .../app/services/image_builder.rb | 2 +- .../supplied_product_importer_spec.rb | 4 +-- spec/fixtures/files/logo.bmp | Bin 0 -> 1818 bytes .../_import/downloads_from_the_Internet.yml | 2 +- spec/models/enterprise_spec.rb | 26 ++++++------------ spec/services/image_importer_spec.rb | 2 +- spec/system/admin/products_spec.rb | 2 +- spec/system/admin/products_v3/update_spec.rb | 4 +-- 12 files changed, 36 insertions(+), 43 deletions(-) create mode 100644 spec/fixtures/files/logo.bmp diff --git a/Gemfile.lock b/Gemfile.lock index 1f02aa3ad8..1dfd9a5d42 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -104,11 +104,12 @@ GEM rails-html-sanitizer (~> 1.6) active_model_serializers (0.8.4) activemodel (>= 3.0) - active_storage_validations (1.1.4) - activejob (>= 5.2.0) - activemodel (>= 5.2.0) - activestorage (>= 5.2.0) - activesupport (>= 5.2.0) + active_storage_validations (3.0.2) + activejob (>= 6.1.4) + activemodel (>= 6.1.4) + activestorage (>= 6.1.4) + activesupport (>= 6.1.4) + marcel (>= 1.0.3) activejob (7.1.6) activesupport (= 7.1.6) globalid (>= 0.3.6) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index b36c3cb33e..c126b28984 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -110,14 +110,14 @@ class Enterprise < ApplicationRecord end validates :logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :promo_image, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :white_label_logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :terms_and_conditions, content_type: { in: "application/pdf", message: I18n.t(:enterprise_terms_and_conditions_type_error), diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb index e6208e3000..cb2252b2d4 100644 --- a/app/models/enterprise_group.rb +++ b/app/models/enterprise_group.rb @@ -29,11 +29,11 @@ class EnterpriseGroup < ApplicationRecord has_one_attached :promo_image, service: image_service validates :logo, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES validates :promo_image, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ::Spree::Image::ACCEPTED_CONTENT_TYPES scope :by_position, -> { order('position ASC') } scope :on_front_page, -> { where(on_front_page: true) } diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index 350b1c5b4c..431b1679b0 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -2,6 +2,8 @@ module Spree class Image < Asset + ACCEPTED_CONTENT_TYPES = %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + has_one_attached :attachment, service: image_service do |attachment| attachment.variant :mini, resize_to_fill: [48, 48] attachment.variant :small, resize_to_fill: [227, 227] @@ -11,8 +13,8 @@ module Spree validates :attachment, attached: true, - processable_image: true, - content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} + processable_file: true, + content_type: ACCEPTED_CONTENT_TYPES validate :no_attachment_errors def self.default_image_url(size) diff --git a/engines/dfc_provider/app/services/image_builder.rb b/engines/dfc_provider/app/services/image_builder.rb index 06ab056e0c..798a5d0f38 100644 --- a/engines/dfc_provider/app/services/image_builder.rb +++ b/engines/dfc_provider/app/services/image_builder.rb @@ -19,7 +19,7 @@ class ImageBuilder < DfcBuilder def self.import(image_link) url = URI.parse(image_link) - filename = File.basename(image_link) + filename = File.basename(url.path) metadata = { custom: { origin: image_link } } Spree::Image.new.tap do |image| diff --git a/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb b/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb index 815a1c6897..07a8d1bbda 100644 --- a/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb +++ b/engines/dfc_provider/spec/services/supplied_product_importer_spec.rb @@ -251,7 +251,7 @@ RSpec.describe SuppliedProductImporter do supplied_product.isVariantOf << tomatoes imported_product = importer.import_variant(supplied_product, supplier).product - expect(imported_product.image.attachment.filename).to eq "tomato.png?v=1" + expect(imported_product.image.attachment.filename).to eq "tomato.png" expect { importer.import_variant(supplied_product, supplier).product @@ -266,7 +266,7 @@ RSpec.describe SuppliedProductImporter do } .to change { imported_product.image } - expect(imported_product.image.attachment.filename).to eq "tomato.png?v=2" + expect(imported_product.image.attachment.filename).to eq "tomato.png" end context "when spree_product_uri doesn't match the server host" do diff --git a/spec/fixtures/files/logo.bmp b/spec/fixtures/files/logo.bmp new file mode 100644 index 0000000000000000000000000000000000000000..f03e68b138349937618b46d6561098e48e82f842 GIT binary patch literal 1818 zcmbu9NoZ6-5Qg7mm`pUHLOdm|5pwb>2oeUzU=R}!MNt$*R75l$Od!D}3Q=58)Qj=p zMT6po8^nW}1o!2jiAxj(#|0JikSOj5&+mUz72ZKl7V}O0UEN*XUH#t6)|RG&$aeC- z0Y<>bFbosH*dU4CTNnf-#d&C&g+Y-2sU2pkXVrVp|9UMAtV-g%UXE{v2k_RAR!n^f z8(Zkw_nm(KAG##_7yo$@j0DNqH2EtWzTnJ!0bZl&5X zbT>4ZRxYpQd~^|>ugYHFpyaURi`rHDoH4KVDYwv(9AX8oRP51yyZQttGS~V6Q zhu7Wcz0=(u1>KAGru)=8Q2{xdKMJiix4{vRRaxH^w4Q&}@F#RwyczuqH2x9DbXL_! z_6GKW>L0xyHZMb4wi%YeId~7U^)^P&%_-Ae=uXq+=p1CM?eG0MVoxll+Vp|Wq8k4O z*>>0+sW3c_*B!|kJr_Cw1=em+r&*cBI%r>nEcAo+y%L^+%)cY5ef*p1IctgE1?7=x xo}Mw)QqREwXonl%XB^0-J;> Date: Mon, 8 Dec 2025 22:08:59 +0530 Subject: [PATCH 2/2] Extended imageImport and ImageBuilder to get the content type of the file for the attacment for avoiding issues for files without extensions. Updated config/locale/en.yml for the active_storage_validations related error messages --- app/services/image_importer.rb | 4 +- config/locales/en.yml | 80 ++++++++++++++----- .../app/services/image_builder.rb | 4 +- .../api/v0/product_images_controller_spec.rb | 3 +- .../_import/downloads_from_the_Internet.yml | 2 +- spec/services/image_importer_spec.rb | 4 +- 6 files changed, 71 insertions(+), 26 deletions(-) diff --git a/app/services/image_importer.rb b/app/services/image_importer.rb index 9cef10548a..78f525323a 100644 --- a/app/services/image_importer.rb +++ b/app/services/image_importer.rb @@ -11,7 +11,9 @@ class ImageImporter image = Spree::Image.create do |img| PrivateAddressCheck.only_public_connections do - img.attachment.attach(io: valid_url.open, filename:, metadata:) + io = valid_url.open + content_type = Marcel::MimeType.for(io) + img.attachment.attach(io:, filename:, metadata:, content_type:) end end product.image = image if image diff --git a/config/locales/en.yml b/config/locales/en.yml index d3fe23123d..985a743048 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -138,26 +138,66 @@ en: # Used by active_storage_validations errors: messages: - content_type_invalid: "has an invalid content type" - file_size_out_of_range: "size %{file_size} is not between required range" - limit_out_of_range: "total number is out of range" - image_metadata_missing: "is not a valid image" - dimension_min_inclusion: "must be greater than or equal to %{width} x %{height} pixel." - dimension_max_inclusion: "must be less than or equal to %{width} x %{height} pixel." - dimension_width_inclusion: "width is not included between %{min} and %{max} pixel." - dimension_height_inclusion: "height is not included between %{min} and %{max} pixel." - dimension_width_greater_than_or_equal_to: "width must be greater than or equal to %{length} pixel." - dimension_height_greater_than_or_equal_to: "height must be greater than or equal to %{length} pixel." - dimension_width_less_than_or_equal_to: "width must be less than or equal to %{length} pixel." - dimension_height_less_than_or_equal_to: "height must be less than or equal to %{length} pixel." - dimension_width_equal_to: "width must be equal to %{length} pixel." - dimension_height_equal_to: "height must be equal to %{length} pixel." - aspect_ratio_not_square: "must be a square image" - aspect_ratio_not_portrait: "must be a portrait image" - aspect_ratio_not_landscape: "must be a landscape image" - aspect_ratio_is_not: "must have an aspect ratio of %{aspect_ratio}" - aspect_ratio_unknown: "has an unknown aspect ratio" - image_not_processable: "is not a valid image" + content_type_invalid: + one: "has an invalid content type (authorized content type is %{authorized_human_content_types})" + other: "has an invalid content type (authorized content types are %{authorized_human_content_types})" + content_type_spoofed: + one: "has a content type that is not equivalent to the one that is detected through its content (authorized content type is %{authorized_human_content_types})" + other: "has a content type that is not equivalent to the one that is detected through its content (authorized content types are %{authorized_human_content_types})" + file_size_not_less_than: "file size must be less than %{max} (current size is %{file_size})" + file_size_not_less_than_or_equal_to: "file size must be less than or equal to %{max} (current size is %{file_size})" + file_size_not_greater_than: "file size must be greater than %{min} (current size is %{file_size})" + file_size_not_greater_than_or_equal_to: "file size must be greater than or equal to %{min} (current size is %{file_size})" + file_size_not_between: "file size must be between %{min} and %{max} (current size is %{file_size})" + file_size_not_equal_to: "file size must be equal to %{exact} (current size is %{file_size})" + total_file_size_not_less_than: "total file size must be less than %{max} (current size is %{total_file_size})" + total_file_size_not_less_than_or_equal_to: "total file size must be less than or equal to %{max} (current size is %{total_file_size})" + total_file_size_not_greater_than: "total file size must be greater than %{min} (current size is %{total_file_size})" + total_file_size_not_greater_than_or_equal_to: "total file size must be greater than or equal to %{min} (current size is %{total_file_size})" + total_file_size_not_between: "total file size must be between %{min} and %{max} (current size is %{total_file_size})" + total_file_size_not_equal_to: "total file size must be equal to %{exact} (current size is %{total_file_size})" + duration_not_less_than: "duration must be less than %{max} (current duration is %{duration})" + duration_not_less_than_or_equal_to: "duration must be less than or equal to %{max} (current duration is %{duration})" + duration_not_greater_than: "duration must be greater than %{min} (current duration is %{duration})" + duration_not_greater_than_or_equal_to: "duration must be greater than or equal to %{min} (current duration is %{duration})" + duration_not_between: "duration must be between %{min} and %{max} (current duration is %{duration})" + duration_not_equal_to: "duration must be equal to %{exact} (current duration is %{duration})" + limit_out_of_range: + zero: "no files attached (must have between %{min} and %{max} files)" + one: "only 1 file attached (must have between %{min} and %{max} files)" + other: "total number of files must be between %{min} and %{max} files (there are %{count} files attached)" + limit_min_not_reached: + zero: "no files attached (must have at least %{min} files)" + one: "only 1 file attached (must have at least %{min} files)" + other: "%{count} files attached (must have at least %{min} files)" + limit_max_exceeded: + zero: "no files attached (maximum is %{max} files)" + one: "too many files attached (maximum is %{max} files, got %{count})" + other: "too many files attached (maximum is %{max} files, got %{count})" + attachment_missing: "is missing its attachment" + media_metadata_missing: "is not a valid media file" + dimension_min_not_included_in: "must be greater than or equal to %{width} x %{height} pixels" + dimension_max_not_included_in: "must be less than or equal to %{width} x %{height} pixels" + dimension_width_not_included_in: "width is not included between %{min} and %{max} pixels" + dimension_height_not_included_in: "height is not included between %{min} and %{max} pixels" + dimension_width_not_greater_than_or_equal_to: "width must be greater than or equal to %{length} pixels" + dimension_height_not_greater_than_or_equal_to: "height must be greater than or equal to %{length} pixels" + dimension_width_not_less_than_or_equal_to: "width must be less than or equal to %{length} pixels" + dimension_height_not_less_than_or_equal_to: "height must be less than or equal to %{length} pixels" + dimension_width_not_equal_to: "width must be equal to %{length} pixels" + dimension_height_not_equal_to: "height must be equal to %{length} pixels" + aspect_ratio_not_square: "must be square (current file is %{width}x%{height}px)" + aspect_ratio_not_portrait: "must be portrait (current file is %{width}x%{height}px)" + aspect_ratio_not_landscape: "must be landscape (current file is %{width}x%{height}px)" + aspect_ratio_not_x_y: "must be %{authorized_aspect_ratios} (current file is %{width}x%{height}px)" + aspect_ratio_invalid: "has an invalid aspect ratio (valid aspect ratios are %{authorized_aspect_ratios})" + file_not_processable: "is not identified as a valid media file" + pages_not_less_than: "page count must be less than %{max} (current page count is %{pages})" + pages_not_less_than_or_equal_to: "page count must be less than or equal to %{max} (current page count is %{pages})" + pages_not_greater_than: "page count must be greater than %{min} (current page count is %{pages})" + pages_not_greater_than_or_equal_to: "page count must be greater than or equal to %{min} (current page count is %{pages})" + pages_not_between: "page count must be between %{min} and %{max} (current page count is %{pages})" + pages_not_equal_to: "page count must be equal to %{exact} (current page count is %{pages})" not_found: title: "The page you were looking for doesn't exist (404)" message_html: "Please try again diff --git a/engines/dfc_provider/app/services/image_builder.rb b/engines/dfc_provider/app/services/image_builder.rb index 798a5d0f38..e9da4aa36a 100644 --- a/engines/dfc_provider/app/services/image_builder.rb +++ b/engines/dfc_provider/app/services/image_builder.rb @@ -24,7 +24,9 @@ class ImageBuilder < DfcBuilder Spree::Image.new.tap do |image| PrivateAddressCheck.only_public_connections do - image.attachment.attach(io: url.open, filename:, metadata:) + io = url.open + content_type = Marcel::MimeType.for(io) + image.attachment.attach(io:, filename:, metadata:, content_type:) end end rescue StandardError diff --git a/spec/controllers/api/v0/product_images_controller_spec.rb b/spec/controllers/api/v0/product_images_controller_spec.rb index 5fe0e4bb24..ac46c412f4 100644 --- a/spec/controllers/api/v0/product_images_controller_spec.rb +++ b/spec/controllers/api/v0/product_images_controller_spec.rb @@ -45,7 +45,8 @@ RSpec.describe Api::V0::ProductImagesController do expect(response).to have_http_status :unprocessable_entity expect(product_without_image.image).to be_nil expect(json_response["id"]).to eq nil - expect(json_response["errors"]).to include "Attachment has an invalid content type" + expect(json_response["errors"]).to include "Attachment is " \ + "not identified as a valid media file" end end end diff --git a/spec/fixtures/vcr_cassettes/ImageImporter/_import/downloads_from_the_Internet.yml b/spec/fixtures/vcr_cassettes/ImageImporter/_import/downloads_from_the_Internet.yml index eb4760b32b..86808900fb 100644 --- a/spec/fixtures/vcr_cassettes/ImageImporter/_import/downloads_from_the_Internet.yml +++ b/spec/fixtures/vcr_cassettes/ImageImporter/_import/downloads_from_the_Internet.yml @@ -2,7 +2,7 @@ http_interactions: - request: method: get - uri: https://s3.amazonaws.com/ofn_production/eofop2en1y6tu9fr1x9b0wzwgs5r.png + uri: https://s3.amazonaws.com/ofn_production/eofop2en1y6tu9fr1x9b0wzwgs5r body: encoding: US-ASCII string: '' diff --git a/spec/services/image_importer_spec.rb b/spec/services/image_importer_spec.rb index d5cc88b375..d9c7e5cd0b 100644 --- a/spec/services/image_importer_spec.rb +++ b/spec/services/image_importer_spec.rb @@ -3,11 +3,11 @@ require 'spec_helper' RSpec.describe ImageImporter do - let(:ofn_url) { "https://s3.amazonaws.com/ofn_production/eofop2en1y6tu9fr1x9b0wzwgs5r.png" } + let(:ofn_url) { "https://s3.amazonaws.com/ofn_production/eofop2en1y6tu9fr1x9b0wzwgs5r" } let(:product) { create(:product) } describe "#import" do - it "downloads from the Internet", :vcr do + it "downloads from the Internet", :vcr, :aggregate_failures do expect { subject.import(ofn_url, product) }.to change {