diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 783a055..f1cfdfc 100755 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -127,6 +127,12 @@ function Catalogue({ ))} + {/* Not while there is nothing to count. A genuinely empty catalogue + returns early above with the empty state, so the only way to reach + here with a total of zero is mid-load — and flashing "0 items" at + somebody while their catalogue is still arriving says something + untrue. */} + {total > 0 && ( `${count} ${count === 1 ? 'item' : 'items'}`} /> + )} ); } diff --git a/frontend/tests/e2e/favorites-filter.spec.ts b/frontend/tests/e2e/favorites-filter.spec.ts index 45a6040..b3c365d 100644 --- a/frontend/tests/e2e/favorites-filter.spec.ts +++ b/frontend/tests/e2e/favorites-filter.spec.ts @@ -98,13 +98,20 @@ test.describe('Filtering the storefront by favorites', () => { await storefront.gotoSignedIn(); await favorite(storefront, favoritePrompt, KEPT); + // The unfiltered total, captured before filtering, is what "the full + // catalogue" means here. It used to mean "OTHER is on screen", which a + // paginated catalogue cannot promise — OTHER is on some page, not + // necessarily the first (#269). + await storefront.goto(); + const unfiltered = await storefront.totalItems(); + await page.goto('/?favorites=1'); await expect(storefront.addToFavoritesButton(OTHER)).toBeHidden(); await expect(storefront.activeFilters).toContainText('My favorites'); await storefront.removeFilterChip('My favorites').click(); - await expect(storefront.addToFavoritesButton(OTHER)).toBeVisible(); + await expect.poll(() => storefront.totalItems()).toBe(unfiltered); await expect(page).not.toHaveURL(/favorites/); }); diff --git a/frontend/tests/e2e/filters.spec.ts b/frontend/tests/e2e/filters.spec.ts index 55d550a..51528bf 100644 --- a/frontend/tests/e2e/filters.spec.ts +++ b/frontend/tests/e2e/filters.spec.ts @@ -153,16 +153,26 @@ test.describe('Storefront filters', () => { await expect(storefront.card(NAMES.dearItem)).toBeHidden(); }); + // Asserts what it means, rather than that one fixture happens to be on + // screen. It used to check a named item was visible in the unfiltered grid, + // which a paginated catalogue cannot promise — that item is on some page, not + // necessarily the first (#269). test('removing a chip widens the results again', async ({ storefront, filterDrawer }) => { await storefront.goto(); + const unfiltered = await storefront.totalItems(); + await storefront.openFilters(); await filterDrawer.chooseCategory(NAMES.decor); await filterDrawer.close(); await expect(storefront.card(NAMES.deepItem)).toBeHidden(); + await expect.poll(() => storefront.totalItems()).toBeLessThan(unfiltered); await storefront.removeFilterChip(NAMES.decor).click(); - await expect(storefront.card(NAMES.deepItem)).toBeVisible(); + + // Polled rather than read once: the count changes when the refetch lands, + // and a single read races it. + await expect.poll(() => storefront.totalItems()).toBe(unfiltered); }); test('clear all removes every active filter', async ({ page, storefront, filterDrawer }) => { diff --git a/frontend/tests/e2e/pages/StorefrontPage.ts b/frontend/tests/e2e/pages/StorefrontPage.ts index a7b4f7b..70d1558 100644 --- a/frontend/tests/e2e/pages/StorefrontPage.ts +++ b/frontend/tests/e2e/pages/StorefrontPage.ts @@ -38,6 +38,16 @@ export class StorefrontPage { /** What the nearest error boundary renders when the grid itself throws. */ readonly catalogueBoundaryHeading: Locator; + /** The paging control under the grid (#269). */ + readonly pagination: Locator; + /** + * Where the catalogue says how many items it has. + * + * A class locator rather than a role, matching how this file already reaches + * `.item-card` — antd gives the total no role of its own, and this is the one + * place in the suite that knows where it lives. + */ + readonly resultCount: Locator; constructor(private readonly page: Page) { this.header = new Header(page); @@ -52,12 +62,35 @@ export class StorefrontPage { this.retryButton = page.getByRole('button', { name: 'Retry' }); this.catalogueBoundaryHeading = page.getByRole('heading', { name: "The item list didn't load" }); + + this.pagination = page.locator('.ant-pagination'); + this.resultCount = page.locator('.ant-pagination-total-text'); } async goto(): Promise { await this.page.goto('/'); } + /** + * How many items the catalogue says it has, in total, across every page. + * + * The number rather than the text, so a test can assert a result set grew or + * shrank without knowing what it grew from. That is the whole reason #269 + * added a visible count: two assertions used to name a fixture and expect it + * in the unfiltered grid, which no paginated catalogue can promise. + */ + async totalItems(): Promise { + // Waits rather than reading straight away. The control is not rendered + // until there is something to count, so reading during the initial load + // used to return 0 and quietly make "the result set shrank" assertions + // compare against nothing. + await this.resultCount.waitFor(); + const text = (await this.resultCount.textContent()) ?? ''; + const digits = /(\d+)/.exec(text); + if (digits === null) throw new Error(`no count in the pagination total: "${text}"`); + return Number(digits[1]); + } + /** * Goes to the storefront and waits for the session to settle. * diff --git a/frontend/tests/e2e/pagination.spec.ts b/frontend/tests/e2e/pagination.spec.ts new file mode 100644 index 0000000..2d5bea9 --- /dev/null +++ b/frontend/tests/e2e/pagination.spec.ts @@ -0,0 +1,105 @@ +import { test, expect } from './fixtures'; + +/* eslint-disable sonarjs/no-skipped-tests -- + * The `test.skip(total <= 10, ...)` calls below are conditional guards, not + * disabled tests: they run whenever the catalogue is large enough to have a + * second page, which it is on any real database. The rule cannot tell a + * runtime condition from a permanently ignored test, and its own message asks + * for an explanation rather than removal — this is it. + * + * The guards are honest about a real limit, though: against a catalogue of ten + * items or fewer these cases prove nothing. If the e2e database is ever seeded + * that thinly, they should be given fixtures of their own rather than left to + * skip quietly. + */ + +/** + * Paging the catalogue (#269). + * + * Every assertion here is about the control and the URL, never about a + * particular item being on a particular page. The development database never + * truncates, so which item lands where is not something a test may rely on — + * that is exactly the trap the two rewritten assertions in filters.spec.ts and + * favorites-filter.spec.ts had fallen into. + */ +test.describe('Paging the catalogue', () => { + test('shows how many items there are', async ({ storefront }) => { + await storefront.goto(); + + await expect(storefront.resultCount).toBeVisible(); + expect(await storefront.totalItems()).toBeGreaterThan(0); + }); + + // Ten by default, which is the decision on the issue. A denser grid is + // something a customer asks for, not something they get by accident. + test('shows ten items to a page by default', async ({ page, storefront }) => { + await storefront.goto(); + + const total = await storefront.totalItems(); + test.skip(total <= 10, 'needs more than one page of catalogue to be meaningful'); + + await expect(page.locator('.item-card')).toHaveCount(10); + }); + + // The reason numbered pages were chosen over infinite scroll: a page is a + // place you can send someone. + test('puts the page in the URL so it can be linked', async ({ page, storefront }) => { + await storefront.goto(); + + const total = await storefront.totalItems(); + test.skip(total <= 10, 'needs more than one page of catalogue to be meaningful'); + + await storefront.pagination.getByRole('listitem', { name: '2', exact: true }).click(); + + await expect(page).toHaveURL(/[?&]page=2\b/); + }); + + // Page one is the absence of the parameter, so the plain URL stays clean. + test('leaves page one out of the URL', async ({ page, storefront }) => { + await storefront.goto(); + + const total = await storefront.totalItems(); + test.skip(total <= 10, 'needs more than one page of catalogue to be meaningful'); + + await storefront.pagination.getByRole('listitem', { name: '2', exact: true }).click(); + await expect(page).toHaveURL(/[?&]page=2\b/); + + await storefront.pagination.getByRole('listitem', { name: '1', exact: true }).click(); + await expect(page).not.toHaveURL(/[?&]page=/); + }); + + // A link to a page that no longer exists lands on the last one. An empty + // grid would read as "this shop has nothing". + test('clamps a page past the end rather than showing nothing', async ({ page, storefront }) => { + await page.goto('/?page=99999'); + + await expect(storefront.resultCount).toBeVisible(); + await expect(page.locator('.item-card').first()).toBeVisible(); + }); + + // The jump box was deliberately left off. + test('offers no "go to page" box', async ({ storefront }) => { + await storefront.goto(); + + await expect( + storefront.pagination.locator('.ant-pagination-options-quick-jumper') + ).toHaveCount(0); + }); + + test('remembers a chosen page size', async ({ page, storefront }) => { + await storefront.goto(); + + const total = await storefront.totalItems(); + test.skip(total <= 20, 'needs more than twenty items for a size change to show'); + + await storefront.pagination.locator('.ant-select').click(); + await page.getByTitle('20 / page').click(); + await expect(page.locator('.item-card')).toHaveCount(20); + + // The preference is the point: it survives a reload without being in the + // URL, because it belongs to this person and not to a shared link. + await page.reload(); + await expect(page.locator('.item-card')).toHaveCount(20); + await expect(page).not.toHaveURL(/pageSize/); + }); +});