diff --git a/app/assets/javascripts/templates/product_modal.html.haml b/app/assets/javascripts/templates/product_modal.html.haml index 95c74c5f72..9102183872 100644 --- a/app/assets/javascripts/templates/product_modal.html.haml +++ b/app/assets/javascripts/templates/product_modal.html.haml @@ -15,6 +15,6 @@ .columns.small-12.medium-6.large-6.product-img %img{"ng-src" => "{{::product.largeImage}}", "ng-if" => "::product.largeImage"} - %img.placeholder{ src: "/noimage/large.png", "ng-if" => "::!product.largeImage"} + %img.placeholder{ src: Spree::Image.default_image_url(:large), "ng-if" => "::!product.largeImage"} %ng-include{src: "'partials/close.html'"} diff --git a/app/models/application_record.rb b/app/models/application_record.rb index d36b82d6f7..4e726e63b6 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -10,4 +10,12 @@ class ApplicationRecord < ActiveRecord::Base include ArelHelpers::JoinAssociation self.abstract_class = true + + def self.image_service + ENV["S3_BUCKET"].present? ? :amazon_public : :local + end + + def url_for(*args) + Rails.application.routes.url_helpers.url_for(*args) + end end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 9b74a711a5..a4d6ae0370 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -5,20 +5,9 @@ class Enterprise < ApplicationRecord ENTERPRISE_SEARCH_RADIUS = 100 # The next Rails version will have named variants but we need to store them # ourselves for now. - LOGO_SIZES = { - thumb: { gravity: "Center", resize: "100x100^", crop: '100x100+0+0' }, - small: { gravity: "Center", resize: "180x180^", crop: '180x180+0+0' }, - medium: { gravity: "Center", resize: "300x300^", crop: '300x300+0+0' }, - }.freeze - PROMO_IMAGE_SIZES = { - thumb: { resize_to_limit: [100, 100] }, - medium: { resize_to_fill: [720, 156] }, - large: { resize_to_fill: [1200, 260] }, - }.freeze - WHITE_LABEL_LOGO_SIZES = { - default: { gravity: "Center", resize_to_fill: [217, 44] }, - mobile: { gravity: "Center", resize_to_fill: [75, 26] }, - }.freeze + LOGO_SIZES = [:thumb, :small, :medium].freeze + PROMO_IMAGE_SIZES = [:thumb, :medium, :large].freeze + WHITE_LABEL_LOGO_SIZES = [:default, :mobile].freeze VALID_INSTAGRAM_REGEX = %r{\A[a-zA-Z0-9._]{1,30}([^/-]*)\z} searchable_attributes :sells, :is_primary_producer, :name @@ -89,10 +78,21 @@ class Enterprise < ApplicationRecord } accepts_nested_attributes_for :custom_tab - has_one_attached :logo - has_one_attached :promo_image has_one_attached :terms_and_conditions - has_one_attached :white_label_logo + has_one_attached :logo, service: image_service do |attachment| + attachment.variant :thumb, resize_to_fill: [100, 100], crop: [0, 0, 100, 100] + attachment.variant :small, resize_to_fill: [180, 180], crop: [0, 0, 180, 180] + attachment.variant :medium, resize_to_fill: [300, 300], crop: [0, 0, 300, 300] + end + has_one_attached :promo_image, service: image_service do |attachment| + attachment.variant :thumb, resize_to_limit: [100, 100] + attachment.variant :medium, resize_to_fill: [720, 156] + attachment.variant :large, resize_to_fill: [1200, 260] + end + has_one_attached :white_label_logo, service: image_service do |attachment| + attachment.variant :default, resize_to_fill: [217, 44] + attachment.variant :mobile, resize_to_fill: [75, 26] + end validates :logo, processable_image: true, @@ -297,27 +297,15 @@ class Enterprise < ApplicationRecord end def logo_url(name) - return unless logo.variable? - - Rails.application.routes.url_helpers.url_for( - logo.variant(LOGO_SIZES[name]) - ) + image_url_for(logo, name) end def promo_image_url(name) - return unless promo_image.variable? - - Rails.application.routes.url_helpers.url_for( - promo_image.variant(PROMO_IMAGE_SIZES[name]) - ) + image_url_for(promo_image, name) end def white_label_logo_url(name = :default) - return unless white_label_logo.variable? - - Rails.application.routes.url_helpers.url_for( - white_label_logo.variant(WHITE_LABEL_LOGO_SIZES[name]) - ) + image_url_for(white_label_logo, name) end def website @@ -469,6 +457,17 @@ class Enterprise < ApplicationRecord errors.add(:white_label_logo_link, I18n.t(:invalid_url)) end + def image_url_for(image, name) + return unless image.variable? + return image.variant(name).processed.url if image.attachment.service.name == :amazon_public + + url_for(image.variant(name)) + rescue ActiveStorage::Error => e + Rails.logger.error(e.message) + + nil + end + def current_exchange_variants ExchangeVariant.joins(exchange: :order_cycle) .merge(Exchange.outgoing) diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb index 9616c24722..4221776554 100644 --- a/app/models/enterprise_group.rb +++ b/app/models/enterprise_group.rb @@ -25,8 +25,8 @@ class EnterpriseGroup < ApplicationRecord delegate :phone, :address1, :address2, :city, :zipcode, :state, :country, to: :address - has_one_attached :logo - has_one_attached :promo_image + has_one_attached :logo, service: image_service + has_one_attached :promo_image, service: image_service validates :logo, processable_image: true, diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index e328eea790..f6732cfae7 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -2,14 +2,12 @@ module Spree class Image < Asset - 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 - - has_one_attached :attachment + 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] + attachment.variant :product, resize_to_limit: [240, 240] + attachment.variant :large, resize_to_limit: [600, 600] + end validates :attachment, attached: true, @@ -17,20 +15,33 @@ module Spree content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml|webp)\Z} validate :no_attachment_errors + def self.default_image_url(size) + return "/noimage/product.png" unless size&.to_sym.in?([:mini, :small, :product, :large]) + + "/noimage/#{size}.png" + end + def variant(name) if attachment.variable? - attachment.variant(SIZES[name]) + attachment.variant(name) else attachment end end def url(size) - return unless attachment.attached? + return self.class.default_image_url(size) unless attachment.attached? + return variant(size).processed.url if attachment.service.name == :amazon_public - Rails.application.routes.url_helpers.url_for(variant(size)) + url_for(variant(size)) + rescue ActiveStorage::Error => e + Rails.logger.error(e.message) + + self.class.default_image_url(size) end + private + # if there are errors from the plugin, then add a more meaningful message def no_attachment_errors return if errors[:attachment].empty? diff --git a/app/serializers/api/admin/enterprise_serializer.rb b/app/serializers/api/admin/enterprise_serializer.rb index f28e0a3ef7..30c3d2045b 100644 --- a/app/serializers/api/admin/enterprise_serializer.rb +++ b/app/serializers/api/admin/enterprise_serializer.rb @@ -96,9 +96,9 @@ module Api def attachment_urls(attachment, styles) return unless attachment.variable? - styles.transform_values do |transformation| + styles.index_with do |style| Rails.application.routes.url_helpers. - url_for(attachment.variant(transformation)) + url_for(attachment.variant(style)) end end end diff --git a/app/serializers/api/admin/product_serializer.rb b/app/serializers/api/admin/product_serializer.rb index b313d4297e..6dd930f7da 100644 --- a/app/serializers/api/admin/product_serializer.rb +++ b/app/serializers/api/admin/product_serializer.rb @@ -27,11 +27,11 @@ module Api end def image_url - object.images.first&.url(:product) || "/noimage/product.png" + object.images.first&.url(:product) || Spree::Image.default_image_url(:product) end def thumb_url - object.images.first&.url(:mini) || "/noimage/mini.png" + object.images.first&.url(:mini) || Spree::Image.default_image_url(:mini) end def on_hand diff --git a/app/serializers/api/variant_serializer.rb b/app/serializers/api/variant_serializer.rb index 020a48fb93..79b492b528 100644 --- a/app/serializers/api/variant_serializer.rb +++ b/app/serializers/api/variant_serializer.rb @@ -40,7 +40,7 @@ class Api::VariantSerializer < ActiveModel::Serializer end def thumb_url - object.product.images.first&.url(:mini) || "/noimage/mini.png" + object.product.images.first&.url(:mini) || Spree::Image.default_image_url(:mini) end def unit_price_price diff --git a/app/views/spree/admin/images/edit.html.haml b/app/views/spree/admin/images/edit.html.haml index 711dd32fbb..40ff067b0a 100644 --- a/app/views/spree/admin/images/edit.html.haml +++ b/app/views/spree/admin/images/edit.html.haml @@ -11,9 +11,7 @@ .field.alpha.three.columns.align-center = f.label t('spree.thumbnail') %br/ - - # 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)) + = link_to image_tag(@image.url(:small)), @image.url(: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 978de27092..4757f869de 100644 --- a/app/views/spree/admin/images/index.html.haml +++ b/app/views/spree/admin/images/index.html.haml @@ -32,9 +32,7 @@ %td.no-border %span.handle %td - - # 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)) + = link_to image_tag(image.url(:mini)), image.url(:product) %td= options_text_for(image) %td= image.alt %td.actions diff --git a/app/views/spree/admin/products/new.html.haml b/app/views/spree/admin/products/new.html.haml index 4d770bb4af..28a2bbbd8a 100644 --- a/app/views/spree/admin/products/new.html.haml +++ b/app/views/spree/admin/products/new.html.haml @@ -99,7 +99,7 @@ %fieldset.no-border-bottom{ id: "image" } %legend{align: "center"}= t(".image") .row - = image_tag "/noimage/product.png", class: "four columns alpha" + = image_tag Spree::Image.default_image_url(:product), class: "four columns alpha" .row = f.fields_for 'images_attributes[]', f.object.images.build do |image_fields| = image_fields.file_field :attachment diff --git a/app/views/spree/shared/_variant_thumbnail.html.haml b/app/views/spree/shared/_variant_thumbnail.html.haml index 12c2589984..d97d6175ca 100644 --- a/app/views/spree/shared/_variant_thumbnail.html.haml +++ b/app/views/spree/shared/_variant_thumbnail.html.haml @@ -1 +1 @@ -= image_tag(variant.product.images.first&.url(:mini) || "/noimage/mini.png") += image_tag(variant.product.images.first&.url(:mini) || Spree::Image.default_image_url(:mini)) diff --git a/config/application.rb b/config/application.rb index 7ea51da3e5..c3cb2db1c5 100644 --- a/config/application.rb +++ b/config/application.rb @@ -244,6 +244,7 @@ module Openfoodnetwork config.active_storage.service = ENV["S3_BUCKET"].present? ? :amazon : :local config.active_storage.content_types_to_serve_as_binary -= ["image/svg+xml"] config.active_storage.variable_content_types += ["image/svg+xml"] + config.active_storage.url_options = config.action_controller.default_url_options config.exceptions_app = self.routes end diff --git a/config/storage.yml b/config/storage.yml index 255c8298d9..271782041f 100644 --- a/config/storage.yml +++ b/config/storage.yml @@ -19,3 +19,11 @@ amazon: secret_access_key: <%= ENV["S3_SECRET"] %> bucket: <%= ENV["S3_BUCKET"] %> region: <%= ENV.fetch("S3_REGION", "us-east-1") %> + +amazon_public: + service: S3 + public: true + access_key_id: <%= ENV["S3_ACCESS_KEY"] %> + secret_access_key: <%= ENV["S3_SECRET"] %> + bucket: <%= ENV["S3_BUCKET"] %> + region: <%= ENV.fetch("S3_REGION", "us-east-1") %> diff --git a/db/migrate/20230504001141_public_images.rb b/db/migrate/20230504001141_public_images.rb new file mode 100644 index 0000000000..ea0695f67b --- /dev/null +++ b/db/migrate/20230504001141_public_images.rb @@ -0,0 +1,40 @@ +require 'aws-sdk-s3' + +class PublicImages < ActiveRecord::Migration[7.0] + def up + return unless s3_bucket + + check_connection! + set_objects_to_readable + + ActiveStorage::Blob. + where(service_name: "amazon"). + where("content_type LIKE 'image/%'"). + update_all(service_name: "amazon_public") + end + + def down + ActiveStorage::Blob. + where(service_name: "amazon_public"). + where("content_type LIKE 'image/%'"). + update_all(service_name: "amazon") + end + + private + + # Returns an Aws::S3::Bucket object + def s3_bucket + @s3_bucket ||= ActiveStorage::Blob.where(service_name: "amazon").first&.service&.bucket + end + + # Checks bucket status. Throws an error if connection fails + def check_connection! + s3_bucket.exists? + end + + # Sets bucket objects' ACL to "public-read". Performs batched processing internally + # with a custom enumerator, see AWS::Resources::Collection#each for details. + def set_objects_to_readable + s3_bucket.objects.each{|object| object.acl.put(acl: "public-read") } + end +end diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index 40481cf1ea..d6ac3b5b0c 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -31,10 +31,46 @@ module Spree ) end - it "returns nil when the attachment is missing" do + it "returns default image when the attachment is missing" do subject.attachment = nil - expect(subject.url(:small)).to eq nil + expect(subject.url(:small)).to eq "/noimage/small.png" + end + + context "when no image attachment is found" do + it "returns a default product image" do + expect(subject).to receive_message_chain(:attachment, :attached?) { false } + + expect(subject.url(:mini)).to eq "/noimage/mini.png" + end + end + + context "when accessing the image raises an ActiveStorage error" do + it "rescues the error and returns a default product image" do + expect(subject).to receive(:attachment) { raise ActiveStorage::FileNotFoundError } + + expect(subject.url(:small)).to eq "/noimage/small.png" + end + end + + context "when using public images" do + it "returns the direct URL for the processed image" do + allow(subject).to receive_message_chain(:attachment, :attached?) { true } + expect(subject).to receive_message_chain(:attachment, :service, :name) { :amazon_public } + expect(subject).to receive_message_chain(:variant, :processed, :url) { "https://ofn-s3/123.png" } + + expect(subject.url(:small)).to eq "https://ofn-s3/123.png" + end + end + end + + describe "#default_image_url" do + it "returns default product image for a given size" do + expect(subject.class.default_image_url(:mini)).to eq "/noimage/mini.png" + end + + it "returns default product image when no size is given" do + expect(subject.class.default_image_url(nil)).to eq "/noimage/product.png" end end