diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 4797d7a..ef53078 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -1,12 +1,11 @@ import { Router, Request, Response } from 'express'; import { PoolClient } from 'pg'; import { pool, requireRow } from '../db'; -import { readId } from '../utils'; import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect'; import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; -import { tagColorFor } from '../utils'; +import { readId, tagColorFor } from '../utils'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; // The upload pipeline moved to src/imageUpload.ts when #222's public intake // endpoint became a second caller. Mounting uploadImages gets the type diff --git a/backend/src/routes/adminSettings.ts b/backend/src/routes/adminSettings.ts index d4bd8db..1d715d1 100644 --- a/backend/src/routes/adminSettings.ts +++ b/backend/src/routes/adminSettings.ts @@ -17,42 +17,92 @@ router.get('/', asyncRoute(async (_req: Request, res: Response) => { res.json(await getSettings()); })); +/** + * One submitted value, checked. + * + * A refusal is returned rather than sent, so each reader below is a pure + * function of its input and the handler keeps sole responsibility for the + * response. That is also what lets the handler be one loop instead of four: + * the branching lives in these, one or two conditions each, rather than + * accumulating in the route. + */ +type Reading = + | { ok: true; value: number | string } + | { ok: false; error: string } + | { skip: true }; + +const SKIP = { skip: true } as const; + +function readHours(name: SettingName, raw: unknown): Reading { + if (raw === undefined) return SKIP; + const hours = parseFloat(String(raw)); + if (Number.isNaN(hours) || hours <= 0) { + return { ok: false, error: `${name} must be a positive number` }; + } + return { ok: true, value: hours }; +} + +function readText(name: SettingName, raw: unknown): Reading { + if (raw === undefined) return SKIP; + if (typeof raw !== 'string' || raw.trim() === '') { + // Wrong for the two settings whose documented default is empty — + // intakeNotifyEmail and intakeCeilingResetAt cannot currently be cleared. + // Left as it was here deliberately: this change is the complexity refactor, + // and folding a behaviour fix into it would hide the fix. See #280. + return { ok: false, error: `${name} cannot be empty` }; + } + return { ok: true, value: raw }; +} + +/** + * Membership is checked here rather than left to the dropdown. A value outside + * the set would be stored happily and then fail on every submission, surfacing + * only as drafts quietly not appearing (#223). + */ +function readChoice(name: SettingName, raw: unknown): Reading { + if (raw === undefined) return SKIP; + if (typeof raw !== 'string' || !isValidChoice(name as never, raw)) { + const allowed = CHOICE_OPTIONS[name as never] as readonly string[]; + return { ok: false, error: `${name} must be one of: ${allowed.join(', ')}` }; + } + return { ok: true, value: raw }; +} + +/** + * Each group of settings with the reader that validates it. + * + * A table rather than four copies of the same loop. The loops were identical + * apart from their validation, and having four of them was most of this + * handler's cognitive complexity — 18 against a limit of 15, which is what + * SonarQube flagged as the only CRITICAL smell in the project (#181). Adding a + * type now means adding a row. + * + * The `count` settings from #227 are deliberately absent, exactly as before + * this refactor: nothing sends them, the admin screen has no control for them, + * and adding validation for a field no caller submits would be widening the + * behaviour under cover of a complexity fix. + */ +const GROUPS: readonly { + names: readonly SettingName[]; + read: (name: SettingName, raw: unknown) => Reading; +}[] = [ + { names: HOURS_SETTINGS, read: readHours }, + { names: TEXT_SETTINGS, read: readText }, + { names: CHOICE_SETTINGS, read: readChoice } +]; + router.put('/', asyncRoute(async (req: Request, res: Response) => { const values: Partial> = {}; // Only what was sent is validated and written, so a caller updating one field // does not have to echo the others back to avoid clobbering them. - for (const name of HOURS_SETTINGS) { - const raw = req.body[name]; - if (raw === undefined) continue; - const hours = parseFloat(raw); - if (Number.isNaN(hours) || hours <= 0) { - return res.status(400).json({ error: `${name} must be a positive number` }); + for (const group of GROUPS) { + for (const name of group.names) { + const reading = group.read(name, req.body[name]); + if ('skip' in reading) continue; + if (!reading.ok) return res.status(400).json({ error: reading.error }); + values[name] = reading.value; } - values[name] = hours; - } - - for (const name of TEXT_SETTINGS) { - const raw = req.body[name]; - if (raw === undefined) continue; - if (typeof raw !== 'string' || raw.trim() === '') { - return res.status(400).json({ error: `${name} cannot be empty` }); - } - values[name] = raw; - } - - // Membership is checked here rather than left to the dropdown. A value - // outside the set would be stored happily and then fail on every submission, - // surfacing only as drafts quietly not appearing (#223). - for (const name of CHOICE_SETTINGS) { - const raw = req.body[name]; - if (raw === undefined) continue; - if (typeof raw !== 'string' || !isValidChoice(name, raw)) { - return res - .status(400) - .json({ error: `${name} must be one of: ${CHOICE_OPTIONS[name].join(', ')}` }); - } - values[name] = raw; } await updateSettings(values); diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 34e2cc6..2785366 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -119,6 +119,33 @@ const STOREFRONT_DIMENSIONS = [ priceDimension ]; +// Hoisted out of the component that used to declare it inline. +// +// S6478 flags a function-returning-JSX in a prop as "defining a component +// during render". Here it is a render prop — ErrorBoundary's `fallback` is +// typed `(error: Error) => React.ReactNode` and called as +// `this.props.fallback(...)` — so React only ever sees the returned elements, +// never a new component type, and the subtree destruction the rule warns about +// does not happen. The rule's own message offers `allowAsProps` for exactly +// this shape, which cannot be set from here. +// +// Hoisting rather than suppressing because it costs nothing: these close over +// nothing local, so at module level they are one stable function instead of a +// new closure per render, which is mildly better and not a contortion. See #181. +function catalogueErrorFallback(error: Error) { + return ( + window.location.reload()}> + Reload + + } + /> + ); +} + export default function App() { const [authModalOpen, setAuthModalOpen] = useState(false); const [searchParams, setSearchParams] = useSearchParams(); @@ -231,17 +258,7 @@ export default function App() { // the existing end-to-end assertion that the fallback renders still // holds. resetKey={filterKey} - fallback={(error) => ( - window.location.reload()}> - Reload - - } - /> - )} + fallback={catalogueErrorFallback} > {import.meta.env.DEV && } part !== ''); const status = parsedStatus.length > 0 && parsedStatus.every(isPublicStatus) - ? (parsedStatus as ItemStatus[]) + ? parsedStatus : null; const favorites = params.get('favorites'); diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index c8ee9a6..d377603 100755 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -69,6 +69,72 @@ function usePrefersReducedMotion(): boolean { // looking at, rather than a page of its own. It stays a real, linkable URL — // bookmarkable, refreshable, and closed by the browser's Back button — while // never being a place with no way out of it. +// Both hoisted out of the components that declared them inline. +// +// S6478 flags a function-returning-JSX in a prop as "defining a component +// during render". These are render props — ErrorBoundary's `fallback` is typed +// `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)` +// — so React only ever sees the returned elements, never a new component type, +// and the subtree destruction the rule warns about does not happen. The rule's +// own message offers `allowAsProps` for exactly this shape, which cannot be set +// from here. +// +// Hoisting rather than suppressing because it costs nothing: neither closes +// over anything local, so at module level each is one stable function instead +// of a new closure per render. See #181. +function modalErrorFallback(error: Error) { + return ( + , and + // antd would otherwise announce the dialog's accessible name and then the + // identical heading right after it. + footer={null} + onCancel={() => { + window.location.href = '/'; + }} + > + { + window.location.href = '/'; + }} + > + Close + + } + /> + + ); +} + +function pageErrorFallback(error: Error) { + return ( + window.location.reload()}> + Reload + , + + ]} + /> + ); +} + function AppRoutes() { const location = useLocation(); const navigate = useNavigate(); @@ -114,33 +180,7 @@ function AppRoutes() { {/* Rendered outside the Routes above, which are showing the backdrop. */} ( - , and antd would otherwise announce the dialog's accessible - // name and then the identical heading right after it. - footer={null} - onCancel={() => { - window.location.href = '/'; - }} - > - { - window.location.href = '/'; - }} - > - Close - - } - /> - - )} + fallback={modalErrorFallback} > {/* Unconditional, so /?boom=modal fires this boundary with the storefront rendered behind it — no session needed. */} @@ -190,26 +230,7 @@ function Root() { ( - window.location.reload()}> - Reload - , - - ]} - /> - )} + fallback={pageErrorFallback} >