PN-27: Add brands filter to POS items

[MohamedAliSmk]

• Add POS Brands Detail child table doctype for POS Profile
• Add custom_brands_table custom field to POS Profile via fixtures
• Add get_brands API and brand parameter to get_items/get_items_count
• Add brand-aware filter handling in itemSearch store
• Update ItemsSelector UI with dynamic brand/item_group tab switching
• Add brand sort option with context-aware sort menu

[engahmed1190]

Deep Code Review — PR #180

Files changed: 8 | +401 / -32

---

Critical Issues

1. Missing custom field fixture for custom_brands_table — will crash on install/migrate

hooks.py references "POS Profile-custom_brands_table" in the fixtures list, but the actual Custom Field JSON definition for this field is not included in pos_next/fixtures/custom_field.json. On a fresh install or bench migrate, Frappe will try to load this fixture and fail (or silently skip it), meaning the POS Profile will have no custom_brands_table field. The "POS Brands Detail" child doctype exists but is never wired to the POS Profile.

2. pos_profile.append("brands", ...) uses wrong fieldname

In pos_profile.py, both create_pos_profile and update_pos_profile do:

pos_profile.append("brands", {"brand": brand_name})

But the custom field on POS Profile is named custom_brands_table (per the hooks fixture reference), not brands. This will throw a ValueError at runtime — Frappe's append() validates the fieldname against the doctype. Combined with issue #1 (missing fixture), the entire brands CRUD flow in POS Profile management is broken.

3. IFNULL(i.brand, '') IN (...) includes items with no brand when brands are configured

In _build_item_base_conditions:

conditions.append(f"IFNULL(i.brand, '') IN ({placeholders})")
where_params.extend(allowed_brands)

This uses IFNULL(i.brand, '') which converts NULL brands to empty string. If allowed_brands doesn't contain an empty string (it won't — it comes from Link field values), items with NULL brand are excluded. But if someone accidentally adds an empty brand entry, all brand-less items leak through. Should use i.brand IN (...) with a simple IS NOT NULL guard, or explicitly document the behavior.

Actually, re-reading: this is correct in intent (only show items matching configured brands), but the IFNULL wrapper is unnecessary — i.brand IN (...) already excludes NULLs. The IFNULL just adds confusion.

---

Logic Concerns

4. get_items_bulk doesn't pass brand to _build_item_base_conditions

The bulk fetch function (used for offline background sync) calls _build_item_base_conditions without the brand parameter. Since _build_item_base_conditions now also filters by allowed_brands from the POS Profile, the brand restriction (allowed brands) still applies. But the per-brand selection filter won't. This is probably fine for bulk sync, but worth noting — the allowed brands filter will now affect offline sync results, which is a behavioral change for existing users who had no brands configured (they'll get all items) vs those who had brands configured elsewhere.

5. Brand filter and item_group filter are mutually exclusive in the frontend but not in the backend

The backend _build_item_base_conditions accepts BOTH item_group and brand simultaneously and AND's them together. The frontend enforces mutual exclusivity (switching to brand clears item_group and vice versa). If anyone calls the API directly with both, they get an intersection. This isn't necessarily wrong but the implicit contract differs between frontend and backend — document or enforce one way.

6. get_brands fallback returns ALL brands (limit 50) when none configured

if not configured_brands:
    result = (
        frappe.qb.from_(Brand)
        .select(Brand.name.as_("brand"))
        .orderby(Brand.name)
        .limit(50)
        .run(as_dict=True)
    )

When no brands are configured in the POS Profile, it falls back to returning the first 50 brands from the system. This is inconsistent with how item groups work (which only show configured groups). It also means the allowed-brands filter in _build_item_base_conditions returns empty (no restriction), so all items show up, but the brand tabs show only 50 — a mismatch that could confuse users browsing brand "X" but seeing items from brand "Y" in "All Items".

7. get_brands cache is never invalidated

cache_key = f"pos_brands:{pos_profile}"
frappe.cache().set_value(cache_key, result, expires_in_sec=300)

5-minute TTL, but no cache invalidation when the POS Profile brands are updated via update_pos_profile. User updates brands, refreshes POS → stale data for up to 5 minutes. Should invalidate in update_pos_profile or at minimum document this behavior.

8. Sort switching triggers data reload but doesn't cancel in-flight requests

watch(sortBy, async (newSortBy, oldSortBy) => {
    if (newSortBy === 'brand') {
        await itemStore.loadBrands()
        if (selectedItemGroup.value) {
            await itemStore.setSelectedItemGroup(null)
        }
    }
})

Rapidly toggling sort options can stack multiple async operations. No cancellation token or generation counter (unlike other parts of the store that use syncGeneration).

---

Performance

9. _get_allowed_profile_brands runs on EVERY _build_item_base_conditions call

allowed_brands = _get_allowed_profile_brands(pos_profile_doc.name)

This executes a DB query (frappe.get_all("POS Brands Detail", ...)) on every single item fetch, search, count, and bulk load. Should use frappe.get_cached_doc or cache the result per-profile, similar to how item groups are handled.

10. Offline brand filtering fetches 5000 items then filters in JS

const cached = await offlineWorker.searchCachedItems("", 5000, 0)
const filtered = brand
    ? (cached || []).filter(item => (item.brand || '') === brand)
    : (cached || [])

Fetches up to 5000 items from IndexedDB just to filter by brand. Should use an IndexedDB index on brand field (like the existing variant_of index) and query directly.

---

Code Quality

11. Indentation error in loadMore

brand: undefined,
start: currentOffset.value,

The brand: undefined line has extra indentation (tabs don't match the surrounding object properties). Will work but looks sloppy.

12. brand: undefined passed in multiple API calls

Several places pass brand: undefined explicitly:

brand: undefined,

In Frappe's call(), undefined values are stripped from the request. So this works but is noise — just omit the property entirely. It clutters the code and suggests the author wasn't sure how the API wrapper handles undefined.

13. DocType import not shown in diff for get_brands

The get_brands function uses DocType("POS Brands Detail") and DocType("Brand") via pypika query builder, but the import for DocType isn't visible in the diff. Need to verify from pypika import DocType or from frappe.query_builder import DocType exists at the top of items.py.

14. _get_allowed_profile_brands duplicates query logic with get_brands

Both functions query POS Brands Detail for the same POS Profile. _get_allowed_profile_brands uses frappe.get_all while get_brands uses pypika query builder. Should share a single source of truth.

---

Risk Assessment

Risk	Level
Missing custom field fixture — broken on fresh install	High
Wrong fieldname brands vs custom_brands_table in pos_profile.py	High
No cache invalidation for brands	Medium
_get_allowed_profile_brands DB hit on every item query	Medium
Offline 5000-item fetch for brand filter	Medium
No request cancellation on rapid sort toggle	Low
Inconsistent brand/item_group mutual exclusivity	Low

Verdict

Not ready to merge. The two critical issues (#1 missing fixture, #2 wrong fieldname) mean the entire brands feature is non-functional for POS Profile CRUD. The _build_item_base_conditions changes will work for item filtering but the management/configuration side is broken. Fix the fixture, fix the fieldname, add cache invalidation, and consider an IndexedDB index for offline brand queries.

[MohamedAliSmk]

no 4:
current code is consistent:
_build_item_base_conditions enforces global brand restrictions from POS Profile.
get_items_bulk is for broad offline cache, not per-brand slices.