Move Develop changes to version-15 - #370
Open
MostafaKadry wants to merge 57 commits into
Open
Conversation
Previously every payment method in the closing-shift reconciliation started as null with _touched=false, forcing the cashier to click into each row before the form could be submitted. Methods with no cash collected still had to be touched manually. Default closing_amount to 0 and mark rows as touched so the expected amounts compute immediately and the shift can be closed without manually zeroing untouched methods. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a "LIFO Cart Order (Newest on Top)" POS setting. When enabled and no explicit cart sort is active, the most recently added item is shown at the top of the cart instead of the bottom — handy on long carts where the cashier wants to see what was just scanned. - New cart_lifo check field on POS Settings (default off) + backend constants (POS_SETTINGS_FIELDS / DEFAULT_POS_SETTINGS). - posSettings store exposes a cartLifo computed. - useCartSort accepts an optional lifoMode; reverses the list when LIFO is on and no sort column is selected. Explicit sorts are unaffected. - InvoiceCart passes the setting through; toggle lives in the Sales Operations settings group so it takes effect live via reloadSettings(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
…ft-default-zero # Conflicts: # POS/src/components/ShiftClosingDialog.vue
…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
…rder # Conflicts: # POS/src/components/sale/InvoiceCart.vue # POS/src/components/settings/POSSettings.vue # POS/src/composables/useCartSort.js # POS/src/stores/posEvents.js # POS/src/stores/posSettings.js
…nt-logic-pricing-rule feat: implement min/max discount logic for pricing rules and promotional schemes
Updated the coupon validation logic to ensure a customer is selected before applying a coupon. Added a default empty string for the customer prop in the CouponDialog component and improved error handling to prompt the user if no customer is chosen.
…to-be-required feat: make mobile number mandatory in CreateCustomerDialog
Leftover <<<<<<< HEAD marker (no matching ======= / >>>>>>>) broke the Vite/vue-compiler-sfc build, failing CI.
…okups POS Next's own price queries (search_by_barcode, get_item_variants, get_items x2, get_items_bulk in pos_next/api/items.py) never respected Item Price's valid_from/valid_upto window, unlike ERPNext core's own get_item_price. An expired or not-yet-active price could still be served to the till. Adds date-range filtering via one shared _item_price_validity_conditions() predicate + _fetch_uom_prices_map()/_fetch_item_uom_prices() helpers reused across all 5 call sites, instead of duplicating the check inline five times. The predicate mirrors erpnext/stock/get_item_details.py's get_item_price byte for byte (IfNull sentinel-date pattern, same valid_from-desc tie-break for overlapping validity windows), verified directly against that source. Deliberately does NOT import pos_next.promotions or anything else outside this file — develop doesn't have that package yet, and an earlier attempt to backport this by copying the whole file from staging (which does depend on it) broke bench install-app for the entire branch. That attempt was reverted; this is the same fix applied cleanly instead. Verified: ruff clean, file + the doctype controller CI failed on both import cleanly, and a live query against overlapping-validity Item Price rows on the pos site returns the currently-valid rate, not the stale or future one.
… refactor POS components
Add POS Shift History dialog with filters and CSV export
…-default-zero feat(shift-closing): default payment counts to 0 instead of null
…stomer-is-None-while-the-API-expects-a-string.-I-ll-inspect-validate_coupon-and-how-the-POS-client-calls-it feat: enhance coupon validation and customer selection in CouponDialog
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
…I-Server change node-version to 22
…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
…closing-total-followup
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
Merges upstream/develop (42 commits) into the product management branch. Four files conflicted. All were resolved by taking develop's version and re-applying only the functional additions, so none of this branch's prettier churn is reintroduced: - itemSearch.js — reset to develop, then re-added upsertItemInList(), refreshItem() and the store export. Verified by normalizing both files through the same formatter: the delta against develop is now exactly those two functions, the export, and one let -> const. Previously the branch showed ~600 changed lines here, almost all quote style and trailing commas. - ManagementSlider.vue — took develop's formatting, re-applied the Products -> Stock Lookup rename and the new Product Management button. - POSSale.vue — took develop's `let` declarations. The biome-ignore comments are unnecessary; JS is linted by prettier/eslint in pre-commit, not biome. - ar.csv — kept both sides' new strings. No functional change to the feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
The item-group scoping added in review covered get_products (the read
path). The same boundary was not enforced when writing, in two ways.
1. pos_profile was caller-supplied and never validated.
save_product() took the profile name straight from the request and
called frappe.get_cached_doc(), which does no permission check. The
guard that follows is:
if allowed_item_groups and item_group not in allowed_item_groups:
and _get_pos_profile_allowed_item_groups() documents that an empty
list means "no group restriction". So naming any POS Profile with no
item_groups rows made allowed_item_groups falsy and skipped the check
entirely — a cashier scoped to one branch could edit any Item in the
system. It also reached pricing, since the price is written to that
profile's selling_price_list.
Added _validate_pos_profile_access(), matching the POS Profile User
check already used in api/invoices.py and api/credit_sales.py, and
applied it to save_product, get_products and get_item_groups.
2. The update path never checked the item's current group.
Only the incoming item_group was validated, so passing the item_code
of an item in a disallowed group pulled it into an allowed one —
renaming, regrouping, disabling and repricing it on the way. Now the
existing group is validated before the item is mutated.
Neither is exploitable without Item write permission, which the feature
requires anyway. But the point of profile scoping is to hand product
management to a branch cashier without giving them the global item
master, and that boundary did not hold.
Also in this file:
- get_item_groups() had no permission check at all — any authenticated
user could enumerate any profile's item groups. Now requires Item read
plus profile access.
- item.image accepted an arbitrary string, so a crafted call could point
it at an external URL that the POS then renders. Restricted to Frappe
file paths. (Reachable from the desk too, so not introduced here.)
- Removed frappe.db.commit(). The framework commits at the end of a
successful request and rolls back on exception; committing here
defeats that and commits unrelated pending work.
- Sorted the import block (ruff I001), which otherwise fails pre-commit
for anyone touching this file.
Adds test_product_management.py — 10 mock-based tests, no DB. Verified
by mutation: removing the profile check fails
test_rejects_profile_the_user_is_not_assigned_to, and removing the
current-group check fails test_rejects_item_whose_current_group_is_out_of_scope.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
Two formatting-only changes, no behaviour change: - ProductManagement.vue — ran the repo's prettier over the new file. It is a new file, so this is not churn against develop; pre-commit would rewrite it on merge anyway. - itemSearch.js — braced the four switch-case blocks that declare consts (eslint no-case-declarations, 8 errors). This is pre-existing on develop and only surfaces here because pre-commit lints changed files and this branch touches the file. Braces just scope the consts to their case. Verified afterwards that itemSearch.js and POSSale.vue still differ from develop only by this branch's functional additions — no formatting churn was reintroduced into either. Split out so it can be dropped independently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
Two defects stacked, so a failed save showed nothing at all.
1. handleError was undefined.
ProductManagement.vue destructures it from useToast():
const { showSuccess, showError, handleError } = useToast();
useToast() never exported handleError. All four catch blocks —
saveProduct, loadProducts, loadItemGroups, loadUOMs — therefore called
undefined(...), throwing a TypeError *inside the catch*. That
rejection is unhandled, so nothing reaches the user: the spinner stops
in `finally` and no toast appears.
The file was adapted from PromotionManagement.vue, which defines
handleError locally (line 1449) instead of taking it from useToast.
CouponManagement.vue has a third copy. Same class as the refreshItem()
issue already caught in review.
2. Reading _server_messages does not work for errors raised through the
app's `call` wrapper.
frappe-ui's call.js does not pass the response through. It builds a new
Error whose `.message` is only "<method> <exc_type>", and moves the
real text to `.messages` — already parsed out of _server_messages and
flattened to strings. So `error._server_messages` is undefined and the
user gets:
pos_next.api.product_management.save_product ValidationError
The local copies in PromotionManagement.vue and CouponManagement.vue
check only _server_messages, so they have this same blind spot.
Fixed at the root: parseErrorMessage + handleError now live in useToast
and are exported, so the existing call sites become correct unchanged.
parseErrorMessage handles both shapes — frappe-ui's `.messages` array
first, then raw `_server_messages` for the direct fetch() paths — strips
the HTML, de-duplicates, and ignores a `.message` that is just the
"<method> <exc_type>" string. handleError never throws, so a failing
error handler cannot turn a failed action into a silent one again.
Promotion/Coupon keep their local copies for now; they work for raw
responses and migrating them is out of scope here.
Verified against a real failure — an Item save rejected by the Shopify
on_update hook in ecommerce_integrations. Reconstructing the error object
exactly as frappe-ui/src/utils/call.js builds it, the toast goes from
"pos_next.api.product_management.save_product ValidationError" to
"Failed to decrypt key Shopify Account… Encryption key is invalid!
Please check site_config.json…". Null, plain Errors, empty message
arrays and exc_type-only errors all fall back to the default message.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
…ct image The form was capped at `max-w-2xl` (672px) inside a pane that is ~1500px wide on a 1080p screen, so roughly 830px sat empty while the product image was a 128x128 thumbnail. Layout - Dropped the max-w-2xl cap; the form now uses the pane up to 1400px. - Two columns from xl: fields on the start side, a large image panel on the end side. CSS grid flips this for RTL on its own, so the image stays on the end side in Arabic without extra rules. - The image panel is sticky on xl+, so it stays visible while scrolling the UOM conversions. - Below xl it collapses to one column with the image on top, capped at 320px so a square image does not fill a tablet screen. Image - 128x128 thumbnail -> a square panel that is 380px at xl and 460px at 1536px+. About 3x the linear size, 8-13x the area. - The image itself is the upload target: click anywhere on it, or drop a file onto it. Drag state is reflected on the border. - object-contain rather than object-cover, so product photos are not cropped. - Empty state is now an explicit dashed dropzone instead of a grey box. Fields - Grouped into Details / UOM Conversions / Options sections with headings, rather than one flat stack. - Product Name spans the full width; Item Group, UOM and Price share a row once there is room for it. - The two checkboxes became labelled cards with descriptions. - UOM conversion rows use an icon-only remove button, which stops the row wrapping in the narrower column. Breakpoints Tailwind breakpoints are viewport-based, not container-based, so a naive `sm:grid-cols-2` inside the fields column fires while that column is still ~190px. Column widths were worked out per viewport and the breakpoints chosen to match: two columns only from xl (fields 436px), inner grids only from 1536px (fields 599px). Note: `min-[1536px]:` is used rather than `2xl:` because frappe-ui's preset replaces theme.screens with sm/md/lg/xl only (frappe-ui/tailwind/plugin.js:212). `2xl:` compiles to nothing in this project — verified by checking the emitted CSS. Verified every new utility and arbitrary value is present in the built stylesheet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
…Settings
Mobile
The app is responsive (device-width viewport, useResponsivePayment's
isMobileView, mobile detection in performanceConfig, PWA), but this
dialog was not: a fixed 320px list beside the form does not fit a phone.
- Below md the list and the form are one pane at a time (master-detail),
with an explicit back arrow in the form header since the list is no
longer beside it. The arrow mirrors under rtl:.
- Full-bleed on phones; the 95vw/95vh rounded card returns at sm.
- Header, action bar and form padding tighten below sm; the action bar
wraps rather than overflowing, and the product title truncates.
Upload rules
Extensions and max size were hardcoded (2 MB, four MIME types), so the
picker accepted files the server then rejected. Both now come from
System Settings via get_product_image_settings():
- allowed_file_extensions is newline-separated, uppercase, without dots,
and empty means "no restriction" rather than "nothing allowed". It
covers every file type, so it is intersected with the image types this
screen can render — a site allowing CSV should not offer CSV as a
product image.
- max_file_size is stored in MB; get_max_file_size() resolves it to
bytes, falling back to site_config then Frappe's 25 MB default.
Validation matches on extension rather than MIME type, because browsers
report inconsistent types for the same file and the server checks the
extension too. If the fetch fails the screen keeps working on defaults;
the server validates on upload regardless.
The hint under the image now reads the live values:
Allowed file types: JPG, JPEG, PNG, GIF, WEBP — up to 25 MB.
These are configurable in System Settings. Upload happens when the
product is saved.
If the site allows no image types at all, that is stated explicitly
instead of leaving a picker that can never succeed.
Adds 5 tests (15 total) covering the empty-means-unrestricted rule, the
intersection with image types, the no-image-types case, messy operator
input (lowercase, leading dots, blank lines) and the MB-to-bytes
conversion. Verified against the live site: no restriction configured,
so all five types at 25 MB.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
Added POS product management conflict fix
…l-followup Enhance POS Shift History, Payment Handling, and Discount Logic
…osing-total fix: shift closing total reflects money collected, not invoiced
…saves
Editing only the price on a Shopify-synced product failed with
"Invalid image path".
save_product() rewrites every field on each save rather than patching,
so the client resends `image` even when untouched. That value was then
validated against an allowlist of local upload paths:
ALLOWED_IMAGE_PREFIXES = ("/files/", "/private/files/")
Item.image is not always a local upload. The ecommerce_integrations
Shopify sync writes cdn.shopify.com URLs — 26 of 73 items on the site
this was found on. Every one of them failed to save, and the error named
the image on an edit that never touched it.
Two changes:
- Only validate an image the caller is actually changing. If the incoming
value equals the stored one, it is neither reassigned nor checked, so a
price-only edit no longer involves the image at all.
- Accept external URLs. SAFE_IMAGE_PREFIXES is ("/", "http://",
"https://"): the check now rejects script-bearing schemes rather than
forcing images to be local. Restricting to local paths bought little
anyway — anyone with Item write can set an arbitrary image from the
desk — and it broke a legitimate, in-use pattern.
update_product_image() keeps the strict local-path rule, now named
LOCAL_FILE_PREFIXES. That endpoint resolves the URL back to an attached
File record, which an external URL could never match, so local-only is
correct there.
test_rejects_external_image_url asserted the behaviour that caused this
bug and failed as soon as the code was fixed; it is replaced by
test_rejects_script_scheme_on_new_product, which pins the actual
contract. Three regression tests added: the unchanged-synced-URL price
edit, script schemes, and the pending-upload data: URI.
Verified against the real failing item: unchanged synced URL is left
untouched, uploads and external URLs are accepted, javascript:,
JavaScript: and vbscript: are rejected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNMeLBCSBBYMm5k5zDNda4
fix(product-management): stop unchanged image paths blocking product saves
feat(cart): add LIFO cart order setting (newest item on top)
get_receivable_accounts calls cint() without importing it, which raises NameError on develop when credit-sale gating runs. This is a real bug fix (F821), not formatting — keep it reviewable on its own. Co-authored-by: Cursor <cursoragent@cursor.com>
fix(pos_profile): import cint for allow_credit_sale resolution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.