product name alone made required
This commit is contained in:
@@ -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);
|
||||
});
|
||||
|
||||
@@ -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 ?? []);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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
|
||||
</Text>
|
||||
) : null}
|
||||
|
||||
{value.tenantid && categoryOptions.length === 0 && !categories.isLoading ? (
|
||||
<Text type="body" size="sm" style={{ color: 'var(--color-warning, #b7860b)' }}>
|
||||
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 ? (
|
||||
<Text type="body" size="xsm" color="secondary">
|
||||
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.
|
||||
</Text>
|
||||
) : null}
|
||||
</VStack>
|
||||
|
||||
@@ -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
|
||||
</VStack>
|
||||
),
|
||||
},
|
||||
{
|
||||
key: 'categoryid',
|
||||
header: 'Category',
|
||||
align: 'end',
|
||||
width: { type: 'pixel', value: 100 },
|
||||
renderCell: (row) => <Text type="body" size="sm" hasTabularNumbers>{row.categoryid}</Text>,
|
||||
},
|
||||
/* 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
|
||||
/>
|
||||
|
||||
<Text type="body" size="sm" style={{ color: 'var(--color-ink-3)' }}>
|
||||
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.
|
||||
<br />
|
||||
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.
|
||||
</Text>
|
||||
|
||||
{parseError ? (
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
58
src/features/nearle-admin/import/parseProductSheet.test.ts
Normal file
58
src/features/nearle-admin/import/parseProductSheet.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
@@ -20,8 +20,27 @@ async function loadXlsx() {
|
||||
const COLUMN_ALIASES: Record<keyof SheetProductRow, string[]> = {
|
||||
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<ParsedSheet> {
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user