From af72299a574ae36e38e3de24a5449af3da0d1d00 Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Thu, 11 Jun 2026 22:22:11 -0700 Subject: [PATCH] =?UTF-8?q?fix(products):=20serialize=20Variant=20Position?= =?UTF-8?q?=20explicitly=20=E2=80=94=20INV-12=20round-trip=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stored variant position is a CatalogVariant attribute, not a fields{} entry, so the serializer emitted an empty Variant Position cell — which re-imports as 'reset to file order'. A non-sequential stored position (a merchant can import explicit positions) then round-tripped to a spurious update, violating INV-12. Emit str(variant.position) explicitly (like _write_image). The property-test generator now assigns non-sequential positions to lock the regression, plus a targeted test. Also wires isExportEnabled into ProductsPage (was dead code). Both caught by the final code review. Co-Authored-By: Claude Fable 5 --- backend/app/domains/products/serialize.py | 5 +++ backend/tests/test_products_serialize.py | 32 +++++++++++++++++-- .../src/screens/products/ProductsPage.tsx | 7 ++-- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/backend/app/domains/products/serialize.py b/backend/app/domains/products/serialize.py index e3ca683..d085fd4 100644 --- a/backend/app/domains/products/serialize.py +++ b/backend/app/domains/products/serialize.py @@ -109,6 +109,11 @@ def _write_variant(row: dict[str, str], product: CatalogProduct, variant) -> Non # (a no-option product's single variant carries all-NULL options). if product.option_names[slot - 1] and value is not None: row[f"Option{slot} Value"] = value + # position is a CatalogVariant attribute, not a fields{} entry — emit it + # explicitly. An empty Variant Position cell re-imports as "reset to file + # order", so a non-sequential stored position would round-trip to an update + # (INV-12). (Like _write_image, which emits its position attribute.) + row["Variant Position"] = str(variant.position) for field, col in _VARIANT_FIELD_TO_COL.items(): if field in variant.fields: row[col] = _cell(variant.fields[field]) diff --git a/backend/tests/test_products_serialize.py b/backend/tests/test_products_serialize.py index b1c3222..77ca11f 100644 --- a/backend/tests/test_products_serialize.py +++ b/backend/tests/test_products_serialize.py @@ -136,12 +136,15 @@ def _gen_catalog(seed: int) -> dict: if has_opts: sizes = rng.sample(["S", "M", "L", "XL"], rng.randint(1, 4)) for vi, size in enumerate(sizes, start=1): + # Non-sequential positions (×10) so a serializer that drops the + # stored position and lets re-import default to file order is + # caught — a merchant can import explicit positions like 10/20. variants.append(CatalogVariant( - id=pid * 100 + vi, options=(size, "Indigo", None), position=vi, + id=pid * 100 + vi, options=(size, "Indigo", None), position=vi * 10, fields={"sku": f"SKU-{pid}-{vi}", "variant_image": None})) else: variants.append(CatalogVariant( - id=pid * 100 + 1, options=(None, None, None), position=1, + id=pid * 100 + 1, options=(None, None, None), position=rng.choice([1, 5, 9]), fields={"sku": f"SKU-{pid}", "variant_image": None})) images = [] for ii in range(rng.randint(0, 3)): @@ -194,3 +197,28 @@ def test_decimal_and_int_variant_fields_roundtrip(): result = _roundtrip_diff(catalog) assert result.summary["unchanged"] == 1, result.records assert result.summary["updates"] == 0 + + +def test_nonsequential_variant_position_roundtrips(): + """A stored variant position that isn't its file order must survive export — + else re-import reads the empty cell as 'reset to file order' and the round-trip + spuriously updates (INV-12 regression: position is a CatalogVariant attribute, + not a fields{} entry, so the serializer must emit it explicitly).""" + catalog = { + "tee": CatalogProduct( + id=1, handle="tee", title="Tee", option_names=("Size", None, None), + fields={"title": "Tee", "description_html": None, "vendor": None, + "product_type": "standalone", "google_product_category": None, + "tags": [], "status": "active", "published": True}, + variants=[ + CatalogVariant(id=1, options=("S", None, None), position=10, + fields={"sku": "T-S", "variant_image": None}), + CatalogVariant(id=2, options=("M", None, None), position=20, + fields={"sku": "T-M", "variant_image": None}), + ], + images=[], + ) + } + result = _roundtrip_diff(catalog) + assert result.summary["unchanged"] == 1, result.records + assert result.summary["updates"] == 0 diff --git a/frontend/src/screens/products/ProductsPage.tsx b/frontend/src/screens/products/ProductsPage.tsx index ace39e4..61ed96e 100644 --- a/frontend/src/screens/products/ProductsPage.tsx +++ b/frontend/src/screens/products/ProductsPage.tsx @@ -1,5 +1,5 @@ -// Products page (SD-0002 §5.2) — the catalog's home: where imports start and history -// lives. SLICE-5: export is disabled (SLICE-6 ships it); the browsable list is #14's. +// Products page (SD-0002 §5.2) — the catalog's home: where imports start, exports +// download, and history lives. SLICE-6 ships the export menu; the browsable list is #14's. import { useEffect, useState } from "react"; import { dialectLabel, @@ -11,6 +11,7 @@ import { type RunSummary, } from "../../productsApi"; import { Banner } from "../../ui/kit"; +import { isExportEnabled } from "./exportMenu"; const STATUS_LABELS: Record = { applying: "Importing…", @@ -56,7 +57,7 @@ export default function ProductsPage() { ); } - const empty = summary.product_count === 0; + const empty = !isExportEnabled(summary.product_count); return (