From 667a462e0b972059a44bc90f338c8566c4451e7d Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Thu, 11 Jun 2026 15:37:59 -0700 Subject: [PATCH] fix(products): honest position compare for matched variants in diff A Variant Position column previously produced a spurious update with a false before:None (CatalogVariant keeps position as an attribute, not in fields{}). Compare against the attribute, like images already do. Co-Authored-By: Claude Fable 5 --- backend/app/domains/products/diff.py | 17 ++++++++++------- backend/tests/test_products_diff.py | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/backend/app/domains/products/diff.py b/backend/app/domains/products/diff.py index ddd94b0..6c9b5bf 100644 --- a/backend/app/domains/products/diff.py +++ b/backend/app/domains/products/diff.py @@ -9,7 +9,8 @@ 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. -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). """ from __future__ import annotations @@ -89,7 +90,7 @@ def compute_diff(catalog: dict[str, CatalogProduct], products: list[CanonicalPro ) summary[_SUMMARY_KEY[kind]] += 1 fingerprint = hashlib.sha256( - json.dumps(records, sort_keys=True, separators=(",", ":"), default=str).encode() + json.dumps(records, sort_keys=True, separators=(",", ":")).encode() ).hexdigest() return DiffResult(records=records, summary=summary, fingerprint=fingerprint) @@ -140,11 +141,13 @@ def _update_detail(product: CanonicalProduct, current: CatalogProduct) -> dict: {"options": list(variant.options), "kind": "add", "set": _resolved_variant_fields(variant)} ) continue - variant_changes = [ - _change(f, match.fields.get(f), resolved) - for f, resolved in _resolved_fields(variant.fields).items() - if resolved != match.fields.get(f) - ] + variant_changes = [] + for f, resolved in _resolved_fields(variant.fields).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) + if resolved != before: + variant_changes.append(_change(f, before, resolved)) if variant_changes: variant_entries.append( {"options": list(variant.options), "kind": "update", "changes": variant_changes} diff --git a/backend/tests/test_products_diff.py b/backend/tests/test_products_diff.py index 4421d51..55a2f79 100644 --- a/backend/tests/test_products_diff.py +++ b/backend/tests/test_products_diff.py @@ -95,6 +95,24 @@ def test_new_option_combo_is_variant_add_existing_untouched(): assert "add" in kinds +POSITION_HEADER = HEADER + ",Variant Position" + + +def test_matching_variant_position_column_classifies_unchanged(): + # position is an attribute on CatalogVariant (not in fields{}); the compare + # must read it from there, not invent a before:None. + diff = compute_diff(_catalog_mug(), _canon(POSITION_HEADER, MUG_ROW + ",1")) + assert diff.records[0]["kind"] == "unchanged" + + +def test_changed_variant_position_reports_honest_before(): + diff = compute_diff(_catalog_mug(), _canon(POSITION_HEADER, MUG_ROW + ",2")) + [rec] = diff.records + assert rec["kind"] == "update" + [ventry] = rec["detail"]["variants"] + assert {"field": "position", "before": 1, "after": 2} in ventry["changes"] + + def test_error_product_classifies_error(): diff = compute_diff({}, _canon("Handle,Title,Variant Price", "mug,Mug,nope")) [rec] = diff.records