From 831aa3aae0953f7350e22a4027f1e7dba646bb9e Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Jun 2022 14:50:55 +1000 Subject: [PATCH 1/9] Simplify ProductImagesController --- .../api/v0/product_images_controller.rb | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb index fbceddb4fe..b752755a10 100644 --- a/app/controllers/api/v0/product_images_controller.rb +++ b/app/controllers/api/v0/product_images_controller.rb @@ -6,21 +6,19 @@ module Api respond_to :json def update_product_image - @product = Spree::Product.find(params[:product_id]) + 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 - else - @image = @product.images.first - @image.update(attachment: params[:file]) - render json: @image, serializer: ImageSerializer, status: :ok - end + image = product.images.first || Spree::Image.new( + viewable_id: product.master.id, + viewable_type: 'Spree::Variant' + ) + + success_status = image.persisted? ? :ok : :created + + image.update(attachment: params[:file]) + + render json: image, serializer: ImageSerializer, status: success_status end end end From 6b733ad7e2e3ba8909d164b826dc3205bfd3b6e4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Jun 2022 14:56:40 +1000 Subject: [PATCH 2/9] Increase readability of image controller spec Best viewed without whitespace changes. - Decrease indent. - Move `let` to top like in other specs. - Avoid `let!` to speed up the specs. --- .../api/v0/product_images_controller_spec.rb | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/spec/controllers/api/v0/product_images_controller_spec.rb b/spec/controllers/api/v0/product_images_controller_spec.rb index cf658afc1e..d4b5f93fba 100644 --- a/spec/controllers/api/v0/product_images_controller_spec.rb +++ b/spec/controllers/api/v0/product_images_controller_spec.rb @@ -2,37 +2,37 @@ 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(: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 end end From 5f11b6a650d0e6e052e3067becfed1bf2850aed1 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Jun 2022 15:06:17 +1000 Subject: [PATCH 3/9] Respond with errors if image upload fails --- app/controllers/api/v0/product_images_controller.rb | 9 ++++++--- .../api/v0/product_images_controller_spec.rb | 13 +++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb index b752755a10..1df2664224 100644 --- a/app/controllers/api/v0/product_images_controller.rb +++ b/app/controllers/api/v0/product_images_controller.rb @@ -16,9 +16,12 @@ module Api success_status = image.persisted? ? :ok : :created - image.update(attachment: params[:file]) - - render json: image, serializer: ImageSerializer, status: success_status + if image.update(attachment: params[:file]) + render json: image, serializer: ImageSerializer, status: success_status + else + error_json = { errors: image.errors.full_messages } + render json: error_json, status: :unprocessable_entity + end end 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 d4b5f93fba..c40785ba2c 100644 --- a/spec/controllers/api/v0/product_images_controller_spec.rb +++ b/spec/controllers/api/v0/product_images_controller_spec.rb @@ -9,6 +9,8 @@ describe Api::V0::ProductImagesController, type: :controller do 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) } @@ -34,5 +36,16 @@ describe Api::V0::ProductImagesController, type: :controller do 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 From f3f0a8491510cb72673f62aa010d1ed69e4866b4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Jun 2022 15:07:46 +1000 Subject: [PATCH 4/9] Display image upload error as alert Not the nicest UX but better than nothing. --- .../admin/products/services/product_image_service.js.coffee | 2 ++ 1 file changed, 2 insertions(+) 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..653b714904 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,5 @@ 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) => + alert(response.errors.join("\n")) From 004c7eef9eebfae97ce735f1cb382176e6724a66 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 14 Jun 2022 15:07:46 +1000 Subject: [PATCH 5/9] Show SVG product images --- app/models/spree/image.rb | 8 ++++++-- config/application.rb | 1 + spec/models/spree/image_spec.rb | 10 +++++++++- spec/views/spree/orders/show.html.haml_spec.rb | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index da48229f80..761fc62807 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -15,11 +15,15 @@ module Spree 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..940fc797de 100644 --- a/config/application.rb +++ b/config/application.rb @@ -240,5 +240,6 @@ 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"] 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 From 073bad211831bd46dcea1720f7382f74c029405d Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 12 Jul 2022 15:53:32 +0200 Subject: [PATCH 6/9] Allow svg images to be resized/cropped --- config/application.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/application.rb b/config/application.rb index 940fc797de..f71167fd30 100644 --- a/config/application.rb +++ b/config/application.rb @@ -241,5 +241,6 @@ 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"] end end From a80cf37fa6c64d5177d5c2049bf31923c3f18f56 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 12 Jul 2022 18:29:28 +0200 Subject: [PATCH 7/9] Handle errors message formatting --- .../admin/products/services/product_image_service.js.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 653b714904..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 @@ -14,4 +14,8 @@ angular.module("ofn.admin").factory "ProductImageService", (FileUploader, SpreeA product.thumb_url = response.thumb_url product.image_url = response.image_url @imageUploader.onErrorItem = (image, response) => - alert(response.errors.join("\n")) + if Array.isArray(response.errors) + message = response.errors.join("\n") + else + message = response.error.toString() + alert(message) From afa40ea82fe954192117d3f3ec0fd0e1c00281ce Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 12 Jul 2022 18:49:15 +0200 Subject: [PATCH 8/9] Authorize `product` and not `@product` --- app/controllers/api/v0/product_images_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/v0/product_images_controller.rb b/app/controllers/api/v0/product_images_controller.rb index 1df2664224..97e4b2d42c 100644 --- a/app/controllers/api/v0/product_images_controller.rb +++ b/app/controllers/api/v0/product_images_controller.rb @@ -7,7 +7,7 @@ module Api def update_product_image product = Spree::Product.find(params[:product_id]) - authorize! :update, @product + authorize! :update, product image = product.images.first || Spree::Image.new( viewable_id: product.master.id, From 2b67a0fa8035221e3a77ad78bbc1b032b7ecf058 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Wed, 20 Jul 2022 17:26:44 +0200 Subject: [PATCH 9/9] Specify a list of content-types Then remove specific image format that aren't handled by a web browser (such as `image/x+xcf)` + List allowed image formats for enterprises and groups --- app/models/enterprise.rb | 4 ++-- app/models/enterprise_group.rb | 4 ++-- app/models/spree/image.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 461f08f919..2a991dbc68 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 761fc62807..25eca61bf8 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -11,7 +11,7 @@ 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)