From f4a2962981743d4ba73ab3240b4e8ecf251a4a1e Mon Sep 17 00:00:00 2001 From: abhishek Date: Sat, 29 Aug 2026 12:46:14 +0530 Subject: [PATCH] product name alone made required --- src/api/ingest.test.ts | 42 ++++++++++++++ src/api/ingest.ts | 26 ++++++++- .../nearle-admin/import/ImportScope.tsx | 56 ++++++++++++++---- .../nearle-admin/import/SheetImportPanel.tsx | 27 +++++---- .../nearle-admin/import/openingStock.test.ts | 47 +++++++++++++++ .../import/parseProductSheet.test.ts | 58 +++++++++++++++++++ .../nearle-admin/import/parseProductSheet.ts | 30 ++++++++-- 7 files changed, 256 insertions(+), 30 deletions(-) create mode 100644 src/features/nearle-admin/import/parseProductSheet.test.ts diff --git a/src/api/ingest.test.ts b/src/api/ingest.test.ts index 45492bd..d1e0e71 100644 --- a/src/api/ingest.test.ts +++ b/src/api/ingest.test.ts @@ -161,3 +161,45 @@ test('a retired drop with nowhere to follow is finished', () => { assert.equal(isSettled(retired), true); }); + +/* ── Cross-drop contamination ─────────────────────────────────────────────── */ + +// An admin can assemble one run from several drops, so a run's manifest can +// carry other senders' products. Applying our sheet's price and opening stock +// to those would stock someone else's goods into our merchant's branch. +test('only our own file contributes products', () => { + const run = { + ...held, + status: 'done' as const, + files: [ + { + index: 0, + filename: 'ours.csv', + status: 'done' as const, + result: { + products: [ + { image_id: 'amul_a', brand: 'amul', product_name: 'Ours', disposition: 'inserted' as const }, + ], + }, + }, + { + index: 1, + filename: 'someone-elses.csv', + status: 'done' as const, + result: { + products: [ + { image_id: 'amul_b', brand: 'amul', product_name: 'Theirs', disposition: 'inserted' as const }, + ], + }, + }, + ], + } satisfies IngestBatch; + + const mine = productsOf(run, ['ours.csv']); + assert.equal(mine.length, 1); + assert.equal(mine[0]?.product_name, 'Ours'); + + // Unfiltered still returns everything — the filter is the caller's decision, + // and every caller that prices products must make it. + assert.equal(productsOf(run).length, 2); +}); diff --git a/src/api/ingest.ts b/src/api/ingest.ts index fde8f54..8ec72aa 100644 --- a/src/api/ingest.ts +++ b/src/api/ingest.ts @@ -426,9 +426,29 @@ export async function resolveBatch(batch: IngestBatch, signal?: AbortSignal): Pr } } -/** Every product a finished batch wrote, across its files. */ -export function productsOf(batch: IngestBatch): IngestProduct[] { - return (batch.files ?? []).flatMap((file) => file.result?.products ?? []); +/** + * The products a finished run wrote, optionally narrowed to our own files. + * + * `filenames` is not optional in practice and should always be passed. An admin + * can assemble ONE run from several drops — the owning team's own words: "a run + * an admin assembled from several drops lists every file in it, so you may see + * filenames batched alongside your own" — so a run's manifest can contain other + * senders' products. + * + * Reading all of them was a real hazard, not a tidiness point. The sheet's price + * and opening stock are applied to whatever the manifest is matched against, so + * a product from someone else's sheet sharing a name with one of our rows would + * have been priced and stocked from OUR file, into OUR merchant's branch. + * + * Filtering by filename is the best this contract allows and it is not airtight: + * two senders can both upload `products.csv`. Narrowing by drop would be exact, + * and the run's files carry no drop reference to narrow by — worth asking for. + */ +export function productsOf(batch: IngestBatch, filenames?: readonly string[]): IngestProduct[] { + const wanted = filenames ? new Set(filenames) : null; + return (batch.files ?? []) + .filter((file) => !wanted || wanted.has(file.filename)) + .flatMap((file) => file.result?.products ?? []); } /** diff --git a/src/features/nearle-admin/import/ImportScope.tsx b/src/features/nearle-admin/import/ImportScope.tsx index ff2a0d5..9cc93bd 100644 --- a/src/features/nearle-admin/import/ImportScope.tsx +++ b/src/features/nearle-admin/import/ImportScope.tsx @@ -24,6 +24,16 @@ import { Text } from '@astryxdesign/core/Text'; import { VStack } from '@astryxdesign/core/VStack'; import { useTenantCategories, useTenantLocations, useTenants } from '@/queries/hooks'; +/** + * The category the customer app browses. + * + * A constant because it is one on the app side too — its browse screen asks for + * `categoryid: 2` — and because a product filed anywhere else is invisible to + * shoppers rather than merely misfiled. Used only as a floor for a merchant with + * no categories of their own; an established tenant's real list always wins. + */ +const APP_BROWSE_CATEGORY = 2; + /** Everything an upload needs before it can be turned into stock. */ export interface ImportTarget { tenantid: number; @@ -69,14 +79,36 @@ export function ImportScope({ value, onChange, isTenantFixed = false }: ImportSc [locations.data], ); - const categoryOptions = useMemo( - () => - (categories.data ?? []).map((entry) => ({ - value: String(entry.categoryid), - label: entry.categoryname, - })), - [categories.data], - ); + /** + * The tenant's own categories, with a floor for a merchant that has none yet. + * + * `gettenantcategories` is synthesised from the categories a tenant's products + * ALREADY use, which makes it right for an established shop and empty for a + * new one — and a new shop is exactly who is doing their first import. Without + * a floor the picker would be empty, the upload blocked, and the only way to + * get a category would be to import a product, which is the thing being + * blocked. + * + * The floor is the category the customer app actually browses. Measured, not + * assumed: every tenant with products reports category 2 and nothing else, + * category 2 carries the ten real retail subcategories (Vegetables & Fruits, + * Dairy Deli & Egg, and so on), and `scripts/appgap.mjs` shows the app asking + * for `categoryid: 2`. + * + * Note what is deliberately NOT offered: `getproductcategories`, the master + * table, returns `1001: vegetables` for every tenant. It looks like a + * perfectly good category and is a trap — the app does not browse it, so + * anything filed there is invisible to shoppers. + */ + const categoryOptions = useMemo(() => { + const own = (categories.data ?? []).map((entry) => ({ + value: String(entry.categoryid), + label: entry.categoryname, + })); + if (own.length > 0) return own; + if (categories.isLoading) return []; + return [{ value: String(APP_BROWSE_CATEGORY), label: 'General (the category the app shows)' }]; + }, [categories.data, categories.isLoading]); /** * A single branch is not a choice, so it is made rather than offered. @@ -153,10 +185,10 @@ export function ImportScope({ value, onChange, isTenantFixed = false }: ImportSc ) : null} - {value.tenantid && categoryOptions.length === 0 && !categories.isLoading ? ( - - This merchant has no categories yet. Products imported without one are invisible in the - customer app, so add a category before uploading. + {value.tenantid && (categories.data ?? []).length === 0 && !categories.isLoading ? ( + + This merchant has no categories of its own yet, so the one the customer app browses is + offered instead. Their first import is what creates the list. ) : null} diff --git a/src/features/nearle-admin/import/SheetImportPanel.tsx b/src/features/nearle-admin/import/SheetImportPanel.tsx index 1fe660d..3062b49 100644 --- a/src/features/nearle-admin/import/SheetImportPanel.tsx +++ b/src/features/nearle-admin/import/SheetImportPanel.tsx @@ -165,7 +165,11 @@ export function SheetImportPanel({ tenantid, locationid }: SheetImportPanelProps setShelving(null); setIsWorking(true); try { - const products = productsOf(batch); + // Narrowed to OUR file. A run can be assembled from several drops, so + // its manifest may carry other senders' products — and matching those + // against our sheet would price and stock someone else's goods into this + // merchant's branch. + const products = productsOf(batch, file ? [file.name] : undefined); const nextPlan = planOpeningStock(products, parsed.rows); setPlan(nextPlan); @@ -211,13 +215,9 @@ export function SheetImportPanel({ tenantid, locationid }: SheetImportPanelProps ), }, - { - key: 'categoryid', - header: 'Category', - align: 'end', - width: { type: 'pixel', value: 100 }, - renderCell: (row) => {row.categoryid}, - }, + /* No Category column. The console no longer reads one from the sheet — it + is chosen once above and applied to every row — so a column here would + show 0 on every line and invite somebody to "fix" it in the file. */ { key: 'retailprice', header: 'Retail', @@ -535,9 +535,14 @@ export function SheetImportPanel({ tenantid, locationid }: SheetImportPanelProps /> - Required columns: productname, productsku, categoryid, retailprice, productcost. The - catalogue's category names do not map to a tenant's own ids, so categoryid must be in - the file. + Only a product-name column is required. Everything else is optional: a price, a cost + and an opening stock are used when present, and a SKU makes the match back to your rows + exact rather than by name. +
+ You do not need a categoryid column — the category chosen above applies to every row + that does not name one. Leaving it out is the safer shape: a wrong id in a sheet is + accepted silently and hides the product from shoppers, while the picker can only offer + ids the customer app actually browses. {parseError ? ( diff --git a/src/features/nearle-admin/import/openingStock.test.ts b/src/features/nearle-admin/import/openingStock.test.ts index be177d7..8680b68 100644 --- a/src/features/nearle-admin/import/openingStock.test.ts +++ b/src/features/nearle-admin/import/openingStock.test.ts @@ -157,3 +157,50 @@ test('a fractional or negative opening stock is made sane', () => { const b = buildImportRequests(plan2, { tenantid: 1, locationid: 2, fallbackCategoryId: 2, catalogueIds: ids }); assert.equal(b.requests[0]?.quantity, 3); }); + +/* ── A sheet with no categoryid column at all ─────────────────────────────── */ + +// parseProductSheet defaults a missing categoryid to 0, and 0 is the value the +// customer app rejects outright. The picker's choice is what must fill it — +// otherwise dropping the column would silently hide every product uploaded. +test('a sheet with no category column gets the operator choice on every row', () => { + const rows = [ + sheetRow({ productname: 'Amul Butter 100g', productsku: 'A1', categoryid: 0 }), + sheetRow({ productname: 'Amul Ghee 1L', productsku: 'A2', categoryid: 0 }), + ]; + const products = [ + manifest({ product_sku: 'A1' }), + manifest({ image_id: 'amul_amul_ghee_1l', product_name: 'Amul Ghee 1L', product_sku: 'A2' }), + ]; + + const plan = planOpeningStock(products, rows); + const { requests } = buildImportRequests(plan, { + tenantid: 1141, + locationid: 1179, + fallbackCategoryId: 2, + catalogueIds: new Map([ + ['amul_amul_butter_100g', 42], + ['amul_amul_ghee_1l', 43], + ]), + }); + + assert.equal(requests.length, 2); + for (const req of requests) { + assert.equal(req.categoryid, 2, 'never 0 — that is the invisible state'); + } +}); + +// The sheet no longer supplies a category at all — parseProductSheet stopped +// mapping the column, so every row arrives as 0 and the operator answer is the +// only source. This is the guard on that: if a row ever carries a stray value +// it is still used, but nothing in the parser produces one any more. +test('the operator answer applies even when a row somehow carries zero', () => { + const plan = planOpeningStock([manifest()], [sheetRow({ categoryid: 0 })]); + const { requests } = buildImportRequests(plan, { + tenantid: 1141, + locationid: 1179, + fallbackCategoryId: 2, + catalogueIds: ids, + }); + assert.equal(requests[0]?.categoryid, 2); +}); diff --git a/src/features/nearle-admin/import/parseProductSheet.test.ts b/src/features/nearle-admin/import/parseProductSheet.test.ts new file mode 100644 index 0000000..09b9043 --- /dev/null +++ b/src/features/nearle-admin/import/parseProductSheet.test.ts @@ -0,0 +1,58 @@ +/** + * What the console reads out of a merchant's spreadsheet. + * + * Driven through the real parser with a real File, because the behaviour under + * test is header matching and that is exactly what a hand-built fixture would + * skip past. + */ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { parseProductSheet } from './parseProductSheet'; + +const sheet = (csv: string) => new File([csv], 'products.csv', { type: 'text/csv' }); + +// The template ships a `Category` column holding a NAME, for the ingest service. +// It used to be aliased onto `categoryid`, run through toNumber, and become 0 — +// silently, and while counting as a mapped column so it never showed up as +// ignored either. +test('a text Category column is not read as a category id', async () => { + const parsed = await parseProductSheet( + sheet('Product Name,Brand,Category,MRP\nBritannia Marie Gold 250g,Britannia,Biscuits,30\n'), + ); + + assert.equal(parsed.rows.length, 1); + assert.equal(parsed.rows[0]?.categoryid, 0, 'the console supplies the category, not the sheet'); + assert.ok( + parsed.unmappedColumns.some((column) => /category/i.test(column)), + `Category should be reported as a column we do not read, got ${JSON.stringify(parsed.unmappedColumns)}`, + ); +}); + +// Even a literal categoryid column is ignored now: a number nobody can verify +// by looking at it is the failure mode this removes. +test('an explicit categoryid column is ignored', async () => { + const parsed = await parseProductSheet( + sheet('Product Name,categoryid,MRP\nAmul Butter 100g,1001,60\n'), + ); + assert.equal(parsed.rows[0]?.categoryid, 0, '1001 would have hidden the product from shoppers'); +}); + +// Everything the console DOES still read has to keep working. +test('price, cost, sku and opening stock are still read', async () => { + const parsed = await parseProductSheet( + sheet('Product Name,SKU,MRP,Cost,Opening Stock\nAmul Ghee 1L,ACME-GHE-1,540,470,18\n'), + ); + const row = parsed.rows[0]!; + assert.equal(row.productname, 'Amul Ghee 1L'); + assert.equal(row.productsku, 'ACME-GHE-1'); + assert.equal(row.retailprice, 540); + assert.equal(row.productcost, 470); + assert.equal(row.quantity, 18); +}); + +// Only a product name is required — the ingest service's rule, not ours. +test('a sheet with nothing but product names is valid', async () => { + const parsed = await parseProductSheet(sheet('Product Name\nBritannia Good Day 200g\n')); + assert.equal(parsed.rows.length, 1); + assert.equal(parsed.issues.filter((issue) => issue.field === 'productname').length, 0); +}); diff --git a/src/features/nearle-admin/import/parseProductSheet.ts b/src/features/nearle-admin/import/parseProductSheet.ts index 290043d..a238f8b 100644 --- a/src/features/nearle-admin/import/parseProductSheet.ts +++ b/src/features/nearle-admin/import/parseProductSheet.ts @@ -20,8 +20,27 @@ async function loadXlsx() { const COLUMN_ALIASES: Record = { productname: ['productname', 'product', 'name', 'itemname', 'description'], productsku: ['productsku', 'sku', 'code', 'itemcode', 'barcode'], - categoryid: ['categoryid', 'category'], - subcategoryid: ['subcategoryid', 'subcategory'], + /** + * Deliberately EMPTY: the console no longer reads a category from the sheet. + * + * It is chosen once per upload in the scope picker instead, from the tenant's + * own list, and applied to every row. Two reasons, and the second is the one + * that bit: + * + * - A number typed on every row is a number nobody can verify by looking at + * it. A wrong one is accepted silently and hides the product from + * shoppers entirely — the customer app rejects `categoryid` 0 and browses + * only its own category, so a plausible-looking value like 1001 (which + * `getproductcategories` returns for every tenant) produces products that + * upload, price, stock, and cannot be found. + * - `category` was an alias here, so the TEXT column in our own template + * ("Biscuits") was read as a category id, failed `toNumber`, and became 0 + * without a word. It was also counted as a mapped column, so it never + * appeared in `unmappedColumns` either. That column belongs to the ingest + * service, which reads it as a name; it was never ours. + */ + categoryid: [], + subcategoryid: [], retailprice: ['retailprice', 'price', 'mrp', 'sellingprice'], productcost: ['productcost', 'cost', 'purchaseprice', 'costprice'], taxpercent: ['taxpercent', 'tax', 'gst', 'gstpercent'], @@ -121,8 +140,11 @@ export async function parseProductSheet(file: File): Promise { const productcost = toNumber(picked.productcost); const taxpercent = toNumber(picked.taxpercent) ?? 0; const quantity = toNumber(picked.quantity) ?? 0; - const categoryid = toNumber(picked.categoryid) ?? 0; - const subcategoryid = toNumber(picked.subcategoryid) ?? 0; + // Always zero — see COLUMN_ALIASES. The category is the operator's answer + // for the whole upload, filled in by `buildImportRequests`, not a per-row + // number read from the file. + const categoryid = 0; + const subcategoryid = 0; // Only the product name is required, and that is the ingest service's rule // rather than ours: everything else is optional and enriched when blank,