Skip to content

Commit f5eedc0

Browse files
LancelcodeLancelcode
committed
refactor: simplify shortlist logic by removing unnecessary readiness checks in components and tests
1 parent bfbc71e commit f5eedc0

5 files changed

Lines changed: 49 additions & 121 deletions

File tree

‎src/components/NurseryActions.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ interface Props {
1515
*/
1616
export function NurseryActions({ nursery }: Props) {
1717
const [open, setOpen] = useState(false);
18-
const { has, toggle, ready } = useShortlist();
19-
const saved = ready && has(nursery.id);
18+
const { has, toggle } = useShortlist();
19+
const saved = has(nursery.id);
2020

2121
return (
2222
<>
@@ -93,4 +93,4 @@ export function NurseryActions({ nursery }: Props) {
9393
</div>
9494
</>
9595
);
96-
}
96+
}

‎src/components/NurseryCard.tsx‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,11 @@ function ofstedBadgeType(rating: OfstedRating): BadgeType {
6060
}
6161

6262
export function NurseryCard({ nursery, onEnquire }: Props) {
63-
const { has, toggle, ready } = useShortlist();
64-
const saved = ready && has(nursery.id);
63+
const { has, toggle } = useShortlist();
64+
// Don't gate on `ready`. ids starts as [] so has() returns false initially
65+
// anyway. Gating on ready means a click before the first useEffect fires
66+
// leaves aria-pressed stuck on false even after the id is added.
67+
const saved = has(nursery.id);
6568
const slug = slugify(nursery.name);
6669

6770
return (
@@ -265,4 +268,4 @@ export function NurseryCard({ nursery, onEnquire }: Props) {
265268
</div>
266269
</article>
267270
);
268-
}
271+
}

‎tests/enquiry.spec.ts‎

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,8 @@ import { test, expect } from "@playwright/test";
33
/**
44
* Enquiry flow tests.
55
*
6-
* We intercept POST /api/enquiry and return a mock 200 response so that:
7-
* - No Resend emails are sent during CI
8-
* - Tests are deterministic (no network variability from an email provider)
9-
* - The free Resend allowance is not consumed by automated test runs
10-
*
11-
* The intercepted handler validates that the request body is well-formed,
12-
* so we are still testing that the form sends the right payload.
6+
* POST /api/enquiry is intercepted so no Resend emails are sent in CI.
7+
* The interceptor still validates the request shape, so we test the real form.
138
*/
149

1510
const TEST_EMAIL = "djibysowrebollo@gmail.com";
@@ -20,25 +15,14 @@ const START_DATE = "2025-01-20";
2015

2116
async function mockEnquiryRoute(page: import("@playwright/test").Page) {
2217
await page.route("**/api/enquiry", async (route) => {
23-
const request = route.request();
24-
const body = request.postDataJSON() as Record<string, unknown>;
18+
const body = route.request().postDataJSON() as Record<string, unknown>;
2519
const required = ["nurseryName", "nurseryArea", "name", "email", "phone", "childDob", "startDate"];
2620
const missing = required.filter((k) => !body[k]);
27-
2821
if (missing.length > 0) {
29-
await route.fulfill({
30-
status: 400,
31-
contentType: "application/json",
32-
body: JSON.stringify({ error: `Missing: ${missing.join(", ")}` }),
33-
});
22+
await route.fulfill({ status: 400, contentType: "application/json", body: JSON.stringify({ error: `Missing: ${missing.join(", ")}` }) });
3423
return;
3524
}
36-
37-
await route.fulfill({
38-
status: 200,
39-
contentType: "application/json",
40-
body: JSON.stringify({ success: true }),
41-
});
25+
await route.fulfill({ status: 200, contentType: "application/json", body: JSON.stringify({ success: true }) });
4226
});
4327
}
4428

@@ -47,12 +31,10 @@ async function fillEnquiryForm(page: import("@playwright/test").Page) {
4731
await page.getByPlaceholder("07700 900000").fill(TEST_PHONE);
4832
await page.getByPlaceholder("you@example.com").fill(TEST_EMAIL);
4933
await page.getByRole("button", { name: "Continue" }).click();
50-
5134
await page.locator('input[type="date"]').first().fill(CHILD_DOB);
5235
await page.locator('input[type="date"]').last().fill(START_DATE);
5336
await page.getByPlaceholder("Any questions or specific requirements...").fill(TEST_MESSAGE);
5437
await page.getByRole("button", { name: "Send enquiry" }).click();
55-
5638
await expect(page.getByText("Enquiry sent")).toBeVisible({ timeout: 10000 });
5739
}
5840

@@ -66,15 +48,13 @@ test.describe("Enquiry flow", () => {
6648
test("enquiry from list view", async ({ page }) => {
6749
await page.getByRole("button", { name: "Enquire" }).first().click();
6850
await expect(page.getByText("Enquire at")).toBeVisible();
69-
7051
await fillEnquiryForm(page);
71-
7252
await page.getByRole("button", { name: "Done" }).click();
7353
await expect(page.locator("text=Meadowside Nursery")).toBeVisible();
7454
});
7555

7656
test("enquiry from map view", async ({ page }) => {
77-
// ViewToggle buttons have role="tab" — use getByRole("tab") not "button"
57+
// ViewToggle buttons have role="tab" — must use getByRole("tab")
7858
await page.getByRole("tab", { name: "Map" }).click();
7959
await page.waitForTimeout(2000);
8060

@@ -84,31 +64,26 @@ test.describe("Enquiry flow", () => {
8464

8565
await page.locator(".leaflet-popup button", { hasText: "Enquire" }).click();
8666
await expect(page.getByText("Enquire at")).toBeVisible();
87-
8867
await fillEnquiryForm(page);
8968
await page.getByRole("button", { name: "Done" }).click();
9069
});
9170

9271
test("validation rejects an empty name", async ({ page }) => {
9372
await page.getByRole("button", { name: "Enquire" }).first().click();
9473
await expect(page.getByText("Enquire at")).toBeVisible();
95-
9674
await page.getByPlaceholder("07700 900000").fill(TEST_PHONE);
9775
await page.getByPlaceholder("you@example.com").fill(TEST_EMAIL);
9876
await page.getByRole("button", { name: "Continue" }).click();
99-
10077
await expect(page.getByText("Name is required")).toBeVisible();
10178
await expect(page.getByText("Child date of birth")).not.toBeVisible();
10279
});
10380

10481
test("validation rejects a malformed email", async ({ page }) => {
10582
await page.getByRole("button", { name: "Enquire" }).first().click();
106-
10783
await page.getByPlaceholder("Your name").fill("Test User");
10884
await page.getByPlaceholder("07700 900000").fill(TEST_PHONE);
10985
await page.getByPlaceholder("you@example.com").fill("not-an-email");
11086
await page.getByRole("button", { name: "Continue" }).click();
111-
11287
await expect(page.getByText("Enter a valid email")).toBeVisible();
11388
});
11489

‎tests/seo.spec.ts‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ test.describe("SEO", () => {
2424
test("home page has canonical link, OG tags, and JSON-LD", async ({ page }) => {
2525
await page.goto("http://localhost:3000");
2626

27-
// Canonical — Next.js renders the full URL without trailing slash e.g.
28-
// "https://nvvri.co.uk". Accept any value that contains the domain.
27+
// Canonical — Next.js renders the full URL without a trailing slash.
28+
// Check that it contains the domain rather than matching an exact pattern.
2929
const canonical = page.locator('link[rel="canonical"]');
3030
await expect(canonical).toHaveAttribute("href", /nvvri/);
3131

@@ -46,10 +46,7 @@ test.describe("SEO", () => {
4646

4747
test("nursery detail page has Preschool JSON-LD", async ({ page }) => {
4848
await page.goto("http://localhost:3000/nursery/meadowside-nursery");
49-
50-
await expect(
51-
page.getByRole("heading", { name: "Meadowside Nursery" })
52-
).toBeVisible({ timeout: 10000 });
49+
await expect(page.getByRole("heading", { name: "Meadowside Nursery" })).toBeVisible({ timeout: 10000 });
5350

5451
const jsonLd = await page
5552
.locator('script[type="application/ld+json"]')
@@ -66,16 +63,13 @@ test.describe("SEO", () => {
6663

6764
test("unknown nursery slug shows not-found page", async ({ page }) => {
6865
// In dev mode Next.js may return 200 for notFound() pages.
69-
// Just verify the correct content is shown regardless of status code.
66+
// Check the content only, not the status code.
7067
await page.goto("http://localhost:3000/nursery/does-not-exist");
71-
await expect(page.getByText("Nursery not found")).toBeVisible({
72-
timeout: 10000,
73-
});
68+
await expect(page.getByText("Nursery not found")).toBeVisible({ timeout: 10000 });
7469
});
7570

7671
test("admin route is hidden from unauthenticated visitors", async ({ request }) => {
7772
const res = await request.get("http://localhost:3000/admin/searches");
78-
// Middleware returns 404 to hide the route
7973
expect(res.status()).toBe(404);
8074
});
8175
});

‎tests/shortlist.spec.ts‎

Lines changed: 29 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -3,115 +3,71 @@ import { test, expect } from "@playwright/test";
33
/**
44
* Shortlist tests.
55
*
6-
* After clicking a heart button the aria-label changes from
7-
* "Add X to shortlist" to "Remove X from shortlist".
8-
* We look for the Remove label to confirm the save worked rather than
9-
* checking aria-pressed on the original locator (which no longer matches
10-
* after the label changes).
6+
* After clicking a heart the aria-label changes from "Add X to shortlist"
7+
* to "Remove X from shortlist". We confirm the save by looking for the
8+
* Remove-labelled button rather than checking aria-pressed on a locator
9+
* that no longer matches after the label changes.
1110
*/
1211
test.describe("Shortlist", () => {
1312
test.beforeEach(async ({ page }) => {
1413
await page.goto("http://localhost:3000");
15-
await expect(page.locator("text=Meadowside Nursery")).toBeVisible({
16-
timeout: 10000,
17-
});
14+
await expect(page.locator("text=Meadowside Nursery")).toBeVisible({ timeout: 10000 });
1815
});
1916

2017
test("save and unsave a nursery from a card", async ({ page }) => {
21-
// Click the first heart
22-
const addButton = page
23-
.getByRole("button", { name: /Add .* to shortlist/ })
24-
.first();
25-
await addButton.click();
18+
// Save the first nursery
19+
await page.getByRole("button", { name: /Add .* to shortlist/ }).first().click();
2620

27-
// After saving the label changes to "Remove" — look for that
28-
const removeButton = page
29-
.getByRole("button", { name: /Remove .* from shortlist/ })
30-
.first();
21+
// The label changes to Remove after saving — confirm that
22+
const removeButton = page.getByRole("button", { name: /Remove .* from shortlist/ }).first();
3123
await expect(removeButton).toBeVisible({ timeout: 5000 });
3224
await expect(removeButton).toHaveAttribute("aria-pressed", "true");
3325

3426
// Nav counter should appear
35-
await expect(
36-
page.getByRole("link", { name: /View shortlist/ })
37-
).toBeVisible({ timeout: 5000 });
27+
await expect(page.getByRole("link", { name: /View shortlist/ })).toBeVisible({ timeout: 5000 });
3828

3929
// Unsave it
4030
await removeButton.click();
41-
// Button should flip back to "Add"
42-
await expect(
43-
page.getByRole("button", { name: /Add .* to shortlist/ }).first()
44-
).toBeVisible({ timeout: 5000 });
31+
await expect(page.getByRole("button", { name: /Add .* to shortlist/ }).first()).toBeVisible({ timeout: 5000 });
4532
});
4633

4734
test("shortlist page shows saved nurseries and compare view", async ({ page }) => {
48-
// Save first two nurseries
49-
const hearts = page.getByRole("button", { name: /Add .* to shortlist/ });
50-
await hearts.nth(0).click();
51-
// Wait for first to be saved before clicking second
52-
await expect(
53-
page.getByRole("button", { name: /Remove .* from shortlist/ }).first()
54-
).toBeVisible({ timeout: 5000 });
35+
// Save two nurseries
36+
const allAddButtons = page.getByRole("button", { name: /Add .* to shortlist/ });
5537

56-
await hearts.first().click(); // re-click Add on the next un-saved card
57-
// Actually nth(0) is now Remove, so click the new first Add
58-
const secondAdd = page
59-
.getByRole("button", { name: /Add .* to shortlist/ })
60-
.first();
61-
await secondAdd.click();
62-
await expect(
63-
page.getByRole("button", { name: /Remove .* from shortlist/ })
64-
).toHaveCount(2, { timeout: 5000 });
38+
await allAddButtons.first().click();
39+
await expect(page.getByRole("button", { name: /Remove .* from shortlist/ })).toHaveCount(1, { timeout: 5000 });
40+
41+
await allAddButtons.first().click();
42+
await expect(page.getByRole("button", { name: /Remove .* from shortlist/ })).toHaveCount(2, { timeout: 5000 });
6543

6644
// Navigate to shortlist
6745
await page.goto("http://localhost:3000/shortlist");
68-
await expect(
69-
page.getByRole("heading", { name: "Your shortlist" })
70-
).toBeVisible({ timeout: 10000 });
46+
await expect(page.getByRole("heading", { name: "Your shortlist" })).toBeVisible({ timeout: 10000 });
7147

72-
// Should see the compare tab (only shown when 2+ nurseries saved)
48+
// Compare tab only shows when 2+ nurseries are saved
7349
const compareTab = page.getByRole("tab", { name: "Compare" });
7450
await expect(compareTab).toBeVisible({ timeout: 5000 });
7551
await compareTab.click();
7652

77-
// Compare table row labels
78-
await expect(
79-
page.getByRole("rowheader", { name: "Daily fee" })
80-
).toBeVisible();
81-
await expect(
82-
page.getByRole("rowheader", { name: "Ofsted" })
83-
).toBeVisible();
53+
await expect(page.getByRole("rowheader", { name: "Daily fee" })).toBeVisible();
54+
await expect(page.getByRole("rowheader", { name: "Ofsted" })).toBeVisible();
8455
});
8556

8657
test("shortlist persists across page reloads", async ({ page }) => {
87-
const addButton = page
88-
.getByRole("button", { name: /Add .* to shortlist/ })
89-
.first();
90-
await addButton.click();
91-
92-
// Confirm saved
93-
await expect(
94-
page.getByRole("button", { name: /Remove .* from shortlist/ }).first()
95-
).toBeVisible({ timeout: 5000 });
58+
await page.getByRole("button", { name: /Add .* to shortlist/ }).first().click();
59+
await expect(page.getByRole("button", { name: /Remove .* from shortlist/ }).first()).toBeVisible({ timeout: 5000 });
9660

9761
await page.reload();
98-
await expect(page.locator("text=Meadowside Nursery")).toBeVisible({
99-
timeout: 10000,
100-
});
62+
await expect(page.locator("text=Meadowside Nursery")).toBeVisible({ timeout: 10000 });
10163

102-
// After reload the saved nursery should still show Remove
103-
await expect(
104-
page.getByRole("button", { name: /Remove .* from shortlist/ }).first()
105-
).toBeVisible({ timeout: 5000 });
64+
// Should still show as saved after reload
65+
await expect(page.getByRole("button", { name: /Remove .* from shortlist/ }).first()).toBeVisible({ timeout: 5000 });
10666
});
10767

10868
test("empty shortlist shows guidance", async ({ page }) => {
10969
await page.goto("http://localhost:3000/shortlist");
110-
await expect(page.getByText("Nothing saved yet")).toBeVisible({
111-
timeout: 10000,
112-
});
113-
await expect(
114-
page.getByRole("link", { name: "Browse nurseries" })
115-
).toBeVisible();
70+
await expect(page.getByText("Nothing saved yet")).toBeVisible({ timeout: 10000 });
71+
await expect(page.getByRole("link", { name: "Browse nurseries" })).toBeVisible();
11672
});
11773
});

0 commit comments

Comments
 (0)