Per review, remove flag argument from products_relation

products_relation is now split in two, products_relation and
products_relation_incl_supplier_properties.
It avoids using a flag argument which is not a a good practice see:
https://martinfowler.com/bliki/FlagArgument.html

app/services/order_cycles/distributed_products_service.rb

@@ -11,10 +11,14 @@ def initialize(distributor, order_cycle, customer)
       @customer = customer
     end
 
-    def products_relation(supplier_properties: nil)
-      query = relation_by_sorting(supplier_properties)
+    def products_relation
+      relation_by_sorting.order(Arel.sql(order))
+    end
+
+    def products_relation_incl_supplier_properties
+      query = relation_by_sorting(supplier_properties: true)
 
-      query = supplier_property_join(query) if supplier_properties.present?
+      query = supplier_property_join(query)
 
       query.order(Arel.sql(order))
     end
@@ -30,10 +34,10 @@ def variants_relation
 
     attr_reader :distributor, :order_cycle, :customer
 
-    def relation_by_sorting(supplier_properties)
+    def relation_by_sorting(supplier_properties: false)
       query = Spree::Product.where(id: stocked_products)
 
-      if sorting == "by_producer" || supplier_properties.present?
+      if sorting == "by_producer" || supplier_properties
         # Joins on the first product variant to allow us to filter product by supplier. This is so
         # enterprise can display product sorted by supplier in a custom order on their shopfront.
         #

app/services/products_renderer.rb

@@ -35,7 +35,11 @@ def products
     return unless order_cycle
 
     @products ||= begin
-      results = distributed_products.products_relation(supplier_properties: supplier_properties_arg)
+      results = if supplier_properties.present?
+                  distributed_products.products_relation_incl_supplier_properties
+                else
+                  distributed_products.products_relation
+                end
 
       results = filter(results)
       # Scope results with variant_overrides
@@ -52,8 +56,6 @@ def enterprise_fee_calculator
   end
 
   def filter(query)
-    supplier_properties = supplier_properties_arg
-
     ransack_results = query.ransack(args[:q]).result.to_a
 
     return ransack_results if supplier_properties.blank?
@@ -83,12 +85,12 @@ def filter(query)
     ransack_results
   end
 
-  def supplier_properties_arg
+  def supplier_properties
     args[:q]&.slice("with_variants_supplier_properties")
   end
 
   def supplier_property_ids
-    supplier_properties_arg["with_variants_supplier_properties"]
+    supplier_properties["with_variants_supplier_properties"]
   end
 
   def with_properties

spec/services/order_cycles/distributed_products_service_spec.rb

@@ -3,6 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe OrderCycles::DistributedProductsService do
+  # NOTE: product_relation_incl_supplier is tested via ProductsRenderer specs:
+  # spec/services/products_renderer_spec.rb
+
   describe "#products_relation" do
     subject(:products_relation) {
       described_class.new(distributor, order_cycle, customer).products_relation

spec/services/products_renderer_spec.rb

@@ -83,7 +83,6 @@
           expect(products).to eq([product_apples, product_cherries])
         end
 
-        # TODO this is a bit flaky due to banana bread having two supplier
         it "filters products with a product property or a producer property" do
           cakes_supplier.producer_properties.create!({ property_id: property_organic.id,
                                                        value: '1', position: 1 })