From 17fe492bf47e92323e9fd5524af0da4b508b8fe5 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 26 Feb 2024 20:53:27 +1100 Subject: [PATCH 1/6] Fix typo --- spec/factories/variant_factory.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/variant_factory.rb b/spec/factories/variant_factory.rb index dc729a29e6..245a6b78ac 100644 --- a/spec/factories/variant_factory.rb +++ b/spec/factories/variant_factory.rb @@ -14,7 +14,7 @@ FactoryBot.define do primary_taxon { Spree::Taxon.first || FactoryBot.create(:taxon) } supplier { Enterprise.is_primary_producer.first || FactoryBot.create(:supplier_enterprise) } - # createing a product here will end up creating an extra variant, as creating product will + # creating a product here will end up creating an extra variant, as creating product will # create a "standard variant" by default. We could try to pass the variant instance we # are creating but it fails because then the variant instance gets saved and it fails because # the product isn't associated yet. It's a chicken and egg problem. From 79a22aefc31688489763d7bcdc79de685ff49eff Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Jun 2024 15:52:18 +1000 Subject: [PATCH 2/6] Refactor #products_relation Due to primary_taxon and supplier having moved to the variant, filtering by_producer and by_category with a custom order involves some complicated sql queries. So we moved the sorting logic from ProductsRenderer to OrderCycles::DistributedProductsService so we can keep the complicated SQL logic contained in one place --- .../distributed_products_service.rb | 107 ++++++++++++------ .../distributed_products_service_spec.rb | 103 +++++++++++++---- 2 files changed, 149 insertions(+), 61 deletions(-) diff --git a/app/services/order_cycles/distributed_products_service.rb b/app/services/order_cycles/distributed_products_service.rb index 5f349dae37..33756748a2 100644 --- a/app/services/order_cycles/distributed_products_service.rb +++ b/app/services/order_cycles/distributed_products_service.rb @@ -4,50 +4,19 @@ # The stock-checking includes on_demand and stock level overrides from variant_overrides. module OrderCycles - class DistributedProductsService + class DistributedProductsService # rubocop:disable Metrics/ClassLength def initialize(distributor, order_cycle, customer) @distributor = distributor @order_cycle = order_cycle @customer = customer end - # TODO refactor products_taxons_relation and products_supplier_relation - # Joins on the first product variant to allow us to filter product by taxon. # This is so - # enterprise can display product sorted by category in a custom order on their shopfront. - # - # Caveat, the category sorting won't work properly if there are multiple variant with different - # category for a given product. - # - def products_taxons_relation - Spree::Product.where(id: stocked_products). - joins("LEFT JOIN ( - SELECT DISTINCT ON(product_id) id, product_id, primary_taxon_id - FROM spree_variants WHERE deleted_at IS NULL - ) first_variant ON spree_products.id = first_variant.product_id"). - select("spree_products.*, first_variant.primary_taxon_id"). - group("spree_products.id, first_variant.primary_taxon_id") - end + def products_relation(supplier_properties: nil) + @query = relation_by_sorting(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. - # - # Caveat, the supplier sorting won't work properly if there are multiple variant with different - # supplier for a given product. - # - def products_supplier_relation - Spree::Product.where(id: stocked_products). - joins("LEFT JOIN (SELECT DISTINCT ON(product_id) id, product_id, supplier_id - FROM spree_variants WHERE deleted_at IS NULL) first_variant - ON spree_products.id = first_variant.product_id"). - select("spree_products.*, first_variant.supplier_id"). - group("spree_products.id, first_variant.supplier_id") - end + supplier_property_join if supplier_properties.present? - def supplier_property_join(query) - query.joins(" - JOIN enterprises ON enterprises.id = first_variant.supplier_id - LEFT OUTER JOIN producer_properties ON producer_properties.producer_id = enterprises.id - ") + @query.order(Arel.sql(order)) end def variants_relation @@ -61,6 +30,72 @@ module OrderCycles attr_reader :distributor, :order_cycle, :customer + def relation_by_sorting(supplier_properties) + query = Spree::Product.where(id: stocked_products) + + if sorting == "by_producer" || supplier_properties.present? + # 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. + # + # Caveat, the supplier sorting won't work properly if there are multiple variant with + # different supplier for a given product. + query. + joins("LEFT JOIN (SELECT DISTINCT ON(product_id) id, product_id, supplier_id + FROM spree_variants WHERE deleted_at IS NULL) first_variant + ON spree_products.id = first_variant.product_id"). + select("spree_products.*, first_variant.supplier_id"). + group("spree_products.id, first_variant.supplier_id") + elsif sorting == "by_category" + # Joins on the first product variant to allow us to filter product by taxon. # This is so + # enterprise can display product sorted by category in a custom order on their shopfront. + # + # Caveat, the category sorting won't work properly if there are multiple variant with + # different category for a given product. + query. + joins("LEFT JOIN ( + SELECT DISTINCT ON(product_id) id, product_id, primary_taxon_id + FROM spree_variants WHERE deleted_at IS NULL + ) first_variant ON spree_products.id = first_variant.product_id"). + select("spree_products.*, first_variant.primary_taxon_id"). + group("spree_products.id, first_variant.primary_taxon_id") + else + query.group("spree_products.id") + end + end + + def sorting + distributor.preferred_shopfront_product_sorting_method + end + + def supplier_property_join + @query = @query.joins(" + JOIN enterprises ON enterprises.id = first_variant.supplier_id + LEFT OUTER JOIN producer_properties ON producer_properties.producer_id = enterprises.id + ") + end + + def order + if distributor.preferred_shopfront_product_sorting_method == "by_producer" && + distributor.preferred_shopfront_producer_order.present? + order_by_producer = distributor + .preferred_shopfront_producer_order + .split(",").map { |id| "first_variant.supplier_id=#{id} DESC" } + .join(", ") + + "#{order_by_producer}, spree_products.name ASC, spree_products.id ASC" + elsif distributor.preferred_shopfront_product_sorting_method == "by_category" && + distributor.preferred_shopfront_taxon_order.present? + order_by_category = distributor + .preferred_shopfront_taxon_order + .split(",").map { |id| "first_variant.primary_taxon_id=#{id} DESC" } + .join(", ") + + "#{order_by_category}, spree_products.name ASC, spree_products.id ASC" + else + "spree_products.name ASC, spree_products.id" + end + end + def stocked_products order_cycle. variants_distributed_by(distributor). diff --git a/spec/services/order_cycles/distributed_products_service_spec.rb b/spec/services/order_cycles/distributed_products_service_spec.rb index 259955d41a..c352214a9a 100644 --- a/spec/services/order_cycles/distributed_products_service_spec.rb +++ b/spec/services/order_cycles/distributed_products_service_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' RSpec.describe OrderCycles::DistributedProductsService do - describe "#products_supplier_relation" do + describe "#products_relation" do + subject(:products_relation) { + described_class.new(distributor, order_cycle, customer).products_relation + } + let(:distributor) { create(:distributor_enterprise) } let(:product) { create(:product) } let(:variant) { product.variants.first } @@ -16,16 +20,12 @@ RSpec.describe OrderCycles::DistributedProductsService do supplier = create(:supplier_enterprise) variant.update(supplier: ) create(:variant, product:, supplier: ) - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).to eq([product]) + expect(products_relation).to eq([product]) end describe "product distributed by distributor in the OC" do it "returns products" do - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).to eq([product]) + expect(products_relation).to eq([product]) end end @@ -39,9 +39,7 @@ RSpec.describe OrderCycles::DistributedProductsService do end it "does not return product" do - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).not_to include product + expect(products_relation).not_to include product end end @@ -52,25 +50,20 @@ RSpec.describe OrderCycles::DistributedProductsService do end it "does not return product" do - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).not_to include product + expect(products_relation).not_to include product end end describe "filtering products that are out of stock" do context "with regular variants" do it "returns product when variant is in stock" do - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).to include product + expect(products_relation).to include product end it "does not return product when variant is out of stock" do variant.update_attribute(:on_hand, 0) - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).not_to include product + + expect(products_relation).not_to include product end end @@ -80,20 +73,80 @@ RSpec.describe OrderCycles::DistributedProductsService do } it "does not return product when an override is out of stock" do - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).not_to include product + expect(products_relation).not_to include product end it "returns product when an override is in stock" do variant.update_attribute(:on_hand, 0) override.update_attribute(:count_on_hand, 10) - expect( - described_class.new(distributor, order_cycle, customer).products_supplier_relation - ).to include product + + expect(products_relation).to include product end end end + + describe "sorting" do + let(:order_cycle) do + create(:simple_order_cycle, distributors: [distributor]) + end + let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } + + let(:fruits) { create(:taxon) } + let(:cakes) { create(:taxon) } + let(:fruits_supplier) { create(:supplier_enterprise) } + let(:cakes_supplier) { create(:supplier_enterprise) } + let!(:product_apples) { + create(:product, name: "apples", primary_taxon_id: fruits.id, + supplier_id: fruits_supplier.id, inherits_properties: true) + } + let!(:product_banana_bread) { + create(:product, name: "banana bread", primary_taxon_id: cakes.id, + supplier_id: cakes_supplier.id, inherits_properties: true) + } + let!(:product_cherries) { + create(:product, name: "cherries", primary_taxon_id: fruits.id, + supplier_id: fruits_supplier.id, inherits_properties: true) + } + let!(:product_doughnuts) { + create(:product, name: "doughnuts", primary_taxon_id: cakes.id, + supplier_id: cakes_supplier.id, inherits_properties: true) + } + + before do + exchange.variants << product_apples.variants.first + exchange.variants << product_banana_bread.variants.first + exchange.variants << product_cherries.variants.first + exchange.variants << product_doughnuts.variants.first + end + + it "sorts products by the distributor's preferred taxon list" do + allow(distributor) + .to receive(:preferred_shopfront_product_sorting_method) { "by_category" } + allow(distributor) + .to receive(:preferred_shopfront_taxon_order) { "#{cakes.id},#{fruits.id}" } + + expect(products_relation) + .to eq([product_banana_bread, product_doughnuts, product_apples, product_cherries]) + end + + it "sorts products by the distributor's preferred producer list" do + allow(distributor) + .to receive(:preferred_shopfront_product_sorting_method) { "by_producer" } + allow(distributor).to receive(:preferred_shopfront_producer_order) { + "#{cakes_supplier.id},#{fruits_supplier.id}" + } + + expect(products_relation) + .to eq([product_banana_bread, product_doughnuts, product_apples, product_cherries]) + end + + it "alphabetizes products by name when taxon list is not set" do + allow(distributor).to receive(:preferred_shopfront_taxon_order) { "" } + + expect(products_relation) + .to eq([product_apples, product_banana_bread, product_cherries, product_doughnuts]) + end + end end describe "#variants_relation" do From c372bf746ab67a9f3c454f33705a8f8f08f67601 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Jun 2024 16:10:00 +1000 Subject: [PATCH 3/6] Refactor ProductRenderer The sorting logic has been moved to OrderCycles::DistributedProductsService#product_relations Plus hopefully fix the spec flackiness --- app/services/products_renderer.rb | 63 +++++++------------------ spec/services/products_renderer_spec.rb | 28 ----------- 2 files changed, 18 insertions(+), 73 deletions(-) diff --git a/app/services/products_renderer.rb b/app/services/products_renderer.rb index 9db07fa975..bade34a4c2 100644 --- a/app/services/products_renderer.rb +++ b/app/services/products_renderer.rb @@ -3,7 +3,7 @@ require 'open_food_network/scope_product_to_hub' -class ProductsRenderer # rubocop:disable Metrics/ClassLength +class ProductsRenderer include Pagy::Backend class NoProducts < RuntimeError; end @@ -35,8 +35,7 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength return unless order_cycle @products ||= begin - results = products_relation. - order(Arel.sql(products_order)) + results = distributed_products.products_relation(supplier_properties: supplier_properties_arg) results = filter(results) # Scope results with variant_overrides @@ -52,38 +51,21 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength OpenFoodNetwork::EnterpriseFeeCalculator.new distributor, order_cycle end - # TODO refactor this, distributed_products should be able to give use the relation based - # on the sorting method, same for ordering. It would prevent the SQL implementation from - # leaking here - def products_relation - if distributor.preferred_shopfront_product_sorting_method == "by_category" && - distributor.preferred_shopfront_taxon_order.present? - return distributed_products.products_taxons_relation - end - - distributed_products.products_supplier_relation - end - - # TODO: refactor to address CyclomaticComplexity - def filter(query) # rubocop:disable Metrics/CyclomaticComplexity - supplier_properties = args[:q]&.slice("with_variants_supplier_properties") + def filter(query) + supplier_properties = supplier_properties_arg ransack_results = query.ransack(args[:q]).result.to_a return ransack_results if supplier_properties.blank? - with_properties = args[:q]&.dig("with_properties") supplier_properties_results = [] - if supplier_properties.present? # We can't search on an association's scope with ransack, a work around is to define # the a scope on the parent (Spree::Product) but because we are joining on "first_variant" # to get the supplier it doesn't work, so we do the filtering manually here # see: - # OrderCycleDistributedProducts#products_supplier_relation - # OrderCycleDistributedProducts#supplier_property_join - supplier_property_ids = supplier_properties["with_variants_supplier_properties"] - supplier_properties_results = distributed_products.supplier_property_join(query). + # OrderCycleDistributedProducts#products_relation + supplier_properties_results = query. where(producer_properties: { property_id: supplier_property_ids }). where(inherits_properties: true) end @@ -101,6 +83,18 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength ransack_results end + def supplier_properties_arg + args[:q]&.slice("with_variants_supplier_properties") + end + + def supplier_property_ids + supplier_properties_arg["with_variants_supplier_properties"] + end + + def with_properties + args[:q]&.dig("with_properties") + end + def paginate(results) _pagy, paginated_results = pagy_array( results, @@ -115,27 +109,6 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength OrderCycles::DistributedProductsService.new(distributor, order_cycle, customer) end - # TODO refactor, see above - def products_order - if distributor.preferred_shopfront_product_sorting_method == "by_producer" && - distributor.preferred_shopfront_producer_order.present? - order_by_producer = distributor - .preferred_shopfront_producer_order - .split(",").map { |id| "first_variant.supplier_id=#{id} DESC" } - .join(", ") - "#{order_by_producer}, spree_products.name ASC, spree_products.id ASC" - elsif distributor.preferred_shopfront_product_sorting_method == "by_category" && - distributor.preferred_shopfront_taxon_order.present? - order_by_category = distributor - .preferred_shopfront_taxon_order - .split(",").map { |id| "first_variant.primary_taxon_id=#{id} DESC" } - .join(", ") - "#{order_by_category}, spree_products.name ASC, spree_products.id ASC" - else - "spree_products.name ASC, spree_products.id" - end - end - def variants_for_shop @variants_for_shop ||= begin scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) diff --git a/spec/services/products_renderer_spec.rb b/spec/services/products_renderer_spec.rb index 4044b354e6..dfec59bce7 100644 --- a/spec/services/products_renderer_spec.rb +++ b/spec/services/products_renderer_spec.rb @@ -38,34 +38,6 @@ RSpec.describe ProductsRenderer do exchange.variants << product_doughnuts.variants.first end - describe "sorting" do - it "sorts products by the distributor's preferred taxon list" do - allow(distributor) - .to receive(:preferred_shopfront_taxon_order) { "#{cakes.id},#{fruits.id}" } - products = products_renderer.send(:products) - expect(products) - .to eq([product_banana_bread, product_doughnuts, product_apples, product_cherries]) - end - - it "sorts products by the distributor's preferred producer list" do - allow(distributor) - .to receive(:preferred_shopfront_product_sorting_method) { "by_producer" } - allow(distributor).to receive(:preferred_shopfront_producer_order) { - "#{cakes_supplier.id},#{fruits_supplier.id}" - } - products = products_renderer.send(:products) - expect(products) - .to eq([product_banana_bread, product_doughnuts, product_apples, product_cherries]) - end - - it "alphabetizes products by name when taxon list is not set" do - allow(distributor).to receive(:preferred_shopfront_taxon_order) { "" } - products = products_renderer.send(:products) - expect(products) - .to eq([product_apples, product_banana_bread, product_cherries, product_doughnuts]) - end - end - context "filtering" do it "filters products by name_or_meta_keywords_or_variants_display_as_or_" \ "variants_display_name_or_variants_supplier_name_cont" do From 5c136f8baae48a0fec209f46a469f4247cafab5f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 17 Jun 2024 16:16:57 +1000 Subject: [PATCH 4/6] Fix OrderCycleController to use products_relation --- app/controllers/api/v0/order_cycles_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/v0/order_cycles_controller.rb b/app/controllers/api/v0/order_cycles_controller.rb index ccf6a24dd6..0b864f55ea 100644 --- a/app/controllers/api/v0/order_cycles_controller.rb +++ b/app/controllers/api/v0/order_cycles_controller.rb @@ -96,7 +96,7 @@ module Api def distributed_products OrderCycles::DistributedProductsService.new( distributor, order_cycle, customer - ).products_supplier_relation.pluck(:id) + ).products_relation.pluck(:id) end end end From 5c3acbbcafccb63e4c6fe76d600cbd678c34f86c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 10 Jul 2024 11:45:39 +1000 Subject: [PATCH 5/6] Per review, remove instance variable @query --- .../order_cycles/distributed_products_service.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/services/order_cycles/distributed_products_service.rb b/app/services/order_cycles/distributed_products_service.rb index 33756748a2..74658971f3 100644 --- a/app/services/order_cycles/distributed_products_service.rb +++ b/app/services/order_cycles/distributed_products_service.rb @@ -12,11 +12,11 @@ module OrderCycles end def products_relation(supplier_properties: nil) - @query = relation_by_sorting(supplier_properties) + query = relation_by_sorting(supplier_properties) - supplier_property_join if supplier_properties.present? + query = supplier_property_join(query) if supplier_properties.present? - @query.order(Arel.sql(order)) + query.order(Arel.sql(order)) end def variants_relation @@ -67,8 +67,8 @@ module OrderCycles distributor.preferred_shopfront_product_sorting_method end - def supplier_property_join - @query = @query.joins(" + def supplier_property_join(query) + query.joins(" JOIN enterprises ON enterprises.id = first_variant.supplier_id LEFT OUTER JOIN producer_properties ON producer_properties.producer_id = enterprises.id ") From 686fe8c028fc4e5ed7d811f7d971d26f5393044a Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 10 Jul 2024 12:05:47 +1000 Subject: [PATCH 6/6] 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 })