-
Notifications
You must be signed in to change notification settings - Fork 0
Fix promo code spinner stuck when applied before selecting a ticket #151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
069ed35
Rework promo status into canonical signals plus display projection
gcutrini 240e2bf
Distinguish applied from verified promo codes
gcutrini 19e2774
Ignore promo validations superseded by a ticket switch
gcutrini 1e83046
Restore the advance verdict from onRevalidate
gcutrini 2463c2e
Ignore Playwright output directories
gcutrini 34198e0
Let the promo code hook own its validation in-flight flag
gcutrini be094db
Let the user retry a promo code validation that failed
gcutrini 1dc156d
Wait for the error modal in the promo code spec instead of sampling f…
gcutrini 521ac07
Report auto-apply failure from tryAutoApply
gcutrini 9ad75df
Stop exposing isDiscoveredCode from usePromoCode
gcutrini 5e7b8c8
Stop the promo code apply flag hanging on an aborted request
gcutrini a11fee5
Let the promo code hook own the validation verdict
gcutrini 9fbc16b
Abandon a promo code validation when its code stops being applied
gcutrini 391bad0
Record which promo code each validation answered
gcutrini 7a00e91
Keep a code auto-applied when validation fails without deciding
gcutrini ae8b6af
Stop applying promo code quantity caps after an undecided validation
gcutrini File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,4 +5,6 @@ node_modules | |
| coverage | ||
| *.log | ||
| .idea/ | ||
| .env | ||
| .env | ||
| test-results | ||
| playwright-report | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| const { test, expect } = require('@playwright/test'); | ||
| const { | ||
| ticketType, | ||
| discoveryResponse, | ||
| ticketTypesResponse, | ||
| taxTypesResponse, | ||
| validationResponse, | ||
| } = require('./fixtures'); | ||
|
|
||
| // Advancing past the ticket step re-validates an applied manual code and gates | ||
| // changeStep on the result (registration-form's handleAdvanceFromTicketStep). | ||
| // Nothing else covers that path: the other Next-clicking spec types a code | ||
| // without applying it, so it exits on the unapplied-code warning first. | ||
|
|
||
| // Routes everything the ticket step needs, letting the caller decide how each | ||
| // successive validation call responds. | ||
| const setup = async (page, validationResponses) => { | ||
| let call = 0; | ||
| await page.route('**/promo-codes/all/discover*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(discoveryResponse([])) }) | ||
| ); | ||
| await page.route('**/ticket-types/allowed*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(ticketTypesResponse([ticketType()])) }) | ||
| ); | ||
| await page.route('**/tax-types*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(taxTypesResponse()) }) | ||
| ); | ||
| await page.route('**/promo-codes/*/apply*', route => { | ||
| // Last entry repeats, so a single-entry list means "always this". | ||
| const next = validationResponses[Math.min(call++, validationResponses.length - 1)]; | ||
| route.fulfill({ | ||
| status: next.status, | ||
| contentType: 'application/json', | ||
| body: JSON.stringify(next.body ?? validationResponse()), | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| // uicore raises its own modal for statuses it does not recognise. It overlays | ||
| // the form and swallows clicks, so wait for it and clear it the way a user | ||
| // would before touching anything underneath. Waiting rather than sampling | ||
| // matters: the modal renders a beat after the response lands, and an | ||
| // instantaneous check can miss it and leave it covering the next click. | ||
| const dismissServerErrorModal = async (page) => { | ||
| const confirm = page.locator('.swal2-confirm'); | ||
| await confirm.waitFor({ state: 'visible' }); | ||
| await confirm.click(); | ||
| await expect(page.locator('.swal2-container')).toHaveCount(0); | ||
| }; | ||
|
|
||
| const applyCode = async (page, code) => { | ||
| await page.locator('[data-testid="ticket-dropdown"]').click(); | ||
| await page.locator('[data-testid="ticket-list"] >> text=Early Bird Ticket').click(); | ||
| await page.fill('input[placeholder="Enter your promo code"]', code); | ||
| await page.click('button:has-text("Apply")'); | ||
| }; | ||
|
|
||
| test('Next retries a validation that failed transiently, and advances once it succeeds', async ({ page }) => { | ||
| // A server error decides nothing about the code, so it must not strand the | ||
| // user: pressing Next again has to re-run the validation rather than sit on | ||
| // a dead button. | ||
| await setup(page, [{ status: 500, body: { message: 'Server error' } }, { status: 200 }]); | ||
| await page.goto('/'); | ||
|
|
||
| await applyCode(page, 'EARLYCODE'); | ||
| await dismissServerErrorModal(page); | ||
|
|
||
| const next = page.locator('button:has-text("Next")'); | ||
| await expect(next).toBeEnabled(); | ||
| await next.click(); | ||
|
|
||
| await expect(page.locator('button:has-text("Back")')).toBeVisible(); | ||
| await expect(page.locator('text=* Required fields')).toBeVisible(); | ||
| }); | ||
|
|
||
| test('Next does not advance while validation keeps failing', async ({ page }) => { | ||
| // The retry must not become a way through on an unverified code. | ||
| await setup(page, [{ status: 500, body: { message: 'Server error' } }]); | ||
| await page.goto('/'); | ||
|
|
||
| await applyCode(page, 'EARLYCODE'); | ||
| await dismissServerErrorModal(page); | ||
|
|
||
| await page.locator('button:has-text("Next")').click(); | ||
| await dismissServerErrorModal(page); | ||
|
|
||
| // Still on the ticket step: neither of these renders until it is left. | ||
| await expect(page.locator('button:has-text("Back")')).toHaveCount(0); | ||
| await expect(page.locator('text=* Required fields')).toHaveCount(0); | ||
| }); | ||
|
|
||
| test('Next advances to personal information with an applied promo code', async ({ page }) => { | ||
| await page.route('**/promo-codes/all/discover*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(discoveryResponse([])) }) | ||
| ); | ||
| await page.route('**/ticket-types/allowed*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(ticketTypesResponse([ticketType()])) }) | ||
| ); | ||
| await page.route('**/tax-types*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(taxTypesResponse()) }) | ||
| ); | ||
| await page.route('**/promo-codes/*/apply*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(validationResponse()) }) | ||
| ); | ||
|
|
||
| await page.goto('/'); | ||
|
|
||
| await page.locator('[data-testid="ticket-dropdown"]').click(); | ||
| await page.locator('[data-testid="ticket-list"] >> text=Early Bird Ticket').click(); | ||
|
|
||
| await page.fill('input[placeholder="Enter your promo code"]', 'EARLYCODE'); | ||
| await page.click('button:has-text("Apply")'); | ||
| await expect(page.getByTestId('promo-applied')).toBeVisible(); | ||
|
|
||
| await page.click('button:has-text("Next")'); | ||
|
|
||
| // Both only render once the ticket step has been left (button-bar gates the | ||
| // Back button and the required-fields note on step !== STEP_SELECT_TICKET_TYPE). | ||
| // Purchaser Information is not a usable signal here: it is on screen during | ||
| // the ticket step too. | ||
| await expect(page.locator('button:has-text("Back")')).toBeVisible(); | ||
| await expect(page.locator('text=* Required fields')).toBeVisible(); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| const { test, expect } = require('@playwright/test'); | ||
| const { | ||
| ticketType, | ||
| discoveryResponse, | ||
| ticketTypesResponse, | ||
| taxTypesResponse, | ||
| validationResponse, | ||
| } = require('./fixtures'); | ||
|
|
||
| // ── Helpers ── | ||
|
|
||
| const setupRoutes = async (page, { discovery = [], tickets = [], taxes = [], validation = null } = {}) => { | ||
| await page.route('**/promo-codes/all/discover*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(discoveryResponse(discovery)) }) | ||
| ); | ||
|
|
||
| await page.route('**/ticket-types/allowed*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(ticketTypesResponse(tickets)) }) | ||
| ); | ||
|
|
||
| await page.route('**/tax-types*', route => | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(taxTypesResponse(taxes)) }) | ||
| ); | ||
|
|
||
| if (validation) { | ||
| await page.route('**/promo-codes/*/apply*', route => | ||
| route.fulfill({ | ||
| status: validation.status || 200, | ||
| contentType: 'application/json', | ||
| body: JSON.stringify(validation.body || validationResponse()), | ||
| }) | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const selectTicket = async (page, ticketName) => { | ||
| await page.locator('[data-testid="ticket-dropdown"]').click(); | ||
| await page.locator(`[data-testid="ticket-list"] >> text=${ticketName}`).click(); | ||
| }; | ||
|
|
||
| // ── Apply before selecting a ticket ── | ||
|
|
||
| // Two ticket types so the post-apply single-ticket auto-select does not kick | ||
| // in — the applied code genuinely rests with no ticket picked. | ||
| const twoTickets = [ | ||
| ticketType(), | ||
| ticketType({ id: 189, name: 'General Admission', cost: 900 }), | ||
| ]; | ||
|
|
||
| const promoSpinner = (page) => page.getByTestId('promo-spinner'); | ||
| const promoApplied = (page) => page.getByTestId('promo-applied'); | ||
|
|
||
| test.describe('apply promo code before selecting a ticket', () => { | ||
| test('settles instead of spinning forever', async ({ page }) => { | ||
| await setupRoutes(page, { | ||
| tickets: twoTickets, | ||
| discovery: [], | ||
| validation: { status: 200, body: validationResponse() }, | ||
| }); | ||
| await page.goto('/'); | ||
|
|
||
| // Apply a code with NO ticket selected | ||
| await page.fill('input[placeholder="Enter your promo code"]', 'EARLYCODE'); | ||
| await page.click('button:has-text("Apply")'); | ||
|
|
||
| // Input locks with a Remove affordance | ||
| await expect(page.locator('input[placeholder="Enter your promo code"][readonly]')).toBeVisible(); | ||
| await expect(page.locator('button:has-text("Remove")')).toBeVisible(); | ||
|
|
||
| // No spinner. No success mark either: nothing has verified the code. | ||
| await expect(promoSpinner(page)).toHaveCount(0); | ||
| await expect(promoApplied(page)).toHaveCount(0); | ||
| }); | ||
|
|
||
| test('validates the code once a ticket is picked afterwards', async ({ page }) => { | ||
| let validationCalls = 0; | ||
| await setupRoutes(page, { | ||
| tickets: twoTickets, | ||
| discovery: [], | ||
| }); | ||
| await page.route('**/promo-codes/*/apply*', route => { | ||
| validationCalls += 1; | ||
| route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(validationResponse()) }); | ||
| }); | ||
| await page.goto('/'); | ||
|
|
||
| await page.fill('input[placeholder="Enter your promo code"]', 'EARLYCODE'); | ||
| await page.click('button:has-text("Apply")'); | ||
| await expect(page.locator('button:has-text("Remove")')).toBeVisible(); | ||
| await expect(promoApplied(page)).toHaveCount(0); | ||
| expect(validationCalls).toBe(0); | ||
|
|
||
| // Picking a ticket triggers the deferred promo+ticket validation, | ||
| // which is what promotes the code to verified. | ||
| await selectTicket(page, 'General Admission'); | ||
| await expect.poll(() => validationCalls).toBeGreaterThan(0); | ||
| await expect(promoApplied(page)).toBeVisible(); | ||
| await expect(promoSpinner(page)).toHaveCount(0); | ||
| }); | ||
|
|
||
| test('shows the spinner only while validation is in flight', async ({ page }) => { | ||
| // The other assertions in this file check the spinner is absent, which | ||
| // would also pass if the locator stopped matching anything. Holding the | ||
| // validation open pins the spinner with a positive assertion, so a | ||
| // renamed or removed icon fails here instead of passing silently. | ||
| await setupRoutes(page, { tickets: twoTickets, discovery: [] }); | ||
|
|
||
| // Slow the validation enough to observe the in-flight state. | ||
| await page.route('**/promo-codes/*/apply*', async (route) => { | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)); | ||
| await route.fulfill({ | ||
| status: 200, | ||
| contentType: 'application/json', | ||
| body: JSON.stringify(validationResponse()), | ||
| }); | ||
| }); | ||
|
|
||
| await page.goto('/'); | ||
| await page.fill('input[placeholder="Enter your promo code"]', 'EARLYCODE'); | ||
| await page.click('button:has-text("Apply")'); | ||
|
|
||
| // Resting unverified: no validation has been requested yet. | ||
| await expect(page.locator('button:has-text("Remove")')).toBeVisible(); | ||
| await expect(promoSpinner(page)).toHaveCount(0); | ||
|
|
||
| // Selecting a ticket starts the validation, which is now slow enough | ||
| // to catch mid-flight. | ||
| await selectTicket(page, 'General Admission'); | ||
| await expect(promoSpinner(page)).toBeVisible(); | ||
| await expect(promoApplied(page)).toHaveCount(0); | ||
|
|
||
| // And it resolves to verified once the request lands. | ||
| await expect(promoApplied(page)).toBeVisible(); | ||
| await expect(promoSpinner(page)).toHaveCount(0); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert exactly one deferred validation request.
Line 94 accepts duplicate validation requests. The test title requires one request after ticket selection. Change the assertion to
toBe(1).Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gcutrini Not changing this — the suggested assertion doesn't do what it claims.
expect.pollre-evaluates until the assertion passes and then returns, sotoBe(1)stops the moment the counter hits 1 and is just as blind to a later duplicate astoBeGreaterThan(0). Asserting "exactly one" would need a settle-then-check (e.g. await the ✓, then a bareexpect(validationCalls).toBe(1)outsidepoll).The count is also one by construction: discovery is stubbed empty, so
discoveredPromoCodeis null andonTicketSelectedtakes theisApplied && !isDiscoveredCodebranch atsrc/hooks/usePromoCode.js:191-194, which callsonRevalidateonce and returns. The ticket-sync effect atsrc/components/ticket-type/index.js:108-115takes theupdatedCurrentTicketpath once a ticket is set and never revalidates.This thread can be resolved.