From 3f96e6cdb30a1319788d1d9ca20b7cb0bb239afa Mon Sep 17 00:00:00 2001 From: Oier Bravo Urtasun Date: Mon, 18 May 2026 11:18:37 +0200 Subject: [PATCH] feat(fase-15): fetchSuggestions weight ordering + commonItems store (15.2) fetchSuggestions now: - Orders by `weight DESC, use_count DESC, last_used_at DESC` so admin- promoted items lead the dropdown regardless of historical use_count. - Accepts `{ excludeNames?: string[]; limit?: number }`. The exclude filter is applied as `.not('name', 'in', '(...)')` only when the sanitised list is non-empty (PostgREST renders an empty list as a SQL error). Each name is lowercased + trimmed to match the normalised form `item_frequency.name` is stored in. commonItems.ts is the unbounded counterpart used by /collective/manage: loadCommonItems (full catalogue, same sort), setWeight (RPC + optimistic upsert into the store), purge (RPC + optimistic remove). Integration test common-items.test.ts (6 specs) covers the role gate (admin OK, member/guest get P0002), the boost-promotes-low-use-count ordering, the excludeNames filter, the RPC purge, and the seed-via-RPC-with-use_count=0 flow. Co-Authored-By: Claude Opus 4.7 --- apps/web/src/lib/stores/commonItems.ts | 105 +++++++++++ apps/web/src/lib/stores/lists.ts | 65 +++++-- .../test-utils/tests/common-items.test.ts | 170 ++++++++++++++++++ 3 files changed, 326 insertions(+), 14 deletions(-) create mode 100644 apps/web/src/lib/stores/commonItems.ts create mode 100644 packages/test-utils/tests/common-items.test.ts diff --git a/apps/web/src/lib/stores/commonItems.ts b/apps/web/src/lib/stores/commonItems.ts new file mode 100644 index 0000000..84f6cc7 --- /dev/null +++ b/apps/web/src/lib/stores/commonItems.ts @@ -0,0 +1,105 @@ +/** + * Common-items store (Fase 15) — the admin curation view of the per-collective + * `item_frequency` table. Unlike `fetchSuggestions` (which is the bounded + * dropdown query on the lists page), this loader is unbounded: the manage + * screen shows the full catalogue so admins can sort, search and act on it. + * + * Writes go through SECURITY DEFINER RPCs that gate on + * `collective_members.role = 'admin'`. The all-deny RLS from migration 006 + * is the safety net; the RPCs raise P0002 ("only admins…") for non-admins. + */ +import { writable } from 'svelte/store'; +import { getSupabase } from '$lib/supabase'; +import type { ItemFrequency } from '@colectivo/types'; + +export const commonItems = writable([]); +export const commonItemsLoading = writable(false); + +export async function loadCommonItems(collectiveId: string): Promise { + commonItemsLoading.set(true); + try { + const { data, error } = await getSupabase() + .from('item_frequency') + .select('*') + .eq('collective_id', collectiveId) + .order('weight', { ascending: false }) + .order('use_count', { ascending: false }) + .order('last_used_at', { ascending: false }); + + if (!error && data) { + commonItems.set(data as ItemFrequency[]); + } + } finally { + commonItemsLoading.set(false); + } +} + +/** + * Upsert weight for a single (collective, name). The RPC normalises name to + * lower(trim(...)) internally; we mirror the same normalisation client-side + * so the optimistic update lands on the right row. + * + * Returns the error (or null) so the caller can surface it as toast/message. + */ +export async function setWeight( + collectiveId: string, + name: string, + weight: number +): Promise { + const normalized = name.toLowerCase().trim(); + if (!normalized) return new Error('name must not be empty'); + + const { error } = await getSupabase().rpc('set_item_frequency_weight', { + p_collective_id: collectiveId, + p_name: normalized, + p_weight: weight + }); + if (error) return new Error(error.message); + + // Optimistic local update — upsert into the store. + commonItems.update((arr) => { + const idx = arr.findIndex((r) => r.name === normalized); + if (idx >= 0) { + const next = [...arr]; + next[idx] = { ...next[idx], weight }; + return sortRows(next); + } + const row: ItemFrequency = { + collective_id: collectiveId, + name: normalized, + use_count: 0, + weight, + last_used_at: new Date().toISOString() + }; + return sortRows([...arr, row]); + }); + return null; +} + +/** + * Delete the (collective, name) row from item_frequency. Does NOT touch + * shopping_items — items already living in lists are untouched, but the + * dropdown will stop suggesting this name until someone adds it again (the + * existing trigger will re-create the row with weight=0 then). + */ +export async function purge(collectiveId: string, name: string): Promise { + const normalized = name.toLowerCase().trim(); + if (!normalized) return new Error('name must not be empty'); + + const { error } = await getSupabase().rpc('purge_item_frequency', { + p_collective_id: collectiveId, + p_name: normalized + }); + if (error) return new Error(error.message); + + commonItems.update((arr) => arr.filter((r) => r.name !== normalized)); + return null; +} + +function sortRows(arr: ItemFrequency[]): ItemFrequency[] { + return [...arr].sort((a, b) => { + if (b.weight !== a.weight) return b.weight - a.weight; + if (b.use_count !== a.use_count) return b.use_count - a.use_count; + return b.last_used_at.localeCompare(a.last_used_at); + }); +} diff --git a/apps/web/src/lib/stores/lists.ts b/apps/web/src/lib/stores/lists.ts index 4c0c435..4a1b1d3 100644 --- a/apps/web/src/lib/stores/lists.ts +++ b/apps/web/src/lib/stores/lists.ts @@ -274,26 +274,63 @@ export async function reorderItems(items: Pick[]) { // ── Suggestions (item_frequency) ────────────────────────────────────────────── +/** + * Fase 15: ordering switches from pure `use_count DESC` to + * `weight DESC, use_count DESC, last_used_at DESC` — admins can curate the + * top of the dropdown via the new `set_item_frequency_weight` RPC. + * + * `excludeNames` (optional) drops entries already present in the caller's + * working list so the user is not nudged to re-add what they already have. + * Each entry is lowercased + trimmed before being sent (matching the + * normalized form `item_frequency.name` is stored in). When the array is + * empty the .not() filter is skipped entirely — PostgREST renders an empty + * `(...)` list as a SQL error, and the cost of adding the filter for zero + * effect would be wasted bytes on the URL anyway. + * + * `limit` overrides the default (5 without prefix, 10 with). The /collective + * manage view does not call this — it has its own unbounded list query in + * commonItems.ts — but UI experimentation may want a smaller bar. + */ +export interface FetchSuggestionsOptions { + excludeNames?: string[]; + limit?: number; +} + export async function fetchSuggestions( collectiveId: string, - prefix: string + prefix: string, + options: FetchSuggestionsOptions = {} ): Promise { - if (!prefix.trim()) { - const { data } = await getSupabase() - .from('item_frequency') - .select('*') - .eq('collective_id', collectiveId) - .order('use_count', { ascending: false }) - .limit(5); - return (data as ItemFrequency[]) ?? []; - } + const hasPrefix = !!prefix.trim(); + const limit = options.limit ?? (hasPrefix ? 10 : 5); - const { data } = await getSupabase() + let query = getSupabase() .from('item_frequency') .select('*') - .eq('collective_id', collectiveId) - .ilike('name', `${prefix.toLowerCase().trim()}%`) + .eq('collective_id', collectiveId); + + if (hasPrefix) { + query = query.ilike('name', `${prefix.toLowerCase().trim()}%`); + } + + const exclude = (options.excludeNames ?? []) + .map((n) => n.toLowerCase().trim()) + .filter((n) => n.length > 0); + + if (exclude.length > 0) { + // PostgREST in() takes a comma-separated list wrapped in parens. We + // double-quote each entry to be safe against names containing commas + // or parens (e.g. "rice (white)"); the inner double quotes are escaped + // per PostgREST's filter grammar. + const list = `(${exclude.map((n) => `"${n.replace(/"/g, '\\"')}"`).join(',')})`; + query = query.not('name', 'in', list); + } + + const { data } = await query + .order('weight', { ascending: false }) .order('use_count', { ascending: false }) - .limit(10); + .order('last_used_at', { ascending: false }) + .limit(limit); + return (data as ItemFrequency[]) ?? []; } diff --git a/packages/test-utils/tests/common-items.test.ts b/packages/test-utils/tests/common-items.test.ts new file mode 100644 index 0000000..5116c75 --- /dev/null +++ b/packages/test-utils/tests/common-items.test.ts @@ -0,0 +1,170 @@ +/** + * CI-INT-series: common items (item_frequency.weight + admin RPCs + suggestion + * ordering with weight). Fase 15. + * + * The RPC role-gate is what pgTAP can't cover (postgres bypasses auth.uid), + * so we drive both happy and denied paths through authenticated JWTs here. + * + * CI-INT-01 admin set weight=50 → suggestions surface that item first. + * CI-INT-02 excludeNames filters out the listed entries. + * CI-INT-03 member set weight → P0002. + * CI-INT-04 purge as admin removes the row. + * CI-INT-05 purge as guest → P0002. + * CI-INT-06 set_item_frequency_weight on a brand-new name inserts a row + * with use_count=0. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { createClientAs, createAdminClient } from '../src/supabase-clients.js'; +import { ANA_ID, BORJA_ID, DAVID_ID, COLLECTIVE_ID } from '../src/seed-constants.js'; + +const admin = createAdminClient(); + +// The integration tests share a single seed collective with everything else; +// we tag our test rows with a unique-ish prefix so cleanup is bounded and we +// don't trip over the seeded "milk" / "bread" / etc. +const PREFIX = 'cifrq-'; +const ITEM_BOOST = `${PREFIX}boost`; +const ITEM_NORMAL = `${PREFIX}normal`; +const ITEM_HIDDEN = `${PREFIX}hidden`; +const ITEM_NEW = `${PREFIX}seeded-new`; +const ITEM_TO_PURGE = `${PREFIX}purgeme`; + +beforeAll(async () => { + // Seed three rows with descending use_counts; the boost test relies on + // "boost" having a LOWER use_count than "normal" so the only way it can + // surface first is via weight. + await admin + .from('item_frequency') + .upsert( + [ + { collective_id: COLLECTIVE_ID, name: ITEM_NORMAL, use_count: 10, weight: 0 }, + { collective_id: COLLECTIVE_ID, name: ITEM_BOOST, use_count: 1, weight: 0 }, + { collective_id: COLLECTIVE_ID, name: ITEM_HIDDEN, use_count: 5, weight: 0 }, + { collective_id: COLLECTIVE_ID, name: ITEM_TO_PURGE, use_count: 3, weight: 0 } + ], + { onConflict: 'collective_id,name' } + ); +}); + +afterAll(async () => { + await admin + .from('item_frequency') + .delete() + .eq('collective_id', COLLECTIVE_ID) + .like('name', `${PREFIX}%`); +}); + +describe('item_frequency.weight + admin RPCs', () => { + it('CI-INT-01: admin set weight=50 promotes the row in the ordered query', async () => { + const ana = await createClientAs(ANA_ID); + + // Reset baseline first so the test is order-independent inside a single run. + await ana.rpc('set_item_frequency_weight', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_NORMAL, + p_weight: 0 + }); + + const { error: rpcError } = await ana.rpc('set_item_frequency_weight', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_BOOST, + p_weight: 50 + }); + expect(rpcError).toBeNull(); + + // Mirror the production suggestion query (no prefix): order by weight, + // then use_count, then last_used_at. Boost has use_count=1 vs normal's + // 10 — weight is the only way it can surface above. + const { data, error } = await ana + .from('item_frequency') + .select('name, weight, use_count') + .eq('collective_id', COLLECTIVE_ID) + .like('name', `${PREFIX}%`) + .order('weight', { ascending: false }) + .order('use_count', { ascending: false }); + + expect(error).toBeNull(); + expect(data?.[0]?.name).toBe(ITEM_BOOST); + expect(data?.[0]?.weight).toBe(50); + }); + + it('CI-INT-02: excludeNames filters out the listed entries via .not("name","in",...)', async () => { + const ana = await createClientAs(ANA_ID); + const exclude = [ITEM_BOOST, ITEM_NORMAL]; + const excludeList = `(${exclude.map((n) => `"${n}"`).join(',')})`; + + const { data, error } = await ana + .from('item_frequency') + .select('name') + .eq('collective_id', COLLECTIVE_ID) + .like('name', `${PREFIX}%`) + .not('name', 'in', excludeList); + + expect(error).toBeNull(); + const names = (data ?? []).map((r) => r.name); + expect(names).not.toContain(ITEM_BOOST); + expect(names).not.toContain(ITEM_NORMAL); + // The other test rows should still be there. + expect(names).toContain(ITEM_HIDDEN); + }); + + it('CI-INT-03: member (Borja) cannot set weight — RPC raises P0002', async () => { + const borja = await createClientAs(BORJA_ID); + const { error } = await borja.rpc('set_item_frequency_weight', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_HIDDEN, + p_weight: -50 + }); + expect(error).not.toBeNull(); + // PostgREST surfaces the SQLSTATE as `code` on the error object. + // P0002 is the custom errcode the RPC raises on the role gate. + expect((error as { code?: string }).code).toBe('P0002'); + }); + + it('CI-INT-04: admin (Ana) can purge a row via purge_item_frequency', async () => { + const ana = await createClientAs(ANA_ID); + const { error: rpcError } = await ana.rpc('purge_item_frequency', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_TO_PURGE + }); + expect(rpcError).toBeNull(); + + const { data } = await admin + .from('item_frequency') + .select('name') + .eq('collective_id', COLLECTIVE_ID) + .eq('name', ITEM_TO_PURGE) + .maybeSingle(); + expect(data).toBeNull(); + }); + + it('CI-INT-05: guest (David) cannot purge — RPC raises P0002', async () => { + const david = await createClientAs(DAVID_ID); + const { error } = await david.rpc('purge_item_frequency', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_HIDDEN + }); + expect(error).not.toBeNull(); + expect((error as { code?: string }).code).toBe('P0002'); + }); + + it('CI-INT-06: set_item_frequency_weight on a new name inserts row with use_count=0', async () => { + const ana = await createClientAs(ANA_ID); + const { error: rpcError } = await ana.rpc('set_item_frequency_weight', { + p_collective_id: COLLECTIVE_ID, + p_name: ITEM_NEW, + p_weight: 50 + }); + expect(rpcError).toBeNull(); + + const { data } = await admin + .from('item_frequency') + .select('name, use_count, weight') + .eq('collective_id', COLLECTIVE_ID) + .eq('name', ITEM_NEW) + .single(); + expect(data?.use_count).toBe(0); + expect(data?.weight).toBe(50); + expect(data?.name).toBe(ITEM_NEW); + }); +});