Merge branch 'main' into feature/63-admin-gate

backend/migrations/1787300000000_default-items-to-pending.js

@@ -0,0 +1,18 @@
+exports.up = (pgm) => {
+  pgm.sql(`
+    -- New items are staged, not published. Before this an item was live on the
+    -- storefront the instant it was created, with no way to add something, look
+    -- at it, and then decide it was ready.
+    --
+    -- Only the default changes. Existing rows keep whatever status they have —
+    -- backfilling would un-publish the entire live catalogue, which is the one
+    -- thing this migration must not do.
+    ALTER TABLE items ALTER COLUMN status SET DEFAULT 'pending';
+  `);
+};
+
+exports.down = (pgm) => {
+  pgm.sql(`
+    ALTER TABLE items ALTER COLUMN status SET DEFAULT 'available';
+  `);
+};

backend/src/itemFilters.ts

@@ -16,12 +16,17 @@ export interface ItemFilters {
   favoritesOnly: boolean;
 }
 
-export type ItemStatus = 'available' | 'reserved' | 'sold';
+export type ItemStatus = 'pending' | 'available' | 'reserved' | 'sold';
 
 // Matched exactly, not case-insensitively: `items.status` only ever holds these
 // lowercase values, so accepting 'Reserved' would quietly return nothing rather
 // than reporting that the filter was wrong.
-const ITEM_STATUSES: readonly string[] = ['available', 'reserved', 'sold'];
+const ITEM_STATUSES: readonly string[] = ['pending', 'available', 'reserved', 'sold'];
+
+// Storefront-invalid statuses. This parser is shared with the admin routes,
+// where filtering by 'pending' is exactly the point, so the public routes have
+// to refuse it themselves rather than the parser refusing it for everyone.
+export const NON_PUBLIC_STATUSES: readonly ItemStatus[] = ['pending'];
 
 export interface BuiltFilter {
   clauses: string[];

backend/src/routes/admin.ts

@@ -267,6 +267,40 @@ router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Respons
   res.json(rows[0]);
 }));
 
