Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/queries/outstanding_balance_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
#
# Note this query object and `app/models/concerns/balance.rb` should implement the same behavior
# until we find a better way. If you change one, please, change the other too.
#
# KNOWN DIVERGENCE: Balance#new_outstanding_balance deducts incomplete customer credit payments
# (payments.incomplete.customer_credit.sum(:amount)) from the balance, but this SQL query has no
# join to the payments table and cannot do the same. As a result, orders where customer credit has
# been applied but not yet captured are counted as fully unpaid here, causing the dashboard
# customers tab to overstate what the customer owes compared to the Customers report.
Comment on lines +14 to +19

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for documenting the divergence. It's out of scope for this PR, but it would be good to get others point on vue on this. OutstandingBalanceQuery takes into account any existing order, ie it includes non placed orders. This doesn't really make sense to me, if the order is not placed then the balance is not technically outstanding ? If we fix the query to only includes placed order, then the divergence will go away. @mkllnk @dacook what do you think ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, yes what you say makes sense.
So in other words, there's a (now known) bug in the admin customers page, and the above describes how to fix it. Seems like a good first issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug sounds related, but might be more complicated.. 😳 #6038

Without investigating further, I would probably just add the above comment to that issue for future reference... unless Gaetan you see a better way?

class OutstandingBalanceQuery
# All the states of a finished order but that shouldn't count towards the balance (the customer
# didn't get the order for whatever reason). Note it does not include complete
Expand Down
2 changes: 2 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3508,6 +3508,8 @@ en:
report_line_cost_of_produce: Cost of produce
report_line_line_items: line items
report_header_last_completed_order_date: Last completed order date
report_header_balance_due: "Balance Due (%{currency_symbol})"
report_header_credit_due: "Available Credit (%{currency_symbol})"
report_xero_configuration: Xero Configuration
initial_invoice_number: "Initial invoice number"
invoice_date: "Invoice date"
Expand Down
24 changes: 22 additions & 2 deletions lib/reporting/reports/customers/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def query_result
end.values
end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude is correct in its assessment but the intent of the comment is to say we are not going to fix it. Surprisingly, rubocop is not complaining after removal, so happy to keep as it is.

def columns
{
first_name: proc { |orders| last_completed_order(orders).billing_address.firstname },
Expand All @@ -35,9 +35,29 @@ def columns
total_orders: proc { |orders| orders.count },
total_incl_tax: proc { |orders| orders.map(&:total).compact.sum },
last_completed_order_date: proc { |orders| last_completed_order_date(orders) },
# Outstanding amount the customer still owes across all their orders at this hub.
# Uses outstanding_balance (which deducts incomplete customer credit payments) rather
# than the raw order total, so applied-but-not-yet-captured credits are accounted for.
# Floored at zero — negative values mean the shop owes the customer, which is
# captured separately in credit_due.
balance_due: proc { |orders|
total = orders.sum { |o| o.outstanding_balance.to_f }
total.positive? ? total : 0
},
# Credit held in the customer's account (from CustomerAccountTransaction records),
# available to be applied to future orders. This is a separate ledger from order
# balances — credit is granted explicitly (e.g. via the bulk credit admin action)
# and does not simply reflect overpayments on individual orders.
credit_due: proc { |orders|
orders.first.customer&.credit_balance || 0
},
}
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity

def columns_format
{ balance_due: :currency, credit_due: :currency }
end

def filter(orders)
filter_to_completed_at filter_to_distributor filter_to_order_cycle orders
Expand Down
32 changes: 24 additions & 8 deletions spec/lib/reports/customers_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"Email", "Phone", "Hub", "Hub Address",
"Shipping Method", "Total Number of Orders",
"Total incl. tax ($)",
"Last completed order date"])
"Last completed order date",
"Balance Due ($)", "Available Credit ($)"])
end

it "builds a table from a list of variants" do
Expand All @@ -27,7 +28,8 @@
o.email, a.phone, d.name,
[d.address.address1, d.address.address2,
d.address.city].join(" "),
o.shipping_method.name, 1, o.total, "none"
o.shipping_method.name, 1, o.total, "none",
"$0.00", "$0.00"
]])
end

Expand Down Expand Up @@ -65,10 +67,19 @@
[a.address1, a.address2, a.city].join(" "),
o1.email, a.phone, d.name,
[d.address.address1, d.address.address2, d.address.city].join(" "),
o1.shipping_method.name, 2, o1.total + o2.total, "2023-01-02"
o1.shipping_method.name, 2, o1.total + o2.total, "2023-01-02",
"$26.00", "$0.00"
]])
end

context "when the customer has a credit balance" do
before { create(:customer_account_transaction, customer:, amount: 25.00) }

it "reflects the credit balance in the credit_due column" do
expect(subject.table_rows.first.last).to eq("$25.00")
end
end

context "orders from different hubs" do
let!(:d2) { create(:distributor_enterprise) }
let!(:sm2) { create(:shipping_method, distributors: [d2]) }
Expand All @@ -87,13 +98,15 @@
[a.address1, a.address2, a.city].join(" "),
o1.email, a.phone, d.name,
[d.address.address1, d.address.address2, d.address.city].join(" "),
o1.shipping_method.name, 1, o1.total, "2023-01-01"
o1.shipping_method.name, 1, o1.total, "2023-01-01",
"$13.00", "$0.00"
], [
a.firstname, a.lastname,
[a.address1, a.address2, a.city].join(" "),
o2.email, a.phone, d2.name,
[d2.address.address1, d2.address.address2, d2.address.city].join(" "),
o2.shipping_method.name, 1, o2.total, "2023-01-02"
o2.shipping_method.name, 1, o2.total, "2023-01-02",
"$13.00", "$0.00"
]])
end
end
Expand All @@ -112,7 +125,8 @@
context "when the shipping method column is being included" do
let(:fields_to_show) do
[:first_name, :last_name, :billing_address, :email, :phone, :hub, :hub_address,
:shipping_method, :total_orders, :total_incl_tax, :last_completed_order_date]
:shipping_method, :total_orders, :total_incl_tax, :last_completed_order_date,
:balance_due, :credit_due]
end
subject { described_class.new(user, { fields_to_show: }) }

Expand All @@ -129,7 +143,8 @@
a.phone,
d.name,
[d.address.address1, d.address.address2, d.address.city].join(" "),
o1.shipping_method.name, 1, o1.total, o1.completed_at.strftime("%Y-%m-%d")
o1.shipping_method.name, 1, o1.total, o1.completed_at.strftime("%Y-%m-%d"),
"$13.00", "$0.00"
],
[
a.firstname,
Expand All @@ -139,7 +154,8 @@
a.phone,
d.name,
[d.address.address1, d.address.address2, d.address.city].join(" "),
sm2.name, 1, o2.total, o2.completed_at.strftime("%Y-%m-%d")
sm2.name, 1, o2.total, o2.completed_at.strftime("%Y-%m-%d"),
"$13.00", "$0.00"
]
]
)
Expand Down
1 change: 1 addition & 0 deletions spec/system/admin/reports/customers_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"First Name", "Last Name", "Billing Address", "Email", "Phone",
"Hub", "Hub Address", "Shipping Method", "Total Number of Orders",
"Total incl. tax ($)", "Last completed order date",
"Balance Due ($)", "Available Credit ($)",
]
]
)
Expand Down