Fix missing image in autocomplete variant

[rubenochiavone]

Description

If variant don't have images no image appears in autocomplete view (for example, trying to add a product variant to an order from admin). It should instead display master image as in the other parts of solidus.

To fix it, added display_images method in Spree::Variant to get image list to be used for variant. This method checks if variant.images is an empty array before returning it but also falls back to variant.product.master.images to avoid returning an empty array.

Since image from autocomplete variant views are created using Spree::Gallery::VariantGallery, changed variant.images call to variant.display_images in images method.

Edit

To fix it, improve Spree::Gallery::VariantGallery#images method to checks if @variant.images is empty before returning it but also falls back to @variant.product.master.images to avoid returning Spree::Image.none.

Closes #2951

Checklist:

• Pull Request guidelines are respected
• Changes are covered by tests (if possible)
• Each commit has a meaningful message attached that described WHAT changed, and WHY

Not sure if "Documentation/Readme have been updated accordingly" should be done for this one.

[aitbw]

Hey @rubenochiavone, thanks a lot for this PR! 🙌

I left you some comments about some improvements we can do for this patch. Care to take a lot at them and tell me what you think? 🤗

[aitbw]

Why are we creating a new variable in here? Can't we use images directly?

[rubenochiavone]

Yeah, my bad.

Although looking it again, I found out that current code and the one you proposed do a SELECT 1 + SELECT queries.

  Spree::Image Exists (0.3ms)  SELECT  1 AS one FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ? LIMIT ?  [["viewable_id", 1], ["viewable_type", "Spree::Variant"], ["LIMIT", 1]]
  Spree::Image Load (0.3ms)  SELECT  "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ? ORDER BY "spree_assets"."position" ASC LIMIT ?

Initially I proposed

imgs = images
return imgs if imgs.size > 0

Which trigger a count query then return the collection proxy.

   (0.1ms)  SELECT COUNT(*) FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ?  [["viewable_id", 1], ["viewable_type", "Spree::Variant"]]
  Spree::Image Load (0.2ms)  SELECT  "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ? ORDER BY "spree_assets"."position" ASC LIMIT ?

I had to refactor because @houndci-bot complained about using .size > 0.

So, the best of both would be:

return images if images.size > 0 # or return images if images.count > 0

WDYT?

[aitbw]

Perfect! 👍

[aitbw]

I can understand the reasoning to create a new variable in here but I think the name is too generic and can cause confusion with the images variable, something like master_image might be better. WDYT?

[rubenochiavone]

Agreed.

[kennyadsl]

Maybe this logic should go into the VariantGallery directly. Also, I think it can be simplified with something similar (didn't test):

def images
  @images ||=
    @variant.images.presence ||
    (!@variant.is_master? && @variant.product.master.images).presence ||
    Spree::Image.none
end

I think it's safe to assume that each product has a master variant without the need to check that. What do you think?

[rubenochiavone]

Maybe this logic should go into the VariantGallery directly.

Nice point, but since variant gallery class could be changed in config isn't it better to leave this implementation inside Variant class?

core/lib/spree/app_configuration.rb:417

class_name_attribute :variant_gallery_class, default: 'Spree::Gallery::VariantGallery'

Also, I think it can be simplified with something similar

def images
  @images ||=
    @variant.images.presence ||
    ([email protected]_master? && @variant.product.master.images).presence ||
    Spree::Image.none
end

.presence would eventually (after 2-3 methods) call .empty?.

rails/rails/activerecord/lib/active_record/associations/collection_proxy.rb:841

      # Returns +true+ if the collection is empty. If the collection has been
      # loaded it is equivalent
      # to <tt>collection.size.zero?</tt>. If the collection has not been loaded,
      # it is equivalent to <tt>!collection.exists?</tt>. If the collection has
      # not already been loaded and you are going to fetch the records anyway it
      # is better to check <tt>collection.length.zero?</tt>.
      # ...
      def empty?

.exists will always rely on database - see rails/rails/activerecord/lib/active_record/relation/finder_methods.rb:277. Maybe .length?

rails/rails/activerecord/lib/active_record/associations/collection_proxy.rb:814

      ##
      # :method: length
      #
      # :call-seq:
      #   length()
      #
      # Returns the size of the collection calling +size+ on the target.
      # If the collection has been already loaded, +length+ and +size+ are
      # equivalent. If not and you are going to need the records anyway this
      # method will take one less query. Otherwise +size+ is more efficient.

what about .size?

rails/rails/activerecord/lib/active_record/associations/collection_proxy.rb:786

      # Returns the size of the collection. If the collection hasn't been loaded,
      # it executes a <tt>SELECT COUNT(*)</tt> query. Else it calls <tt>collection.size</tt>.
      #
      # If the collection has been already loaded +size+ and +length+ are
      # equivalent. If not and you are going to need the records anyway
      # +length+ will take one less query. Otherwise +size+ is more efficient.
      # ...
      def size

So, when there are images return images if images.length.zero? is more effiecient, otherwise return images if images.size.zero? is more efficient. Not sure what is best 😬

Some people from SO recommends .size - https://stackoverflow.com/questions/12253277/size-length-and-count-in-rails.

I think it's safe to assume that each product has a master variant without the need to check that.

Yeah, but some tests fails. Maybe update those test cases?

$ cd core
$ bundle exec rspec

...

Failures:

  1) Spree::Gallery::VariantGallery behaves like a gallery #images 
     Failure/Error: master_images = product.master.images
     
     NoMethodError:
       undefined method `master' for nil:NilClass
     Shared Example Group: "a gallery" called from ./spec/models/spree/gallery/variant_gallery_spec.rb:20
     # ./app/models/spree/variant.rb:397:in `display_images'
     # ./app/models/spree/gallery/variant_gallery.rb:14:in `images'
     # ./lib/spree/testing_support/shared_examples/gallery.rb:5:in `block (3 levels) in <top (required)>'
     # ./lib/spree/testing_support/shared_examples/gallery.rb:7:in `block (3 levels) in <top (required)>'

WDYT?

[kennyadsl]

Thanks for the detailed response!

Nice point, but since variant gallery class could be changed in config isn't it better to leave this implementation inside Variant class?

Not sure which benefits would we have doing this, can you please explain better what's your concern? In my opinion this is a logic only related to which images we want to display for a variant and should stay in this VariantGallery class that has been added exactly to cointain this kind of logic.

I think .lenght and .presence are equivalent since they make the same query.

.presence
  Spree::Image Load (0.3ms)  SELECT "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ? ORDER BY "spree_assets"."position" ASC  [["viewable_id", 20], ["viewable_type", "Spree::Variant"]]

.length.zero?
  Spree::Image Load (0.3ms)  SELECT "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ? ORDER BY "spree_assets"."position" ASC  [["viewable_id", 20], ["viewable_type", "Spree::Variant"]]

.size.zero?
   (0.2ms)  SELECT COUNT(*) FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_id" = ? AND "spree_assets"."viewable_type" = ?  [["viewable_id", 20], ["viewable_type", "Spree::Variant"]]

You are right about .size being more efficient only if we don't need to also take records anyway, so only when there are no images for that variant. I think this is an implementation detail though and we should just rely on the method that fits more in terms of code readabilty/simplicity here.

Maybe update those test cases?

Yes, specs there use:

let(:variant) { Spree::Variant.new }

which is fast but I think it does not reflect an intact data representation.

Maybe we should use:

let(:variant) { build_stubbed(:variant) }

and probably, into another PR, we should also add a validation on the variant since right now we can't call .valid? on an instance of Spree::Variant that does not have the product set:

v = Spree::Variant.new
=> #<Spree::Variant id: nil, sku: "", weight: 0.0, height: nil, width: nil, depth: nil, deleted_at: nil, is_master: false, product_id: nil, cost_price: nil, position: nil, cost_currency: nil, track_inventory: true, tax_category_id: nil, updated_at: nil, created_at: nil>
irb(main):090:0> v.valid?
Traceback (most recent call last):
        1: from (irb):90
RuntimeError (No master variant found to infer price)

Hope this makes sense but feel free to let me know if you think it doesn't. 🙂

[rubenochiavone]

Thank you for the insights!

Not sure which benefits would we have doing this, can you please explain better what's your concern? In my opinion this is a logic only related to which images we want to display for a variant and should stay in this VariantGallery class that has been added exactly to cointain this kind of logic.

Agreed. Just to note some concerns

• Maybe a client code could benefit from this model method somewhere else without being required to use VariantGallery class
• Also a different variant gallery class could decide which method to use

You are right about .size being more efficient only if we don't need to also take records anyway, so only when there are no images for that variant. I think this is an implementation detail though and we should just rely on the method that fits more in terms of code readabilty/simplicity here.

👍

Yes, specs there use:

let(:variant) { Spree::Variant.new }

which is fast but I think it does not reflect an intact data representation.

Maybe we should use:

let(:variant) { build_stubbed(:variant) }

👍

and probably, into another PR, we should also add a validation on the variant since right now we can't call .valid? on an instance of Spree::Variant that does not have the product set:

v = Spree::Variant.new
=> #<Spree::Variant id: nil, sku: "", weight: 0.0, height: nil, width: nil, depth: nil, deleted_at: nil, > is_master: false, product_id: nil, cost_price: nil, position: nil, cost_currency: nil, track_inventory: true, tax_category_id: nil, updated_at: nil, created_at: nil>
irb(main):090:0> v.valid?
Traceback (most recent call last):
        1: from (irb):90
RuntimeError (No master variant found to infer price)

Should I create an issue?

[kennyadsl]

Maybe a client code could benefit from this model method somewhere else without being required to use VariantGallery class
Also a different variant gallery class could decide which method to use

Both are legit concerns IMO but it's very simple to add that method into a user's app via a Spree::Variant decorator or directly into a custom variant_gallery_class if needed and I'd prefer not to add another public interface (that we'll need to maintain) unless this is strictly needed.

Should I create an issue?

Done: #3036

[rubenochiavone]

👍

Done, please review 😄

[rubenochiavone]

Updated commit message and PR description to reflect reviewed changes

[kennyadsl]

Thanks, there are small specs improvements (tests descriptions stuff), but mainly I think that we are missing the spec for when there are no images on the product master as well. Can you please add it?

[kennyadsl]

Can we avoid using should in favor of fallbacks here? Also, there's an extra space after variant.product.master.images.

[rubenochiavone]

Done

[kennyadsl]

Technically it should be an ActiveRecord::Relation not an array 🤓. I'd avoid specifying what it is at all here, ... is empty is enough IMO.

[rubenochiavone]

Agreed. Done

[kennyadsl]

Ah, and maybe also spec for when the current variant is master?

[kennyadsl]

Why do we need to set all these associations here?

[rubenochiavone]

We didn't, copy+paste mistake... Fixed it 😄

[kennyadsl]

Thanks a lot!

[jacobherrington]

Looks good, thanks for the specs! 🍰