solidus icon indicating copy to clipboard operation
solidus copied to clipboard

Product ordering on taxon show page

Open ollieh-m opened this issue 7 years ago • 10 comments

(I'm not using the issue template because I'm not actually sure of the correct/expected behaviour here...)

If a product (Product A) is attached to two taxons (Taxon A and Taxon B), which are related to each other as parent and child, that product could have two different positions, one for its association with Taxon A and one for its association with Taxon B. This causes a problem when trying to paginate a list of products that belong to Taxon A and Taxon A's children, because the list of distinct, sorted products to be paginated duplicates Product A - because Product A has two distinct position values. The first X number of products from the list actually has 1 fewer distinct products than expected.

It's also not clear how products should be ordered on a taxon page that includes products related to the parent and children taxons. One product could be positioned second on the parent taxon. Another product could be positioned first on the child taxon. Should the latter product come top? What if they are both positioned first in their respective taxons? And, related to the point above, what if a product is positioned first on the parent taxon and the same product is positioned second on the child taxon - which position should take precedence?

I'm aware I may be misunderstanding how taxons are designed to be used, in which case I'd really appreciate some clarity. If a product should only be related to one taxon per taxon tree, there should probably be a validation constraint to enforce this. But also, if that is the case, I'm still unsure how products should be ordered on a taxon page, and in particular how the admin could control the order on a given taxon page.

ollieh-m avatar Feb 02 '18 11:02 ollieh-m

I found this bug too.

This behavior is caused by this code (https://github.com/solidusio/solidus/blob/master/core/app/models/spree/product/scopes.rb#L65)

add_search_scope :in_taxon do |taxon|
      includes(:classifications).
      where("spree_products_taxons.taxon_id" => taxon.self_and_descendants.pluck(:id)).
      order(Spree::Classification.arel_table[:position].asc)
end

And there is not a single chance that it will work correctly for products attached to several taxons that coincide with the condition where("spree_products_taxons.taxon_id" => taxon.self_and_descendants.pluck(:id))

It looks like this bug was born when positions was added to classifications (products_taxons).

For quick fix you can override this scope.

add_search_scope :in_taxon do |taxon|
      includes(:classifications).
      where("spree_products_taxons.taxon_id" => taxon.id).
      order(Spree::Classification.arel_table[:position].asc)
end

bitberry-dev avatar Nov 17 '18 22:11 bitberry-dev

I see this issue is old, I'm having a related issue with pagination.

Pages are calculated incorrectly as the scope is applying a distinct sql clause on spree_products_taxons.position which doesn't consider the possibility of having 2 products with the same position on 2 different taxonomies.

I'm currently patching this line https://github.com/solidusio/solidus/blob/3724171102e737afa6afed5f8eb9ed58c0063e7b/core/app/models/spree/product/scopes.rb#L73

replacing it with:

              .select(Spree::Classification.arel_table[:product_id])

With this small tweak the distinct will be applied on product IDs.

And BTW, I'm currently overriding on my App the position ordering and using alphabetical, I'm not exactly sure if this breaks something else at this point.

manuca avatar Jun 08 '20 22:06 manuca

This is an interesting one. I'd expect the the product to show multiple times if it was included multiple times, and that the pagination would correctly handle that situation.

jarednorman avatar Jun 08 '20 22:06 jarednorman

Maybe I'm not following but why would one want a product to appear multiple times when listing products inside a given taxonomy?

manuca avatar Jun 08 '20 22:06 manuca

Honestly, upon review, I'm not sure what the desired behaviour is here. The initial issue outlines possibilities:

It's also not clear how products should be ordered on a taxon page that includes products related to the parent and children taxons. One product could be positioned second on the parent taxon. Another product could be positioned first on the child taxon. Should the latter product come top? What if they are both positioned first in their respective taxons? And, related to the point above, what if a product is positioned first on the parent taxon and the same product is positioned second on the child taxon - which position should take precedence?

jarednorman avatar Jun 10 '20 23:06 jarednorman

Yeah me neither, the sort order could be defined in different and arbitrary ways.

I think that order even if arbitrary is not that big of a deal. The big problem is that there are products that don't show up at all when they have the same position in a parent/child taxon hierarchy.

I did some monkey patching on Spree::Product to fix this issue locally. I can't say I'm super comfortable with the solution but it gets the job done. Here it is in case you want to take a look at it.

module ProductDecorator
  def self.prepended(base)
    base.class_eval do
      # Hack to get around multiple products with same `position` in parent/child
      # taxon relationship.
      #
      # See:
      # https://github.com/solidusio/solidus/issues/2551
      return unless ::Spree::Product.search_scopes.include?(:in_taxon)

      singleton_class.send(:define_method, :in_taxon) do |taxon|
        includes(:classifications)
          .where("spree_products_taxons.taxon_id" => taxon.self_and_descendants.pluck(:id))
          .select(Spree::Classification.arel_table[:product_id])
          .order(Spree::Classification.arel_table[:position].asc)
      end
    end
  end

  ::Spree::Product.prepend self
end

manuca avatar Jun 11 '20 01:06 manuca

What's the use case for having a product in a taxon as well as one of its children?

jarednorman avatar Jun 11 '20 02:06 jarednorman

I believe when you have a category/sub-category organization for products you'd like a product to appear both in the general category as well as in the more specific one. At least that is the use case I'm facing.

manuca avatar Jun 11 '20 02:06 manuca

As I understand it, this issue relates to the situation when you're displaying the products of a taxon and its children. Perhaps I'm missing something.

jarednorman avatar Jun 11 '20 03:06 jarednorman

I was also struggling with the same issue until i found this thread. in the latest master, i found this:

https://github.com/solidusio/solidus/blob/35d6d71cc660e9fd375d4d326266f031e2ffb0e1/core/app/models/spree/product/scopes.rb#L76-L83

If you don't want to monkey-patch, you can also adjust your base scope and change:

base_scope = base_scope.in_taxon(@properties[:taxon]) if @properties[:taxon].present? to

if @properties[:taxon].present?
  base_scope = base_scope.in_taxons(@properties[:taxon])
  base_scope = base_scope.select('spree_products.*, spree_products_taxons.position').distinct
                         .order('spree_products_taxons.position')
end

It seems to fix my problem, because in_taxons does the magic that is needed to filter out duplicates, but be aware that it does not order by position as the in_taxon did. therefore i added the ordering again, which seems not yet like the perfect solution.

But i think somewhere in in_taxons we could find the solution!

sihu avatar May 05 '22 16:05 sihu