From 20abaaa950cd3afb8211b19d8661d286f47cd8db Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 13 Jan 2021 14:28:30 +0100 Subject: [PATCH] Reuse outstanding balance statement across queries Instead of relying on Spree::Order#outstanding_balance we make us of the result set `balance_value` computed column. So, we ask PostgreSQL to compute it instead of Ruby and then serialize it from that computed column. That's a bit faster to compute that way and let's reuse logic. We hide this new implementation under this features' toggle so it's only used when enabled. We want hit the old behaviour by default. --- app/controllers/spree/users_controller.rb | 3 +- app/models/spree/order.rb | 4 +-- app/queries/outstanding_balance.rb | 17 +++++++++-- app/serializers/api/order_serializer.rb | 8 +++++ app/services/customers_with_balance.rb | 30 ++----------------- spec/serializers/api/order_serializer_spec.rb | 26 ++++++++++++++++ 6 files changed, 56 insertions(+), 32 deletions(-) diff --git a/app/controllers/spree/users_controller.rb b/app/controllers/spree/users_controller.rb index a4029d33cb..7d46be6748 100644 --- a/app/controllers/spree/users_controller.rb +++ b/app/controllers/spree/users_controller.rb @@ -17,9 +17,10 @@ module Spree @orders = @user.orders .where.not(Spree::Order.in_incomplete_state.where_values_hash) .select('spree_orders.*') - .select(OutstandingBalance.new.query) .order('completed_at desc') + @orders = OutstandingBalance.new(@orders).query + customers = spree_current_user.customers @shops = Enterprise .where(id: @orders.pluck(:distributor_id).uniq | customers.pluck(:enterprise_id)) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index ce279ec58b..a908c56e1e 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -132,8 +132,8 @@ module Spree # All the states an order can be in before completing the checkout PRIOR_TO_COMPLETION_STATES = %w(cart address delivery payment).freeze - # All the states of a complete order but that shouldn't count towards the balance. Those that the - # customer won't enjoy. + # All the states of a complete order but that shouldn't count towards the balance. Those that + # the customer won't enjoy. NON_FULFILLED_STATES = %w(canceled returned).freeze scope :in_incomplete_state, -> { where(state: PRIOR_TO_COMPLETION_STATES) } diff --git a/app/queries/outstanding_balance.rb b/app/queries/outstanding_balance.rb index cc9ba7ab58..a31bb1b2df 100644 --- a/app/queries/outstanding_balance.rb +++ b/app/queries/outstanding_balance.rb @@ -9,17 +9,30 @@ # # See CompleteOrdersWithBalance or CustomersWithBalance as examples. class OutstandingBalance + # The relation must be an ActiveRecord::Relation object with `spree_orders` in the SQL statement + # FROM for #statement to work. + def initialize(relation = nil) + @relation = relation + end + def query + relation.select("#{statement} AS balance_value") + end + + # Arel doesn't support CASE statements until v7.1.0 so we'll have to wait with SQL literals + # a little longer. See https://github.com/rails/arel/pull/400 for details. + def statement <<-SQL.strip_heredoc CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total - WHEN state IS NOT NULL THEN payment_total - total + WHEN state IS NOT NULL THEN payment_total - total ELSE 0 END - AS balance_value SQL end private + attr_reader :relation + def non_fulfilled_states_group states = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) } Arel::Nodes::Grouping.new(states) diff --git a/app/serializers/api/order_serializer.rb b/app/serializers/api/order_serializer.rb index 427c222ea2..73532a1704 100644 --- a/app/serializers/api/order_serializer.rb +++ b/app/serializers/api/order_serializer.rb @@ -7,6 +7,14 @@ module Api has_many :payments, serializer: Api::PaymentSerializer + def outstanding_balance + if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, object.user) + -object.balance_value + else + object.outstanding_balance + end + end + def payments object.payments.joins(:payment_method).completed end diff --git a/app/services/customers_with_balance.rb b/app/services/customers_with_balance.rb index 5548fa4531..bf6cf70396 100644 --- a/app/services/customers_with_balance.rb +++ b/app/services/customers_with_balance.rb @@ -10,23 +10,15 @@ class CustomersWithBalance joins(left_join_complete_orders). group("customers.id"). select("customers.*"). - select(outstanding_balance) + select(outstanding_balance_sum) end private attr_reader :enterprise - # Arel doesn't support CASE statements until v7.1.0 so we'll have to wait with SQL literals - # a little longer. See https://github.com/rails/arel/pull/400 for details. - def outstanding_balance - <<-SQL.strip_heredoc - SUM( - CASE WHEN state IN #{non_fulfilled_states_group.to_sql} THEN payment_total - WHEN state IS NOT NULL THEN payment_total - total - ELSE 0 END - ) AS balance_value - SQL + def outstanding_balance_sum + "SUM(#{OutstandingBalance.new.statement}) AS balance_value" end # The resulting orders are in states that belong after the checkout. Only these can be considered @@ -42,20 +34,4 @@ class CustomersWithBalance states = Spree::Order::PRIOR_TO_COMPLETION_STATES.map { |state| Arel::Nodes.build_quoted(state) } Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states) end - - def non_fulfilled_states_group - states_group = Spree::Order::NON_FULFILLED_STATES.map { |state| Arel::Nodes.build_quoted(state) } - Arel::Nodes::Grouping.new(states_group) - end - - # All the states an order can be in before completing the checkout - def prior_to_completion_states - %w(cart address delivery payment) - end - - # All the states of a complete order but that shouldn't count towards the balance. Those that the - # customer won't enjoy. - def non_fulfilled_states - %w(canceled returned) - end end diff --git a/spec/serializers/api/order_serializer_spec.rb b/spec/serializers/api/order_serializer_spec.rb index 04ef728d59..9b32b754e4 100644 --- a/spec/serializers/api/order_serializer_spec.rb +++ b/spec/serializers/api/order_serializer_spec.rb @@ -23,4 +23,30 @@ describe Api::OrderSerializer do expect(serializer.to_json).to match completed_payment.amount.to_s expect(serializer.to_json).to_not match payment.amount.to_s end + + describe '#outstanding_balance' do + context 'when the customer_balance feature is enabled' do + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, order.user) { true } + + allow(order).to receive(:balance_value).and_return(-1.23) + end + + it "returns the object's balance_value from the users perspective" do + expect(serializer.serializable_hash[:outstanding_balance]).to eq(1.23) + end + end + + context 'when the customer_balance is not enabled' do + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, order.user) { false } + end + + it 'calls #outstanding_balance on the object' do + expect(serializer.serializable_hash[:outstanding_balance]).to eq(1.0) + end + end + end end