raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-01-24 20:36:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #6366 from coopdevs/fix-customer-balance-n+1

Browse Source 
Fix customer balance n+1
This commit is contained in:
Pau Pérez Fabregat
2021-01-13 10:25:47 +01:00
committed by GitHub
parent f633f1809f 33c72ecab0
commit 0b686bbd32
13 changed files with 556 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
app/controllers/admin/customers_controller.rb
View File

@@ -17,13 +17,22 @@ module Admin
respond_to do |format|
format.html
format.json do
render_as_json @collection,
tag_rule_mapping: tag_rule_mapping,
customer_tags: customer_tags_by_id
render json: @collection,
each_serializer: index_each_serializer,
tag_rule_mapping: tag_rule_mapping,
customer_tags: customer_tags_by_id
end
end
end
def index_each_serializer
if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user)
::Api::Admin::CustomerWithBalanceSerializer
else
::Api::Admin::CustomerWithCalculatedBalanceSerializer
end
end
def show
render_as_json @customer, ams_prefix: params[:ams_prefix]
end
@@ -63,10 +72,20 @@ module Admin
private
def collection
return Customer.where("1=0") unless json_request? && params[:enterprise_id].present?
if json_request? && params[:enterprise_id].present?
customers_relation.
includes(:bill_address, :ship_address, user: :credit_cards)
else
Customer.where('1=0')
end
end
Customer.of(managed_enterprise_id).
includes(:bill_address, :ship_address, user: :credit_cards)
def customers_relation
if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, spree_current_user)
CustomersWithBalance.new(managed_enterprise_id).query
else
Customer.of(managed_enterprise_id)
end
end
def managed_enterprise_id

21
app/serializers/api/admin/customer_serializer.rb
View File

@@ -4,7 +4,7 @@ module Api
module Admin
class CustomerSerializer < ActiveModel::Serializer
attributes :id, :email, :enterprise_id, :user_id, :code, :tags, :tag_list, :name,
:allow_charges, :default_card_present?, :balance, :balance_status
:allow_charges, :default_card_present?
has_one :ship_address, serializer: Api::AddressSerializer
has_one :bill_address, serializer: Api::AddressSerializer
@@ -17,20 +17,6 @@ module Api
customer_tag_list.join(",")
end
def balance
Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s
end
def balance_status
if balance_value.positive?
"credit_owed"
elsif balance_value.negative?
"balance_due"
else
""
end
end
def tags
customer_tag_list.map do |tag|
tag_rule_map = options[:tag_rule_mapping].andand[tag]
@@ -51,11 +37,6 @@ module Api
options[:customer_tags].andand[object.id] || []
end
def balance_value
@balance_value ||=
OpenFoodNetwork::UserBalanceCalculator.new(object.email, object.enterprise).balance
end
end
end
end

25
app/serializers/api/admin/customer_with_balance_serializer.rb Normal file
View File

@@ -0,0 +1,25 @@
# frozen_string_literal: true
module Api
module Admin
class CustomerWithBalanceSerializer < CustomerSerializer
attributes :balance, :balance_status
delegate :balance_value, to: :object
def balance
Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s
end
def balance_status
if balance_value.positive?
"credit_owed"
elsif balance_value.negative?
"balance_due"
else
""
end
end
end
end
end

28
app/serializers/api/admin/customer_with_calculated_balance_serializer.rb Normal file
View File

@@ -0,0 +1,28 @@
# frozen_string_literal: true
module Api
module Admin
class CustomerWithCalculatedBalanceSerializer < CustomerSerializer
attributes :balance, :balance_status
def balance
Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s
end
def balance_status
if balance_value.positive?
"credit_owed"
elsif balance_value.negative?
"balance_due"
else
""
end
end
def balance_value
@balance_value ||=
OpenFoodNetwork::UserBalanceCalculator.new(object.email, object.enterprise).balance
end
end
end
end

61
app/services/customers_with_balance.rb Normal file
View File

@@ -0,0 +1,61 @@
# frozen_string_literal: true
class CustomersWithBalance
def initialize(enterprise)
@enterprise = enterprise
end
def query
Customer.of(enterprise).
joins(left_join_complete_orders).
group("customers.id").
select("customers.*").
select(outstanding_balance)
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
end
# The resulting orders are in states that belong after the checkout. Only these can be considered
# for a customer's balance.
def left_join_complete_orders
<<-SQL.strip_heredoc
LEFT JOIN spree_orders ON spree_orders.customer_id = customers.id
AND #{complete_orders.to_sql}
SQL
end
def complete_orders
states_group = prior_to_completion_states.map { |state| Arel::Nodes.build_quoted(state) }
Arel::Nodes::NotIn.new(Spree::Order.arel_table[:state], states_group)
end
def non_fulfilled_states_group
states_group = 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

12
lib/open_food_network/feature_toggle.rb
View File

@@ -31,17 +31,19 @@ module OpenFoodNetwork
end
def self.enable(feature_name, user_emails)
return unless user_emails.present?
Thread.current[:features] ||= {}
Thread.current[:features][feature_name] = Feature.new(user_emails)
end
def initialize
@features = Thread.current[:features]
@features = Thread.current[:features] || {}
end
def enabled?(feature_name, user)
if user.present?
feature = features[feature_name]
feature = features.fetch(feature_name, NullFeature.new)
feature.enabled?(user)
else
true?(env_variable_value(feature_name))
@@ -78,4 +80,10 @@ module OpenFoodNetwork
attr_reader :users
end
class NullFeature
def enabled?(_user)
false
end
end
end

119
spec/controllers/admin/customers_controller_spec.rb
View File

@@ -1,6 +1,7 @@
# frozen_string_literal: true
require 'spec_helper'
require 'open_food_network/user_balance_calculator'
module Admin
describe CustomersController, type: :controller do
@@ -41,6 +42,124 @@ module Admin
expect(ActiveModel::ArraySerializer).to receive(:new)
spree_get :index, params
end
context 'when the customer_balance feature is enabled' do
let(:customers_with_balance) { instance_double(CustomersWithBalance) }
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, enterprise.owner) { true }
end
it 'calls CustomersWithBalance' do
allow(CustomersWithBalance)
.to receive(:new).with(enterprise) { customers_with_balance }
expect(customers_with_balance).to receive(:query) { Customer.none }
spree_get :index, params
end
it 'serializes using CustomerWithBalanceSerializer' do
expect(Api::Admin::CustomerWithBalanceSerializer).to receive(:new)
spree_get :index, params
end
end
context 'when the customer_balance feature is not enabled' do
let(:calculator) do
instance_double(OpenFoodNetwork::UserBalanceCalculator, balance: 0)
end
it 'calls Customer.of' do
expect(Customer).to receive(:of).twice.with(enterprise) { Customer.none }
spree_get :index, params
end
it 'serializes calling the UserBalanceCalculator' do
expect(OpenFoodNetwork::UserBalanceCalculator)
.to receive(:new).with(customer.email, customer.enterprise) { calculator }
spree_get :index, params
end
end
context 'when the customer has no orders' do
it 'includes the customer balance in the response' do
spree_get :index, params
expect(json_response.first["balance"]).to eq("$0.00")
end
end
context 'when the customer has complete orders' do
let(:order) { create(:order, customer: customer, state: 'complete') }
let!(:line_item) { create(:line_item, order: order, price: 10.0) }
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, enterprise.owner) { true }
end
it 'includes the customer balance in the response' do
spree_get :index, params
expect(json_response.first["balance"]).to eq("$-10.00")
end
end
context 'when the customer has canceled orders' do
let(:order) { create(:order, customer: customer) }
let!(:line_item) { create(:line_item, order: order, price: 10.0) }
let!(:payment) { create(:payment, order: order, amount: order.total) }
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, enterprise.owner) { true }
allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true)
order.process_payments!
order.update_attribute(:state, 'canceled')
end
it 'includes the customer balance in the response' do
spree_get :index, params
expect(json_response.first["balance"]).to eq("$10.00")
end
end
context 'when the customer has cart orders' do
let(:order) { create(:order, customer: customer, state: 'cart') }
let!(:line_item) { create(:line_item, order: order, price: 10.0) }
it 'includes the customer balance in the response' do
spree_get :index, params
expect(json_response.first["balance"]).to eq("$0.00")
end
end
context 'when the customer has an order with a void payment' do
let(:order) { create(:order, customer: customer, state: 'complete') }
let!(:line_item) { create(:line_item, order: order, price: 10.0) }
let!(:payment) { create(:payment, order: order, amount: order.total) }
before do
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, enterprise.owner) { true }
allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true)
order.process_payments!
payment.void_transaction!
end
it 'includes the customer balance in the response' do
expect(order.payment_total).to eq(0)
spree_get :index, params
expect(json_response.first["balance"]).to eq('$-10.00')
end
end
end
context "and enterprise_id is not given in params" do

2
spec/factories/order_factory.rb
View File

@@ -9,7 +9,7 @@ FactoryBot.define do
user
bill_address
completed_at { nil }
email { user.email }
email { user&.email || customer.email }
factory :order_with_totals do
after(:create) do |order|

40
spec/features/admin/customers_spec.rb
View File

@@ -97,9 +97,36 @@ feature 'Customers' do
describe "for a shop with multiple customers" do
before do
mock_balance(customer1.email, managed_distributor1, 88)
mock_balance(customer2.email, managed_distributor1, -99)
mock_balance(customer4.email, managed_distributor1, 0)
allow(OpenFoodNetwork::FeatureToggle)
.to receive(:enabled?).with(:customer_balance, user) { true }
create(
:order,
total: 0,
payment_total: 88,
distributor: managed_distributor1,
user: nil,
state: 'complete',
customer: customer1
)
create(
:order,
total: 99,
payment_total: 0,
distributor: managed_distributor1,
user: nil,
state: 'complete',
customer: customer2
)
create(
:order,
total: 0,
payment_total: 0,
distributor: managed_distributor1,
user: nil,
state: 'complete',
customer: customer4
)
customer4.update enterprise: managed_distributor1
end
@@ -121,13 +148,6 @@ feature 'Customers' do
expect(page).to have_content "$0.00"
end
end
def mock_balance(email, enterprise, balance)
user_balance_calculator = instance_double(OpenFoodNetwork::UserBalanceCalculator)
expect(OpenFoodNetwork::UserBalanceCalculator).to receive(:new).
with(email, enterprise).and_return(user_balance_calculator)
expect(user_balance_calculator).to receive(:balance).and_return(balance)
end
end
it "allows updating of attributes" do

21
spec/models/spree/order_spec.rb
View File

@@ -201,16 +201,27 @@ describe Spree::Order do
let(:payment) { build(:payment) }
before { allow(order).to receive_messages pending_payments: [payment], total: 10 }
it "should process the payments" do
expect(payment).to receive(:process!)
expect(order.process_payments!).to be_truthy
end
it "should return false if no pending_payments available" do
allow(order).to receive_messages pending_payments: []
expect(order.process_payments!).to be_falsy
end
context "when the processing is sucessful" do
it "should process the payments" do
expect(payment).to receive(:process!)
expect(order.process_payments!).to be_truthy
end
it "stores the payment total on the order" do
allow(payment).to receive(:process!)
allow(payment).to receive(:completed?).and_return(true)
order.process_payments!
expect(order.payment_total).to eq(payment.amount)
end
end
context "when a payment raises a GatewayError" do
before { expect(payment).to receive(:process!).and_raise(Spree::Core::GatewayError) }

9
spec/models/spree/payment_spec.rb
View File

@@ -584,6 +584,15 @@ describe Spree::Payment do
)
end
end
context 'when the payment was completed but now void' do
let(:payment) { create(:payment, amount: 100, order: order, state: 'completed') }
it 'updates order payment total' do
payment.void
expect(order.payment_total).to eq 0
end
end
end
context "#build_source" do

