Skip to content

feat(navbar): seed the menu from item groups and link one group per category - #25

Open
Rl0007 wants to merge 3 commits into
mainfrom
feat/ecommerce-category-from-item-group
Open

feat(navbar): seed the menu from item groups and link one group per category#25
Rl0007 wants to merge 3 commits into
mainfrom
feat/ecommerce-category-from-item-group

Conversation

@Rl0007

@Rl0007 Rl0007 commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

Ecommerce Category linked item groups through a child table (Ecommerce Category Item Group, field link_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:

  • Nobody ever links more than one group. 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.fromkeys for the publish cascade). The table was a one-row table with a join to pay for.
  • A fresh store looks broken. No categories means no navbar, no sidebar facets, and nothing in the collections sitemap segment.

Solution

Seed the menu from the Item Group tree on install, and replace the child table with a single item_group Link.

The seeding is the existing importer, not a new mechanism: import_from_item_group's body is extracted into seed_categories_from_item_groups(), and after_install calls it through seed_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_events on 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_tree drops 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:

  1. Backfill. Each entry's first linked group moves onto item_group in one chunked query-builder UPDATE. Rows left behind by a link-type switch (the old set_link never cleared the table) are skipped — carrying those over would resurrect a link the owner had already replaced.
  2. Rehome the extras. An entry that linked several groups keeps the first and gets one sibling entry per remaining group. Dropping them would take those products out of the navigation. A sibling sits at the entry's own depth, so it can never breach MAX_MENU_DEPTH.
  3. Seed if empty, then drop the child table — both the DocType doc (migrate only syncs the JSON files it finds) and the table itself, since delete_doc does not take the table with it. The drop goes through frappe.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_install does the seeding.

Consumers updated

navbar_manager (set_link, get_linked_nodes, get_subtree_item_groups) · shop_data facets and the legacy nav shape · build_listing_href / get_node_href · the Desk inspector JS (MultiSelectPills → Link) · install_demo_data (which was already writing an item_group field that no longer existed, so its demo categories linked nothing) · product_filter.html comment.

www/sitemap.py / sitemap_segment.py need no change — they filter on route_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_groups fixture (one entry linking two groups), then bench migrate on this branch.

patch logged: True
child doctype exists: False
child table exists: False

{'name': 'MigDemo Menswear',                  'link_type': '',           'item_group': None,             'lft': 1, 'rgt': 8}
{'name': 'MigDemo Tops',                      'link_type': 'Item Group', 'item_group': 'MigDemo Shirts', 'lft': 2, 'rgt': 3}
{'name': 'MigDemo Bags',                      'link_type': 'Item Group', 'item_group': 'MigDemo Bags',   'lft': 4, 'rgt': 5}
{'name': 'MigDemo Menswear - MigDemo Belts',  'link_type': 'Item Group', 'item_group': 'MigDemo Belts',  'lft': 6, 'rgt': 7}

ROOT MigDemo Menswear /en/products?category=migdemo_menswear
    MigDemo Bags                     | MigDemo Bags   | ...&subcategory=MigDemo%20Bags
    MigDemo Tops                     | MigDemo Shirts | ...&subcategory=MigDemo%20Shirts
    MigDemo Menswear - MigDemo Belts | MigDemo Belts  | ...&subcategory=MigDemo%20Belts

Belts — the second group on MigDemo 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.

Running 13 integration tests for ls_shop
TestLegacyItemGroupLinkMigration
   ✔ test_extra_linked_groups_are_rehomed_as_siblings
   ✔ test_rows_left_behind_by_a_link_type_switch_are_ignored
   ✔ test_running_the_migration_twice_changes_nothing
   ✔ test_the_legacy_table_is_dropped
   ✔ test_the_linked_group_lands_on_the_entry
   ✔ test_the_migrated_menu_still_renders
TestMenuSeeding
   ✔ test_a_new_item_group_does_not_appear_in_the_menu_on_its_own
   ✔ test_install_seeds_the_menu_only_when_the_store_has_none
   ✔ test_seeded_nested_entries_stay_out_of_the_sitemap
   ✔ test_seeding_copies_the_item_group_tree_shape
   ✔ test_seeding_gives_every_entry_valid_tree_bounds
   ✔ test_seeding_twice_adds_nothing
   ✔ test_the_menu_and_the_catalogue_stay_independent
Ran 13 tests in 1.831s
OK

Existing suites, updated for the new shape:

test_navbar_flow      Ran 14 tests   OK (skipped=1)
test_storefront_nav   Ran  8 tests   OK
test_navbar_manager   Ran 24 tests   24/24 TestNavbarManager pass

ruff check and ruff format --check pass across the app.

Known devbox gaps (pre-existing, not from this change)

Flagging so the CI baseline is not read as a regression:

  • TestNavbarPublishCascade.setUpClass errors on LinkValidationError: 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 in setUpClass on frappe.get_all("Style Attribute Configurator", limit=1)[0]IndexError; same missing-catalogue gap. The other 31 pass.
  • after_migrate on that box raises ModuleNotFoundError: No module named 'erpnext.selling.doctype.quotation.mapper' from ls_shop/api/payments.py:8. Reproduced identically at 4afe430, 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

  • Capability reduction, accepted. An entry can no longer link several item groups by hand. Nothing in-tree ever produced that, every consumer flattened it, and existing multi-group data is preserved as siblings rather than dropped. Worth calling out explicitly at review.
  • Diverges from the ivendnext_ecommerce prior 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. Its import_from_item_group and NSM-suppression approach are reused verbatim; only the storage shape differs. Its base_setup.py also 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 exact item_group IN (...) with no descendant expansion — so the tab opens an empty grid. This is pre-existing behaviour of import_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 in get_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant