diff --git a/docs/tickets/mps-24.md b/docs/tickets/mps-24.md index 3044871..fa1907f 100644 --- a/docs/tickets/mps-24.md +++ b/docs/tickets/mps-24.md @@ -363,6 +363,24 @@ Tests (`test_functional.py::TestProductTagsSpa`): Deferred (occasional click, not the hot path): AJAX-ifying the "Suggest categories" link — still a full navigation by design. +**Phase 2.8b** (shipped 2026-05-16): the generic `data-tag-form` +**`submit`-event** interception proved unreliable in the field — +operator reported Add / Delete / reorder *all* still full-reloaded +while the explicit click handlers (focus/drag) worked. Root fix: +**one unified capture-phase `click` handler** (`onTagFormClick`) +on every submit control inside `form[data-tag-form]`. It +`preventDefault()`s (native submit never starts → no reload, no +double-handling), runs the delete confirm via `data-confirm` +(inline `onclick="return confirm()"` removed from `shop_tags.j2` +**and** the JS `appendTagRow` builder — it fought the interception), +routes `reorder` → `doReorder` (in-place swap) and everything else +(create/add, delete, attach/detach, apply/dismiss suggestion) → +`submitForm`. The `submit` listener is kept only as the Enter-key +fallback; `escapeJs` removed (dead after the onclick→data-confirm +switch). Tests: `test_ajax_reorder_arrow_returns_json_and_moves`, +`test_ajax_delete_tag_returns_json`, +`test_bulk_tagger_delete_uses_data_confirm_not_inline_onclick`. + ### Phase 2.7 — per-product SPA tag chips on product edit (shipped 2026-05-16) Operator report: adding/removing a tag on the product edit page diff --git a/make_post_sell/static/js/tag_bulk.js b/make_post_sell/static/js/tag_bulk.js index 4a338bc..e2e3aca 100644 --- a/make_post_sell/static/js/tag_bulk.js +++ b/make_post_sell/static/js/tag_bulk.js @@ -26,18 +26,56 @@ // self-gates inside maybeIntercept(). document.querySelectorAll("form[data-tag-form]").forEach(attach); - // Event delegation isn't enough because we re-render rows; we - // re-attach to any new forms after each mutation via attachAll(). + // PRIMARY interception path (MPS-24 Phase 2.8b): a capture-phase + // *click* listener on submit controls inside [data-tag-form] forms. + // The generic `submit`-event interception below proved unreliable + // in the field (every Add / Delete / reorder still full-reloaded + // for the operator). A click handler that preventDefault()s stops + // the native form submission BEFORE it starts, so the submit event + // never fires — no double-handling — and the page never reloads. + document.addEventListener("click", onTagFormClick, true); + + // SECONDARY fallback: keep the submit listener so keyboard submits + // (Enter in the "Add tag" field) are still AJAX. Mutually exclusive + // with the click path (a prevented click never emits `submit`). document.addEventListener("submit", maybeIntercept, true); - // MPS-24 Phase 2.8: the heavy full-page reload on this page was the - // tag-focus navigation (a plain ), not the forms. - // On a 481-product catalog every tag click reloaded the whole bulk - // tagger. Intercept it + wire real drag-to-reorder. + // MPS-24 Phase 2.8: the other heavy full-page reload was the + // tag-focus navigation (a plain ). Intercept it + + // wire real drag-to-reorder. wireFocusLinks(); wireDragAndDrop(); } + /* The one robust path every button-driven tag action flows through. + * Routes reorder to doReorder() (in-place row swap) and everything + * else to submitForm(); handles the delete confirm via data-confirm + * so there's no inline onclick fighting the interception. */ + function onTagFormClick(ev) { + if ( + ev.button !== 0 || + ev.metaKey || ev.ctrlKey || ev.shiftKey || ev.altKey + ) { + return; // let the browser do its thing for modified clicks + } + const btn = ev.target.closest( + 'form[data-tag-form] button[type="submit"],' + + 'form[data-tag-form] input[type="submit"]' + ); + if (!btn) return; + const form = btn.closest("form[data-tag-form]"); + if (!form) return; + ev.preventDefault(); // kills the native submit + full reload + if (btn.disabled) return; + const confirmMsg = btn.getAttribute("data-confirm"); + if (confirmMsg && !window.confirm(confirmMsg)) return; + if (form.getAttribute("data-tag-form") === "reorder") { + doReorder(form, btn); + return; + } + submitForm(form); + } + function attachAll(root) { (root || document).querySelectorAll("form[data-tag-form]").forEach(attach); } @@ -51,9 +89,12 @@ const form = ev.target; if (!(form instanceof HTMLFormElement)) return; if (!form.hasAttribute("data-tag-form")) return; - // Confirm dialog is on the delete button via inline onclick; if it - // returned false the submit never fires, so we don't need to repeat - // the check here. + // Keyboard-submit fallback (e.g. Enter in the "Add tag" field). + // Button clicks are handled earlier by onTagFormClick(), which + // preventDefault()s so this never fires for them. Reorder via + // keyboard is degenerate (arrows are buttons), so route the rest + // through submitForm(); the delete confirm only matters on the + // click path. ev.preventDefault(); submitForm(form); } @@ -130,6 +171,39 @@ }); } + /* MPS-24 Phase 2.8b: ↑/↓ reorder is AJAX. Routed here from the + * unified onTagFormClick() handler (action === "reorder"); reuses + * onReorder() for the in-place row swap. The view already answers + * action=reorder AJAX with {tag_slug, direction, moved}. */ + async function doReorder(form, btn) { + const fd = new FormData(form); + btn.disabled = true; // guard against double-click during the round-trip + try { + const res = await fetch(form.action, { + method: "POST", + body: fd, + headers: { "X-Requested-With": "XMLHttpRequest" }, + credentials: "same-origin", + }); + if (!res.ok) throw new Error("server " + res.status); + const ct = res.headers.get("content-type") || ""; + if (ct.indexOf("application/json") === -1) { + throw new Error("non-JSON response"); + } + const data = await res.json(); + flashAll(data.messages); + // onReorder() swaps the rows AND calls syncReorderButtonStates(), + // which re-derives the correct disabled state for every arrow — + // so we deliberately don't re-enable btn ourselves on success. + onReorder(form, data); + } catch (err) { + // Graceful fallback: a real submit still reorders server-side + // (the no-JS path) — page reloads, disabled state irrelevant. + console.warn("tag_bulk: reorder AJAX failed, submitting:", err); + form.submit(); + } + } + /* ----- handlers --------------------------------------------------- */ function onCreate(form, data) { @@ -211,7 +285,7 @@ '' + '' + '' + + 'data-confirm="Delete tag ' + escapeHtml(tag.name) + '?">Delete' + ''; list.appendChild(li); attachAll(li); @@ -515,10 +589,6 @@ .replace(/'/g, "'"); } - function escapeJs(s) { - return String(s).replace(/\\/g, "\\\\").replace(/'/g, "\\'"); - } - function cssEscape(s) { if (window.CSS && window.CSS.escape) return window.CSS.escape(s); return String(s).replace(/(["\\])/g, "\\$1"); diff --git a/make_post_sell/templates/shop_tags.j2 b/make_post_sell/templates/shop_tags.j2 index 5412545..ffb802a 100644 --- a/make_post_sell/templates/shop_tags.j2 +++ b/make_post_sell/templates/shop_tags.j2 @@ -131,7 +131,7 @@ + data-confirm="Delete tag {{ tag.name }}?">Delete {% endfor %} diff --git a/make_post_sell/tests/test_functional.py b/make_post_sell/tests/test_functional.py index 47ca602..8fa2d24 100644 --- a/make_post_sell/tests/test_functional.py +++ b/make_post_sell/tests/test_functional.py @@ -6971,6 +6971,80 @@ class TestProductTagsSpa(_AuthenticatedBase): self.assertEqual(pos["alpha"], 1) self.assertEqual(pos["bravo"], 2) + def test_ajax_delete_tag_returns_json(self): + """Delete is now AJAX via the unified click handler (was inline + onclick=confirm). Server must answer AJAX with JSON so the row + is removed in place — no full page reload.""" + shop_id, _ = self._make_product() + self.testapp.post( + f"/s/{shop_id}/tags", + {"action": "create", "name": "Doomed"}, + headers={"X-Requested-With": "XMLHttpRequest"}, status=200, + ) + res = self.testapp.post( + f"/s/{shop_id}/tags", + {"action": "delete", "tag_slug": "doomed"}, + headers={"X-Requested-With": "XMLHttpRequest"}, + status=200, + ) + self.assertIn("application/json", res.content_type) + self.assertEqual(res.json["status"], "ok") + self.assertEqual(res.json["deleted_slug"], "doomed") + from ..models.tag import get_tag_by_shop_and_slug + from ..models.shop import get_shop_by_id + self.dbsession.expire_all() + shop = get_shop_by_id(self.dbsession, shop_id) + self.assertIsNone( + get_tag_by_shop_and_slug(self.dbsession, shop, "doomed") + ) + + def test_bulk_tagger_delete_uses_data_confirm_not_inline_onclick(self): + """Regression: the delete confirm must be data-confirm (owned by + the unified click handler), not an inline onclick that fights + the interception and lets the page reload.""" + shop_id, _ = self._make_product() + self.testapp.post( + f"/s/{shop_id}/tags", + {"action": "create", "name": "Checkme"}, + headers={"X-Requested-With": "XMLHttpRequest"}, status=200, + ) + res = self.testapp.get(f"/s/{shop_id}/tags", status=200) + body = res.body.decode() + self.assertIn('data-confirm="Delete tag Checkme?"', body) + self.assertNotIn("onclick=\"return confirm", body) + + def test_ajax_reorder_arrow_returns_json_and_moves(self): + """The ↑/↓ arrows POST action=reorder&direction=up|down and must + answer AJAX with JSON {tag_slug, direction, moved} so tag_bulk.js + swaps the row in place — no full page reload.""" + from ..models.tag import get_tag_by_shop_and_slug + from ..models.shop import get_shop_by_id + shop_id, _ = self._make_product() + for name in ("First", "Second"): + self.testapp.post( + f"/s/{shop_id}/tags", + {"action": "create", "name": name}, + headers={"X-Requested-With": "XMLHttpRequest"}, status=200, + ) + # Move "Second" up — it should swap ahead of "First". + res = self.testapp.post( + f"/s/{shop_id}/tags", + {"action": "reorder", "tag_slug": "second", "direction": "up"}, + headers={"X-Requested-With": "XMLHttpRequest"}, + status=200, + ) + self.assertIn("application/json", res.content_type) + data = res.json + self.assertEqual(data["status"], "ok") + self.assertEqual(data["tag_slug"], "second") + self.assertEqual(data["direction"], "up") + self.assertTrue(data["moved"]) + self.dbsession.expire_all() + shop = get_shop_by_id(self.dbsession, shop_id) + second = get_tag_by_shop_and_slug(self.dbsession, shop, "second") + first = get_tag_by_shop_and_slug(self.dbsession, shop, "first") + self.assertLess(second.position, first.position) + def test_bulk_tagger_bare_get_renders_without_products(self): """Phase 2.8 perf: a bare GET (no focus / no suggestions) must not load+render the whole catalog. Page renders, focus section