Merge pull request #9301 from mkllnk/3975-unsupported-image

Allow SVG images for products and show errors when uploading unsupported file

app/assets/javascripts/admin/products/services/product_image_service.js.coffee

@@ -13,3 +13,9 @@ angular.module("ofn.admin").factory "ProductImageService", (FileUploader, SpreeA
       @imageUploader.onSuccessItem = (image, response) =>
         product.thumb_url = response.thumb_url
         product.image_url = response.image_url
+      @imageUploader.onErrorItem = (image, response) =>
+        if Array.isArray(response.errors)
+          message = response.errors.join("\n")
+        else
+          message = response.error.toString()
+        alert(message)

app/controllers/api/v0/product_images_controller.rb

@@ -6,20 +6,21 @@ module Api
       respond_to :json
 
       def update_product_image
-        @product = Spree::Product.find(params[:product_id])
-        authorize! :update, @product
+        product = Spree::Product.find(params[:product_id])
+        authorize! :update, product
 
-        if @product.images.first.nil?
-          @image = Spree::Image.create(
-            attachment: params[:file],
-            viewable_id: @product.master.id,
-            viewable_type: 'Spree::Variant'
-          )
-          render json: @image, serializer: ImageSerializer, status: :created
+        image = product.images.first || Spree::Image.new(
+          viewable_id: product.master.id,
+          viewable_type: 'Spree::Variant'
+        )
+
+        success_status = image.persisted? ? :ok : :created
+
+        if image.update(attachment: params[:file])
+          render json: image, serializer: ImageSerializer, status: success_status
         else
-          @image = @product.images.first
-          @image.update(attachment: params[:file])
-          render json: @image, serializer: ImageSerializer, status: :ok
+          error_json = { errors: image.errors.full_messages }
+          render json: error_json, status: :unprocessable_entity
         end
       end
     end

app/models/enterprise.rb

@@ -84,8 +84,8 @@ class Enterprise < ApplicationRecord
   has_one_attached :promo_image
   has_one_attached :terms_and_conditions
 
-  validates :logo, content_type: %r{\Aimage/.*\Z}
-  validates :promo_image, content_type: %r{\Aimage/.*\Z}
+  validates :logo, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml)\Z}
+  validates :promo_image, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml)\Z}
   validates :terms_and_conditions, content_type: {
     in: "application/pdf",
     message: I18n.t(:enterprise_terms_and_conditions_type_error),

app/models/enterprise_group.rb

@@ -28,8 +28,8 @@ class EnterpriseGroup < ApplicationRecord
   has_one_attached :logo
   has_one_attached :promo_image
 
-  validates :logo, content_type: %r{\Aimage/.*\Z}
-  validates :promo_image, content_type: %r{\Aimage/.*\Z}
+  validates :logo, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml)\Z}
+  validates :promo_image, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml)\Z}
 
   scope :by_position, -> { order('position ASC') }
   scope :on_front_page, -> { where(on_front_page: true) }

app/models/spree/image.rb

@@ -11,15 +11,19 @@ module Spree
 
     has_one_attached :attachment
 
-    validates :attachment, attached: true, content_type: %r{\Aimage/.*\Z}
+    validates :attachment, attached: true, content_type: %r{\Aimage/(png|jpeg|gif|jpg|svg\+xml)\Z}
     validate :no_attachment_errors
 
     def variant(name)
-      attachment.variant(SIZES[name])
+      if attachment.variable?
+        attachment.variant(SIZES[name])
+      else
+        attachment
+      end
     end
 
     def url(size)
-      return unless attachment.variable?
+      return unless attachment.attached?
 
       Rails.application.routes.url_helpers.url_for(variant(size))
     end

config/application.rb

@@ -240,5 +240,7 @@ module Openfoodnetwork
     Rails.autoloaders.main.ignore(Rails.root.join('app/webpacker'))
 
     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"]
   end
 end

spec/controllers/api/v0/product_images_controller_spec.rb

@@ -2,37 +2,50 @@
 
 require 'spec_helper'
 
-module Api
-  describe V0::ProductImagesController, type: :controller do
-    include AuthenticationHelper
-    include FileHelper
-    render_views
+describe Api::V0::ProductImagesController, type: :controller do
+  include AuthenticationHelper
+  include FileHelper
+  render_views
 
-    describe "uploading an image" do
-      before do
-        allow(controller).to receive(:spree_current_user) { current_api_user }
-      end
+  describe "uploading an image" do
+    let(:image) { Rack::Test::UploadedFile.new(black_logo_file, 'image/png') }
+    let(:pdf) { Rack::Test::UploadedFile.new(pdf_path, 'application/pdf') }
+    let(:pdf_path) { Rails.root.join("public/Terms-of-service.pdf") }
+    let(:product_without_image) { create(:product) }
+    let(:product_with_image) { create(:product_with_image) }
+    let(:current_api_user) { create(:admin_user) }
 
-      let(:image) { Rack::Test::UploadedFile.new(black_logo_file, 'image/png') }
-      let!(:product_without_image) { create(:product) }
-      let!(:product_with_image) { create(:product_with_image) }
-      let(:current_api_user) { create(:admin_user) }
+    before do
+      allow(controller).to receive(:spree_current_user) { current_api_user }
+    end
 
-      it "saves a new image when none is present" do
-        post :update_product_image, xhr: true,
-                                    params: { product_id: product_without_image.id, file: image, use_route: :product_images }
+    it "saves a new image when none is present" do
+      post :update_product_image, xhr: true, params: {
+        product_id: product_without_image.id, file: image, use_route: :product_images
+      }
 
-        expect(response.status).to eq 201
-        expect(product_without_image.images.first.id).to eq json_response['id']
-      end
+      expect(response.status).to eq 201
+      expect(product_without_image.images.first.id).to eq json_response['id']
+    end
 
-      it "updates an existing product image" do
-        post :update_product_image, xhr: true,
-                                    params: { product_id: product_with_image.id, file: image, use_route: :product_images }
+    it "updates an existing product image" do
+      post :update_product_image, xhr: true, params: {
+        product_id: product_with_image.id, file: image, use_route: :product_images
+      }
 
-        expect(response.status).to eq 200
-        expect(product_with_image.images.first.id).to eq json_response['id']
-      end
+      expect(response.status).to eq 200
+      expect(product_with_image.images.first.id).to eq json_response['id']
+    end
+
+    it "reports errors when saving fails" do
+      post :update_product_image, xhr: true, params: {
+        product_id: product_without_image.id, file: pdf, use_route: :product_images
+      }
+
+      expect(response.status).to eq 422
+      expect(product_without_image.images.count).to eq 0
+      expect(json_response["id"]).to eq nil
+      expect(json_response["errors"]).to include "Attachment has an invalid content type"
     end
   end
 end

spec/models/spree/image_spec.rb

@@ -21,11 +21,19 @@ module Spree
         )
       end
 
-      it "returns nil for unsupported formats" do
+      it "returns download link for unsupported formats" do
         subject.attachment_blob.update_columns(
           content_type: "application/octet-stream"
         )
 
+        expect(subject.url(:small)).to match(
+          %r|^http://test\.host/rails/active_storage/blobs/redirect/.+/logo-black\.png$|
+        )
+      end
+
+      it "returns nil when the attachment is missing" do
+        subject.attachment = nil
+
         expect(subject.url(:small)).to eq nil
       end
     end

spec/views/spree/orders/show.html.haml_spec.rb

@@ -51,7 +51,7 @@ describe "spree/orders/show.html.haml" do
 
     render
 
-    expect(rendered).to have_no_css("img[src*='logo.png']")
+    expect(rendered).to have_css("img[src*='logo.png']")
     expect(rendered).to have_content("R123456789")
   end
 end