Fixes 422 error due to bad sql building

[cyrillefr]

What? Why?

• Closes Filtering product by supplier property breaks when enterprise has custom sorting by category set #12656

When some properties are defined at the enterprise level, there is a 422 return code as well as a "Products loading ..." displayed when customer filters products.

Reason is a bug in SQL Query building.
First part of query use supplier_properties parameter, but not second part, that can leads to mismatch between the 2 parts:

openfoodnetwork/app/services/order_cycles/distributed_products_service.rb

Lines 37 to 40 in dfea0cd

	def relation_by_sorting(supplier_properties: false)
	query = Spree::Product.where(id: stocked_products)
	if sorting == "by_producer" || supplier_properties

The 2nd bit:

openfoodnetwork/app/services/order_cycles/distributed_products_service.rb

Lines 81 to 83 in dfea0cd

	def order
	if distributor.preferred_shopfront_product_sorting_method == "by_producer" &&
	distributor.preferred_shopfront_producer_order.present?

What should we test?

See § "Steps to Reproduce" from #12656

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[cyrillefr]

Hello @rioug ,

I uploaded a bug here in the order method (the || is not good).

I have working on the order method locally, it looks a bit like that

elsif sorting_with_supplier_properties?(supplier_properties)
  order_by_property = distributor.producer_properties 
    .map { |pp| "producer_properties.property_id=#{pp.property_id} DESC" } 
    .join(", ")
     "#{order_by_property}, spree_products.name ASC, spree_products.id ASC"

def sorting_with_supplier_properties?(supplier_properties)
  supplier_properties && distributor.producer_properties.present?
end

I am not sure whether my use of distributor.producer_properties is good( I have seen it used in specs).
But I realise now that may be I should make another if in the relation_by_sorting method, to use
producer_properties.property_id in the select and in the group by.

What do you think ?

[rioug]

If I am following correctly what you are saying, I think you are trying to something we don't need. There is no need to order by producer_properties.

I think what we need to do here, it's to get rid of the supplier_properties flag (and the || ) in relation_by_sorting and update the the query when we are sorting "by_category", we need to include the first_variant.supplier_id in the select. Doing that allows us to use supplier_property_join when sorting "by_category" without blowing up. Does that make sense ? I have a feeling that might not work but it's a good place to start.

Good job on cleaning up order !

[rioug]

As a first step you should try to add the missing spec in here :

openfoodnetwork/spec/services/products_renderer_spec.rb

Line 139 in dfea0cd

it "filters products with property when sorting is enabled" do

We need the scenario where preferred_shopfront_taxon_order is set on the distributor and we are filtering by producer properties.

[cyrillefr]

Hello @rioug ,

Thanks for the tip.
It seems I handled this issue from the wrong end with this supplier_properties parameter ....
So, I removed it from the relation_by_sorting.
I added supplier_id in the 2nd part of the method, and it works.
I also added one spec.

Checks have failed, but I cannot reproduce one and the 2nd fail is a redis issue ...
Could you please rerun the checks ? It should be ok

[rioug]

Awesome, thanks for fixing this @cyrillefr 🙏 ! I had a feeling it wasn't going to be this simple but I am glad to be proven wrong 😄

[filipefurtad0]

Thanks so much @cyrillefr ,

I could verify the bug is fixed indeed in staging. Before this PR, the bug is reproducible:

After this PR the bug is gone, and the correct results are returned:

Merging!
Thank you.