Merge pull request #3305 from coopdevs/improve-shops-performance-take-2

Improve shops performance take 2

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)')

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
@@ -81,25 +81,41 @@ 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)
-
-      else
-        product_properties = Spree::Property.ever_sold_by(object)
-        producer_property_ids = ProducerProperty.ever_sold_by(object).pluck(:property_id)
+      (distributed_product_properties + distributed_producer_properties).uniq do |property_object|
+        property_object.property.presentation
       end
+    end
 
-      producer_properties = Spree::Property.where(id: producer_property_ids)
+    def distributed_product_properties
+      properties = Spree::Property
+        .joins(products: { variants: { exchanges: :order_cycle } })
+        .merge(Exchange.outgoing)
+        .merge(Exchange.to_enterprise(enterprise))
+        .select('DISTINCT spree_properties.*')
 
-      OpenFoodNetwork::PropertyMerge.merge product_properties, producer_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 } } }
+          }
+        )
+        .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? object
+      data.active_distributors.andand.include? enterprise
     end
 
     # Map svg icons.
@@ -111,7 +127,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.
@@ -123,7 +139,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.
@@ -137,7 +153,7 @@ module Api
         producer_shop: "ofn-i_059-producer",
         producer: "ofn-i_059-producer",
       }
-      icon_fonts[object.category]
+      icon_fonts[enterprise.category]
     end
 
     private

spec/controllers/shops_controller_spec.rb

@@ -15,4 +15,64 @@ describe ShopsController, type: :controller do
     get :index
     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
+
+    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

spec/serializers/api/cached_enterprise_serializer_spec.rb

@@ -23,4 +23,67 @@ 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(:shop) { create(:distributor_enterprise) }
+
+    let(:options) { { data: enterprise_injection_data } }
+
+    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.supplied_products << product
+
+      create(
+        :simple_order_cycle,
+        coordinator: shop,
+        suppliers: [producer],
+        distributors: [shop],
+        variants: product.variants
+      )
+    end
+
+    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.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.map(&:presentation))
+          .to eq(producer.producer_properties.map(&:property).map(&:presentation))
+      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.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.map(&:presentation))
+          .to eq(producer.producer_properties.map(&:property).map(&:presentation))
+      end
+    end
+  end
 end