Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveStorage support #2974

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ jobs:
parallelism: *parallelism
steps: *steps

postgres_activestorage:
<<: *postgres
environment:
<<: *environment
ACTIVE_STORAGE: 'true'

postgres_rails51:
<<: *postgres
environment:
Expand All @@ -79,3 +85,4 @@ workflows:
- mysql
- postgres_rails51
- mysql_rails51
- postgres_activestorage
5 changes: 5 additions & 0 deletions api/app/controllers/spree/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ class BaseController < ActionController::Base
before_action :authorize_for_order, if: proc { order_token.present? }
before_action :authenticate_user
before_action :load_user_roles
if defined? ActiveStorage
before_action do
ActiveStorage::Current.host = request.base_url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about extracting it into a method? It could be reused in that way (I noticed it's used elsewhere)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to stick to how it's done within AS, I think it will probably need to be revised once Rails 5.1 support is dropped

end
end

rescue_from ActionController::ParameterMissing, with: :parameter_missing_error
rescue_from ActiveRecord::RecordNotFound, with: :not_found
Expand Down
2 changes: 1 addition & 1 deletion api/app/views/spree/api/images/_image.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
json.(image, *image_attributes)
json.(image, :viewable_type, :viewable_id)
Spree::Image.attachment_definitions[:attachment][:styles].each do |k, _v|
json.set! "#{k}_url", image.attachment.url(k)
json.set! "#{k}_url", image.url(k)
end
2 changes: 1 addition & 1 deletion backend/app/views/spree/admin/taxons/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<%= f.field_container :icon do %>
<%= f.label :icon %><br />
<%= f.file_field :icon %>
<%= image_tag f.object.icon(:mini) if f.object.icon.present? %>
<%= image_tag f.object.icon(:mini) if f.object.icon_present? %>
<% end %>
</div>

Expand Down
6 changes: 6 additions & 0 deletions core/app/controllers/spree/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,11 @@ class Spree::BaseController < ApplicationController
include Spree::Core::ControllerHelpers::Store
include Spree::Core::ControllerHelpers::StrongParameters

if defined? ActiveStorage
before_action do
ActiveStorage::Current.host = request.base_url
end
end

respond_to :html
end
66 changes: 66 additions & 0 deletions core/app/models/concerns/spree/active_storage_attachment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# frozen_string_literal: true

module Spree
elia marked this conversation as resolved.
Show resolved Hide resolved
module ActiveStorageAttachment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you added a second module for active storage support? Why not have these methods in the other module as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is included by both AS implementations (Image and Taxon) and is an extraction of repeated code. The call between extracting/DRYing and leaving the duplication was, admittedly, close. But in the end I ended up refactoring in order to avoid the risk of leaving one of the two behind when fixing stuff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ActiveStorage is meant to add attachments to any model I like the idea here.

extend ActiveSupport::Concern

# @private
def self.attachment_variant(attachment, style:, default_style:, styles:)
return unless attachment && attachment.attachment

if style.nil? || style == default_style
attachment_variant = attachment
else
attachment_variant = attachment.variant(
resize: styles[style.to_sym],
strip: true,
'auto-orient': true,
colorspace: 'sRGB',
).processed
end

attachment_variant
end

class_methods do
def redefine_attachment_writer_with_legacy_io_support(name)
define_method :"#{name}=" do |attachable|
attachment = public_send(name)

case attachable
when ActiveStorage::Blob, ActionDispatch::Http::UploadedFile,
Rack::Test::UploadedFile, Hash, String
attachment.attach(attachable)
when ActiveStorage::Attached
attachment.attach(attachable.blob)
else # assume it's an IO
if attachable.respond_to?(:to_path)
filename = attachable.to_path
else
filename = SecureRandom.uuid
end
attachable.rewind

attachment.attach(
io: attachable,
filename: filename
)
end
end
end

def validate_attachment_to_be_an_image(name)
method_name = :"attached_#{name}_is_an_image"

define_method method_name do
attachment = public_send(name)
next if attachment.nil? || attachment.attachment.nil?

errors.add name, 'is not an image' unless attachment.attachment.image?
end

validate method_name
end
end
end
end
64 changes: 21 additions & 43 deletions core/app/models/spree/image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,55 +2,33 @@

module Spree
class Image < Asset
validate :no_attachment_errors

has_attached_file :attachment,
styles: { mini: '48x48>', small: '100x100>', product: '240x240>', large: '600x600>' },
default_style: :product,
default_url: 'noimage/:style.png',
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' }
validates_attachment :attachment,
presence: true,
content_type: { content_type: %w(image/jpeg image/jpg image/png image/gif) }
if ::Spree::Config.image_attachment_module.blank?
Spree::Deprecation.warn <<-MESSAGE.strip_heredoc + "\n\n"
Using Paperclip as image_attachment_module for Solidus.

Please configure Spree::Config.image_attachment_module in your store
initializer.

To use the ActiveStorage adapter (recommended):
Spree.config do |config|
config.image_attachment_module = 'Spree::Image::ActiveStorageAttachment'
end

To use the Paperclip adapter (legacy, deprecated):
Spree.config do |config|
config.image_attachment_module = 'Spree::Image::PaperclipAttachment'
end
MESSAGE
::Spree::Config.image_attachment_module = 'Spree::Image::PaperclipAttachment'
end

# 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, if: :valid?
include ::Spree::Config.image_attachment_module.to_s.constantize

def mini_url
Spree::Deprecation.warn(
'Spree::Image#mini_url is DEPRECATED. Use Spree::Image#url(:mini) instead.'
)
attachment.url(:mini, false)
end

def url(size)
attachment.url(size)
end

def filename
attachment_file_name
end

def find_dimensions
temporary = attachment.queued_for_write[:original]
filename = temporary.path unless temporary.nil?
filename = attachment.path if filename.blank?
geometry = Paperclip::Geometry.from_file(filename)
self.attachment_width = geometry.width
self.attachment_height = geometry.height
end

# if there are errors from the plugin, then add a more meaningful message
def no_attachment_errors
unless attachment.errors.empty?
# uncomment this to get rid of the less-than-useful interim messages
# errors.clear
errors.add :attachment, "Paperclip returned errors for file '#{attachment_file_name}' - check ImageMagick installation or image source file."
false
end
url(:mini, false)
end
end
end
92 changes: 92 additions & 0 deletions core/app/models/spree/image/active_storage_attachment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# frozen_string_literal: true

require 'active_storage'

module Spree::Image::ActiveStorageAttachment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use to redefine modules/classes into existing ones by repeating their inheritances on each level, see #2572 for example for more information. In this case, it would be:

module Spree
  class Image < Asset
    module ActiveStorageAttachment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, but used this way there's no risk of incurring in the problem described in #2572 (at least I don't see it). The problem there was using the class keyword on an autoloaded constant with different superclass declaration.

Sometimes it was class A < B, sometimes class A, depending on load-order if the second form was loaded first it would result in an error (the other way around works because you can omit the superclass when reopening a class).

Let me know if the change is still needed 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this module is someway loaded before Spree::Image?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will autoload Spree::Image and then if it's the configured module will go back and load itself.
That said I tried to load it by itself but I didn't succeed, so I don't think that's an issue, do you have any specific case in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think it can work but I'd still change it for consistency. I also found this related PR #2098 and I think the goal there was to explicitly inform the type of all the modules/classes involved in the inheritance.

extend ActiveSupport::Concern
include Spree::ActiveStorageAttachment

module DeprecatedPaperclipAPI
def attachment(*args)
if args.size == 1
# TODO: deprecation
style = args.first
Spree::ActiveStorageAttachment.attachment_variant(
super(),
style: style,
default_style: default_style,
styles: ATTACHMENT_STYLES
)
else
# With 0 args will be ok, otherwise will raise an ArgumentError
super
end
end
end

included do
has_one_attached :attachment
redefine_attachment_writer_with_legacy_io_support :attachment
validate_attachment_to_be_an_image :attachment
validates :attachment, presence: true
prepend DeprecatedPaperclipAPI
end

class_methods do
def attachment_definitions
{ attachment: { styles: ATTACHMENT_STYLES } }
end
end