46
spec/serializers/api/admin/customer_with_balance_serializer_spec.rb Normal file
View File

@@ -0,0 +1,46 @@
# frozen_string_literal: true
require 'spec_helper'
describe Api::Admin::CustomerWithBalanceSerializer do
let(:serialized_customer) { described_class.new(customer) }
describe '#balance' do
let(:customer) { double(Customer, balance_value: 1.2) }
let(:money) { instance_double(Spree::Money, to_s: "$1.20") }
before do
allow(Spree::Money).to receive(:new).with(1.2, currency: "AUD") { money }
end
it 'returns the balance_value as a money amount' do
expect(serialized_customer.balance).to eq("$1.20")
end
end
describe '#balance_status' do
context 'when the balance_value is positive' do
let(:customer) { double(Customer, balance_value: 1) }
it 'returns credit_owed' do
expect(serialized_customer.balance_status).to eq("credit_owed")
end
end
context 'when the balance_value is negative' do
let(:customer) { double(Customer, balance_value: -1) }
it 'returns credit_owed' do
expect(serialized_customer.balance_status).to eq("balance_due")
end
end
context 'when the balance_value is zero' do
let(:customer) { double(Customer, balance_value: 0) }
it 'returns credit_owed' do
expect(serialized_customer.balance_status).to eq("")
end
end
end
end

185
spec/services/customers_with_balance_spec.rb Normal file
View File

@@ -0,0 +1,185 @@
# frozen_string_literal: true
require 'spec_helper'
describe CustomersWithBalance do
subject(:customers_with_balance) { described_class.new(customer.enterprise.id) }
describe '#query' do
let(:customer) { create(:customer) }
let(:total) { 200.00 }
let(:order_total) { 100.00 }
context 'when orders are in cart state' do
before do
create(:order, customer: customer, total: order_total, payment_total: 0, state: 'cart')
create(:order, customer: customer, total: order_total, payment_total: 0, state: 'cart')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(0)
end
end
context 'when orders are in address state' do
before do
create(:order, customer: customer, total: order_total, payment_total: 0, state: 'address')
create(:order, customer: customer, total: order_total, payment_total: 50, state: 'address')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(0)
end
end
context 'when orders are in delivery state' do
before do
create(:order, customer: customer, total: order_total, payment_total: 0, state: 'delivery')
create(:order, customer: customer, total: order_total, payment_total: 50, state: 'delivery')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(0)
end
end
context 'when orders are in payment state' do
before do
create(:order, customer: customer, total: order_total, payment_total: 0, state: 'payment')
create(:order, customer: customer, total: order_total, payment_total: 50, state: 'payment')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(0)
end
end
context 'when no orders where paid' do
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(-total)
end
end
context 'when an order was paid' do
let(:payment_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
order.update_attribute(:state, 'checkout')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - total)
end
end
context 'when an order is canceled' do
let(:payment_total) { 100.00 }
let(:non_canceled_orders_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
create(
:order,
customer: customer,
total: order_total,
payment_total: order_total,
state: 'canceled'
)
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - non_canceled_orders_total)
end
end
context 'when an order is resumed' do
let(:payment_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
order.update_attribute(:state, 'resumed')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - total)
end
end
context 'when an order is in payment' do
let(:payment_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
order.update_attribute(:state, 'payment')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - total)
end
end
context 'when an order is awaiting_return' do
let(:payment_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
order.update_attribute(:state, 'awaiting_return')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - total)
end
end
context 'when an order is returned' do
let(:payment_total) { order_total }
let(:non_returned_orders_total) { order_total }
before do
order = create(:order, customer: customer, total: order_total, payment_total: 0)
order.update_attribute(:state, 'checkout')
order = create(:order, customer: customer, total: order_total, payment_total: payment_total)
order.update_attribute(:state, 'returned')
end
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(payment_total - non_returned_orders_total)
end
end
context 'when there are no orders' do
it 'returns the customer balance' do
customer = customers_with_balance.query.first
expect(customer.balance_value).to eq(0)
end
end
end
end