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.

Sign up for GitHub

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

Jump to bottom

Add ActiveStorage adapter #3501

Merged
merged 8 commits into from
Apr 20, 2020
Merged

Add ActiveStorage adapter #3501

merged 8 commits into from
Apr 20, 2020

Conversation

filippoliverani
Copy link
Contributor

@filippoliverani filippoliverani commented Jan 31, 2020 •
edited
Loading

Description
This PR stems from the discussion here #2725 and builds upon @elia's work here #2974 and here #3237 🙏 .

From Rails 6.1 ActiveStorage will support public blob URLs (rails/rails@94584c2) and Solidus should be ready to offer an ActiveStorage adapter at least for new stores.

This PR adds an experimental ActiveStorage support leveraging config.image_attachment_module and config.taxon_attachment_module extension points. Solidus attachments heavily rely on Paperclip API, to avoid any change in existing code I tried to adapt ActiveStorage API to Paperclip attachment public API.

To run tests using ActiveStorage, ENABLE_ACTIVE_STORAGE env var must be set and Rails version should be >= 6.1.0.alpha/master.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

@filippoliverani filippoliverani marked this pull request as ready for review January 31, 2020 13:00
@elia elia mentioned this pull request Mar 6, 2020
Closed
7 tasks
houndci-bot
houndci-bot reviewed Mar 10, 2020
View reviewed changes
core/lib/spree/testing_support/dummy_app.rb Outdated Show resolved Hide resolved
core/spec/models/spree/taxon_spec.rb Outdated Show resolved Hide resolved
core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/lib/spree/core.rb Outdated Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Show resolved Hide resolved
@filippoliverani filippoliverani mentioned this pull request Mar 13, 2020
Merged
2 tasks
@filippoliverani filippoliverani force-pushed the add-active-storage-adapter branch 5 times, most recently from a693500 to b1dfa37 Compare March 20, 2020 11:28
elia
elia requested changes Mar 20, 2020
View reviewed changes
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

This is great! ✨
So good to see someone pick up the slack! 😄

I commented with a few nits and a proposal for getting rid of the methods added on Spree::Core.

Also would like to propose to reorder the commits so that they're easier to follow while reviewing (e.g. putting all the preparatory code at the begining, new specs, helper methods, etc. and leaving the core of the PR for the last commits), just an idea to save some skipping back and forth for those that review commit-by-commit.

core/lib/spree/testing_support/factories/image_factory.rb Outdated Show resolved Hide resolved
core/lib/spree/app_configuration.rb Show resolved Hide resolved
core/lib/spree/core/engine.rb Outdated Show resolved Hide resolved
core/lib/spree/awesome_nested_set_override.rb Outdated Show resolved Hide resolved
@filippoliverani filippoliverani force-pushed the add-active-storage-adapter branch 3 times, most recently from 33b5c23 to bb31261 Compare March 27, 2020 10:33
aldesantis
aldesantis requested changes Apr 3, 2020
View reviewed changes
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

This is amazing, thanks @filippoliverani. Just a couple of comments!

core/app/models/concerns/spree/active_storage_adapter/attachment.rb Outdated Show resolved Hide resolved
core/app/models/concerns/spree/active_storage_adapter.rb Outdated Show resolved Hide resolved
filippoliverani and others added 3 commits April 10, 2020 17:45
@filippoliverani
Add ActiveStorage attachments support
0ab905c 
To work seamlessly with the current code an adapter is needed to make
ActiveStorage attachments behave exactly like Paperclip ones.
@filippoliverani
Add check on Rails version if ActiveStorage enabled
3d99065 
Allow to enable ActiveStorage attachments adapter only if Rails version
is greater than 6.1.0.alpha. ActiveStorage Attachment adapter needs
public blob URLs which has been introduced in rails/rails@94584c2.
@filippoliverani
Set ActiveStorage host in base controllers
d802e5b 
ActiveStorage host is always required to make Disk service work
correctly.
filippoliverani and others added 5 commits April 10, 2020 17:45
@filippoliverani
Override broken awesome_nested_set set_depth! method
73cd0ef 
Skips a breaking model reload before NestedSet update depth logic.
This issue has already been addressed in:
collectiveidea/awesome_nested_set#413
The patch can be removed when a new version of awesome_nested_set will
be released.
@filippoliverani
Add ActiveStorage testing support
c01d6ac 
Paperclip is still the default storage adapter. To explicitly switch
tests to ActiveStorage, ENABLE_ACTIVE_STORAGE env variable must be set.
@filippoliverani
Improve Image and Taxon attachments test coverage
41df955
@filippoliverani
Add a CircleCI job with ActiveStorage adapter enabled
b097583
@filippoliverani
Move non-Paperclip dependent examples to Taxon spec
0ac3904
kennyadsl
kennyadsl approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Super awesome work @filippoliverani, thank you!