+// Publishing is the existing mark-available: it already sets status='available'
+// and clears sold_at, reserved_until and paypal_order_id, all of which are
+// no-ops on a pending item. A second endpoint running the same UPDATE would be
+// duplication, so the admin UI labels that button "Publish" when the item is
+// pending. This is the reverse, and it is not symmetrical — see the guard.
+router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Response) => {
+  const { rows } = await pool.query(`SELECT status FROM items WHERE id = $1`, [req.params.id]);
+  if (!rows.length) {
+    return res.status(404).json({ error: 'not found' });
+  }
+
+  const status = rows[0].status;
+  if (status === 'pending') {
+    return res.status(400).json({ error: 'this item is already pending' });
+  }
+  // Reserved and sold are not drafts. A reserved item is in someone's cart
+  // right now and hiding it would strand them mid-checkout; a sold item is a
+  // record of something that happened, and pulling it back would quietly
+  // rewrite that. Both are refused by name so the reason is on screen rather
+  // than left to be guessed from a generic error.
+  if (status === 'reserved') {
+    return res.status(400).json({ error: 'a customer is holding this item — it cannot be unpublished' });
+  }
+  if (status === 'sold') {
+    return res.status(400).json({ error: 'a sold item cannot be unpublished' });
+  }
+
+  const { rows: updated } = await pool.query(
+    `UPDATE items SET status='pending' WHERE id=$1 RETURNING *`,
+    [req.params.id]
+  );
+  res.json(updated[0]);
+}));
+
 router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Response) => {
   const { rows } = await pool.query(
     `UPDATE items SET status='available', sold_at=NULL, reserved_until=NULL, paypal_order_id=NULL

backend/src/routes/filters.ts

@@ -12,18 +12,26 @@ router.get('/', asyncRoute(async (_req: Request, res: Response) => {
     pool.query(
       `SELECT id, name, parent_id, sort_order FROM categories ORDER BY sort_order, lower(name)`
     ),
+    // Pending items are excluded from the count, not just from the catalogue.
+    // Counting them would show a customer a tag reading "Rare (1)", and
+    // filtering by it would then report that nothing matches.
     pool.query(
-      `SELECT t.id, t.name, t.color, COUNT(it.item_id)::int AS item_count
+      `SELECT t.id, t.name, t.color, COUNT(i.id)::int AS item_count
        FROM tags t
        LEFT JOIN item_tags it ON it.tag_id = t.id
+       LEFT JOIN items i ON i.id = it.item_id AND i.status <> 'pending'
        GROUP BY t.id
        ORDER BY lower(t.name)`
     ),
     // An empty catalogue would otherwise hand the slider a null range.
+    //
+    // Pending items are excluded, or a staged item priced far above or below
+    // everything on sale would stretch the slider to a range no visible item
+    // occupies — the customer drags to the end and finds nothing there.
     pool.query(
       `SELECT COALESCE(MIN(price_cents), 0)::int AS min_cents,
               COALESCE(MAX(price_cents), 0)::int AS max_cents
-       FROM items`
+       FROM items WHERE status <> 'pending'`
     )
   ]);
 

backend/src/routes/items.ts

@@ -2,7 +2,13 @@ import { Router, Request, Response } from 'express';
 import { pool } from '../db';
 import { asyncRoute } from '../asyncRoute';
 import { PUBLIC_ITEM_SELECT } from '../itemSelect';
-import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters';
+import { parseItemFilters, buildItemFilterSql, FilterError, NON_PUBLIC_STATUSES } from '../itemFilters';
+
+// Applied to every public read, unconditionally. This route has never had a
+// status filter of its own — sold items are listed and rendered with a Sold
+// badge on purpose — so hiding pending items cannot be expressed as one more
+// optional filter. It has to be a clause the caller cannot opt out of.
+const EXCLUDE_PENDING = `i.status <> 'pending'`;
 
 const router = Router();
 
@@ -28,14 +34,31 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => {
     return res.status(401).json({ error: 'sign in to filter by favorites' });
   }
 
+  // Refused rather than quietly answered. The filter parser is shared with the
+  // admin routes, where 'pending' is valid, so it parses here too — and with
+  // the exclusion below it would return an empty list, which reads as "no items
+  // match" rather than "you may not ask that".
+  if (filters.status && NON_PUBLIC_STATUSES.includes(filters.status)) {
+    return res.status(400).json({ error: 'invalid status' });
+  }
+
   const { clauses, params } = buildItemFilterSql(filters, 1, req.customerId ?? null);
-  const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
-  const { rows } = await pool.query(`${PUBLIC_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);
+  const where = [EXCLUDE_PENDING, ...clauses].join(' AND ');
+  const { rows } = await pool.query(
+    `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`,
+    params
+  );
   res.json(rows);
 }));
 
 router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
