fix: MPS-24 2.6c — sidebar category links hit tag route, not home catch-all
Operator review of 2.6b: clicking any sidebar category landed on a
page that looked exactly like the shop home (lanes), ignoring the
tag filter.
Root cause: _facet_nav.j2 built category links as
{absolute_url}/tag/{slug}. absolute_url() includes the shop slug
(/s/{id}/{shop_slug}), so the link became
/s/{id}/{shop_slug}/tag/{slug}. The tag detail route is
/s/{shop_id}/tag/{slug} — no shop-slug segment — so that path missed
shop_tag_detail and fell through to the shop_slug catch-all
(/s/{shop_id}/{slug:.*}), rendering the shop home.
Fix: macros now take a tag_base arg =
request.shop.absolute_url(request, slug=False) (= /s/{id}).
Category links build {tag_base}/tag/{slug} — matches
shop_tag_detail exactly. The All link keeps the slugged base_url
(shop home). All three callers (shop_tag.j2, home.j2, shop.j2)
pass both.
Regression coverage:
- test_facet_category_link_renders_tag_detail_not_home (new)
- test_tag_detail_renders_facet_sidebar (asserts slug-less link,
asserts NOT slugged link)
Docs: CLAUDE.md facet-nav note, ticket Phase 2.6c.
This commit is contained in:
parent
8970e960fb
commit
2640b035bc
7 changed files with 92 additions and 13 deletions
|
|
@ -498,6 +498,15 @@ and `details()` (mobile/tablet `<details>` accordion). Both call the
|
|||
same internal `facet_form()` so the controls stay identical across
|
||||
viewports — only the wrapper differs.
|
||||
|
||||
Macros take TWO base URLs: `base_url` (slugged shop home, for the
|
||||
"All" link) and `tag_base` (`request.shop.absolute_url(request,
|
||||
slug=False)` = `/s/{id}`, no shop slug). Category links MUST use
|
||||
`{tag_base}/tag/{slug}` so they match the `shop_tag_detail` route
|
||||
(`/s/{shop_id}/tag/{slug}`). Using the slugged URL produces
|
||||
`/s/{id}/{shop_slug}/tag/{slug}` which falls through to the
|
||||
`shop_slug` catch-all and silently renders the shop home instead of
|
||||
the filtered tag page (the 2.6b→2.6c defect).
|
||||
|
||||
Every page that opts into categorization (operator picks `home_layout
|
||||
>= 1`, or tag detail page) includes BOTH variants in the markup. CSS
|
||||
hides one per viewport: `details.facet-details` hidden ≥800px, `aside
|
||||
|
|
|
|||
|
|
@ -319,6 +319,26 @@ picker is Phase 2).
|
|||
| `tests/test_models.py` | `TestTagSuggestPureFunctions` — 11 unit tests over tokenize / stem / cluster |
|
||||
| `tests/test_functional.py` | `test_suggest_clusters_renders_candidates`, `test_apply_suggestion_creates_tag_and_attaches_products`, `test_dismiss_suggestion_adds_to_stopwords`, `test_apply_suggestion_rejects_empty_input` |
|
||||
|
||||
### Phase 2.6c — fix sidebar category links falling through to shop-home catch-all (shipped 2026-05-15)
|
||||
|
||||
Defect from 2.6b operator review: clicking any category in the desktop
|
||||
sidebar navigated to a page that "looked exactly like home" (screenshots
|
||||
in chat). Root cause: the facet macro built category links as
|
||||
`{absolute_url}/tag/{slug}` where `absolute_url` includes the shop slug
|
||||
(`/s/{id}/{shop_slug}`). The resulting path `/s/{id}/{shop_slug}/tag/{slug}`
|
||||
does **not** match the tag detail route `/s/{shop_id}/tag/{slug}` — it
|
||||
falls through to the `shop_slug` catch-all (`/s/{shop_id}/{slug:.*}`)
|
||||
and renders the shop home / lanes, ignoring the tag entirely.
|
||||
|
||||
Fix: `_facet_nav.j2` macros take a new `tag_base` arg =
|
||||
`request.shop.absolute_url(request, slug=False)` (= `/s/{id}`, no shop
|
||||
slug). Category links now build `{tag_base}/tag/{slug}` which matches
|
||||
`shop_tag_detail` exactly. The "All" link still uses the slugged
|
||||
`base_url` (shop home). All three call sites (shop_tag.j2, home.j2,
|
||||
shop.j2) updated. Regression test:
|
||||
`test_facet_category_link_renders_tag_detail_not_home` +
|
||||
hardened assertions in `test_tag_detail_renders_facet_sidebar`.
|
||||
|
||||
### Phase 2.6b — facet nav on shop home (layout 2) + mobile SERP rows under each lane (shipped 2026-05-15)
|
||||
|
||||
Follow-up to 2.6 after operator review on tablet: layout 2 (sectioned
|
||||
|
|
|
|||
|
|
@ -11,7 +11,12 @@
|
|||
|
||||
Args:
|
||||
variant — "sidebar" (desktop <aside>) or "details" (mobile <details>)
|
||||
base_url — shop absolute URL for "All" + tag links (no trailing slash)
|
||||
base_url — shop home URL for the "All" link (slug ok)
|
||||
tag_base — slug-LESS shop base (/s/{id}) so {tag_base}/tag/{slug}
|
||||
matches the shop_tag_detail route exactly. The route
|
||||
is /s/{shop_id}/tag/{slug} — a shop-slug segment in
|
||||
the path would fall through to the shop_slug
|
||||
catch-all and render the shop home instead.
|
||||
sort_options — list of (key, label) tuples for the sort <select>
|
||||
sort_key — currently selected sort key
|
||||
price_min — current min in cents (int or None)
|
||||
|
|
@ -24,7 +29,7 @@
|
|||
{%- if cents is not none -%}{{ (cents / 100)|round(2) }}{%- endif -%}
|
||||
{% endmacro %}
|
||||
|
||||
{% macro facet_form(base_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
{% macro facet_form(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
<form method="get" action="" class="facet-form">
|
||||
|
||||
<section class="facet-section">
|
||||
|
|
@ -69,7 +74,7 @@
|
|||
</li>
|
||||
{% for t in facet_tags %}
|
||||
<li>
|
||||
<a href="{{ base_url }}/tag/{{ t.slug }}"
|
||||
<a href="{{ tag_base }}/tag/{{ t.slug }}"
|
||||
class="facet-tag{% if active_tag and active_tag.id == t.id %} facet-tag-active{% endif %}"
|
||||
rel="nofollow">{{ t.name }}</a>
|
||||
</li>
|
||||
|
|
@ -81,18 +86,18 @@
|
|||
</form>
|
||||
{% endmacro %}
|
||||
|
||||
{% macro sidebar(base_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
{% macro sidebar(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
<aside class="facet-nav" aria-label="Filter and sort">
|
||||
{{ facet_form(base_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet_form(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
</aside>
|
||||
{% endmacro %}
|
||||
|
||||
{% macro details(base_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
{% macro details(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) %}
|
||||
<details class="facet-details">
|
||||
<summary class="facet-details-summary">
|
||||
🔍 Filter & sort
|
||||
{%- if price_min is not none or price_max is not none %} <span class="facet-details-active-mark">•</span>{% endif -%}
|
||||
</summary>
|
||||
{{ facet_form(base_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet_form(base_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
</details>
|
||||
{% endmacro %}
|
||||
|
|
|
|||
|
|
@ -81,6 +81,7 @@
|
|||
{% else %}
|
||||
|
||||
{% set shop_url = request.shop.absolute_url(request) %}
|
||||
{% set tag_base = request.shop.absolute_url(request, slug=False) %}
|
||||
{# MPS-24 Phase 2.6: when the operator opted into categorization
|
||||
(home_layout 1 chips or 2 lanes), give shoppers the full facet
|
||||
nav — sidebar on desktop, <details> accordion on mobile/tablet.
|
||||
|
|
@ -105,11 +106,11 @@
|
|||
|
||||
{% if show_facet_nav %}
|
||||
{# Mobile/tablet: collapsed <details> facets above the grid. #}
|
||||
{{ facet.details(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.details(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
<div class="tag-detail-layout">
|
||||
|
||||
{# Desktop sidebar. #}
|
||||
{{ facet.sidebar(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.sidebar(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
|
||||
<div class="tag-detail-content">
|
||||
{% endif %}
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@
|
|||
{% endif %}
|
||||
|
||||
{% set shop_url = request.shop.absolute_url(request) %}
|
||||
{% set tag_base = request.shop.absolute_url(request, slug=False) %}
|
||||
{% set show_facet_nav = request.shop.home_layout and request.shop.home_layout >= 1 %}
|
||||
|
||||
{# MPS-24: tag chip strip — shown on layout 1 (chips) and 2 (lanes). #}
|
||||
|
|
@ -32,11 +33,11 @@
|
|||
|
||||
{% if show_facet_nav %}
|
||||
{# Mobile/tablet: collapsed <details> facets above the grid. #}
|
||||
{{ facet.details(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.details(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
<div class="tag-detail-layout">
|
||||
|
||||
{# Desktop sidebar. #}
|
||||
{{ facet.sidebar(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.sidebar(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
|
||||
<div class="tag-detail-content">
|
||||
{% endif %}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
{% block content -%}
|
||||
|
||||
{% set shop_url = request.shop.absolute_url(request) %}
|
||||
{% set tag_base = request.shop.absolute_url(request, slug=False) %}
|
||||
|
||||
<section class="one-column tag-detail-header">
|
||||
<h1 class="type-headline-3">{{ active_tag.name }}</h1>
|
||||
|
|
@ -12,12 +13,12 @@
|
|||
|
||||
{# Mobile/tablet: collapsed <details> facets above the grid (CSS hides
|
||||
it ≥800px where the sidebar takes over). #}
|
||||
{{ facet.details(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.details(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
|
||||
<div class="tag-detail-layout">
|
||||
|
||||
{# Desktop sidebar — hidden <800px via CSS. #}
|
||||
{{ facet.sidebar(shop_url, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
{{ facet.sidebar(shop_url, tag_base, sort_options, sort_key, price_min, price_max, facet_tags, active_tag) }}
|
||||
|
||||
<div class="tag-detail-content">
|
||||
|
||||
|
|
|
|||
|
|
@ -8534,6 +8534,48 @@ class TestHomeLayoutAndTags(_AuthenticatedBase):
|
|||
# Both sidebar (desktop) and details accordion (mobile) render.
|
||||
self.assertIn('facet-nav', body)
|
||||
self.assertIn('facet-details', body)
|
||||
# Regression: category links must hit the slug-LESS tag route
|
||||
# (/s/{id}/tag/{slug}), NOT /s/{id}/{shop_slug}/tag/{slug}.
|
||||
# The latter falls through to the shop_slug catch-all and
|
||||
# renders the shop home instead of the filtered tag page.
|
||||
self.assertIn(f'/s/{shop.id}/tag/math"', body)
|
||||
self.assertNotIn(f'/s/{shop.id}/{shop.slug}/tag/', body)
|
||||
|
||||
def test_facet_category_link_renders_tag_detail_not_home(self):
|
||||
"""Clicking a sidebar category must land on the tag detail page
|
||||
(filtered SERP), not fall through to the shop home catch-all."""
|
||||
shop, product = self._make_shop_with_product("facet-route-shop")
|
||||
# Layout 2 so the home renders the facet sidebar.
|
||||
self.testapp.post(
|
||||
f"/s/{shop.id}/settings",
|
||||
{
|
||||
"form_section": "home-layout-settings",
|
||||
"home_layout": "2",
|
||||
"home_layout_tag_limit": "5",
|
||||
"home_layout_per_lane_limit": "10",
|
||||
"submit": "Save Home Layout",
|
||||
},
|
||||
)
|
||||
self.testapp.post(
|
||||
f"/p/{product.id}/edit",
|
||||
{
|
||||
"title": product.title,
|
||||
"description": "One. Two. Three. Four. Five. Six.",
|
||||
"price": str(product.price),
|
||||
"visibility": "1",
|
||||
"tags": "Math",
|
||||
},
|
||||
)
|
||||
# The shop home sidebar link must be the slug-less tag route.
|
||||
res = self.testapp.get(f"/s/{shop.id}/{shop.slug}")
|
||||
self.assertIn(f'/s/{shop.id}/tag/math"', res.body.decode())
|
||||
# Following that exact route renders the tag detail SERP page,
|
||||
# which carries the unique .tag-detail-header h1 (the shop home
|
||||
# lanes view never emits that element).
|
||||
res = self.testapp.get(f"/s/{shop.id}/tag/math")
|
||||
body = res.body.decode()
|
||||
self.assertIn('tag-detail-header', body)
|
||||
self.assertIn('serp-list', body)
|
||||
|
||||
def test_tag_detail_price_filter_narrows_grid(self):
|
||||
"""?price_min and ?price_max remove products outside the range."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue