Pn 65 expenses in pos screen

[MohamedAliSmk]

Summary

• Added ExpenseDialog component for recording expenses in the POS.
• Updated ShiftClosingDialog to display POS expenses summary and net cash impact.
• Enhanced InvoiceCart to include a button for recording POS expenses.
• Introduced new computed properties for managing expenses in the POS shift store.
• Updated translations to support new expense-related features.

Security decision: ignore_permissions on Employee / Account lookups

POS cashiers typically lack Employee and Account read permissions, so the till cannot populate the expense dialog with a normal get_list under their role.

get_active_employees and get_expense_accounts therefore use ignore_permissions=True only after validate_open_shift proves the caller owns an open shift for the requested profile. That is an intentional till trade-off:

Control	Behaviour
Gate	Open-shift ownership (validate_open_shift) before any bypassed read
Scope	Shift company only; Active employees; non-group, non-disabled expense ledgers
Bound	Accounts capped (EXPENSE_ACCOUNT_PAGE_LENGTH, default 50) with search_expense_accounts for further search
Fields	Employees return only name and employee_name (what the dialog select needs)
Revisit	Prefer a narrower Employee/Account role for cashiers if HR/chart exposure at the till becomes unacceptable

Also recorded in the module docstring of pos_next/api/expenses.py.

Test plan

• Open a shift as a cashier without Employee/Account read perms; expense dialog still loads accounts and employees
• Account autocomplete searches server-side and does not dump the full chart
• Recording an expense creates a submitted Journal Entry and updates shift totals
• Closing shift shows expenses / net cash impact correctly

N3 — branch narrowed (force-push)

Unrelated riders were dropped from this PR so the diff is expenses-only:

Dropped	Lands via
8231e26 offline batch/serial	#359
05a7b28 / 59acd2f / 9e9c26f EOD + translations	#275
f5ae353 collected vs invoiced (+ undo commit)	already on develop via #312
a4f6ffd ruff sweep	dropped; real cint fix → #373

Also folded N7 cosmetics: removed dead company_currency from bootstrap pos_profile payload; updated ExpenseDialog currency comment.

[github-actions]

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]

This pull request has been automatically closed due to inactivity.

If you'd like to continue working on this:

1. Reopen this PR or create a new one
2. Rebase on the latest develop branch
3. Address any pending review comments

Thank you for your contributions to POS Next!

[engahmed1190]

Review: architecture, security, performance, coverage

Reviewed against 04034eb (merge base 9212350). Verified on a local bench: Frappe + ERPNext, 3 sites. Backend tests run — 17/17 pass with a site context. ruff check clean on every new file. The report's SQL is fully parameterised; no injection surface anywhere in the new code.

One blocker, four high, seven medium. Everything else here is good work, and worth saying first: this PR has a real description, its validators are comprehensive and each one is individually tested, the report and workspace entry are wired up, and the translations landed in the same change. That is a markedly more finished piece of work than #360.

The blocker, in one line

posa_maximum_expense_amount ships with no default, and validate_expense_amount reads if maximum_amount <= 0: return. So ticking one checkbox on a POS Profile gives every cashier on it an unbounded authority to submit Journal Entries crediting the till, through a whitelisted endpoint that runs ignore_permissions=True. Details inline on custom/pos_profile.json and api/expenses.py.

One finding I inverted while checking it

The report grants POSNext Cashier, which reads like cross-company exposure. It isn't: frappe.desk.query_report.run also checks has_permission(ref_doctype, "report") on Journal Entry (frappe/desk/query_report.py:207), and I confirmed on this bench that POSNext Cashier holds no Journal Entry DocPerm or Custom DocPerm and the shipped fixtures/custom_docperm.json adds none. A cashier clicking the report gets "Must have report permission to access this report." So the role grant is an inert config bug, not a hole. The real half — no company scoping in the query — is filed as M1.

Merge collision with #360

Both PRs are open against develop and both touch POS/components.d.ts, POS/src/components/sale/InvoiceCart.vue, POS/src/pages/POSSale.vue and pos_next/api/bootstrap.py. components.d.ts will conflict outright — both register a new component. Whoever merges second resolves all four by hand.

This branch is also 17 commits behind develop. Rebase before re-review so the diff under discussion is the diff that will merge.

Filed as comments, not a change request — the call on merge is yours.

[engahmed1190]

What to finish before re-review

Everything open from review #5142106656, as a checklist. Each item names the file it lives in and the condition that closes it, so "done" is demonstrable rather than asserted.

21 items — 5 must close before re-review (Gate 1), 7 correctness, 4 tests, 3 cleanup, 2 hygiene.

---

Gate 1 — Blocking

The validation layer here is thorough and each validator is tested. These five are where the money actually escapes it.

• B1 — Make a blank expense maximum mean "not configured", not "unlimited" · custom/pos_profile.json:342, api/expenses.py:134
posa_maximum_expense_amount ships with no default (every other new field in that file carries default: "0"), and validate_expense_amount does if maximum_amount <= 0: return. Tick Allow POS Expense, get an empty Currency field, and the only monetary bound on the endpoint is gone. From there any cashier with an open shift can submit unbounded Journal Entries crediting the till through a whitelisted endpoint running ignore_permissions=True.
  Done when: enabling POS expenses without setting a maximum either refuses the expense or applies a shipped default — and the field description says what blank means.

• H1 — Lock the shift around check-and-insert · api/expenses.py:127, 311
validate_expense_amount reads the shift total, compares, returns; _create_expense_journal_entry inserts afterwards, with nothing serialising the two. Two concurrent requests each pass a 100 limit with amount 100 and both commit. The dialog's disabled-while-submitting button closes the casual double-click, so the open path is a replayed or scripted request.
  Done when: the total is read under for_update=True on the opening shift, inside the same transaction as the insert.

• H2 — Compute the limit from the credit rows, not posa_expense_amount · api/expenses.py:154
  That custom field is independent of the JE's actual lines and is read_only: 1 in the UI only — still writable via API. A JE with posa_is_pos_expense = 1 and a negative posa_expense_amount lowers the shift total and inflates everyone's allowance. JE create is privileged, which caps severity, but summing the credits removes the class of problem.
  Done when: get_shift_expense_total derives from the JE lines, and a JE whose custom field disagrees with its rows can't move the limit.

• H3 — Make the two ignore_permissions bypasses a recorded decision · api/expenses.py:273, 307
get_active_employees returns 200 employee IDs and names; get_expense_accounts returns the company's entire expense chart of accounts with limit_page_length=0. Both reachable by anyone with an open shift. The docstring explains why ("POS cashiers may lack Employee read perm") — that may be the right trade, but it's documented in one docstring and nowhere else.
  Done when: the bypass is justified in the PR description, the account list is bounded or server-searched, and the employee list returns only what the dialog needs.

• H4 — Add a cancel path · api/expenses.py:368, pos_closing_shift.py
create_pos_expense submits with no way to reverse it, so a mistyped amount needs desk access. Worse: get_pos_expenses filters docstatus = 1, so cancelling a JE after the shift closes silently drops it from that query while the submitted POS Closing Shift keeps the old total_pos_expenses and reconciliation rows. The two records disagree permanently, with nothing flagging it.
  Done when: a cancel endpoint exists with the same shift/ownership checks as create, and it refuses once the shift is closed.

---

Gate 2 — Correctness

• M1 — Scope the report by company; fix or drop the cashier role · pos_expense_report.py:96, pos_expense_report.json:36
get_data builds its WHERE from supplied filters only — company is optional, and raw SQL bypasses Company User Permission records. Any role holding Journal Entry report permission (including Sales Manager) sees every company. Separately, the POSNext Cashier grant is inert: query_report.run also checks has_permission("Journal Entry", "report") (frappe/desk/query_report.py:207) and that role has no JE permission, so the menu entry appears for people it will refuse.
  Done when: company is mandatory or defaulted and the user's permitted companies are applied; and the cashier either gets the permission or is dropped from the roles list.

• M2 — Make the closing-shift expense row an audit record · pos_closing_shift_expense.json:16
expense_account and employee are Data not Link, and there is no journal_entry field. No click-through to the voucher, no referential integrity, and nothing tying total_pos_expenses to the JEs behind it. get_pos_expenses already returns journal_entry; make_closing_shift_from_opening just drops it.
  Done when: the row links to its Journal Entry, and account/employee are Links.

• M3 — Stop resolving the payment account to an arbitrary ledger · api/expenses.py:249 → api/invoices.py:294
get_payment_account's fifth fallback is frappe.db.get_value("Account", {...Cash/Bank...}) with no order_by. It was written for invoice payment display; this PR is the first caller to use its result as the credit side of a submitted JE. Also here: the bare except Exception: return None swallows a specific, actionable error and replaces it with "Payment Account is required", and the resolver runs twice per create.
  Done when: an unresolvable payment account fails loudly with the underlying reason, and no expense can credit a ledger nobody chose.

• M4 — Decide whether non-cash modes should be allowed · api/expenses.py:204
validate_mode_of_payment checks the mode is on the profile and resolves to an account, but not what kind. An expense against a card mode credits the bank account, and make_closing_shift_from_opening then subtracts it from expected card takings.
  Decide: if paying by card is intended, say so in the field help; if not, restrict to account_type = "Cash".

• M5 — Post overnight expenses to the shift's date · api/expenses.py:331
posting_date: today(). A shift opened at 22:00 with an expense at 00:30 posts to the new day while reconciliation still counts it against the shift. Drawer maths holds; GL date and shift date diverge.
  Done when: the posting date derives from the shift, or the divergence is a documented decision.

• M6 — Stop overloading cheque_no · api/expenses.py:334
  The shift is already in posa_pos_opening_shift a few lines below. cheque_no / cheque_date are the JE's bank-reference fields and surface in bank reconciliation views.

• M7 — Handle multi-currency on the Journal Entry · api/expenses.py:350
  Only *_in_account_currency is set — no exchange_rate, no company-currency debit/credit. On a foreign-currency payment account ERPNext fills those from today's rate. The rest of this app takes this seriously; pos_closing_shift.py has a whole get_base_value helper for it.

---

Gate 3 — Tests

17/17 pass and every validator has a real assertion — this is genuinely good unit testing. The gap is that nothing exercises the part that moves money.

• T1 — One test that builds a real Journal Entry · new
  _create_expense_journal_entry is mocked out in the only test that calls create_pos_expense (test_expenses.py:183-211), so nothing verifies that debit and credit balance, that cost_center lands on both rows, M3's account resolution, M5's posting date, or M7's currency handling. One integration test covers all five.

• T2 — Cover the "no maximum" behaviour · test_expenses.py
  A test asserting what happens when posa_maximum_expense_amount is unset would have made B1 obvious at review time. Add it, then change it when B1 is fixed.

• T3 — Replace the two change-detector tests · test_expenses.py:159, 213
  Both assert full assert_called_once_with argument lists, so reordering a fields list breaks them with no bug present. The one at :159 also asserts the permission bypass from H3, which makes that harder to revisit.

• T4 — Cover the reconciliation seam · pos_closing_shift.py
  Nothing tests make_closing_shift_from_opening's expense aggregation — where expenses meet payment_reconciliation and total_pos_expenses. That is also where H4's desync shows up.

  Not asked for, but noted: there are no .spec.js files anywhere on this branch, so ExpenseDialog.vue (363 lines, carrying the client-side limit arithmetic) has no frontend test. The vitest harness arrives in Feature/thin develop split #360 — worth adding one once these land in the same branch.

---

Gate 4 — Cleanups

• L1 — <ExpenseDialog> is mounted without an @expense-created listener, but the component emits it; the emit is dead and nothing refreshes after recording · POSSale.vue:590
• L2 — currency prop defaults to "USD" in a multi-currency POS; require it, or default to the profile currency · ExpenseDialog.vue:146
• L3 — get_expense_accounts uses limit_page_length=0; the dialog already has a search box, so a server-side search query is the natural shape · api/expenses.py:307

---

Gate 5 — Before asking for re-review

• D1 — Rebase on develop. This branch is 17 commits behind (merge base 9212350) and was last touched 2026-09-04. Rebase so the diff under discussion is the diff that will merge.
• D2 — Coordinate the collision with Feature/thin develop split #360. Both PRs are open against develop and both touch POS/components.d.ts, POS/src/components/sale/InvoiceCart.vue, POS/src/pages/POSSale.vue and pos_next/api/bootstrap.py. components.d.ts will conflict outright — both register a new component. Agree which merges first, and re-check that both features still appear in the POS menu afterwards.

---

Definition of done

bench --site <site> run-tests --app pos_next --module pos_next.api.test_expenses
ruff check pos_next/api/expenses.py pos_next/pos_next/report/pos_expense_report/

# the one that proves B1 is fixed:
# enable posa_allow_pos_expense on a profile, leave the maximum blank,
# call create_pos_expense -> must refuse, not succeed

Current baseline for comparison: 17/17 pass, ruff clean. Both were verified on a local bench before this review.

[engahmed1190]

Cross-reference: this PR and #360

Both are open against develop and both touch the same four files, so they need sequencing rather than parallel merges.

Shared files

File	#318 adds	#360 adds
POS/components.d.ts	ExpenseDialog	AuthorizationDialog
POS/src/pages/POSSale.vue	menu button + dialog mount	menu button + dialog mount
POS/src/components/sale/InvoiceCart.vue	expense button	wallet/free-item changes
pos_next/api/bootstrap.py	2 keys on result["pos_profile"]	authorization_policy, authorization_pin_length

components.d.ts will conflict outright — each registers a new component. The other three are textually adjacent rather than contradictory, but all four want a human resolving them.

Suggested order: this PR first.

Not a judgement on quality — it is about how far each one's blocker is from done:

• Pn 65 expenses in pos screen #318 (this PR): the blocker is a config decision — posa_maximum_expense_amount ships with no default and <= 0 is read as "unlimited". Five Gate 1 items, all inside api/expenses.py and one JSON file. Nothing here depends on Feature/thin develop split #360.
• Feature/thin develop split #360: the blocker is architectural — the optional-app gate is evaluated once per worker process against an arbitrary site, so the split's central mechanism does not currently engage. That is a rework, plus two open ownership decisions.

Landing this one first means #360 absorbs the four-file conflict, which is fair: it is the PR staying open longer, and it is already rebasing.

Whoever merges second: resolve all four by hand, then confirm both the POS Expense and the Return authorization entries still appear in the POS menu — the two features add sibling buttons to the same menu and the same dialog mount point, so a bad merge silently drops one.

Both PRs also need a rebase regardless — this one is 17 commits behind develop, #360 is behind its own merge base a07f981.

Reviews and per-PR checklists are in the comments on each. Item counts: 21 here, 29 on #360.

[engahmed1190]

Review board — PRs #318 and #360

Everything open across both PRs, in one place. 50 items. Full detail is in each PR's review and checklist comment; this is the consolidated view.

	Gate 1	Gate 2	Tests	Cleanup + hygiene	Total
#318 POS expenses	5	7	4	5	21
#360 Thin develop split	6	7	5	11	29

---

Merge sequence

First — land #318. Its blocker is a one-line config decision. Five Gate 1 items, all local to one module. Nothing in it depends on #360.

Then — rebase #360 onto it. #360 resolves the four-file collision, since it is the one still needing architectural work. Merging it first would make #318 pay a cost it did not incur.

Throughout — neither ships without its blocker closed. Both blockers are unbounded-authority defects, not polish. Everything else on this board can follow.

---

The two blockers

Everything else here is secondary to these.

#318 — A blank expense maximum means unlimited

Tick one checkbox on a POS Profile and every cashier on it can submit unbounded Journal Entries crediting the till, through a whitelisted endpoint running ignore_permissions=True. posa_maximum_expense_amount ships with no default, and <= 0 is read as "no limit".

custom/pos_profile.json:342 · api/expenses.py:134

#360 — The optional-app gate never engages

Decided once per worker process against whichever site it touched first. Reproduced on a 3-site bench: fleet.localhost, which does not have pos_next installed, decided pos_next's pricing-patch state for posnext.local, which does. The split's central mechanism does not do what it claims.

pos_next/__init__.py:27

---

#318 · POS expenses — 21 items

Closer to mergeable. Validators are thorough and tested; 17/17 pass. The gaps are all downstream of the money leaving.

Gate 1 — Blocking (5)

• B1 Blank maximum must mean "not configured", not unlimited · custom/pos_profile.json:342, api/expenses.py:134
• H1 Lock the shift around check-and-insert · api/expenses.py:127
• H2 Sum the credit rows, not the writable custom field · api/expenses.py:154
• H3 Justify and narrow the two ignore_permissions bypasses · api/expenses.py:273, 307
• H4 Add a cancel path, refused once the shift closes · api/expenses.py:368

Gate 2 — Correctness (7)

• M1 Scope the report by company; fix or drop the inert cashier role · pos_expense_report.py:96
• M2 Link the closing-shift row to its Journal Entry; Data → Link · pos_closing_shift_expense.json
• M3 Stop resolving the payment account to an arbitrary ledger · api/expenses.py:249
• M4 🔀 decide — whether non-cash modes may fund an expense · api/expenses.py:204
• M5 Post overnight expenses to the shift's date · api/expenses.py:331
• M6 Stop overloading cheque_no to carry the shift · api/expenses.py:334
• M7 Handle multi-currency on the Journal Entry · api/expenses.py:350

Gate 3 — Tests (4)

• T1 One test that builds a real Journal Entry — covers M3, M5, M7 at once
• T2 Cover the "no maximum" behaviour that hid B1
• T3 Replace the two change-detector tests · test_expenses.py:159, 213
• T4 Cover the reconciliation seam in make_closing_shift_from_opening

Gate 4–5 — Cleanup & hygiene (5)

• L1 Dead @expense-created emit — nothing refreshes · POSSale.vue:590
• L2 Hardcoded "USD" currency default · ExpenseDialog.vue:146
• L3 Unbounded account list — server-side search instead · api/expenses.py:307
• D1 Rebase — 17 commits behind develop
• D2 Coordinate the four-file collision with Feature/thin develop split #360

---

#360 · Thin develop split — 29 items

Further out. The auth gate underneath is well built; the problems are at its edges and in the split mechanism itself.

Gate 1 — Blocking (6)

• B1 Move the optional-app gate to call time; delete the duplicate helper · pos_next/__init__.py:27
• H2 TTL and invalidation on the wallet-mode cache · api/wallet.py:182
• H3 Stop OR-ing company and transaction currency · overrides/sales_invoice.py:124
• H4 Show the approver the action, amount and invoice · AuthorizationDialog.vue:15
• H5 Key the rate limit on approver, not IP · api/authorization.py:95
• M1 Close self-service PIN enrolment · api/authorization.py:163

Gate 2 — Correctness & decisions (7)

• M3 🔀 decide — who owns the authorization gate · POS/src/utils/promoApi.js:11
• M6 🔀 decide — move Gift Pool and GWP out of the core cart · posCart.js:1134–1330
• M2 Create the two audit fields, or stop writing to them · authorization/gate.py:102
• M4 Move payment-account validation after super().validate() · overrides/sales_invoice.py:76
• M5 Stop silently re-attributing invoices to the walk-in customer · overrides/sales_invoice.py:90
• M8 Wire enforce_context to a real action, or delete it · authorization/gate.py:64
• M9 pickle → json; stop the grant TTL refreshing itself · authorization/grants.py:40, 96

Gate 3 — Tests (5)

• T1 A test that would have caught B1
• T2 Cover overrides/sales_invoice.py — currently zero
• T3 Pin the GWP carve/restore round trip
• T4 Replace the smoke-test assertions that cannot fail
• T5 Prove the offline cart and server agree on gifts

Gate 4–5 — Cleanup & hygiene (11)

• L1 PIN removal logged as RESULT_PIN_SET · api/authorization.py:185
• L2 Unreachable branch in _resolve_leaf_territory · api/customers.py:229
• L3 Failing external sync rolls back a created customer · integrations/registry.py:61
• L4 Redundant sync_return_loyalty_program in on_submit
• L5 Smoke runner hardcodes site brainwise.dev
• L6 Note the memory ceiling on the Item Price snapshot
• L7 Clear ruff on the changed files
• L8 Confirm the deleted packed-bundle merge is covered
• D1 Write the PR description — body is empty
• D2 Document the fail-closed return behaviour and the 180s TTL
• D3 Rebase on develop

---

The collision

Both PRs touch POS/components.d.ts, POS/src/components/sale/InvoiceCart.vue, POS/src/pages/POSSale.vue and pos_next/api/bootstrap.py. The first will conflict outright — each registers a new component. Whoever merges second resolves all four by hand and re-checks that both features still appear in the POS menu.

Four decisions, not patches

Marked 🔀 above. Each wants a person, and each belongs in the PR description once settled.

1. Feature/thin develop split #360 — who owns the authorization gate (pos_next or the satellite).
2. Feature/thin develop split #360 — whether Gift Pool and GWP belong in the core cart, given offerStrategies.js exists for exactly that.
3. Pn 65 expenses in pos screen #318 — whether non-cash modes may fund an expense.
4. Pn 65 expenses in pos screen #318 — what a blank expense maximum should mean.

What I withdrew

Two findings did not survive checking, and are deliberately not on this board:

• Feature/thin develop split #360 — get_invoices. I reported it as a regression that lost search, date filters and the page-size clamp. Wrong: I diffed develop..pr360 instead of the merge base. Commit 98b39e5 improved it on develop after this branch point; the PR never touched it and the rebase will take develop's version.
• Pn 65 expenses in pos screen #318 — the report's POSNext Cashier grant. It looked like cross-company exposure. It isn't: frappe.desk.query_report.run also checks has_permission("Journal Entry", "report") (frappe/desk/query_report.py:207), and that role holds no Journal Entry permission on this bench, with none added by the shipped fixtures. A cashier gets refused. The inert role grant is filed as a config bug inside M1; the real half is the missing company scoping.

---

#318 @ 04034eb (merge base 9212350) · #360 @ 5d4c16f (merge base a07f981). Verified on a local bench: Frappe + ERPNext, 3 sites, dns_multitenant. #318 backend 17/17 pass; #360 vitest 12/12 pass; ruff and biome run on both.

[MohamedAliSmk]

D2 closed — collision with #360

Merge order (agreed): land #318 first, then rebase #360 onto develop.

Same rationale as the review board: #318’s remaining work is local; #360 is the longer-lived architectural PR and should absorb the four-file resolve.

Shared files — keep both sides

File	Keep from #318	Keep from #360
POS/components.d.ts	ExpenseDialog	AuthorizationDialog
POS/src/pages/POSSale.vue	header POS Expense + <ExpenseDialog> mount + @show-expense / :allow-pos-expense	:company="shiftStore.profileCompany" on InvoiceCart
POS/src/components/sale/InvoiceCart.vue	empty-cart POS Expense button + allowPosExpense + show-expense emit	wallet / free-item / promo cart changes
pos_next/api/bootstrap.py	posa_allow_pos_expense, posa_maximum_expense_amount on pos_profile	system_timezone, authorization_policy, authorization_pin_length, extend_bootstrap_settings

components.d.ts will conflict outright (both register a new component). Resolution is additive: both lines stay.

Post-merge menu re-check (owner of #360 after rebase)

1. POS Expense still in the header ⋮ menu (gated by canRecordPosExpense).
2. POS Expense still on the InvoiceCart empty-cart grid (gated by allowPosExpense).
3. Return Invoice still on that same grid — Feature/thin develop split #360 does not add a sibling menu entry; it gates the existing Return path via AuthorizationDialog mounted in App.vue.
4. Management slider still shows Promotions (and Product Management / Stock Lookup as on develop).

Correction to the earlier cross-ref wording: current #360 only adds :company in POSSale.vue; the auth dialog mount is in App.vue, not a second POSSale menu button. The failure mode to watch is still the same — a bad resolve that drops Expense wiring or leaves Return unwired to requireAuthorization.

[engahmed1190]

Re-review — PR #318 "POS Expenses in POS Screen"

Head 7818730 · merge base d02f852 (current develop tip) · reviewed 2026-09-09
Follow-up to review #5142106656 (21 items).

---

Decision

Not yet — two things to fix, both small. 18 of 21 items are genuinely closed.

The security work asked for is done and done well. What blocks is not a design
gap, it is that one fix references a database column that does not exist, and
the four integration tests written to close T1/T4 do not run.

	Count	Items
✅ Closed, verified	18	B1 H1 H2 H3 H4 · M1 M2 M4 M5 M6 M7 · T2 T3 · L1 L2 L3 D1 D2
⚠️ Partial	3	M3, T1, T4
❌ Not started	0	—

To close:

1. POS Payment Method.default_account does not exist — api/expenses.py:383 and :394. Every unconfigured-mode-of-payment call raises a raw SQL error instead of the friendly setup message the M3 fix was written to produce. This is what makes T1 error. (N1)
2. The cashier types profile currency; the server books company currency — ExpenseDialog.vue:208 vs expenses.py:525. On any profile whose currency differs from its company's, both the posted amount and the shift limit are wrong by the exchange rate. (N6)
3. The T1/T4 integration tests create no fixtures — they reference _Test Company and a POS Profile that only test_promotions.py builds, and error on any clean bench. (N2)
4. Split out the unrelated commits — ~350 lines of PR #359's offline batch/serial work and EOD-report changes ride along in this branch. (N3)

---

Measured, on a live bench

Frappe 15.111.1 + ERPNext 15.111.0, site posnext.local, MariaDB.
Prior review's published baseline: 17/17 pass, ruff clean.

Gate	Baseline	Now
pos_next.api.test_expenses	17/17 pass	26 tests — 25 pass, 1 error
...pos_closing_shift.test_pos_closing_shift	not covered	11 tests — 8 pass, 3 error
...utils.tests.test_pos_closing_print	—	5/5 pass
ruff check (changed .py)	clean	1 error (I001, test_pos_closing_shift.py:24)
Rebase (D1)	17 behind	0 behind / 20 ahead of d02f852 ✅

Test count nearly doubled — T1–T4 did add real coverage. The four that error are
the four that matter most, and they error for fixable reasons, not design ones.

bench run-tests --app pos_next (whole app) cannot collect at all: test_bank_deposits.py:4
imports IntegrationTestCase, which does not exist in Frappe v15. Pre-existing on
develop, not this PR — but it means per-module runs are the only option today.

---

Item-by-item

Gate 1 — Blocking · 5/5 closed

	Verdict	Evidence
B1 blank maximum ≠ unlimited	✅	custom/pos_profile.json — default: "0", description now reads "Blank or zero means not configured and blocks expense recording (not unlimited)." expenses.py:213-221 throws on <= 0. Client mirrors it at ExpenseDialog.vue:437. Test test_validate_expense_amount_rejects_unconfigured_limit.
H1 lock check-and-insert	✅	expenses.py:229-239 — frappe.db.get_value("POS Opening Shift", …, for_update=True) before the SUM, in the same request transaction as the JE insert/submit. Frappe emits a blocking SELECT … FOR UPDATE (frappe/database/query.py:83-84). Comment names the reason. Test asserts lock precedes SUM.
H2 sum credit rows	✅	get_shift_expense_total (:253) sums jea.credit over Journal Entry Account. get_pos_expenses (:638) and pos_expense_report.py:133 do the same. posa_expense_amount is still written but no longer authoritative anywhere.
H3 justify + narrow the bypasses	✅	Justified in the PR description (table), the module docstring (:8-15), and per-function docstrings. Accounts capped at EXPENSE_ACCOUNT_PAGE_LENGTH = 50 with search_expense_accounts for server-side search; employees capped at 200 and return only name/employee_name.
H4 cancel path	✅	cancel_pos_expense (:106-160) — validates enabled, open shift, ownership, posa_is_pos_expense, shift match, profile match, docstatus == 1, and creator-or-cancel-permission. validate_open_shift requires status == "Open", so it is refused once closed.

Gate 2 — Correctness · 6/7 closed

	Verdict	Evidence
M1 report company scoping	✅	apply_company_scope (pos_expense_report.py:157-173) — company required (throws), defaults to user default, checked against get_permitted_documents("Company") before the SQL. Roles reduced to Accounts Manager / System Manager / Nexus POS Manager; cashier + Sales Manager dropped. A Journal Entry read+report Custom DocPerm for Nexus POS Manager was added and hooks.py fixture filter widened to export it — so the grant now actually works.
M2 audit-record child table	✅	pos_closing_shift_expense.json — journal_entry → Link (Journal Entry), expense_account → Link (Account), employee → Link (Employee). pos_closing_shift.py:642 stores journal_entry. ShiftClosingDialog.vue:566-584 shows the voucher column.
M3 payment-account resolution	⚠️ partial	See N1. The arbitrary Cash/Bank fallback is gone ✅, the error message is informative ✅, and the resolver runs once ✅ (validate_mode_of_payment returns the account). But the two remaining fallbacks read a field that does not exist, so the intended throw never happens — a SQL error happens instead.
M4 non-cash modes — decision	✅	Decided: Cash only. validate_mode_of_payment:325-340 throws unless account_type == "Cash", naming the mode, the account and its type. get_cash_payment_methods filters the dialog the same way.
M5 overnight posting date	✅	_shift_posting_date(period_start_date) (:515), fed from shift.period_start_date. Falls back to today() only when absent.
M6 stop overloading cheque_no	✅	No cheque_no / cheque_date anywhere in the module. Shift lives only in posa_pos_opening_shift.
M7 multi-currency	✅	_account_row_amounts (:522-573) sets account_currency, exchange_rate, company-currency debit/credit and *_in_account_currency; sets the JE multi_currency flag; throws when a rate cannot be found. The item as written is closed. But the input to it has an unstated basis — see N6.

Gate 3 — Tests · 2/4 closed

	Verdict	Evidence
T1 real Journal Entry test	⚠️ partial	TestPOSExpenseJournalEntry exists (test_expenses.py:398) and asserts the right things. It errors: OperationalError (1054) Unknown column 'ppm.default_account' at test_expenses.py:437 → expenses.py:391. Root cause N1. It also creates no fixtures (N2).
T2 cover "no maximum"	✅	test_validate_expense_amount_rejects_unconfigured_limit — passes.
T3 replace change-detectors	✅	test_get_active_employees_scoped_to_company now asserts company/status filters and that a finite page cap exists, not a full kwarg list. The ignore_permissions assertion is gone, so the bypass is no longer pinned by a test.
T4 reconciliation seam	⚠️ partial	Three well-aimed tests exist (test_pos_closing_shift.py:373, 395, 414) covering totals, the H4 cancel desync, and multi-expense aggregation. All three error on LinkValidationError: Could not find Company: _Test Company, POS Profile: _PNXT_TEST_POS_PROFILE__Test Company (N2).

Gate 4–5 — Cleanup & hygiene · 5/5 closed

	Verdict	Evidence
L1 dead emit	✅	POSSale.vue:599 wires @expense-created="handleExpenseCreated", plus @expense-cancelled. Handlers are log-only by design — the dialog reloads its own data — and say so.
L2 hardcoded "USD"	✅	ExpenseDialog.vue:208 — props.currency || shiftStore.profileCurrency. Prop documented as "prefer passing explicitly". Formatting moved to the shared utils/currency helper.
L3 unbounded account list	✅	Capped at 50 + search_expense_accounts endpoint + debounced @search in the dialog (:414).
D1 rebase	✅	0 behind / 20 ahead of d02f852. Verified against a fresh fetch — GitHub's own file list for this PR is stale and still shows the pre-rebase shape.
D2 collision with #360	✅	Merge order agreed (318 first), per-file keep-both table posted, plus the hooks.py fifth file the first review missed.

---

New findings

Confined to code the fixes introduced.

N1 — POS Payment Method.default_account does not exist · blocking · api/expenses.py:383, 394

The M3 rewrite resolves a payment account in three steps. Steps 2 and 3 both read
default_account off POS Payment Method — a field that is not on that DocType:

DESCRIBE `tabPOS Payment Method`
→ name, creation, modified, modified_by, owner, docstatus, idx,
  default, allow_in_returns, mode_of_payment, parent, parentfield, parenttype

ERPNext v15's pos_payment_method.json declares exactly three fields: default,
mode_of_payment, allow_in_returns. pos_next adds no custom field for it.

So the moment step 1 (Mode of Payment Account) misses — which is precisely the
misconfiguration M3 asked to handle loudly — the caller gets a raw SQL error instead
of "Please set default Cash account in Mode of Payment X for company Y." The friendly
frappe.throw at :401 is unreachable. Both steps were confirmed on the bench:

# step 2 (:383) — the production path, since validate_mode_of_payment always passes pos_profile
frappe.db.get_value("POS Payment Method", {...}, "default_account")
→ OperationalError: (1054, "Unknown column 'default_account' in 'SELECT'")

# step 3 (:394) — reached when pos_profile is None, which is how the T1 test calls it
→ OperationalError: (1054, "Unknown column 'ppm.default_account' in 'SELECT'")

Not a regression — the pre-existing api/invoices.py:312 carries the same broken
SQL, latent for the same reason. But M3 asked for this path to fail loudly with the
underlying reason, and it does not.

Done when: steps 2 and 3 are deleted (leaving Mode of Payment Account → throw),
or repointed at a field that exists. Deleting them is the right call, not just the
smaller one: get_cash_payment_methods builds the dialog's mode list from
pos_profile.get_payment_methods, which resolves account_type through a join on
Mode of Payment Account alone. A mode with no row there never reaches the dialog, so
steps 2 and 3 are unreachable from the only caller that matters — they can only ever
fire on the error path they break.

Also here: the comment at :390 says the third fallback is "when pos_profile was
not passed", but it runs whenever step 2 misses, and its LIMIT 1 has no ORDER BY
— the same non-determinism M3 asked to remove, narrowed from all Cash/Bank ledgers to
all POS Profiles in the company. Deleting it closes this too.

N2 — The T1/T4 integration tests build no fixtures · blocking · test_expenses.py:398, test_pos_closing_shift.py:310

Both new classes declare their dependencies as bare string constants and have only a
tearDown that rolls back — no setUpClass, no record creation:

COMPANY = "_Test Company"
EXPENSE_ACCOUNT = "Travel Expenses - _TC"
COST_CENTER = "Main - _TC"
PROFILE = "_PNXT_TEST_POS_PROFILE__Test Company"

_PNXT_TEST_POS_PROFILE__Test Company is created in exactly one place in this repo —
a local helper inside pos_next/test_promotions.py:304. Nothing in either new module
calls it. So these four tests pass only if an unrelated module happened to run first
in the same session and leave records behind.

On a clean bench all four error before reaching an assertion. That means T1 and T4
cannot yet demonstrate what they were written to demonstrate — the money path and the
reconciliation seam are still unproven by a green run.

Done when: each class creates its own company / accounts / cost center / POS Profile
in setUpClass (or reuses the test_promotions.py helper), and both modules pass from
a clean database.

N3 — Unrelated work rides along in this branch · should be split

Eight files and roughly 350 lines have nothing to do with expenses:

Commit	Brings in	Files
8231e26	PR #359's offline batch/serial work — still open separately	BatchSerialDialog.vue, itemSearch.js, serialNumber.js, offline/items.js (+111)
05a7b28 59acd2f 9e9c26f	EOD-report fixes + translations	pos_next_eod_report.json, pos_closing_print.py, test_pos_closing_print.py, 3 × .csv
f5ae353	"track collected money alongside invoiced totals"	ShiftClosingDialog.vue, pos_closing_shift.py

Three of #359's four files are byte-identical to that PR's head
(offline/items.js, serialNumber.js, BatchSerialDialog.vue — verified by hash).
Whichever of #318 / #359 lands second will now conflict on files neither review
covered, and #359's review no longer governs code that ships through #318.

Done when: those commits are dropped from this branch and land through their own PRs
— or the PR description says they are deliberately bundled and names who reviewed them.

N6 — The amount the cashier types has no agreed currency · blocking · ExpenseDialog.vue:208 ↔ api/expenses.py:525

H2 and M7 are each internally correct, and they agree with each other — the limit sums
jea.credit in company currency, and _account_row_amounts treats the incoming amount
as company currency. The gap is at the boundary: nothing establishes that the number
the cashier typed is in company currency.

• POSSale.vue:597 passes :currency="shiftStore.profileCurrency".
• posShift.js:17 — profileCurrency = currentProfile.currency, i.e. POS Profile.currency, a reqd: 1 Link on the ERPNext DocType that is set independently of Company.default_currency.
• ExpenseDialog.vue:208-211 formats the limit, the recorded total, the remaining allowance and every listed expense with that currency.
• The server books the same number as company currency and enforces the limit in company currency.

On a profile whose currency differs from its company's, a cashier reading "Shift limit:
500" and typing 100 in what the UI labels their profile's currency has 100 company
currency debited to the expense account — and the limit they are measured against is
500 company currency, not the 500 they were shown. Both the GL amount and the bound are
off by the exchange rate.

posa_maximum_expense_amount is a Currency field with options: null, so it declares
no currency basis at all — the desk renders it in the system default, which is a third
possible currency.

This is the one seam the multi-currency work did not reach, and it is only reachable in
exactly the multi-currency setups M7 was written for.

Done when: the basis is stated and enforced in one direction — simplest is to label
the dialog and the field in company currency (fetch Company.default_currency into
the dialog rather than using profileCurrency, and set the custom field's options to a
company-currency field). Converting the input instead is also valid, but then the limit
comparison has to convert too.

N4 — ruff regression · test_pos_closing_shift.py:24

I001 unsorted-imports. The prior review's baseline was ruff-clean on every changed
file. ruff check --fix closes it.

N5 — timer not cleared on unmount · minor · ExpenseDialog.vue:414

accountSearchTimer is cleared on each new keystroke but never on unmount, so a
dialog closed within 250 ms of typing still fires one search. Harmless — the resource
call is guarded — but onUnmounted(() => clearTimeout(accountSearchTimer)) is one line.

---

What got noticeably better

Worth saying plainly, because most of the 21 came back well done:

• H1's lock is correct, not cargo-culted — it is taken on the right row, before the
  read, in the same transaction as the write, and the comment explains why it exists.
• H2 and M7 agree with each other server-side. Summing credit in company currency
  and treating the JE amount as company currency means the shift limit and the GL measure
  the same thing — two fixes landed by different commits that could easily have
  contradicted. N6 is the boundary they share not being nailed down, not either being wrong.
• M4 was answered as a decision, not patched around — Cash-only, enforced server-side,
  mirrored in the dialog, with the reason in the error message.
• T3 was actually re-thought, not edited. The tests now assert intent (company scope,
  a finite cap) rather than argument lists, and they no longer pin the permission bypass.
• D1 is fully current against a develop that moved twice during the review window.

The remaining work is two mechanical fixes and a branch split. Nothing here needs a
redesign.

---

Caveats on this review

• _Test Company could not be created on this bench — ERPNext's Company test record
  provisions an Email Account and the sandbox has no outbound network
  (Error connecting via IMAP/POP3). So N1 is proven (column absent, per DESCRIBE
  and the failing test) and N2 is proven (no fixture creation, per the source), but
  whether T1/T4 would pass given fixtures is undetermined here. Someone on a normal
  bench should confirm after N1 is fixed.
• Frontend has no vitest run in this branch; ExpenseDialog.vue (518 lines, carrying
  the client-side limit arithmetic) remains untested. The harness arrives in #360 — the
  first review noted this and it is still worth one spec once the branches meet.
• Bench changes made to run this review (posnext.local, local only): installed
  erpnext, set allow_tests true, and ran ERPNext's test_journal_entry once to seed
  test records. _Test Company could not be seeded — see above. The repo was left on
  develop with no modifications; refs pr318-fresh / pr359-fresh remain fetched.

[engahmed1190]

Task list — after re-review at 7818730

Re-reviewed against merge base d02f852 (current develop tip). The 21 items from
review #5142106656
are 18 closed, 3 partial, 0 untouched. Test count went 17 → 26 and the rebase is fully current.

6 items open — 4 must close before merge (Gate 1), 2 cleanup. Same format as last time:
each names the file and the condition that closes it.

---

Gate 1 — Blocking

• N1 — POS Payment Method.default_account does not exist · api/expenses.py:383, :394
  M3's rewrite resolves a payment account in three steps. Steps 2 and 3 read default_account
  off POS Payment Method; ERPNext v15 declares only default, mode_of_payment,
  allow_in_returns, and pos_next adds no custom field. Both confirmed on a bench:

  step 2 (:383) → OperationalError (1054, "Unknown column 'default_account' in 'SELECT'")
  step 3 (:394) → OperationalError (1054, "Unknown column 'ppm.default_account' in 'SELECT'")
  

  Step 2 is the production path (validate_mode_of_payment always passes pos_profile). So the
  exact misconfiguration M3 asked to fail loudly now raises a raw SQL error, and the friendly
  frappe.throw at :401 is unreachable. This is also why T1 errors.
  Done when: steps 2 and 3 are deleted, leaving Mode of Payment Account → throw. Deletion is
  the right call, not just the smaller one — get_cash_payment_methods builds the dialog's mode
  list from pos_profile.get_payment_methods, which resolves account_type through a join on
  Mode of Payment Account alone. A mode with no row there never reaches the dialog, so these two
  steps can only ever fire on the error path they break.

• N6 — the amount the cashier types has no agreed currency · ExpenseDialog.vue:208 ↔ api/expenses.py:525
  H2 and M7 are each right and they agree with each other. The gap is at their shared boundary:
  nothing establishes that the typed number is in company currency.
  POSSale.vue:597 passes :currency="shiftStore.profileCurrency" → posShift.js:17 →
  POS Profile.currency, a reqd: 1 Link set independently of Company.default_currency.
  The dialog labels the limit, the recorded total, the remaining allowance and every listed
  expense with it — while the server books the same number as company currency and enforces the
  limit in company currency.
  On a profile whose currency differs from its company's, a cashier shown "Shift limit: 500" who
  types 100 gets 100 company currency debited, measured against 500 company currency. Both
  the GL amount and the bound are off by the exchange rate. Reachable only in the multi-currency
  setups M7 was written for. posa_maximum_expense_amount also carries options: null, so it
  declares no currency basis at all.
  Done when: the basis is stated and enforced one way. Simplest: label the dialog and the field
  in company currency (fetch Company.default_currency instead of profileCurrency, set the
  custom field's options). Converting the input instead is valid too — but then the limit
  comparison has to convert as well.

• N2 — the T1/T4 integration tests build no fixtures · test_expenses.py:398, test_pos_closing_shift.py:310
  Both new classes declare _Test Company, Travel Expenses - _TC, Main - _TC and
  _PNXT_TEST_POS_PROFILE__Test Company as bare constants and have only a tearDown that rolls
  back — no setUpClass, no record creation. That profile name is built in exactly one place in
  this repo: a local helper in pos_next/test_promotions.py:304, which neither module calls.
  So all four pass only if an unrelated module happened to run first and leave records behind. On
  a clean bench they error before reaching an assertion — which means T1 and T4 cannot yet
  demonstrate the money path or the reconciliation seam.
  Done when: each class creates its own company / accounts / cost center / POS Profile in
  setUpClass (or reuses the test_promotions.py helper), and both modules pass from a clean DB.

• N3 — unrelated work rides along in this branch
  Eight files, ~350 lines, nothing to do with expenses:

  Commit	Brings in	Files
  8231e26	PR feat: enhance offline batch and serial number management #359's offline batch/serial work — still open separately	BatchSerialDialog.vue, itemSearch.js, serialNumber.js, offline/items.js (+111)
  05a7b28 59acd2f 9e9c26f	EOD-report fixes + translations	pos_next_eod_report.json, pos_closing_print.py, test_pos_closing_print.py, 3 × .csv
  f5ae353	"track collected money alongside invoiced totals"	ShiftClosingDialog.vue, pos_closing_shift.py

  Three of feat: enhance offline batch and serial number management #359's four files are byte-identical to that PR's head (verified by hash). Whichever
  of Pn 65 expenses in pos screen #318 / feat: enhance offline batch and serial number management #359 lands second now conflicts on files neither review covered, and feat: enhance offline batch and serial number management #359's review no
  longer governs code shipping through Pn 65 expenses in pos screen #318.
  Done when: those commits are dropped and land through their own PRs — or the description says
  they are deliberately bundled and names who reviewed them.

---

Gate 2 — Cleanup

• N4 — ruff regression · test_pos_closing_shift.py:24
I001 unsorted-imports. Last review's baseline was ruff-clean on every changed file.
  ruff check --fix closes it.

• N5 — timer not cleared on unmount · ExpenseDialog.vue:414
accountSearchTimer is cleared on each keystroke but never on unmount, so a dialog closed within
  250 ms of typing still fires one search. Harmless — the resource call is guarded — but
  onUnmounted(() => clearTimeout(accountSearchTimer)) is one line.

---

Closed last round — no action

B1 H1 H2 H3 H4 · M1 M2 M4 M5 M6 M7 · T2 T3 · L1 L2 L3 D1 D2

M3, T1 and T4 are the three partials, tracked above as N1 and N2.

---

Definition of done

# both must pass from a clean database, not just a warm bench
bench --site <site> run-tests --app pos_next --module pos_next.api.test_expenses
bench --site <site> run-tests --app pos_next --module pos_next.pos_next.doctype.pos_closing_shift.test_pos_closing_shift

ruff check $(git diff --name-only $(git merge-base HEAD upstream/develop)..HEAD | grep '\.py$')

# the one that proves N1 is fixed: a Mode of Payment with no Mode of Payment Account row
# -> must throw "Please set default Cash account in Mode of Payment X", not OperationalError 1054

Current: 26 tests / 25 pass / 1 error (test_expenses), 11 / 8 / 3 error
(test_pos_closing_shift), 1 ruff error. Measured on Frappe 15.111.1 + ERPNext 15.111.0.

bench run-tests --app pos_next (whole app) cannot collect at all — test_bank_deposits.py:4
imports IntegrationTestCase, absent in Frappe v15. Pre-existing on develop, not this PR,
but it means per-module runs are the only option today.

[engahmed1190]

Still open at 3ab88dc — two items

Re-checked against head 3ab88dc, merge base d02f852. N1, N2, N4, N5 are closed, and
M3 / T1 / T4 close with them. The gate is green on a live bench:

Gate	At 7818730	At 3ab88dc
pos_next.api.test_expenses	25 pass, 1 error	27/27 pass
...pos_closing_shift.test_pos_closing_shift	8 pass, 3 error	11/11 pass
...utils.tests.test_pos_closing_print	5/5	5/5
ruff check (21 changed .py)	1 error (I001)	1 error — pre-existing on develop, see below

Two things are not done. Neither is a redesign.

---

1. N6 is not closed — the store rung it added is inert · POS/src/stores/posShift.js:19

640533d made the right call and recorded it in three places (field options, both
docstrings, the error formatting). Server side is correct. The client wiring is not:

const companyCurrency = computed(
    () => currentProfile.value?.company_currency || profileCurrency.value,
)

currentProfile is not the bootstrap payload. It is shiftState.value.pos_profile
(useShift.js:157), and every writer of shiftState (useShift.js:32, 50, 68, 100, 139)
is fed by shifts.check_opening_shift or shifts.create_opening_shift — both of which
return frappe.get_doc("POS Profile", …) (shifts.py:95, :142). A POS Profile document
has no company_currency field. The key bootstrap.py:89-99 adds lands in a different
store (stores/bootstrap.js:59) that nothing writes into shiftState.

So company_currency is always undefined, and companyCurrency always evaluates to
profileCurrency — POS Profile.currency, the exact value N6 said must not be the basis.
POSSale.vue:597 passes that into a prop documented as "Company.default_currency … not
POS Profile.currency."

Not user-visible today, and worth saying plainly: ExpenseDialog's currency prefers
dialogDataResource.data?.company_currency (:215-222), which is server-authoritative and
correct, and the whole form sits behind v-else on dialogDataResource.loading
(ExpenseDialog.vue:4 / :9) so no label paints before it resolves. The defect is that the
two fallback rungs behind it are wrong, under a name and a docstring that say they are
right — which is how this comes back later.

Done when: companyCurrency reads a source that actually carries it. shiftState.company
is already a full Company document from all three endpoints:

const companyCurrency = computed(
    () => shiftState.value.company?.default_currency || profileCurrency.value,
)

One line, and it covers the online path, the offline-restore path and create_opening_shift.

---

2. N3 — the branch split, and a4f6ffd made it bigger

PR description is unchanged, and the commits are still here. Neither half of the "done when"
happened.

Commit	Brings in	Lines
8231e26	PR #359's offline batch/serial — 3 of its 4 files still byte-identical to that PR's head	+161
05a7b28 59acd2f 9e9c26f	EOD-report fixes + translations	~+12 + 60 CSV
f5ae353	collected-money alongside invoiced totals	ShiftClosingDialog.vue, pos_closing_shift.py
a4f6ffd	new — repo-wide ruff --fix across 10 more unrelated files	±31

a4f6ffd ("ruff check fixes") touches 11 files; 10 have nothing to do with expenses —
credit_sales.py, invoices.py, offers.py, partial_payments.py (36 lines),
pos_profile.py, shifts.py, install.py, overrides/pricing_rule.py,
test_packed_items_regression.py, test_promotions.py. Dict→dict, str(e)→{e!s},
import reordering, in modules several other open PRs own. N4 asked for one file to be
import-sorted; the fix brought ten passengers.

The churn itself is safe — I imported the eight non-test modules under site context and each
loads clean, and both except … as e → except … rewrites (partial_payments.py:319,
:469) have bodies that use frappe.get_traceback(), not e.

One of them is a real bug fix and should not ship as formatting: pos_profile.py:7 adds
from frappe.utils import cint. Ruff does not add imports — that is a hand fix for the F821
it flagged, and cint is genuinely undefined at pos_profile.py:155 on develop. A live
NameError in allow_credit_sale resolution, unrelated to expenses, currently shipping only
through this PR.

Done when: those commits land through their own PRs — or the PR description says they are
deliberately bundled and names who reviewed them. If the sweep lands separately, call the
cint fix out wherever it goes.

---

Notes, not blockers

• The one remaining ruff error is not yours. pos_next/api/invoices.py:1669 UP038 is on
  develop too — running ruff against d02f852:pos_next/api/invoices.py reports 3 errors
  there vs 1 now. It only appears in the changed-file list because the sweep pulled
  invoices.py into this diff. N4 itself is closed — test_pos_closing_shift.py:24 is clean.
• D2: 640533d adds company_currency to bootstrap.py's result["pos_profile"] — the
  dict flagged as Feature/thin develop split #360's collision point. Merge order is already agreed (318 first), so nothing
  changes; the keep-both table just understates the surface by one key now.
• bench --site … execute frappe.modules.utils.sync_customizations fails on develop too:
  POS Closing Shift: Options must be a valid DocType for field Bank Deposit —
  custom/pos_closing_shift.json:13 points at "Bank Deposits". Pre-existing at d02f852,
  same family as the test_bank_deposits.py:4 → IntegrationTestCase collection failure that
  still blocks run-tests --app pos_next. Not this PR, but nobody owns it.
• ensure_pos_expense_fixtures() commits (:147) so tearDown does not roll the fixtures
  away — deliberate, and test_promotions.py:530 already does the same. Worth knowing: on a
  site with no _Test Company it creates one (India / INR, :17-38) and commits it uncleaned.
  Reachable only from the two setUpClass methods, so running the expense tests is the only
  trigger.

---

What came back well

• N1 was fixed the larger way. The review offered "delete them or repoint them"; you deleted
  both rungs and the now-unused pos_profile parameter, and left a docstring saying why those
  fallbacks could never have fired. The dead LIMIT 1-without-ORDER BY went with them. That
  version does not grow back.
• N2 was solved at the right altitude. The obvious fix was four inlined get_doc calls per
  setUpClass. Instead the fixtures went into one module that reuses the promotions helpers —
  same profile naming, same _resolve_* functions — so the two suites cannot drift. Proven here,
  not assumed: this bench had only Wind Power LLC (abbr WP), no _Test Company and no _TC,
  before the run — and _Test Company existed after it. The fixture succeeds on the exact insert
  path that defeated ERPNext's own Company test record last time.
• The test count moved for the right reason. 26 → 27, with the four previously-erroring tests
  now green: the money path and the reconciliation seam are demonstrated, not claimed.

[MohamedAliSmk]

@engahmed1190
Cleaning this up means dropping the unrelated riders from #318 (offline batch/serial → #359, EOD/translations → #275, collected totals already on develop via #312, plus the a4f6ffd ruff sweep except the real cint fix in pos_profile.py, which will go in its own small PR).

That will look like a large force-push / history rewrite on PN-65-Excepenses-In-Pos-Screen: many files leave the diff, commit SHAs change, and GitHub may re-request review. The expense feature itself is not being redesigned — the PR is just being narrowed to expenses-only.

Happy to proceed once you’re OK with that churn, or we can document deliberate bundling in the PR description instead if you prefer not to rewrite.

[engahmed1190]

@MohamedAliSmk — thanks for laying out the options rather than guessing. Two things: N7 is confirmed closed, and the answer on N3 is please proceed with the narrowing force-push.

---

N7 — closed at b8dc9a3

const companyCurrency = computed(
    () => shiftState.value.company?.default_currency || profileCurrency.value,
);

Verified across every writer of shiftState, not just the happy path:

Path	useShift.js	Carries company
check_opening_shift success	:32	Full Company doc (shifts.py:96) ✅
Offline restore from localStorage	:68	Same doc, serialized ✅
create_opening_shift success	:100	Full Company doc (shifts.py:143) ✅
No open shift / after closing	:50, :139	null — no dialog reachable in these states ✅

shiftState was already imported at posShift.js:1, so the fix needed no new wiring. The
typed amount, the shift limit and the JE booking now share one documented basis end to end.
N6 is closed.

Two cosmetic leftovers, noted for completeness only — neither is worth a commit on its own,
fold them in if the branch is being rewritten anyway:

• bootstrap.py:89-99's company_currency key is now dead. getPreloadedPOSProfile()
  (stores/bootstrap.js:103) is exported but never called, so nothing consumes it.
• ExpenseDialog.vue:214's comment still reads "prop/bootstrap next"; the prop now comes
  from the store via shiftState, not from bootstrap.

---

N3 — proceed with the split

Your mapping is right, and one part of it is stronger than the review credited. I checked
the claim about collected totals:

$ git log --oneline develop --grep="collected" -i
48ce436 fix: track collected money alongside invoiced totals, not instead of them

f5ae353 on this branch carries the identical commit subject. That rider is not merely
misplaced — it is already on develop via #312. Keeping it guarantees a conflict for whoever
rebases next; that is a correctness argument, not a tidiness one.

Three reasons the churn is worth paying now rather than later:

1. f5ae353 is a verified duplicate of develop. It cannot merge cleanly twice.
2. Three of feat: enhance offline batch and serial number management #359's four files are byte-identical to that PR's head. Landing them through
   Pn 65 expenses in pos screen #318 means feat: enhance offline batch and serial number management #359's own review no longer governs code that ships — the reviewer there
   approved a diff that will have already merged elsewhere.
3. The cost is at its minimum today. Pn 65 expenses in pos screen #318 is 0 behind develop, the merge order with
   Feature/thin develop split #360 is already agreed (318 first), and no reviewer has an open thread mid-flight. Every
   day this waits, and certainly once Feature/thin develop split #360 rebases, the rewrite gets more expensive.

A force-push that drops files and rewrites SHAs is expected and fine here. Re-requested review
is the intended cost, not an accident.

Your plan needs no amendment. Restating it as the acceptance condition:

• 8231e26 → feat: enhance offline batch and serial number management #359
• 05a7b28 / 59acd2f / 9e9c26f → Feat/eod shift report print #275
• f5ae353 → dropped (already on develop via fix: shift closing total reflects money collected, not invoiced #312)
• a4f6ffd → dropped, except the cint import at pos_profile.py:7

On that last point — splitting the cint fix into its own small PR is the right call and worth
saying why out loud: cint is genuinely undefined at pos_profile.py:155 on develop, so that
one line closes a live NameError in allow_credit_sale resolution. Buried in a commit titled
"ruff check fixes" it reads as formatting and nobody backports it. On its own it is reviewable
in thirty seconds.

Done when: git diff --stat $(git merge-base HEAD develop)..HEAD contains only
expense-related files, and the four riders are open (or already merged) under their own numbers.

Once that lands, this is a merge. Everything else is closed and the gate is green —
27/27 on api.test_expenses, 11/11 on pos_closing_shift, 5/5 on pos_closing_print, and the
single remaining ruff error (invoices.py:1669 UP038) is pre-existing on develop.

[engahmed1190]

Post-split review — head 13a653e

N3 is closed. The split is clean and the gate is green. Two mechanical items stand between
this and merge; neither is a code defect in the feature.

Verified against the rewritten history, by diffing the pre- and post-rewrite trees rather than
reading commit subjects.

---

The split — verified

44 files → 27. Every rider is gone:

Rider	Status
8231e26 — #359 offline batch/serial (4 files)	Gone ✅
05a7b28 / 59acd2f / 9e9c26f — EOD report + translations	Gone ✅ (including one line each from ar.csv, id.csv, pt-br.csv)
f5ae353 — collected money	Gone ✅ — see note below
a4f6ffd — ruff sweep (10 unrelated files)	Gone ✅
cint import	Split out as #373, already merged ✅

The 20 commits remaining between d02f852 and 13a653e are all expense work.

On f5ae353: dropping it changed the tree by zero bytes — ShiftClosingDialog.vue and
pos_closing_shift.py are byte-identical before and after the rewrite. That confirms your read
that it was already on develop via #312: its unique contribution had been reconciled away by
the earlier dedup at 7818730. develop and this branch each carry exactly one
__("Collected") card; there was never a double-render risk, and there is none now.

Nothing was lost in the rewrite. The only differences between the old head b8dc9a3 and
13a653e, outside the removed riders, are the three you intended: the dead company_currency
key out of bootstrap.py, the ExpenseDialog.vue:214 comment, and the offers.py typing import
restored with the sweep.

13a653e's cleanup is safe — nothing consumed bootstrap's key. getPreloadedPOSProfile()
(stores/bootstrap.js:103) is exported but has no callers, and the dialog resolves currency from
dialogDataResource and shiftState.company.default_currency, neither of which touches it.

---

Gate, on a live bench

Frappe 15.111.1 + ERPNext 15.111.0, posnext.local, MariaDB.

Gate	At 3ab88dc	At 13a653e
pos_next.api.test_expenses	27/27	27/27 pass ✅
...pos_closing_shift.test_pos_closing_shift	11/11	11/11 pass ✅
ruff check (changed .py, now 9 files)	1 error — pre-existing	1 error — regression, see below
Behind develop	0	2

N6/N7 from 6c80f31 survived the rewrite intact, and both suites still pass on a branch whose
SHAs all changed.

---

Two things left

1. N4 regressed · test_pos_closing_shift.py:11

pos_next/pos_next/doctype/pos_closing_shift/test_pos_closing_shift.py:11:1: I001 [*] Import block is un-sorted or un-formatted

Dropping a4f6ffd wholesale took its one legitimate line with it — the blank line after the
import block in test_pos_closing_shift.py was the only part of that sweep that belonged to this
PR. Understandable casualty of a clean revert; it just needs putting back.

Done when: ruff check --fix pos_next/pos_next/doctype/pos_closing_shift/test_pos_closing_shift.py.

Worth noting the upside: invoices.py left the diff with the sweep, so the pre-existing UP038
that had to be caveated last round is out of scope. Once this one line is back, the changed
files are ruff-clean outright for the first time on this branch — no asterisk.

2. Rebase · 2 behind

develop moved to 582ef7b when your own #373 merged at 15:44. 13a653e is 2 behind. D1 asked
for current, so this needs a rebase before merge — trivial, and no conflict surface: #373 touches
pos_profile.py, which this branch no longer carries.

---

Then it merges

Everything else is closed and verified: B1 H1–H4, M1–M7, T1–T4, L1–L3, D1–D2, and N1–N7. The
security work, the lock, the currency basis, the fixtures and the reconciliation seam all hold
up under a green run.

The split was the right call and it was executed cleanly — the riders are gone, nothing in the
feature was disturbed, and #373 landing separately means that NameError fix reaches develop
on its own timeline instead of riding an expenses PR. Fix the one I001, rebase, and this is a
merge from my side.