Variant image rules

[richardnuno]

Begin using rules to administer how images are associated to
products/variants.

The current way to associate images to variants forces admins to
upload the same image multiple times if they want to associate an
image with multiple variants.

This change introduces variant image rules (similar to variant property
rules) which allow an admin to associate an image to multiple variants
at once, based on option values.

Taking for example a store that sells t-shirts, with a product that has
multiple variants that vary based on size and color. With variant image
rules, the admin can define a rule that will apply an image to green
shirts of all sizes, rather than upload the same image for the small,
medium and large green t-shirts.

The rules can be as specific or as generic as desired by the admin
(rules without any conditions will apply to all variants).

The change doesn't remove the current way of managing images but it does
introduce deprecation warnings, displays a warning in the current image
management admin page and provides a rake task to migrate current
variant images to the rule based logic.

[athal7]

i think a separate nested controller might make sense here rather than shoehorning into the current one. Probably same goes for variant property rules. Not sure if this is the right time to make that change or not.

[richardnuno]

I was hoping to have changes like that (as well as combining the ruleset models) in separate followup PRs. It's looking like a pretty big PR already and I think separate PRs would make it easier to review if everyone's OK with that.

[athal7]

seems reasonable to me, lets just make sure to follow through on it

[jhawthorn]

I would rather see that done initially. Especially combining the rulesets, which will require a migration if done later to copy rules from one table to the merged one and fix up the links.

[jordan-brough]

I think we should not combine the rulesets. We will get no significant performance benefit out of doing that, it makes for a more confusing data model, imo, and after chatting with Richard we already have some differences between the two that will make the logic uglier. If we have shared behavior between the two then let's make some modules or helpers to avoid excessive repetition. But I think we'll have a saner data model and cleaner code by keeping them separate.

[athal7]

👍 for composition

[jhawthorn]

I'll avoid some more detailed review as this is still a work in progress, but I'd like to see how the api/frontend-of-the-admin part of this would be handled. I noticed that the variant search select no longer has images for variants (since they no longer have images).

[jhawthorn]

Use try! not try.

Did you mean || or | here?

[richardnuno]

The || is intentional, I only want to load the associated images if there are no images determined by the rules.

[richardnuno]

@athal7 @jhawthorn added a rake task in the last commit that creates a product with over 4000 variants to test the performance implications of the change. Hoping it might be helpful in the future to test other scenarios.

On my local machine I get the following response times:

url: http://localhost:3000/api/products/ruby-on-rails-baseball-jersey
master: Completed 200 OK in 766ms (Views: 697.4ms | ActiveRecord: 46.4ms)
branch variant-image-rules (3 image rules): Completed 200 OK in 1058ms (Views: 963.5ms | ActiveRecord: 48.6ms)

url: http://localhost:3000/api/products/performance-shirt
master: Completed 200 OK in 248782ms (Views: 225584.3ms | ActiveRecord: 18565.9ms)
branch variant-image-rules (3 image rules): Completed 200 OK in 367021ms (Views: 344624.9ms | ActiveRecord: 18368.9ms)

As expected there is a performance difference. It's pretty significant with a large amount of variants but as @athal7 mentioned (and as you can see in the response time in master) there are already some existing performance issues with the endpoint.

I'm also very interested in hearing suggestions on how to tackle the performance concern here.

[jhawthorn]

@richardnuno thanks for the sample data, but I don't think your performance test was really measuring the impact this would have on production. The sample data did not include any images and caching was turned off.

I ran the sandbox against both master and this branch with the server in a production-like environment: config.cache_classes = true and config.action_controller.perform_caching = true. I added one image for each colour on this PR's branch.

Testing both branches had approximately the same runtime 👍 about 3.5 seconds (slowish, but unchanged). However, this is our best case scenario with this change, since we're hitting the product endpoint, it only needs one query for each of the variant_image_rules, variant_image_rule_conditions, variant_image_rule_value, and image.

