fix(products): serialize Variant Position explicitly — INV-12 round-trip bug
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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).
|
# (a no-option product's single variant carries all-NULL options).
|
||||||
if product.option_names[slot - 1] and value is not None:
|
if product.option_names[slot - 1] and value is not None:
|
||||||
row[f"Option{slot} Value"] = value
|
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():
|
for field, col in _VARIANT_FIELD_TO_COL.items():
|
||||||
if field in variant.fields:
|
if field in variant.fields:
|
||||||
row[col] = _cell(variant.fields[field])
|
row[col] = _cell(variant.fields[field])
|
||||||
|
|||||||
@@ -136,12 +136,15 @@ def _gen_catalog(seed: int) -> dict:
|
|||||||
if has_opts:
|
if has_opts:
|
||||||
sizes = rng.sample(["S", "M", "L", "XL"], rng.randint(1, 4))
|
sizes = rng.sample(["S", "M", "L", "XL"], rng.randint(1, 4))
|
||||||
for vi, size in enumerate(sizes, start=1):
|
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(
|
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}))
|
fields={"sku": f"SKU-{pid}-{vi}", "variant_image": None}))
|
||||||
else:
|
else:
|
||||||
variants.append(CatalogVariant(
|
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}))
|
fields={"sku": f"SKU-{pid}", "variant_image": None}))
|
||||||
images = []
|
images = []
|
||||||
for ii in range(rng.randint(0, 3)):
|
for ii in range(rng.randint(0, 3)):
|
||||||
@@ -194,3 +197,28 @@ def test_decimal_and_int_variant_fields_roundtrip():
|
|||||||
result = _roundtrip_diff(catalog)
|
result = _roundtrip_diff(catalog)
|
||||||
assert result.summary["unchanged"] == 1, result.records
|
assert result.summary["unchanged"] == 1, result.records
|
||||||
assert result.summary["updates"] == 0
|
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
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
// Products page (SD-0002 §5.2) — the catalog's home: where imports start and history
|
// Products page (SD-0002 §5.2) — the catalog's home: where imports start, exports
|
||||||
// lives. SLICE-5: export is disabled (SLICE-6 ships it); the browsable list is #14's.
|
// download, and history lives. SLICE-6 ships the export menu; the browsable list is #14's.
|
||||||
import { useEffect, useState } from "react";
|
import { useEffect, useState } from "react";
|
||||||
import {
|
import {
|
||||||
dialectLabel,
|
dialectLabel,
|
||||||
@@ -11,6 +11,7 @@ import {
|
|||||||
type RunSummary,
|
type RunSummary,
|
||||||
} from "../../productsApi";
|
} from "../../productsApi";
|
||||||
import { Banner } from "../../ui/kit";
|
import { Banner } from "../../ui/kit";
|
||||||
|
import { isExportEnabled } from "./exportMenu";
|
||||||
|
|
||||||
const STATUS_LABELS: Record<string, string> = {
|
const STATUS_LABELS: Record<string, string> = {
|
||||||
applying: "Importing…",
|
applying: "Importing…",
|
||||||
@@ -56,7 +57,7 @@ export default function ProductsPage() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
const empty = summary.product_count === 0;
|
const empty = !isExportEnabled(summary.product_count);
|
||||||
return (
|
return (
|
||||||
<div className="products">
|
<div className="products">
|
||||||
<header className="products__header">
|
<header className="products__header">
|
||||||
|
|||||||
Reference in New Issue
Block a user