From 5a85d7a77b5228f54caa99f6d6791d58d462f7fa Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 12:27:30 +0100 Subject: [PATCH 01/14] RSpec3-ize controller spec --- spec/controllers/shops_controller_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/controllers/shops_controller_spec.rb b/spec/controllers/shops_controller_spec.rb index 400fa2f563..44f896d5f3 100644 --- a/spec/controllers/shops_controller_spec.rb +++ b/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 From 41d988176c806e469962390451cd362edac52126 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 12:32:30 +0100 Subject: [PATCH 02/14] Pass enterprises to inject method from controller --- app/controllers/shops_controller.rb | 1 + app/helpers/injection_helper.rb | 9 +++++++-- app/views/shops/_hubs.html.haml | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/controllers/shops_controller.rb b/app/controllers/shops_controller.rb index aa0d2f8f33..1a0eacade4 100644 --- a/app/controllers/shops_controller.rb +++ b/app/controllers/shops_controller.rb @@ -4,5 +4,6 @@ class ShopsController < BaseController before_filter :enable_embedded_shopfront def index + @enterprises = Enterprise.activated.includes(address: :state).all end end diff --git a/app/helpers/injection_helper.rb b/app/helpers/injection_helper.rb index f4e857607e..dfcf2bb6ff 100644 --- a/app/helpers/injection_helper.rb +++ b/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 diff --git a/app/views/shops/_hubs.html.haml b/app/views/shops/_hubs.html.haml index 3e477b9551..aa41f3f229 100644 --- a/app/views/shops/_hubs.html.haml +++ b/app/views/shops/_hubs.html.haml @@ -1,4 +1,4 @@ -= inject_enterprises += inject_enterprises(@enterprises) #hubs.hubs{"ng-controller" => "EnterprisesCtrl", "ng-cloak" => true} .row From c6ab5feb81dd8b6dc43aca3c868c8cf8de6426cc Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 13:31:44 +0100 Subject: [PATCH 03/14] Move cached & uncached serializers to their files --- .../api/cached_enterprise_serializer.rb | 145 ++++++++++++++++ app/serializers/api/enterprise_serializer.rb | 158 ------------------ .../api/uncached_enterprise_serializer.rb | 13 ++ 3 files changed, 158 insertions(+), 158 deletions(-) create mode 100644 app/serializers/api/cached_enterprise_serializer.rb create mode 100644 app/serializers/api/uncached_enterprise_serializer.rb diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb new file mode 100644 index 0000000000..e42887fe9b --- /dev/null +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -0,0 +1,145 @@ +require 'open_food_network/property_merge' + +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 diff --git a/app/serializers/api/enterprise_serializer.rb b/app/serializers/api/enterprise_serializer.rb index e3829174a2..fa0b09762a 100644 --- a/app/serializers/api/enterprise_serializer.rb +++ b/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 diff --git a/app/serializers/api/uncached_enterprise_serializer.rb b/app/serializers/api/uncached_enterprise_serializer.rb new file mode 100644 index 0000000000..010348da87 --- /dev/null +++ b/app/serializers/api/uncached_enterprise_serializer.rb @@ -0,0 +1,13 @@ +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 From 23f629cfd6452fdda84cba283777e3241f675287 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 13:33:20 +0100 Subject: [PATCH 04/14] Autofix all Rubocop violations in serializer --- .../api/cached_enterprise_serializer.rb | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index e42887fe9b..8fcf9df328 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -4,18 +4,16 @@ 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 + :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 @@ -88,11 +86,11 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer # This results in 3 queries per enterprise if active - product_properties = Spree::Property.currently_sold_by(object) + 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) + product_properties = Spree::Property.ever_sold_by(object) producer_property_ids = ProducerProperty.ever_sold_by(object).pluck(:property_id) end @@ -108,11 +106,11 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer # 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", + 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 @@ -120,11 +118,11 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer # 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", + 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 @@ -134,11 +132,11 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer # 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", + 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 From bc7258d43b43cb8cf1da2074b3f0e4f03b61ca71 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 13:54:41 +0100 Subject: [PATCH 05/14] Test #supplied_properties helper methods --- .../api/cached_enterprise_serializer.rb | 19 +++++++++--- .../api/cached_enterprise_serializer_spec.rb | 29 +++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 spec/serializers/api/cached_enterprise_serializer_spec.rb diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 8fcf9df328..24c16d97b5 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -74,11 +74,16 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer 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 + def product_properties + Spree::Property.applied_by(object) + end + def producer_properties + enterprise.properties + end + + # This results in 3 queries per enterprise + def supplied_properties OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties end @@ -140,4 +145,10 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer } icon_fonts[object.category] end + + private + + def enterprise + object + end end diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb new file mode 100644 index 0000000000..8f500dca57 --- /dev/null +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe Api::CachedEnterpriseSerializer do + let(:cached_enterprise_serializer) { described_class.new(enterprise) } + let(:enterprise) { create(:enterprise) } + + describe '#product_properties' do + let(:property) { create(:property) } + + before do + product = create(:product, properties: [property]) + enterprise.supplied_products << product + end + + it 'returns the properties of the products supplied by the enterprise' do + expect(cached_enterprise_serializer.product_properties).to eq([property]) + end + end + + describe '#producer_properties' do + let(:property) { create(:property) } + + before { enterprise.properties << property } + + it 'returns the properties of the enterprise' do + expect(cached_enterprise_serializer.producer_properties).to eq([property]) + end + end +end From 8e1f9a1ba3e95461554e3955dbb3decaa6acd029 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 13:57:19 +0100 Subject: [PATCH 06/14] Refactor #product_properties not to fetch ids --- 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 24c16d97b5..cbf562d26d 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -75,7 +75,9 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer end def product_properties - Spree::Property.applied_by(object) + Spree::Property + .joins(product_properties: { product: :supplier }) + .where(spree_products: { supplier_id: enterprise }) end def producer_properties From 1363a2a17dc6152175d6849a4172768b260de7ed Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 16:01:03 +0100 Subject: [PATCH 07/14] Improve readability of spec --- .../open_food_network/property_merge_spec.rb | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/spec/lib/open_food_network/property_merge_spec.rb b/spec/lib/open_food_network/property_merge_spec.rb index 85f4200ab0..37568e706c 100644 --- a/spec/lib/open_food_network/property_merge_spec.rb +++ b/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 From faac5f4b2d414b0f521a1ea71cc6d1251d3445f6 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 17:01:04 +0100 Subject: [PATCH 08/14] Refactor PropertyMerge to use Ruby's #uniq instead --- app/models/spree/property_decorator.rb | 3 +++ lib/open_food_network/property_merge.rb | 13 ++----------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/app/models/spree/property_decorator.rb b/app/models/spree/property_decorator.rb index 7349436a2b..5950212149 100644 --- a/app/models/spree/property_decorator.rb +++ b/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 diff --git a/lib/open_food_network/property_merge.rb b/lib/open_food_network/property_merge.rb index 9c21fb7b0d..82bf92eaee 100644 --- a/lib/open_food_network/property_merge.rb +++ b/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 From 149fc1ac6f3876acf34245e9e1a598ec6cd2651c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 17:17:55 +0100 Subject: [PATCH 09/14] Inline PropertyMerge's #merge into serializer Now that it has become a plain Ruby #uniq, there's no point on having the abstraction and indirection. --- .../api/cached_enterprise_serializer.rb | 4 ++- .../api/cached_enterprise_serializer_spec.rb | 34 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index cbf562d26d..a9bf15743d 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -86,7 +86,9 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer # This results in 3 queries per enterprise def supplied_properties - OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties + (product_properties + producer_properties).uniq do |property_object| + property_object.property.presentation + end end def distributed_properties diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 8f500dca57..017403de65 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -26,4 +26,38 @@ describe Api::CachedEnterpriseSerializer do expect(cached_enterprise_serializer.producer_properties).to eq([property]) end end + + describe '#supplied_properties' do + 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 + allow(cached_enterprise_serializer) + .to receive(:product_properties) { [property] } + allow(cached_enterprise_serializer) + .to receive(:producer_properties) { [duplicate_property, different_property] } + + merge = cached_enterprise_serializer.supplied_properties + expect(merge).to eq([property, different_property]) + end + end + + describe "merging ProducerProperties and Spree::ProductProperties" do + 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 + allow(cached_enterprise_serializer) + .to receive(:product_properties) { [product_property] } + allow(cached_enterprise_serializer) + .to receive(:producer_properties) { [duplicate_product_property, producer_property] } + + merge = cached_enterprise_serializer.supplied_properties + expect(merge).to eq([product_property, producer_property]) + end + end + end end From e40c1c08ca075f85ddc74e60e80942ea854a3b9f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 17:32:32 +0100 Subject: [PATCH 10/14] Refactor spec to not stub the class under test This also relies on ActiveRecord which will come in handy as we move on the refactor. --- .../api/cached_enterprise_serializer_spec.rb | 32 ++++++------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index 017403de65..dbef516df5 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -32,32 +32,18 @@ describe Api::CachedEnterpriseSerializer do let(:duplicate_property) { create(:property, presentation: 'One') } let(:different_property) { create(:property, presentation: 'Two') } - describe "merging Spree::Properties" do - it "merges properties" do - allow(cached_enterprise_serializer) - .to receive(:product_properties) { [property] } - allow(cached_enterprise_serializer) - .to receive(:producer_properties) { [duplicate_property, different_property] } - - merge = cached_enterprise_serializer.supplied_properties - expect(merge).to eq([property, different_property]) - end + let(:enterprise) do + create(:enterprise, properties: [duplicate_property, different_property]) end - describe "merging ProducerProperties and Spree::ProductProperties" do - 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) } + before do + product = create(:product, properties: [property]) + enterprise.supplied_products << product + end - it "merges properties" do - allow(cached_enterprise_serializer) - .to receive(:product_properties) { [product_property] } - allow(cached_enterprise_serializer) - .to receive(:producer_properties) { [duplicate_product_property, producer_property] } - - merge = cached_enterprise_serializer.supplied_properties - expect(merge).to eq([product_property, producer_property]) - end + it "removes duplicate product and producer properties" do + merge = cached_enterprise_serializer.supplied_properties + expect(merge).to eq([property, different_property]) end end end From 4608c434b4384213d92ca5eafea17d5395d61d82 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 18:17:58 +0100 Subject: [PATCH 11/14] Eager load producer properties to avoid N+1 This totally removes the following N+1 query from the GET /shops request ```sql SELECT "spree_properties".* FROM "spree_properties" INNER JOIN "producer_properties" ON "spree_properties"."id" = "producer_properties"."property_id" WHERE "producer_properties"."producer_id" = 17 ORDER BY producer_properties.position; ``` The producer properties of the corresponding enterprises are now loaded in a single query fired from the controller. --- app/controllers/shops_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/shops_controller.rb b/app/controllers/shops_controller.rb index 1a0eacade4..228df01095 100644 --- a/app/controllers/shops_controller.rb +++ b/app/controllers/shops_controller.rb @@ -4,6 +4,10 @@ class ShopsController < BaseController before_filter :enable_embedded_shopfront def index - @enterprises = Enterprise.activated.includes(address: :state).all + @enterprises = Enterprise + .activated + .includes(address: :state) + .includes(:properties) + .all end end From c330d931ce16b185bcb3630cdc440204139b091b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 26 Nov 2018 20:01:01 +0100 Subject: [PATCH 12/14] Eager load product properties to avoid N+1 This totally removes the following N+1 query from the GET /shops request ```sql SELECT "spree_properties".* FROM "spree_properties" INNER JOIN "spree_product_properties" ON "spree_product_properties"."property_id" = "spree_properties"."id" INNER JOIN "spree_products" ON "spree_products"."id" = "spree_product_properties"."product_id" INNER JOIN "enterprises" ON "enterprises"."id" = "spree_products"."supplier_id" WHERE "spree_products"."supplier_id" = 24; ``` The product properties of the corresponding enterprises are now loaded in a single query fired from the controller. --- app/controllers/shops_controller.rb | 1 + app/serializers/api/cached_enterprise_serializer.rb | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/shops_controller.rb b/app/controllers/shops_controller.rb index 228df01095..8a657990d8 100644 --- a/app/controllers/shops_controller.rb +++ b/app/controllers/shops_controller.rb @@ -8,6 +8,7 @@ class ShopsController < BaseController .activated .includes(address: :state) .includes(:properties) + .includes(supplied_products: :properties) .all end end diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index a9bf15743d..47481f068f 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -75,16 +75,13 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer end def product_properties - Spree::Property - .joins(product_properties: { product: :supplier }) - .where(spree_products: { supplier_id: enterprise }) + enterprise.supplied_products.flat_map(&:properties) end def producer_properties enterprise.properties end - # This results in 3 queries per enterprise def supplied_properties (product_properties + producer_properties).uniq do |property_object| property_object.property.presentation From e0f43191e2a38b4fb2b852eeca9b1ce8a313e785 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 28 Nov 2018 13:52:35 +0100 Subject: [PATCH 13/14] Make helper methods private Otherwise next dev might think these are attributes of the resulting JSON serialized object. They just made the refactoring a bit easier. --- .../api/cached_enterprise_serializer.rb | 16 +++++------ .../api/cached_enterprise_serializer_spec.rb | 27 ++----------------- 2 files changed, 10 insertions(+), 33 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index 47481f068f..b13b7a27b3 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -74,14 +74,6 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer ids_to_objs options[:data].supplied_taxons[object.id] end - def product_properties - enterprise.supplied_products.flat_map(&:properties) - end - - def producer_properties - enterprise.properties - end - def supplied_properties (product_properties + producer_properties).uniq do |property_object| property_object.property.presentation @@ -149,6 +141,14 @@ class Api::CachedEnterpriseSerializer < ActiveModel::Serializer private + def product_properties + enterprise.supplied_products.flat_map(&:properties) + end + + def producer_properties + enterprise.properties + end + def enterprise object end diff --git a/spec/serializers/api/cached_enterprise_serializer_spec.rb b/spec/serializers/api/cached_enterprise_serializer_spec.rb index dbef516df5..bea4b79bca 100644 --- a/spec/serializers/api/cached_enterprise_serializer_spec.rb +++ b/spec/serializers/api/cached_enterprise_serializer_spec.rb @@ -4,29 +4,6 @@ describe Api::CachedEnterpriseSerializer do let(:cached_enterprise_serializer) { described_class.new(enterprise) } let(:enterprise) { create(:enterprise) } - describe '#product_properties' do - let(:property) { create(:property) } - - before do - product = create(:product, properties: [property]) - enterprise.supplied_products << product - end - - it 'returns the properties of the products supplied by the enterprise' do - expect(cached_enterprise_serializer.product_properties).to eq([property]) - end - end - - describe '#producer_properties' do - let(:property) { create(:property) } - - before { enterprise.properties << property } - - it 'returns the properties of the enterprise' do - expect(cached_enterprise_serializer.producer_properties).to eq([property]) - end - end - describe '#supplied_properties' do let(:property) { create(:property, presentation: 'One') } let(:duplicate_property) { create(:property, presentation: 'One') } @@ -42,8 +19,8 @@ describe Api::CachedEnterpriseSerializer do end it "removes duplicate product and producer properties" do - merge = cached_enterprise_serializer.supplied_properties - expect(merge).to eq([property, different_property]) + properties = cached_enterprise_serializer.supplied_properties + expect(properties).to eq([property, different_property]) end end end From 96202ab00dc25c5f0ef86ed34990e4c28232a10c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 28 Nov 2018 14:17:16 +0100 Subject: [PATCH 14/14] Fix Rubocop violations --- .../api/cached_enterprise_serializer.rb | 282 +++++++++--------- .../api/uncached_enterprise_serializer.rb | 18 +- 2 files changed, 154 insertions(+), 146 deletions(-) diff --git a/app/serializers/api/cached_enterprise_serializer.rb b/app/serializers/api/cached_enterprise_serializer.rb index b13b7a27b3..4070cfa7d5 100644 --- a/app/serializers/api/cached_enterprise_serializer.rb +++ b/app/serializers/api/cached_enterprise_serializer.rb @@ -1,155 +1,161 @@ require 'open_food_network/property_merge' -class Api::CachedEnterpriseSerializer < ActiveModel::Serializer - include SerializerHelper +module Api + class CachedEnterpriseSerializer < ActiveModel::Serializer + include SerializerHelper - cached + 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: 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 - (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) + def cache_key + object.andand.cache_key end - producer_properties = Spree::Property.where(id: producer_property_ids) + 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 - OpenFoodNetwork::PropertyMerge.merge product_properties, producer_properties - end + attributes :taxons, :supplied_taxons - def active - options[:data].active_distributors.andand.include? object - end + has_one :address, serializer: AddressSerializer - # 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 + has_many :supplied_properties, serializer: PropertySerializer + has_many :distributed_properties, serializer: PropertySerializer - # 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 + def pickup + services = data.shipping_method_services[object.id] + services ? services[:pickup] : false + 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 + def delivery + services = data.shipping_method_services[object.id] + services ? services[:delivery] : false + end - private + def email_address + object.email_address.to_s.reverse + end - def product_properties - enterprise.supplied_products.flat_map(&:properties) - end + def hash + object.to_param + end - def producer_properties - enterprise.properties - end + def logo + object.logo(:medium) if object.logo? + end - def enterprise - object + 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 diff --git a/app/serializers/api/uncached_enterprise_serializer.rb b/app/serializers/api/uncached_enterprise_serializer.rb index 010348da87..bad8808533 100644 --- a/app/serializers/api/uncached_enterprise_serializer.rb +++ b/app/serializers/api/uncached_enterprise_serializer.rb @@ -1,13 +1,15 @@ -class Api::UncachedEnterpriseSerializer < ActiveModel::Serializer - include SerializerHelper +module Api + class UncachedEnterpriseSerializer < ActiveModel::Serializer + include SerializerHelper - attributes :orders_close_at, :active + attributes :orders_close_at, :active - def orders_close_at - options[:data].earliest_closing_times[object.id] - end + def orders_close_at + options[:data].earliest_closing_times[object.id] + end - def active - options[:data].active_distributors.andand.include? object + def active + options[:data].active_distributors.andand.include? object + end end end