What needs to still be tested is the variant index or variant search route with multiple products, which is where I think the worst impact from this change will be.

[richardnuno]

@jhawthorn I pushed up a branch that includes the same type of sample data setup script included in the latest commit of this PR but for master. It creates 10 products with about 4000 variants each. Each variant has 6 images associated with it, either by using the direct association present in master or using the variant image rules in this PR. Let me know if you think they're not equivalent in any way.

I tried hitting the api endpoints below with the application settings you mentioned config.cache_classes = true and config.action_controller.perform_caching = true. I got the following response times for each branch (master is actually variant-image-perf-compare but I thought master would make it easier to compare) on my local machine.

Cold cache

	master	variant-image-rules
/api/variants?q[product_name_or_sku_cont]=performance&in_stock_only=true	3916ms	3482ms
/api/variants (variants from current sample products)	4432ms	3462ms
/api/variants?page=1945 (variants from performance sample products)	3871ms	3005ms

Warm cache

	master	variant-image-rules
/api/variants?q[product_name_or_sku_cont]=performance&in_stock_only=true	531ms	350ms
/api/variants (variants from current sample products)	569ms	181ms
/api/variants?page=1945 (variants from performance sample products)	436ms	205ms

[mtomov]

Hello,

I think using rules for images is overly complicated. Not only from code perspective, but also from UI and administration point of view.

What you are effectively trying to do is to assign a single image to multiple, but not all variants. I think this can be accomplished by changing the existing one-to-many relationship between variants and images to a many-to-many one.

From a data perspective, that would require one more table in the classical implementation of many-to-many. In the case of Postgres, it can be elegantly solved using an array column image_ids on the variant table.

The trickier part of this approach is to create a nice, maybe drag & drop UI, which would allow users to just pick from the pool of pictures available on all variants, and assign them to any variant.

[richardnuno]

Hi @mtomov

I agree that at first sight this seems overly complicated but the reason behind this approach is performance. The challenges/benefits explained here also apply to variant images.

Perhaps the comment on variant_property_rule.rb should be moved to concerns/spree/variant_rule.rb to make the reasoning behind this approach clearer?

[BenMorganIO]

Since the position is in the default scope, maybe index this as well?

[tvdeyen]

The current state of image handling in Spree is just bad. Very tedious. Way too many clicks involved and purely unusable for large stores.

In Alchemy we have: - JS multiple simultaneously Image uploader with drag n drop interface - a master image library where images can be assigned from to as many elements you want - a built in image cropping tool

Maybe I can port some of these features over into Solidus.

I think an global image library where images can be assigned to as many variants you want would be a great addition for any store.

The current state is just not practical anymore.

Best Thomas

Am 20.11.2015 um 16:45 schrieb Martin Tomov notifications@github.com:

Hello,

I think using rules for images is overly complicated. Not only from code perspective, but also from UI and administration point of view.

What you are effectively trying to do is to assign a single image to multiple, but not all variants. I think this can be accomplished by changing the existing one-to-many relationship between variants and images to a many-to-many one.

From a data perspective, that would require one more table in the classical implementation of many-to-many. In the case of Postgres, it can be elegantly solved using an array column image_ids on the variant table.

The trickier part of this approach is to create a nice, maybe drag & drop UI, which would allow users to just pick from the pool of pictures available on all variants, and assign them to any variant.

—
Reply to this email directly or view it on GitHub.

[jhawthorn]

This is looking good.

I was still hoping to see variant_image_rule_conditions and values combined with the variant_property_rule_ counterparts. Is that still in the works?

[richardnuno]

I agree with @jordan-brough's comment and would prefer to delay combining the two until it's necessary.

[richardnuno]

After thinking on this for a while, this is probably not a good addition to core. Going to do this as a customization on our store for now.

[mtomov]

Thank you for your effort on that though. It is indeed significant!