Add concept of Image gallery #625

cbrunsdon · 2015-12-26T06:28:39Z

This PR comes out of the conversation of #495

We need a better way for stores to customize how different items relate to images, I believe we're missing the concept of a "gallery" that gives stores that control.

A single ActiveRecord association is too simple for stores to model the relationship between a variant (or product) and its images, this PR creates the concept of a "Gallery" for stores to implement and model that relationship as it pertains to their store.

To do this I needed to:

Shift existing calls onto this new api
mark old API as deprecated
create ProductAssociation for Spree::Product's
make sure preload_params are used everywhere to cut down on queries

I'm open to any and all feedback on how this is being executed, and especially around me documentation which I'm specifically trying to improve upon.

tvdeyen · 2015-12-26T10:46:03Z

Great stuff.

We also need some global image/media library where admins can choose the Images from and assign them to galleries.

Images need to be assignable multiple times so a n:m relation Spree::GalleryImage that joins Spree::Image and Spree::Gallery would be best fit IMO.

Galleries represent collections of images. They are the interface stores will implement to customize how objects in stores will associate with images. They also provide the functionality to pick what the "primary" and "best" images are, the images that best represent the gallery.

This implements the traditional Solidus behavior of using the ActiveRecord association between variants and the polymorphic "viewable" on images. It is a simple one-to-many relationship, but will fallback to a product to find a "best" image if no images exist on the association.

Historically many stores have overridden how a variant should be related to its images. This solidifies the extension points a store can hook into how a variant is related to its images.

