diff --git a/backend/app/domains/products/diff.py b/backend/app/domains/products/diff.py index 976a5bb..3dc4b3c 100644 --- a/backend/app/domains/products/diff.py +++ b/backend/app/domains/products/diff.py @@ -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 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 -(§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); file variants match catalog variants by their option-value combination (INV-13). """ @@ -69,6 +71,8 @@ class VariantPlan: kind: str # "add" | "update" canonical: CanonicalVariant 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 changes: dict[str, object] = field(default_factory=dict) @@ -145,7 +149,10 @@ def _add_plan(product: CanonicalProduct) -> ProductPlan: kind="add", canonical=product, 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=[ ImagePlan(kind="add", source_url=i.source_url, position=i.position, 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()}, "option_names": list(plan.canonical.option_names), "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 ], "images": [ @@ -200,17 +208,20 @@ def _update_plan(product: CanonicalProduct, current: CatalogProduct) -> tuple[Pr variant_plans: list[VariantPlan] = [] variant_entries: list[dict] = [] 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) 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( - {"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 variant_changes: dict[str, object] = {} 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 # fields{} — compare it explicitly (as images do for theirs). 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)) if variant_changes: 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( {"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]: - return {f: _json_safe(v) for f, v in resolved_fields(variant.fields).items()} +def resolved_variant_fields(variant: CanonicalVariant, file_order: int) -> dict[str, object]: + """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: diff --git a/backend/app/domains/products/service.py b/backend/app/domains/products/service.py index b235445..3e0ce96 100644 --- a/backend/app/domains/products/service.py +++ b/backend/app/domains/products/service.py @@ -107,9 +107,13 @@ def confirm_draft(conn: psycopg.Connection, storefront_id: int, account_id: int, catalog = repo.load_catalog(conn, storefront_id) diff_result = diff.compute_diff(catalog, products) if diff_result.fingerprint != row["fingerprint"]: + # Release the read snapshot; nothing written. + conn.rollback() raise PreviewStale() summary_counts = diff_result.summary if summary_counts["adds"] + summary_counts["updates"] == 0: + # Release the read snapshot; nothing written. + conn.rollback() raise NothingToApply() error_rows = [ error.as_json() @@ -179,12 +183,12 @@ def _apply_product_plan(conn: psycopg.Connection, storefront_id: int, else: 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: if variant_plan.kind == "add": - fields = diff.resolved_fields(variant_plan.canonical.fields) - position = fields.get("position") or file_positions[id(variant_plan.canonical)] + fields = diff.resolved_variant_fields(variant_plan.canonical, variant_plan.file_order) + # 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") image_id = image_ids[url] if url else None repo.insert_variant( diff --git a/backend/tests/test_products_diff.py b/backend/tests/test_products_diff.py index 55a2f79..c7ccc86 100644 --- a/backend/tests/test_products_diff.py +++ b/backend/tests/test_products_diff.py @@ -113,6 +113,24 @@ def test_changed_variant_position_reports_honest_before(): 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(): diff = compute_diff({}, _canon("Handle,Title,Variant Price", "mug,Mug,nope")) [rec] = diff.records diff --git a/backend/tests/test_products_service.py b/backend/tests/test_products_service.py index c2ceff9..542b6ad 100644 --- a/backend/tests/test_products_service.py +++ b/backend/tests/test_products_service.py @@ -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) +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): draft = _validate(migrated_conn, merchant, MIXED_CSV) run_id = products.confirm_draft(