Merge pull request #3151 from coopdevs/remove-frontoffice-enterprises-n+1

Remove frontoffice enterprises n+1

app/controllers/shops_controller.rb

@@ -4,5 +4,11 @@ class ShopsController < BaseController
   before_filter :enable_embedded_shopfront
 
   def index
+    @enterprises = Enterprise
+      .activated
+      .includes(address: :state)
+      .includes(:properties)
+      .includes(supplied_products: :properties)
+      .all
   end
 end

app/helpers/injection_helper.rb

@@ -1,8 +1,13 @@
 require 'open_food_network/enterprise_injection_data'
 
 module InjectionHelper
-  def inject_enterprises
-    inject_json_ams "enterprises", Enterprise.activated.includes(address: :state).all, Api::EnterpriseSerializer, enterprise_injection_data
+  def inject_enterprises(enterprises = Enterprise.activated.includes(address: :state).all)
+    inject_json_ams(
+      'enterprises',
+      enterprises,
+      Api::EnterpriseSerializer,
+      enterprise_injection_data
+    )
   end
 
   def inject_enterprise_and_relatives

app/models/spree/property_decorator.rb

@@ -26,6 +26,9 @@ module Spree
     # When a Property is destroyed, dependent-destroy will destroy all ProductProperties,
     # which will take care of refreshing the products cache
 
+    def property
+      self
+    end
 
     private
 

app/serializers/api/cached_enterprise_serializer.rb

@@ -0,0 +1,161 @@
+require 'open_food_network/property_merge'
+
+module Api
+  class CachedEnterpriseSerializer < ActiveModel::Serializer
+    include SerializerHelper
+
+    cached
+
+    def cache_key
+      object.andand.cache_key
+    end
+
+    attributes :name, :id, :description, :latitude, :longitude,
+               :long_description, :website, :instagram, :linkedin, :twitter,
+               :facebook, :is_primary_producer, :is_distributor, :phone, :visible,
+               :email_address, :hash, :logo, :promo_image, :path, :pickup, :delivery,
+               :icon, :icon_font, :producer_icon_font, :category, :producers, :hubs
+
+    attributes :taxons, :supplied_taxons
+
+    has_one :address, serializer: AddressSerializer
+
+    has_many :supplied_properties, serializer: PropertySerializer
+    has_many :distributed_properties, serializer: PropertySerializer
+
+    def pickup
+      services = data.shipping_method_services[object.id]
+      services ? services[:pickup] : false
+    end
+
+    def delivery
+      services = data.shipping_method_services[object.id]
+      services ? services[:delivery] : false
+    end
+
+    def email_address
+      object.email_address.to_s.reverse
+    end
+
+    def hash
+      object.to_param
+    end
+
+    def logo
+      object.logo(:medium) if object.logo?
+    end
+
+    def promo_image
+      object.promo_image(:large) if object.promo_image?
+    end
+
+    def path
+      enterprise_shop_path(object)
+    end
+
+    def producers
+      relatives = data.relatives[object.id]
+      ids_to_objs(relatives.andand[:producers])
+    end
+
+    def hubs
+      relatives = data.relatives[object.id]
+      ids_to_objs(relatives.andand[:distributors])
+    end
+
+    def taxons
+      if active
+        ids_to_objs data.current_distributed_taxons[object.id]
+      else
+        ids_to_objs data.all_distributed_taxons[object.id]
+      end
+    end
+
+    def supplied_taxons
+      ids_to_objs data.supplied_taxons[object.id]
+    end
+
+    def supplied_properties
+      (product_properties + producer_properties).uniq do |property_object|
+        property_object.property.presentation
+      end
+    end
+
+    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)
+      end
+
+      producer_properties = Spree::Property.where(id: producer_property_ids)
+
+      OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties
+    end
+
+    def active
+      data.active_distributors.andand.include? object
+    end
+
+    # Map svg icons.
+    def icon
+      icons = {
+        hub: "/assets/map_005-hub.svg",
+        hub_profile: "/assets/map_006-hub-profile.svg",
+        producer_hub: "/assets/map_005-hub.svg",
+        producer_shop: "/assets/map_003-producer-shop.svg",
+        producer: "/assets/map_001-producer-only.svg",
+      }
+      icons[object.category]
+    end
+
+    # Choose regular icon font for enterprises.
+    def icon_font
+      icon_fonts = {
+        hub: "ofn-i_063-hub",
+        hub_profile: "ofn-i_064-hub-reversed",
+        producer_hub: "ofn-i_063-hub",
+        producer_shop: "ofn-i_059-producer",
+        producer: "ofn-i_059-producer",
+      }
+      icon_fonts[object.category]
+    end
+
+    # Choose producer page icon font - yes, sadly its got to be different.
+    # This duplicates some code but covers the producer page edge case where
+    # producer-hub has a producer icon without needing to duplicate the category logic in angular.
+    def producer_icon_font
+      icon_fonts = {
+        hub: "",
+        hub_profile: "",
+        producer_hub: "ofn-i_059-producer",
+        producer_shop: "ofn-i_059-producer",
+        producer: "ofn-i_059-producer",
+      }
+      icon_fonts[object.category]
+    end
+
+    private
+
+    def product_properties
+      enterprise.supplied_products.flat_map(&:properties)
+    end
+
+    def producer_properties
+      enterprise.properties
+    end
+
+    def enterprise
+      object
+    end
+
+    def data
+      options[:data]
+    end
+  end
+end