ATTACHMENT_STYLES = {
mini: '48x48>',
small: '100x100>',
product: '240x240>',
large: '600x600>',
}

def default_style
:original
end

def url(style = default_style, options = {})
options = normalize_url_options(options)

Spree::ActiveStorageAttachment.attachment_variant(
attachment,
style: style,
default_style: default_style,
styles: ATTACHMENT_STYLES
)&.service_url(options)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token error

end

def filename
attachment.blob.filename.to_s
end

def attachment_width
attachment.metadata[:width]
end

def attachment_height
attachment.metadata[:height]
end

def attachment_present?
attachment.attached?
end

private

def normalize_url_options(options)
if [true, false].include? options # Paperclip backwards compatibility.
Spree::Deprecation.warn(
"Using #{self.class}#url with true/false as second parameter is deprecated, if you "\
"want to enable/disable timestamps pass `timestamps: true` (or `false`)."
)
options = { timestamp: options }
end

options
end
end
55 changes: 55 additions & 0 deletions core/app/models/spree/image/paperclip_attachment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# frozen_string_literal: true

module Spree::Image::PaperclipAttachment
extend ActiveSupport::Concern

included do
validate :no_attachment_errors

has_attached_file :attachment,
styles: { mini: '48x48>', small: '100x100>', product: '240x240>', large: '600x600>' },
default_style: :product,
default_url: 'noimage/:style.png',
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' }
validates_attachment :attachment,
presence: true,
content_type: { content_type: %w[image/jpeg image/jpg image/png image/gif] }

# 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, if: :valid?
end

def url(size)
attachment.url(size)
end

def filename
attachment_file_name
end

def attachment_present?
attachment.present?
end

def find_dimensions
temporary = attachment.queued_for_write[:original]
filename = temporary.path unless temporary.nil?
filename = attachment.path if filename.blank?
geometry = Paperclip::Geometry.from_file(filename)
self.attachment_width = geometry.width
self.attachment_height = geometry.height
end

# if there are errors from the plugin, then add a more meaningful message
def no_attachment_errors
unless attachment.errors.empty?
# uncomment this to get rid of the less-than-useful interim messages
# errors.clear
errors.add :attachment, "Paperclip returned errors for file '#{attachment_file_name}' - check ImageMagick installation or image source file."
false
end
end
end
30 changes: 21 additions & 9 deletions core/app/models/spree/taxon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,27 @@ class Taxon < Spree::Base
after_save :touch_ancestors_and_taxonomy
after_touch :touch_ancestors_and_taxonomy

has_attached_file :icon,
styles: { mini: '32x32>', normal: '128x128>' },
default_style: :mini,
url: '/spree/taxons/:id/:style/:basename.:extension',
path: ':rails_root/public/spree/taxons/:id/:style/:basename.:extension',
default_url: '/assets/default_taxon.png'

validates_attachment :icon,
content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] }
if ::Spree::Config.taxon_attachment_module.blank?
Spree::Deprecation.warn <<-MESSAGE.strip_heredoc + "\n\n"
Using Paperclip as taxon_attachment_module for Taxon.

Please configure Spree::Config.taxon_attachment_module in your store
initializer.

To use the ActiveStorage adapter (recommended):
Spree.config do |config|
config.image_attachment_module = 'Spree::Taxon::ActiveStorageAttachment'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be config.taxon_attachement_module

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks good catch!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still unsolved?

end

To use the Paperclip adapter (legacy, deprecated):
Spree.config do |config|
config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment'
end
MESSAGE
::Spree::Config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment'
end

include ::Spree::Config.taxon_attachment_module.to_s.constantize

self.whitelisted_ransackable_attributes = %w[name]

Expand Down
Loading