diff --git a/src/features/catalogue/tenantCategories.test.ts b/src/features/catalogue/tenantCategories.test.ts new file mode 100644 index 0000000..297b102 --- /dev/null +++ b/src/features/catalogue/tenantCategories.test.ts @@ -0,0 +1,43 @@ +import { strict as assert } from 'node:assert'; +import { test } from 'node:test'; +import { APP_BROWSE_CATEGORY, categoryOptionsFor } from './tenantCategories'; + +test('a new tenant is offered a category, so it can import its first product', () => { + // The regression. `gettenantcategories` is derived from products the tenant + // ALREADY has, so a shop with none gets []. CatalogueBrowser then refuses + // every import ("no category to file into") and the only way to acquire a + // category is to import a product. Measured on 2026-09-02: 37 of the 45 most + // recent tenants had zero categories and could not add their first catalogue + // product at all. + const options = categoryOptionsFor([], false); + assert.equal(options.length, 1); + assert.equal(options[0]?.value, String(APP_BROWSE_CATEGORY)); +}); + +test('that first product is filed where the customer app will actually find it', () => { + // Not merely "some category". Anything other than 2 imports a product no + // shopper can see — which is the exact failure the refusal existed to + // prevent, so a floor that got this wrong would be worse than the deadlock. + assert.equal(categoryOptionsFor([], false)[0]?.value, '2'); +}); + +test("an established tenant's own categories are left alone", () => { + const own = [ + { categoryid: 2, categoryname: 'Grocery' }, + { categoryid: 9, categoryname: 'Chilled' }, + ]; + assert.deepEqual(categoryOptionsFor(own, false), [ + { value: '2', label: 'Grocery' }, + { value: '9', label: 'Chilled' }, + ]); +}); + +test('nothing is offered while the real answer is still in flight', () => { + // Showing "General" mid-load and swapping it for the tenant's real list a + // moment later moves where a product lands without the merchant touching it. + assert.deepEqual(categoryOptionsFor(undefined, true), []); +}); + +test('a missing list settles the same way as an empty one', () => { + assert.equal(categoryOptionsFor(undefined, false).length, 1); +}); diff --git a/src/features/catalogue/tenantCategories.ts b/src/features/catalogue/tenantCategories.ts new file mode 100644 index 0000000..9bb9e93 --- /dev/null +++ b/src/features/catalogue/tenantCategories.ts @@ -0,0 +1,55 @@ +import type { ProductCategory } from '@/api/types'; + +/** + * 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. + * + * Measured, not assumed: every tenant on the platform that has products reports + * category 2 and nothing else, and category 2 carries the ten real retail + * subcategories. + */ +export const APP_BROWSE_CATEGORY = 2; + +export interface CategoryOption { + value: string; + label: string; +} + +/** + * A tenant's categories, with a floor for one that has none yet. + * + * `gettenantcategories` is synthesised from the categories a tenant's products + * ALREADY use. That makes it right for an established shop and empty for a new + * one — and a new shop is exactly who is importing their first product. + * + * Without a floor the two rules deadlock: the catalogue refuses to import + * without a category (correctly — `categoryid` 0 is invisible to shoppers), and + * the only way to acquire a category is to import a product. Measured on + * 2026-09-02: tenant 1148 had 0 products and 0 categories and could not add its + * first one from the catalogue at all. + * + * The floor is the category the app actually browses, so a product filed under + * it is sellable rather than merely stored. 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, + * because the app never asks for it. + * + * `isLoading` returns an empty list rather than the floor: offering it while the + * real answer is still in flight would show a shop a category it does not use + * and then swap it underneath them. + */ +export function categoryOptionsFor( + categories: readonly ProductCategory[] | undefined, + isLoading: boolean, +): CategoryOption[] { + const own = (categories ?? []).map((entry) => ({ + value: String(entry.categoryid), + label: entry.categoryname, + })); + if (own.length > 0) return own; + if (isLoading) return []; + return [{ value: String(APP_BROWSE_CATEGORY), label: 'General (the category the app shows)' }]; +} diff --git a/src/features/nearle-admin/import/ImportScope.tsx b/src/features/nearle-admin/import/ImportScope.tsx index 1e3d5da..784545c 100644 --- a/src/features/nearle-admin/import/ImportScope.tsx +++ b/src/features/nearle-admin/import/ImportScope.tsx @@ -23,16 +23,7 @@ import { Selector } from '@astryxdesign/core/Selector'; 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; +import { categoryOptionsFor } from '@/features/catalogue/tenantCategories'; /** Everything an upload needs before it can be turned into stock. */ export interface ImportTarget { @@ -100,15 +91,10 @@ export function ImportScope({ value, onChange, isTenantFixed = false }: ImportSc * 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]); + const categoryOptions = useMemo( + () => categoryOptionsFor(categories.data, categories.isLoading), + [categories.data, categories.isLoading], + ); /** * A single branch is not a choice, so it is made rather than offered. diff --git a/src/features/onboarding/OnboardingGate.tsx b/src/features/onboarding/OnboardingGate.tsx index e6ca607..a24c261 100644 --- a/src/features/onboarding/OnboardingGate.tsx +++ b/src/features/onboarding/OnboardingGate.tsx @@ -2,8 +2,9 @@ import { useEffect } from 'react'; import { useLocation, useNavigate } from 'react-router-dom'; import { useAuth } from '@/auth/AuthContext'; import { useBranchScope } from '@/features/store-admin/BranchScope'; -import { useOwnTenant } from '@/queries/hooks'; -import { readOnboarding } from './onboardingState'; +import { useLocationProducts } from '@/queries/hooks'; +import { readOnboarding, writeOnboarding } from './onboardingState'; +import { shouldOfferSetup } from './shouldOfferSetup'; /** * Sends a first-time merchant to setup instead of the dashboard. @@ -11,41 +12,63 @@ import { readOnboarding } from './onboardingState'; * Renders nothing — it is a decision, not a screen, and it makes that decision * once. Three rules keep it from becoming a trap: * - * - **Only once.** After the first redirect, `started` is set and nobody is - * ever sent again. Somebody who leaves setup has left it. + * - **Only once.** The redirect records `started`, so nobody is ever sent + * twice. Somebody who leaves setup has left it. * - **Only from the landing page.** A merchant who deep-links to Sales, or is * already reading Inventory, is not hauled away from what they opened. - * - **Only when there is something to do.** A shop whose profile is already - * filled in is not a first-time user, however new the account. + * - **Only when there is something to do.** A shop already trading is not a + * first-time user, however new the account. * - * The redirect waits for the shop record. Judging "incomplete" while the query - * is still in flight would redirect every merchant on every first paint. + * ── Why "something to do" is measured in products ─────────────────────────── + * + * This used to ask whether the shop had a name, an address and a phone number, + * and so it never fired once — measured 2026-09-02 on tenants 1150, 1148 and + * 1146, every one of them newly created and every one of them already carrying + * all three. It could not have been otherwise: `createtenantuser` COLLECTS + * exactly those fields, so Nearle Admin fills them in at the moment the tenant + * is created, before the merchant has ever signed in. The test was reading the + * onboarding form's own output and concluding the merchant had been onboarded. + * + * Products are the honest question. A shop with none cannot sell anything, so + * it is a shop that still needs setup no matter how complete its profile looks; + * a shop with stock on the shelf is working and must not be interrupted. + * + * The product list is awaited before deciding. Judging "incomplete" while it is + * in flight would redirect every merchant on their first paint — including the + * established ones this is written to leave alone. */ export function OnboardingGate() { const navigate = useNavigate(); const { pathname } = useLocation(); const { user } = useAuth(); const { tenantid } = useBranchScope(); - const shop = useOwnTenant(tenantid || undefined); + // Tenant-wide: a merchant with stock in any branch is trading. `allBranches` + // because no single outlet is selected this early. + const products = useLocationProducts(tenantid || undefined, undefined, 0, { allBranches: true }); useEffect(() => { - if (!user?.userid || !tenantid || shop.isLoading || !shop.data) return; - if (pathname !== '/admin/console') return; + if (!user?.userid || !tenantid) return; const state = readOnboarding(user.userid, tenantid); - if (state.started || state.finished) return; - - // "Set up" means the shop can be found and described. Products and stock - // come later and are not a reason to interrupt somebody. - const record = shop.data as unknown as Record; - const hasBasics = - String(record['tenantname'] ?? '').trim() !== '' && - String(record['address'] ?? '').trim() !== '' && - String(record['primarycontact'] ?? '').trim() !== ''; - if (hasBasics) return; + const isReady = !products.isLoading && !!products.data; + if (!shouldOfferSetup({ pathname, isReady, productCount: products.data?.length ?? 0, state })) { + return; + } + // Recorded BEFORE navigating, so the decision cannot be taken twice. Without + // this the merchant is trapped: the welcome screen offers no way out, so + // clicking Console to leave setup lands back on `/admin/console` and is + // redirected here again, forever. + writeOnboarding(user.userid, tenantid, { ...state, started: true }); navigate('/admin/onboarding', { replace: true }); - }, [user?.userid, tenantid, shop.isLoading, shop.data, pathname, navigate]); + }, [ + user?.userid, + tenantid, + products.isLoading, + products.data, + pathname, + navigate, + ]); return null; } diff --git a/src/features/onboarding/shouldOfferSetup.test.ts b/src/features/onboarding/shouldOfferSetup.test.ts new file mode 100644 index 0000000..595f152 --- /dev/null +++ b/src/features/onboarding/shouldOfferSetup.test.ts @@ -0,0 +1,65 @@ +import { strict as assert } from 'node:assert'; +import { test } from 'node:test'; +import { EMPTY_DELIVERY, EMPTY_STORE, type OnboardingState } from './onboardingState'; +import { shouldOfferSetup } from './shouldOfferSetup'; + +const FRESH: OnboardingState = { + step: 'welcome', + completed: [], + skipped: [], + finished: false, + started: false, + store: EMPTY_STORE, + delivery: EMPTY_DELIVERY, +}; + +const at = (over: Partial[0]> = {}) => + shouldOfferSetup({ + pathname: '/admin/console', + isReady: true, + productCount: 0, + state: FRESH, + ...over, + }); + +test('a brand-new store admin is sent to setup on first login', () => { + // The reported bug, and it survived a day of testing because the old rule + // asked whether the shop had a name, address and phone — which + // `createtenantuser` fills in at creation, so it was true of every new + // tenant and the redirect never fired once. + assert.equal(at(), true); +}); + +test('a shop whose profile Nearle Admin already filled in is STILL offered setup', () => { + // Tenants 1150, 1148 and 1146 were all newly created and all carried a name, + // an address and a phone number. A complete profile is evidence about who + // created the tenant, not about whether the merchant has been onboarded. + assert.equal(at({ productCount: 0 }), true); +}); + +test('a trading shop is never interrupted', () => { + // R mart, 24 products. Stock on the shelf means somebody is working. + assert.equal(at({ productCount: 24 }), false); +}); + +test('the offer is made once, so declining it is possible', () => { + // Without this the merchant is trapped: the welcome screen offers no way out, + // so clicking Console to leave lands on `/admin/console` and bounces back. + assert.equal(at({ state: { ...FRESH, started: true } }), false); +}); + +test('somebody who finished setup is not sent through it again', () => { + assert.equal(at({ state: { ...FRESH, finished: true } }), false); +}); + +test('nobody is hauled away from the page they opened', () => { + for (const pathname of ['/admin/sales', '/admin/inventory', '/admin/reports']) { + assert.equal(at({ pathname }), false, pathname); + } +}); + +test('no decision is taken while the shop and its products are still loading', () => { + // An empty product list mid-flight looks exactly like a shop with no stock, + // which would redirect every established merchant on their first paint. + assert.equal(at({ isReady: false }), false); +}); diff --git a/src/features/onboarding/shouldOfferSetup.ts b/src/features/onboarding/shouldOfferSetup.ts new file mode 100644 index 0000000..8bc6f2f --- /dev/null +++ b/src/features/onboarding/shouldOfferSetup.ts @@ -0,0 +1,41 @@ +import type { OnboardingState } from './onboardingState'; + +/** + * Whether to send this merchant to setup rather than the dashboard. + * + * Pulled out of `OnboardingGate` so it can be tested without a router, a query + * client or a DOM — the previous version's bug was a single wrong boolean, and + * a wrong boolean inside a `useEffect` is invisible to every test in this repo. + */ +export interface SetupDecision { + /** Where the merchant is. The offer is only made from the landing page. */ + pathname: string; + /** False while either query is still in flight — decide on facts, not blanks. */ + isReady: boolean; + /** How many products the tenant has, across every branch. */ + productCount: number; + state: OnboardingState; +} + +export function shouldOfferSetup({ + pathname, + isReady, + productCount, + state, +}: SetupDecision): boolean { + // A merchant who opened Sales or Inventory directly is working. Only somebody + // who arrived at the dashboard is between things. + if (pathname !== '/admin/console') return false; + + // Redirecting on incomplete data would catch established shops on first paint. + if (!isReady) return false; + + // Offered once. `started` is recorded by the redirect itself, not by the + // merchant accepting it, so declining is possible at all. + if (state.started || state.finished) return false; + + // The honest test of a first-time user. NOT the shop's name, address and + // phone: `createtenantuser` collects those, so Nearle Admin fills them in + // before the merchant ever signs in and they are true of every new tenant. + return productCount === 0; +} diff --git a/src/features/store-admin/CataloguePanel.tsx b/src/features/store-admin/CataloguePanel.tsx index 60063a9..85a0109 100644 --- a/src/features/store-admin/CataloguePanel.tsx +++ b/src/features/store-admin/CataloguePanel.tsx @@ -2,6 +2,7 @@ import { useMemo } from 'react'; import { useTenantCategories } from '@/queries/hooks'; import { CatalogueBrowser } from '@/features/catalogue/CatalogueBrowser'; import { useBranchScope } from './BranchScope'; +import { categoryOptionsFor } from '@/features/catalogue/tenantCategories'; /** * The Store Admin's half of the catalogue: browse it, add to your own products. @@ -35,16 +36,16 @@ export function CataloguePanel() { * return under any category it asks for. It was not an opt-out from filing a * product; it was an opt-out from selling it, offered as the default. * - * A tenant with no categories now gets an explicit refusal from - * `CatalogueBrowser` rather than a silent 0. + * A tenant with no categories of its own is not refused either, which was + * the overcorrection: the refusal was right about `categoryid` 0 and wrong + * about who it caught. `gettenantcategories` is derived from products a + * tenant already has, so the shops it blocked were new ones importing their + * first product — 37 of the 45 most recent tenants. See + * `categoryOptionsFor` for the floor that resolves it. */ const categoryOptions = useMemo( - () => - (categories.data ?? []).map((entry) => ({ - value: String(entry.categoryid), - label: entry.categoryname, - })), - [categories.data], + () => categoryOptionsFor(categories.data, categories.isLoading), + [categories.data, categories.isLoading], ); return (