From 7cc2bc4fde83d2c4c059c19afee3242afc01e821 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 18 Jun 2015 13:11:11 +1000 Subject: [PATCH] Perform variant override scoping on product/variant by external class. Centralise this so we can load everything in one go. --- app/controllers/shop_controller.rb | 7 ++- .../admin/variants_controller_decorator.rb | 3 +- app/models/spree/inventory_unit_decorator.rb | 3 +- app/models/spree/order_populator_decorator.rb | 4 +- app/models/spree/product_decorator.rb | 4 -- app/models/spree/variant_decorator.rb | 3 - lib/open_food_network/scope_product_to_hub.rb | 23 +++++--- lib/open_food_network/scope_variant_to_hub.rb | 56 ++++++++++--------- .../controller_helpers/order_decorator.rb | 3 +- .../scope_variant_to_hub_spec.rb | 13 +++-- spec/models/spree/order_populator_spec.rb | 5 +- 11 files changed, 67 insertions(+), 57 deletions(-) diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index c9fe15504f..ef78605b41 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -46,10 +46,12 @@ class ShopController < BaseController def products_for_shop if current_order_cycle + scoper = OpenFoodNetwork::ScopeProductToHub.new(current_distributor) + current_order_cycle. valid_products_distributed_by(current_distributor). order(taxon_order). - each { |p| p.scope_to_hub current_distributor }. + each { |p| scoper.scope(p) }. select { |p| !p.deleted? && p.has_stock_for_distribution?(current_order_cycle, current_distributor) } end end @@ -69,9 +71,10 @@ class ShopController < BaseController # We use the in_stock? method here instead of the in_stock scope because we need to # look up the stock as overridden by VariantOverrides, and the scope method is not affected # by them. + scoper = OpenFoodNetwork::ScopeVariantToHub.new(current_distributor) Spree::Variant. for_distribution(current_order_cycle, current_distributor). - each { |v| v.scope_to_hub current_distributor }. + each { |v| scoper.scope(v) }. select(&:in_stock?) end diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index 2308d2a923..814cfb12f8 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -14,9 +14,10 @@ Spree::Admin::VariantsController.class_eval do if params[:distributor_id].present? distributor = Enterprise.find params[:distributor_id] @variants = @variants.in_distributor(distributor) + scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) # Perform scoping after all filtering is done. # Filtering could be a problem on scoped variants. - @variants.each { |v| v.scope_to_hub(distributor) } + @variants.each { |v| scoper.scope(v) } end end diff --git a/app/models/spree/inventory_unit_decorator.rb b/app/models/spree/inventory_unit_decorator.rb index 939a97996f..9868596b41 100644 --- a/app/models/spree/inventory_unit_decorator.rb +++ b/app/models/spree/inventory_unit_decorator.rb @@ -4,9 +4,10 @@ module Spree return [] unless order.completed? #increase inventory to meet initial requirements + scoper = OpenFoodNetwork::ScopeVariantToHub.new(order.distributor) order.line_items.each do |line_item| # Scope variant to hub so that stock levels may be subtracted from VariantOverride. - line_item.variant.scope_to_hub order.distributor + scoper.scope(line_item.variant) increase(order, line_item.variant, line_item.quantity) end diff --git a/app/models/spree/order_populator_decorator.rb b/app/models/spree/order_populator_decorator.rb index 3759866236..cd80d17b97 100644 --- a/app/models/spree/order_populator_decorator.rb +++ b/app/models/spree/order_populator_decorator.rb @@ -9,7 +9,7 @@ Spree::OrderPopulator.class_eval do errors.add(:base, "That distributor or order cycle can't supply all the products in your cart. Please choose another.") end - if valid? + if valid? @order.with_lock do @order.empty! if overwrite @@ -33,7 +33,7 @@ Spree::OrderPopulator.class_eval do def attempt_cart_add(variant_id, quantity, max_quantity = nil) quantity = quantity.to_i variant = Spree::Variant.find(variant_id) - variant.scope_to_hub @distributor + OpenFoodNetwork::ScopeVariantToHub.new(@distributor).scope(variant) if quantity > 0 if check_stock_levels(variant, quantity) && check_order_cycle_provided_for(variant) && diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index c789d81963..2cf13baf84 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -1,8 +1,4 @@ -require 'open_food_network/scope_product_to_hub' - Spree::Product.class_eval do - include OpenFoodNetwork::ProductScopableToHub - # We have an after_destroy callback on Spree::ProductOptionType. However, if we # don't specify dependent => destroy on this association, it is not called. See: # https://github.com/rails/rails/issues/7618 diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index a1a68afd9e..3ed87159a7 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -1,10 +1,7 @@ -require 'open_food_network/scope_variant_to_hub' require 'open_food_network/enterprise_fee_calculator' require 'open_food_network/option_value_namer' Spree::Variant.class_eval do - include OpenFoodNetwork::VariantScopableToHub - has_many :exchange_variants, dependent: :destroy has_many :exchanges, through: :exchange_variants has_many :variant_overrides diff --git a/lib/open_food_network/scope_product_to_hub.rb b/lib/open_food_network/scope_product_to_hub.rb index b214abc0d8..8c59feb3cc 100644 --- a/lib/open_food_network/scope_product_to_hub.rb +++ b/lib/open_food_network/scope_product_to_hub.rb @@ -1,16 +1,21 @@ require 'open_food_network/scope_variant_to_hub' module OpenFoodNetwork - module ScopeProductToHub - def variants_distributed_by(order_cycle, distributor) - super.each { |v| v.scope_to_hub @hub } - end - end - - module ProductScopableToHub - def scope_to_hub(hub) - extend OpenFoodNetwork::ScopeProductToHub + class ScopeProductToHub + def initialize(hub) @hub = hub end + + def scope(product) + product.send :extend, OpenFoodNetwork::ScopeProductToHub::ScopeProductToHub + product.instance_variable_set :@hub, @hub + end + + + module ScopeProductToHub + def variants_distributed_by(order_cycle, distributor) + super.each { |v| ScopeVariantToHub.new(@hub).scope(v) } + end + end end end diff --git a/lib/open_food_network/scope_variant_to_hub.rb b/lib/open_food_network/scope_variant_to_hub.rb index 91ed634043..4e6e454368 100644 --- a/lib/open_food_network/scope_variant_to_hub.rb +++ b/lib/open_food_network/scope_variant_to_hub.rb @@ -1,30 +1,36 @@ module OpenFoodNetwork - module ScopeVariantToHub - def price - VariantOverride.price_for(@hub, self) || super - end - - def price_in(currency) - Spree::Price.new(amount: price, currency: currency) - end - - def count_on_hand - VariantOverride.count_on_hand_for(@hub, self) || super - end - - def decrement!(attribute, by=1) - if attribute == :count_on_hand && VariantOverride.stock_overridden?(@hub, self) - VariantOverride.decrement_stock! @hub, self, by - else - super - end - end - end - - module VariantScopableToHub - def scope_to_hub(hub) - extend OpenFoodNetwork::ScopeVariantToHub + class ScopeVariantToHub + def initialize(hub) @hub = hub end + + def scope(variant) + variant.send :extend, OpenFoodNetwork::ScopeVariantToHub::ScopeVariantToHub + variant.instance_variable_set :@hub, @hub + end + + + module ScopeVariantToHub + def price + VariantOverride.price_for(@hub, self) || super + end + + def price_in(currency) + Spree::Price.new(amount: price, currency: currency) + end + + def count_on_hand + VariantOverride.count_on_hand_for(@hub, self) || super + end + + def decrement!(attribute, by=1) + if attribute == :count_on_hand && VariantOverride.stock_overridden?(@hub, self) + VariantOverride.decrement_stock! @hub, self, by + else + super + end + end + end + end end diff --git a/lib/spree/core/controller_helpers/order_decorator.rb b/lib/spree/core/controller_helpers/order_decorator.rb index 88ae412ec2..dc38e861e6 100644 --- a/lib/spree/core/controller_helpers/order_decorator.rb +++ b/lib/spree/core/controller_helpers/order_decorator.rb @@ -3,8 +3,9 @@ Spree::Core::ControllerHelpers::Order.class_eval do order = current_order_without_scoped_variants(create_order_if_necessary) if order + scoper = OpenFoodNetwork::ScopeVariantToHub.new(order.distributor) order.line_items.each do |li| - li.variant.scope_to_hub order.distributor + scoper.scope(li.variant) end end diff --git a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb index 429bfbe082..dbdf5c074e 100644 --- a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb +++ b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb @@ -5,16 +5,17 @@ module OpenFoodNetwork let(:hub) { create(:distributor_enterprise) } let(:v) { create(:variant, price: 11.11, count_on_hand: 1) } let(:vo) { create(:variant_override, hub: hub, variant: v, price: 22.22, count_on_hand: 2) } + let(:scoper) { ScopeVariantToHub.new(hub) } describe "overriding price" do it "returns the overridden price when one is present" do vo - v.scope_to_hub hub + scoper.scope v v.price.should == 22.22 end it "returns the variant's price otherwise" do - v.scope_to_hub hub + scoper.scope v v.price.should == 11.11 end end @@ -22,12 +23,12 @@ module OpenFoodNetwork describe "overriding price_in" do it "returns the overridden price when one is present" do vo - v.scope_to_hub hub + scoper.scope v v.price_in('AUD').amount.should == 22.22 end it "returns the variant's price otherwise" do - v.scope_to_hub hub + scoper.scope v v.price_in('AUD').amount.should == 11.11 end end @@ -35,12 +36,12 @@ module OpenFoodNetwork describe "overriding stock levels" do it "returns the overridden stock level when one is present" do vo - v.scope_to_hub hub + scoper.scope v v.count_on_hand.should == 2 end it "returns the variant's stock level otherwise" do - v.scope_to_hub hub + scoper.scope v v.count_on_hand.should == 1 end end diff --git a/spec/models/spree/order_populator_spec.rb b/spec/models/spree/order_populator_spec.rb index 5602e46da5..c4ee8e44b6 100644 --- a/spec/models/spree/order_populator_spec.rb +++ b/spec/models/spree/order_populator_spec.rb @@ -21,7 +21,7 @@ module Spree op.populate(params).should be_false op.errors.to_a.should == ["That distributor or order cycle can't supply all the products in your cart. Please choose another."] end - + it "empties the order if override is true" do op.stub(:distribution_can_supply_products_in_cart).and_return true order.stub(:with_lock).and_yield @@ -38,7 +38,7 @@ module Spree it "attempts cart add with max_quantity" do op.stub(:distribution_can_supply_products_in_cart).and_return true order.should_receive(:empty!) - params = {variants: {"1" => {quantity: 1, max_quantity: 2}}} + params = {variants: {"1" => {quantity: 1, max_quantity: 2}}} order.stub(:with_lock).and_yield op.should_receive(:attempt_cart_add).with("1", 1, 2).and_return true op.populate(params, true) @@ -48,7 +48,6 @@ module Spree describe "attempt_cart_add" do it "performs additional validations" do variant = double(:variant) - variant.stub(:scope_to_hub) quantity = 123 Spree::Variant.stub(:find).and_return(variant)