From fc7cb226f42efe054ebc9c8b4596d292e9f940c9 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:31:09 +0200 Subject: [PATCH] fix(archives): run search, filter and sort server-side ArchivesPage fetched one 20-row page and then filtered and sorted only that array in memory, while "Showing X of 802" / "Page 1 of 41" kept reporting the full unfiltered count. Searching for an archive that exists but is not on the current page returned a false "0 results" with no hint the search was page-scoped. The backend did not support the params (it read only page/limit and hardcoded orderBy archived_at desc), so all three are new. Follows adminEvents/crud.js for the shape and customerAccountsService for the case-insensitive predicate: whereRaw with a bound parameter, never interpolated, and sortBy whitelisted to date/name/size before it reaches orderBy. The same applyFilters() closure runs against both the count query and the row query, so the total cannot drift from the rows again. Frontend mirrors EventsListPage: 300ms debounce, reset to page 1 on any query change, placeholderData so keystrokes don't flash the spinner. Two interpretation calls: - sortBy=size orders by summed photo bytes, not the zip's on-disk size. The Size column comes from a per-row fs.stat done after pagination and there is no archive_size column, so a global sort on the real zip size would stat all 802 files per request. Ordering is near-identical except for rows whose zip is missing. Adding events.archive_size would be a migration, out of scope. - No LIKE-metacharacter escaping. escapeLikePattern() does .replace(/'/g,"''"), which corrupts a bound value ("Sarah's Birthday"), and its backslash escaping is a no-op on SQLite without an ESCAPE clause. Matched customerAccountsService instead. A literal % typed by an admin acts as a wildcard in a read-only search; no injection risk. Pre-existing and untouched: the four stat cards still aggregate the current page only. Refs testplan REPORT.md #9 (Part 3, I.01). --- .../routes/adminArchivesQuery.test.js | 209 ++++++++++++++++++ backend/src/routes/adminArchives.js | 59 +++-- frontend/src/pages/admin/ArchivesPage.tsx | 55 ++--- .../archivesServerSideQuery.test.tsx | 136 ++++++++++++ frontend/src/services/archive.service.ts | 15 +- 5 files changed, 424 insertions(+), 50 deletions(-) create mode 100644 backend/__tests__/routes/adminArchivesQuery.test.js create mode 100644 frontend/src/pages/admin/__tests__/archivesServerSideQuery.test.tsx diff --git a/backend/__tests__/routes/adminArchivesQuery.test.js b/backend/__tests__/routes/adminArchivesQuery.test.js new file mode 100644 index 00000000..1904f53f --- /dev/null +++ b/backend/__tests__/routes/adminArchivesQuery.test.js @@ -0,0 +1,209 @@ +/** + * The archives list must resolve search / type filter / sort in SQL. + * + * Before this, GET /admin/archives ignored every query param except page and + * limit: the UI fetched one 20-row page and filtered it in JavaScript while + * the pagination footer kept reporting the unfiltered server-side total. An + * archive that matched the search but lived on another page came back as a + * false "0 results". These tests pin the params the route now honours, and + * — the part that actually made the bug visible — that `pagination.total` + * describes the *filtered* set. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-archquery-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'archquery-test-secret'; + +const request = require('supertest'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal, buildRouteApp } = require('../integration/helpers/crmDb'); + +describe('GET /admin/archives query params (#I.01)', () => { + let db; let cleanup; let app; let token; + + // name, type, archived_at, photo sizes + const fixtures = [ + ['Alpha Wedding', 'wedding', '2026-01-05T10:00:00.000Z', [300]], + ['Bravo Birthday', 'birthday', '2026-02-05T10:00:00.000Z', [100]], + ['Charlie Wedding', 'wedding', '2026-03-05T10:00:00.000Z', [500, 400]], + ['Delta Corporate', 'corporate', '2026-04-05T10:00:00.000Z', [200]], + ['Echo WEDDING Gala', 'wedding', '2026-05-05T10:00:00.000Z', [50]], + ]; + + const list = async (query) => { + const res = await request(app) + .get('/admin/archives') + .query(query) + .set('Authorization', `Bearer ${token}`); + expect(res.status).toBe(200); + return res.body; + }; + + const names = (body) => body.archives.map((a) => a.eventName); + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const role = await db('roles').where({ name: 'super_admin' }).first(); + const inserted = await db('admin_users').insert({ + username: 'arch-admin', + email: 'arch-admin@example.com', + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }).returning('id'); + const adminId = inserted[0]?.id ?? inserted[0]; + token = jwt.sign( + { id: adminId, username: 'arch-admin', type: 'admin', role: 'super_admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + + let i = 0; + for (const [eventName, eventType, archivedAt, sizes] of fixtures) { + const slug = `arch-${i++}`; + const ev = await db('events').insert({ + slug, + event_type: eventType, + event_name: eventName, + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: `tok-${slug}`, + share_link: `/gallery/${slug}/tok-${slug}`, + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 0, + is_archived: 1, + is_draft: 0, + archived_at: archivedAt, + created_at: new Date().toISOString(), + }).returning('id'); + const eventId = ev[0]?.id ?? ev[0]; + + let p = 0; + for (const size of sizes) { + await db('photos').insert({ + event_id: eventId, + filename: `${slug}-${p++}.jpg`, + path: `events/archived/${slug}.jpg`, + type: 'individual', + size_bytes: size, + uploaded_at: new Date().toISOString(), + }); + } + } + + // A live event that must never surface in the archives list. + await db('events').insert({ + slug: 'not-archived', + event_type: 'wedding', + event_name: 'Alpha Live Wedding', + event_date: '2026-08-01', + host_email: 'h@example.com', + admin_email: 'a@example.com', + password_hash: 'x', + share_token: 'tok-not-archived', + share_link: '/gallery/not-archived/tok-not-archived', + expires_at: new Date(Date.now() + 7 * 864e5).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }); + + app = buildRouteApp('/admin/archives', require('../../src/routes/adminArchives')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + test('no params: every archive, newest first, unfiltered total', async () => { + const body = await list({}); + expect(names(body)).toEqual([ + 'Echo WEDDING Gala', 'Delta Corporate', 'Charlie Wedding', 'Bravo Birthday', 'Alpha Wedding', + ]); + expect(body.pagination.total).toBe(5); + }); + + test('search filters in SQL and the total describes the filtered set', async () => { + const body = await list({ search: 'wedding' }); + // Case-insensitive, matches "Echo WEDDING Gala" too, and never the + // non-archived "Alpha Live Wedding". + expect(names(body).sort()).toEqual(['Alpha Wedding', 'Charlie Wedding', 'Echo WEDDING Gala']); + expect(body.pagination.total).toBe(3); + expect(body.pagination.totalPages).toBe(1); + }); + + test('search reaches rows that are not on page 1 — the actual bug', async () => { + // limit=2 puts "Alpha Wedding" (oldest) on page 3 of the unfiltered list. + // Client-side filtering of page 1 returned nothing for this query. + const body = await list({ search: 'alpha', limit: 2, page: 1 }); + expect(names(body)).toEqual(['Alpha Wedding']); + expect(body.pagination.total).toBe(1); + }); + + test('search with no match returns an empty page and a zero total', async () => { + const body = await list({ search: 'zzz-nothing' }); + expect(body.archives).toEqual([]); + expect(body.pagination.total).toBe(0); + expect(body.pagination.totalPages).toBe(0); + }); + + test('type filter narrows the rows and the total; "all" is a no-op', async () => { + const filtered = await list({ type: 'wedding' }); + expect(names(filtered).sort()).toEqual(['Alpha Wedding', 'Charlie Wedding', 'Echo WEDDING Gala']); + expect(filtered.pagination.total).toBe(3); + + const all = await list({ type: 'all' }); + expect(all.pagination.total).toBe(5); + }); + + test('search and type filter combine', async () => { + const body = await list({ search: 'wedding', type: 'birthday' }); + expect(body.archives).toEqual([]); + expect(body.pagination.total).toBe(0); + }); + + test('sortBy=name orders across the whole set, not just the page', async () => { + const page1 = await list({ sortBy: 'name', limit: 2, page: 1 }); + expect(names(page1)).toEqual(['Alpha Wedding', 'Bravo Birthday']); + + const page3 = await list({ sortBy: 'name', limit: 2, page: 3 }); + expect(names(page3)).toEqual(['Echo WEDDING Gala']); + }); + + test('sortBy=size orders by archived content size, largest first', async () => { + const body = await list({ sortBy: 'size' }); + expect(names(body)).toEqual([ + 'Charlie Wedding', // 900 + 'Alpha Wedding', // 300 + 'Delta Corporate', // 200 + 'Bravo Birthday', // 100 + 'Echo WEDDING Gala' // 50 + ]); + }); + + test('an unknown sortBy falls back to the date ordering', async () => { + const body = await list({ sortBy: 'events.id; drop table events' }); + expect(names(body)[0]).toBe('Echo WEDDING Gala'); + expect(body.pagination.total).toBe(5); + }); + + test('quotes in the search are bound as a value, not injected as SQL', async () => { + const body = await list({ search: '\'; DROP TABLE events; --' }); + expect(body.archives).toEqual([]); + // The table is still there. + expect((await list({})).pagination.total).toBe(5); + }); +}); diff --git a/backend/src/routes/adminArchives.js b/backend/src/routes/adminArchives.js index 4a25213a..5a3ffdd1 100644 --- a/backend/src/routes/adminArchives.js +++ b/backend/src/routes/adminArchives.js @@ -19,26 +19,53 @@ const router = express.Router(); router.get('/', adminAuth, requirePermission('archives.view'), async (req, res) => { try { const { page, limit, offset } = getPagination(req); + const search = typeof req.query.search === 'string' ? req.query.search.trim() : ''; + const type = typeof req.query.type === 'string' ? req.query.type.trim() : ''; + const sortBy = ['date', 'name', 'size'].includes(req.query.sortBy) ? req.query.sortBy : 'date'; - // Get total count - const totalCount = await db('events') - .where('is_archived', formatBoolean(true)) - .count('id as count') + // Search and type filtering run in SQL so both the returned rows and + // the total count cover the whole archive table, not just the page the + // client happens to be on. Values are bound, never interpolated. + const applyFilters = (query) => { + if (search) { + query.whereRaw('LOWER(events.event_name) LIKE ?', [`%${search.toLowerCase()}%`]); + } + if (type && type !== 'all') { + query.where('events.event_type', type); + } + return query; + }; + + // Get total count (of the filtered set, so pagination stays truthful) + const totalCount = await applyFilters( + db('events').where('events.is_archived', formatBoolean(true)) + ) + .count('events.id as count') .first(); // Get archived events - const archives = await db('events') - .select( - 'events.*', - db.raw('COUNT(DISTINCT photos.id) as photo_count'), - db.raw('SUM(photos.size_bytes) as total_size') - ) - .leftJoin('photos', 'events.id', 'photos.event_id') - .where('events.is_archived', formatBoolean(true)) - .groupBy('events.id') - .orderBy('events.archived_at', 'desc') - .limit(limit) - .offset(offset); + const archivesQuery = applyFilters( + db('events') + .select( + 'events.*', + db.raw('COUNT(DISTINCT photos.id) as photo_count'), + db.raw('SUM(photos.size_bytes) as total_size') + ) + .leftJoin('photos', 'events.id', 'photos.event_id') + .where('events.is_archived', formatBoolean(true)) + ).groupBy('events.id'); + + if (sortBy === 'name') { + archivesQuery.orderBy('events.event_name', 'asc'); + } else if (sortBy === 'size') { + // The zip's on-disk size is only known after the per-row fs.stat below, + // so a global size sort has to use the archived content size instead. + archivesQuery.orderByRaw('COALESCE(SUM(photos.size_bytes), 0) desc'); + } else { + archivesQuery.orderBy('events.archived_at', 'desc'); + } + + const archives = await archivesQuery.limit(limit).offset(offset); // Check if archive files exist and get their sizes const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); diff --git a/frontend/src/pages/admin/ArchivesPage.tsx b/frontend/src/pages/admin/ArchivesPage.tsx index 67ff5038..31352b97 100644 --- a/frontend/src/pages/admin/ArchivesPage.tsx +++ b/frontend/src/pages/admin/ArchivesPage.tsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import { Archive, Download, @@ -18,7 +18,7 @@ import { toast } from 'react-toastify'; import { Button, Input, Card, Loading } from '../../components/common'; import { PermissionGate } from '../../components/admin/PermissionGate'; import { useQuery } from '@tanstack/react-query'; -import { archiveService } from '../../services/archive.service'; +import { archiveService, type ArchiveSortBy } from '../../services/archive.service'; import { useTranslation } from 'react-i18next'; import { useLocalizedDate } from '../../hooks/useLocalizedDate'; import { useMutationWithToast } from '../../hooks'; @@ -28,11 +28,23 @@ export const ArchivesPage: React.FC = () => { const { t } = useTranslation(); const { formatTime: fmtTime } = useLocalizedDate(); const [searchTerm, setSearchTerm] = useState(''); + const [debouncedSearchTerm, setDebouncedSearchTerm] = useState(''); const [filterType, setFilterType] = useState('all'); - const [sortBy, setSortBy] = useState<'date' | 'size' | 'name'>('date'); + const [sortBy, setSortBy] = useState('date'); const [currentPage, setCurrentPage] = useState(1); // const navigate = useNavigate(); + useEffect(() => { + const t = setTimeout(() => setDebouncedSearchTerm(searchTerm.trim()), 300); + return () => clearTimeout(t); + }, [searchTerm]); + + // Reset to page 1 whenever the query changes so users don't get stuck on a + // page index that no longer exists in the new result set. + useEffect(() => { + setCurrentPage(1); + }, [debouncedSearchTerm, filterType, sortBy]); + // Helper function to safely format dates const formatDate = (dateString: string | null | undefined, formatStr: string): string => { if (!dateString) return ''; @@ -44,37 +56,18 @@ export const ArchivesPage: React.FC = () => { } }; - // Fetch archives from API + // Fetch archives from API. Search, type filter and sort are all applied + // server-side against the whole archive table — doing them in the client + // silently scoped them to the 20 rows of the current page while the + // pagination footer kept reporting the unfiltered total. const { data: archivesData, isLoading } = useQuery({ - queryKey: ['admin-archives', currentPage], - queryFn: () => archiveService.getArchives(currentPage, 20), + queryKey: ['admin-archives', currentPage, debouncedSearchTerm, filterType, sortBy], + queryFn: () => archiveService.getArchives(currentPage, 20, debouncedSearchTerm || undefined, filterType, sortBy), + placeholderData: (prev) => prev, }); const archives = archivesData?.archives || []; - const filteredArchives = archives.filter(archive => { - if (filterType !== 'all' && archive.eventType !== filterType) { - return false; - } - if (searchTerm) { - const term = searchTerm.toLowerCase(); - return archive.eventName.toLowerCase().includes(term); - } - return true; - }).sort((a, b) => { - switch (sortBy) { - case 'name': - return a.eventName.localeCompare(b.eventName); - case 'size': - return b.archiveSize - a.archiveSize; - case 'date': - default: - const dateA = a.archivedAt ? new Date(a.archivedAt).getTime() : 0; - const dateB = b.archivedAt ? new Date(b.archivedAt).getTime() : 0; - return dateB - dateA; - } - }); - const getTotalSize = () => { return archives.reduce((sum, archive) => sum + archive.archiveSize, 0); }; @@ -257,14 +250,14 @@ export const ArchivesPage: React.FC = () => { - {filteredArchives.length === 0 ? ( + {archives.length === 0 ? ( {t('archives.noArchivesFound')} ) : ( - filteredArchives.map((archive) => ( + archives.map((archive) => (
diff --git a/frontend/src/pages/admin/__tests__/archivesServerSideQuery.test.tsx b/frontend/src/pages/admin/__tests__/archivesServerSideQuery.test.tsx new file mode 100644 index 00000000..afd66dd5 --- /dev/null +++ b/frontend/src/pages/admin/__tests__/archivesServerSideQuery.test.tsx @@ -0,0 +1,136 @@ +/** + * /admin/archives search, type filter and sort were applied client-side to + * whatever 20-row page happened to be loaded (QA I.01), while the + * pagination footer kept reporting the full server-side total. An archive on + * page 7 was invisible to a search, with no hint the search was page-scoped. + * + * These pin the contract that fixes it: every control is a server query param, + * and changing any of them goes back to page 1. + */ +import React from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router-dom'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (k: string, fb?: unknown) => (typeof fb === 'string' ? fb : k), + i18n: { language: 'en' }, + }), + }; +}); + +vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), info: vi.fn() } })); + +vi.mock('../../../hooks/usePublicSettings', () => ({ + PUBLIC_SETTINGS_QUERY_KEY: ['public-settings'], + usePublicSettings: () => ({ data: {} }), +})); + +vi.mock('../../../contexts/PermissionsContext', () => ({ + usePermissions: () => ({ + hasPermission: () => true, + hasAnyPermission: () => true, + hasAllPermissions: () => true, + isSuperAdmin: true, + isLoading: false, + }), +})); + +const getArchives = vi.fn(); +vi.mock('../../../services/archive.service', () => ({ + archiveService: { + getArchives: (...args: unknown[]) => getArchives(...args), + restoreArchive: vi.fn(), + deleteArchive: vi.fn(), + downloadArchive: vi.fn(), + formatBytes: (b: number) => `${b} B`, + }, +})); + +import { ArchivesPage } from '../ArchivesPage'; + +const page = (archives: unknown[], total: number) => ({ + archives, + pagination: { page: 1, limit: 20, total, totalPages: Math.ceil(total / 20) }, +}); + +const archive = (id: number, eventName: string, eventType = 'wedding') => ({ + id, + slug: `slug-${id}`, + eventName, + eventDate: '2026-08-01', + eventType, + hostEmail: 'h@example.com', + archivedAt: '2026-08-02T10:00:00.000Z', + expiresAt: '2026-09-01T10:00:00.000Z', + photoCount: 3, + originalSize: 100, + archiveSize: 100, + archivePath: 'events/archived/x.zip', +}); + +function renderPage() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + + + ); +} + +describe('ArchivesPage server-side query (QA I.01)', () => { + beforeEach(() => { + getArchives.mockReset(); + getArchives.mockResolvedValue(page([archive(1, 'Alpha Wedding')], 802)); + }); + + it('sends the search term to the server instead of filtering the loaded page', async () => { + renderPage(); + const input = await screen.findByPlaceholderText('archives.searchPlaceholder'); + expect(getArchives).toHaveBeenLastCalledWith(1, 20, undefined, 'all', 'date'); + + await userEvent.type(input, 'bravo'); + + // Debounced — one request for the settled term, not one per keystroke. + await waitFor( + () => expect(getArchives).toHaveBeenLastCalledWith(1, 20, 'bravo', 'all', 'date'), + { timeout: 2000 } + ); + }); + + it('sends the type filter and the sort key to the server', async () => { + renderPage(); + await screen.findByDisplayValue('archives.allTypes'); + + await userEvent.selectOptions(screen.getByDisplayValue('archives.allTypes'), 'birthday'); + await waitFor(() => expect(getArchives).toHaveBeenLastCalledWith(1, 20, undefined, 'birthday', 'date')); + + await userEvent.selectOptions(screen.getByDisplayValue('archives.sortByDate'), 'size'); + await waitFor(() => expect(getArchives).toHaveBeenLastCalledWith(1, 20, undefined, 'birthday', 'size')); + }); + + it('renders exactly the rows the server returned, unfiltered by the client', async () => { + // A row the old client-side filter would have dropped: the server decided + // it matches, so the page must show it. + getArchives.mockResolvedValue(page([archive(2, 'Bravo Birthday', 'birthday')], 1)); + renderPage(); + expect(await screen.findByText('Bravo Birthday')).toBeInTheDocument(); + }); + + it('goes back to page 1 when the query changes', async () => { + renderPage(); + await userEvent.click(await screen.findByText('common.next')); + await waitFor(() => expect(getArchives).toHaveBeenLastCalledWith(2, 20, undefined, 'all', 'date')); + + await userEvent.selectOptions(screen.getByDisplayValue('archives.allTypes'), 'corporate'); + await waitFor(() => expect(getArchives).toHaveBeenLastCalledWith(1, 20, undefined, 'corporate', 'date')); + }); +}); diff --git a/frontend/src/services/archive.service.ts b/frontend/src/services/archive.service.ts index 3e6c97bd..6cd58bce 100644 --- a/frontend/src/services/archive.service.ts +++ b/frontend/src/services/archive.service.ts @@ -33,6 +33,8 @@ export interface ArchiveDetails extends Archive { }; } +export type ArchiveSortBy = 'date' | 'name' | 'size'; + export interface ArchivesResponse { archives: Archive[]; pagination: { @@ -44,10 +46,17 @@ export interface ArchivesResponse { } export const archiveService = { - // Get all archives with pagination - async getArchives(page: number = 1, limit: number = 20): Promise { + // Get all archives — pagination, search, type filter and sort are all + // resolved server-side so they apply to the whole archive table. + async getArchives( + page: number = 1, + limit: number = 20, + search?: string, + type?: string, + sortBy?: ArchiveSortBy + ): Promise { const response = await api.get('/admin/archives', { - params: { page, limit } + params: { page, limit, search: search || undefined, type: type || undefined, sortBy } }); return response.data; },