Make #outstanding_balance return an OrderBalance

This will let us branch by abstraction. All existing calls to
`#outstanding_balance` will go through `OrderBalance` hence, will
check the feature toggle.

Note that by default, `OrderBalance` will end up calling
`#old_outstanding_balance`. As the name states, that's exactly what
`#outstanding_balance` was so far. This means no consumers will see any
change in behavior. We just added on item in the call stack (sort of).
This commit is contained in:
Pau Perez
2021-03-15 17:50:44 +01:00
parent 7e176298ef
commit 5815d1a4a4
13 changed files with 196 additions and 81 deletions

View File

@@ -6,6 +6,6 @@ module OrderHelper
end
def outstanding_balance_label(order)
order.outstanding_balance.negative? ? t(:credit_owed) : t(:balance_due)
order.outstanding_balance.label
end
end

View File

@@ -7,13 +7,28 @@ require 'active_support/concern'
module Balance
FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze
# Branches by the OrderBalance abstraction
def outstanding_balance
OrderBalance.new(self)
end
# This method is the one we're gradually replacing with `#new_outstanding_balance`. Having them
# separate enables us to choose which implementation we want depending on the context using
# a feature toggle. This avoids inconsistent behavior during that incremental refactoring.
#
# It is meant to be removed as soon as we get product approval that the new implementation has
# been working correctly in production.
def old_outstanding_balance
total - payment_total
end
# Returns the order balance by considering the total as money owed to the order distributor aka.
# the shop, and as a positive balance of said enterprise. If the customer pays it all, they
# the shop, and as a positive balance of said enterprise. If the customer pays it all, the
# distributor and customer are even.
#
# Note however, this is meant to be used only in the context of a single order object. When
# working with a collection of orders, such an index controller action, please consider using
# `app/queries/oustanding_balance.rb` instead so we avoid potential N+1s.
# `app/queries/outstanding_balance.rb` instead so we avoid potential N+1s.
def new_outstanding_balance
if state.in?(FINALIZED_NON_SUCCESSFUL_STATES)
-payment_total
@@ -22,22 +37,11 @@ module Balance
end
end
# This method is the one we're gradually replacing with `#new_outstanding_balance`. Having them
# separate enables us to choose which implementation we want depending on the context using
# a feature toggle. This avoids incosistent behavior across the app during that incremental
# refactoring.
#
# It is meant to be removed as soon as we get product approval that the new implementation has
# been working correctly in production.
def outstanding_balance
total - payment_total
end
def outstanding_balance?
!outstanding_balance.zero?
end
def display_outstanding_balance
Spree::Money.new(outstanding_balance, currency: currency)
outstanding_balance.amount
end
end

View File

@@ -1,6 +1,8 @@
# frozen_string_literal: true
class OrderBalance
delegate :zero?, :abs, :to_s, to: :to_f
def initialize(order)
@order = order
end
@@ -17,12 +19,10 @@ class OrderBalance
if customer_balance_enabled?
order.new_outstanding_balance
else
order.outstanding_balance
order.old_outstanding_balance
end
end
delegate :zero?, to: :to_f
private
attr_reader :order

View File

@@ -9,7 +9,7 @@ module OrderManagement
let(:payment_setup) { OrderManagement::Subscriptions::PaymentSetup.new(order) }
describe "#call!" do
let!(:payment){ create(:payment, amount: 10) }
let!(:payment) { create(:payment, amount: 10) }
context "when no pending payments are present" do
let(:payment_method) { create(:payment_method) }
@@ -17,7 +17,7 @@ module OrderManagement
before do
allow(order).to receive(:pending_payments).once { [] }
allow(order).to receive(:outstanding_balance) { 5 }
allow(order).to receive(:old_outstanding_balance) { 5 }
allow(order).to receive(:subscription) { subscription }
end
@@ -31,14 +31,14 @@ module OrderManagement
before { allow(order).to receive(:pending_payments).once { [payment] } }
context "when the payment total doesn't match the outstanding balance on the order" do
before { allow(order).to receive(:outstanding_balance) { 5 } }
before { allow(order).to receive(:old_outstanding_balance) { 5 } }
it "updates the payment total to reflect the outstanding balance" do
expect{ payment_setup.call! }.to change(payment, :amount).from(10).to(5)
end
end
context "when the payment total matches the outstanding balance on the order" do
before { allow(order).to receive(:outstanding_balance) { 10 } }
before { allow(order).to receive(:old_outstanding_balance) { 10 } }
it "does nothing" do
expect{ payment_setup.call! }.to_not change(payment, :amount).from(10)
@@ -51,7 +51,7 @@ module OrderManagement
let!(:payment2) { create(:payment, order: order) }
before do
allow(order).to receive(:outstanding_balance) { 7 }
allow(order).to receive(:old_outstanding_balance) { 7 }
allow(order).to receive(:pending_payments).once { [payment1, payment2] }
end

View File

@@ -100,7 +100,7 @@ module OpenFoodNetwork
proc { |orders| orders.first.distributor.name },
proc { |orders| orders.to_a.sum(&:item_total) },
proc { |orders| orders.sum(&:ship_total) },
proc { |orders| orders.sum(&:outstanding_balance) },
proc { |orders| orders.sum { |order| order.outstanding_balance.to_f } },
proc { |orders| orders.map(&:total).sum }]
when "payment_totals"
[proc { |orders| orders.first.payment_state },
@@ -124,7 +124,7 @@ module OpenFoodNetwork
}.sum(&:amount)
}
},
proc { |orders| orders.sum(&:outstanding_balance) }]
proc { |orders| orders.sum { |order| order.outstanding_balance.to_f } }]
else
[proc { |payments| payments.first.order.payment_state },
proc { |payments| payments.first.order.distributor.name },

View File

@@ -45,12 +45,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "voids the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'void'
order.reload
expect(order.payment_total).to eq 0
expect(order.outstanding_balance).to_not eq 0
expect(order.outstanding_balance.to_f).to_not eq 0
end
end
@@ -64,12 +64,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "does not void the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
expect(flash[:error]).to eq "Bup-bow!"
end
end
@@ -104,12 +104,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "partially refunds the payment" do
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to eq order.total
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
end
end
@@ -123,12 +123,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "does not void the payment" do
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
expect(flash[:error]).to eq "Bup-bow!"
end
end
@@ -169,12 +169,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "voids the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'void'
order.reload
expect(order.payment_total).to eq 0
expect(order.outstanding_balance).to_not eq 0
expect(order.outstanding_balance.to_f).to_not eq 0
end
end
@@ -189,12 +189,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "does not void the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
expect(flash[:error]).to eq "Bup-bow!"
end
end
@@ -211,12 +211,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "can still void the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'void'
order.reload
expect(order.payment_total).to eq 0
expect(order.outstanding_balance).to_not eq 0
expect(order.outstanding_balance.to_f).to_not eq 0
end
end
end
@@ -252,12 +252,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "partially refunds the payment" do
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to eq order.total
expect(order.outstanding_balance).to eq 0
expect(order.outstanding_balance.to_f).to eq 0
end
end
@@ -271,12 +271,12 @@ describe Spree::Admin::PaymentsController, type: :controller do
it "does not void the payment" do
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
spree_put :fire, params
expect(payment.reload.state).to eq 'completed'
order.reload
expect(order.payment_total).to eq order.total + 5
expect(order.outstanding_balance).to eq(-5)
expect(order.outstanding_balance.to_f).to eq(-5)
expect(flash[:error]).to eq "Bup-bow!"
end
end

View File

@@ -10,10 +10,13 @@ feature '
let(:order) { create(:completed_order_with_fees) }
scenario "visiting the payment form" do
login_as_admin_and_visit spree.new_admin_order_payment_path order
describe "payments/new" do
it "displays the order balance as the default payment amount" do
login_as_admin_and_visit spree.new_admin_order_payment_path order
expect(page).to have_content "New Payment"
expect(page).to have_content I18n.t(:new_payment)
expect(page).to have_field(:payment_amount, with: order.outstanding_balance.to_f)
end
end
context "with sensitive payment fee" do
@@ -26,10 +29,10 @@ feature '
payment_method.save!
end
scenario "visiting the payment form" do
it "renders the new payment page" do
login_as_admin_and_visit spree.new_admin_order_payment_path order
expect(page).to have_content "New Payment"
expect(page).to have_content I18n.t(:new_payment)
end
end
end

View File

@@ -15,7 +15,7 @@ describe Spree::OrderMailer do
end
context 'when the order has outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 123 } }
before { allow(order).to receive(:old_outstanding_balance) { 123 } }
it 'renders the amount as money' do
expect(email.body).to include('$123')
@@ -23,7 +23,7 @@ describe Spree::OrderMailer do
end
context 'when the order has no outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 0 } }
before { allow(order).to receive(:old_outstanding_balance) { 0 } }
it 'displays the payment status' do
expect(email.body).to include(I18n.t(:email_payment_not_paid))
@@ -58,7 +58,7 @@ describe Spree::OrderMailer do
end
context 'when the order has outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 123 } }
before { allow(order).to receive(:old_outstanding_balance) { 123 } }
it 'renders the amount as money' do
expect(email.body).to include('$123')
@@ -66,7 +66,7 @@ describe Spree::OrderMailer do
end
context 'when the order has no outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 0 } }
before { allow(order).to receive(:old_outstanding_balance) { 0 } }
it 'displays the payment status' do
expect(email.body).to include(I18n.t(:email_payment_not_paid))

View File

@@ -88,7 +88,7 @@ describe SubscriptionMailer, type: :mailer do
end
context 'when the order has outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 123 } }
before { allow(order).to receive(:old_outstanding_balance) { 123 } }
it 'renders the amount as money' do
expect(email.body).to include('$123')
@@ -96,7 +96,7 @@ describe SubscriptionMailer, type: :mailer do
end
context 'when the order has no outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 0 } }
before { allow(order).to receive(:old_outstanding_balance) { 0 } }
it 'displays the payment status' do
expect(email.body).to include(I18n.t(:email_payment_not_paid))
@@ -141,7 +141,7 @@ describe SubscriptionMailer, type: :mailer do
end
context 'when the order has outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 123 } }
before { allow(order).to receive(:old_outstanding_balance) { 123 } }
it 'renders the amount as money' do
expect(email.body).to include('$123')
@@ -149,7 +149,7 @@ describe SubscriptionMailer, type: :mailer do
end
context 'when the order has no outstanding balance' do
before { allow(order).to receive(:outstanding_balance) { 0 } }
before { allow(order).to receive(:old_outstanding_balance) { 0 } }
it 'displays the payment status' do
expect(email.body).to include(I18n.t(:email_payment_not_paid))

View File

@@ -133,12 +133,12 @@ describe Balance do
end
end
context "#outstanding_balance" do
context "#old_outstanding_balance" do
context 'when orders are in cart state' do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'cart') }
it 'returns the order balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -146,7 +146,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'address') }
it 'returns the order balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -154,7 +154,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'delivery') }
it 'returns the order balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -162,7 +162,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') }
it 'returns the order balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -170,7 +170,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -178,7 +178,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -186,7 +186,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'canceled') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -194,7 +194,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'resumed') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -202,7 +202,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -210,7 +210,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'awaiting_return') }
it 'returns the customer balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -218,7 +218,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'returned') }
it 'returns the balance' do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -226,7 +226,7 @@ describe Balance do
let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') }
it "returns positive" do
expect(order.outstanding_balance).to eq(100 - 10)
expect(order.old_outstanding_balance).to eq(100 - 10)
end
end
@@ -234,7 +234,7 @@ describe Balance do
let(:order) { create(:order, total: 8.20, payment_total: 10.20, state: 'complete') }
it "returns negative amount" do
expect(order.outstanding_balance).to eq(-2.00)
expect(order.old_outstanding_balance).to eq(-2.00)
end
end
end

View File

@@ -16,7 +16,7 @@ describe OrderBalance do
context 'when the balance is postive' do
before do
allow(order).to receive(:outstanding_balance) { 10 }
allow(order).to receive(:old_outstanding_balance) { 10 }
end
it "returns 'balance due'" do
@@ -26,7 +26,7 @@ describe OrderBalance do
context 'when the balance is negative' do
before do
allow(order).to receive(:outstanding_balance) { -10 }
allow(order).to receive(:old_outstanding_balance) { -10 }
end
it "returns 'credit owed'" do
@@ -36,7 +36,7 @@ describe OrderBalance do
context 'when the balance is zero' do
before do
allow(order).to receive(:outstanding_balance) { 0 }
allow(order).to receive(:old_outstanding_balance) { 0 }
end
it "returns 'balance due'" do
@@ -86,7 +86,7 @@ describe OrderBalance do
describe '#amount' do
context 'when the customer_balance feature is disabled' do
before do
allow(order).to receive(:outstanding_balance) { 10 }
allow(order).to receive(:old_outstanding_balance) { 10 }
end
before do
@@ -124,7 +124,7 @@ describe OrderBalance do
context 'when the balance is zero' do
before do
allow(order).to receive(:outstanding_balance) { 0 }
allow(order).to receive(:old_outstanding_balance) { 0 }
end
it 'returns true' do
@@ -134,7 +134,7 @@ describe OrderBalance do
context 'when the balance is positive' do
before do
allow(order).to receive(:outstanding_balance) { 10 }
allow(order).to receive(:old_outstanding_balance) { 10 }
end
it 'returns false' do
@@ -144,7 +144,7 @@ describe OrderBalance do
context 'when the balance is negative' do
before do
allow(order).to receive(:outstanding_balance) { -10 }
allow(order).to receive(:old_outstanding_balance) { -10 }
end
it 'returns false' do
@@ -199,7 +199,7 @@ describe OrderBalance do
end
it 'calls #outstanding_balance' do
expect(order).to receive(:outstanding_balance)
expect(order).to receive(:old_outstanding_balance)
order_balance.to_f
end
end
@@ -216,4 +216,112 @@ describe OrderBalance do
end
end
end
describe '#abs' do
context 'when the customer_balance feature is disabled' do
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, user) { false }
end
context 'when the balance is zero' do
before do
allow(order).to receive(:old_outstanding_balance) { 0 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(0)
end
end
context 'when the balance is positive' do
before do
allow(order).to receive(:old_outstanding_balance) { 10 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(10)
end
end
context 'when the balance is negative' do
before do
allow(order).to receive(:old_outstanding_balance) { -10 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(10)
end
end
end
context 'when the customer_balance feature is enabled' do
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, user) { true }
end
context 'when the balance is zero' do
before do
allow(order).to receive(:new_outstanding_balance) { 0 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(0)
end
end
context 'when the balance is positive' do
before do
allow(order).to receive(:new_outstanding_balance) { 10 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(10)
end
end
context 'when the balance is negative' do
before do
allow(order).to receive(:new_outstanding_balance) { -10 }
end
it 'returns its absolute value' do
expect(order_balance.abs).to eq(10)
end
end
end
end
describe '#to_s' do
context 'when the customer_balance feature is disabled' do
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, user) { false }
end
before do
allow(order).to receive(:old_outstanding_balance) { 10 }
end
it 'returns the balance as a string' do
expect(order_balance.to_s).to eq('10')
end
end
context 'when the customer_balance feature is enabled' do
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, user) { true }
end
before do
allow(order).to receive(:new_outstanding_balance) { 10 }
end
it 'returns the balance as a string' do
expect(order_balance.to_s).to eq('10')
end
end
end
end

View File

@@ -310,7 +310,7 @@ describe Spree::Order do
context "#display_outstanding_balance" do
it "returns the value as a spree money" do
allow(order).to receive(:outstanding_balance) { 10.55 }
allow(order).to receive(:old_outstanding_balance) { 10.55 }
expect(order.display_outstanding_balance).to eq Spree::Money.new(10.55)
end
end

View File

@@ -382,7 +382,7 @@ describe Spree::Payment do
end
it "resulting payment should have correct values" do
allow(payment.order).to receive(:outstanding_balance) { 100 }
allow(payment.order).to receive(:old_outstanding_balance) { 100 }
allow(payment).to receive(:credit_allowed) { 10 }
offsetting_payment = payment.credit!
@@ -402,7 +402,7 @@ describe Spree::Payment do
end
it 'lets the new payment to be saved' do
allow(payment.order).to receive(:outstanding_balance) { 100 }
allow(payment.order).to receive(:old_outstanding_balance) { 100 }
allow(payment).to receive(:credit_allowed) { 10 }
offsetting_payment = payment.credit!