Backend scores updates on products
This commit is contained in:
437
tests/test_brand_table_scores.py
Normal file
437
tests/test_brand_table_scores.py
Normal file
@@ -0,0 +1,437 @@
|
||||
"""The score columns on the brand tables, and the mirror that fills them.
|
||||
|
||||
THE FAILURE THIS FILE EXISTS FOR
|
||||
--------------------------------
|
||||
`nutrition_score` and `health_score` live on `nutrition_insights`. A consumer
|
||||
reading product rows straight out of Postgres - the merchant console does
|
||||
exactly this - could not see them at all, because nothing joined the two.
|
||||
|
||||
The fix denormalises both onto every `brand_*` table. That creates a second
|
||||
copy of a number, and the two ways a second copy goes wrong are what most of
|
||||
this file pins:
|
||||
|
||||
1. `upsert_brand_products` overwrites every column it names. If the score
|
||||
columns were added to that INSERT, every re-seed and pipeline re-run would
|
||||
set them to NULL, because the rows arriving there are built by
|
||||
`_to_storage_row` and carry no score. So the statement must NEVER name
|
||||
them, and `test_the_insert_does_not_name_the_score_columns` is the guard.
|
||||
|
||||
2. The two score-write paths spell the brand differently - enrichment stores
|
||||
`Cadbury`, the Excel upload stores `cadbury`. An `=` match would mirror one
|
||||
and silently skip the other.
|
||||
|
||||
No database is involved: `_connect` is a recorder, so the assertions are about
|
||||
the exact SQL and parameters sent.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
|
||||
import pytest
|
||||
|
||||
from app.services import brand_sync, nutrition_score_sync, vector_store
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The columns exist
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class MigrationCursor:
|
||||
"""A brand table that exists but has none of the current columns."""
|
||||
|
||||
def __init__(self):
|
||||
self.statements = []
|
||||
|
||||
def execute(self, sql, params=None):
|
||||
self.statements.append(" ".join(str(sql).split()))
|
||||
|
||||
def fetchall(self):
|
||||
return [] # no existing columns -> every column is missing
|
||||
|
||||
|
||||
def test_the_migration_adds_both_columns_to_an_existing_table():
|
||||
"""`_ensure_columns` is the only migration mechanism there is - no Alembic,
|
||||
no migrations directory. A column it does not add is a column that never
|
||||
appears on a brand table that already exists, which is all of them."""
|
||||
cur = MigrationCursor()
|
||||
|
||||
vector_store._ensure_columns(cur, "brand_cadbury")
|
||||
|
||||
assert ("ALTER TABLE brand_cadbury ADD COLUMN IF NOT EXISTS "
|
||||
"nutrition_score NUMERIC") in cur.statements
|
||||
assert ("ALTER TABLE brand_cadbury ADD COLUMN IF NOT EXISTS "
|
||||
"health_score NUMERIC") in cur.statements
|
||||
|
||||
|
||||
def test_the_migration_still_adds_the_columns_it_always_did():
|
||||
cur = MigrationCursor()
|
||||
|
||||
vector_store._ensure_columns(cur, "brand_cadbury")
|
||||
|
||||
joined = " | ".join(cur.statements)
|
||||
for col in ("product_name", "image_id", "hsn_code", "barcode",
|
||||
"selling_price", "search_query", "embedding"):
|
||||
assert f"ADD COLUMN IF NOT EXISTS {col} " in joined
|
||||
|
||||
|
||||
def test_a_brand_new_table_is_born_with_both_columns():
|
||||
ddl = vector_store.get_brand_table_ddl("Cadbury")
|
||||
|
||||
assert "nutrition_score NUMERIC" in ddl
|
||||
assert "health_score NUMERIC" in ddl
|
||||
|
||||
|
||||
def test_the_scores_are_numeric_not_text():
|
||||
"""A TEXT score sorts lexically: '9' > '80'. Any ORDER BY or range filter
|
||||
the consumer writes would be quietly wrong."""
|
||||
ddl = vector_store.get_brand_table_ddl("Amul")
|
||||
|
||||
assert "health_score TEXT" not in ddl
|
||||
assert "nutrition_score TEXT" not in ddl
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The guard that the whole design rests on
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _upsert_source() -> str:
|
||||
import inspect
|
||||
return " ".join(inspect.getsource(vector_store.upsert_brand_products).split())
|
||||
|
||||
|
||||
def test_the_insert_does_not_name_the_score_columns():
|
||||
"""DO NOT "FIX" THIS BY ADDING THEM.
|
||||
|
||||
`upsert_brand_products` writes rows built by `_to_storage_row`,
|
||||
`_build_product_dict` and the seed loader, none of which carry a score.
|
||||
Naming the score columns in the INSERT would therefore set them to NULL on
|
||||
every re-seed, every pipeline re-run and every product edit - wiping the
|
||||
data this feature exists to provide. A column the statement never names is
|
||||
a column it cannot damage.
|
||||
"""
|
||||
src = _upsert_source()
|
||||
|
||||
insert_start = src.index("INSERT INTO {table_name}")
|
||||
statement = src[insert_start:src.index('""",', insert_start)]
|
||||
|
||||
assert "nutrition_score" not in statement, (
|
||||
"upsert_brand_products must not write nutrition_score - it would NULL "
|
||||
"it on every catalogue re-write. nutrition_score_sync owns this column."
|
||||
)
|
||||
assert "health_score" not in statement, (
|
||||
"upsert_brand_products must not write health_score - it would NULL it "
|
||||
"on every catalogue re-write. nutrition_score_sync owns this column."
|
||||
)
|
||||
|
||||
|
||||
def test_the_insert_still_writes_the_columns_it_always_did():
|
||||
"""The negative above must not have been achieved by breaking the write."""
|
||||
src = _upsert_source()
|
||||
|
||||
for col in ("product_name", "image_id", "hsn_code", "barcode",
|
||||
"selling_price", "search_query", "embedding"):
|
||||
assert f"{col} = EXCLUDED.{col}" in src or f"({col}," in src or f" {col}," in src
|
||||
|
||||
|
||||
def test_image_id_is_still_the_fifth_element_of_the_row_tuple():
|
||||
"""`image_ids = [r[4] for r in rows]` is a hard-coded positional index.
|
||||
Anything inserted before image_id in the tuple breaks the conflict target
|
||||
silently, and products start overwriting each other."""
|
||||
src = _upsert_source()
|
||||
|
||||
assert "image_ids = [r[4] for r in rows if r[4]]" in src
|
||||
columns = src[src.index("INSERT INTO {table_name}"):]
|
||||
columns = columns[columns.index("(") + 1:columns.index(")")]
|
||||
names = [c.strip() for c in columns.split(",")]
|
||||
assert names[4] == "image_id"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The mirror: brand tables
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class FakeCursor:
|
||||
def __init__(self, conn):
|
||||
self.conn = conn
|
||||
|
||||
def execute(self, sql, params=None):
|
||||
self.conn.calls.append((" ".join(str(sql).split()), params))
|
||||
|
||||
def fetchone(self):
|
||||
return (self.conn.next_count,)
|
||||
|
||||
@property
|
||||
def rowcount(self):
|
||||
return self.conn.next_count
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, *exc):
|
||||
return False
|
||||
|
||||
|
||||
class FakeConn:
|
||||
def __init__(self, next_count=1):
|
||||
self.calls = []
|
||||
self.next_count = next_count
|
||||
self.commits = 0
|
||||
self.rollbacks = 0
|
||||
|
||||
def cursor(self):
|
||||
return FakeCursor(self)
|
||||
|
||||
def commit(self):
|
||||
self.commits += 1
|
||||
|
||||
def rollback(self):
|
||||
self.rollbacks += 1
|
||||
|
||||
def close(self):
|
||||
pass
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mirror(monkeypatch):
|
||||
"""One brand table, `brand_cadbury`, displayed as 'Cadbury'."""
|
||||
conn = FakeConn()
|
||||
monkeypatch.setattr(nutrition_score_sync, "_connect", lambda: conn)
|
||||
monkeypatch.setattr(nutrition_score_sync, "ensure_brand_schema", lambda b: "")
|
||||
monkeypatch.setattr(nutrition_score_sync, "_list_brand_table_suffixes",
|
||||
lambda cur, include_inactive=False: ["cadbury"])
|
||||
monkeypatch.setattr(nutrition_score_sync, "display_name_for_suffix",
|
||||
lambda s, brand_map=None: s.title())
|
||||
return conn
|
||||
|
||||
|
||||
def test_the_mirror_copies_scores_onto_the_brand_table(mirror):
|
||||
changed = nutrition_score_sync.sync_scores_to_brand_tables()
|
||||
|
||||
assert changed == {"Cadbury": 2} # one copied + one cleared
|
||||
copy_sql = mirror.calls[0][0]
|
||||
assert "UPDATE brand_cadbury b" in copy_sql
|
||||
assert "SET nutrition_score = i.nutrition_score" in copy_sql
|
||||
assert "health_score = i.health_score" in copy_sql
|
||||
assert mirror.commits == 1
|
||||
|
||||
|
||||
def test_the_brand_match_is_case_insensitive(mirror):
|
||||
"""Enrichment writes 'Cadbury', the Excel upload writes 'cadbury'. An `=`
|
||||
match would mirror the first and silently skip the second - and the table
|
||||
would still look populated, which is why this needs a test rather than a
|
||||
comment."""
|
||||
nutrition_score_sync.sync_scores_to_brand_tables()
|
||||
|
||||
for sql, params in mirror.calls:
|
||||
assert "lower(i.brand) = lower(%s)" in sql
|
||||
assert params == ("Cadbury",)
|
||||
assert "i.brand = %s" not in sql
|
||||
|
||||
|
||||
def test_the_mirror_uses_is_distinct_from_so_a_second_run_is_a_no_op(mirror):
|
||||
"""`!=` is never true against NULL, so an unscored row would be rewritten
|
||||
on every run and `rowcount` would stop meaning 'rows actually changed'."""
|
||||
nutrition_score_sync.sync_scores_to_brand_tables()
|
||||
|
||||
copy_sql = mirror.calls[0][0]
|
||||
assert "IS DISTINCT FROM" in copy_sql
|
||||
assert "!=" not in copy_sql
|
||||
|
||||
|
||||
def test_a_score_whose_insight_row_has_gone_is_cleared(mirror):
|
||||
"""Otherwise a product deleted by the non-consumable purge keeps a health
|
||||
score on its brand row forever - the mirror would be an accumulator."""
|
||||
nutrition_score_sync.sync_scores_to_brand_tables()
|
||||
|
||||
clear_sql = mirror.calls[1][0]
|
||||
assert "SET nutrition_score = NULL, health_score = NULL" in clear_sql
|
||||
assert "NOT EXISTS" in clear_sql
|
||||
|
||||
|
||||
def test_nothing_is_written_on_a_dry_run(mirror):
|
||||
nutrition_score_sync.sync_scores_to_brand_tables(dry_run=True)
|
||||
|
||||
assert mirror.commits == 0
|
||||
for sql, _ in mirror.calls:
|
||||
assert sql.startswith("SELECT count(*)")
|
||||
|
||||
|
||||
def test_a_brand_filter_narrows_the_run(mirror):
|
||||
assert nutrition_score_sync.sync_scores_to_brand_tables(["Amul"]) == {}
|
||||
assert mirror.calls == []
|
||||
|
||||
assert nutrition_score_sync.sync_scores_to_brand_tables(["Cadbury"])
|
||||
|
||||
|
||||
def test_the_table_suffix_is_also_accepted_as_a_brand(mirror):
|
||||
"""`sync_after_write` is called from the Excel path, which lowercases the
|
||||
brand, so 'cadbury' must reach brand_cadbury."""
|
||||
assert nutrition_score_sync.sync_scores_to_brand_tables(["cadbury"])
|
||||
|
||||
|
||||
def test_one_broken_table_does_not_abort_the_catalogue(monkeypatch):
|
||||
class Exploding(FakeConn):
|
||||
"""Hands out one working cursor (for the suffix listing), then fails on
|
||||
the first table and recovers for the second."""
|
||||
|
||||
def __init__(self):
|
||||
super().__init__()
|
||||
self.handed_out = 0
|
||||
|
||||
def cursor(self):
|
||||
self.handed_out += 1
|
||||
if self.handed_out == 2:
|
||||
raise RuntimeError("table is locked")
|
||||
return FakeCursor(self)
|
||||
|
||||
conn = Exploding()
|
||||
monkeypatch.setattr(nutrition_score_sync, "_connect", lambda: conn)
|
||||
monkeypatch.setattr(nutrition_score_sync, "ensure_brand_schema", lambda b: "")
|
||||
monkeypatch.setattr(nutrition_score_sync, "_list_brand_table_suffixes",
|
||||
lambda cur, include_inactive=False: ["a", "b"])
|
||||
monkeypatch.setattr(nutrition_score_sync, "display_name_for_suffix",
|
||||
lambda s, brand_map=None: s.title())
|
||||
|
||||
nutrition_score_sync.sync_scores_to_brand_tables() # must not raise
|
||||
|
||||
|
||||
def test_sync_after_write_never_raises(monkeypatch):
|
||||
"""The mirror is a convenience. The caller's write to nutrition_insights
|
||||
has already succeeded and must not be reported as failed."""
|
||||
def boom(*a, **k):
|
||||
raise RuntimeError("database on fire")
|
||||
|
||||
monkeypatch.setattr(nutrition_score_sync, "sync_scores_to_brand_tables", boom)
|
||||
|
||||
nutrition_score_sync.sync_after_write(["Cadbury"])
|
||||
|
||||
|
||||
def test_no_connection_is_not_an_error(monkeypatch):
|
||||
monkeypatch.setattr(nutrition_score_sync, "_connect", lambda: None)
|
||||
|
||||
assert nutrition_score_sync.sync_scores_to_brand_tables() == {}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The mirror: seed catalog JSON
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# A product carrying the keys the Cadbury catalog really has. The point of the
|
||||
# test below is that all of them survive.
|
||||
FULL_PRODUCT = {
|
||||
"product_name": "Cadbury Oreo",
|
||||
"title": "Cadbury Oreo 30g",
|
||||
"image_id": "cadbury_cadbury_oreo_30g",
|
||||
"brand_name": "Cadbury",
|
||||
"category": "Biscuits",
|
||||
"size": "30g",
|
||||
"variant_key": "oreo-30g",
|
||||
"validation_status": "passed",
|
||||
"confidence_score": 0.91,
|
||||
"validation_issues": [],
|
||||
"price_analysis": {"median": 10},
|
||||
"best_deals": ["x"],
|
||||
"primary_image": "http://example/i.jpg",
|
||||
"image_source": "wikimedia",
|
||||
"total_variants": 5,
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def seed(tmp_path, monkeypatch):
|
||||
"""One seed catalog on disk, with one fully-populated product."""
|
||||
path = tmp_path / "brand_catalog_cadbury.json"
|
||||
path.write_text(json.dumps({
|
||||
"brand": "Cadbury",
|
||||
"total_products": 1,
|
||||
"products": [dict(FULL_PRODUCT)],
|
||||
}, indent=2), encoding="utf-8")
|
||||
|
||||
monkeypatch.setattr(brand_sync, "seed_catalog_paths", lambda *a, **k: [path])
|
||||
monkeypatch.setattr(nutrition_score_sync, "_canonical_brand", lambda raw: "Cadbury")
|
||||
monkeypatch.setattr(nutrition_score_sync, "_score_index",
|
||||
lambda: {("cadbury", "cadbury_cadbury_oreo_30g"): (13.2, 11.2)})
|
||||
return path
|
||||
|
||||
|
||||
def _products(path):
|
||||
return json.loads(path.read_text(encoding="utf-8"))["products"]
|
||||
|
||||
|
||||
def test_the_scores_are_written_into_the_seed_file(seed):
|
||||
changed = nutrition_score_sync.sync_scores_to_seed_files()
|
||||
|
||||
assert changed == {"brand_catalog_cadbury.json": 1}
|
||||
product = _products(seed)[0]
|
||||
assert product["nutrition_score"] == 13.2
|
||||
assert product["health_score"] == 11.2
|
||||
|
||||
|
||||
def test_every_other_key_survives(seed):
|
||||
"""THE REGRESSION GUARD FOR THE OBVIOUS SHORTCUT.
|
||||
|
||||
`export_brand_to_seed_file` would have been the easy way to do this, but
|
||||
`upsert_products_into_catalog_file` does `products_list[idx] = clean` - a
|
||||
REPLACE - and rebuilds each product from EXPORT_COLUMNS. Running it over
|
||||
this file would drop validation_status, confidence_score, size,
|
||||
variant_key, price_analysis and the rest on the floor.
|
||||
"""
|
||||
nutrition_score_sync.sync_scores_to_seed_files()
|
||||
|
||||
product = _products(seed)[0]
|
||||
for key, value in FULL_PRODUCT.items():
|
||||
assert product[key] == value, f"{key} was lost or changed"
|
||||
|
||||
assert set(product) == set(FULL_PRODUCT) | {"nutrition_score", "health_score"}
|
||||
|
||||
|
||||
def test_an_unscored_product_gets_an_explicit_null(seed, monkeypatch):
|
||||
"""Present-and-null, not absent. A consumer must be able to tell 'we have
|
||||
not scored this' from 'this build of the file predates the field'."""
|
||||
monkeypatch.setattr(nutrition_score_sync, "_score_index",
|
||||
lambda: {("cadbury", "something_else"): (1.0, 2.0)})
|
||||
|
||||
nutrition_score_sync.sync_scores_to_seed_files()
|
||||
|
||||
product = _products(seed)[0]
|
||||
assert product["nutrition_score"] is None
|
||||
assert product["health_score"] is None
|
||||
|
||||
|
||||
def test_a_dry_run_leaves_the_file_untouched(seed):
|
||||
before = seed.read_text(encoding="utf-8")
|
||||
|
||||
changed = nutrition_score_sync.sync_scores_to_seed_files(dry_run=True)
|
||||
|
||||
assert changed == {"brand_catalog_cadbury.json": 1}
|
||||
assert seed.read_text(encoding="utf-8") == before
|
||||
|
||||
|
||||
def test_a_second_run_reports_nothing_to_do(seed):
|
||||
nutrition_score_sync.sync_scores_to_seed_files()
|
||||
|
||||
assert nutrition_score_sync.sync_scores_to_seed_files() == {}
|
||||
|
||||
|
||||
def test_no_temp_file_is_left_behind(seed):
|
||||
nutrition_score_sync.sync_scores_to_seed_files()
|
||||
|
||||
assert list(seed.parent.glob("*.tmp")) == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The export column list
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_the_export_list_carries_the_scores():
|
||||
assert "nutrition_score" in brand_sync.EXPORT_COLUMNS
|
||||
assert "health_score" in brand_sync.EXPORT_COLUMNS
|
||||
|
||||
|
||||
def test_the_export_list_did_not_lose_anything():
|
||||
"""It is the round-trip contract - a name dropped here silently strips that
|
||||
field out of every exported catalog."""
|
||||
for col in ("product_name", "image_id", "hsn_code", "final_selling_price",
|
||||
"selling_price", "barcode", "barcode_type", "fssai_license",
|
||||
"product_sku", "search_query"):
|
||||
assert col in brand_sync.EXPORT_COLUMNS
|
||||
@@ -168,6 +168,24 @@ def test_the_sheets_values_are_kept_and_nothing_else_is_invented(store):
|
||||
assert row[field] is None, f"{field} was invented: {row[field]!r}"
|
||||
|
||||
|
||||
def test_the_pipeline_never_sends_a_nutrition_score(store):
|
||||
"""`brand_*` carries nutrition_score/health_score, but the pipeline is not
|
||||
what fills them - `nutrition_score_sync` mirrors them out of
|
||||
nutrition_insights after enrichment has computed one.
|
||||
|
||||
The keys must be ABSENT from the row handed to upsert_brand_products, not
|
||||
present-and-None. `upsert_brand_products` deliberately omits both columns
|
||||
from its INSERT so a catalogue re-write cannot NULL a real score; a row
|
||||
that started carrying them would be the first step towards someone
|
||||
"fixing" that omission.
|
||||
"""
|
||||
_run(["Product Name", "Weight", "Selling Price"], [["Apple", "500g", 155]])
|
||||
|
||||
row = store[0][1]
|
||||
assert "nutrition_score" not in row
|
||||
assert "health_score" not in row
|
||||
|
||||
|
||||
def test_the_price_band_is_eight_percent_of_the_sheet_price(store):
|
||||
_run(["Product Name", "Weight", "Selling Price"], [["Tomato", "1kg", 100]])
|
||||
|
||||
|
||||
Reference in New Issue
Block a user