fix(products): cleared Variant Position resolves to file order, not NULL
A blank Variant Position cell previewed position->null and aborted the confirm transaction (variant.position is NOT NULL). Resolve the clear to the variant's file order at diff time so preview and apply stay in lockstep; carry file_order on VariantPlan instead of an identity-keyed map; release the read snapshot before PreviewStale/NothingToApply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -11,7 +11,9 @@ fingerprint over the records detect catalog drift between preview and confirm.
|
|||||||
Deliberately DB-free: the catalog snapshot dataclasses are defined here and the
|
Deliberately DB-free: the catalog snapshot dataclasses are defined here and the
|
||||||
repo layer builds them. Only fields present in the file's canonical fields{}
|
repo layer builds them. Only fields present in the file's canonical fields{}
|
||||||
participate in a comparison — an absent column is untouched, never a change
|
participate in a comparison — an absent column is untouched, never a change
|
||||||
(§6.5.1); a present-but-empty cell resolves to the field's CLEAR_DEFAULTS entry.
|
(§6.5.1); a present-but-empty cell resolves to the field's CLEAR_DEFAULTS entry
|
||||||
|
— except a variant's position, whose default is the variant's 1-based file
|
||||||
|
order within its product ("defaults to file order"), resolved here at diff time.
|
||||||
Catalog variants/images absent from the file are likewise untouched (INV-10);
|
Catalog variants/images absent from the file are likewise untouched (INV-10);
|
||||||
file variants match catalog variants by their option-value combination (INV-13).
|
file variants match catalog variants by their option-value combination (INV-13).
|
||||||
"""
|
"""
|
||||||
@@ -69,6 +71,8 @@ class VariantPlan:
|
|||||||
kind: str # "add" | "update"
|
kind: str # "add" | "update"
|
||||||
canonical: CanonicalVariant
|
canonical: CanonicalVariant
|
||||||
catalog_id: int | None # None for add
|
catalog_id: int | None # None for add
|
||||||
|
# 1-based index within the product's file variants — the position default.
|
||||||
|
file_order: int = 0
|
||||||
# field -> resolved after-value (update only); adds resolve from canonical.fields
|
# field -> resolved after-value (update only); adds resolve from canonical.fields
|
||||||
changes: dict[str, object] = field(default_factory=dict)
|
changes: dict[str, object] = field(default_factory=dict)
|
||||||
|
|
||||||
@@ -145,7 +149,10 @@ def _add_plan(product: CanonicalProduct) -> ProductPlan:
|
|||||||
kind="add",
|
kind="add",
|
||||||
canonical=product,
|
canonical=product,
|
||||||
catalog=None,
|
catalog=None,
|
||||||
variant_plans=[VariantPlan(kind="add", canonical=v, catalog_id=None) for v in product.variants],
|
variant_plans=[
|
||||||
|
VariantPlan(kind="add", canonical=v, catalog_id=None, file_order=order)
|
||||||
|
for order, v in enumerate(product.variants, start=1)
|
||||||
|
],
|
||||||
image_plans=[
|
image_plans=[
|
||||||
ImagePlan(kind="add", source_url=i.source_url, position=i.position,
|
ImagePlan(kind="add", source_url=i.source_url, position=i.position,
|
||||||
alt_text=i.alt_text, image_id=None)
|
alt_text=i.alt_text, image_id=None)
|
||||||
@@ -164,7 +171,8 @@ def _add_detail(plan: ProductPlan) -> dict:
|
|||||||
"set": {f: _json_safe(v) for f, v in set_fields.items()},
|
"set": {f: _json_safe(v) for f, v in set_fields.items()},
|
||||||
"option_names": list(plan.canonical.option_names),
|
"option_names": list(plan.canonical.option_names),
|
||||||
"variants": [
|
"variants": [
|
||||||
{"options": list(vp.canonical.options), "set": _resolved_variant_fields(vp.canonical)}
|
{"options": list(vp.canonical.options),
|
||||||
|
"set": _resolved_variant_fields(vp.canonical, vp.file_order)}
|
||||||
for vp in plan.variant_plans
|
for vp in plan.variant_plans
|
||||||
],
|
],
|
||||||
"images": [
|
"images": [
|
||||||
@@ -200,17 +208,20 @@ def _update_plan(product: CanonicalProduct, current: CatalogProduct) -> tuple[Pr
|
|||||||
variant_plans: list[VariantPlan] = []
|
variant_plans: list[VariantPlan] = []
|
||||||
variant_entries: list[dict] = []
|
variant_entries: list[dict] = []
|
||||||
by_options = {v.options: v for v in current.variants}
|
by_options = {v.options: v for v in current.variants}
|
||||||
for variant in product.variants:
|
for file_order, variant in enumerate(product.variants, start=1):
|
||||||
match = by_options.get(variant.options)
|
match = by_options.get(variant.options)
|
||||||
if match is None:
|
if match is None:
|
||||||
variant_plans.append(VariantPlan(kind="add", canonical=variant, catalog_id=None))
|
variant_plans.append(
|
||||||
|
VariantPlan(kind="add", canonical=variant, catalog_id=None, file_order=file_order)
|
||||||
|
)
|
||||||
variant_entries.append(
|
variant_entries.append(
|
||||||
{"options": list(variant.options), "kind": "add", "set": _resolved_variant_fields(variant)}
|
{"options": list(variant.options), "kind": "add",
|
||||||
|
"set": _resolved_variant_fields(variant, file_order)}
|
||||||
)
|
)
|
||||||
continue
|
continue
|
||||||
variant_changes: dict[str, object] = {}
|
variant_changes: dict[str, object] = {}
|
||||||
variant_change_entries: list[dict] = []
|
variant_change_entries: list[dict] = []
|
||||||
for f, resolved in resolved_fields(variant.fields).items():
|
for f, resolved in resolved_variant_fields(variant, file_order).items():
|
||||||
# position lives on the catalog variant as an attribute, not in
|
# position lives on the catalog variant as an attribute, not in
|
||||||
# fields{} — compare it explicitly (as images do for theirs).
|
# fields{} — compare it explicitly (as images do for theirs).
|
||||||
before = match.position if f == "position" else match.fields.get(f)
|
before = match.position if f == "position" else match.fields.get(f)
|
||||||
@@ -219,7 +230,8 @@ def _update_plan(product: CanonicalProduct, current: CatalogProduct) -> tuple[Pr
|
|||||||
variant_change_entries.append(_change(f, before, resolved))
|
variant_change_entries.append(_change(f, before, resolved))
|
||||||
if variant_changes:
|
if variant_changes:
|
||||||
variant_plans.append(
|
variant_plans.append(
|
||||||
VariantPlan(kind="update", canonical=variant, catalog_id=match.id, changes=variant_changes)
|
VariantPlan(kind="update", canonical=variant, catalog_id=match.id,
|
||||||
|
file_order=file_order, changes=variant_changes)
|
||||||
)
|
)
|
||||||
variant_entries.append(
|
variant_entries.append(
|
||||||
{"options": list(variant.options), "kind": "update", "changes": variant_change_entries}
|
{"options": list(variant.options), "kind": "update", "changes": variant_change_entries}
|
||||||
@@ -284,8 +296,18 @@ def resolved_product_fields(product: CanonicalProduct) -> dict[str, object]:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def _resolved_variant_fields(variant: CanonicalVariant) -> dict[str, object]:
|
def resolved_variant_fields(variant: CanonicalVariant, file_order: int) -> dict[str, object]:
|
||||||
return {f: _json_safe(v) for f, v in resolved_fields(variant.fields).items()}
|
"""File-present variant fields with clears resolved. A cleared position has
|
||||||
|
no CLEAR_DEFAULTS entry — it resets to the variant's 1-based file order
|
||||||
|
within its product ("defaults to file order"), never to NULL."""
|
||||||
|
resolved = resolved_fields(variant.fields)
|
||||||
|
if "position" in resolved and resolved["position"] is None:
|
||||||
|
resolved["position"] = file_order
|
||||||
|
return resolved
|
||||||
|
|
||||||
|
|
||||||
|
def _resolved_variant_fields(variant: CanonicalVariant, file_order: int) -> dict[str, object]:
|
||||||
|
return {f: _json_safe(v) for f, v in resolved_variant_fields(variant, file_order).items()}
|
||||||
|
|
||||||
|
|
||||||
def _change(field_name: str, before: object, after: object) -> dict:
|
def _change(field_name: str, before: object, after: object) -> dict:
|
||||||
|
|||||||
@@ -107,9 +107,13 @@ def confirm_draft(conn: psycopg.Connection, storefront_id: int, account_id: int,
|
|||||||
catalog = repo.load_catalog(conn, storefront_id)
|
catalog = repo.load_catalog(conn, storefront_id)
|
||||||
diff_result = diff.compute_diff(catalog, products)
|
diff_result = diff.compute_diff(catalog, products)
|
||||||
if diff_result.fingerprint != row["fingerprint"]:
|
if diff_result.fingerprint != row["fingerprint"]:
|
||||||
|
# Release the read snapshot; nothing written.
|
||||||
|
conn.rollback()
|
||||||
raise PreviewStale()
|
raise PreviewStale()
|
||||||
summary_counts = diff_result.summary
|
summary_counts = diff_result.summary
|
||||||
if summary_counts["adds"] + summary_counts["updates"] == 0:
|
if summary_counts["adds"] + summary_counts["updates"] == 0:
|
||||||
|
# Release the read snapshot; nothing written.
|
||||||
|
conn.rollback()
|
||||||
raise NothingToApply()
|
raise NothingToApply()
|
||||||
error_rows = [
|
error_rows = [
|
||||||
error.as_json()
|
error.as_json()
|
||||||
@@ -179,12 +183,12 @@ def _apply_product_plan(conn: psycopg.Connection, storefront_id: int,
|
|||||||
else:
|
else:
|
||||||
repo.update_image(conn, image_plan.image_id, image_plan.changes)
|
repo.update_image(conn, image_plan.image_id, image_plan.changes)
|
||||||
|
|
||||||
# File order within the product, for variant adds without an explicit position.
|
|
||||||
file_positions = {id(v): i for i, v in enumerate(plan.canonical.variants, start=1)}
|
|
||||||
for variant_plan in plan.variant_plans:
|
for variant_plan in plan.variant_plans:
|
||||||
if variant_plan.kind == "add":
|
if variant_plan.kind == "add":
|
||||||
fields = diff.resolved_fields(variant_plan.canonical.fields)
|
fields = diff.resolved_variant_fields(variant_plan.canonical, variant_plan.file_order)
|
||||||
position = fields.get("position") or file_positions[id(variant_plan.canonical)]
|
# diff time resolved any cleared position to file order; the
|
||||||
|
# file_order fallback covers an absent position column.
|
||||||
|
position = fields.get("position") or variant_plan.file_order
|
||||||
url = fields.get("variant_image")
|
url = fields.get("variant_image")
|
||||||
image_id = image_ids[url] if url else None
|
image_id = image_ids[url] if url else None
|
||||||
repo.insert_variant(
|
repo.insert_variant(
|
||||||
|
|||||||
@@ -113,6 +113,24 @@ def test_changed_variant_position_reports_honest_before():
|
|||||||
assert {"field": "position", "before": 1, "after": 2} in ventry["changes"]
|
assert {"field": "position", "before": 1, "after": 2} in ventry["changes"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_blank_position_cell_resolves_to_file_order_unchanged():
|
||||||
|
# A present-but-empty Variant Position cell resets to file order (the spec's
|
||||||
|
# "defaults to file order"), never to NULL — here file order matches the
|
||||||
|
# catalog position, so nothing changes.
|
||||||
|
diff = compute_diff(_catalog_mug(), _canon(POSITION_HEADER, MUG_ROW + ","))
|
||||||
|
assert diff.records[0]["kind"] == "unchanged"
|
||||||
|
|
||||||
|
|
||||||
|
def test_blank_position_cell_updates_to_file_order():
|
||||||
|
catalog = _catalog_mug()
|
||||||
|
catalog["moon-mug"].variants[0].position = 2
|
||||||
|
diff = compute_diff(catalog, _canon(POSITION_HEADER, MUG_ROW + ","))
|
||||||
|
[rec] = diff.records
|
||||||
|
assert rec["kind"] == "update"
|
||||||
|
[ventry] = rec["detail"]["variants"]
|
||||||
|
assert {"field": "position", "before": 2, "after": 1} in ventry["changes"]
|
||||||
|
|
||||||
|
|
||||||
def test_error_product_classifies_error():
|
def test_error_product_classifies_error():
|
||||||
diff = compute_diff({}, _canon("Handle,Title,Variant Price", "mug,Mug,nope"))
|
diff = compute_diff({}, _canon("Handle,Title,Variant Price", "mug,Mug,nope"))
|
||||||
[rec] = diff.records
|
[rec] = diff.records
|
||||||
|
|||||||
@@ -150,6 +150,25 @@ def test_confirm_update_changes_only_diffed_fields(migrated_conn, merchant):
|
|||||||
assert migrated_conn.execute("SELECT count(*) FROM product").fetchone()[0] == 2 # no dupes (BUC-3)
|
assert migrated_conn.execute("SELECT count(*) FROM product").fetchone()[0] == 2 # no dupes (BUC-3)
|
||||||
|
|
||||||
|
|
||||||
|
def test_confirm_blank_position_cell_round_trips(migrated_conn, merchant):
|
||||||
|
# A present-but-empty Variant Position cell resolves to file order at diff
|
||||||
|
# time — never SET position = NULL (which would abort the confirm on the
|
||||||
|
# NOT NULL constraint).
|
||||||
|
d1 = _validate(migrated_conn, merchant)
|
||||||
|
products.confirm_draft(migrated_conn, merchant["storefront_id"], merchant["account_id"], d1["id"])
|
||||||
|
blank_position_csv = (
|
||||||
|
b"Handle,Title,Vendor,Variant Price,Variant Position\n"
|
||||||
|
b"moon-mug,Moon Mug,Acme,21.00,\n"
|
||||||
|
b"star-tee,Star Tee,Acme,24.00,\n"
|
||||||
|
)
|
||||||
|
d2 = _validate(migrated_conn, merchant, blank_position_csv)
|
||||||
|
products.confirm_draft(migrated_conn, merchant["storefront_id"], merchant["account_id"], d2["id"])
|
||||||
|
price = migrated_conn.execute(
|
||||||
|
"SELECT v.price FROM variant v JOIN product p ON p.id = v.product_id WHERE p.handle='moon-mug'"
|
||||||
|
).fetchone()[0]
|
||||||
|
assert str(price) == "21.00"
|
||||||
|
|
||||||
|
|
||||||
def test_confirm_mixed_applies_valid_records_errors(migrated_conn, merchant):
|
def test_confirm_mixed_applies_valid_records_errors(migrated_conn, merchant):
|
||||||
draft = _validate(migrated_conn, merchant, MIXED_CSV)
|
draft = _validate(migrated_conn, merchant, MIXED_CSV)
|
||||||
run_id = products.confirm_draft(
|
run_id = products.confirm_draft(
|
||||||
|
|||||||
Reference in New Issue
Block a user