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.

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

Product properties duplicating #647

Closed
connecticus opened this issue Jan 3, 2016 · 12 comments
Closed

Product properties duplicating #647

connecticus opened this issue Jan 3, 2016 · 12 comments

Comments

@connecticus
Copy link

We detected an issue by adding properties to a product. Solidus doesn't associate same properties in different products. So we've got a bunch of duplicated properties in /properties. Of course it makes filtering impossible.

@BenMorganIO
Copy link
Contributor

Hey, if you can provide steps to reproduce the issue, that would help move things along faster :)

@connecticus
Copy link
Author

@BenMorganIO, Thank you. It is solidus_globalize-related issue I think. Just try to add any options in different products and look at the /admin/properties All properties does not reuse existing property name and adding a new one.

@peterberkenbosch
Copy link
Contributor

This is by design @connecticus //cc @BenMorganIO properties are mapped 1:1 to products and just key:value pairs on the product. Option Types are the place to use those currently.

@dhonig
Copy link

dhonig commented Jan 4, 2016

Worth mentioning that Product Prototypes may help resolve the issue:
https://guides.spreecommerce.com/user/product_prototypes.html

@Senjai
Copy link
Contributor

Senjai commented Jan 4, 2016

@connecticus Product properties were not meant to be used for filtering purposes in the first place. They were meant only for frontend display where users would have a "properties" box or the like on the product page which would display additional information.

Searching on these properties is a common thing people want to do and I usually recommend that users add their own model associated with products to act as a profile of sorts with proper normalization. I find that product properties as implemented are not easily used for search purposes and would recommend against using them for that purpose.

@dhonig
Copy link

dhonig commented Jan 4, 2016

@Senjai both SOLR and ElasticSearch can deal with filtering on product properties with the existing models just fine for filtering and or faceting/aggregations. The trick is just configuring the index correctly and creating the right code for populating the index. I think you need a really good reason to create models outside of the spree norms, not sure if just replacing properties is a good enough one.
In publishing you have a set of standard metadata such as ISBN, etc that is usually a part of every product, for this case I think it makes sense to have another model to represent these attributes....But it is interesting to know that people are having problems filtering on properties with Ransack. To me this is something that might deserve a bit more attention.....

@connecticus
Copy link
Author

Thank you, guys, but I think nothing to discuss, it is globalize bug (w/o globalize Solidus is working as well as Spree till 3.X).
And Spree 3.X has had same issue - spree/spree#6556

@cbrunsdon
Copy link
Contributor

@connecticus thanks for checking in, if you end up filing a bug against solidus_globalize please link it here for future people.

killing this issue for now

@connecticus
Copy link
Author

Thank you very much @cbrunsdon

connecticus pushed a commit to connecticus/solidus that referenced this issue Jan 5, 2016
@connecticus
Copy link
Author

Solution:: There's an issue with find_by_id and globalize. It's documented here: globalize/globalize#423

It creates a strange query

SELECT "spree_properties".* FROM "spree_properties" INNER JOIN "spree_property_translations" ON "spree_property_translations"."spree_property_id" = "spree_properties"."id" 
WHERE "spree_property_translations"."name" = '--- !ruby/object:ActiveRecord::StatementCache::Substitute {}' 
AND "spree_property_translations"."locale" IN ('pt', 'en') LIMIT 1

We can just waived use find_by :name to workaround globalize/globalize#423 bug

#core/app/models/concerns/spree/ordered_property_value_list.rb

def property_name=(name)
  if name.present?
    property = Property.where(name: name).first ||
    Property.create(name: name, presentation: name)
    self.property = property
  end
end

@cbrunsdon
Copy link
Contributor

Fantastic, thanks @connecticus. I'm a big fan of the workaround for this. If we can get a fix out the door without doing any real work I say lets do it. If you don't shoot off a PR by tomorrow either @jhawthorn or I will do it, as it seems this is an issue many people will run into.

(Also: edited your comment for formatting)

@jakemumu
Copy link

jakemumu commented Oct 1, 2017

@Senjai sorry to bring an old thread back to life, but if users aren't meant to filter products by property than how are we meant to customize our products for filtering beyond things such as price & size?

Is it possible for the Solidus team to post an example to this issue somewhere? I've been banging my head against the wall trying to filter by multiple properties at once.

Essentially, I'm not sure what "add their own model associated with products to act as a profile of sorts with proper normalization." entails.

Thanks,

J

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants