-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Categories page needs a check #133
Comments
Not sure it's related but the clothing category is empty in the demo. http://demo.solidus.io/t/categories/clothing |
Some comments after investigating from my side. I'm new to solidus, but something that stroke me is that http://localhost:3000/t/categories is a link to a taxon page in opposition to a taxonomy page. My understanding is that taxonomies are higher-level hierarchies, and I had expected that such a top position in the sitemap would have corresponded to one of them: In fact, we have both taxonomy and taxon with the name irb(main):144:0> Taxonomy.find_by_name('Categories')
Spree::Taxonomy Load (0.2ms) SELECT "spree_taxonomies".* FROM "spree_taxonomies" WHERE "spree_taxonomies"."name" = ? ORDER BY "spree_taxonomies"."position" ASC LIMIT ? [["name", "Categories"], ["LIMIT", 1]]
=> #<Spree::Taxonomy id: 1, name: "Categories", created_at: "2021-03-02 04:44:17.869375000 +0000", updated_at: "2021-03-02 04:45:00.185461000 +0000", position: 1>
irb(main):145:0> Taxon.find_by_name('Categories')
Spree::Taxon Load (0.1ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."name" = ? LIMIT ? [["name", "Categories"], ["LIMIT", 1]]
=> #<Spree::Taxon id: 1, parent_id: nil, position: 0, name: "Categories", permalink: "categories", taxonomy_id: 1, lft: 1, rgt: 16, icon_file_name: nil, icon_content_type: nil, icon_file_size: nil, icon_updated_at: nil, description: nil, created_at: "2021-03-02 04:44:17.893612000 +0000", updated_at: "2021-03-02 04:45:00.184130000 +0000", meta_title: nil, meta_description: nil, meta_keywords: nil, depth: 0>
With the previous observation in mind, it's not unexpected to have duplicated products on that page. The top irb(main):152:0> Product.find_by_name('Solidus Long Sleeve').taxons.pluck(:name)
Spree::Product Load (0.4ms) SELECT "spree_products".* FROM "spree_products" WHERE "spree_products"."deleted_at" IS NULL AND "spree_products"."name" = ? LIMIT ? [["name", "Solidus Long Sleeve"], ["LIMIT", 1]]
(0.3ms) SELECT "spree_taxons"."name" FROM "spree_taxons" INNER JOIN "spree_products_taxons" ON "spree_taxons"."id" = "spree_products_taxons"."taxon_id" WHERE "spree_products_taxons"."product_id" = ? [["product_id", 2]]
=> ["Shirts", "Solidus"]
irb(main):153:0> Taxon.find_by_name('Shirts').parent.name
Spree::Taxon Load (0.2ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."name" = ? LIMIT ? [["name", "Shirts"], ["LIMIT", 1]]
Spree::Taxon Load (0.1ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."id" = ? LIMIT ? [["id", 3], ["LIMIT", 1]]
=> "Clothing"
irb(main):154:0> Taxon.find_by_name('Shirts').parent.parent.name
Spree::Taxon Load (0.2ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."name" = ? LIMIT ? [["name", "Shirts"], ["LIMIT", 1]]
Spree::Taxon Load (0.1ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."id" = ? LIMIT ? [["id", 3], ["LIMIT", 1]]
Spree::Taxon Load (0.1ms) SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."id" = ? LIMIT ? [["id", 1], ["LIMIT", 1]]
=> "Categories"
It seems they do appear now (however not with the best style):
It seems it does now: |
Having each taxonomy with a "root" taxon inside with the same name is a design choice: we want to have a root node in the tree that represents the main level of the category. You might argue why we need both taxons and taxonomies, it would be a good question. I think the answer is to conceptually split the ways to classify products, for example: Brands and Categories are probably hard to push in the same bucket of taxons conceptually. More info here: https://guides.solidus.io/developers/products-and-variants/taxonomies-and-taxons.html So, yes it's expected to have this kind of duplication. I think this is more a UX issue than a technical one, what we see reflects what the code is supposed to work. |
IMHO it's kind of confusing. I'd say that the very fundamental purpose of taxonomies is that of splitting up conceptually different taxons: A taxon belongs to a taxonomy. It's not such a big deal, because, in the end, we're just talking about a design decision made in the sample application. But it's true that it could be interpreted as a recommended practice. |
I agree and I think part of the issue is due to trying to make this fit into the gem we use for this. |
@kennyadsl I now understand what you meant by a design choice. I see it's a design choice in solidus itself: The behavior is so embedded that this consistency is enforced automatically by a hook: You can create a new taxonomy in the UI: But when you edit it you're doing so in relation to this automatically created taxon: This can lead to inconsistencies. For instance, when you rename a taxonomy its root taxon gets also updated, but it doesn't happen if what you rename is the root taxon in the first place: I think that this behavior is brittle and breaks with the principle of least surprise. On top of that, it's something that is not documented and, unless I'm missing something, makes the taxonomy model completely useless. I'm not sure about which options are available for us because a fix for it would probably have backward incompatibility issues. In a free of any constraints scenario, probably the best solution would be to deprecate the taxonomy model. |
Yes, I think that deprecating taxonomies is a good idea for keeping backward compatibility, and as soon as we provide a valid alternative and a clear migration path, that's totally doable. I think we may also have a terminology problem removing taxonomies. For example, when you need a new classification in your store (for example "brands"), you have to technically create a taxon, but that's not a normal taxon, it is a "root taxon" because it doesn't have any taxon parent. I think (maybe) that's part of why the taxonomy model has been added. |
Yeah, the other option is to remove root taxons and maintain taxonomies. Probably it would be more correct semantically but it would be harder to integrate. But, anyway, the possibility of inconsistencies is inherent to |
This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions. |
These are our products (12) in sandbox app:
But in the categories page (http://localhost:3000/t/categories) we have
The text was updated successfully, but these errors were encountered: