Skip to content

fix: shift closing total reflects money collected, not invoiced - #312

Merged
MostafaKadry merged 7 commits into
BrainWise-DEV:developfrom
NotAbdelrahmanelsayed:fix/credit-sale-closing-total
Sep 8, 2026
Merged

fix: shift closing total reflects money collected, not invoiced#312
MostafaKadry merged 7 commits into
BrainWise-DEV:developfrom
NotAbdelrahmanelsayed:fix/credit-sale-closing-total

Conversation

@NotAbdelrahmanelsayed

Copy link
Copy Markdown
Contributor

Problem

When closing a POS shift, the closing total counts the full grand total of every invoice linked to the shift — even pure Pay-on-Account credit sales where no money was received. The cash Payment Reconciliation is built only from real payment rows, so it is already correct — but Net Sales (grand_total) disagrees with it within a single shift:

Event Cash reconciliation Net Sales (grand_total)
Credit sale created (no money) +0 (correct) +full amount (bug)
Old due collected today +amount (correct) +0

Root cause: pos_closing_shift.py › _process_invoice() added base_grand_total (the full invoice value) into grand_total/net_total/sales_total and the per-row amount, ignoring how much was actually paid.

Fix

For non-return invoices, the money summaries and the per-row grand_total now use the amount actually collected (base_paid_amount):

  • pure credit sale → contributes 0
  • partial sale → contributes only its down-payment
  • net_total scaled by paid_ratio = base_paid / base_grand_total

The full invoice value is preserved in transaction_amount; display-only invoice_total and outstanding_amount are added for the dialog and stripped before the child-table set (Sales Invoice Reference has no such columns). Quantities, tax accrual and the returns branch are unchanged. With this change Net Sales == sum of per-invoice collected == reconciliation cash delta.

The Close Shift dialog shows an On Account / Partially Paid badge and an Unpaid: {amount} sub-line on rows collected for less than their invoice value (desktop + mobile).

Edge case: if a credit sale is created and its due is collected within the same shift, the down-payment shows in Net Sales and the later dues Payment Entry shows in reconciliation — both are real takings, reported in their respective sections.

Tests

Adds unit tests for _process_invoice covering fully-paid, pure-credit, partial and credit-return cases, asserting the summary equals only the collected portions and per-row amounts sum to the header total. Adds the Arabic translation for the new Unpaid: {0} string.

🤖 Generated with Claude Code

NotAbdelrahmanelsayed and others added 2 commits June 12, 2026 21:09
Credit (Pay-on-Account) sales inflated the POS shift closing total.
`_process_invoice()` added the full `base_grand_total` of every invoice to
the sales summary (`grand_total`/`net_total`/`sales_total`) and the per-row
amount, regardless of how much was actually paid. The cash Payment
Reconciliation, however, is built only from real payment rows — so a pure
credit sale pushed Net Sales up by the full amount while contributing 0 to
the drawer, leaving the two figures inconsistent within a single shift.

For non-return invoices the money summaries and the per-row `grand_total`
now use the amount actually collected (`base_paid_amount`): a pure credit
sale contributes 0, a partial sale contributes only its down-payment, and
`net_total` is scaled by the paid ratio. The full invoice value is preserved
in `transaction_amount`; display-only `invoice_total` and
`outstanding_amount` are added for the dialog badge and stripped before the
child-table set. Returns, quantities and tax accrual are unchanged.

The Close Shift dialog now shows an "On Account" / "Partially Paid" badge and
an "Unpaid: {amount}" sub-line on rows collected for less than their invoice
value. Adds unit tests for the collected-money totals and an Arabic string.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…closing-total

# Conflicts:
#	POS/src/components/ShiftClosingDialog.vue
#	pos_next/pos_next/doctype/pos_closing_shift/pos_closing_shift.py
#	pos_next/pos_next/doctype/pos_closing_shift/test_pos_closing_shift.py
#	pos_next/translations/ar.csv
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity for 21 days.

To keep this PR open, please:

  • Rebase on the latest develop branch
  • Address any pending review comments
  • Reply with an update on the PR status

If no further activity occurs within the next 14 days, this PR will be automatically closed.

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity for 21 days.

To keep this PR open, please:

  • Rebase on the latest develop branch
  • Address any pending review comments
  • Reply with an update on the PR status

If no further activity occurs within the next 14 days, this PR will be automatically closed.

@github-actions github-actions Bot added stale and removed stale labels Aug 14, 2026
@MostafaKadry

Copy link
Copy Markdown
Collaborator

Nice fix @NotAbdelrahmanelsayed!, this closes a real gap between Net Sales and what's actually in the drawer. Two things I found while testing that are worth patching before merge:

  1. change_amount isn't netted out of "collected" (_process_invoice):
    paid_amount is the raw sum of tendered payment rows, not net of change. So a $15.50 sale paid with a $20 bill ($4.50 change) currently computes collected = 20, not 15.50. Net Sales gets inflated by every cash-with-change sale, and outstanding_amount goes negative instead of 0.

Fix: subtract change before it's used anywhere:
base_change = get_base_value(invoice, "change_amount", "base_change_amount", conversion_rate)
base_paid = get_base_value(invoice, "paid_amount", "base_paid_amount", conversion_rate) - base_change
paid_ratio = (base_paid / base_grand_total) if base_grand_total else 0
(you already compute base_change further down for the cash-reconciliation bucket — just hoist that one line up and reuse it, no need to compute it twice.)

  1. Taxes stayed accrual-basis, everything else went cash-basis

net_total/grand_total now scale by paid_ratio for partial/credit sales, but the tax loop still aggregates the full tax_amount regardless of what was collected. That breaks grand_total ≈ net_total + taxes for any shift with a partial/credit sale that has tax on it. Worth a deliberate call. I'd scale it too, for consistency:
tax_ratio = 1 if is_return else paid_ratio
for t in invoice.taxes:
tax_amount = get_base_value(t, "tax_amount", "base_tax_amount", conversion_rate) * tax_ratio
_aggregate_tax(taxes, t.account_head, t.rate, tax_amount)

One unrelated nit: test_pos_closing_shift.py is 4-space indented. repo convention is tabs (ruff format will flag it).

Addresses review from @MostafaKadry on PR BrainWise-DEV#312:

- paid_amount is the raw tendered total, so cash change given back to
  the customer (e.g. $20 tendered on a $15.50 sale) was inflating
  "collected" and driving outstanding_amount negative. Netted
  base_change out of base_paid before it's used anywhere, and reused
  the value for the existing cash-reconciliation subtraction instead
  of computing it twice.
- net_total/grand_total scaled by paid_ratio for partial/credit sales,
  but the tax loop still aggregated the full tax_amount. Scaled taxes
  by the same ratio (1 for returns) so grand_total stays consistent
  with net_total + taxes.
- Reformatted test_pos_closing_shift.py to tabs (repo convention) and
  added regression tests for both fixes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TngwKeKiHtHt6jFP9pcnY6
@NotAbdelrahmanelsayed

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @MostafaKadry — pushed both fixes in 052ece5:

  1. change_amount is now netted out of base_paid right where it's computed (and the later cash-reconciliation subtraction reuses that same value instead of recomputing it).
  2. Taxes now scale by paid_ratio (1 for returns), so grand_total ≈ net_total + taxes holds for partial/credit sales.

Also reformatted test_pos_closing_shift.py to tabs and added regression tests for both cases (change-amount netting, tax scaling, and that returns keep full tax). All 6 unit tests pass locally.

@MostafaKadry

Copy link
Copy Markdown
Collaborator

@NotAbdelrahmanelsayed could you please fix (CI / Server) Failing, while I working in reviewing this pr or another one from your ones. thanks

MostafaKadry and others added 3 commits September 6, 2026 13:00
…them

Reworks the credit-sale closing fix so the cash-basis numbers live in their
own fields rather than overwriting the accrual ones.

grand_total, net_total and the taxes table go back to the invoiced amount.
Two new read-only fields on POS Closing Shift carry the cash view:

  collected_amount   money actually taken during the shift
  outstanding_total  invoiced value still owed by customers

invoiced == collected + outstanding, asserted in the tests.

Why the change of approach:

- POS Closing Shift.grand_total is persisted and already read elsewhere —
  get_shift_history() surfaces it as the "Sales" column and summary card.
  Redefining it as "collected" would have left historical rows meaning
  "invoiced" and new rows meaning "collected", with nothing to tell them
  apart. It now keeps its meaning and Shift History needs no change.

- Scaling taxes by the paid ratio kept grand_total = net_total + taxes, but
  tax posts to the GL in full at invoice submission regardless of what was
  collected, so the scaled table could not be reconciled against the VAT
  accounts. With nothing scaled the identity holds by construction.

- The ratio also misreported write-offs: a 100 invoice settled by 90 cash
  plus a 10 write-off reported net_total 90 and tax 0.9. Covered by a
  regression test.

Also:

- Port the same cash-basis logic to pos_closing_shift.js. The desk form
  mirrors _process_invoice(); leaving it on the old path would have made
  closing a shift from the desk and from the POS produce different totals
  for the same invoices.

- Closing dialog gains "Collected" and "On Account" cards, so the cashier
  can still see what was sold, not only what was banked. Per-invoice badges
  now key off collected_amount, since grand_total is invoiced again.

- Arabic strings for the three new labels.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
Dead assignment — the value is never read. It predates this branch, but
ruff lints changed files, so it fails pre-commit for anything touching
this module.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
@MostafaKadry

Copy link
Copy Markdown
Collaborator

please check this PR: NotAbdelrahmanelsayed#3

…l-followup

Enhance POS Shift History, Payment Handling, and Discount Logic
@NotAbdelrahmanelsayed

Copy link
Copy Markdown
Contributor Author

@MostafaKadry merged your fork PR (#3) into this branch — CI is green now (Node 20→22 was the actual cause of the Server check failures across the board, not anything in this PR; #3 happened to include that fix too). Went with your collected_amount/outstanding_total design over my paid_ratio scaling from 052ece5, for the reasons you laid out (Shift History parity, tax reconciliation, the write-off edge case). Ready for another look whenever you have time.

@MostafaKadry
MostafaKadry merged commit e0a52c5 into BrainWise-DEV:develop Sep 8, 2026
3 checks passed
MohamedAliSmk pushed a commit that referenced this pull request Sep 9, 2026
Addresses review from @MostafaKadry on PR #312:

- paid_amount is the raw tendered total, so cash change given back to
  the customer (e.g. $20 tendered on a $15.50 sale) was inflating
  "collected" and driving outstanding_amount negative. Netted
  base_change out of base_paid before it's used anywhere, and reused
  the value for the existing cash-reconciliation subtraction instead
  of computing it twice.
- net_total/grand_total scaled by paid_ratio for partial/credit sales,
  but the tax loop still aggregated the full tax_amount. Scaled taxes
  by the same ratio (1 for returns) so grand_total stays consistent
  with net_total + taxes.
- Reformatted test_pos_closing_shift.py to tabs (repo convention) and
  added regression tests for both fixes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TngwKeKiHtHt6jFP9pcnY6
@MohamedAliSmk MohamedAliSmk mentioned this pull request Sep 9, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants