From 727eef3c4fbbdbccbd5d0d928408f1128ea5b697 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 6 Apr 2022 14:48:06 +1000 Subject: [PATCH 01/10] Replace Paperclippable ContentConfig The old Paperclip configuration was very clever and easy to use but it was also a complicated implementation building on the complicated Spree preference system. I simplified this with Active Storage, storing simple references to blob ids and default URLs as backup. --- app/controllers/admin/contents_controller.rb | 26 ++++++-- app/models/concerns/file_preferences.rb | 43 +++++++++++++ app/models/content_configuration.rb | 21 +++---- .../spree/preferences/file_configuration.rb | 62 ------------------- app/views/admin/contents/_fieldset.html.haml | 5 +- app/views/home/index.html.haml | 2 +- app/views/layouts/darkswarm.html.haml | 2 +- app/views/layouts/mailer.html.haml | 2 +- app/views/shared/_footer.html.haml | 2 +- app/views/shared/menu/_large_menu.html.haml | 2 +- app/views/shared/menu/_mobile_menu.html.haml | 2 +- .../shared/menu/_offcanvas_menu.html.haml | 2 +- lib/open_food_network/paperclippable.rb | 51 --------------- spec/models/content_configuration_spec.rb | 8 +-- .../preferences/file_configuration_spec.rb | 59 ------------------ 15 files changed, 86 insertions(+), 203 deletions(-) create mode 100644 app/models/concerns/file_preferences.rb delete mode 100644 app/models/spree/preferences/file_configuration.rb delete mode 100644 lib/open_food_network/paperclippable.rb delete mode 100644 spec/models/spree/preferences/file_configuration_spec.rb diff --git a/app/controllers/admin/contents_controller.rb b/app/controllers/admin/contents_controller.rb index 5841fa84eb..0a41cc389a 100644 --- a/app/controllers/admin/contents_controller.rb +++ b/app/controllers/admin/contents_controller.rb @@ -10,14 +10,14 @@ module Admin def update params.each do |name, value| - if ContentConfig.has_preference?(name) || ContentConfig.has_attachment?(name) - ContentConfig.public_send("#{name}=", value) + if value.is_a?(ActionDispatch::Http::UploadedFile) + blob = store_file(value) + update_preference("#{name}_blob_id", blob.id) + else + update_preference(name, value) end end - # Save any uploaded images - ContentConfig.save - flash[:success] = t(:successfully_updated, resource: I18n.t('admin.contents.edit.your_content')) @@ -26,6 +26,22 @@ module Admin private + def store_file(attachable) + ActiveStorage::Blob.create_and_upload!( + io: attachable.open, + filename: attachable.original_filename, + content_type: attachable.content_type, + service_name: :local, + identify: false, + ) + end + + def update_preference(name, value) + return unless ContentConfig.has_preference?(name) + + ContentConfig.public_send("#{name}=", value) + end + def preference_sections [ PreferenceSections::HeaderSection.new, diff --git a/app/models/concerns/file_preferences.rb b/app/models/concerns/file_preferences.rb new file mode 100644 index 0000000000..68682b3263 --- /dev/null +++ b/app/models/concerns/file_preferences.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module FilePreferences + extend ActiveSupport::Concern + + included do + @default_urls = {} + end + + class_methods do + def file_preference(name, default_url: nil) + preference "#{name}_blob_id", :integer + @default_urls[name] = default_url if default_url + end + + def default_url(name) + @default_urls[name] + end + end + + def preference_type(key) + if has_preference?("#{key}_blob_id") + :file + else + super(key) + end + end + + def url_for(name) + blob = blob_for(name) + + if blob + Rails.application.routes.url_helpers.url_for(blob) + else + self.class.default_url(name) + end + end + + def blob_for(name) + blob_id = get_preference("#{name}_blob_id") + ActiveStorage::Blob.find_by(id: blob_id) if blob_id + end +end diff --git a/app/models/content_configuration.rb b/app/models/content_configuration.rb index 0f34598cd7..d0160407a4 100644 --- a/app/models/content_configuration.rb +++ b/app/models/content_configuration.rb @@ -1,23 +1,17 @@ # frozen_string_literal: true -require 'open_food_network/paperclippable' - -class ContentConfiguration < Spree::Preferences::FileConfiguration - include OpenFoodNetwork::Paperclippable +class ContentConfiguration < Spree::Preferences::Configuration + include FilePreferences # Header - preference :logo, :file - preference :logo_mobile, :file - preference :logo_mobile_svg, :file - has_attached_file :logo, default_url: "/default_images/ofn-logo.png" - has_attached_file :logo_mobile - has_attached_file :logo_mobile_svg, default_url: "/default_images/ofn-logo-mobile.svg" + file_preference :logo, default_url: "/default_images/ofn-logo.png" + file_preference :logo_mobile + file_preference :logo_mobile_svg, default_url: "/default_images/ofn-logo-mobile.svg" # Home page preference :home_page_alert_html, :text - preference :home_hero, :file + file_preference :home_hero, default_url: "/default_images/home.jpg" preference :home_show_stats, :boolean, default: true - has_attached_file :home_hero, default_url: "/default_images/home.jpg" # Map preference :open_street_map_enabled, :boolean, default: false @@ -66,8 +60,7 @@ class ContentConfiguration < Spree::Preferences::FileConfiguration preference :menu_7_icon_name, :string, default: "ofn-i_013-help" # Footer - preference :footer_logo, :file - has_attached_file :footer_logo, default_url: "/default_images/ofn-logo-footer.png" + file_preference :footer_logo, default_url: "/default_images/ofn-logo-footer.png" # Other preference :footer_facebook_url, :string, default: "https://www.facebook.com/OpenFoodNet" diff --git a/app/models/spree/preferences/file_configuration.rb b/app/models/spree/preferences/file_configuration.rb deleted file mode 100644 index 421484993f..0000000000 --- a/app/models/spree/preferences/file_configuration.rb +++ /dev/null @@ -1,62 +0,0 @@ -# frozen_string_literal: true - -module Spree - module Preferences - class FileConfiguration < Configuration - def self.preference(name, type, *args) - if type == :file - # Active Storage blob id: - super "#{name}_blob_id", :integer, *args - - # Paperclip attachment attributes: - super "#{name}_file_name", :string, *args - super "#{name}_content_type", :string, *args - super "#{name}_file_size", :integer, *args - super "#{name}_updated_at", :string, *args - - else - super name, type, *args - end - end - - def get_preference(key) - if !has_preference?(key) && has_attachment?(key) - # Call Paperclip's attachment method: - public_send key - elsif key.ends_with?("_blob") - # Find referenced Active Storage blob: - blob_id = super("#{key}_id") - ActiveStorage::Blob.find_by(id: blob_id) - else - super key - end - end - alias :[] :get_preference - - def preference_type(name) - if has_attachment? name - :file - else - super name - end - end - - # Spree's Configuration responds to preference methods via method_missing, but doesn't - # override respond_to?, which consequently reports those methods as unavailable. Paperclip - # errors if respond_to? isn't correct, so we override it here. - def respond_to?(method, include_all = false) - name = method.to_s.delete('=') - reference_name = "#{name}_id" - - super(self.class.preference_getter_method(name), include_all) || - super(reference_name, include_all) || - super(method, include_all) - end - - def has_attachment?(name) - self.class.respond_to?(:attachment_definitions) && - self.class.attachment_definitions.key?(name.to_sym) - end - end - end -end diff --git a/app/views/admin/contents/_fieldset.html.haml b/app/views/admin/contents/_fieldset.html.haml index 509bf49f43..0821e4e837 100644 --- a/app/views/admin/contents/_fieldset.html.haml +++ b/app/views/admin/contents/_fieldset.html.haml @@ -8,5 +8,8 @@ - text = t(key) .field = label_tag(key, text + ': ') + tag(:br) if type != :boolean - = preference_field_tag(key, ContentConfig[key], :type => type) + - if type == :file + = file_field_tag(key, type: type) + - else + = preference_field_tag(key, ContentConfig[key], type: type) = label_tag(key, text) + tag(:br) if type == :boolean diff --git a/app/views/home/index.html.haml b/app/views/home/index.html.haml index 49da911e20..899e19ca72 100644 --- a/app/views/home/index.html.haml +++ b/app/views/home/index.html.haml @@ -1,5 +1,5 @@ :css - #tagline:before { background-image: url("#{ContentConfig.home_hero.url}") } + #tagline:before { background-image: url("#{ContentConfig.url_for(:home_hero)}") } - content_for :page_alert do diff --git a/app/views/layouts/darkswarm.html.haml b/app/views/layouts/darkswarm.html.haml index 0607ec2cb5..b213f52c07 100644 --- a/app/views/layouts/darkswarm.html.haml +++ b/app/views/layouts/darkswarm.html.haml @@ -4,7 +4,7 @@ %meta{name: 'viewport', content: "width=device-width,initial-scale=1.0"}/ %meta{property: "og:title", content: content_for?(:title) ? yield(:title) : t(:title)} %meta{property: "og:description", content: content_for?(:description) ? yield(:description) : t(:site_meta_description)} - %meta{property: "og:image", content: content_for?(:image) ? yield(:image) : ContentConfig.logo.url} + %meta{property: "og:image", content: content_for?(:image) ? yield(:image) : ContentConfig.url_for(:logo)} - if !Rails.env.production? || @noindex_meta_tag %meta{name: "robots", content: "noindex"} %title= content_for?(:title) ? "#{yield(:title)} - #{t(:title)}".html_safe : "#{t(:welcome_to)} #{t(:title)}" diff --git a/app/views/layouts/mailer.html.haml b/app/views/layouts/mailer.html.haml index 2536308b57..0160dfd727 100644 --- a/app/views/layouts/mailer.html.haml +++ b/app/views/layouts/mailer.html.haml @@ -15,7 +15,7 @@ %table{:bgcolor => "#f2f2f2"} %tr %td - %img{src: ContentConfig.footer_logo.url, width: "144", height: "50"}/ + %img{src: ContentConfig.url_for(:footer_logo), width: "144", height: "50"}/ %td{:align => "right"} %h6.collapse = Spree::Config[:site_name] diff --git a/app/views/shared/_footer.html.haml b/app/views/shared/_footer.html.haml index 4e7a5d05e1..4d6723892d 100644 --- a/app/views/shared/_footer.html.haml +++ b/app/views/shared/_footer.html.haml @@ -96,7 +96,7 @@ .row.legal .small-12.medium-3.medium-offset-2.columns.text-left %a{href: main_app.root_path} - %img{src: ContentConfig.footer_logo.url, width: "220"} + %img{src: ContentConfig.url_for(:footer_logo), width: "220"} .small-12.medium-5.columns.text-left %p.text-small = t '.footer_legal_call' diff --git a/app/views/shared/menu/_large_menu.html.haml b/app/views/shared/menu/_large_menu.html.haml index 1b48aa6a72..d8cb828ca6 100644 --- a/app/views/shared/menu/_large_menu.html.haml +++ b/app/views/shared/menu/_large_menu.html.haml @@ -3,7 +3,7 @@ %ul.nav-logo %li.ofn-logo %a{href: main_app.root_path} - %img{src: ContentConfig.logo.url} + %img{src: ContentConfig.url_for(:logo)} %li.powered-by %img{src: '/favicon.ico'} %span diff --git a/app/views/shared/menu/_mobile_menu.html.haml b/app/views/shared/menu/_mobile_menu.html.haml index 83c3d1c44d..aca1fa70d6 100644 --- a/app/views/shared/menu/_mobile_menu.html.haml +++ b/app/views/shared/menu/_mobile_menu.html.haml @@ -6,7 +6,7 @@ %section.left .ofn-logo %a{href: main_app.root_path} - %img{src: ContentConfig.logo_mobile.url, srcset: ContentConfig.logo_mobile_svg.url, width: "75", height: "26"} + %img{src: ContentConfig.url_for(:logo_mobile), srcset: ContentConfig.url_for(:logo_mobile_svg), width: "75", height: "26"} %section.right{"ng-cloak" => true} %span.cart-span{"ng-class" => "{ dirty: Cart.dirty || Cart.empty(), 'pure-dirty': Cart.dirty }"} diff --git a/app/views/shared/menu/_offcanvas_menu.html.haml b/app/views/shared/menu/_offcanvas_menu.html.haml index b6e553bc8c..ca8b907cf7 100644 --- a/app/views/shared/menu/_offcanvas_menu.html.haml +++ b/app/views/shared/menu/_offcanvas_menu.html.haml @@ -2,7 +2,7 @@ %ul.off-canvas-list %li.ofn-logo %a{href: main_app.root_path} - %img{src: ContentConfig.logo_mobile.url, srcset: ContentConfig.logo_mobile_svg.url, width: "75", height: "26"} + %img{src: ContentConfig.url_for(:logo_mobile), srcset: ContentConfig.url_for(:logo_mobile_svg), width: "75", height: "26"} - [*1..7].each do |menu_number| - menu_name = "menu_#{menu_number}" - if ContentConfig[menu_name].present? diff --git a/lib/open_food_network/paperclippable.rb b/lib/open_food_network/paperclippable.rb deleted file mode 100644 index e510c6af82..0000000000 --- a/lib/open_food_network/paperclippable.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -# Allow use of Paperclip's has_attached_file on non-ActiveRecord classes -# https://gist.github.com/basgys/5712426 - -module OpenFoodNetwork - module Paperclippable - def self.included(base) - base.extend(ActiveModel::Naming) - base.extend(ActiveModel::Callbacks) - base.include(ActiveModel::Validations) - base.include(Paperclip::Glue) - - # Paperclip required callbacks - base.define_model_callbacks(:save, only: [:after]) - base.define_model_callbacks(:commit, only: [:after]) - base.define_model_callbacks(:destroy, only: [:before, :after]) - - # Initialise an ID - base.__send__(:attr_accessor, :id) - base.instance_variable_set :@id, 1 - end - - # ActiveModel requirements - def to_model - self - end - - def valid?() true end - - def new_record?() true end - - def destroyed?() true end - - def save - run_callbacks :save do - end - true - end - - def errors - obj = Object.new - def obj.[](_key) [] end - - def obj.full_messages() [] end - - def obj.any?() false end - obj - end - end -end diff --git a/spec/models/content_configuration_spec.rb b/spec/models/content_configuration_spec.rb index 8d2cc5a8d0..141d4111cf 100644 --- a/spec/models/content_configuration_spec.rb +++ b/spec/models/content_configuration_spec.rb @@ -5,10 +5,10 @@ require 'spec_helper' describe ContentConfiguration do describe "default logos and home_hero" do it "sets a default url with existing image" do - expect(image_exist?(ContentConfig.logo.options[:default_url])).to be true - expect(image_exist?(ContentConfig.logo_mobile_svg.options[:default_url])).to be true - expect(image_exist?(ContentConfig.home_hero.options[:default_url])).to be true - expect(image_exist?(ContentConfig.footer_logo.options[:default_url])).to be true + expect(image_exist?(ContentConfig.url_for(:logo))).to be true + expect(image_exist?(ContentConfig.url_for(:logo_mobile_svg))).to be true + expect(image_exist?(ContentConfig.url_for(:home_hero))).to be true + expect(image_exist?(ContentConfig.url_for(:footer_logo))).to be true end def image_exist?(default_url) diff --git a/spec/models/spree/preferences/file_configuration_spec.rb b/spec/models/spree/preferences/file_configuration_spec.rb deleted file mode 100644 index b68eeb270e..0000000000 --- a/spec/models/spree/preferences/file_configuration_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -module Spree - module Preferences - class TestConfiguration < FileConfiguration - preference :name, :string - - include OpenFoodNetwork::Paperclippable - preference :logo, :file - has_attached_file :logo - end - - describe FileConfiguration do - let(:c) { TestConfiguration.new } - - describe "getting preferences" do - it "returns regular preferences" do - c.name = 'foo' - expect(c.get_preference(:name)).to eq('foo') - end - - it "returns file preferences" do - expect(c.get_preference(:logo)).to be_a Paperclip::Attachment - end - - it "returns regular preferences via []" do - c.name = 'foo' - expect(c[:name]).to eq('foo') - end - - it "returns file preferences via []" do - expect(c[:logo]).to be_a Paperclip::Attachment - end - end - - describe "getting preference types" do - it "returns regular preference types" do - expect(c.preference_type(:name)).to eq(:string) - end - - it "returns file preference types" do - expect(c.preference_type(:logo)).to eq(:file) - end - end - - describe "respond_to?" do - it "responds to preference getters" do - expect(c.respond_to?(:name)).to be true - end - - it "responds to preference setters" do - expect(c.respond_to?(:name=)).to be true - end - end - end - end -end From f29e569f1bc4eac68524f96d9163399a1dd9bf60 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 7 Apr 2022 14:43:54 +1000 Subject: [PATCH 02/10] Remove Paperclip migration code --- .../from_paperclip_to_active_storage.rake | 136 ------------------ ...m_paperclip_to_active_storage_rake_spec.rb | 117 --------------- 2 files changed, 253 deletions(-) delete mode 100644 lib/tasks/from_paperclip_to_active_storage.rake delete mode 100644 spec/lib/tasks/from_paperclip_to_active_storage_rake_spec.rb diff --git a/lib/tasks/from_paperclip_to_active_storage.rake b/lib/tasks/from_paperclip_to_active_storage.rake deleted file mode 100644 index edd0a2e2ee..0000000000 --- a/lib/tasks/from_paperclip_to_active_storage.rake +++ /dev/null @@ -1,136 +0,0 @@ -# frozen_string_literal: true - -namespace :from_paperclip_to_active_storage do - # This migration can't be a pure database migration because we need to know - # the location of current files which is computed by Paperclip depending on - # the `url` option. - desc "Copy data to Active Storage tables referencing Paperclip files" - task migrate: :environment do - Rails.application.eager_load! - - HasMigratingFile.migrating_models.each do |model_name| - puts "Migrating #{model_name}" - migrate_model(model_name.constantize) - end - end - - # We have a special class called ContentConfiguration which is not a model - # and therfore can't use the normal Active Storage magic. - # - # It uses `Spree::Preference`s to store all the Paperclip attributes. These - # files are stored locally and we can replace them with preferences pointing - # to an Active Storage blob. - desc "Associate ContentConfig to ActiveStorage blobs" - task copy_content_config: :environment do - [ - :logo, - :logo_mobile, - :logo_mobile_svg, - :home_hero, - :footer_logo, - ].each do |name| - migrate_content_config_file(name) - end - end - - def migrate_model(model) - duplicated_attachment_names(model).each do |name| - migrate_attachment(model, name) - end - end - - def migrate_attachment(model, name) - records_to_migrate = missing_active_storage_attachment(model, name) - - print " - #{name} (#{records_to_migrate.count}) " - - records_to_migrate.find_each do |record| - attach_paperclip(name, record) - end - - puts "" - end - - def attach_paperclip(name, record) - paperclip = record.public_send(name) - - if paperclip.respond_to?(:s3_object) - attachment = storage_record_for(name, paperclip) - record.public_send("#{name}_attachment=", attachment) - print "." - elsif File.exist?(paperclip.path) - record.attach_file(name, File.open(paperclip.path)) - record.save! - print "." - else - print "x" - end - rescue StandardError => e - puts "x" - puts e.message - end - - # Creates an Active Storage record pointing to the same file Paperclip - # stored on AWS S3. Getting the checksum requires a HEAD request. - # In my tests, I could process 100 records per minute this way. - def storage_record_for(name, paperclip) - checksum = hex_to_base64_digest(paperclip.s3_object(:original).etag.delete('"')) - - blob = ActiveStorage::Blob.new( - key: paperclip.path(:original), - filename: paperclip.original_filename, - content_type: paperclip.content_type, - metadata: {}, - byte_size: paperclip.size, - checksum: checksum, - created_at: paperclip.updated_at, - ) - ActiveStorage::Attachment.new( - name: name, - blob: blob, - created_at: paperclip.updated_at, - ) - end - - def migrate_content_config_file(name) - paperclip = ContentConfig.public_send(name) - - return if ContentConfig.public_send("#{name}_blob") - return if paperclip.path.blank? || !paperclip.exists? - - blob = ActiveStorage::Blob.create_and_upload!( - io: File.open(paperclip.path), - filename: paperclip.original_filename, - content_type: paperclip.content_type, - identify: false, - ) - - ContentConfig.public_send("#{name}_blob_id=", blob.id) - puts "Copied #{name}" - end - - def duplicated_attachment_names(model) - paperclip_attachments = model.attachment_definitions.keys.map(&:to_s) - active_storage_attachments = model.attachment_reflections.keys - - only_paperclip = paperclip_attachments - active_storage_attachments - only_active_storage = active_storage_attachments - paperclip_attachments - both = paperclip_attachments & active_storage_attachments - - puts "WARNING: not migrating #{only_paperclip}" if only_paperclip.present? - puts "WARNING: no source for #{only_active_storage}" if only_active_storage.present? - - both - end - - # Records with Paperclip but without an Active storage attachment yet - def missing_active_storage_attachment(model, attachment) - model.where.not("#{attachment}_file_name" => [nil, ""]). - left_outer_joins("#{attachment}_attachment".to_sym). - where(active_storage_attachments: { id: nil }) - end - - def hex_to_base64_digest(hexdigest) - [[hexdigest].pack("H*")].pack("m0") - end -end diff --git a/spec/lib/tasks/from_paperclip_to_active_storage_rake_spec.rb b/spec/lib/tasks/from_paperclip_to_active_storage_rake_spec.rb deleted file mode 100644 index 210607034a..0000000000 --- a/spec/lib/tasks/from_paperclip_to_active_storage_rake_spec.rb +++ /dev/null @@ -1,117 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" -require "rake" - -describe "from_paperclip_to_active_storage.rake" do - include FileHelper - - let(:file) { Rack::Test::UploadedFile.new(black_logo_file, 'image/png') } - let(:s3_config) { - { - url: ":s3_alias_url", - storage: :s3, - s3_credentials: { - access_key_id: "A...A", - secret_access_key: "H...H", - }, - s3_headers: { "Cache-Control" => "max-age=31557600" }, - bucket: "ofn", - s3_protocol: "https", - s3_host_alias: "ofn.s3.us-east-1.amazonaws.com", - - # This is for easier testing: - path: "/:id/:style/:basename.:extension", - } - } - - before(:all) do - Rake.application.rake_require "tasks/from_paperclip_to_active_storage" - Rake::Task.define_task(:environment) - end - - describe ":migrate" do - it "creates Active Storage records for existing images on disk" do - image = Spree::Image.create!(attachment: file) - image.attachment_attachment.delete - image.attachment_blob.delete - - expect { - run_task "from_paperclip_to_active_storage:migrate" - }.to change { - image.reload.active_storage_attachment.attached? - }.to(true) - end - - it "creates Active Storage records for images on AWS S3" do - attachment_definition = Spree::Image.attachment_definitions[:attachment] - allow(Spree::Image).to receive(:attachment_definitions).and_return( - attachment: attachment_definition.merge(s3_config) - ) - allow(Rails.application.config.active_storage). - to receive(:service).and_return(:test_amazon) - - stub_request(:put, /amazonaws/).to_return(status: 200, body: "", headers: {}) - stub_request(:head, /amazonaws/).to_return( - status: 200, body: "", - headers: { - "ETag" => '"87b0a401e077485a078c0a15ceb7eb39"' - } - ) - stub_request(:put, /amazonaws/).to_return(status: 200, body: "", headers: {}) - - image = Spree::Image.create!(attachment: file) - image.attachment_attachment.delete - image.attachment_blob.delete - - expect { - run_task "from_paperclip_to_active_storage:migrate" - }.to change { - image.reload.active_storage_attachment.attached? - }.to(true) - - # The checksum can be computed with Active Storage: - # - # ActiveStorage::Blob.build_after_unfurling( - # io: file, identify: false, - # filename: "logo-black.png", - # content_type: "image/png", - # ).checksum - expect(image.attachment_blob.checksum).to eq "h7CkAeB3SFoHjAoVzrfrOQ==" - end - end - - describe ":copy_content_config" do - it "doesn't copy default images" do - run_task "from_paperclip_to_active_storage:copy_content_config" - - expect(ContentConfig.logo_blob).to eq nil - end - - it "copies uploaded images" do - ContentConfig.logo = file - ContentConfig.logo.save - - run_task "from_paperclip_to_active_storage:copy_content_config" - - expect(ContentConfig.logo_blob).to be_a ActiveStorage::Blob - end - - it "doesn't copy twice" do - ContentConfig.logo = file - ContentConfig.logo.save - - expect { - run_task "from_paperclip_to_active_storage:copy_content_config" - run_task "from_paperclip_to_active_storage:copy_content_config" - }.to change { - ActiveStorage::Blob.count - }.by(1) - end - end - - def run_task(name) - Rake::Task[name].reenable - Rake.application.invoke_task(name) - end -end From 421ffae78cf064bf0ca55e02230ff4614e93e4bd Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 12 Apr 2022 12:08:43 +1000 Subject: [PATCH 03/10] Replace Paperclip on TermsOfServeFile --- app/models/terms_of_service_file.rb | 17 +++++++---------- .../admin/terms_of_service_files/show.html.haml | 2 +- spec/models/terms_of_service_file_spec.rb | 14 ++++++++------ 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/app/models/terms_of_service_file.rb b/app/models/terms_of_service_file.rb index d652c37f7e..2fb35aab3c 100644 --- a/app/models/terms_of_service_file.rb +++ b/app/models/terms_of_service_file.rb @@ -1,11 +1,9 @@ # frozen_string_literal: true class TermsOfServiceFile < ApplicationRecord - include HasMigratingFile + has_one_attached :attachment - has_one_migrating :attachment - - validates :attachment, presence: true + validates :attachment, attached: true # The most recently uploaded file is the current one. def self.current @@ -13,7 +11,11 @@ class TermsOfServiceFile < ApplicationRecord end def self.current_url - current&.attachment&.url || Spree::Config.footer_tos_url + if current + Rails.application.routes.url_helpers.url_for(current.attachment) + else + Spree::Config.footer_tos_url + end end # If no file has been uploaded, we don't know when the old terms have @@ -21,9 +23,4 @@ class TermsOfServiceFile < ApplicationRecord def self.updated_at current&.updated_at || Time.zone.now end - - def touch(_) - # Ignore Active Storage changing the timestamp during migrations. - # This can be removed once we got rid of Paperclip. - end end diff --git a/app/views/admin/terms_of_service_files/show.html.haml b/app/views/admin/terms_of_service_files/show.html.haml index 94b7633c52..fd78d4e7ee 100644 --- a/app/views/admin/terms_of_service_files/show.html.haml +++ b/app/views/admin/terms_of_service_files/show.html.haml @@ -6,7 +6,7 @@ .admin-current-terms-of-service - if @current_file - %p= t(".current_terms_html", tos_link: link_to(t(".terms_of_service"), @current_file.attachment.url), datetime: @current_file.updated_at) + %p= t(".current_terms_html", tos_link: link_to(t(".terms_of_service"), @current_file.attachment), datetime: @current_file.updated_at) %p= link_to t(".delete"), main_app.admin_terms_of_service_files_path, method: "delete", data: { confirm: t(".confirm_delete") } - else %p diff --git a/spec/models/terms_of_service_file_spec.rb b/spec/models/terms_of_service_file_spec.rb index 7f638d0d69..74a28697a9 100644 --- a/spec/models/terms_of_service_file_spec.rb +++ b/spec/models/terms_of_service_file_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe TermsOfServiceFile do let(:pdf) { File.open(Rails.root.join("public/Terms-of-service.pdf")) } + let(:upload) { Rack::Test::UploadedFile.new(pdf, "application/pdf") } describe ".current" do it "returns nil" do @@ -12,8 +13,8 @@ describe TermsOfServiceFile do it "returns the last one" do existing = [ - TermsOfServiceFile.create!(attachment: pdf), - TermsOfServiceFile.create!(attachment: pdf), + TermsOfServiceFile.create!(attachment: upload), + TermsOfServiceFile.create!(attachment: upload), ] expect(TermsOfServiceFile.current).to eq existing.last @@ -27,10 +28,10 @@ describe TermsOfServiceFile do expect(subject).to eq "/Terms-of-service.pdf" end - it "points to the last uploaded file with timestamp parameter" do - file = TermsOfServiceFile.create!(attachment: pdf) + it "points to a stored file" do + file = TermsOfServiceFile.create!(attachment: upload) - expect(subject).to match %r{^/system/terms_of_service_files/attachments.*Terms-of-service\.pdf\?\d+$} + expect(subject).to match /active_storage.*Terms-of-service\.pdf$/ end end @@ -45,7 +46,8 @@ describe TermsOfServiceFile do it "returns the time when the terms were last updated" do update_time = 1.day.ago - file = TermsOfServiceFile.create!(attachment: pdf, updated_at: update_time) + file = TermsOfServiceFile.create!(attachment: upload) + file.update(updated_at: update_time) # The database isn't as precise as Ruby's time and rounds. expect(subject).to be_within(0.001).of(update_time) From 45995ac98441d6aa6074ebdb8e1af0d5cc29e730 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 12 Apr 2022 14:37:41 +1000 Subject: [PATCH 04/10] Replace Paperclip on EnterpriseGroup --- app/models/enterprise_group.rb | 21 ++++--------------- .../enterprise_groups/_form_images.html.haml | 8 +++---- app/views/groups/show.html.haml | 11 +++++----- spec/models/enterprise_group_spec.rb | 11 ++-------- 4 files changed, 15 insertions(+), 36 deletions(-) diff --git a/app/models/enterprise_group.rb b/app/models/enterprise_group.rb index 8b2c908006..dcce2ab192 100644 --- a/app/models/enterprise_group.rb +++ b/app/models/enterprise_group.rb @@ -1,12 +1,9 @@ # frozen_string_literal: true require 'open_food_network/locking' -require 'spree/core/s3_support' class EnterpriseGroup < ApplicationRecord - include HasMigratingFile include PermalinkGenerator - include Spree::Core::S3Support acts_as_list @@ -28,21 +25,11 @@ class EnterpriseGroup < ApplicationRecord delegate :phone, :address1, :address2, :city, :zipcode, :state, :country, to: :address - has_one_migrating :logo, - styles: { medium: "100x100" }, - url: '/images/enterprise_groups/logos/:id/:style/:basename.:extension', - path: 'public/images/enterprise_groups/logos/:id/:style/:basename.:extension' + has_one_attached :logo + has_one_attached :promo_image - has_one_migrating :promo_image, - styles: { large: ["1200x260#", :jpg] }, - url: '/images/enterprise_groups/promo_images/:id/:style/:basename.:extension', - path: 'public/images/enterprise_groups/promo_images/:id/:style/:basename.:extension' - - validates_attachment_content_type :logo, content_type: %r{\Aimage/.*\Z} - validates_attachment_content_type :promo_image, content_type: %r{\Aimage/.*\Z} - - supports_s3 :logo - supports_s3 :promo_image + validates :logo, content_type: %r{\Aimage/.*\Z} + validates :promo_image, content_type: %r{\Aimage/.*\Z} scope :by_position, -> { order('position ASC') } scope :on_front_page, -> { where(on_front_page: true) } diff --git a/app/views/admin/enterprise_groups/_form_images.html.haml b/app/views/admin/enterprise_groups/_form_images.html.haml index aa31913a20..a86d4abf05 100644 --- a/app/views/admin/enterprise_groups/_form_images.html.haml +++ b/app/views/admin/enterprise_groups/_form_images.html.haml @@ -6,13 +6,13 @@ %div{'ofn-with-tip' => t('admin_enterprise_groups_data_powertip_logo')} %a= t 'admin.whats_this' .omega.eight.columns - = image_tag @object.logo.url if @object.logo.present? - = f.file_field :logo + = image_tag @object.logo if @object.logo.attached? + = f.file_field :logo, accept: "image/*" .row .alpha.three.columns = f.label :promo_image, 'ofn-with-tip' => t(:admin_enterprise_groups_data_powertip_promo_image) %div{'ofn-with-tip' => t('admin_enterprise_groups_data_powertip_promo_image')} %a= t 'admin.whats_this' .omega.eight.columns - = image_tag @object.promo_image.url if @object.promo_image.present? - = f.file_field :promo_image + = image_tag @object.promo_image if @object.promo_image.attached? + = f.file_field :promo_image, accept: "image/*" diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 0116a5d4d1..b8eca916de 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -3,7 +3,7 @@ - content_for(:description) do = @group.description - content_for(:image) do - = @group.logo.url + = url_for(@group.logo) if @group.logo.attached? - content_for :scripts do = render partial: "shared/google_maps_js" @@ -22,14 +22,13 @@ %header .row .small-12.columns - - if @group.promo_image.present? - %img{"src" => @group.promo_image} + = image_tag @group.promo_image.variant(resize_to_limit: [1200, 260]) if @group.promo_image.attached? .row .small-12.columns.group-header.pad-top - - if @group.logo.present? - %img.group-logo{"src" => @group.logo} + - if @group.logo.attached? + = image_tag @group.logo.variant(resize_to_limit: [100, 100]), class: "group-logo" - else - %img.group-logo{"src" => '/noimage/group.png' } + = image_tag "/noimage/group.png", class: "group-logo" %h2.group-name= @group.name %p= @group.description diff --git a/spec/models/enterprise_group_spec.rb b/spec/models/enterprise_group_spec.rb index 6c443a25e1..2d62bce69c 100644 --- a/spec/models/enterprise_group_spec.rb +++ b/spec/models/enterprise_group_spec.rb @@ -39,8 +39,8 @@ describe EnterpriseGroup do e = build(:enterprise_group, description: '') end - it { is_expected.to have_attached_file :promo_image } - it { is_expected.to have_attached_file :logo } + it { is_expected.to have_one_attached :promo_image } + it { is_expected.to have_one_attached :logo } end describe "relations" do @@ -50,13 +50,6 @@ describe EnterpriseGroup do eg.enterprises << e expect(eg.reload.enterprises).to eq([e]) end - - # it "can have an image" do - # eg = create(:enterprise_group) - # image_file = File.open(File.expand_path('../../../app/assets/images/logo-white.png', __FILE__)) - # image = Spree::Image.create(viewable_id: eg.id, viewable_type: 'EnterpriseGroup', attachment: image_file) - # eg.reload.image.should == image - # end end describe "scopes" do From 4a0ed999194215d3478da5048162ef8b6619e856 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 13 Apr 2022 17:09:48 +1000 Subject: [PATCH 05/10] Replace Paperclip on Enterprise model We configured Paperclip to convert images to JPG in some cases but I omitted that here because we don't need it. If an image is better represented as PNG or another format then the user should be able to choose that. Some specs were also testing the generated URL but the Active Storage URL doesn't contain a style name anymore and it's not helpful to test the URL. --- .../v0/enterprise_attachment_controller.rb | 2 +- .../api/v0/enterprises_controller.rb | 6 +- app/models/enterprise.rb | 66 +++++++++++-------- .../api/admin/enterprise_serializer.rb | 37 ++++++----- .../api/cached_enterprise_serializer.rb | 4 +- .../api/enterprise_shopfront_serializer.rb | 4 +- .../api/shop_for_orders_serializer.rb | 2 +- app/services/terms_of_service.rb | 4 +- .../_all_terms_and_conditions.html.haml | 2 +- .../checkout/_terms_and_conditions.html.haml | 2 +- app/views/enterprises/shop.html.haml | 2 +- app/views/shopping_shared/_header.html.haml | 4 +- .../_terms_and_conditions.html.haml | 4 +- .../spree/admin/orders/invoice2.html.haml | 4 +- .../spree/order_mailer/cancel_email.html.haml | 3 +- .../confirm_email_for_customer.html.haml | 3 +- .../confirm_email_for_shop.html.haml | 3 +- .../subscription_mailer/_header.html.haml | 3 +- .../confirmation_summary_email.html.haml | 3 +- .../placement_summary_email.html.haml | 3 +- .../api/v0/enterprises_controller_spec.rb | 4 +- .../api/v0/logos_controller_spec.rb | 6 +- .../api/v0/promo_images_controller_spec.rb | 6 +- .../terms_and_conditions_controller_spec.rb | 8 +-- spec/models/spree/image_spec.rb | 5 +- .../api/admin/enterprise_serializer_spec.rb | 8 +-- spec/services/terms_of_service_spec.rb | 7 +- spec/support/file_helper.rb | 4 +- spec/system/admin/enterprises/images_spec.rb | 4 +- .../enterprises/terms_and_conditions_spec.rb | 28 +++----- .../system/consumer/shopping/checkout_spec.rb | 32 ++++----- .../system/consumer/shopping/shopping_spec.rb | 5 +- spec/system/consumer/split_checkout_spec.rb | 34 +++++----- 33 files changed, 156 insertions(+), 156 deletions(-) diff --git a/app/controllers/api/v0/enterprise_attachment_controller.rb b/app/controllers/api/v0/enterprise_attachment_controller.rb index 6e183371a1..06555b277b 100644 --- a/app/controllers/api/v0/enterprise_attachment_controller.rb +++ b/app/controllers/api/v0/enterprise_attachment_controller.rb @@ -14,7 +14,7 @@ module Api respond_to :json def destroy - unless @enterprise.public_send("#{attachment_name}?") + unless @enterprise.public_send(attachment_name).attached? return respond_with_conflict(error: destroy_attachment_does_not_exist_error_message) end diff --git a/app/controllers/api/v0/enterprises_controller.rb b/app/controllers/api/v0/enterprises_controller.rb index 9dd4737a5d..5a92439001 100644 --- a/app/controllers/api/v0/enterprises_controller.rb +++ b/app/controllers/api/v0/enterprises_controller.rb @@ -44,9 +44,9 @@ module Api authorize! :update, @enterprise if params[:logo] && @enterprise.update( logo: params[:logo] ) - render html: @enterprise.logo.url(:medium), status: :ok - elsif params[:promo] && @enterprise.update( promo_image: params[:promo] ) - render html: @enterprise.promo_image.url(:medium), status: :ok + render(html: @enterprise.logo_url(:medium), status: :ok) + elsif params[:promo] && @enterprise.update!( promo_image: params[:promo] ) + render(html: @enterprise.promo_image_url(:medium), status: :ok) else invalid_resource!(@enterprise) end diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index 9b9e587bed..f355c25f42 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -1,13 +1,20 @@ # frozen_string_literal: false -require 'spree/core/s3_support' - class Enterprise < ApplicationRecord - include HasMigratingFile - include Spree::Core::S3Support - SELLS = %w(unspecified none own any).freeze ENTERPRISE_SEARCH_RADIUS = 100 + # The next Rails version will have named variants but we need to store them + # ourselves for now. + LOGO_SIZES = { + thumb: { resize_to_limit: [100, 100] }, + small: { resize_to_limit: [180, 180] }, + medium: { resize_to_limit: [300, 300] }, + }.freeze + PROMO_IMAGE_SIZES = { + thumb: { resize_to_limit: [100, 100] }, + medium: { resize_to_fill: [720, 156] }, + large: { resize_to_fill: [1200, 260] }, + }.freeze searchable_attributes :sells, :is_primary_producer searchable_associations :properties @@ -73,31 +80,16 @@ class Enterprise < ApplicationRecord tag_rule[:preferred_customer_tags].blank? } - has_one_migrating :logo, - styles: { medium: "300x300>", small: "180x180>", thumb: "100x100>" }, - url: '/images/enterprises/logos/:id/:style/:basename.:extension', - path: 'public/images/enterprises/logos/:id/:style/:basename.:extension' + has_one_attached :logo + has_one_attached :promo_image + has_one_attached :terms_and_conditions - has_one_migrating :promo_image, - styles: { - large: ["1200x260#", :jpg], - medium: ["720x156#", :jpg], - thumb: ["100x100>", :jpg] - }, - url: '/images/enterprises/promo_images/:id/:style/:basename.:extension', - path: 'public/images/enterprises/promo_images/:id/:style/:basename.:extension' - validates_attachment_content_type :logo, content_type: %r{\Aimage/.*\Z} - validates_attachment_content_type :promo_image, content_type: %r{\Aimage/.*\Z} - - has_one_migrating :terms_and_conditions, - url: '/files/enterprises/terms_and_conditions/:id/:basename.:extension', - path: 'public/files/enterprises/terms_and_conditions/:id/:basename.:extension' - validates_attachment_content_type :terms_and_conditions, - content_type: "application/pdf", - message: I18n.t(:enterprise_terms_and_conditions_type_error) - - supports_s3 :logo - supports_s3 :promo_image + validates :logo, content_type: %r{\Aimage/.*\Z} + validates :promo_image, content_type: %r{\Aimage/.*\Z} + validates :terms_and_conditions, content_type: { + in: "application/pdf", + message: I18n.t(:enterprise_terms_and_conditions_type_error), + } validates :name, presence: true validate :name_is_unique @@ -284,6 +276,22 @@ class Enterprise < ApplicationRecord relatives_including_self.is_primary_producer end + def logo_url(name) + return unless logo.attached? + + Rails.application.routes.url_helpers.url_for( + logo.variant(LOGO_SIZES[name]) + ) + end + + def promo_image_url(name) + return unless promo_image.attached? + + Rails.application.routes.url_helpers.url_for( + promo_image.variant(PROMO_IMAGE_SIZES[name]) + ) + end + def website strip_url self[:website] end diff --git a/app/serializers/api/admin/enterprise_serializer.rb b/app/serializers/api/admin/enterprise_serializer.rb index 019250f634..21f08a3492 100644 --- a/app/serializers/api/admin/enterprise_serializer.rb +++ b/app/serializers/api/admin/enterprise_serializer.rb @@ -22,21 +22,26 @@ module Api has_one :business_address, serializer: Api::AddressSerializer def logo - attachment_urls(object.logo, [:thumb, :small, :medium]) + attachment_urls(object.logo, Enterprise::LOGO_SIZES) end def promo_image - attachment_urls(object.promo_image, [:thumb, :medium, :large]) + attachment_urls(object.promo_image, Enterprise::PROMO_IMAGE_SIZES) end def terms_and_conditions - return unless object.terms_and_conditions.file? + return unless object.terms_and_conditions.attached? - object.terms_and_conditions.url + Rails.application.routes.url_helpers. + url_for(object.terms_and_conditions) + end + + def terms_and_conditions_file_name + object.terms_and_conditions_blob&.filename end def terms_and_conditions_updated_at - object.terms_and_conditions_updated_at&.to_s + object.terms_and_conditions_blob&.created_at&.to_s end def tag_groups @@ -76,19 +81,19 @@ module Api # Returns a hash of URLs for specified versions of an attachment. # - # Example: + # Example result: # - # attachment_urls(object.logo, [:thumb, :small, :medium]) - # # { - # # thumb: LOGO_THUMB_URL, - # # small: LOGO_SMALL_URL, - # # medium: LOGO_MEDIUM_URL - # # } - def attachment_urls(attachment, versions) - return unless attachment.file? + # { + # thumb: LOGO_THUMB_URL, + # small: LOGO_SMALL_URL, + # medium: LOGO_MEDIUM_URL + # } + def attachment_urls(attachment, styles) + return unless attachment.attached? - versions.each_with_object({}) do |version, urls| - urls[version] = attachment.url(version) + styles.transform_values do |transformation| + Rails.application.routes.url_helpers. + url_for(attachment.variant(transformation)) end end end diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index ede433065b..ef6b37c9cf 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -44,11 +44,11 @@ module Api end def logo - enterprise.logo(:medium) if enterprise.logo? + enterprise.logo_url(:medium) end def promo_image - enterprise.promo_image(:large) if enterprise.promo_image? + enterprise.promo_image_url(:large) end def path diff --git a/app/serializers/api/enterprise_shopfront_serializer.rb b/app/serializers/api/enterprise_shopfront_serializer.rb index cee6cc5145..894152778f 100644 --- a/app/serializers/api/enterprise_shopfront_serializer.rb +++ b/app/serializers/api/enterprise_shopfront_serializer.rb @@ -41,11 +41,11 @@ module Api end def logo - enterprise.logo(:medium) if enterprise.logo? + enterprise.logo_url(:medium) end def promo_image - enterprise.promo_image(:large) if enterprise.promo_image? + enterprise.promo_image_url(:large) end def path diff --git a/app/serializers/api/shop_for_orders_serializer.rb b/app/serializers/api/shop_for_orders_serializer.rb index a8dc799012..b7b19cb5fc 100644 --- a/app/serializers/api/shop_for_orders_serializer.rb +++ b/app/serializers/api/shop_for_orders_serializer.rb @@ -9,7 +9,7 @@ module Api end def logo - object.logo(:small) if object.logo? + object.logo_url(:small) end end end diff --git a/app/services/terms_of_service.rb b/app/services/terms_of_service.rb index 95cbf661d6..5a55010272 100644 --- a/app/services/terms_of_service.rb +++ b/app/services/terms_of_service.rb @@ -5,7 +5,7 @@ class TermsOfService return false unless accepted_at = customer&.terms_and_conditions_accepted_at accepted_at > if distributor - distributor.terms_and_conditions_updated_at + distributor.terms_and_conditions_blob.created_at else TermsOfServiceFile.updated_at end @@ -20,6 +20,6 @@ class TermsOfService end def self.distributor_terms_required?(distributor) - distributor.terms_and_conditions.file? + distributor.terms_and_conditions.attached? end end diff --git a/app/views/checkout/_all_terms_and_conditions.html.haml b/app/views/checkout/_all_terms_and_conditions.html.haml index 5a90b15cfa..6af5326284 100644 --- a/app/views/checkout/_all_terms_and_conditions.html.haml +++ b/app/views/checkout/_all_terms_and_conditions.html.haml @@ -1,4 +1,4 @@ %p %input{ type: 'checkbox', id: 'accept_terms', ng: { model: "terms_and_conditions_accepted", init: "terms_and_conditions_accepted = #{all_terms_and_conditions_already_accepted?}" } } %label.small{for: "accept_terms"} - = t('.message_html', terms_and_conditions_link: link_to( t(".terms_and_conditions"), current_order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms) + = t('.message_html', terms_and_conditions_link: link_to( t(".terms_and_conditions"), current_order.distributor.terms_and_conditions, target: '_blank'), tos_link: link_to_platform_terms) diff --git a/app/views/checkout/_terms_and_conditions.html.haml b/app/views/checkout/_terms_and_conditions.html.haml index e84b4b9c81..608a0fd66b 100644 --- a/app/views/checkout/_terms_and_conditions.html.haml +++ b/app/views/checkout/_terms_and_conditions.html.haml @@ -1,3 +1,3 @@ %p %input{ type: 'checkbox', id: 'accept_terms', ng: { model: "terms_and_conditions_accepted", init: "terms_and_conditions_accepted=#{terms_and_conditions_already_accepted?}" } } - %label.small{for: "accept_terms"}= t('.message_html', terms_and_conditions_link: link_to( t( '.link_text' ), current_order.distributor.terms_and_conditions.url, target: '_blank')) + %label.small{for: "accept_terms"}= t('.message_html', terms_and_conditions_link: link_to( t( '.link_text' ), current_order.distributor.terms_and_conditions, target: '_blank')) diff --git a/app/views/enterprises/shop.html.haml b/app/views/enterprises/shop.html.haml index 150452a4e9..628f4b8630 100644 --- a/app/views/enterprises/shop.html.haml +++ b/app/views/enterprises/shop.html.haml @@ -3,7 +3,7 @@ - content_for(:description) do = current_distributor.description - content_for(:image) do - = current_distributor.logo.url + = url_for(current_distributor.logo) if current_distributor.logo.attached? - content_for :injection_data do - cache(*CacheService::FragmentCaching.ams_shop(@enterprise)) do diff --git a/app/views/shopping_shared/_header.html.haml b/app/views/shopping_shared/_header.html.haml index 643ba2912f..45d4d9c952 100644 --- a/app/views/shopping_shared/_header.html.haml +++ b/app/views/shopping_shared/_header.html.haml @@ -4,8 +4,8 @@ %distributor.details.row .small-12.medium-12.large-8.columns #distributor_title - - if distributor.logo? - %img.left{src: distributor.logo.url(:thumb)} + - if distributor.logo.attached? + = image_tag distributor.logo_url(:thumb), class: "left" = render DistributorTitleComponent.new(name: distributor.name) %location= distributor.address.city diff --git a/app/views/split_checkout/_terms_and_conditions.html.haml b/app/views/split_checkout/_terms_and_conditions.html.haml index 0acf25c899..6463df2053 100644 --- a/app/views/split_checkout/_terms_and_conditions.html.haml +++ b/app/views/split_checkout/_terms_and_conditions.html.haml @@ -3,7 +3,7 @@ - if platform_terms_required? && distributor_terms_required? = f.check_box :accept_terms, { name: "accept_terms", checked: all_terms_and_conditions_already_accepted? }, 1, nil = f.label :accept_terms do - = t('split_checkout.step3.all_terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms) + = t('split_checkout.step3.all_terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions, target: '_blank'), tos_link: link_to_platform_terms) - elsif platform_terms_required? = f.check_box :accept_terms, { name: "accept_terms", checked: platform_tos_already_accepted? }, 1, nil = f.label :accept_terms do @@ -11,7 +11,7 @@ - elsif distributor_terms_required? = f.check_box :accept_terms, { name: "accept_terms", checked: terms_and_conditions_already_accepted? }, 1, nil = f.label :accept_terms do - = t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank')) + = t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions, target: '_blank')) = f.error_message_on :terms_and_conditions, standalone: true diff --git a/app/views/spree/admin/orders/invoice2.html.haml b/app/views/spree/admin/orders/invoice2.html.haml index 689909d856..1e8545cfb1 100644 --- a/app/views/spree/admin/orders/invoice2.html.haml +++ b/app/views/spree/admin/orders/invoice2.html.haml @@ -6,9 +6,9 @@ %td{ :align => "left" } %h4 = t :tax_invoice - - if @order.distributor.display_invoice_logo? && @order.distributor.logo.present? + - if @order.distributor.display_invoice_logo? && @order.distributor.logo.attached? %td{ :align => "right", rowspan: 2 } - = wicked_pdf_image_tag @order.distributor.logo(:small), width: 150, height: 150 + = wicked_pdf_image_tag @order.distributor.logo.variant(resize_to_limit: [150, 150]) %tr{ valign: "top" } %td{ :align => "left" } - if @order.distributor.business_address.blank? diff --git a/app/views/spree/order_mailer/cancel_email.html.haml b/app/views/spree/order_mailer/cancel_email.html.haml index 0c46511c3d..4854c417b1 100755 --- a/app/views/spree/order_mailer/cancel_email.html.haml +++ b/app/views/spree/order_mailer/cancel_email.html.haml @@ -6,7 +6,8 @@ = t(".customer_greeting", name: @order.bill_address.firstname) %h4 = t(".instructions_html", distributor: @order.distributor.name ) - %img{src: "#{@order.distributor.logo.url(:medium)}"} + - if @order.distributor.logo.attached? + = image_tag @order.distributor.logo_url(:medium) %p.callout = t(".dont_cancel", email: @order.distributor.contact.email) diff --git a/app/views/spree/order_mailer/confirm_email_for_customer.html.haml b/app/views/spree/order_mailer/confirm_email_for_customer.html.haml index 42ed2d9cc1..c7f1597128 100644 --- a/app/views/spree/order_mailer/confirm_email_for_customer.html.haml +++ b/app/views/spree/order_mailer/confirm_email_for_customer.html.haml @@ -11,7 +11,8 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - %img.float-right{:src => "#{@order.distributor.logo.url(:medium)}"}/ + - if @order.distributor.logo.attached? + = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear %p   diff --git a/app/views/spree/order_mailer/confirm_email_for_shop.html.haml b/app/views/spree/order_mailer/confirm_email_for_shop.html.haml index 90ec5386d1..a86c6cda88 100644 --- a/app/views/spree/order_mailer/confirm_email_for_shop.html.haml +++ b/app/views/spree/order_mailer/confirm_email_for_shop.html.haml @@ -11,7 +11,8 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - %img.float-right{:src => "#{@order.distributor.logo.url(:medium)}"}/ + - if @order.distributor.logo.attached? + = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear %p   diff --git a/app/views/subscription_mailer/_header.html.haml b/app/views/subscription_mailer/_header.html.haml index cb6f2d8dfe..1f70213378 100644 --- a/app/views/subscription_mailer/_header.html.haml +++ b/app/views/subscription_mailer/_header.html.haml @@ -11,5 +11,6 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - %img.float-right{:src => "#{@order.distributor.logo.url(:medium)}"}/ + - if @order.distributor.logo.attached? + = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear diff --git a/app/views/subscription_mailer/confirmation_summary_email.html.haml b/app/views/subscription_mailer/confirmation_summary_email.html.haml index 861df53454..c6279620a1 100644 --- a/app/views/subscription_mailer/confirmation_summary_email.html.haml +++ b/app/views/subscription_mailer/confirmation_summary_email.html.haml @@ -11,7 +11,8 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - %img.float-right{:src => "#{@shop.logo.url(:medium)}"}/ + - if @shop.logo.attached? + = image_tag @shop.logo_url(:medium), class: "float-right" %span.clear = render 'summary_overview', summary: @summary diff --git a/app/views/subscription_mailer/placement_summary_email.html.haml b/app/views/subscription_mailer/placement_summary_email.html.haml index 861df53454..292496fdc4 100644 --- a/app/views/subscription_mailer/placement_summary_email.html.haml +++ b/app/views/subscription_mailer/placement_summary_email.html.haml @@ -11,7 +11,8 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - %img.float-right{:src => "#{@shop.logo.url(:medium)}"}/ + - if @shop.logo.attached? + = image_tag @shop.logo_variant(:medium), class: "float-right" %span.clear = render 'summary_overview', summary: @summary diff --git a/spec/controllers/api/v0/enterprises_controller_spec.rb b/spec/controllers/api/v0/enterprises_controller_spec.rb index ce9cf59254..2eb0127eeb 100644 --- a/spec/controllers/api/v0/enterprises_controller_spec.rb +++ b/spec/controllers/api/v0/enterprises_controller_spec.rb @@ -85,14 +85,14 @@ describe Api::V0::EnterprisesController, type: :controller do api_post :update_image, logo: logo, id: enterprise.id expect(response.status).to eq 200 expect(response.content_type).to eq "text/html" - expect(response.body).to match %r{/images/enterprises/logos/\d*/medium/logo\.png\?\d*} + expect(response.body).to match /logo\.png$/ end it "I can update enterprise promo image" do api_post :update_image, promo: logo, id: enterprise.id expect(response.status).to eq 200 expect(response.content_type).to eq "text/html" - expect(response.body).to match %r{/images/enterprises/promo_images/\d*/medium/logo\.jpg\?\d*} + expect(response.body).to match /logo\.png$/ end end end diff --git a/spec/controllers/api/v0/logos_controller_spec.rb b/spec/controllers/api/v0/logos_controller_spec.rb index 4d82469775..58d6e410a5 100644 --- a/spec/controllers/api/v0/logos_controller_spec.rb +++ b/spec/controllers/api/v0/logos_controller_spec.rb @@ -35,7 +35,7 @@ module Api expect(response.status).to eq 200 expect(json_response["id"]).to eq enterprise.id enterprise.reload - expect(enterprise.logo?).to be false + expect(enterprise.logo).to_not be_attached end context "when logo does not exist" do @@ -75,7 +75,7 @@ module Api spree_delete :destroy, enterprise_id: enterprise expect(response.status).to eq(401) enterprise.reload - expect(enterprise.logo?).to be true + expect(enterprise.logo).to be_attached end end @@ -86,7 +86,7 @@ module Api spree_delete :destroy, enterprise_id: enterprise expect(response.status).to eq(401) enterprise.reload - expect(enterprise.logo?).to be true + expect(enterprise.logo).to be_attached end end end diff --git a/spec/controllers/api/v0/promo_images_controller_spec.rb b/spec/controllers/api/v0/promo_images_controller_spec.rb index 22296172d1..dd31be66dc 100644 --- a/spec/controllers/api/v0/promo_images_controller_spec.rb +++ b/spec/controllers/api/v0/promo_images_controller_spec.rb @@ -35,7 +35,7 @@ module Api expect(response.status).to eq 200 expect(json_response["id"]).to eq enterprise.id enterprise.reload - expect(enterprise.promo_image?).to be false + expect(enterprise.promo_image).to_not be_attached end context "when promo image does not exist" do @@ -75,7 +75,7 @@ module Api spree_delete :destroy, enterprise_id: enterprise expect(response.status).to eq(401) enterprise.reload - expect(enterprise.promo_image?).to be true + expect(enterprise.promo_image).to be_attached end end @@ -86,7 +86,7 @@ module Api spree_delete :destroy, enterprise_id: enterprise expect(response.status).to eq(401) enterprise.reload - expect(enterprise.promo_image?).to be true + expect(enterprise.promo_image).to be_attached end end end diff --git a/spec/controllers/api/v0/terms_and_conditions_controller_spec.rb b/spec/controllers/api/v0/terms_and_conditions_controller_spec.rb index 6a8fc673a5..f99c75b5ae 100644 --- a/spec/controllers/api/v0/terms_and_conditions_controller_spec.rb +++ b/spec/controllers/api/v0/terms_and_conditions_controller_spec.rb @@ -12,15 +12,15 @@ module Api let(:enterprise_manager) { create(:user, enterprises: [enterprise]) } describe "removing terms and conditions file" do - let(:fake_terms_file_path) { black_logo_file } + let(:terms_file_path) { Rails.root.join("public/Terms-of-service.pdf") } let(:terms_and_conditions_file) { - Rack::Test::UploadedFile.new(fake_terms_file_path, "application/pdf") + Rack::Test::UploadedFile.new(terms_file_path, "application/pdf") } let(:enterprise) { create(:enterprise, owner: enterprise_owner) } before do allow(controller).to receive(:spree_current_user) { current_user } - enterprise.update terms_and_conditions: terms_and_conditions_file + enterprise.update!(terms_and_conditions: terms_and_conditions_file) end context "as manager" do @@ -32,7 +32,7 @@ module Api expect(response.status).to eq 200 expect(json_response["id"]).to eq enterprise.id enterprise.reload - expect(enterprise.terms_and_conditions?).to be false + expect(enterprise.terms_and_conditions).to_not be_attached end context "when terms and conditions file does not exist" do diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index 06de9c955b..f7024d0e9b 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -6,13 +6,12 @@ module Spree describe Image do include FileHelper - let(:file) { Rack::Test::UploadedFile.new(black_logo_file, 'image/png') } let(:product) { create(:product) } describe "using local storage" do it "stores a new image" do image = Spree::Image.create!( - attachment: file, + attachment: black_logo_file, viewable: product.master, ) @@ -78,7 +77,7 @@ module Spree stub_request(:put, as_upload_pattern).to_return(status: 200, body: "", headers: {}) image = Spree::Image.create!( - attachment: file, + attachment: black_logo_file, viewable: product.master, ) diff --git a/spec/serializers/api/admin/enterprise_serializer_spec.rb b/spec/serializers/api/admin/enterprise_serializer_spec.rb index 1c4b2a1127..9c3e6b86d1 100644 --- a/spec/serializers/api/admin/enterprise_serializer_spec.rb +++ b/spec/serializers/api/admin/enterprise_serializer_spec.rb @@ -16,8 +16,7 @@ describe Api::Admin::EnterpriseSerializer do context "when there is a logo" do let(:image) do - image = white_logo_file - Rack::Test::UploadedFile.new(image, "image/png") + black_logo_file end it "includes URLs of image versions" do @@ -42,14 +41,13 @@ describe Api::Admin::EnterpriseSerializer do context "when there is a promo image" do let(:image) do - image = black_logo_file - Rack::Test::UploadedFile.new(image, "image/png") + black_logo_file end it "includes URLs of image versions" do serializer = Api::Admin::EnterpriseSerializer.new(enterprise) expect(serializer.as_json[:promo_image]).to_not be_blank - expect(serializer.as_json[:promo_image][:medium]).to match(/logo-black.jpg/) + expect(serializer.as_json[:promo_image][:medium]).to match(/logo-black\.png$/) end end diff --git a/spec/services/terms_of_service_spec.rb b/spec/services/terms_of_service_spec.rb index 5d1e169529..514f9dfb50 100644 --- a/spec/services/terms_of_service_spec.rb +++ b/spec/services/terms_of_service_spec.rb @@ -34,12 +34,17 @@ describe TermsOfService do context "a customer has accepted the distributor terms of service" do before do allow(customer).to receive(:terms_and_conditions_accepted_at) { Time.zone.now - 1.week } - allow(distributor).to receive(:terms_and_conditions_updated_at) { Time.zone.now - 2.weeks } + allow(distributor).to receive(:terms_and_conditions_blob) { + ActiveStorage::Blob.new(created_at: Time.zone.now - 2.weeks) + } end it "should reflect whether the platform TOS have been accepted since the last update" do expect { allow(distributor).to receive(:terms_and_conditions_updated_at) { Time.zone.now } + allow(distributor).to receive(:terms_and_conditions_blob) { + ActiveStorage::Blob.new(created_at: Time.zone.now) + } }.to change { TermsOfService.tos_accepted?(customer, distributor) }.from(true).to(false) diff --git a/spec/support/file_helper.rb b/spec/support/file_helper.rb index 290467905e..8f4ead848b 100644 --- a/spec/support/file_helper.rb +++ b/spec/support/file_helper.rb @@ -2,11 +2,11 @@ module FileHelper def black_logo_file - File.open(black_logo_path) + Rack::Test::UploadedFile.new(black_logo_path) end def white_logo_file - File.open(black_logo_path) + Rack::Test::UploadedFile.new(white_logo_path) end def black_logo_path diff --git a/spec/system/admin/enterprises/images_spec.rb b/spec/system/admin/enterprises/images_spec.rb index 11d9e2e09b..5c9d483893 100644 --- a/spec/system/admin/enterprises/images_spec.rb +++ b/spec/system/admin/enterprises/images_spec.rb @@ -80,7 +80,7 @@ describe "Managing enterprise images" do go_to_images within ".page-admin-enterprises-form__promo-image-field-group" do - expect_preview_image "logo-white.jpg" + expect_preview_image "logo-white.png" end # Replacing image @@ -91,7 +91,7 @@ describe "Managing enterprise images" do go_to_images within ".page-admin-enterprises-form__promo-image-field-group" do - expect_preview_image "logo-black.jpg" + expect_preview_image "logo-black.png" end # Removing image diff --git a/spec/system/admin/enterprises/terms_and_conditions_spec.rb b/spec/system/admin/enterprises/terms_and_conditions_spec.rb index f5e11e3657..d15f286162 100644 --- a/spec/system/admin/enterprises/terms_and_conditions_spec.rb +++ b/spec/system/admin/enterprises/terms_and_conditions_spec.rb @@ -24,48 +24,36 @@ describe "Uploading Terms and Conditions PDF" do end end - let(:white_pdf_file_name) { Rails.root.join("app/webpacker/images/logo-white.pdf") } - let(:black_pdf_file_name) { Rails.root.join("app/webpacker/images/logo-black.pdf") } - - around do |example| - # Create fake PDFs from PNG images - FileUtils.cp(white_logo_path, white_pdf_file_name) - FileUtils.cp(black_logo_path, black_pdf_file_name) - - example.run - - # Delete fake PDFs - FileUtils.rm_f(white_pdf_file_name) - FileUtils.rm_f(black_pdf_file_name) - end + let(:original_terms) { Rails.root.join("public/Terms-of-service.pdf") } + let(:updated_terms) { Rails.root.join("public/Terms-of-ServiceUK.pdf") } it "uploading terms and conditions" do go_to_business_details # Add PDF - attach_file "enterprise[terms_and_conditions]", white_pdf_file_name + attach_file "enterprise[terms_and_conditions]", original_terms time = Time.zone.local(2002, 4, 13, 0, 0, 0) Timecop.freeze(run_time = time) do click_button "Update" - expect(distributor.reload.terms_and_conditions_updated_at).to eq run_time + expect(distributor.reload.terms_and_conditions_blob.created_at).to eq run_time end expect(page). to have_content "Enterprise \"#{distributor.name}\" has been successfully updated!" go_to_business_details - expect(page).to have_selector "a[href*='logo-white.pdf'][target=\"_blank\"]" + expect(page).to have_selector "a[href*='Terms-of-service.pdf'][target=\"_blank\"]" expect(page).to have_content time.strftime("%F %T") # Replace PDF - attach_file "enterprise[terms_and_conditions]", black_pdf_file_name + attach_file "enterprise[terms_and_conditions]", updated_terms click_button "Update" expect(page). to have_content "Enterprise \"#{distributor.name}\" has been successfully updated!" - expect(distributor.reload.terms_and_conditions_updated_at).to_not eq run_time + expect(distributor.reload.terms_and_conditions_blob.created_at).to_not eq run_time go_to_business_details - expect(page).to have_selector "a[href*='logo-black.pdf']" + expect(page).to have_selector "a[href*='Terms-of-ServiceUK.pdf']" end end end diff --git a/spec/system/consumer/shopping/checkout_spec.rb b/spec/system/consumer/shopping/checkout_spec.rb index 3ab016e3a4..22e8b4e623 100644 --- a/spec/system/consumer/shopping/checkout_spec.rb +++ b/spec/system/consumer/shopping/checkout_spec.rb @@ -88,6 +88,12 @@ describe "As a consumer I want to check out my cart", js: true do context 'login in as user' do let(:user) { create(:user) } + let(:pdf_upload) { + Rack::Test::UploadedFile.new( + Rails.root.join("public/Terms-of-service.pdf"), + "application/pdf" + ) + } before do login_as(user) @@ -158,21 +164,16 @@ describe "As a consumer I want to check out my cart", js: true do end context "when distributor has T&Cs" do - let(:fake_terms_and_conditions_path) { white_logo_path } - let(:terms_and_conditions_file) { - Rack::Test::UploadedFile.new(fake_terms_and_conditions_path, "application/pdf") - } - before do - order.distributor.terms_and_conditions = terms_and_conditions_file - order.distributor.save + order.distributor.terms_and_conditions = pdf_upload + order.distributor.save! end describe "when customer has not accepted T&Cs before" do it "shows a link to the T&Cs and disables checkout button until terms are accepted" do visit checkout_path - expect(page).to have_link("Terms and Conditions", - href: order.distributor.terms_and_conditions.url) + + expect(page).to have_link("Terms and Conditions") expect(page).to have_button("Place order now", disabled: true) @@ -193,7 +194,7 @@ describe "As a consumer I want to check out my cart", js: true do end describe "but afterwards the enterprise has uploaded a new T&Cs file" do - before { order.distributor.update terms_and_conditions_updated_at: Time.zone.now } + before { order.distributor.terms_and_conditions.attach(pdf_upload) } it "disables checkout button until terms are accepted" do visit checkout_path @@ -230,7 +231,7 @@ describe "As a consumer I want to check out my cart", js: true do context "when the terms have been accepted in the past" do before do TermsOfServiceFile.create!( - attachment: File.open(Rails.root.join("public/Terms-of-service.pdf")), + attachment: pdf_upload, updated_at: 1.day.ago, ) customer = create(:customer, enterprise: order.distributor, user: user) @@ -255,14 +256,10 @@ describe "As a consumer I want to check out my cart", js: true do end context "when the seller's terms and the platform's terms have to be accepted" do - let(:fake_terms_and_conditions_path) { white_logo_path } - let(:terms_and_conditions_file) { - Rack::Test::UploadedFile.new(fake_terms_and_conditions_path, "application/pdf") - } let(:tos_url) { "https://example.org/tos" } before do - order.distributor.terms_and_conditions = terms_and_conditions_file + order.distributor.terms_and_conditions = pdf_upload order.distributor.save! allow(Spree::Config).to receive(:shoppers_require_tos).and_return(true) @@ -273,8 +270,7 @@ describe "As a consumer I want to check out my cart", js: true do visit checkout_path within "#checkout_form" do - expect(page).to have_link("Terms and Conditions", - href: order.distributor.terms_and_conditions.url) + expect(page).to have_link("Terms and Conditions") expect(page).to have_link("Terms of service", href: tos_url) expect(page).to have_button("Place order now", disabled: true) end diff --git a/spec/system/consumer/shopping/shopping_spec.rb b/spec/system/consumer/shopping/shopping_spec.rb index 1fdc0b756c..b2ea899b9c 100644 --- a/spec/system/consumer/shopping/shopping_spec.rb +++ b/spec/system/consumer/shopping/shopping_spec.rb @@ -30,8 +30,7 @@ describe "As a consumer I want to shop with a distributor", js: true do it "shows a distributor with images" do # Given the distributor has a logo - distributor.logo = File.new(white_logo_path) - distributor.save! + distributor.update!(logo: white_logo_file) # Then we should see the distributor and its logo visit shop_path @@ -39,7 +38,7 @@ describe "As a consumer I want to shop with a distributor", js: true do within ".tab-buttons" do click_link "About" end - expect(first("distributor img")['src']).to include distributor.logo.url(:thumb) + expect(first("distributor img")['src']).to include "logo-white.png" end it "shows the producers for a distributor" do diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index cbc5284519..f6a544a369 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -747,6 +747,15 @@ describe "As a consumer, I want to checkout my order", js: true do end describe "terms and conditions" do + let(:system_terms_path) { Rails.root.join("public/Terms-of-service.pdf") } + let(:shop_terms_path) { Rails.root.join("public/Terms-of-ServiceUK.pdf") } + let(:system_terms) { + Rack::Test::UploadedFile.new(system_terms_path, "application/pdf") + } + let(:shop_terms) { + Rack::Test::UploadedFile.new(shop_terms_path, "application/pdf") + } + context "when none are required" do it "doesn't show checkbox or links" do visit checkout_step_path(:summary) @@ -760,21 +769,14 @@ describe "As a consumer, I want to checkout my order", js: true do end context "when distributor has T&Cs" do - let(:fake_terms_and_conditions_path) { white_logo_path } - let(:terms_and_conditions_file) { - Rack::Test::UploadedFile.new(fake_terms_and_conditions_path, "application/pdf") - } - let(:terms_url) { order.distributor.terms_and_conditions.url } - before do - order.distributor.terms_and_conditions = terms_and_conditions_file - order.distributor.save + order.distributor.update!(terms_and_conditions: shop_terms) end describe "when customer has not accepted T&Cs before" do it "shows a link to the T&Cs and disables checkout button until terms are accepted" do visit checkout_step_path(:summary) - expect(page).to have_link "Terms and Conditions", href: terms_url + expect(page).to have_link "Terms and Conditions", href: /#{shop_terms_path.basename}$/ expect(page).to have_field "order_accept_terms", checked: false end end @@ -791,7 +793,7 @@ describe "As a consumer, I want to checkout my order", js: true do end describe "but afterwards the enterprise has uploaded a new T&Cs file" do - before { order.distributor.update terms_and_conditions_updated_at: Time.zone.now } + before { order.distributor.update!(terms_and_conditions: shop_terms) } it "disables checkout button until terms are accepted" do visit checkout_step_path(:summary) @@ -820,7 +822,7 @@ describe "As a consumer, I want to checkout my order", js: true do context "when the terms have been accepted in the past" do before do TermsOfServiceFile.create!( - attachment: File.open(Rails.root.join("public/Terms-of-service.pdf")), + attachment: system_terms, updated_at: 1.day.ago, ) customer = create(:customer, enterprise: order.distributor, user: user) @@ -837,16 +839,10 @@ describe "As a consumer, I want to checkout my order", js: true do end context "when the seller's terms and the platform's terms have to be accepted" do - let(:fake_terms_and_conditions_path) { white_logo_path } - let(:terms_and_conditions_file) { - Rack::Test::UploadedFile.new(fake_terms_and_conditions_path, "application/pdf") - } let(:tos_url) { "https://example.org/tos" } - let(:terms_url) { order.distributor.terms_and_conditions.url } before do - order.distributor.terms_and_conditions = terms_and_conditions_file - order.distributor.save! + order.distributor.update!(terms_and_conditions: shop_terms) allow(Spree::Config).to receive(:shoppers_require_tos).and_return(true) allow(Spree::Config).to receive(:footer_tos_url).and_return(tos_url) @@ -855,7 +851,7 @@ describe "As a consumer, I want to checkout my order", js: true do it "shows links to both terms and all need accepting" do visit checkout_step_path(:summary) - expect(page).to have_link "Terms and Conditions", href: terms_url + expect(page).to have_link "Terms and Conditions", href: /#{shop_terms_path.basename}$/ expect(page).to have_link "Terms of service", href: tos_url expect(page).to have_field "order_accept_terms", checked: false end From b7efa1b018d18ee74058ee3aa5f8ca4aa449d066 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 15 Apr 2022 16:35:52 +1000 Subject: [PATCH 06/10] Replace Paperclip on Spree::Image --- .../spree/admin/products_controller.rb | 3 - app/models/spree/image.rb | 98 ++++--------------- .../supplied_product_serializer.rb | 2 +- .../api/admin/product_serializer.rb | 12 +-- .../api/admin/variant_serializer.rb | 4 +- app/serializers/api/image_serializer.rb | 8 +- app/serializers/api/variant_serializer.rb | 6 +- app/services/image_importer.rb | 17 +--- app/views/spree/admin/images/edit.html.haml | 4 +- app/views/spree/admin/images/index.html.haml | 4 +- .../spree/shared/_variant_thumbnail.html.haml | 4 +- config/initializers/spree.rb | 5 - config/locales/en.yml | 1 - lib/spree/core/product_duplicator.rb | 2 +- .../spree/admin/products_controller_spec.rb | 5 +- spec/factories/order_cycle_factory.rb | 2 +- .../lib/spree/core/product_duplicator_spec.rb | 7 +- spec/models/spree/image_spec.rb | 89 +++++------------ spec/services/image_importer_spec.rb | 2 +- spec/support/api_helper.rb | 2 +- 20 files changed, 73 insertions(+), 204 deletions(-) diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb index 9aae19e015..e81e846629 100644 --- a/app/controllers/spree/admin/products_controller.rb +++ b/app/controllers/spree/admin/products_controller.rb @@ -39,9 +39,6 @@ module Spree render :new end end - rescue Paperclip::Errors::NotIdentifiedByImageMagickError - @object.errors.add(:base, t('spree.admin.products.image_upload_error')) - respond_with(@object) end def show diff --git a/app/models/spree/image.rb b/app/models/spree/image.rb index 996067ec6f..da48229f80 100644 --- a/app/models/spree/image.rb +++ b/app/models/spree/image.rb @@ -1,103 +1,39 @@ # frozen_string_literal: true -require 'spree/core/s3_support' - module Spree class Image < Asset - include HasMigratingFile + 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 - validates_attachment_presence :attachment + has_one_attached :attachment + + validates :attachment, attached: true, content_type: %r{\Aimage/.*\Z} validate :no_attachment_errors - # This is where the styles are used in the app: - # - mini: used in the BackOffice: Bulk Product Edit page and Order Cycle edit page - # - small: used in the FrontOffice: Product List page - # - product: used in the BackOffice: Product Image upload modal in the Bulk Product Edit page - # and Product image edit page - # - large: used in the FrontOffice: product modal - has_one_migrating :attachment, - styles: { mini: "48x48#", small: "227x227#", - product: "240x240>", large: "600x600>" }, - default_style: :product, - url: '/spree/products/:id/:style/:basename.:extension', - path: ':rails_root/public/spree/products/:id/:style/:basename.:extension', - convert_options: { all: '-strip -auto-orient -colorspace sRGB' } - - # save the w,h of the original image (from which others can be calculated) - # we need to look at the write-queue for images which have not been saved yet - after_post_process :find_dimensions - - include Spree::Core::S3Support - supports_s3 :attachment - - # used by admin products autocomplete - def mini_url - attachment.url(:mini, false) + def variant(name) + attachment.variant(SIZES[name]) end - def find_dimensions - return if attachment.errors.present? + def url(size) + return unless attachment.variable? - geometry = Paperclip::Geometry.from_file(local_filename_of_original) - - self.attachment_width = geometry.width - self.attachment_height = geometry.height - end - - def local_filename_of_original - temporary = attachment.queued_for_write[:original] - - if temporary&.path.present? - temporary.path - else - attachment.path - end + Rails.application.routes.url_helpers.url_for(variant(size)) end # if there are errors from the plugin, then add a more meaningful message def no_attachment_errors - return if attachment.errors.empty? + return if errors[:attachment].empty? - if errors.all? { |e| e.type == "Paperclip::Errors::NotIdentifiedByImageMagickError" } + if errors.all? { |e| e.type == :content_type_invalid } attachment.errors.clear errors.add :base, I18n.t('spree.admin.products.image_upload_error') - else - errors.add :attachment, - I18n.t('spree.admin.products.paperclip_image_error', attachment_file_name: attachment_file_name) end + false end - - def self.set_attachment_attribute(attribute_name, attribute_value) - attachment_definitions[:attachment][attribute_name] = attribute_value - end - - def self.set_storage_attachment_attributes - if Spree::Config[:use_s3] - set_s3_attachment_attributes - else - attachment_definitions[:attachment].delete(:storage) - end - end - - def self.set_s3_attachment_attributes - set_attachment_attribute(:storage, :s3) - set_attachment_attribute(:s3_credentials, s3_credentials) - set_attachment_attribute(:s3_headers, - ActiveSupport::JSON.decode(Spree::Config[:s3_headers])) - set_attachment_attribute(:bucket, Spree::Config[:s3_bucket]) - - # We use :s3_alias_url (virtual host url style) and set the URL on property s3_host_alias - set_attachment_attribute(:s3_host_alias, attachment_definitions[:attachment][:url]) - set_attachment_attribute(:url, ":s3_alias_url") - end - private_class_method :set_s3_attachment_attributes - - def self.s3_credentials - { access_key_id: Spree::Config[:s3_access_key], - secret_access_key: Spree::Config[:s3_secret], - bucket: Spree::Config[:s3_bucket] } - end - private_class_method :s3_credentials end end diff --git a/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb b/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb index f5ed38c76a..fa0c9e7365 100644 --- a/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb +++ b/app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb @@ -11,7 +11,7 @@ module Api end def image_url - object.images.present? ? object.images.first.attachment.url(:mini) : nil + object.images.first&.url(:mini) end def master_id diff --git a/app/serializers/api/admin/product_serializer.rb b/app/serializers/api/admin/product_serializer.rb index 6805d81d1f..d65909a521 100644 --- a/app/serializers/api/admin/product_serializer.rb +++ b/app/serializers/api/admin/product_serializer.rb @@ -13,19 +13,11 @@ module Api has_one :master, serializer: Api::Admin::VariantSerializer def image_url - if object.images.present? - object.images.first.attachment.url(:product) - else - "/noimage/product.png" - end + object.images.first&.url(:product) || "/noimage/product.png" end def thumb_url - if object.images.present? - object.images.first.attachment.url(:mini) - else - "/noimage/mini.png" - end + object.images.first&.url(:mini) || "/noimage/mini.png" end def on_hand diff --git a/app/serializers/api/admin/variant_serializer.rb b/app/serializers/api/admin/variant_serializer.rb index 1be17e1c19..062ce4ceee 100644 --- a/app/serializers/api/admin/variant_serializer.rb +++ b/app/serializers/api/admin/variant_serializer.rb @@ -33,9 +33,7 @@ module Api end def image - return if object.product.images.empty? - - object.product.images.first.mini_url + object.product.images.first&.url(:mini) end def in_stock diff --git a/app/serializers/api/image_serializer.rb b/app/serializers/api/image_serializer.rb index ead54697d6..57d44ef74e 100644 --- a/app/serializers/api/image_serializer.rb +++ b/app/serializers/api/image_serializer.rb @@ -4,18 +4,18 @@ class Api::ImageSerializer < ActiveModel::Serializer attributes :id, :alt, :thumb_url, :small_url, :image_url, :large_url def thumb_url - object.attachment.url(:mini, false) + object.url(:mini) end def small_url - object.attachment.url(:small, false) + object.url(:small) end def image_url - object.attachment.url(:product, false) + object.url(:product) end def large_url - object.attachment.url(:large, false) + object.url(:large) end end diff --git a/app/serializers/api/variant_serializer.rb b/app/serializers/api/variant_serializer.rb index 524d940656..f58810c52b 100644 --- a/app/serializers/api/variant_serializer.rb +++ b/app/serializers/api/variant_serializer.rb @@ -35,11 +35,7 @@ class Api::VariantSerializer < ActiveModel::Serializer end def thumb_url - if object.product.images.present? - object.product.images.first.attachment.url(:mini) - else - "/noimage/mini.png" - end + object.product.images.first&.url(:mini) || "/noimage/mini.png" end def unit_price_price diff --git a/app/services/image_importer.rb b/app/services/image_importer.rb index 8fc243e755..706de935a0 100644 --- a/app/services/image_importer.rb +++ b/app/services/image_importer.rb @@ -2,21 +2,12 @@ class ImageImporter def import(url, product) - attach(download(url), product) - end + valid_url = URI.parse(url) + file = open(valid_url.to_s) + filename = File.basename(valid_url.path) - private - - def download(url) - local_file = Tempfile.new - remote_file = open(url) - IO.copy_stream(remote_file, local_file) - local_file - end - - def attach(file, product) Spree::Image.create( - attachment: file, + attachment: { io: file, filename: filename }, viewable_id: product.master.id, viewable_type: Spree::Variant, ) diff --git a/app/views/spree/admin/images/edit.html.haml b/app/views/spree/admin/images/edit.html.haml index 49d1361daf..970f67d132 100644 --- a/app/views/spree/admin/images/edit.html.haml +++ b/app/views/spree/admin/images/edit.html.haml @@ -11,7 +11,9 @@ .field.alpha.three.columns.align-center = f.label t('spree.thumbnail') %br/ - = link_to image_tag(@image.attachment.url(:small)), @image.attachment.url(:product) + - # 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)) .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 7f777c573a..978de27092 100644 --- a/app/views/spree/admin/images/index.html.haml +++ b/app/views/spree/admin/images/index.html.haml @@ -32,7 +32,9 @@ %td.no-border %span.handle %td - = link_to image_tag(image.attachment.url(:mini)), image.attachment.url(:product) + - # 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)) %td= options_text_for(image) %td= image.alt %td.actions diff --git a/app/views/spree/shared/_variant_thumbnail.html.haml b/app/views/spree/shared/_variant_thumbnail.html.haml index 719725c927..d92e205a0b 100644 --- a/app/views/spree/shared/_variant_thumbnail.html.haml +++ b/app/views/spree/shared/_variant_thumbnail.html.haml @@ -1,4 +1,6 @@ - if variant.product.images.length == 0 = image_tag("/noimage/mini.png") - else - = image_tag(variant.product.images.first.attachment.url(:mini)) + - # A Rails bug makes it necessary to call `main_app.url_for` here. + - # https://github.com/rails/rails/issues/31325 + = image_tag(main_app.url_for(variant.product.images.first.variant(:mini))) diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 145b585335..c3712a72f9 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -29,11 +29,6 @@ Rails.application.reloader.to_prepare do # applied correctly in Spree::Config. MailConfiguration.apply! - # Attachments settings - Spree::Image.set_attachment_attribute(:path, ENV['ATTACHMENT_PATH']) if ENV['ATTACHMENT_PATH'] - Spree::Image.set_attachment_attribute(:url, ENV['ATTACHMENT_URL']) if ENV['ATTACHMENT_URL'] - Spree::Image.set_storage_attachment_attributes - # TODO Work out why this is necessary # Seems like classes within OFN module become 'uninitialized' when server reloads # unless the empty module is explicity 'registered' here. Something to do with autoloading? diff --git a/config/locales/en.yml b/config/locales/en.yml index 82c667fb07..dc17d1d821 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3902,7 +3902,6 @@ See the %{link} to find out more about %{sitename}'s features and to start using no_payment_via_admin_backend: Paypal payments cannot be captured in the Backoffice products: image_upload_error: "The product image was not recognised. Please upload an image in PNG or JPG format." - paperclip_image_error: "Paperclip returned errors for file '%{attachment_file_name}' - check ImageMagick installation or image source file." new: title: "New Product" new_product: "New Product" diff --git a/lib/spree/core/product_duplicator.rb b/lib/spree/core/product_duplicator.rb index f4b778432f..659d2c5c6d 100644 --- a/lib/spree/core/product_duplicator.rb +++ b/lib/spree/core/product_duplicator.rb @@ -46,7 +46,7 @@ module Spree def duplicate_image(image) new_image = image.dup - new_image.assign_attributes(attachment: image.attachment.clone) + new_image.attachment.attach(image.attachment_blob) new_image end diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb index 7f2177494d..5474e5edad 100644 --- a/spec/controllers/spree/admin/products_controller_spec.rb +++ b/spec/controllers/spree/admin/products_controller_spec.rb @@ -163,9 +163,8 @@ describe Spree::Admin::ProductsController, type: :controller do } ) - expect do - spree_put :create, product: product_attrs_with_image - end.not_to raise_error Paperclip::Errors::NotIdentifiedByImageMagickError + spree_put :create, product: product_attrs_with_image + expect(response.status).to eq 200 end end diff --git a/spec/factories/order_cycle_factory.rb b/spec/factories/order_cycle_factory.rb index 2b1c50ddeb..8dbfcc6acd 100644 --- a/spec/factories/order_cycle_factory.rb +++ b/spec/factories/order_cycle_factory.rb @@ -35,7 +35,7 @@ FactoryBot.define do viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "position 1", - attachment: white_logo_file, + attachment: Rack::Test::UploadedFile.new(white_logo_path), position: 1 ) diff --git a/spec/lib/spree/core/product_duplicator_spec.rb b/spec/lib/spree/core/product_duplicator_spec.rb index 4026c01c8f..05813f104d 100644 --- a/spec/lib/spree/core/product_duplicator_spec.rb +++ b/spec/lib/spree/core/product_duplicator_spec.rb @@ -70,11 +70,8 @@ describe Spree::Core::ProductDuplicator do expect(new_variant).to receive(:price=).with(variant.price) expect(new_variant).to receive(:currency=).with(variant.currency) - expect(image.attachment).to receive(:clone).and_return(image.attachment) - - expect(new_image).to receive(:assign_attributes). - with(attachment: image.attachment). - and_return(new_image) + expect(image).to receive(:attachment_blob) + expect(new_image).to receive_message_chain(:attachment, :attach) expect(new_property).to receive(:created_at=).with(nil) expect(new_property).to receive(:updated_at=).with(nil) diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index f7024d0e9b..52debc535a 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -6,91 +6,54 @@ module Spree describe Image do include FileHelper + subject { + Spree::Image.create!( + attachment: black_logo_file, + viewable: product.master, + ) + } let(:product) { create(:product) } + describe "#url" do + it "returns URLs for different sizes" do + expect(subject.url(:small)).to match( + %r|^http://test\.host/rails/active_storage/representations/redirect/.+/logo-black\.png$| + ) + end + + it "returns nil for unsupported formats" do + subject.attachment_blob.update_columns( + content_type: "application/octet-stream" + ) + + expect(subject.url(:small)).to eq nil + end + end + describe "using local storage" do it "stores a new image" do - image = Spree::Image.create!( - attachment: black_logo_file, - viewable: product.master, - ) + attachment = subject.attachment + expect(attachment.attached?).to eq true - attachment = image.attachment - expect(attachment.exists?).to eq true - expect(attachment.file?).to eq true - expect(attachment.url).to match %r"^/spree/products/[0-9]+/product/logo-black\.png\?[0-9]+$" - end - - it "duplicates the image with Active Storage" do - image = Spree::Image.create!( - attachment: file, - viewable: product.master, - ) - - attachment = image.active_storage_attachment url = Rails.application.routes.url_helpers.url_for(attachment) - expect(url).to match %r|^http://test\.host/rails/active_storage/blobs/redirect/[[:alnum:]-]+/logo-black\.png$| end end describe "using AWS S3" do - let(:s3_config) { - { - url: ":s3_alias_url", - storage: :s3, - s3_credentials: { - access_key_id: "A...A", - secret_access_key: "H...H", - }, - s3_headers: { "Cache-Control" => "max-age=31557600" }, - bucket: "ofn", - s3_protocol: "https", - s3_host_alias: "ofn.s3.us-east-1.amazonaws.com", - - # This is for easier testing: - path: "/:id/:style/:basename.:extension", - } - } - before do - attachment_definition = Spree::Image.attachment_definitions[:attachment] - allow(Spree::Image).to receive(:attachment_definitions).and_return( - attachment: attachment_definition.merge(s3_config) - ) allow(Rails.application.config.active_storage). to receive(:service).and_return(:test_amazon) end it "saves a new image when none is present" do - # Paperclip requests - upload_pattern = %r"^https://ofn.s3.amazonaws.com/[0-9]+/(original|mini|small|product|large)/logo-black.png$" - download_pattern = %r"^https://ofn.s3.amazonaws.com/[0-9]+/product/logo-black.png$" - public_url_pattern = %r"^https://ofn.s3.us-east-1.amazonaws.com/[0-9]+/product/logo-black.png\?[0-9]+$" - - stub_request(:put, upload_pattern).to_return(status: 200, body: "", headers: {}) - stub_request(:head, download_pattern).to_return(status: 200, body: "", headers: {}) - # Active Storage requests as_upload_pattern = %r"^https://ofn.s3.amazonaws.com/[[:alnum:]]+$" stub_request(:put, as_upload_pattern).to_return(status: 200, body: "", headers: {}) - image = Spree::Image.create!( - attachment: black_logo_file, - viewable: product.master, - ) - - # Paperclip - attachment = image.attachment - expect(attachment.exists?).to eq true - expect(attachment.file?).to eq true - expect(attachment.url).to match public_url_pattern - - # Active Storage - attachment = image.active_storage_attachment - expect(attachment).to be_attached - expect(Rails.application.routes.url_helpers.url_for(attachment)). + expect(subject.attachment).to be_attached + expect(Rails.application.routes.url_helpers.url_for(subject.attachment)). to match %r"^http://test\.host/rails/active_storage/blobs/redirect/[[:alnum:]-]+/logo-black\.png" end end diff --git a/spec/services/image_importer_spec.rb b/spec/services/image_importer_spec.rb index 88ba329786..9ec390d25f 100644 --- a/spec/services/image_importer_spec.rb +++ b/spec/services/image_importer_spec.rb @@ -15,7 +15,7 @@ describe ImageImporter do }.by(1) expect(product.images.count).to eq 1 - expect(product.images.first.attachment.size).to eq 6274 + expect(product.images.first.attachment_blob.byte_size).to eq 6274 end end end diff --git a/spec/support/api_helper.rb b/spec/support/api_helper.rb index 37d1d6a4d1..7632c90c86 100644 --- a/spec/support/api_helper.rb +++ b/spec/support/api_helper.rb @@ -28,7 +28,7 @@ module OpenFoodNetwork end def image(filename) - File.open(Rails.root + "spec/support/fixtures" + filename) + Rack::Test::UploadedFile.new(Rails.root + "spec/support/fixtures" + filename) end end end From 86731d7e30b5c66217baeaea958224a8e1e8e387 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 22 Apr 2022 15:51:05 +1000 Subject: [PATCH 07/10] Remove compatibility code for migrating files The migration should be complete now. --- app/models/concerns/has_migrating_file.rb | 67 ----------------------- 1 file changed, 67 deletions(-) delete mode 100644 app/models/concerns/has_migrating_file.rb diff --git a/app/models/concerns/has_migrating_file.rb b/app/models/concerns/has_migrating_file.rb deleted file mode 100644 index 923ee59ba3..0000000000 --- a/app/models/concerns/has_migrating_file.rb +++ /dev/null @@ -1,67 +0,0 @@ -# frozen_string_literal: true - -module HasMigratingFile - extend ActiveSupport::Concern - - @migrating_models = [] - - def self.migrating_models - @migrating_models - end - - included do - HasMigratingFile.migrating_models.push(name) - end - - class_methods do - def has_one_migrating(name, paperclip_options = {}) - # Active Storage declaration - has_one_attached name - - # Backup Active Storage methods before they get overridden by Paperclip. - alias_method "active_storage_#{name}", name - alias_method "active_storage_#{name}=", "#{name}=" - - # Paperclip declaration - # - # This will define the `name` and `name=` methods as well. - has_attached_file name, paperclip_options - - # Paperclip callback to duplicate file with Active Storage - # - # We store files with Paperclip *and* Active Storage while we migrate - # old Paperclip files to Active Storage. This enables availability - # during the migration. - public_send("after_#{name}_post_process") do - path = processed_local_file_path(name) - if public_send(name).errors.blank? && path.present? - attach_file(name, File.open(path)) - end - end - end - end - - def attach_file(name, io) - attachable = { - io: io, - filename: public_send("#{name}_file_name"), - content_type: public_send("#{name}_content_type"), - identify: false, - } - public_send("active_storage_#{name}=", attachable) - end - - private - - def processed_local_file_path(name) - attachment = public_send(name) - - temporary = attachment.queued_for_write[:original] - - if temporary&.path.present? - temporary.path - else - attachment.path - end - end -end From bea080a9b1f204a7eda7c8de748454c258f0a9c1 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 25 Apr 2022 14:14:39 +1000 Subject: [PATCH 08/10] Remove Paperclip It has been replaced by Active Storage. --- .rubocop_todo.yml | 1 - Gemfile | 1 - Gemfile.lock | 13 --------- config/initializers/paperclip.rb | 46 -------------------------------- lib/spree/core.rb | 1 - lib/spree/core/s3_support.rb | 35 ------------------------ spec/base_spec_helper.rb | 2 -- 7 files changed, 99 deletions(-) delete mode 100644 config/initializers/paperclip.rb delete mode 100644 lib/spree/core/s3_support.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 3dba2bd123..0e9cdb3975 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -441,7 +441,6 @@ Metrics/AbcSize: - 'lib/reporting/reports/payments/payments_report.rb' - 'lib/reporting/reports/sales_tax/sales_tax_report.rb' - 'lib/spree/core/controller_helpers/order.rb' - - 'lib/spree/core/s3_support.rb' - 'lib/tasks/enterprises.rake' - 'spec/services/order_checkout_restart_spec.rb' diff --git a/Gemfile b/Gemfile index a6ec2e5a78..1725e5e0d1 100644 --- a/Gemfile +++ b/Gemfile @@ -86,7 +86,6 @@ gem 'bootsnap', require: false gem 'geocoder' gem 'gmaps4rails' gem 'mimemagic', '> 0.3.5' -gem 'paperclip', '~> 3.4.1' gem 'paper_trail', '~> 12.1.0' gem 'rack-rewrite' gem 'rack-ssl', require: 'rack/ssl' diff --git a/Gemfile.lock b/Gemfile.lock index 13fb10770e..9001b74101 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -219,10 +219,7 @@ GEM rubyzip (>= 1.3.0, < 3) childprocess (4.1.0) chronic (0.10.2) - climate_control (0.2.0) cliver (0.3.2) - cocaine (0.5.8) - climate_control (>= 0.0.3, < 1.0) coderay (1.1.3) coffee-rails (5.0.0) coffee-script (>= 2.2.0) @@ -392,9 +389,6 @@ GEM marcel (1.0.2) matrix (0.4.2) method_source (1.0.0) - mime-types (3.3.1) - mime-types-data (~> 3.2015) - mime-types-data (3.2021.0225) mimemagic (0.4.3) nokogiri (~> 1) rake @@ -427,12 +421,6 @@ GEM paper_trail (12.1.0) activerecord (>= 5.2) request_store (~> 1.1) - paperclip (3.4.2) - activemodel (>= 3.0.0) - activerecord (>= 3.0.0) - activesupport (>= 3.0.0) - cocaine (~> 0.5.0) - mime-types parallel (1.21.0) paranoia (2.4.3) activerecord (>= 4.0, < 6.2) @@ -785,7 +773,6 @@ DEPENDENCIES order_management! pagy (~> 5.1) paper_trail (~> 12.1.0) - paperclip (~> 3.4.1) paranoia (~> 2.4) paypal-sdk-merchant (= 1.117.2) pdf-reader diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb deleted file mode 100644 index a58c6bdf9d..0000000000 --- a/config/initializers/paperclip.rb +++ /dev/null @@ -1,46 +0,0 @@ -Paperclip::Attachment.default_options[:source_file_options] = { - all: "-auto-orient" -} - -url_adapters = [ - "Paperclip::UriAdapter", - "Paperclip::HttpUrlProxyAdapter", - "Paperclip::DataUriAdapter" -] - -# Remove Paperclip URL adapters from registered handlers -Paperclip.io_adapters.registered_handlers.delete_if do |_proc, adapter_class| - url_adapters.include? adapter_class.to_s -end - -if Paperclip::VERSION.to_f < 3.5 - if Rails::VERSION::MAJOR > 4 - # Patches an error for missing method #silence_stream with Rails 5.0 - # Can be removed after Paperclip is upgraded to 3.5+ - module Paperclip - class GeometryDetector - def silence_stream(_stream, &block) - yield - end - end - end - end -else - Rails.logger.warn "The Paperclip::GeometryDetector patch can now be removed." -end - -module UpdatedUrlGenerator - def escape_url(url) - (url.respond_to?(:escape) ? url.escape : URI::Parser.new.escape(url)). - gsub(/(\/.+)\?(.+\.)/, '\1%3F\2') - end -end - -module PaperclipImageErrors - def mark_invalid(record, attribute, types) - record.errors.add attribute, :invalid, **options.merge(:types => types.join(', ')) - end -end - -Paperclip::UrlGenerator.prepend(UpdatedUrlGenerator) -Paperclip::Validators::AttachmentPresenceValidator.prepend(PaperclipImageErrors) diff --git a/lib/spree/core.rb b/lib/spree/core.rb index 900da7c45b..3daa196700 100644 --- a/lib/spree/core.rb +++ b/lib/spree/core.rb @@ -6,7 +6,6 @@ require 'awesome_nested_set' require 'cancan' require 'pagy' require 'mail' -require 'paperclip' require 'paranoia' require 'ransack' require 'state_machines' diff --git a/lib/spree/core/s3_support.rb b/lib/spree/core/s3_support.rb deleted file mode 100644 index 92229c15f5..0000000000 --- a/lib/spree/core/s3_support.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module Spree - module Core - # This module exists to reduce duplication in S3 settings between - # the Image and Taxon models in Spree - module S3Support - extend ActiveSupport::Concern - - included do - def self.supports_s3(field) - # Load user defined paperclip settings - config = Spree::Config - return unless config[:use_s3] - - s3_creds = { access_key_id: config[:s3_access_key], - secret_access_key: config[:s3_secret], - bucket: config[:s3_bucket] } - attachment_definitions[field][:storage] = :s3 - attachment_definitions[field][:s3_credentials] = s3_creds - attachment_definitions[field][:s3_headers] = ActiveSupport::JSON. - decode(config[:s3_headers]) - attachment_definitions[field][:bucket] = config[:s3_bucket] - if config[:s3_protocol].present? - attachment_definitions[field][:s3_protocol] = config[:s3_protocol].downcase - end - - return if config[:s3_host_alias].blank? - - attachment_definitions[field][:s3_host_alias] = config[:s3_host_alias] - end - end - end - end -end diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index 617ed81a84..5b865822de 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -14,7 +14,6 @@ require 'rspec/rails' require 'capybara' require 'rspec/retry' require 'paper_trail/frameworks/rspec' -require "paperclip/matchers" require "factory_bot_rails" require 'shoulda/matchers' @@ -126,7 +125,6 @@ RSpec.configure do |config| config.infer_spec_type_from_file_location! config.include FactoryBot::Syntax::Methods - config.include Paperclip::Shoulda::Matchers config.include JsonSpec::Helpers config.include Rails.application.routes.url_helpers From 4facab03359eb54209f855d5acc33141699ae412 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 1 Jun 2022 17:14:30 +1000 Subject: [PATCH 09/10] Guard against invariable file types Australian production had one JPG image which was not recognised as such. The `content_type` was missing and trying to generate a URL for a variant raised an error and crashed the page. Testing for `variable?` includes testing for `attached?` and is more defensive. --- app/models/enterprise.rb | 4 ++-- app/serializers/api/admin/enterprise_serializer.rb | 2 +- app/views/groups/show.html.haml | 4 ++-- app/views/shopping_shared/_header.html.haml | 2 +- app/views/spree/admin/orders/invoice2.html.haml | 2 +- app/views/spree/order_mailer/cancel_email.html.haml | 2 +- .../spree/order_mailer/confirm_email_for_customer.html.haml | 2 +- app/views/spree/order_mailer/confirm_email_for_shop.html.haml | 2 +- app/views/subscription_mailer/_header.html.haml | 2 +- .../subscription_mailer/confirmation_summary_email.html.haml | 2 +- .../subscription_mailer/placement_summary_email.html.haml | 2 +- 11 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index f355c25f42..461f08f919 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -277,7 +277,7 @@ class Enterprise < ApplicationRecord end def logo_url(name) - return unless logo.attached? + return unless logo.variable? Rails.application.routes.url_helpers.url_for( logo.variant(LOGO_SIZES[name]) @@ -285,7 +285,7 @@ class Enterprise < ApplicationRecord end def promo_image_url(name) - return unless promo_image.attached? + return unless promo_image.variable? Rails.application.routes.url_helpers.url_for( promo_image.variant(PROMO_IMAGE_SIZES[name]) diff --git a/app/serializers/api/admin/enterprise_serializer.rb b/app/serializers/api/admin/enterprise_serializer.rb index 21f08a3492..2071024f3e 100644 --- a/app/serializers/api/admin/enterprise_serializer.rb +++ b/app/serializers/api/admin/enterprise_serializer.rb @@ -89,7 +89,7 @@ module Api # medium: LOGO_MEDIUM_URL # } def attachment_urls(attachment, styles) - return unless attachment.attached? + return unless attachment.variable? styles.transform_values do |transformation| Rails.application.routes.url_helpers. diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index b8eca916de..13966cba3f 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -22,10 +22,10 @@ %header .row .small-12.columns - = image_tag @group.promo_image.variant(resize_to_limit: [1200, 260]) if @group.promo_image.attached? + = image_tag @group.promo_image.variant(resize_to_limit: [1200, 260]) if @group.promo_image.variable? .row .small-12.columns.group-header.pad-top - - if @group.logo.attached? + - if @group.logo.variable? = image_tag @group.logo.variant(resize_to_limit: [100, 100]), class: "group-logo" - else = image_tag "/noimage/group.png", class: "group-logo" diff --git a/app/views/shopping_shared/_header.html.haml b/app/views/shopping_shared/_header.html.haml index 45d4d9c952..144c3fc427 100644 --- a/app/views/shopping_shared/_header.html.haml +++ b/app/views/shopping_shared/_header.html.haml @@ -4,7 +4,7 @@ %distributor.details.row .small-12.medium-12.large-8.columns #distributor_title - - if distributor.logo.attached? + - if distributor.logo.variable? = image_tag distributor.logo_url(:thumb), class: "left" = render DistributorTitleComponent.new(name: distributor.name) %location= distributor.address.city diff --git a/app/views/spree/admin/orders/invoice2.html.haml b/app/views/spree/admin/orders/invoice2.html.haml index 1e8545cfb1..be43fb767c 100644 --- a/app/views/spree/admin/orders/invoice2.html.haml +++ b/app/views/spree/admin/orders/invoice2.html.haml @@ -6,7 +6,7 @@ %td{ :align => "left" } %h4 = t :tax_invoice - - if @order.distributor.display_invoice_logo? && @order.distributor.logo.attached? + - if @order.distributor.display_invoice_logo? && @order.distributor.logo.variable? %td{ :align => "right", rowspan: 2 } = wicked_pdf_image_tag @order.distributor.logo.variant(resize_to_limit: [150, 150]) %tr{ valign: "top" } diff --git a/app/views/spree/order_mailer/cancel_email.html.haml b/app/views/spree/order_mailer/cancel_email.html.haml index 4854c417b1..edfc66f162 100755 --- a/app/views/spree/order_mailer/cancel_email.html.haml +++ b/app/views/spree/order_mailer/cancel_email.html.haml @@ -6,7 +6,7 @@ = t(".customer_greeting", name: @order.bill_address.firstname) %h4 = t(".instructions_html", distributor: @order.distributor.name ) - - if @order.distributor.logo.attached? + - if @order.distributor.logo.variable? = image_tag @order.distributor.logo_url(:medium) %p.callout diff --git a/app/views/spree/order_mailer/confirm_email_for_customer.html.haml b/app/views/spree/order_mailer/confirm_email_for_customer.html.haml index c7f1597128..8894805243 100644 --- a/app/views/spree/order_mailer/confirm_email_for_customer.html.haml +++ b/app/views/spree/order_mailer/confirm_email_for_customer.html.haml @@ -11,7 +11,7 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - - if @order.distributor.logo.attached? + - if @order.distributor.logo.variable? = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear diff --git a/app/views/spree/order_mailer/confirm_email_for_shop.html.haml b/app/views/spree/order_mailer/confirm_email_for_shop.html.haml index a86c6cda88..0e5b4510bf 100644 --- a/app/views/spree/order_mailer/confirm_email_for_shop.html.haml +++ b/app/views/spree/order_mailer/confirm_email_for_shop.html.haml @@ -11,7 +11,7 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - - if @order.distributor.logo.attached? + - if @order.distributor.logo.variable? = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear diff --git a/app/views/subscription_mailer/_header.html.haml b/app/views/subscription_mailer/_header.html.haml index 1f70213378..6f7a593669 100644 --- a/app/views/subscription_mailer/_header.html.haml +++ b/app/views/subscription_mailer/_header.html.haml @@ -11,6 +11,6 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - - if @order.distributor.logo.attached? + - if @order.distributor.logo.variable? = image_tag @order.distributor.logo_url(:medium), class: "float-right" %span.clear diff --git a/app/views/subscription_mailer/confirmation_summary_email.html.haml b/app/views/subscription_mailer/confirmation_summary_email.html.haml index c6279620a1..4cdefdffe1 100644 --- a/app/views/subscription_mailer/confirmation_summary_email.html.haml +++ b/app/views/subscription_mailer/confirmation_summary_email.html.haml @@ -11,7 +11,7 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - - if @shop.logo.attached? + - if @shop.logo.variable? = image_tag @shop.logo_url(:medium), class: "float-right" %span.clear diff --git a/app/views/subscription_mailer/placement_summary_email.html.haml b/app/views/subscription_mailer/placement_summary_email.html.haml index 292496fdc4..3c8a21baca 100644 --- a/app/views/subscription_mailer/placement_summary_email.html.haml +++ b/app/views/subscription_mailer/placement_summary_email.html.haml @@ -11,7 +11,7 @@ %table.column{:align => "left"} %tr %td{:align => "right"} - - if @shop.logo.attached? + - if @shop.logo.variable? = image_tag @shop.logo_variant(:medium), class: "float-right" %span.clear From 076efd653d04cc6701b6c0da326eb0242a6de5d4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 2 Jun 2022 12:13:34 +1000 Subject: [PATCH 10/10] Correct checksum of big files stored on AWS S3 --- ...0602013938_compute_checksum_for_big_files.rb | 17 +++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220602013938_compute_checksum_for_big_files.rb diff --git a/db/migrate/20220602013938_compute_checksum_for_big_files.rb b/db/migrate/20220602013938_compute_checksum_for_big_files.rb new file mode 100644 index 0000000000..697f731e5d --- /dev/null +++ b/db/migrate/20220602013938_compute_checksum_for_big_files.rb @@ -0,0 +1,17 @@ +# When migrating to Active Storage, we used Amazon's ETag for the blob +# checksum. But big files have been uploaded in chunks and then the checksum +# differs. We need to recalculate the checksum for large files. +class ComputeChecksumForBigFiles < ActiveRecord::Migration[6.1] + def up + blobs_with_incorrect_checksum.find_each do |blob| + md5 = Digest::MD5.base64digest(blob.download) + blob.update(checksum: md5) + end + end + + def blobs_with_incorrect_checksum + ActiveStorage::Blob. + where(service_name: "amazon"). + where("byte_size >= 20000000") + end +end diff --git a/db/schema.rb b/db/schema.rb index 0ad1f12db3..010059c3d6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_04_10_162955) do +ActiveRecord::Schema.define(version: 2022_06_02_013938) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql"