Feature/thin develop split - #360
Conversation
…evelop. Keep origin/develop baseline offers; GWP/auth Python stay in posnext_promotions. Free-bundle SI merge is no longer in the POS Next invoice class. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Updated App.vue to conditionally render AuthorizationDialog based on authGateInstalled. - Modified SelectInput.vue to support multiple selections and improved UI for selected options. - Refactored CreateCustomerDialog.vue to use requiresSplitCustomerName for customer name handling. - Adjusted InvoiceCart.vue to utilize the new wallet API and removed Magento dependencies. - Enhanced PromotionManagement.vue to include Gift Pool promotion type and related UI elements. - Introduced applyOfflineGiftPool function in posCart.js for handling gift pool discounts. - Updated various API endpoints to ensure compatibility with new promotional features. Co-authored-by: Cursor <cursoragent@cursor.com>
- Updated useInvoice.js to prevent free items from inflating stock checks and adjusted actual_qty to prefer original_stock. - Improved posCart.js to utilize new stock validation functions for accurate quantity checks, ensuring free items do not affect paid item stock validation. - Added utility functions in stockValidator.js for calculating stock quantities and validating item availability based on combined paid and free rows. - Modified _collect_stock_errors in invoices.py to sum quantities per item_code and warehouse, ensuring accurate stock error reporting. Co-authored-by: Cursor <cursoragent@cursor.com>
- Added conditional rendering for buttons in InvoiceCart.vue to prevent actions on locked free rows. - Updated quantity handling logic to ensure free items do not interfere with paid item interactions. - Introduced isLockedFreeRow function to streamline checks for free item status. - Enhanced posCart.js to manage free item quantities more effectively, ensuring accurate invoice processing for promotions like GWP. Co-authored-by: Cursor <cursoragent@cursor.com>
…alidation rules, and grant management
- Updated CreateCustomerDialog.vue to require a selected country code for customer submission and improved mobile number handling. - Enhanced posCart.js and posOffers.js to include gross and net amount calculations for offer validation, ensuring accurate eligibility checks based on item discounts. - Refactored customer group and territory resolution logic in customers.py to prefer non-group entries, improving customer creation reliability.
engahmed1190
left a comment
There was a problem hiding this comment.
Review: architecture, security, performance, coverage
Reviewed against 5d4c16f (merge base a07f981). Verified on a local bench: 3 sites, dns_multitenant: true, 9 gunicorn workers, Frappe + ERPNext v16-era Sales Invoice. JS suite run (vitest run — 12 pass), compileall clean, ruff and biome run on the changed files.
One blocker, four high, nine medium. The blocker is that the split's central mechanism does not work: the optional-app gate is evaluated once per worker process, against whichever site that worker happened to touch first. On this bench a site that does not even have pos_next installed decided pos_next's pricing-rule patch state for a site that does. Details inline on pos_next/__init__.py.
The authorization gate is well-built underneath — the registry/gate/grants/policy separation is clean, consume being compare-and-set rather than delete is the right call and the reasoning in the docstring is correct, and it is the best-tested code in the PR. The problems are at its edges: the approver dialog never shows what is being approved, the rate limit keys on IP rather than approver, PIN enrolment is self-service, and the two audit fields it writes to the invoice are never created.
Not blocking, but worth deciding before merge
promoApi.jsroutes authorization toposnext_promotions.api.authorization.*whenever that app is installed, whilebefore_submitis still enforced bypos_next's gate readingpos_next's redis keyspace. Nothing in this repo can verify that pairing and no test covers it.applyOfflineGiftPool/applyOfflineGwpSameItem(~200 lines of satellite behaviour) are hardcoded inposCart.jsand gated at runtime — while this same PR shipsofferStrategies.js, a plugin registry built for exactly that.
Coverage
authorization/ is genuinely well tested for its size. The gaps are where the money is: overrides/sales_invoice.py (the largest money-path rewrite here) and posCart.js (+396) have zero tests, and four of the findings below live in that untested code. test_no_promotions_import.py is solid — a real AST walk. test_split_smoke.py mostly skipTests when the optional app is absent, and a couple of cases assert nothing that can fail.
Does POS Next work standalone? Static imports: yes, verified — nothing imports posnext_promotions, magento_integration or masar_miraaya, and the app imports and patches cleanly without them. Runtime: not established, for the two reasons above.
One correction to an earlier draft of this review
I initially flagged get_invoices as having lost its search/date filters and its page-size clamp. That was my error: I diffed against develop's tip rather than the merge base. 98b39e5 improved get_invoices on develop after this branch point; this PR never touched it and the merge will take develop's version. Withdrawn.
Filed as comments, not a change request — the call on merge is yours.
| {{ __("Authorization Required") }} | ||
| </h3> | ||
| <p class="mt-1 text-sm text-gray-500 dark:text-gray-400"> | ||
| {{ __("A manager must approve this action.") }} |
There was a problem hiding this comment.
High — the approver is never shown what they are approving.
This dialog renders "A manager must approve this action.", an approver picker and a PIN box. It never shows the action name, the amount, the customer or the invoice. state.actionLabel is captured in useAuthorization.js and never rendered anywhere.
The refund amount is supplied by the client (ReturnInvoiceDialog.vue, in the requireAuthorization context) and becomes the grant's binding ceiling on the server. So a manager who walks to a till and types a PIN has no way to tell whether they are approving a 12.00 refund or a 12,000.00 one — which is the one thing this control exists to prevent.
Render the action, the amount and return_against in the dialog body.
There was a problem hiding this comment.
@engahmed1190 I think this is more of a UX improvement than a security/correctness issue in our current flow.
The authorization dialog is not shown to the manager on a separate device or in isolation. The cashier calls the manager over to the cashier's terminal, where the full invoice and its details are already visible. The customer is also physically present with the printed invoice, which is required to process the return, so the manager can verify the printed invoice against the invoice shown on the cashier's screen before approving.
The Auth Required dialog is simply the final approval step after those checks have already been performed.
This flow is also exactly what the business team presented to the customer, and it matches the current customer's agreed requirements. So I would not classify the absence of these details in the authorization dialog as a critical security gap. I agree that showing the action, amount, and return reference could be a useful UX improvement, but I don't think it should be treated as a high-severity issue for this flow.
|
|
||
|
|
||
| @frappe.whitelist() | ||
| @rate_limit(limit=5, seconds=60) |
There was a problem hiding this comment.
High — this rate limit keys on IP, so it is one bucket for the whole store.
frappe.rate_limiter.rate_limit defaults to ip_based=True, and no key is passed, so the identity is frappe.local.request_ip. Every till behind one NAT shares a single bucket of 5 approvals per minute. A busy shop hits 429s on legitimate approvals, and the failure mode is a hard block on refunds.
Pass key="approver" or set ip_based=False. pin.max_failures + the lockout is already the brute-force control here, so this decorator does not need to carry that job.
| "pos_next.api.wallet.validate_wallet_payment", | ||
| "pos_next.overrides.pricing_rule.apply_min_max_price_discounts", | ||
| ], | ||
| "before_submit": "pos_next.authorization.gate.enforce_document", |
There was a problem hiding this comment.
Worth documenting somewhere, because it is a behaviour change with no error message that explains it: this hook is unconditional, and the grant token only ever reaches a document through api/invoices.py (invoice_doc.flags[TOKEN_FLAG]).
So once a POS Authorization Rule exists for returns, every other path that submits a POS return — desk UI, bench execute, a background job, a bulk tool — is refused, because it carries no flag. That is fail-closed and probably the right default, but the first person to submit a credit note from the desk will have no idea why.
Related: GRANT_TTL = 180. Any retry of submit_invoice more than three minutes after approval — a slow line, an offline replay via offline/sync.js — needs the manager to walk back to the till.
What to finish before re-reviewEverything open from review #5141722425, as a checklist. Each item names the file it lives in and the condition that closes it, so "done" is demonstrable rather than asserted. 29 items — 13 must close before re-review (Gates 1–2), 5 tests, 8 cleanups, 3 hygiene. Gate 1 — BlockingThe split does not currently do what it claims, and money and approval paths are wrong. These are what make the PR unmergeable rather than merely unpolished.
Gate 2 — Correctness, and two open decisions
Gate 3 — Tests
Gate 4 — Cleanups
Gate 5 — Before asking for re-review
Definition of doneRun these and paste the output here. Green, plus Gates 1–2 closed, is what makes this ready for a second look. cd POS && yarn test:run # currently 12 pass — should grow with T1–T5
bench --site <site> run-tests --app pos_next
ruff check pos_next scripts
cd POS && yarn biome lint src
# and the one that proves B1, on a bench with 2+ sites:
python -c "import frappe; frappe.init(site='A'); frappe.destroy(); frappe.init(site='B')"
# -> get_installed_apps must not be called during either initDeliberately not on this list: the repo-wide Biome formatting drift (predates this PR), and |
Import-time _promotions_installed() froze the patch gate to whichever site warmed a multi-tenant worker first, causing silent wrong prices or double-patching across sites. Always install the patches and defer via optional_apps.promotions_installed() on each call.
Stop collapsing base_amount and amount into one payment_amount — credit/debit must stay company currency, and a zero base after change netting must skip instead of falling back to the txn amount.
…ion. - Added expires_in_sec=300 as a safety net on pos_next_wallet_payment_modes - Clear the key from Mode of Payment after_insert / on_update / on_trash Toggling is_wallet_payment now refreshes the map immediately, so validate_wallet_payment still runs the balance check. TTL covers any missed invalidation path without needing a Redis flush.
Cross-reference: this PR and #318Both are open against Shared files
Suggested order: #318 first, then this one rebases onto it. Not a judgement on quality — it is about how far each one's blocker is from done:
Since this PR is the one staying open longer and already needs a rebase, it is the natural place to absorb the four-file conflict. When you rebase: after resolving, confirm both the Return authorization and the POS Expense entries 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. One thing worth knowing before you rebase: Reviews and per-PR checklists are in the comments on each. Item counts: 29 here, 21 on #318. |
Review board — PRs #318 and #360Everything 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.
Merge sequenceFirst — 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 blockersEverything else here is secondary to these. #318 — A blank expense maximum means unlimitedTick one checkbox on a POS Profile and every cashier on it can submit unbounded Journal Entries crediting the till, through a whitelisted endpoint running
#360 — The optional-app gate never engagesDecided once per worker process against whichever site it touched first. Reproduced on a 3-site bench:
#318 · POS expenses — 21 itemsCloser to mergeable. Validators are thorough and tested; 17/17 pass. The gaps are all downstream of the money leaving. Gate 1 — Blocking (5)
Gate 2 — Correctness (7)
Gate 3 — Tests (4)
Gate 4–5 — Cleanup & hygiene (5)
#360 · Thin develop split — 29 itemsFurther out. The auth gate underneath is well built; the problems are at its edges and in the split mechanism itself. Gate 1 — Blocking (6)
Gate 2 — Correctness & decisions (7)
Gate 3 — Tests (5)
Gate 4–5 — Cleanup & hygiene (11)
The collisionBoth PRs touch Four decisions, not patchesMarked 🔀 above. Each wants a person, and each belongs in the PR description once settled.
What I withdrewTwo findings did not survive checking, and are deliberately not on this board:
#318 @ |
_ensure_pos_customer() stays before ERPNext’s validate (customer is still required up front). The payment-account check now runs once debit_to exists, so it catches same-account Mode of Payment issues on first save instead of only on submit re-validate.
- **Blank customer** → still falls back to the POS Profile default (walk-in convenience unchanged) - **Non-empty customer that doesn’t exist** → throws `Invalid Customer` instead of quietly swapping to the default - **Neither resolves** → still returns `None`, and `_ensure_pos_customer` throws as before
pos_next Extended offerStrategies.js with registerProductOfferStrategy / getProductStrategyForOffer (keyed by promotion_type) applyOfflineFreeItem only asks the registry; removed applyOfflineGiftPool, applyOfflineGwpSameItem, and the isPromotionsAppInstalled gates from posCart.js posnext_promotions Added public/pos/offer-strategies.js — registers Gift Pool + GWP with the same offline behaviour as before (round-robin, once-per-cart paidQty gate, absolute free-row qty) Tests: offerStrategies vitest (6) and test_no_promotions_import (9) pass.
Extract syncCartFreeItems so free-row qty restore isn't only enforced by call-site agreement, and add round-trip tests for the silent undercharge case.
…mmediately; group or missing names fall through to the next candidate, then any leaf, then "All Territories".
…g the loyalty/bootstrap runners. Left alone on purpose: validate_customer_create — must raise prepare_customer_doc — runs before insert(), so a failure never leaves an orphaned local Customer; keeping it strict still makes sense unless you want prepare to be best-effort too
Hardcoding brainwise.dev broke other benches; insert apps on sys.path before importing frappe so the path change actually applies.
…when optional apps are missing.
Replaced tautologies
prepare_customer_doc: AST check that it reads pos_next_customer_prepare, plus mocked hooks proving empty → False, falsy → False, truthy → True (and that kwargs are forwarded).
extend_bootstrap_settings: mocked hook must mutate a fresh {}; empty hooks leave settings unchanged.
Dropped skip-heavy “when installed” checks (doc events, LP API, promotions override) — those belong in the satellite apps. Replaced with a static assert that pos_next/hooks.py does not register Magento handlers.
Kept wallet mocks, no-magento-module import, and bootstrap defaults from DEFAULT_POS_SETTINGS.
…sts doesn’t export it). That import was only in test_bank_deposits.py.
Collision with #318 — agreed merge orderLand #318 first. This PR (#360) rebases second and resolves the four-file collision.
After rebase — menu smoke check
|
Blank approver: Rejected before processing. PIN lockout: Now fail-closed if Redis/cache is unavailable. Cache outage bugs: Fixed; logging is now best-effort and non-blocking. 429 UX: Replaced Frappe’s technical message with a clear user-facing message. Verification: 63 backend + 36 frontend tests passed; 9/9 split guards; build clean. Trade-off: Redis outage blocks approvals intentionally for security. Deployment note: Ensure X-Forwarded-For preserves the real client IP behind any LB/CDN.
…nal framework operations, preventing unrelated back-office documents from being blocked. Grant TTL: Increased from 3 to 15 minutes, configurable via POS Authorization Settings. Security: Longer TTL does not weaken grant scope; grants remain bound to session, document, action, and amount, and are single-use/revocable.
|
@MohamedAliSmk @engahmed1190 All the requested issues and comments have been addressed and thoroughly tested. From my side, all three points have been resolved. @MohamedAliSmk , is there anything else currently blocking the merge of this branch? |
|
@MohamedAliSmk @MostafaKadry, please review the action-item checklist in the comments of PR #360 and check off any items that have already been completed. For any remaining items, please add a brief status update. |
No description provided.