Brand image display cards

tests/test_brand_overview_sample_image.py

@@ -0,0 +1,133 @@
+"""Which image a brand card ends up showing.
+
+The brand grid renders one photo per brand, sampled from that brand's products.
+Ten of the fifty-five cards were rendering the initials monogram instead, and
+none of them were missing data - every one had a non-empty `image_url` that
+simply could not be fetched:
+
+*   five (Balaji, Colin, Hindustan Unilever, Own Products, PepsiCo) were handed
+    an `http://` URL. The site is https, so the browser blocks it as mixed
+    content. Each of those brands already held working https URLs - Colin nine
+    of them, Hindustan Unilever seventy-four - on the very same rows.
+*   four (Everest, Haldirams, MDH, Naga) were handed a URL into an S3 bucket
+    that 404s on every object, minted by a constructor that never checked.
+
+Both classes look identical to the frontend: `Boolean(image_url)` is true, so
+the card renders an <img> and only finds out at fetch time. That is why the
+choice has to be made here, where the alternatives are still visible.
+
+These are pure-function tests on dicts. No database, no network - the module
+is imported for two helpers and nothing else, which also keeps them honest
+under `filterwarnings = error`.
+"""
+from __future__ import annotations
+
+from app.services.vector_store import _pick_sample_image, _usable_image_url
+
+DEAD_S3 = "https://nearledaily.s3.ap-south-1.amazonaws.com/daily/brands/mdh/x/image_000.jpg"
+
+
+def _row(image_id, image_url=None, image_urls=None):
+    return {"image_id": image_id, "image_url": image_url, "image_urls": image_urls or []}
+
+
+# ---------------------------------------------------------------------------
+# The bug that started this
+# ---------------------------------------------------------------------------
+def test_https_beats_http_even_on_an_older_row():
+    """The Colin case, which is the whole reason the selection changed.
+
+    Colin's newest row leads with http://officio.in/... and carries working
+    Amazon https URLs further down. Picking positionally gave the blocked one.
+    """
+    rows = [
+        _row("newest", "http://officio.in/Colin-Glass-Cleaner.jpg"),
+        _row("older", "https://m.media-amazon.com/images/I/61PAkjiijnL.jpg"),
+    ]
+    image_id, url = _pick_sample_image(rows)
+    assert url == "https://m.media-amazon.com/images/I/61PAkjiijnL.jpg"
+    assert image_id == "older", "the id must follow the URL that won, not row 0"
+
+
+def test_https_inside_the_array_beats_the_http_scalar_on_the_same_row():
+    rows = [_row(
+        "only",
+        "http://officio.in/x.jpg",
+        ["http://officio.in/x.jpg", "https://m.media-amazon.com/i/61.jpg"],
+    )]
+    assert _pick_sample_image(rows)[1] == "https://m.media-amazon.com/i/61.jpg"
+
+
+# ---------------------------------------------------------------------------
+# The non-regression that matters most
+# ---------------------------------------------------------------------------
+def test_an_http_only_brand_keeps_its_image():
+    """Preferring https must never mean discarding the only image there is.
+
+    A card that might render beats one that certainly will not, and plenty of
+    brands legitimately hold a single http URL.
+    """
+    rows = [_row("b", "http://only-image-in-the-world/x.jpg")]
+    image_id, url = _pick_sample_image(rows)
+    assert url == "http://only-image-in-the-world/x.jpg"
+    assert image_id == "b"
+
+
+# ---------------------------------------------------------------------------
+# Values that pass IS NOT NULL but are not images
+# ---------------------------------------------------------------------------
+def test_empty_and_blank_strings_are_skipped():
+    rows = [_row("c", "", ["", "   ", "https://good/y.jpg"])]
+    assert _pick_sample_image(rows)[1] == "https://good/y.jpg"
+
+
+def test_non_http_values_are_rejected():
+    for junk in ("", "   ", None, 42, "data:image/png;base64,AAAA", "daily/brands/x/i.jpg"):
+        assert _usable_image_url(junk) is None, junk
+
+
+def test_protocol_relative_urls_are_promoted_to_https():
+    assert _usable_image_url("//cdn.example/x.jpg") == "https://cdn.example/x.jpg"
+
+
+# ---------------------------------------------------------------------------
+# The dead bucket
+# ---------------------------------------------------------------------------
+def test_a_known_dead_host_loses_to_a_live_one():
+    rows = [_row("newest", DEAD_S3), _row("older", "https://live.example/z.jpg")]
+    image_id, url = _pick_sample_image(rows)
+    assert url == "https://live.example/z.jpg"
+    assert image_id == "older"
+
+
+def test_a_known_dead_host_is_never_returned_even_alone():
+    """Everest, Haldirams, MDH and Naga hold nothing else.
+
+    Returning None here is correct: it lets the router try S3 and then the card
+    fall back to its monogram, rather than rendering a broken image.
+    """
+    image_id, url = _pick_sample_image([_row("mdh_row", DEAD_S3)])
+    assert url is None
+    assert image_id == "mdh_row", "the id still has to reach the S3 lookup"
+
+
+# ---------------------------------------------------------------------------
+# Shape guarantees the router depends on
+# ---------------------------------------------------------------------------
+def test_no_usable_url_still_returns_the_newest_image_id():
+    """brands.py resolves sample_image_id against S3 when there is no URL.
+
+    Some brand tables predate the URL columns entirely and have only image_id,
+    so dropping the id would regress them to a monogram.
+    """
+    assert _pick_sample_image([_row("keep-me", None, [])]) == ("keep-me", None)
+
+
+def test_no_rows_at_all():
+    assert _pick_sample_image([]) == (None, None)
+
+
+def test_first_usable_row_wins_when_schemes_tie():
+    """With nothing to separate them, newest-first ordering still decides."""
+    rows = [_row("new", "https://a/1.jpg"), _row("old", "https://b/2.jpg")]
+    assert _pick_sample_image(rows) == ("new", "https://a/1.jpg")

tests/test_brand_registry.py

@@ -18,6 +18,8 @@ import pytest
 
 from app.services.brand_registry import (
     BRAND_ALIASES,
+    BRAND_LOGOS,
+    get_brand_logo,
     get_fssai_license,
     resolve_parent_brand,
 )
@@ -155,6 +157,46 @@ def test_the_haldiram_licence_survives_the_alias() -> None:
     assert get_fssai_license("Haldirams") != get_fssai_license("lion dates")
 
 
+def test_every_logo_key_is_its_own_canonical_parent() -> None:
+    """A key the alias map rewrites is dead on arrival, and silently so.
+
+    get_brand_logo resolves to the parent before looking up, so a key like
+    "haldiram's" would never be reached - and a missing logo is a legitimate
+    outcome for 52 of 55 brands, so nothing else would ever flag it.
+    """
+    for key in BRAND_LOGOS:
+        assert resolve_parent_brand(key).lower().strip() == key, (
+            f"{key!r} resolves to {resolve_parent_brand(key)!r} and can never be looked up"
+        )
+
+
+def test_every_logo_is_https() -> None:
+    """http:// is blocked as mixed content on the https site.
+
+    Serving a blocked image is half the bug this map was added to fix, so it
+    must not be reintroducible through the fix itself.
+    """
+    for key, url in BRAND_LOGOS.items():
+        assert url.startswith("https://"), f"{key} logo is not https: {url}"
+
+
+def test_the_haldiram_logo_survives_the_alias() -> None:
+    """Same trap as the licence above: both spellings must resolve."""
+    assert get_brand_logo("Haldiram") == get_brand_logo("Haldirams")
+    assert get_brand_logo("HALDIRAMS") == get_brand_logo("haldiram's")
+    assert get_brand_logo("Haldiram") is not None
+
+
+def test_a_brand_without_a_curated_logo_returns_none() -> None:
+    """None is the normal answer and means "use a product image instead".
+
+    The card falls through to the sampled photo and then to its monogram, so
+    this path is the one almost every brand takes.
+    """
+    assert get_brand_logo("Britannia") is None
+    assert get_brand_logo("no such brand at all") is None
+
+
 def test_known_sub_brands_still_route_to_their_family() -> None:
     """Word-boundary matching must not break legitimate sub-brand routing."""
     assert resolve_parent_brand("Dove") == "hindustan unilever"