diff --git a/app/models/concerns/adjustment_scopes.rb b/app/models/concerns/adjustment_scopes.rb new file mode 100644 index 0000000000..927e9e8c1c --- /dev/null +++ b/app/models/concerns/adjustment_scopes.rb @@ -0,0 +1,19 @@ +module AdjustmentScopes + extend ActiveSupport::Concern + + PAYMENT_FEE_SCOPE = { originator_type: 'Spree::PaymentMethod' }.freeze + SHIPPING_SCOPE = { originator_type: 'Spree::ShippingMethod' }.freeze + ELIGIBLE_SCOPE = { eligible: true }.freeze + + def payment_fee_scope + PAYMENT_FEE_SCOPE + end + + def shipping_scope + SHIPPING_SCOPE + end + + def eligible_scope + ELIGIBLE_SCOPE + end +end diff --git a/app/models/spree/adjustment_decorator.rb b/app/models/spree/adjustment_decorator.rb index fbce22843c..2e1dfcebf7 100644 --- a/app/models/spree/adjustment_decorator.rb +++ b/app/models/spree/adjustment_decorator.rb @@ -1,4 +1,5 @@ require 'spree/localized_number' +require 'concerns/adjustment_scopes' module Spree Adjustment.class_eval do @@ -19,7 +20,9 @@ module Spree scope :with_tax, -> { where('spree_adjustments.included_tax > 0') } scope :without_tax, -> { where('spree_adjustments.included_tax = 0') } - scope :payment_fee, -> { where(originator_type: 'Spree::PaymentMethod') } + scope :payment_fee, -> { where(AdjustmentScopes::PAYMENT_FEE_SCOPE) } + scope :shipping, -> { where(AdjustmentScopes::SHIPPING_SCOPE) } + scope :eligible, -> { where(AdjustmentScopes::ELIGIBLE_SCOPE) } attr_accessible :included_tax diff --git a/app/models/spree/line_item_decorator.rb b/app/models/spree/line_item_decorator.rb index ee36d5d415..5581993dff 100644 --- a/app/models/spree/line_item_decorator.rb +++ b/app/models/spree/line_item_decorator.rb @@ -102,9 +102,11 @@ Spree::LineItem.class_eval do def price_with_adjustments # EnterpriseFee#create_adjustment applies adjustments on line items to their parent order, # so line_item.adjustments returns an empty array - return 0 if quantity == 0 + return 0 if quantity.zero? - (price + order.adjustments.where(source_id: id).sum(&:amount) / quantity).round(2) + line_item_adjustments = OrderAdjustmentsFetcher.new(order).line_item_adjustments(self) + + (price + line_item_adjustments.sum(&:amount) / quantity).round(2) end def single_display_amount_with_adjustments diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 9ae51b2304..28d487d6fd 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -10,6 +10,8 @@ end Spree::Order.class_eval do prepend OrderShipment + delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher + belongs_to :order_cycle belongs_to :distributor, class_name: 'Enterprise' belongs_to :customer @@ -263,14 +265,6 @@ Spree::Order.class_eval do order_cycle.items_bought_by_user(user, distributor) end - def admin_and_handling_total - adjustments.eligible.where("originator_type = ? AND source_type != ?", 'EnterpriseFee', 'Spree::LineItem').sum(&:amount) - end - - def payment_fee - adjustments.payment_fee.map(&:amount).sum - end - # Does this order have shipments that can be shipped? def ready_to_ship? shipments.any?(&:can_ship?) @@ -355,6 +349,10 @@ Spree::Order.class_eval do private + def adjustments_fetcher + @adjustments_fetcher ||= OrderAdjustmentsFetcher.new(self) + end + def skip_payment_for_subscription? subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now end diff --git a/app/services/order_adjustments_fetcher.rb b/app/services/order_adjustments_fetcher.rb new file mode 100644 index 0000000000..dfde72f142 --- /dev/null +++ b/app/services/order_adjustments_fetcher.rb @@ -0,0 +1,84 @@ +# This class allows orders with eager-loaded adjustment objects to calculate various adjustment +# types without triggering additional queries. +# +# For example; `order.adjustments.shipping.sum(&:amount)` would normally trigger a new query +# regardless of whether or not adjustments have been preloaded, as `#shipping` is an adjustment +# scope, eg; `scope :shipping, where(originator_type: 'Spree::ShippingMethod')`. +# +# Here the adjustment scopes are moved to a shared module, and `adjustments.loaded?` is used to +# check if the objects have already been fetched and initialized. If they have, `order.adjustments` +# will be an Array, and we can select the required objects without hitting the database. If not, it +# will fetch the adjustments via their scopes as normal. + +class OrderAdjustmentsFetcher + include AdjustmentScopes + + def initialize(order) + @order = order + end + + def admin_and_handling_total + admin_and_handling_fees.map(&:amount).sum + end + + def payment_fee + sum_adjustments "payment_fee" + end + + def ship_total + sum_adjustments "shipping" + end + + def line_item_adjustments(line_item) + if adjustments_eager_loaded? + adjustments.select{ |adjustment| adjustment.source_id == line_item.id } + else + adjustments.where(source_id: line_item.id) + end + end + + private + + attr_reader :order + + def adjustments + order.adjustments + end + + def adjustments_eager_loaded? + adjustments.loaded? + end + + def sum_adjustments(scope) + collect_adjustments(scope).map(&:amount).sum + end + + def collect_adjustments(scope) + if adjustments_eager_loaded? + adjustment_scope = public_send("#{scope}_scope") + + adjustments.select do |adjustment| + match_by_scope(adjustment, adjustment_scope) + end + else + adjustments.scoped.public_send scope + end + end + + def admin_and_handling_fees + if adjustments_eager_loaded? + adjustments.select do |adjustment| + match_by_scope(adjustment, eligible_scope) && + adjustment.originator_type == 'EnterpriseFee' && + adjustment.source_type != 'Spree::LineItem' + end + else + adjustments.eligible. + where("originator_type = ? AND source_type != ?", 'EnterpriseFee', 'Spree::LineItem') + end + end + + def match_by_scope(adjustment, scope) + adjustment.public_send(scope.keys.first) == scope.values.first + end +end diff --git a/lib/open_food_network/orders_and_fulfillments_report.rb b/lib/open_food_network/orders_and_fulfillments_report.rb index 6bb607af3d..f930ba50c6 100644 --- a/lib/open_food_network/orders_and_fulfillments_report.rb +++ b/lib/open_food_network/orders_and_fulfillments_report.rb @@ -1,5 +1,4 @@ require "open_food_network/reports/line_items" -require "open_food_network/reports/helper" require "open_food_network/orders_and_fulfillments_report/supplier_totals_report" require "open_food_network/orders_and_fulfillments_report/supplier_totals_by_distributor_report" require "open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report" diff --git a/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb b/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb index cb73eb4bc2..74456d174f 100644 --- a/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb +++ b/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb @@ -2,8 +2,6 @@ module OpenFoodNetwork class OrdersAndFulfillmentsReport class CustomerTotalsReport - include OpenFoodNetwork::Reports::Helper - REPORT_TYPE = "order_cycle_customer_totals".freeze attr_reader :context @@ -63,10 +61,10 @@ module OpenFoodNetwork proc { |_line_items| "" }, proc { |line_items| line_items.sum(&:amount) }, - proc { |line_items| amount_with_adjustments(line_items) }, - proc { |line_items| admin_and_handling_total(line_items.first.order) }, - proc { |line_items| ship_total(line_items.first.order) }, - proc { |line_items| payment_fee(line_items.first.order) }, + proc { |line_items| line_items.sum(&:amount_with_adjustments) }, + proc { |line_items| line_items.first.order.admin_and_handling_total }, + proc { |line_items| line_items.first.order.ship_total }, + proc { |line_items| line_items.first.order.payment_fee }, proc { |line_items| line_items.first.order.total }, proc { |line_items| line_items.first.order.paid? ? I18n.t(:yes) : I18n.t(:no) }, @@ -132,7 +130,7 @@ module OpenFoodNetwork proc { |line_items| line_items.sum(&:quantity) }, proc { |line_items| line_items.sum(&:amount) }, - proc { |line_items| amount_with_adjustments(line_items) }, + proc { |line_items| line_items.sum(&:amount_with_adjustments) }, proc { |_line_items| "" }, proc { |_line_items| "" }, proc { |_line_items| "" }, @@ -196,6 +194,13 @@ module OpenFoodNetwork order: [:bill_address, :ship_address, :order_cycle, :adjustments, :payments, :user, :distributor, shipments: { shipping_rates: :shipping_method }] }] end + + private + + def shipping_method(line_items) + line_items.first.order.shipments.first. + andand.shipping_rates.andand.first.andand.shipping_method + end end end end diff --git a/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report.rb b/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report.rb index 28c81103dc..4f7cdf6f72 100644 --- a/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report.rb +++ b/lib/open_food_network/orders_and_fulfillments_report/distributor_totals_by_supplier_report.rb @@ -1,8 +1,6 @@ module OpenFoodNetwork class OrdersAndFulfillmentsReport class DistributorTotalsBySupplierReport - include OpenFoodNetwork::Reports::Helper - REPORT_TYPE = "order_cycle_distributor_totals_by_supplier".freeze attr_reader :context @@ -34,7 +32,7 @@ module OpenFoodNetwork proc { |_line_items| "" }, proc { |_line_items| "" }, proc { |line_items| line_items.sum(&:amount) }, - proc { |line_items| ship_total(line_items.first.order) }, + proc { |line_items| line_items.map(&:order).uniq.sum(&:ship_total) }, proc { |_line_items| "" } ] }, diff --git a/lib/open_food_network/reports/helper.rb b/lib/open_food_network/reports/helper.rb deleted file mode 100644 index 620b1d6fe0..0000000000 --- a/lib/open_food_network/reports/helper.rb +++ /dev/null @@ -1,42 +0,0 @@ -module OpenFoodNetwork - module Reports - module Helper - def admin_and_handling_total(order) - order.adjustments.select do |a| - a.eligible && a.originator_type == 'EnterpriseFee' && a.source_type != 'Spree::LineItem' - end.map(&:amount).sum.to_f - end - - def ship_total(order) - order.adjustments.select{ |a| a.originator_type == 'Spree::ShippingMethod' }. - map(&:amount).sum.to_f - end - - def payment_fee(order) - order.adjustments.select{ |a| a.originator_type == 'Spree::PaymentMethod' }. - map(&:amount).sum.to_f - end - - def amount_with_adjustments(line_items) - line_items.map do |line_item| - price_with_adjustments(line_item) * line_item.quantity - end.sum - end - - def price_with_adjustments(line_item) - return 0 if line_item.quantity.zero? - (line_item.price + line_item_adjustments(line_item).map(&:amount).sum / line_item.quantity). - round(2) - end - - def line_item_adjustments(line_item) - line_item.order.adjustments.select{ |a| a.source_id == line_item.id } - end - - def shipping_method(line_items) - line_items.first.order.shipments.first. - andand.shipping_rates.andand.first.andand.shipping_method - end - end - end -end diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index d7c89ceb30..e39b7e9d43 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -285,6 +285,8 @@ module Spree li = LineItem.new allow(li).to receive(:price) { 55.55 } + allow(li).to receive_message_chain(:order, :adjustments, :loaded?) + allow(li).to receive_message_chain(:order, :adjustments, :select) allow(li).to receive_message_chain(:order, :adjustments, :where, :sum) { 11.11 } allow(li).to receive(:quantity) { 2 } expect(li.price_with_adjustments).to eq(61.11) @@ -296,6 +298,8 @@ module Spree li = LineItem.new allow(li).to receive(:price) { 55.55 } + allow(li).to receive_message_chain(:order, :adjustments, :loaded?) + allow(li).to receive_message_chain(:order, :adjustments, :select) allow(li).to receive_message_chain(:order, :adjustments, :where, :sum) { 11.11 } allow(li).to receive(:quantity) { 2 } expect(li.amount_with_adjustments).to eq(122.22)