Merge pull request #10804 from Matt-Yorkley/public-images

Update ActiveStorage image processing

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'"}

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

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)

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,

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?

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

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

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

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

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

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

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))

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

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") %>

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

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