This method's goal is to deprecate Spree::Variant#display_image display_image's goal was to allow the caller not to have to handle the case where a variant did not have an associated image by creating a Spree::Image.new and passing that back. This left too much responcibility in the Spree::Variant class and eventually led to us taking a boolean param to decide whether or not to fallback to product image if no variant is available, farther muddeling the API. Moving this to a helper puts it closer to the display logic (which is the only reason we'd want a Spree::Image.new) and more appropriate for the call. My bad.

Displays the given Spree::Image if one exists, or the default image otherwise. This is a convienience for the frontend/backend over a common task of showing an image in a style

This was my fault, I though the Spree::Variant creating its own fallback image if none existed would be a good move, it was not. It put too much responcibility inside the variant and made it too hard for callers to know where images were coming from (the product? the variant? either?) and got more complex when the fallback flag was added. Rather than using this, which was mostly to support view logic, the new calls in Spree::ImagesHelper(#spree_image_tag|#image_or_default) should be used.

It was deprecated in a previous commit.

Was deprecated in an earlier commit

This gallery is responsible for duplicating the existing behavior in association products to images. It continues the work in Spree::Gallery::Base to allow stores to extract out the logic for associating images to objects in their stores, giving a simple and clear extension point.

Just like Variant#display_image, this was a mistake to add to Product as deciding the fallback Spree::Image.new should not happen inside of the Product class. Callers could not know where this image came from (the master, or a variant), or could not easily know if no image existed when a blank new one was returned.

Was deprecated in an earlier commit.

Anyone implementing a Gallery class can use these example groups to ensure compatibility with our Core gallery

This will allow us to use our pluggable galleries but still preload images where desired

Both of these are the same (option_values is repeated in and outside of the hash)

This will allow the codebase to use whatever :included models are desired by the gallery, rather than hardcoding the associations throughout the codebase

Switches any instances I could find where preload associations were hardcoded to instead use the one defined by the active variant gallery

gmacdougall · 2016-01-07T14:13:34Z

api/app/controllers/spree/api/variants_controller.rb

+        def include_list
+          [{ option_values: :option_type }, :product, :default_price,
+           Spree::Config.variant_gallery_class.preload_params,
+           { stock_items: :stock_location }]


The readability of this could be improved by putting one thing per line:

[ :default_price, :product, { option_values: :option_type }, { stock_items: :stock_location }, Spree::Config.variant_gallery_class.preload_params ]

gmacdougall · 2016-01-07T14:19:22Z

I just have a couple minor formatting things that I'd prefer to see in order to keep the code looking tidy. Great work on this! 🌟

jordan-brough · 2016-01-13T13:14:33Z

I like how this is looking! One issue that comes to mind: I wonder if we're still tying this too closely to ActiveRecord.

E.g. this:

# frontend/app/controllers/spree/products_controller.rb
def show
  @variants = @product.variants.includes(..., Spree::Config.variant_gallery_class.preload_params)
  ...
end

Seems to be assuming that you can perform an efficient preload all the information you need to display the images for those variants using ActiveRecord includes. Is that correct? If so, I don't think that would work for a rule-based system like the one we are now using at Bonobos. Where does that particular variant-image preload get used btw? I took just a very quick look and couldn't immediately find it.

jordan-brough · 2016-01-13T13:21:40Z

To be clear: What I'm thinking we might need, very roughly, is something like:

def show
  @variants = ...
  @variant_image_displayer = VariantImageDiisplayer.new(@variants)
  ...
end

and somewhere in the view:

<%= spree_image_tag(@variant_image_displayer.image_for(variant)) %>

i.e. something that lets us do a preload in an efficient way, without relying on ActiveRecord includes.

tvdeyen · 2017-06-13T07:29:50Z

Closing as stale. Please reopen if you think this is still something we should to tackle.

cbrunsdon force-pushed the image_gallery branch from a9beee2 to d2467c5 Compare December 26, 2015 09:22

cbrunsdon force-pushed the image_gallery branch 9 times, most recently from ca980a7 to 4703a68 Compare January 6, 2016 19:04

Clarke Brunsdon added 15 commits January 6, 2016 11:10

Allow a variant's gallery to be configurable

864cd19

Historically many stores have overridden how a variant should be related to its images. This solidifies the extension points a store can hook into how a variant is related to its images.

Add spree_image_tag

b4c669a

Displays the given Spree::Image if one exists, or the default image otherwise. This is a convienience for the frontend/backend over a common task of showing an image in a style

Pull Spree::Variant#display_image out of frontend

894ff88

It was deprecated in a previous commit.

Pull Spree::Variant#display_image out of backend

70c39c2

Was deprecated in an earlier commit

Remove all calls to Spree::Product.display_image

1e915f8

Was deprecated in an earlier commit.

Add a shared example to test behaves like gallery

8860499

Anyone implementing a Gallery class can use these example groups to ensure compatibility with our Core gallery

Use preload_params on product gallery

90963e4

This will allow us to use our pluggable galleries but still preload images where desired

Remove duplication in :includes

8b6ea1a

Both of these are the same (option_values is repeated in and outside of the hash)

Add preload_params to the variant gallery

2959777

This will allow the codebase to use whatever :included models are desired by the gallery, rather than hardcoding the associations throughout the codebase

cbrunsdon force-pushed the image_gallery branch from 4703a68 to ff47a7c Compare January 6, 2016 23:30

cbrunsdon changed the title ~~[WIP] Add concept of Image gallery~~ Add concept of Image gallery Jan 6, 2016

cbrunsdon force-pushed the image_gallery branch from ff47a7c to 879e7b1 Compare January 6, 2016 23:48

Switch to :include the variant gallery preload

52606f7

Switches any instances I could find where preload associations were hardcoded to instead use the one defined by the active variant gallery

cbrunsdon force-pushed the image_gallery branch from 879e7b1 to 52606f7 Compare January 7, 2016 00:06

gmacdougall reviewed Jan 7, 2016
View reviewed changes

tvdeyen closed this Jun 13, 2017

This was referenced Oct 24, 2017

Add #gallery to Spree::Variant and Spree::Product #2323

Closed

[RFC] Move Paperclip into an extension #2324

Closed

swcraig mentioned this pull request Nov 6, 2017

Add #gallery to Variant and Product #2337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concept of Image gallery #625

Add concept of Image gallery #625

cbrunsdon commented Dec 26, 2015

tvdeyen commented Dec 26, 2015

gmacdougall Jan 7, 2016

gmacdougall commented Jan 7, 2016

jordan-brough commented Jan 13, 2016

jordan-brough commented Jan 13, 2016

tvdeyen commented Jun 13, 2017

Add concept of Image gallery #625

Add concept of Image gallery #625

Conversation

cbrunsdon commented Dec 26, 2015

tvdeyen commented Dec 26, 2015

gmacdougall Jan 7, 2016

Choose a reason for hiding this comment

gmacdougall commented Jan 7, 2016

jordan-brough commented Jan 13, 2016

jordan-brough commented Jan 13, 2016

tvdeyen commented Jun 13, 2017