diff --git a/app/assets/javascripts/admin/products/services/product_image_service.js.coffee b/app/assets/javascripts/admin/products/services/product_image_service.js.coffee index 3d36d0a914..40e7ab43c0 100644 --- a/app/assets/javascripts/admin/products/services/product_image_service.js.coffee +++ b/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) diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb index fbceddb4fe..97e4b2d42c 100644 --- a/app/controllers/api/v0/product_images_controller.rb +++ b/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 diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 5d7892f3ee..bd49da2bdc 100644 --- a/app/models/enterprise.rb +++ b/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), diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb index dcce2ab192..60d2516890 100644 --- a/app/models/enterprise_group.rb +++ b/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) } diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index da48229f80..25eca61bf8 100644 --- a/app/models/spree/image.rb +++ b/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 diff --git a/config/application.rb b/config/application.rb index 7d88b45db8..f71167fd30 100644 --- a/config/application.rb +++ b/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 diff --git a/spec/controllers/api/v0/product_images_controller_spec.rb b/spec/controllers/api/v0/product_images_controller_spec.rb index cf658afc1e..c40785ba2c 100644 --- a/spec/controllers/api/v0/product_images_controller_spec.rb +++ b/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 diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index 52debc535a..40481cf1ea 100644 --- a/spec/models/spree/image_spec.rb +++ b/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 diff --git a/spec/views/spree/orders/show.html.haml_spec.rb b/spec/views/spree/orders/show.html.haml_spec.rb index 1b9a2882cf..6980b58caf 100644 --- a/spec/views/spree/orders/show.html.haml_spec.rb +++ b/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