From bde20c8539b1cdb89d9d8bc2268888b5eb1071db Mon Sep 17 00:00:00 2001 From: Elstonyth <124340021+elstonyth@users.noreply.github.com> Date: Thu, 30 Jul 2026 01:51:33 +0800 Subject: [PATCH 1/6] feat(admin): bulk import products from the PriceCharting collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Inventory > "Add from PC Collection", the bulk sibling of the existing single-item Add-from-PriceCharting page. The operator scans their own PriceCharting collection, filters it, and onboards many holdings in one pass through the SAME creation path — same per-grade price lookup, same single-item endpoint, same batch runner — so an imported product is indistinguishable from a hand-searched one. Shape follows what the live API actually returns (measured against the real collection, 9,000+ offers): - The collection is five figures and still growing, so the route serves ONE upstream page per request and hands back the cursor; the admin page scans with live counters, a Stop/Continue control, and a 300-row render cap. A server-side walk would have been a multi-minute request that reports no progress and dies on any timeout. - `seller` is NOT required: the API token identifies the account (a wrong seller 400s upstream with "user not found"). PRICECHARTING_SELLER_ID stays supported for the case where the token's account is not the owner. - Grade tags upstream are free text — "Ungraded", "PSA 10", "Graded 9", "BGS 10 Black", "ACE 10", plus the video-game condition family. gradeForIncludeString maps them onto PRICE_FIELDS labels and returns null for anything PriceCharting has no field for, so the operator picks the tier rather than the import guessing on a money field. Money and inventory correctness: - market_value is ALWAYS the live per-grade FMV re-read through /admin/pricecharting/product, never the offer's own valuation. The nightly sync skips moves beyond MAX_SYNC_DELTA_RATIO, so an offer-valued import would not be corrected later — it would stick. - Holdings that would mint the same product (same PriceCharting product at the same grade tag) are merged into one draft with units summed. Importing them separately collided on the generated handle and undercounted stock. - The offer's own grade tag wins over the price tier when recording grader/grade: a collection offer IS the slab, unlike a search hit. Half grades stay blank rather than minting an off-scale grade. Also: offer images are accepted only from PriceCharting's own bucket (the create step stores non-PC URLs verbatim instead of ingesting them), and CardPokemonFields takes an idPrefix so N pickers on one screen stop sharing three hardcoded DOM ids. Tests: unit coverage for the tag mapping and offer normalization from real upstream rows, a stubbed-fetch spec pinning the outgoing request (status, cursor pass-through, optional seller), and an http spec for mounting + auth + the missing-token setup message. The happy path was verified once against the live API through the booted route (cursor pages disjoint; a "Graded 9" holding priced at its Grade 9 tier) — that spec is not committed, as CI has no token. Co-Authored-By: Claude Opus 5 --- .../src/components/GraderGradeSelect.tsx | 20 +- backend/apps/admin/src/i18n/en.json | 45 + backend/apps/admin/src/lib/admin-rest.ts | 33 + backend/apps/admin/src/lib/format.test.ts | 41 +- backend/apps/admin/src/lib/format.ts | 48 +- .../src/routes/cards/CardPokemonFields.tsx | 16 +- .../products/from-pc-collection/page.tsx | 835 ++++++++++++++++++ backend/packages/api/.env.template | 5 + .../http/pc-collection.spec.ts | 64 ++ .../__tests__/collection-offers.unit.spec.ts | 194 ++++ .../__tests__/collection-route.unit.spec.ts | 147 +++ .../admin/pricecharting/collection-offers.ts | 79 ++ .../admin/pricecharting/collection/route.ts | 72 ++ .../src/modules/packs/pricecharting-grades.ts | 48 + 14 files changed, 1618 insertions(+), 29 deletions(-) create mode 100644 backend/apps/admin/src/routes/products/from-pc-collection/page.tsx create mode 100644 backend/packages/api/integration-tests/http/pc-collection.spec.ts create mode 100644 backend/packages/api/src/api/admin/pricecharting/__tests__/collection-offers.unit.spec.ts create mode 100644 backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts create mode 100644 backend/packages/api/src/api/admin/pricecharting/collection-offers.ts create mode 100644 backend/packages/api/src/api/admin/pricecharting/collection/route.ts diff --git a/backend/apps/admin/src/components/GraderGradeSelect.tsx b/backend/apps/admin/src/components/GraderGradeSelect.tsx index 8ada16bf3..303544fe9 100644 --- a/backend/apps/admin/src/components/GraderGradeSelect.tsx +++ b/backend/apps/admin/src/components/GraderGradeSelect.tsx @@ -1,24 +1,6 @@ import { useTranslation } from 'react-i18next'; import { Label, Select } from '@medusajs/ui'; - -// Client mirror of PSA's canonical 11-point scale (backend source of truth: -// packages/api/src/api/admin/media/label.ts PSA_GRADES — keep in sync). -// Qualifier half-grades (2.5–9.5) deliberately excluded (§3a): the catalog -// doesn't carry them, and 9.5 is a PriceCharting tier, never a PSA grade. -// 1.5 stays — PSA's base FR grade. -const PSA_GRADES = [ - '10', - '9', - '8', - '7', - '6', - '5', - '4', - '3', - '2', - '1.5', - '1', -] as const; +import { PSA_GRADES } from '../lib/format'; const GRADERS = ['PSA', 'BGS', 'CGC', 'SGC'] as const; const NONE = '__none__'; // @medusajs/ui Select rejects '' as an item value diff --git a/backend/apps/admin/src/i18n/en.json b/backend/apps/admin/src/i18n/en.json index 7f8a84967..6e4b5f28c 100644 --- a/backend/apps/admin/src/i18n/en.json +++ b/backend/apps/admin/src/i18n/en.json @@ -179,6 +179,51 @@ "queueSkippedCapped": "{{n}} created, {{skipped}} skipped. Errors not shown: {{hidden}}." } }, + "pcCollection": { + "title": "Add from PriceCharting collection", + "subtitle": "Scan your own PriceCharting collection and onboard many holdings in one pass. It runs to thousands of items, so the scan pages through with live counters and can be stopped as soon as what you want is on screen. Importing re-reads each pick at its live per-grade value (not the collection valuation); you then set units held and link a pixel Pokémon before saving the batch.", + "fetch": "Scan my collection", + "refetch": "Rescan from the start", + "stop": "Stop scan", + "continue": "Continue scan", + "progress": "{{scanned}} scanned ({{pages}} pages) · {{shown}} shown", + "complete": "Whole collection scanned", + "partial": "Partial — scan stopped early", + "empty": "No holdings came back from PriceCharting.", + "filter": "Filter by name or set…", + "cardsOnly": "Cards only", + "gradedOnly": "Graded only", + "selectAll": "Select all shown", + "deselectAll": "Deselect all shown", + "import": "Import selected ({{n}})", + "importing": "Pricing {{done}} / {{total}}…", + "tierUnmapped": "Pick after import", + "pickTier": "Pick a grade tier", + "valueHint": "Value shown is PriceCharting's valuation of your holding, for triage only. Import re-reads the live per-grade price so it matches what the nightly sync writes.", + "drafts": "Imported ({{n}}) — finish these before saving", + "slabLine": "PriceCharting tag {{tag}} · recorded as {{grader}}", + "noGrader": "no grader (tier is a price comp)", + "remove": "Remove import", + "save": "Save {{n}} products", + "col": { + "item": "Item", + "pcTag": "PriceCharting tag", + "tier": "Grade tier", + "offerValue": "Holding value" + }, + "skip": { + "noImage": "{{name}}: PriceCharting has no card photo — add it on the search page.", + "noPrices": "{{name}}: PriceCharting lists no prices for this product." + }, + "toast": { + "imported": "{{n}} imported — set units and pixel Pokémon, then save.", + "skipped": "{{skipped}} skipped.", + "skippedCapped": "{{skipped}} skipped. Errors not shown: {{hidden}}." + }, + "stopImport": "Stop import", + "rowCap": "Showing the first {{shown}} of {{total}} matches — narrow the filter to reach the rest.", + "merged": "{{n}} holdings of this card at this grade — one product, units preset to {{n}}." + }, "cards": { "title": "Gacha Cards", "subtitle": "Inventory products registered as gacha prizes. Add the item to the product catalog first, then register it here; rarity is chosen per pack in each pack's win-rate editor.", diff --git a/backend/apps/admin/src/lib/admin-rest.ts b/backend/apps/admin/src/lib/admin-rest.ts index dd587e164..0fd831ec1 100644 --- a/backend/apps/admin/src/lib/admin-rest.ts +++ b/backend/apps/admin/src/lib/admin-rest.ts @@ -448,6 +448,39 @@ export async function getPriceChartingProduct(id: string): Promise { return data.product; } +// One holding in the operator's own PriceCharting collection. Carries no FMV: +// `value_usd` is PriceCharting's valuation of that offer, shown for triage only — +// the import re-reads the per-grade price so market_value matches what the +// nightly sync will write. A 503 means PRICECHARTING_API_TOKEN is not +// configured; surface the message and use the search page instead. +export interface PcOffer { + offer_id: string | null; + product_id: string; + name: string; + set: string; + /** PriceCharting's own grade tag, verbatim ("PSA 10", "Graded 9"). */ + include: string; + condition: string; + value_usd: number | null; + /** `include` mapped to a priceable grade tier, or null — operator picks. */ + grade: string | null; + image: string | null; + /** Upstream `is-card`: a real collection also holds games and hardware. */ + is_card: boolean; +} + +/** One page (~30) of the collection plus the cursor for the next one; an empty + * cursor means the collection is exhausted. Paged rather than walked server + * side because the collection runs to five figures. */ +export async function getPriceChartingCollection( + cursor = '', +): Promise<{ offers: PcOffer[]; cursor: string }> { + return getJson<{ offers: PcOffer[]; cursor: string }>( + '/admin/pricecharting/collection' + + (cursor ? `?cursor=${encodeURIComponent(cursor)}` : ''), + ); +} + // §7a label prefill: year (set release) + note (rarity) from pokemontcg.io, // proxied server-side. Always resolves — a lookup miss/outage returns nulls, // never throws (see api/admin/tcg/tcg-meta.ts). diff --git a/backend/apps/admin/src/lib/format.test.ts b/backend/apps/admin/src/lib/format.test.ts index 88cee28dd..7ff1e9ca3 100644 --- a/backend/apps/admin/src/lib/format.test.ts +++ b/backend/apps/admin/src/lib/format.test.ts @@ -5,6 +5,7 @@ import { fmtPct, usdToMyr, gradeToGrader, + graderFromInclude, orderDateTime, deliveryStatusLabel, } from './format'; @@ -50,13 +51,19 @@ describe('timeAgo', () => { expect(timeAgo(new Date(now - 30_000).toISOString(), now)).toBe('just now'); }); it('returns whole minutes', () => { - expect(timeAgo(new Date(now - 5 * 60_000).toISOString(), now)).toBe('5m ago'); + expect(timeAgo(new Date(now - 5 * 60_000).toISOString(), now)).toBe( + '5m ago', + ); }); it('returns whole hours', () => { - expect(timeAgo(new Date(now - 3 * 3_600_000).toISOString(), now)).toBe('3h ago'); + expect(timeAgo(new Date(now - 3 * 3_600_000).toISOString(), now)).toBe( + '3h ago', + ); }); it('returns whole days', () => { - expect(timeAgo(new Date(now - 2 * 86_400_000).toISOString(), now)).toBe('2d ago'); + expect(timeAgo(new Date(now - 2 * 86_400_000).toISOString(), now)).toBe( + '2d ago', + ); }); it('returns an em dash for an invalid ISO string', () => { expect(timeAgo('not-a-date', now)).toBe('—'); @@ -65,10 +72,14 @@ describe('timeAgo', () => { expect(timeAgo(new Date(now - 60_000).toISOString(), now)).toBe('1m ago'); }); it('returns "1h ago" at exactly 60 minutes', () => { - expect(timeAgo(new Date(now - 3_600_000).toISOString(), now)).toBe('1h ago'); + expect(timeAgo(new Date(now - 3_600_000).toISOString(), now)).toBe( + '1h ago', + ); }); it('returns "1d ago" at exactly 24 hours', () => { - expect(timeAgo(new Date(now - 86_400_000).toISOString(), now)).toBe('1d ago'); + expect(timeAgo(new Date(now - 86_400_000).toISOString(), now)).toBe( + '1d ago', + ); }); it('clamps a future timestamp to "just now"', () => { expect(timeAgo(new Date(now + 5_000).toISOString(), now)).toBe('just now'); @@ -154,6 +165,26 @@ describe('orderDateTime', () => { }); }); +describe('graderFromInclude', () => { + it('keeps the grader a collection offer asserts', () => { + expect(graderFromInclude('PSA 9')).toEqual({ grader: 'PSA', grade: '9' }); + expect(graderFromInclude('bgs10')).toEqual({ grader: 'BGS', grade: '10' }); + expect(graderFromInclude(' cgc 8 ')).toEqual({ grader: 'CGC', grade: '8' }); + }); + + it('drops half grades — the catalog scale has no 9.5', () => { + expect(graderFromInclude('BGS 9.5')).toEqual({ grader: '', grade: '' }); + expect(graderFromInclude('PSA 8.5')).toEqual({ grader: '', grade: '' }); + }); + + it('asserts nothing from a tag that names no grader', () => { + expect(graderFromInclude('Loose')).toEqual({ grader: '', grade: '' }); + expect(graderFromInclude('Grade 9')).toEqual({ grader: '', grade: '' }); + expect(graderFromInclude('Gem Mint')).toEqual({ grader: '', grade: '' }); + expect(graderFromInclude('')).toEqual({ grader: '', grade: '' }); + }); +}); + describe('fmtPct', () => { it('formats an integer without decimals', () => { expect(fmtPct(20)).toBe('20%'); diff --git a/backend/apps/admin/src/lib/format.ts b/backend/apps/admin/src/lib/format.ts index d52bf29ad..c88e06705 100644 --- a/backend/apps/admin/src/lib/format.ts +++ b/backend/apps/admin/src/lib/format.ts @@ -94,7 +94,53 @@ export const fmtPct = (n: number): string => // wired through a new workspace package. Keep in sync if the backend changes. // Used by both the from-PriceCharting page and the register-card modal to // derive an operator-facing grader/grade suggestion from a PC tier label. -export function gradeToGrader(label: string): { grader: string; grade: string } { +// PSA's canonical 11-point scale (backend source of truth: +// packages/api/src/api/admin/media/label.ts PSA_GRADES — keep in sync). +// Qualifier half-grades (2.5–9.5) deliberately excluded (§3a): the catalog +// doesn't carry them, and 9.5 is a PriceCharting tier, never a PSA grade. +// 1.5 stays — PSA's base FR grade. +export const PSA_GRADES = [ + '10', + '9', + '8', + '7', + '6', + '5', + '4', + '3', + '2', + '1.5', + '1', +] as const; + +/** + * Grader + grade asserted by a PriceCharting collection offer's own grade tag + * ("PSA 9"). Unlike a search hit — where PriceCharting supplies only a price + * COMP and the operator states the slab — a collection offer IS the slab, so + * the tag is ground truth worth keeping even though it prices off the generic + * "Grade 9" field. + * + * Half grades return blank on purpose: PSA_GRADES carries no 8.5/9.5, so a + * "BGS 9.5" offer imports grader-less rather than minting an off-scale grade + * the card editor would render as legacy. + */ +export function graderFromInclude(include: string): { + grader: string; + grade: string; +} { + const m = /^\s*(psa|bgs|cgc|sgc)\s*(\d+(?:\.5)?)\s*$/i.exec(include); + if (!m) return { grader: '', grade: '' }; + const grade = m[2]; + if (!PSA_GRADES.includes(grade as (typeof PSA_GRADES)[number])) { + return { grader: '', grade: '' }; + } + return { grader: m[1].toUpperCase(), grade }; +} + +export function gradeToGrader(label: string): { + grader: string; + grade: string; +} { for (const g of ['PSA', 'BGS', 'CGC', 'SGC']) { if (label.startsWith(g + ' ')) { return { grader: g, grade: label.slice(g.length + 1) }; diff --git a/backend/apps/admin/src/routes/cards/CardPokemonFields.tsx b/backend/apps/admin/src/routes/cards/CardPokemonFields.tsx index a50dbcc7d..acb041f50 100644 --- a/backend/apps/admin/src/routes/cards/CardPokemonFields.tsx +++ b/backend/apps/admin/src/routes/cards/CardPokemonFields.tsx @@ -26,6 +26,11 @@ type Props = { currentDex?: number | null; /** Card/product title — the default name for a custom-uploaded entry. */ suggestionName: string; + /** Namespaces this instance's DOM ids. REQUIRED when more than one picker is + * on screen (the bulk collection import renders one per draft): without it + * every instance shares `card-pokemon-search`, so clicking any label focuses + * the FIRST input and every aria-controls points at the first listbox. */ + idPrefix?: string; }; const PICKER_LIMIT = 40; @@ -46,7 +51,10 @@ const CardPokemonFields = ({ currentSprite, currentDex, suggestionName, + idPrefix = 'card', }: Props) => { + const searchId = `${idPrefix}-pokemon-search`; + const listboxId = `${idPrefix}-pokemon-listbox`; const [search, setSearch] = useState(''); // The entry chosen THIS session — drives the preview once picked. On first // load it's null and the preview falls back to the card's mirrored sprite. @@ -174,7 +182,7 @@ const CardPokemonFields = ({ return (
-
+ patchDraft(d.key, v)} + idPrefix={`draft-${d.key}`} + /> +
{ }); }); +describe('isForeignOffer', () => { + it('flags only an explicit false', () => { + // A seller-less call comes back with every row false — that is the bug + // signature. Absent is tolerated: a correctly-scoped response with rows in + // it has never been observed (the collection is empty), so demanding + // `=== true` could empty a page that is working fine. + expect(isForeignOffer({ 'user-viewing-own-offers': false })).toBe(true); + expect(isForeignOffer({ 'user-viewing-own-offers': true })).toBe(false); + expect(isForeignOffer({})).toBe(false); + expect(isForeignOffer({ 'user-viewing-own-offers': 'false' })).toBe(false); + }); +}); + describe('normalizeOffer', () => { // Field-for-field the shape the live /api/offers returns (trimmed to the keys // the import uses — upstream rows carry ~60). @@ -155,7 +172,12 @@ describe('normalizeOffer', () => { // during the scan, and the units held would be undercounted. expect(normalizeOffer({ ...RAW, 'offer-id': undefined })).toBeNull(); expect(normalizeOffer({ ...RAW, 'offer-id': ' ' })).toBeNull(); - expect(normalizeOffer({ ...RAW, 'offer-id': 42 })).toBeNull(); + }); + + it('accepts a numeric offer id rather than emptying the page', () => { + // Dropping numerically-keyed rows would render as "collection is empty", + // indistinguishable from the operator's real empty-collection state. + expect(normalizeOffer({ ...RAW, 'offer-id': 42 })?.offer_id).toBe('42'); }); it('falls back to the listed price when the valuation is absent', () => { diff --git a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts index de429f253..9d3701fdb 100644 --- a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts +++ b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts @@ -128,6 +128,28 @@ describe('GET /admin/pricecharting/collection', () => { }); }); + it('drops rows upstream says are not ours, and says how many', async () => { + // The seller id being SET proves configuration, not correctness — a stale + // or typo'd id returns another account's holdings, which would import as + // our stock. This is the flag that exposed exactly that bug. + const { captured } = await runGet( + {}, + { + status: 'success', + offers: [ + { ...OFFER, 'user-viewing-own-offers': false }, + { ...OFFER, 'offer-id': 'mine_1', 'user-viewing-own-offers': true }, + { ...OFFER, 'offer-id': 'mine_2' }, // flag absent — tolerated + ], + }, + ); + + expect(captured.body.foreign_dropped).toBe(1); + expect( + captured.body.offers.map((o: { offer_id: string }) => o.offer_id), + ).toEqual(['mine_1', 'mine_2']); + }); + it('reports an exhausted collection as an empty cursor', async () => { const { captured } = await runGet({}, { status: 'success', offers: [] }); expect(captured.body.cursor).toBe(''); @@ -152,10 +174,13 @@ describe('GET /admin/pricecharting/collection', () => { }); it('502s when PriceCharting reports an error', async () => { - const { captured } = await runGet({}, { - status: 'error', - 'error-message': 'user not found: me', - }); + const { captured } = await runGet( + {}, + { + status: 'error', + 'error-message': 'user not found: me', + }, + ); expect(captured.status).toBe(502); expect(captured.body.message).toContain('user not found'); }); diff --git a/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts b/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts index 2b3c1fb1e..e5ad99363 100644 --- a/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts +++ b/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts @@ -37,6 +37,20 @@ export type PcOffer = { is_card: boolean; }; +/** + * Upstream marks each row with whether the requesting account owns it. FALSE is + * the signature of the bug that shipped once already: /api/offers without a + * `seller` returns everyone's offers, and every row came back false. The route + * drops those rather than trusting that PRICECHARTING_SELLER_ID is OUR id — a + * typo'd or stale one is otherwise indistinguishable from a correct one. + * + * Absent is tolerated, not rejected: a correctly-scoped response has never been + * observed with rows in it (the collection is empty), so demanding `=== true` + * could empty a working page. + */ +export const isForeignOffer = (raw: Record): boolean => + raw['user-viewing-own-offers'] === false; + /** Upstream offer values are integer PENNIES (number or digit string). */ export function penniesToUsd(raw: unknown): number | null { const n = typeof raw === 'string' ? Number(raw.replace(/[$,\s]/g, '')) : raw; @@ -63,8 +77,10 @@ export function normalizeOffer(raw: Record): PcOffer | null { const productId = raw.id != null ? String(raw.id).trim() : ''; if (productId === '') return null; + // String OR number: dropping a numerically-keyed row would empty the page + // in a way indistinguishable from a genuinely empty collection. const offerId = - typeof raw['offer-id'] === 'string' ? raw['offer-id'].trim() : ''; + raw['offer-id'] != null ? String(raw['offer-id']).trim() : ''; if (offerId === '') return null; const include = String(raw['include-string'] ?? ''); diff --git a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts index 1c3c2f351..beac2487c 100644 --- a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts +++ b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts @@ -1,6 +1,10 @@ import { MedusaRequest, MedusaResponse } from '@medusajs/framework/http'; import { pcFetch, PC_TOKEN_MISSING } from '../client'; -import { normalizeOffer, type PcOffer } from '../collection-offers'; +import { + isForeignOffer, + normalizeOffer, + type PcOffer, +} from '../collection-offers'; // GET /admin/pricecharting/collection[?cursor=…] — ONE page (~30) of the // operator's own PriceCharting collection, normalized for bulk import, plus the @@ -70,14 +74,25 @@ export async function GET( const page = Array.isArray(result.data.offers) ? result.data.offers : []; const offers: PcOffer[] = []; + // Rows upstream says are NOT ours never reach the import table. The seller id + // being set proves configuration, not correctness; this proves correctness. + let foreign = 0; for (const raw of page) { if (!raw || typeof raw !== 'object') continue; + if (isForeignOffer(raw)) { + foreign++; + continue; + } const offer = normalizeOffer(raw); if (offer) offers.push(offer); } res.json({ offers, + // Surfaced, not swallowed: a non-zero count means the configured seller id + // is not the account the token is reading, and the operator has to know + // that rather than see a short page. + foreign_dropped: foreign, // Empty when the collection is exhausted — that, not an empty page, is how // the client knows to stop. cursor: typeof result.data.cursor === 'string' ? result.data.cursor : '', diff --git a/backend/packages/api/src/workflows/steps/__tests__/pc-product-handle.unit.spec.ts b/backend/packages/api/src/workflows/steps/__tests__/pc-product-handle.unit.spec.ts new file mode 100644 index 000000000..0583f275e --- /dev/null +++ b/backend/packages/api/src/workflows/steps/__tests__/pc-product-handle.unit.spec.ts @@ -0,0 +1,65 @@ +import { buildPcProductHandle } from '../create-product-from-pricecharting'; + +// The handle is the uniqueness key for a from-PriceCharting product (the SKU is +// derived from it), so two holdings that are genuinely different items must not +// produce the same one — and two imports of the SAME item must. +const H = (grader: string, grade: string, pc_grade: string) => + buildPcProductHandle({ + name: 'Charizard V #79', + grader, + grade, + pc_grade, + pc_product_id: '836945', + }); + +describe('buildPcProductHandle', () => { + it('separates generic tiers of the same card', () => { + // The regression this exists for: PriceCharting drops the grading company + // below its top-tier fields, so "Graded 9" imports grader-less — and before + // the tier fallback it slugged identically to "Ungraded", making the second + // create fail on the unique handle/sku index mid-batch. + const ungraded = H('', '', 'Ungraded'); + const graded9 = H('', '', 'Grade 9'); + const graded8 = H('', '', 'Grade 8'); + + expect(new Set([ungraded, graded9, graded8]).size).toBe(3); + expect(graded9).toBe('charizard-v-79-grade-9-836945'); + }); + + it('is stable for the same holding — re-importing must still collide', () => { + // The unique index is also the idempotency guard: adding the same card at + // the same tier twice should fail, not silently mint a duplicate product. + expect(H('', '', 'Grade 9')).toBe(H('', '', 'Grade 9')); + expect(H('PSA', '10', 'PSA 10')).toBe(H('PSA', '10', 'PSA 10')); + }); + + it('keeps the pre-existing shape when a grader was asserted', () => { + // Byte-identical to the old `name-grader-grade-id` formula, so products + // already in the catalog keep resolving to their handle. + expect(H('PSA', '10', 'PSA 10')).toBe('charizard-v-79-psa-10-836945'); + expect(H('BGS', '9', 'Grade 9')).toBe('charizard-v-79-bgs-9-836945'); + }); + + it('keeps the pre-existing shape for a legacy grade without a grader', () => { + // Catalog rows exist in this shape (e.g. pikachu-227-s-p-9-5-4199649). + expect(H('', '9.5', 'Grade 9.5')).toBe('charizard-v-79-9-5-836945'); + }); + + it('separates two different cards that share a tier', () => { + const a = buildPcProductHandle({ + name: 'Pikachu ex #238', + grader: '', + grade: '', + pc_grade: 'Ungraded', + pc_product_id: '111', + }); + const b = buildPcProductHandle({ + name: 'Pikachu ex #238', + grader: '', + grade: '', + pc_grade: 'Ungraded', + pc_product_id: '222', + }); + expect(a).not.toBe(b); + }); +}); diff --git a/backend/packages/api/src/workflows/steps/create-product-from-pricecharting.ts b/backend/packages/api/src/workflows/steps/create-product-from-pricecharting.ts index 347924819..04ca327bd 100644 --- a/backend/packages/api/src/workflows/steps/create-product-from-pricecharting.ts +++ b/backend/packages/api/src/workflows/steps/create-product-from-pricecharting.ts @@ -55,6 +55,34 @@ const slug = (s: string) => .replace(/[^a-z0-9]+/g, '-') .replace(/^-+|-+$/g, ''); +/** + * Product handle for a PriceCharting import. The grader + grade the operator + * asserted identify the physical slab, so they key the handle — one product per + * (card, grade) rather than per card. + * + * The tier is the fallback for holdings that assert NEITHER. PriceCharting + * drops the grading company below its own top-tier price fields, so a "Graded 9" + * offer arrives grader-less, and §3a forbids inventing one; without the tier, + * "Ungraded" and "Graded 9" of the same card slug identically and the second + * create fails on the unique handle/sku index — which is exactly what a bulk + * collection import does routinely. + * + * Ordering note: the tier sits where grader/grade would be, so a handle that + * HAS a grader is byte-identical to what this produced before the fallback + * existed. Only previously-impossible (colliding) handles change shape. + */ +export const buildPcProductHandle = (input: { + name: string; + grader: string; + grade: string; + pc_grade: string; + pc_product_id: string; +}): string => { + const asserted = `${input.grader}-${input.grade}`.replace(/-/g, '').trim(); + const key = asserted === '' ? input.pc_grade : `${input.grader}-${input.grade}`; + return slug(`${input.name}-${key}-${input.pc_product_id}`); +}; + // Minimal typed view of the remote-query row (strict mode, no `any`). type NewProductRow = { variants?: Array<{ @@ -85,9 +113,7 @@ export const createProductFromPcInvoke = async ( ? await ingestPcImage(container, input.image) : input.image; - const handle = slug( - `${input.name}-${input.grader}-${input.grade}-${input.pc_product_id}`, - ); + const handle = buildPcProductHandle(input); const productInput = buildCardProductInput( { From 2783dfcd869306e4ff3f2162e3e558ab896d16bf Mon Sep 17 00:00:00 2001 From: Elstonyth <124340021+elstonyth@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:49:22 +0800 Subject: [PATCH 5/6] fix(admin): reject an invalid collection cursor instead of restarting the scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: the guard on the upstream cursor silently dropped an overlong value and answered with page 1. That restart is invisible to the scan loop — its dedupe discards every row it already has — so the loop spins to the page cap looking like a hang. A cursor that long is not a cursor, so the route 400s with a message telling the operator to rescan. Co-Authored-By: Claude Opus 5 --- .../__tests__/collection-route.unit.spec.ts | 9 ++++++--- .../admin/pricecharting/collection/route.ts | 20 +++++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts index 9d3701fdb..007a3ed7b 100644 --- a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts +++ b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts @@ -97,12 +97,15 @@ describe('GET /admin/pricecharting/collection', () => { expect(url.searchParams.get('cursor')).toBe('cur/sor+abc'); }); - it('drops an absurdly long cursor rather than forwarding it', async () => { - const { url } = await runGet( + it('rejects an absurdly long cursor instead of restarting the scan', async () => { + // Answering page 1 would be invisible to the client: its dedupe drops every + // row, so the scan spins to the page cap looking like a hang. + const { captured, url } = await runGet( { cursor: 'x'.repeat(600) }, { status: 'success', offers: [] }, ); - expect(url.searchParams.has('cursor')).toBe(false); + expect(captured.status).toBe(400); + expect(url).toBeUndefined(); }); it('trims a padded seller id', async () => { diff --git a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts index beac2487c..21736b667 100644 --- a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts +++ b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts @@ -36,6 +36,16 @@ export const PC_SELLER_MISSING = 'pricecharting.com/selling-available page). Without it PriceCharting returns ' + "every user's offers, not your collection."; +// A real PriceCharting cursor is ~40 characters. Anything past this is not a +// cursor, and the request is rejected rather than quietly answered with page 1: +// a silent restart is invisible to the scan loop (its dedupe drops every row) +// and spins to the page cap looking like a hang. +const MAX_CURSOR_LENGTH = 512; + +export const PC_CURSOR_TOO_LONG = + 'The PriceCharting cursor is not valid (too long). Rescan the collection from ' + + 'the start.'; + type PcOffersResponse = { status: string; 'error-message'?: string; @@ -53,11 +63,13 @@ export async function GET( return; } - // The cursor is the one operator-supplied value that reaches the upstream - // URL. Real ones are ~40 chars; anything longer is not a cursor. - const raw = + // The cursor is the one operator-supplied value that reaches the upstream URL. + const cursor = typeof req.query.cursor === 'string' ? req.query.cursor.trim() : ''; - const cursor = raw.length <= 512 ? raw : ''; + if (cursor.length > MAX_CURSOR_LENGTH) { + res.status(400).json({ message: PC_CURSOR_TOO_LONG }); + return; + } const params: Record = { status: 'collection', seller }; if (cursor) params.cursor = cursor; From 2fbd97088c3442ae6c150c702607082782f8f108 Mon Sep 17 00:00:00 2001 From: Elstonyth <124340021+elstonyth@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:48:52 +0800 Subject: [PATCH 6/6] fix(admin): reject unusable offer ids and non-string cursors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both the same shape: a value coerced instead of refused, producing a wrong result that looks like a normal one. - offer-id: String() on an object yields "[object Object]" — a key EVERY such row would share. The scan keys rows by offer_id, so two distinct holdings would dedupe into one and undercount the units held. Now only a string or a finite number is accepted; anything else takes the existing drop path. - cursor: a repeated ?cursor=a&cursor=b arrives as an ARRAY, which was coerced to '' and answered with page 1. That restart is invisible to the scan loop (its dedupe discards every row it already holds) and spins to the page cap looking like a hang — exactly what the length guard already rejects, so a non-string now 400s the same way. Co-Authored-By: Claude Opus 5 --- .../__tests__/collection-offers.unit.spec.ts | 9 +++++++++ .../__tests__/collection-route.unit.spec.ts | 11 ++++++++++ .../admin/pricecharting/collection-offers.ts | 14 ++++++++++--- .../admin/pricecharting/collection/route.ts | 20 ++++++++++++------- 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-offers.unit.spec.ts b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-offers.unit.spec.ts index a1c200e2e..b42dc11ef 100644 --- a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-offers.unit.spec.ts +++ b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-offers.unit.spec.ts @@ -174,6 +174,15 @@ describe('normalizeOffer', () => { expect(normalizeOffer({ ...RAW, 'offer-id': ' ' })).toBeNull(); }); + it('drops an offer id that is neither a string nor a finite number', () => { + // String({}) is "[object Object]" — a key EVERY such row would share, so + // the scan dedupe would fold distinct holdings into one and undercount. + expect(normalizeOffer({ ...RAW, 'offer-id': {} })).toBeNull(); + expect(normalizeOffer({ ...RAW, 'offer-id': [] })).toBeNull(); + expect(normalizeOffer({ ...RAW, 'offer-id': true })).toBeNull(); + expect(normalizeOffer({ ...RAW, 'offer-id': Number.NaN })).toBeNull(); + }); + it('accepts a numeric offer id rather than emptying the page', () => { // Dropping numerically-keyed rows would render as "collection is empty", // indistinguishable from the operator's real empty-collection state. diff --git a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts index 007a3ed7b..2e65fde11 100644 --- a/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts +++ b/backend/packages/api/src/api/admin/pricecharting/__tests__/collection-route.unit.spec.ts @@ -97,6 +97,17 @@ describe('GET /admin/pricecharting/collection', () => { expect(url.searchParams.get('cursor')).toBe('cur/sor+abc'); }); + it('rejects a non-string cursor instead of restarting the scan', async () => { + // Express turns a repeated ?cursor=a&cursor=b into an array; coercing it to + // '' would silently answer page 1. + const { captured, url } = await runGet( + { cursor: ['a', 'b'] }, + { status: 'success', offers: [] }, + ); + expect(captured.status).toBe(400); + expect(url).toBeUndefined(); + }); + it('rejects an absurdly long cursor instead of restarting the scan', async () => { // Answering page 1 would be invisible to the client: its dedupe drops every // row, so the scan spins to the page cap looking like a hang. diff --git a/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts b/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts index e5ad99363..3ecf8e2ff 100644 --- a/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts +++ b/backend/packages/api/src/api/admin/pricecharting/collection-offers.ts @@ -77,10 +77,18 @@ export function normalizeOffer(raw: Record): PcOffer | null { const productId = raw.id != null ? String(raw.id).trim() : ''; if (productId === '') return null; - // String OR number: dropping a numerically-keyed row would empty the page - // in a way indistinguishable from a genuinely empty collection. + // A string or a finite number, nothing else. A number is accepted because + // dropping a numerically-keyed row would empty the page in a way + // indistinguishable from a genuinely empty collection; anything else is + // refused because String() would coin a key rows can SHARE — `{}` becomes + // "[object Object]" for every such row, and the scan dedupe would then treat + // two distinct holdings as one and undercount the units held. + const rawOfferId = raw['offer-id']; const offerId = - raw['offer-id'] != null ? String(raw['offer-id']).trim() : ''; + typeof rawOfferId === 'string' || + (typeof rawOfferId === 'number' && Number.isFinite(rawOfferId)) + ? String(rawOfferId).trim() + : ''; if (offerId === '') return null; const include = String(raw['include-string'] ?? ''); diff --git a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts index 21736b667..36ed99494 100644 --- a/backend/packages/api/src/api/admin/pricecharting/collection/route.ts +++ b/backend/packages/api/src/api/admin/pricecharting/collection/route.ts @@ -42,9 +42,8 @@ export const PC_SELLER_MISSING = // and spins to the page cap looking like a hang. const MAX_CURSOR_LENGTH = 512; -export const PC_CURSOR_TOO_LONG = - 'The PriceCharting cursor is not valid (too long). Rescan the collection from ' + - 'the start.'; +export const PC_CURSOR_INVALID = + 'The PriceCharting cursor is not valid. Rescan the collection from the start.'; type PcOffersResponse = { status: string; @@ -64,10 +63,17 @@ export async function GET( } // The cursor is the one operator-supplied value that reaches the upstream URL. - const cursor = - typeof req.query.cursor === 'string' ? req.query.cursor.trim() : ''; - if (cursor.length > MAX_CURSOR_LENGTH) { - res.status(400).json({ message: PC_CURSOR_TOO_LONG }); + // Reject anything that is not a plain string of plausible length rather than + // coercing it away: a repeated ?cursor= arrives as an ARRAY, and coercion + // would answer page 1, which is invisible to the scan loop (its dedupe drops + // every row it already holds) and spins to the page cap looking like a hang. + const rawCursor = req.query.cursor; + const cursor = typeof rawCursor === 'string' ? rawCursor.trim() : ''; + if ( + (rawCursor !== undefined && typeof rawCursor !== 'string') || + cursor.length > MAX_CURSOR_LENGTH + ) { + res.status(400).json({ message: PC_CURSOR_INVALID }); return; }