app/serializers/api/enterprise_serializer.rb

@@ -18,161 +18,3 @@ class Api::EnterpriseSerializer < ActiveModel::Serializer
     Api::UncachedEnterpriseSerializer.new(object, @options).serializable_hash || {}
   end
 end
-
-class Api::UncachedEnterpriseSerializer < ActiveModel::Serializer
-  include SerializerHelper
-
-  attributes :orders_close_at, :active
-
-  def orders_close_at
-    options[:data].earliest_closing_times[object.id]
-  end
-
-  def active
-    options[:data].active_distributors.andand.include? object
-  end
-end
-
-class Api::CachedEnterpriseSerializer < ActiveModel::Serializer
-  include SerializerHelper
-
-  cached
-  #delegate :cache_key, to: :object
-
-  def cache_key
-    object.andand.cache_key
-  end
-
-
-  attributes :name, :id, :description, :latitude, :longitude,
-    :long_description, :website, :instagram, :linkedin, :twitter,
-    :facebook, :is_primary_producer, :is_distributor, :phone, :visible,
-    :email_address, :hash, :logo, :promo_image, :path, :pickup, :delivery,
-    :icon, :icon_font, :producer_icon_font, :category, :producers, :hubs
-
-  attributes :taxons, :supplied_taxons
-
-  has_one :address, serializer: Api::AddressSerializer
-
-  has_many :supplied_properties, serializer: Api::PropertySerializer
-  has_many :distributed_properties, serializer: Api::PropertySerializer
-
-  def pickup
-    services = options[:data].shipping_method_services[object.id]
-    services ? services[:pickup] : false
-  end
-
-  def delivery
-    services = options[:data].shipping_method_services[object.id]
-    services ? services[:delivery] : false
-  end
-
-  def email_address
-    object.email_address.to_s.reverse
-  end
-
-  def hash
-    object.to_param
-  end
-
-  def logo
-    object.logo(:medium) if object.logo?
-  end
-
-  def promo_image
-    object.promo_image(:large) if object.promo_image?
-  end
-
-  def path
-    enterprise_shop_path(object)
-  end
-
-  def producers
-    relatives = options[:data].relatives[object.id]
-    ids_to_objs(relatives.andand[:producers])
-  end
-
-  def hubs
-    relatives = options[:data].relatives[object.id]
-    ids_to_objs(relatives.andand[:distributors])
-  end
-
-  def taxons
-    if active
-      ids_to_objs options[:data].current_distributed_taxons[object.id]
-    else
-      ids_to_objs options[:data].all_distributed_taxons[object.id]
-    end
-  end
-
-  def supplied_taxons
-    ids_to_objs options[:data].supplied_taxons[object.id]
-  end
-
-  def supplied_properties
-    # This results in 3 queries per enterprise
-    product_properties  = Spree::Property.applied_by(object)
-    producer_properties = object.properties
-
-    OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties
-  end
-
-  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)
-    end
-
-    producer_properties = Spree::Property.where(id: producer_property_ids)
-
-    OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties
-  end
-
-  def active
-    options[:data].active_distributors.andand.include? object
-  end
-
-  # Map svg icons.
-  def icon
-    icons = {
-      :hub => "/assets/map_005-hub.svg",
-      :hub_profile => "/assets/map_006-hub-profile.svg",
-      :producer_hub => "/assets/map_005-hub.svg",
-      :producer_shop => "/assets/map_003-producer-shop.svg",
-      :producer => "/assets/map_001-producer-only.svg",
-    }
-    icons[object.category]
-  end
-
-  # Choose regular icon font for enterprises.
-  def icon_font
-    icon_fonts = {
-      :hub => "ofn-i_063-hub",
-      :hub_profile => "ofn-i_064-hub-reversed",
-      :producer_hub => "ofn-i_063-hub",
-      :producer_shop => "ofn-i_059-producer",
-      :producer => "ofn-i_059-producer",
-    }
-    icon_fonts[object.category]
-  end
-
-  # Choose producer page icon font - yes, sadly its got to be different.
-  # This duplicates some code but covers the producer page edge case where
-  # producer-hub has a producer icon without needing to duplicate the category logic in angular.
-  def producer_icon_font
-    icon_fonts = {
-      :hub => "",
-      :hub_profile => "",
-      :producer_hub => "ofn-i_059-producer",
-      :producer_shop => "ofn-i_059-producer",
-      :producer => "ofn-i_059-producer",
-    }
-    icon_fonts[object.category]
-  end
-end

app/serializers/api/uncached_enterprise_serializer.rb

@@ -0,0 +1,15 @@
+module Api
+  class UncachedEnterpriseSerializer < ActiveModel::Serializer
+    include SerializerHelper
+
+    attributes :orders_close_at, :active
+
+    def orders_close_at
+      options[:data].earliest_closing_times[object.id]
+    end
+
+    def active
+      options[:data].active_distributors.andand.include? object
+    end
+  end
+end

app/views/shops/_hubs.html.haml

@@ -1,4 +1,4 @@
-= inject_enterprises
+= inject_enterprises(@enterprises)
 
 #hubs.hubs{"ng-controller" => "EnterprisesCtrl", "ng-cloak" => true}
   .row

lib/open_food_network/property_merge.rb

@@ -1,18 +1,9 @@
 module OpenFoodNetwork
   class PropertyMerge
     def self.merge(primary, secondary)
-      primary + secondary.reject do |secondary_p|
-        primary.any? do |primary_p|
-          property_of(primary_p).presentation == property_of(secondary_p).presentation
-        end
+      (primary + secondary).uniq do |property_object|
+        property_object.property.presentation
       end
     end
-
-
-    private
-
-    def self.property_of(p)
-      p.respond_to?(:property) ? p.property : p
-    end
   end
 end

spec/controllers/shops_controller_spec.rb

@@ -2,16 +2,17 @@ require 'spec_helper'
 
 describe ShopsController, type: :controller do
   render_views
+
   let!(:distributor) { create(:distributor_enterprise) }
   let!(:invisible_distributor) { create(:distributor_enterprise, visible: false) }
 
   before do
-    Enterprise.stub_chain("distributors_with_active_order_cycles.ready_for_checkout") { [distributor] }
+    allow(Enterprise).to receive_message_chain(:distributors_with_active_order_cycles, :ready_for_checkout) { [distributor] }
   end
 
   # Exclusion from actual rendered view handled in features/consumer/home
   it "shows JSON for invisible hubs" do
     get :index
-    response.body.should have_content invisible_distributor.name
+    expect(response.body).to have_content(invisible_distributor.name)
   end
 end

spec/lib/open_food_network/property_merge_spec.rb

@@ -2,23 +2,31 @@ require 'spec_helper'
 
 module OpenFoodNetwork
   describe PropertyMerge do
-    let(:p1a) { create(:property, presentation: 'One') }
-    let(:p1b) { create(:property, presentation: 'One') }
-    let(:p2)  { create(:property, presentation: 'Two') }
+    let(:property) { create(:property, presentation: 'One') }
+    let(:duplicate_property) { create(:property, presentation: 'One') }
+    let(:different_property) { create(:property, presentation: 'Two') }
 
     describe "merging Spree::Properties" do
       it "merges properties" do
-        expect(PropertyMerge.merge([p1a], [p1b, p2])).to eq [p1a, p2]
+        merge = PropertyMerge.merge(
+          [property],
+          [duplicate_property, different_property]
+        )
+        expect(merge).to eq [property, different_property]
       end
     end
 
     describe "merging ProducerProperties and Spree::ProductProperties" do
-      let(:pp1a) { create(:product_property,  property: p1a) }
-      let(:pp1b) { create(:producer_property, property: p1b) }
-      let(:pp2)  { create(:producer_property, property: p2) }
+      let(:product_property) { create(:product_property,  property: property) }
+      let(:duplicate_product_property) { create(:producer_property, property: duplicate_property) }
+      let(:producer_property) { create(:producer_property, property: different_property) }
 
       it "merges properties" do
-        expect(PropertyMerge.merge([pp1a], [pp1b, pp2])).to eq [pp1a, pp2]
+        merge = PropertyMerge.merge(
+          [product_property],
+          [duplicate_product_property, producer_property]
+        )
+        expect(merge).to eq [product_property, producer_property]
       end
     end
   end

spec/serializers/api/cached_enterprise_serializer_spec.rb

@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe Api::CachedEnterpriseSerializer do
+  let(:cached_enterprise_serializer) { described_class.new(enterprise) }
+  let(:enterprise) { create(:enterprise) }
+
+  describe '#supplied_properties' do
+    let(:property) { create(:property, presentation: 'One') }
+    let(:duplicate_property) { create(:property, presentation: 'One') }
+    let(:different_property) { create(:property, presentation: 'Two') }
+
+    let(:enterprise) do
+      create(:enterprise, properties: [duplicate_property, different_property])
+    end
+
+    before do
+      product = create(:product, properties: [property])
+      enterprise.supplied_products << product
+    end
+
+    it "removes duplicate product and producer properties" do
+      properties = cached_enterprise_serializer.supplied_properties
+      expect(properties).to eq([property, different_property])
+    end
+  end
+end