From 686fe8c028fc4e5ed7d811f7d971d26f5393044a Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 10 Jul 2024 12:05:47 +1000 Subject: [PATCH] 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 --- .../order_cycles/distributed_products_service.rb | 14 +++++++++----- app/services/products_renderer.rb | 12 +++++++----- .../distributed_products_service_spec.rb | 3 +++ spec/services/products_renderer_spec.rb | 1 - 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/app/services/order_cycles/distributed_products_service.rb b/app/services/order_cycles/distributed_products_service.rb index 74658971f3..e47ec7a4e3 100644 --- a/app/services/order_cycles/distributed_products_service.rb +++ b/app/services/order_cycles/distributed_products_service.rb @@ -11,10 +11,14 @@ module OrderCycles @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 - query = supplier_property_join(query) if supplier_properties.present? + def products_relation_incl_supplier_properties + query = relation_by_sorting(supplier_properties: true) + + query = supplier_property_join(query) query.order(Arel.sql(order)) end @@ -30,10 +34,10 @@ module OrderCycles 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. # diff --git a/app/services/products_renderer.rb b/app/services/products_renderer.rb index bade34a4c2..339d16c925 100644 --- a/app/services/products_renderer.rb +++ b/app/services/products_renderer.rb @@ -35,7 +35,11 @@ class ProductsRenderer 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 @@ class ProductsRenderer 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 @@ class ProductsRenderer 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 diff --git a/spec/services/order_cycles/distributed_products_service_spec.rb b/spec/services/order_cycles/distributed_products_service_spec.rb index c352214a9a..0ba85468a2 100644 --- a/spec/services/order_cycles/distributed_products_service_spec.rb +++ b/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 diff --git a/spec/services/products_renderer_spec.rb b/spec/services/products_renderer_spec.rb index dfec59bce7..170e7b4a2c 100644 --- a/spec/services/products_renderer_spec.rb +++ b/spec/services/products_renderer_spec.rb @@ -83,7 +83,6 @@ RSpec.describe ProductsRenderer do 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 })