Add Balance Due and Available Credit columns to Customers report#14390
Add Balance Due and Available Credit columns to Customers report#14390NiftyGaloot wants to merge 6 commits into
Conversation
- balance_due: use outstanding_balance (OrderBalance object) instead of new_outstanding_balance - credit_due: use customer.credit_balance instead of deriving from order balances
- balance_due: sums outstanding_balance across a customer's orders at a hub, accounting for incomplete customer credit payments (floored at zero) - credit_due: reads customer.credit_balance from CustomerAccountTransaction records — the separate credit ledger, not derived from order balances - columns_format declares both as :currency for the report renderer - Rename column header from "Credit Due" to "Available Credit" to match the customers tab in the dashboard - Document known divergence in OutstandingBalanceQuery: unlike Balance#new_outstanding_balance, the SQL query does not deduct incomplete customer credit payments, causing the dashboard to overstate what a customer owes when credit has been applied but not yet captured Closes openfoodfoundation/wishlist#562 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Restore 2-space indentation in Base#filter (regressed to 1-space) - Add spec asserting credit_due reflects a non-zero CustomerAccountTransaction balance; previously all examples asserted "$0.00", so a regression in Customer#credit_balance would have gone undetected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update column name from "Credit Due ($)" to "Available Credit ($)" to match the renamed i18n key - Remove hasBalanceDue/hasCreditDue assertions that required specific seed data to be present; format and non-negative value checks are sufficient here — non-zero value coverage is provided by customer-report-journey.spec.ts which sets up its own test data Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sigmundpetersen
moved this from All the things 💤
to In Progress ⚙
in OFN Delivery board
|
This is my first attempt at an addition to the OFN code. I used both OpenCode and Claude AI to evaluate and test this wishlist item. The initial work was done using OpenCode with changes to the math suggested after asking for posting a proposed solution and asking for feedback. Additonal review and testing was done with Claude. Playwright allowed an inspection of test transactions in the localhost. The results appear to be working correctly. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The columns method is a data definition hash with multiple procs; the complexity is inherent to its structure, not reducible without obscuring the intent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rioug
left a comment
rioug
left a comment
There was a problem hiding this comment.
@NiftyGaloot thanks for your help 🙏 . The changes look good 👍
For next time, we have Pull Request template, could you use it when creating a PR ? And also, it would have been better to remove the playwright related commits.
| # | ||
| # 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. |
There was a problem hiding this comment.
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 ?
| end | ||
|
|
||
| # rubocop:disable Metrics/AbcSize | ||
| # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity |
There was a problem hiding this comment.
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.
|
@RachL this PR closes a wishlist item, I am not sure what's the usual process, but I though we should have an issue in the OFN repo to link to this PR ? |
rioug
added
the
user facing changes
|
@rioug we leave in wishlist anything that is not prepared / not ready to dev. I don't think it's mandatory to create an issue in the OFN repo. It is however mandatory to align on what is needed before submitting a PR. I don't follow the ai community of practice, therefore I can't comment if this is being done or not. |
|
@rioug @RachL I did look for a way to move the wishlist item into a pr but didn't find how to do it. That was before learning how to use AI to find such things. I asked it this question now: Is there information on how a wishlist item moves to a pr in the OFN documentation? And got this response: OFN's own docs cover parts of the path but don't have a single "wishlist → PR" page. The full pipeline is scattered:
Should something be added to the documents spelling out what's needed to move a wishlist item up the chain to a pr? |
|
@NiftyGaloot just to be sure we are speaking of the same thing :
So you can never transform an issue into a PR. Both will always stay but a PR can close an issue when it's merged. If the question was how do wishlist get priorities in OFN, then in that case the current workflow is that there is a dedicated budget (mostly funded feature currently) and then it moves up. |
|
Thanks @RachL . I just wanted some clarity on who has to say we are happy to proceed with a wishlist item, I am assuming it's Products. I'll raise topic in delivery circle this week. I am just trying to make sure we don't get random PR on something people want to add to OFN without prior consultation with the team/community. |
4 participants
What's changed?
Adds two new columns to the Customers report:
outstanding_balance(which deducts incomplete customer-credit payments) so applied-but-not-yet-captured credits are accounted for. Floored at zero.CustomerAccountTransactionrecords, available to apply to future orders. This is a separate ledger from order balances.Closes openfoodfoundation/wishlist#562
How to test
/admin/reports/customers)No need to run these, they are run as part of Continuous Integration (CI), ie we run the full test suite when a PR is created:
4. Run unit specs:bundle exec rspec spec/lib/reports/customers_report_spec.rb5. Run system spec:bundle exec rspec spec/system/admin/reports/customers_report_spec.rbFiles changed
lib/reporting/reports/customers/base.rb— addedbalance_dueandcredit_duecolumns +columns_formatconfig/locales/en.yml— addedreport_header_balance_dueandreport_header_credit_duei18n keysspec/lib/reports/customers_report_spec.rb— updated all row expectations + added credit balance contextspec/system/admin/reports/customers_report_spec.rb— added new headers to expectation🤖 Generated with Claude Code
Screenshots