@kennyadsl kennyadsl merged commit 138bf61 into solidusio:master Apr 20, 2020
@kennyadsl kennyadsl mentioned this pull request Jan 8, 2021
Merged
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Mar 28, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
911ac5c 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
solidusio#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on [ImageMagick](see
https://legacy.imagemagick.org/Usage/thumbnails/)) to what ActiveStorage
expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
waiting-for-dev added a commit that referenced this pull request Mar 28, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
09b0107 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
@waiting-for-dev waiting-for-dev mentioned this pull request Mar 28, 2022
Merged
5 tasks
waiting-for-dev added a commit that referenced this pull request Mar 28, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
12e80cf 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
waiting-for-dev added a commit that referenced this pull request Mar 28, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
11a12f6 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
waiting-for-dev added a commit that referenced this pull request Apr 1, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
1284caf 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
waiting-for-dev added a commit that referenced this pull request Apr 5, 2022
@waiting-for-dev
Fixes defining thumbnail sizes through ActiveStorage adapter
b231ddd 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
mamhoff pushed a commit to mamhoff/solidus that referenced this pull request Apr 19, 2022
@waiting-for-dev @mamhoff
Fixes defining thumbnail sizes through ActiveStorage adapter
afea5f8 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
solidusio#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
rmparr pushed a commit to rmparr/solidus that referenced this pull request Jun 1, 2022
@waiting-for-dev @rmparr
Fixes defining thumbnail sizes through ActiveStorage adapter
4e57bff 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
solidusio#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
rmparr pushed a commit to rmparr/solidus that referenced this pull request Jun 1, 2022
@waiting-for-dev @rmparr
Fixes defining thumbnail sizes through ActiveStorage adapter
03f4661 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
solidusio#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
cpfergus1 pushed a commit to cpfergus1/solidus that referenced this pull request Aug 25, 2022
@waiting-for-dev @cpfergus1
Fixes defining thumbnail sizes through ActiveStorage adapter
a8274a9 
The adapter intends to make it possible to work with ActiveStorage as it
was Paperclip. We had been using Paperclip until Rails 6.1 introduced
ActiveStorage's public links. We moved to ActiveStorage as the default
file attachment backend from that point on, but we tried to make its API
compatible with Paperclip's one. See
solidusio#3501 for details.

When trying to translate [Paperclip's style
definitions](https://github.com/solidusio/solidus/blob/564b4fe54648537b6b1420eed5a05cba6854e915/core/lib/spree/app_configuration.rb#L494)
(based on
[ImageMagick](https://legacy.imagemagick.org/Usage/thumbnails/)) to what
ActiveStorage expects (which is [image_processing
syntax](https://github.com/janko/image_processing), common for
ImageMagick and libvips), we were transforming everything to a
[`resize_to_limit`](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#resize_to_limit)
option. However, that corresponds to the `>` symbol in ImageMagick, but
not to other options that can be given.

Whit this commit, we add support for defining other kinds of
transformations in ActiveStorage. However, we're still missing more
advanced options that can be given when using [standalone
Paperclip](https://www.rubydoc.info/gems/kt-paperclip/7.1.1) (example `{
geometry: '100x100', format: :jpg }`)  or [standalone
ActiveStorage](https://github.com/janko/image_processing/blob/master/doc/minimagick.md#methods)
(example `rotate: 90`). In the long run, we should aim to completely
differentiate how definitions are given for each library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@elia elia elia approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

@aldesantis aldesantis aldesantis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@filippoliverani @elia @kennyadsl @aldesantis @houndci-bot