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 diff --git a/app/services/order_cycles/distributed_products_service.rb b/app/services/order_cycles/distributed_products_service.rb index 5f349dae37..e47ec7a4e3 100644 --- a/app/services/order_cycles/distributed_products_service.rb +++ b/app/services/order_cycles/distributed_products_service.rb @@ -4,50 +4,23 @@ # 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") + def products_relation + relation_by_sorting.order(Arel.sql(order)) end - # 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 + def products_relation_incl_supplier_properties + query = relation_by_sorting(supplier_properties: true) - 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 = supplier_property_join(query) + + query.order(Arel.sql(order)) end def variants_relation @@ -61,6 +34,72 @@ module OrderCycles attr_reader :distributor, :order_cycle, :customer + def relation_by_sorting(supplier_properties: false) + query = Spree::Product.where(id: stocked_products) + + 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. + # + # 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/app/services/products_renderer.rb b/app/services/products_renderer.rb index 9db07fa975..339d16c925 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,11 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength return unless order_cycle @products ||= begin - results = products_relation. - order(Arel.sql(products_order)) + 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,38 +55,19 @@ 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) 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 +85,18 @@ class ProductsRenderer # rubocop:disable Metrics/ClassLength ransack_results end + def supplier_properties + args[:q]&.slice("with_variants_supplier_properties") + end + + def supplier_property_ids + supplier_properties["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 +111,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/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. diff --git a/spec/services/order_cycles/distributed_products_service_spec.rb b/spec/services/order_cycles/distributed_products_service_spec.rb index 259955d41a..0ba85468a2 100644 --- a/spec/services/order_cycles/distributed_products_service_spec.rb +++ b/spec/services/order_cycles/distributed_products_service_spec.rb @@ -3,7 +3,14 @@ require 'spec_helper' RSpec.describe OrderCycles::DistributedProductsService do - describe "#products_supplier_relation" 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 + } + let(:distributor) { create(:distributor_enterprise) } let(:product) { create(:product) } let(:variant) { product.variants.first } @@ -16,16 +23,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 +42,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 +53,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 +76,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 diff --git a/spec/services/products_renderer_spec.rb b/spec/services/products_renderer_spec.rb index 4044b354e6..170e7b4a2c 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 @@ -111,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 })