Admin side product search not functioning #1570

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

radar commented May 22, 2012

  1. Make a new order from the orders tab in the admin
  2. Attempt to search for a SKU
  3. Witness it not function
Member

radar commented May 22, 2012

I can verify this issue and I know the fix for it, however there's another problem after that. I get undefined method serializable_hash for ActiveRecord::Relation when it attempts to include the variants association on the JSON that it is returning. That's a little frustrating.

I don't know how to fix that particular error right now, but will look into it again later on.

Member

radar commented May 22, 2012

I've turned this issue into a pull request with the fix. If you're game, you can try out the commit in a local copy of Spree and see if you can fix the issue.

Contributor

mscottford commented May 22, 2012

I started trying to fix this issue as well before I figured out how to search to see if it was already reported. My research has led me to the conclusion that the serializable_hash error can be removed by changing

        collection.to_json(:include => {:variants => {:include => {:option_values => {:include => :option_type}, 
                                                      :images => {:only => [:id], :methods => :mini_url}}}, 
                                                      :images => {:only => [:id], :methods => :mini_url}, :master => {}})

to

        collection.to_json(:include => {:variants => {:include => {:option_values => {:include => :option_type}, 
                                                      :images => {:only => [:id], :methods => :mini_url}}}, 
                                                      :master => {}})

I was able to write a controller test that causes this error. If you are interested, I can send along a pull request.

I'll try out your patch locally and let you know if it resolves the problem for me.

Contributor

mscottford commented May 22, 2012

With your patch, I'm not getting the error, but I'm also not getting any results.

Contributor

mscottford commented May 22, 2012

This appears to have been introduced by 4b48e3f. Product#variant_images used to return an array of Image, but now it returns an ActiveRecord::Relation.

Adding .all to the end of the AREL expression fixes the undefined method serializable_hash for ActiveRecord::Relation issue, but it seems to result in a bunch of other test failures.

Any advice?

Contributor

mscottford commented May 22, 2012

This is starting to look like a Rails issue. Instances of ActiveRecord::Relation are not passing the is_a?(Enumberable) test on https://github.com/rails/rails/blob/3-2-stable/activemodel/lib/active_model/serialization.rb#L88. But ActiveRecord::Relation will correctly return a valid value for records.map { |a| a.serializable_hash(opts) }.

Contributor

ademidov commented May 23, 2012

@mscottford yes, it's possibly a Rails issue.
This is because there are no more association images for Products, while :include option takes only enumerables (association target array is enumerable) and not scopes, which are AR::Relation.

In this case I see two ways.

  1. :methods => :images instead of :include => :images
  2. Why images is a method and not an association?
    has_many :images, :through => :variants_including_master,
                      :order => "#{Asset.quoted_table_name}.position",
                      :readonly => true

It's much cleaner than find_by_sql, i feel.

Contributor

mscottford commented May 23, 2012

Thanks @alekseydem. Adding :methods => images instead of :include => :images does prevent the crash, and results do come back. However, each image in the json response is not in the format expected by the Javascript. It expects the image to look like

      "image":{
        "id":1004,
        "mini_url":"/path/to/image"
      }

but it is coming across as

      "image":{
        "alt":null,
        "attachment_content_type":"image/png",
        "attachment_file_name":"apache_baseball.png",
        "attachment_file_size":141361,
        "attachment_height":484,
        "attachment_updated_at":"2012-05-16T20:27:31Z",
        "attachment_width":504,
        "id":1004,
        "position":1,
        "viewable_id":157039898,
        "viewable_type":"Spree::Variant"
      }

It does not look like Object#to_json supports providing options to the :methods that get called.

Contributor

mscottford commented May 23, 2012

I've figured out how to solve this. I'll send a pull request in a few minutes.

Contributor

mscottford commented May 23, 2012

radar closed this in ac2c1ac May 24, 2012

@joahking joahking pushed a commit to joahking/spree that referenced this pull request May 25, 2012

@mscottford @radar mscottford + radar Adds a `serializable_hash` method to the result of `Product#variant_i…
…mages`.

Fixes #1570
20b3043

@tvdeyen tvdeyen pushed a commit to magiclabs/spree that referenced this pull request Jun 1, 2012

@mscottford @radar mscottford + radar Adds a `serializable_hash` method to the result of `Product#variant_i…
…mages`.

Fixes #1570
645bbc2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment