From a5f890cf3667b825b45de941274930d6086be412 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 17 Dec 2018 11:31:05 +0100 Subject: [PATCH 01/17] Fix styling on method --- app/serializers/api/cached_enterprise_serializer.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 4070cfa7d5..fb3ff64586 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -81,16 +81,14 @@ module Api end end + # This results in 3 queries per enterprise def distributed_properties - # This results in 3 queries per enterprise - if active - product_properties = Spree::Property.currently_sold_by(object) - producer_property_ids = ProducerProperty.currently_sold_by(object).pluck(:property_id) - + product_properties = Spree::Property.currently_sold_by(enterprise) + producer_property_ids = ProducerProperty.currently_sold_by(enterprise).pluck(:property_id) else - product_properties = Spree::Property.ever_sold_by(object) - producer_property_ids = ProducerProperty.ever_sold_by(object).pluck(:property_id) + product_properties = Spree::Property.ever_sold_by(enterprise) + producer_property_ids = ProducerProperty.ever_sold_by(enterprise).pluck(:property_id) end producer_properties = Spree::Property.where(id: producer_property_ids) From 64fb23029712b208da68452272a11e4e39105974 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 17 Dec 2018 11:30:06 +0100 Subject: [PATCH 02/17] Make it more obvious we talk about an enterprise --- .../api/cached_enterprise_serializer.rb | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index fb3ff64586..ca05dc6416 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -7,7 +7,7 @@ module Api cached def cache_key - object.andand.cache_key + enterprise.andand.cache_key end attributes :name, :id, :description, :latitude, :longitude, @@ -24,55 +24,55 @@ module Api has_many :distributed_properties, serializer: PropertySerializer def pickup - services = data.shipping_method_services[object.id] + services = data.shipping_method_services[enterprise.id] services ? services[:pickup] : false end def delivery - services = data.shipping_method_services[object.id] + services = data.shipping_method_services[enterprise.id] services ? services[:delivery] : false end def email_address - object.email_address.to_s.reverse + enterprise.email_address.to_s.reverse end def hash - object.to_param + enterprise.to_param end def logo - object.logo(:medium) if object.logo? + enterprise.logo(:medium) if enterprise.logo? end def promo_image - object.promo_image(:large) if object.promo_image? + enterprise.promo_image(:large) if enterprise.promo_image? end def path - enterprise_shop_path(object) + enterprise_shop_path(enterprise) end def producers - relatives = data.relatives[object.id] + relatives = data.relatives[enterprise.id] ids_to_objs(relatives.andand[:producers]) end def hubs - relatives = data.relatives[object.id] + relatives = data.relatives[enterprise.id] ids_to_objs(relatives.andand[:distributors]) end def taxons if active - ids_to_objs data.current_distributed_taxons[object.id] + ids_to_objs data.current_distributed_taxons[enterprise.id] else - ids_to_objs data.all_distributed_taxons[object.id] + ids_to_objs data.all_distributed_taxons[enterprise.id] end end def supplied_taxons - ids_to_objs data.supplied_taxons[object.id] + ids_to_objs data.supplied_taxons[enterprise.id] end def supplied_properties @@ -97,7 +97,7 @@ module Api end def active - data.active_distributors.andand.include? object + data.active_distributors.andand.include? enterprise end # Map svg icons. @@ -109,7 +109,7 @@ module Api producer_shop: "/assets/map_003-producer-shop.svg", producer: "/assets/map_001-producer-only.svg", } - icons[object.category] + icons[enterprise.category] end # Choose regular icon font for enterprises. @@ -121,7 +121,7 @@ module Api producer_shop: "ofn-i_059-producer", producer: "ofn-i_059-producer", } - icon_fonts[object.category] + icon_fonts[enterprise.category] end # Choose producer page icon font - yes, sadly its got to be different. @@ -135,7 +135,7 @@ module Api producer_shop: "ofn-i_059-producer", producer: "ofn-i_059-producer", } - icon_fonts[object.category] + icon_fonts[enterprise.category] end private From 0782907f4ddb09a94e94bed84796644f271f24d0 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 13:29:05 +0100 Subject: [PATCH 03/17] Setup a test order cycle with its exchanges --- .../api/cached_enterprise_serializer_spec.rb | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index bea4b79bca..624224e567 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -23,4 +23,37 @@ describe Api::CachedEnterpriseSerializer do expect(properties).to eq([property, different_property]) end end + + describe '#distributed_properties' do + let(:cached_enterprise_serializer) { described_class.new(shop, options) } + + let(:enterprise_injection_data) do + instance_double(OpenFoodNetwork::EnterpriseInjectionData, active_distributors: []) + end + let(:options) { { data: enterprise_injection_data } } + let(:shop) { create(:distributor_enterprise, name: 'Distributor') } + + let(:property) { create(:property, presentation: 'One') } + + before do + producer = create(:supplier_enterprise, name: 'Supplier') + product = create(:product, supplier: producer, properties: [property]) + + order_cycle = build(:simple_order_cycle, coordinator: shop) + + incoming_exchange = build(:exchange, sender: producer, receiver: shop, incoming: true) + outgoing_exchange = build(:exchange, sender: shop, receiver: shop, incoming: false) + outgoing_exchange.variants << product.variants.first + + order_cycle.exchanges << incoming_exchange + order_cycle.exchanges << outgoing_exchange + + order_cycle.save + end + + it 'does not duplicate properties' do + properties = cached_enterprise_serializer.distributed_properties + expect(properties).to eq([property]) + end + end end From 8fef69387ebc18f687053ebf8c52efa7689a7021 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 13:36:32 +0100 Subject: [PATCH 04/17] Simplify test setup using factory transient attrs --- .../api/cached_enterprise_serializer_spec.rb | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 624224e567..50c6fc115d 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -27,28 +27,26 @@ describe Api::CachedEnterpriseSerializer do describe '#distributed_properties' do let(:cached_enterprise_serializer) { described_class.new(shop, options) } + let(:shop) { create(:distributor_enterprise) } + + let(:options) { { data: enterprise_injection_data } } let(:enterprise_injection_data) do instance_double(OpenFoodNetwork::EnterpriseInjectionData, active_distributors: []) end - let(:options) { { data: enterprise_injection_data } } - let(:shop) { create(:distributor_enterprise, name: 'Distributor') } let(:property) { create(:property, presentation: 'One') } before do - producer = create(:supplier_enterprise, name: 'Supplier') + producer = create(:supplier_enterprise) product = create(:product, supplier: producer, properties: [property]) - order_cycle = build(:simple_order_cycle, coordinator: shop) - - incoming_exchange = build(:exchange, sender: producer, receiver: shop, incoming: true) - outgoing_exchange = build(:exchange, sender: shop, receiver: shop, incoming: false) - outgoing_exchange.variants << product.variants.first - - order_cycle.exchanges << incoming_exchange - order_cycle.exchanges << outgoing_exchange - - order_cycle.save + create( + :simple_order_cycle, + coordinator: shop, + suppliers: [producer], + distributors: [shop], + variants: [product.variants] + ) end it 'does not duplicate properties' do From 47a90e937b6c155f81a16222c4a556730a192e8b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 13:47:43 +0100 Subject: [PATCH 05/17] Have a duplicate product to test with --- spec/serializers/api/cached_enterprise_serializer_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 50c6fc115d..92b62ea919 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -35,10 +35,12 @@ describe Api::CachedEnterpriseSerializer do end let(:property) { create(:property, presentation: 'One') } + let(:duplicate_property) { create(:property, presentation: 'One') } before do - producer = create(:supplier_enterprise) - product = create(:product, supplier: producer, properties: [property]) + product = create(:product, properties: [property]) + producer = create(:supplier_enterprise, properties: [duplicate_property]) + producer.supplied_products << product create( :simple_order_cycle, From 328cda66f57c3c3c61f6cc896e49595e3f2dcde6 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 13:51:06 +0100 Subject: [PATCH 06/17] Stop using PropertyMerge in serializer --- app/serializers/api/cached_enterprise_serializer.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index ca05dc6416..06b3571c1f 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -93,7 +93,9 @@ module Api producer_properties = Spree::Property.where(id: producer_property_ids) - OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties + (product_properties + producer_properties).uniq do |property_object| + property_object.property.presentation + end end def active From 8eb35e491da37b368c8a48c662c1e455d9fd46d0 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 17:18:10 +0100 Subject: [PATCH 07/17] Don't fetch rows by ids of DISTINCT records There's no point on fetching the same records again by the resulting ids of the previous query (nasty way of removing duplicates), when that query has a DISTINCT. --- app/serializers/api/cached_enterprise_serializer.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 06b3571c1f..6198e28a6c 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -85,14 +85,12 @@ module Api def distributed_properties if active product_properties = Spree::Property.currently_sold_by(enterprise) - producer_property_ids = ProducerProperty.currently_sold_by(enterprise).pluck(:property_id) + producer_properties = ProducerProperty.currently_sold_by(enterprise) else product_properties = Spree::Property.ever_sold_by(enterprise) - producer_property_ids = ProducerProperty.ever_sold_by(enterprise).pluck(:property_id) + producer_properties = ProducerProperty.ever_sold_by(enterprise) end - producer_properties = Spree::Property.where(id: producer_property_ids) - (product_properties + producer_properties).uniq do |property_object| property_object.property.presentation end From 92e2ed78d1db8385d6bbb0b3446653ad7362f9c5 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 17:26:37 +0100 Subject: [PATCH 08/17] Test active distributor as well --- .../api/cached_enterprise_serializer_spec.rb | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 92b62ea919..39c3a5fde1 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -30,9 +30,6 @@ describe Api::CachedEnterpriseSerializer do let(:shop) { create(:distributor_enterprise) } let(:options) { { data: enterprise_injection_data } } - let(:enterprise_injection_data) do - instance_double(OpenFoodNetwork::EnterpriseInjectionData, active_distributors: []) - end let(:property) { create(:property, presentation: 'One') } let(:duplicate_property) { create(:property, presentation: 'One') } @@ -51,9 +48,26 @@ describe Api::CachedEnterpriseSerializer do ) end - it 'does not duplicate properties' do - properties = cached_enterprise_serializer.distributed_properties - expect(properties).to eq([property]) + context 'when the enterprise is not an active distributor' do + let(:enterprise_injection_data) do + instance_double(OpenFoodNetwork::EnterpriseInjectionData, active_distributors: []) + end + + it 'does not duplicate properties' do + properties = cached_enterprise_serializer.distributed_properties + expect(properties).to eq([property]) + end + end + + context 'when the enterprise is an active distributor' do + let(:enterprise_injection_data) do + instance_double(OpenFoodNetwork::EnterpriseInjectionData, active_distributors: [shop]) + end + + it 'does not duplicate properties' do + properties = cached_enterprise_serializer.distributed_properties + expect(properties).to eq([property]) + end end end end From d4635bd7cbc70aab7b15112a426de237384d2f7b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 18 Dec 2018 18:16:49 +0100 Subject: [PATCH 09/17] Fetch Property instead of ProducerProps from query By not having to treat producer properties differently we can filter and fetch all properties from DB without having to process it with Ruby and with a single query. --- .../api/cached_enterprise_serializer.rb | 25 ++++++++++++++++--- .../api/cached_enterprise_serializer_spec.rb | 18 ++++++++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 6198e28a6c..8da705a1b0 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -85,17 +85,34 @@ module Api def distributed_properties if active product_properties = Spree::Property.currently_sold_by(enterprise) - producer_properties = ProducerProperty.currently_sold_by(enterprise) else - product_properties = Spree::Property.ever_sold_by(enterprise) - producer_properties = ProducerProperty.ever_sold_by(enterprise) + product_properties = Spree::Property + .joins(products: { variants: { exchanges: :order_cycle } }) + .merge(Exchange.outgoing) + .merge(Exchange.to_enterprise(enterprise)) + .select('DISTINCT spree_properties.*') end - (product_properties + producer_properties).uniq do |property_object| + (product_properties + distributed_producer_properties).uniq do |property_object| property_object.property.presentation end end + def distributed_producer_properties + properties = Spree::Property + .joins( + producer_properties: { + producer: { supplied_products: { variants: { exchanges: :order_cycle } } } + } + ) + .merge(Exchange.outgoing) + .merge(Exchange.to_enterprise(enterprise)) + .select('DISTINCT spree_properties.*') + + return properties.merge(OrderCycle.active) if active + properties + end + def active data.active_distributors.andand.include? enterprise end diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 39c3a5fde1..0d3cd642bf 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -33,10 +33,10 @@ describe Api::CachedEnterpriseSerializer do let(:property) { create(:property, presentation: 'One') } let(:duplicate_property) { create(:property, presentation: 'One') } + let(:producer) { create(:supplier_enterprise, properties: [duplicate_property]) } before do product = create(:product, properties: [property]) - producer = create(:supplier_enterprise, properties: [duplicate_property]) producer.supplied_products << product create( @@ -57,6 +57,14 @@ describe Api::CachedEnterpriseSerializer do properties = cached_enterprise_serializer.distributed_properties expect(properties).to eq([property]) end + + it 'fetches producer properties' do + distributed_producer_properties = cached_enterprise_serializer + .distributed_producer_properties + + expect(distributed_producer_properties) + .to eq(producer.producer_properties.map(&:property)) + end end context 'when the enterprise is an active distributor' do @@ -68,6 +76,14 @@ describe Api::CachedEnterpriseSerializer do properties = cached_enterprise_serializer.distributed_properties expect(properties).to eq([property]) end + + it 'fetches producer properties' do + distributed_producer_properties = cached_enterprise_serializer + .distributed_producer_properties + + expect(distributed_producer_properties) + .to eq(producer.producer_properties.map(&:property)) + end end end end From e55fbf80fc2b5d9d29df69c3b75c8e7a41268fc4 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 19 Dec 2018 11:52:07 +0100 Subject: [PATCH 10/17] Check property presentation attribute only in test The PropertySerializer contains :id, :name and :presentation attributes which leads to JSON objects like `{"id": 1, "name": "foo", "presentation": "foo"}`. Because of this, we don't care about object identity, just about their presentation attribute. --- .../api/cached_enterprise_serializer_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 0d3cd642bf..4401409cb2 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -55,15 +55,15 @@ describe Api::CachedEnterpriseSerializer do it 'does not duplicate properties' do properties = cached_enterprise_serializer.distributed_properties - expect(properties).to eq([property]) + expect(properties.map(&:presentation)).to eq([property.presentation]) end it 'fetches producer properties' do distributed_producer_properties = cached_enterprise_serializer .distributed_producer_properties - expect(distributed_producer_properties) - .to eq(producer.producer_properties.map(&:property)) + expect(distributed_producer_properties.map(&:presentation)) + .to eq(producer.producer_properties.map(&:property).map(&:presentation)) end end @@ -74,15 +74,15 @@ describe Api::CachedEnterpriseSerializer do it 'does not duplicate properties' do properties = cached_enterprise_serializer.distributed_properties - expect(properties).to eq([property]) + expect(properties.map(&:presentation)).to eq([property.presentation]) end it 'fetches producer properties' do distributed_producer_properties = cached_enterprise_serializer .distributed_producer_properties - expect(distributed_producer_properties) - .to eq(producer.producer_properties.map(&:property)) + expect(distributed_producer_properties.map(&:presentation)) + .to eq(producer.producer_properties.map(&:property).map(&:presentation)) end end end From 7b2d4f10ee68732b63b609acee32e0bdd91bf5e8 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 19 Dec 2018 13:01:48 +0100 Subject: [PATCH 11/17] Cover #distributed_properties \w a controller spec This exercises the controller and the serializer ensuring the integration works allowing to confidently refactor things further. --- spec/controllers/shops_controller_spec.rb | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/controllers/shops_controller_spec.rb b/spec/controllers/shops_controller_spec.rb index 44f896d5f3..82ded402fd 100644 --- a/spec/controllers/shops_controller_spec.rb +++ b/spec/controllers/shops_controller_spec.rb @@ -15,4 +15,25 @@ describe ShopsController, type: :controller do get :index expect(response.body).to have_content(invisible_distributor.name) end + + it 'renders distributed properties' do + duplicate_property = create(:property, presentation: 'dairy') + producer = create(:supplier_enterprise, properties: [duplicate_property]) + property = create(:property, presentation: 'dairy') + product = create(:product, properties: [property]) + producer.supplied_products << product + + create( + :simple_order_cycle, + coordinator: distributor, + suppliers: [producer], + distributors: [distributor], + variants: [product.variants] + ) + + get :index + + expect(response.body) + .to match(/"distributed_properties":\[{"id":\d+,"name":"dairy","presentation":"dairy"}\]/) + end end From 6e9bb640f6d7b4b3ec85f596637eefa541e4890b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 19 Dec 2018 14:26:04 +0100 Subject: [PATCH 12/17] Add spacing between long scopes --- app/models/enterprise.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index ea1724f20a..656082bb79 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -114,12 +114,15 @@ class Enterprise < ActiveRecord::Base scope :with_distributed_products_outer, joins('LEFT OUTER JOIN product_distributions ON product_distributions.distributor_id = enterprises.id'). joins('LEFT OUTER JOIN spree_products ON spree_products.id = product_distributions.product_id') + scope :with_order_cycles_as_supplier_outer, joins("LEFT OUTER JOIN exchanges ON (exchanges.sender_id = enterprises.id AND exchanges.incoming = 't')"). joins('LEFT OUTER JOIN order_cycles ON (order_cycles.id = exchanges.order_cycle_id)') + scope :with_order_cycles_as_distributor_outer, joins("LEFT OUTER JOIN exchanges ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f')"). joins('LEFT OUTER JOIN order_cycles ON (order_cycles.id = exchanges.order_cycle_id)') + scope :with_order_cycles_outer, joins("LEFT OUTER JOIN exchanges ON (exchanges.receiver_id = enterprises.id OR exchanges.sender_id = enterprises.id)"). joins('LEFT OUTER JOIN order_cycles ON (order_cycles.id = exchanges.order_cycle_id)') From 24c031b3cd09301d6332b08bcd8f0b0ace93f26d Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 10 Jan 2019 17:48:09 +0100 Subject: [PATCH 13/17] Extract #distributed_product_properties method --- .../api/cached_enterprise_serializer.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 8da705a1b0..caf9164a86 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -83,19 +83,21 @@ module Api # This results in 3 queries per enterprise def distributed_properties + (distributed_product_properties + distributed_producer_properties).uniq do |property_object| + property_object.property.presentation + end + end + + def distributed_product_properties if active - product_properties = Spree::Property.currently_sold_by(enterprise) + Spree::Property.currently_sold_by(enterprise) else - product_properties = Spree::Property + Spree::Property .joins(products: { variants: { exchanges: :order_cycle } }) .merge(Exchange.outgoing) .merge(Exchange.to_enterprise(enterprise)) .select('DISTINCT spree_properties.*') end - - (product_properties + distributed_producer_properties).uniq do |property_object| - property_object.property.presentation - end end def distributed_producer_properties From 19cbda41285d2032453161feca31ef64f936102a Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 11 Jan 2019 14:53:51 +0100 Subject: [PATCH 14/17] Fix Rubocop violation --- app/serializers/api/cached_enterprise_serializer.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index caf9164a86..c88dc2e27b 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -101,12 +101,11 @@ module Api end def distributed_producer_properties - properties = Spree::Property - .joins( - producer_properties: { - producer: { supplied_products: { variants: { exchanges: :order_cycle } } } - } - ) + properties = Spree::Property.joins( + producer_properties: { + producer: { supplied_products: { variants: { exchanges: :order_cycle } } } + } + ) .merge(Exchange.outgoing) .merge(Exchange.to_enterprise(enterprise)) .select('DISTINCT spree_properties.*') From 915b5cf09ed1b3e6cd53b0f8339d353f30b82450 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 26 Feb 2019 11:37:48 +0100 Subject: [PATCH 15/17] Inline also the active product properties query This makes it easier to spot what are the differences and similarities between active and non-active, which in turn, it'll help remove the N+1 the "active" branch causes. Make it look `#distributed_producer_properties` with which seems to share a lot. --- .../api/cached_enterprise_serializer.rb | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index c88dc2e27b..ebb56dfb0e 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -89,23 +89,23 @@ module Api end def distributed_product_properties - if active - Spree::Property.currently_sold_by(enterprise) - else - Spree::Property - .joins(products: { variants: { exchanges: :order_cycle } }) - .merge(Exchange.outgoing) - .merge(Exchange.to_enterprise(enterprise)) - .select('DISTINCT spree_properties.*') - end + properties = Spree::Property + .joins(products: { variants: { exchanges: :order_cycle } }) + .merge(Exchange.outgoing) + .merge(Exchange.to_enterprise(enterprise)) + .select('DISTINCT spree_properties.*') + + return properties.merge(OrderCycle.active) if active + properties end def distributed_producer_properties - properties = Spree::Property.joins( - producer_properties: { - producer: { supplied_products: { variants: { exchanges: :order_cycle } } } - } - ) + properties = Spree::Property + .joins( + producer_properties: { + producer: { supplied_products: { variants: { exchanges: :order_cycle } } } + } + ) .merge(Exchange.outgoing) .merge(Exchange.to_enterprise(enterprise)) .select('DISTINCT spree_properties.*') From a728da4f624d22064246d5e154f395619729a2da Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 26 Feb 2019 12:09:26 +0100 Subject: [PATCH 16/17] Cover product and producer props. in ctrl. spec --- spec/controllers/shops_controller_spec.rb | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/spec/controllers/shops_controller_spec.rb b/spec/controllers/shops_controller_spec.rb index 82ded402fd..5dc929b5b7 100644 --- a/spec/controllers/shops_controller_spec.rb +++ b/spec/controllers/shops_controller_spec.rb @@ -16,10 +16,49 @@ describe ShopsController, type: :controller do expect(response.body).to have_content(invisible_distributor.name) end + it 'renders distributed product properties' do + product_property = create(:property, presentation: 'eggs') + product = create(:product, properties: [product_property]) + producer = create(:supplier_enterprise) + + create( + :simple_order_cycle, + coordinator: distributor, + suppliers: [producer], + distributors: [distributor], + variants: [product.variants] + ) + + get :index + + expect(response.body) + .to match(/"distributed_properties":\[{"id":\d+,"name":"eggs","presentation":"eggs"}\]/) + end + + it 'renders distributed producer properties' do + producer_property = create(:property, presentation: 'certified') + producer = create(:supplier_enterprise, properties: [producer_property]) + product = create(:product) + + create( + :simple_order_cycle, + coordinator: distributor, + suppliers: [producer], + distributors: [distributor], + variants: [product.variants] + ) + + get :index + + expect(response.body) + .to match(/"distributed_properties":\[{"id":\d+,"name":"certified","presentation":"certified"}\]/) + end + it 'renders distributed properties' do duplicate_property = create(:property, presentation: 'dairy') producer = create(:supplier_enterprise, properties: [duplicate_property]) property = create(:property, presentation: 'dairy') + product = create(:product, properties: [property]) producer.supplied_products << product From 54e120889ffb8ef2f89194c1f39216df243ed34f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 4 Mar 2019 11:08:34 +0100 Subject: [PATCH 17/17] Remove useless array wrapping on AR relation --- spec/serializers/api/cached_enterprise_serializer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 4401409cb2..e46b0006b9 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -44,7 +44,7 @@ describe Api::CachedEnterpriseSerializer do coordinator: shop, suppliers: [producer], distributors: [shop], - variants: [product.variants] + variants: product.variants ) end