feat(navbar): seed the menu from item groups and link one group per category - #25
Open
Rl0007 wants to merge 3 commits into
Open
feat(navbar): seed the menu from item groups and link one group per category#25Rl0007 wants to merge 3 commits into
Rl0007 wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Ecommerce Categorylinked item groups through a child table (Ecommerce Category Item Group, fieldlink_item_groups), and a fresh store started with an empty menu — the storefront had no navigation until someone found the "Import from Item Group" button in the menu editor.Two things fall out of that:
import_from_item_group— the only programmatic writer — always writes exactly one row per entry. Multiple rows only ever came from hand-editing the MultiSelectPills control in the Desk inspector. Every consumer then flattens the list straight back out again (",".join(...)for the?subcategory=filter,dict.fromkeysfor the publish cascade). The table was a one-row table with a join to pay for.collectionssitemap segment.Solution
Seed the menu from the Item Group tree on install, and replace the child table with a single
item_groupLink.The seeding is the existing importer, not a new mechanism:
import_from_item_group's body is extracted intoseed_categories_from_item_groups(), andafter_installcalls it throughseed_menu_when_empty(). So install, the upgrade patch, and the editor button all produce the same tree by the same code, and they inherit its idempotency — an entry already linked to the same group under the same parent is reused, never duplicated.It stays a one-time copy, not a live mirror. There is no
doc_eventson Item Group and no scheduled sync: once seeded, the shop owner reorders, renames and prunes the menu without any of it reaching the catalogue. A new Item Group does not appear on its own; re-running the import adds it.get_menu_treedrops from two queries to one — it is read on every storefront render.Migration (
ls_shop/patches/move_ecommerce_category_to_item_group_link.py)The highest-risk part, so it is three ordered, individually-guarded steps:
item_groupin one chunked query-builderUPDATE. Rows left behind by a link-type switch (the oldset_linknever cleared the table) are skipped — carrying those over would resurrect a link the owner had already replaced.MAX_MENU_DEPTH.delete_docdoes not take the table with it. The drop goes throughfrappe.qb.drop_table(...)rather than a literal statement because MariaDB and Postgres quote the identifier differently.Re-running is a no-op at every step: the DocType is gone, so the backfill returns immediately; the tree is non-empty, so the seed skips; the table is gone, so the drop skips. Fresh installs never run it at all — Frappe marks patches executed on install, and
after_installdoes the seeding.Consumers updated
navbar_manager(set_link,get_linked_nodes,get_subtree_item_groups) ·shop_datafacets and the legacy nav shape ·build_listing_href/get_node_href· the Desk inspector JS (MultiSelectPills → Link) ·install_demo_data(which was already writing anitem_groupfield that no longer existed, so its demo categories linked nothing) ·product_filter.htmlcomment.www/sitemap.py/sitemap_segment.pyneed no change — they filter onroute_slug is set, and seeding only adds nested entries, which carry no slug. There is a test pinning that.Evidence
Real upgrade on the devbox: seeded a v1.0.0-shaped site with a 3-row
link_item_groupsfixture (one entry linking two groups), thenbench migrateon this branch.Belts— the second group onMigDemo Tops— survived as a sibling, bounds are contiguous, hrefs render.Tests
New
ls_shop/tests/test_menu_seeding.py(13 tests) covers fresh-install seeding, tree shape, nested-set bounds, idempotency, menu/catalogue independence, the sitemap, and the migration path — the last against a real rebuilt child table, not a stand-in.Existing suites, updated for the new shape:
ruff checkandruff format --checkpass across the app.Known devbox gaps (pre-existing, not from this change)
Flagging so the CI baseline is not read as a regression:
TestNavbarPublishCascade.setUpClasserrors onLinkValidationError: Could not find Default Unit of Measure: Nos. The devbox site has zero UOM records (frappe.db.count("UOM") == 0) — ERPNext's masters were never installed. Nothing in the fixture touches categories.test_seo.py— 3 of 34 error insetUpClassonfrappe.get_all("Style Attribute Configurator", limit=1)[0]→IndexError; same missing-catalogue gap. The other 31 pass.after_migrateon that box raisesModuleNotFoundError: No module named 'erpnext.selling.doctype.quotation.mapper'fromls_shop/api/payments.py:8. Reproduced identically at4afe430, so it predates this branch — the box's erpnext is older than that import expects. It fires after patches, so the migration above still completed.Deliberate trade-offs
ivendnext_ecommerceprior art, which keeps the child table — reading its history, the child table is the newer model there and the single link is the older one, so it is not ahead of us on this. Itsimport_from_item_groupand NSM-suppression approach are reused verbatim; only the storage shape differs. Itsbase_setup.pyalso leaves root tabs unlinked deliberately, which is the next point.Follow-up, deliberately not in this PR
A root tab seeded from a group Item Group links to a group that holds no products directly, and
?subcategory=is an exactitem_group IN (...)with no descendant expansion — so the tab opens an empty grid. This is pre-existing behaviour ofimport_from_item_group(anyone who pressed the button already has it) and seeding on install now makes it visible on more stores. The prior art dodges it by leaving root tabs unlinked; the real fix is expanding a group link to its leaf descendants inget_node_href. That is a listing-semantics change on a hot path and belongs in its own PR — changing it here would also have silently rewritten the import tests.