-  const { rows } = await pool.query(`${PUBLIC_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
+  // Excluded here too, not only from the list. A pending item that stayed
+  // fetchable by id would be hidden from the catalogue and still reachable by
+  // anyone who guessed or kept a link.
+  const { rows } = await pool.query(
+    `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`,
+    [req.params.id]
+  );
   if (!rows.length) return res.status(404).json({ error: 'not found' });
   res.json(rows[0]);
 }));

backend/tests/integration/adminInventory.integration.test.ts

@@ -118,7 +118,9 @@ describe('GET /api/admin/items filtering', () => {
 
   it('rejects an unknown status rather than returning everything', async () => {
     await createItem('A', 1000);
-    const res = await request(app).get('/api/admin/items?status=pending');
+    // Not 'pending': that is a real status now, and deliberately valid on the
+    // admin route — filtering for staged items is the point of it.
+    const res = await request(app).get('/api/admin/items?status=archived');
     expect(res.status).toBe(400);
   });
 

backend/tests/integration/cart.integration.test.ts

@@ -20,7 +20,7 @@ async function registerAndGetAgent(email: string) {
 
 describe('cart', () => {
   it('adding an item to cart marks it reserved', async () => {
-    const { rows } = await pool.query(`INSERT INTO items (name, price_cents) VALUES ('Test Item', 1000) RETURNING id`);
+    const { rows } = await pool.query(`INSERT INTO items (name, price_cents, status) VALUES ('Test Item', 1000, 'available') RETURNING id`);
     const itemId = rows[0].id;
     const agent = await registerAndGetAgent('cart1@example.com');
 
@@ -36,7 +36,7 @@ describe('cart', () => {
   });
 
   it('refuses to add an item that is already reserved', async () => {
-    const { rows } = await pool.query(`INSERT INTO items (name, price_cents) VALUES ('Test Item', 1000) RETURNING id`);
+    const { rows } = await pool.query(`INSERT INTO items (name, price_cents, status) VALUES ('Test Item', 1000, 'available') RETURNING id`);
     const itemId = rows[0].id;
     const agentA = await registerAndGetAgent('cartA@example.com');
     const agentB = await registerAndGetAgent('cartB@example.com');
@@ -47,7 +47,7 @@ describe('cart', () => {
   });
 
   it('removing an item from cart releases it back to available', async () => {
-    const { rows } = await pool.query(`INSERT INTO items (name, price_cents) VALUES ('Test Item', 1000) RETURNING id`);
+    const { rows } = await pool.query(`INSERT INTO items (name, price_cents, status) VALUES ('Test Item', 1000, 'available') RETURNING id`);
     const itemId = rows[0].id;
     const agent = await registerAndGetAgent('cart2@example.com');
 
@@ -74,7 +74,7 @@ describe('cart demo checkout', () => {
 
   it('completes a multi-item cart purchase and marks all items sold', async () => {
     const { rows } = await pool.query(
-      `INSERT INTO items (name, price_cents) VALUES ('Item A', 1000), ('Item B', 2000) RETURNING id`
+      `INSERT INTO items (name, price_cents, status) VALUES ('Item A', 1000, 'available'), ('Item B', 2000, 'available') RETURNING id`
     );
     const [itemA, itemB] = rows;
     const agent = await registerAndGetAgent('checkout1@example.com');
@@ -100,7 +100,7 @@ describe('cart demo checkout', () => {
   });
 
   it('refuses checkout without a shipping address', async () => {
-    const { rows } = await pool.query(`INSERT INTO items (name, price_cents) VALUES ('Item A', 1000) RETURNING id`);
+    const { rows } = await pool.query(`INSERT INTO items (name, price_cents, status) VALUES ('Item A', 1000, 'available') RETURNING id`);
     const agent = await registerAndGetAgent('checkout2@example.com');
     await agent.post(`/api/cart/items/${rows[0].id}`);
 

backend/tests/integration/categoriesTags.integration.test.ts

@@ -30,8 +30,11 @@ async function createItem(
   categoryId: number | null = null,
   tagIds: number[] = []
 ): Promise<number> {
+  // status is explicit rather than left to the column default. These tests are
+  // about items a customer can see, and the default is 'pending' — an item is
+  // staged until an admin publishes it.
   const { rows } = await pool.query(
-    `INSERT INTO items (name, price_cents, category_id) VALUES ($1, $2, $3) RETURNING id`,
+    `INSERT INTO items (name, price_cents, category_id, status) VALUES ($1, $2, $3, 'available') RETURNING id`,
     [name, priceCents, categoryId]
   );
   const itemId = rows[0].id;
@@ -311,6 +314,11 @@ describe('admin item form', () => {
       [create.body.id]
     );
 
+    // Published first: this asserts against PUBLIC_ITEM_SELECT specifically,
+    // which is the shape the images-times-tags join bug would show up in, and
+    // a new item is pending and therefore not publicly fetchable.
+    await request(app).post(`/api/admin/items/${create.body.id}/mark-available`);
+
     const res = await request(app).get(`/api/items/${create.body.id}`);
     expect(res.body.images).toHaveLength(2);
     expect(res.body.tags).toHaveLength(3);

backend/tests/integration/disableCustomer.integration.test.ts

@@ -24,7 +24,7 @@ async function register(email: string) {
 
 async function createItem(name: string) {
   const { rows } = await pool.query(
-    `INSERT INTO items (name, price_cents) VALUES ($1, 1000) RETURNING id`,
+    `INSERT INTO items (name, price_cents, status) VALUES ($1, 1000, 'available') RETURNING id`,
     [name]
   );
   return rows[0].id as number;

backend/tests/integration/favorites.integration.test.ts

@@ -30,7 +30,7 @@ async function register(email: string) {
 
 async function createItem(name: string) {
   const { rows } = await pool.query(
-    `INSERT INTO items (name, price_cents) VALUES ($1, 1000) RETURNING id`,
+    `INSERT INTO items (name, price_cents, status) VALUES ($1, 1000, 'available') RETURNING id`,
     [name]
   );
   return rows[0].id as number;

backend/tests/integration/pendingStatus.integration.test.ts

@@ -0,0 +1,225 @@
+import request from 'supertest';
+import app from '../../src/app';
+import { pool } from '../../src/db';
+import { resetDb, closeDb } from './setup/testDb';
+
+beforeEach(async () => {
+  await resetDb();
+});
+
+afterAll(async () => {
+  await pool.end();
+  await closeDb();
+});
+
+async function createTag(name: string): Promise<number> {
+  const res = await request(app).post('/api/admin/tags').send({ name });
+  expect(res.status).toBe(201);
+  return res.body.id;
+}
+
+// Direct insert so a test can put an item in a specific state without going
+// through the transitions being tested.
+async function insertItem(
+  name: string,
+  priceCents: number,
+  status: string,
+  tagIds: number[] = []
+): Promise<number> {
+  const { rows } = await pool.query(
+    `INSERT INTO items (name, price_cents, status) VALUES ($1, $2, $3) RETURNING id`,
+    [name, priceCents, status]
+  );
+  const id = rows[0].id;
+  for (const tagId of tagIds) {
+    await pool.query(`INSERT INTO item_tags (item_id, tag_id) VALUES ($1, $2)`, [id, tagId]);
+  }
+  return id;
+}
+
+describe('a new item is staged rather than published', () => {
+  it('arrives pending when created through the admin API', async () => {
+    const res = await request(app)
+      .post('/api/admin/items')
+      .field('name', 'Fresh')
+      .field('description', '')
+      .field('price', '25');
+
+    expect(res.status).toBe(200);
+    expect(res.body.status).toBe('pending');
+  });
+});
+
+// Each of these is a separate query, so fixing one proves nothing about the
+// others. They are tested separately for that reason.
+describe('a pending item does not reach the storefront', () => {
+  it('is absent from the catalogue listing', async () => {
+    await insertItem('Staged', 1000, 'pending');
+    await insertItem('Live', 2000, 'available');
+
+    const res = await request(app).get('/api/items');
+
+    expect(res.status).toBe(200);
+    expect(res.body.map((i: { name: string }) => i.name)).toEqual(['Live']);
+  });
+
+  // Hiding it from the list but serving it by id would leave it reachable to
+  // anyone who guessed the id or kept an old link.
+  it('is not fetchable by direct id', async () => {
+    const id = await insertItem('Staged', 1000, 'pending');
+
+    const res = await request(app).get(`/api/items/${id}`);
+
+    expect(res.status).toBe(404);
+  });
+
+  it('is still fetchable by id once published', async () => {
+    const id = await insertItem('Staged', 1000, 'pending');
+    await request(app).post(`/api/admin/items/${id}/mark-available`);
+
+    const res = await request(app).get(`/api/items/${id}`);
+
+    expect(res.status).toBe(200);
+    expect(res.body.name).toBe('Staged');
+  });
+
+  // A count including pending items would show a customer "Rare (1)", and
+  // filtering by it would then report that nothing matches.
+  it('is not counted in the filter drawer tag counts', async () => {
+    const tag = await createTag('rare');
+    await insertItem('Staged', 1000, 'pending', [tag]);
+    await insertItem('Live', 2000, 'available', [tag]);
+
+    const res = await request(app).get('/api/filters');
+
+    const rare = res.body.tags.find((t: { name: string }) => t.name === 'rare');
+    expect(rare.item_count).toBe(1);
+  });
+
+  // The tag must still be listed, at zero. Excluding pending items with a WHERE
+  // rather than in the count would drop the tag's row entirely and make the tag
+  // vanish from the drawer.
+  it('leaves a tag whose only item is pending listed with a count of zero', async () => {
+    const tag = await createTag('unreleased');
+    await insertItem('Staged', 1000, 'pending', [tag]);
+
+    const res = await request(app).get('/api/filters');
+
+    const unreleased = res.body.tags.find((t: { name: string }) => t.name === 'unreleased');
+    expect(unreleased).toBeDefined();
+    expect(unreleased.item_count).toBe(0);
+  });
+
+  // A staged item priced far outside the live range would stretch the slider to
+  // a range no visible item occupies.
+  it('does not stretch the price slider bounds', async () => {
+    await insertItem('Cheap live', 1000, 'available');
+    await insertItem('Dear live', 5000, 'available');
+    await insertItem('Absurd staged', 999999, 'pending');
+
+    const res = await request(app).get('/api/filters');
+
+    expect(res.body.priceRange.min_cents).toBe(1000);
+    expect(res.body.priceRange.max_cents).toBe(5000);
+  });
+});
+
+describe('asking the public API for pending items', () => {
+  // Refused rather than answered with an empty list, which would read as "no
+  // items match" instead of "you may not ask that".
+  it('is refused on the storefront route', async () => {
+    await insertItem('Staged', 1000, 'pending');
+
+    const res = await request(app).get('/api/items?status=pending');
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toBe('invalid status');
+  });
+
+  it('is still allowed on the admin route, which is the point of it', async () => {
+    await insertItem('Staged', 1000, 'pending');
+    await insertItem('Live', 2000, 'available');
+
+    const res = await request(app).get('/api/admin/items?status=pending');
+
+    expect(res.status).toBe(200);
+    expect(res.body.map((i: { name: string }) => i.name)).toEqual(['Staged']);
+  });
+
+  it('leaves the other statuses working on the storefront', async () => {
+    await insertItem('Live', 1000, 'available');
+    await insertItem('Gone', 2000, 'sold');
+
+    const res = await request(app).get('/api/items?status=sold');
+
+    expect(res.status).toBe(200);
+    expect(res.body.map((i: { name: string }) => i.name)).toEqual(['Gone']);
+  });
+});
+
+describe('unpublishing', () => {
+  it('returns an available item to pending and removes it from the catalogue', async () => {
+    const id = await insertItem('Live', 1000, 'available');
+
+    const res = await request(app).post(`/api/admin/items/${id}/unpublish`);
+
+    expect(res.status).toBe(200);
+    expect(res.body.status).toBe('pending');
+    const listing = await request(app).get('/api/items');
+    expect(listing.body).toHaveLength(0);
+  });
+
+  // Not a draft: someone is holding it in their cart right now, and hiding it
+  // would strand them mid-checkout.
+  it('refuses a reserved item and says why', async () => {
+    const id = await insertItem('Held', 1000, 'reserved');
+
+    const res = await request(app).post(`/api/admin/items/${id}/unpublish`);
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('holding this item');
+    const { rows } = await pool.query(`SELECT status FROM items WHERE id = $1`, [id]);
+    expect(rows[0].status).toBe('reserved');
+  });
+
+  // Not a draft either: a sold item is a record of something that happened.
+  it('refuses a sold item and says why', async () => {
+    const id = await insertItem('Gone', 1000, 'sold');
+
+    const res = await request(app).post(`/api/admin/items/${id}/unpublish`);
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('sold item');
+    const { rows } = await pool.query(`SELECT status FROM items WHERE id = $1`, [id]);
+    expect(rows[0].status).toBe('sold');
+  });
+
+  it('refuses an item that is already pending', async () => {
+    const id = await insertItem('Staged', 1000, 'pending');
+
+    const res = await request(app).post(`/api/admin/items/${id}/unpublish`);
+
+    expect(res.status).toBe(400);
+    expect(res.body.error).toContain('already pending');
+  });
+
+  it('reports a missing item as not found rather than as a bad request', async () => {
+    const res = await request(app).post('/api/admin/items/999999/unpublish');
+
+    expect(res.status).toBe(404);
+  });
+});
+
+describe('publishing', () => {
+  it('puts a pending item into the catalogue', async () => {
+    const id = await insertItem('Staged', 1000, 'pending');
+    expect((await request(app).get('/api/items')).body).toHaveLength(0);
+
+    const res = await request(app).post(`/api/admin/items/${id}/mark-available`);
+
+    expect(res.status).toBe(200);
+    expect(res.body.status).toBe('available');
+    const listing = await request(app).get('/api/items');
+    expect(listing.body.map((i: { name: string }) => i.name)).toEqual(['Staged']);
+  });
+});

backend/tests/unit/itemFilters.test.ts

@@ -107,7 +107,9 @@ describe('parseItemFilters', () => {
   });
 
   it('rejects a status outside the known set', () => {
-    expect(() => parseItemFilters({ status: 'pending' })).toThrow(FilterError);
+    // 'pending' used to be the example here and is now a real status, which is
+    // exactly the sort of thing that quietly turns a test into a tautology.
+    expect(() => parseItemFilters({ status: 'archived' })).toThrow(FilterError);
   });
 
   it('rejects a status differing only by case, rather than silently coercing it', () => {