Pre-select the ticket a manually applied promo code affects - #155
Pre-select the ticket a manually applied promo code affects#155gcutrini wants to merge 1 commit into
Conversation
Applying a code only moved the selection for discovered codes, which carry an allowed_ticket_types list. A typed code selected nothing unless the summit had one ticket type, so the user had to work out which ticket their code was for. The API already marks what a code did, so read that off the refreshed catalog instead of comparing it against the one from before: a WithPromoCode audience means a live code unlocked the type, a PrePaid subtype means a prepaid code claims it, and a discount is marked on the ticket. Unlock and discount stay separate signals, since a free type unlocked by a plain discount code is never wrapped and only its audience says a code revealed it. A type on public sale that a prepaid code also claims comes back twice, sharing an id and differing by subtype. Identity is the pair, so selecting, re-syncing and the dropdown keys all compare both. A typed code moves the selection even when one was already made, and decides once: the catalog is rebuilt whenever a sales window opens or closes, and deciding again would overrule what the user chose after. A discovered code takes the first of the several it names, so it only decides while nothing is chosen.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Looks good, but please @gcutrini double check the comments the AI adds. We should not explain every single change with a comment, only critic logic that is not easy to understand by reading the code.
| @@ -0,0 +1,302 @@ | |||
| const { test, expect } = require('@playwright/test'); | |||
There was a problem hiding this comment.
shouldn't this be a .test.js file ? why .spec ?
There was a problem hiding this comment.
and why is this file not placed under a tests dir like all tests in our codebase ?
There was a problem hiding this comment.
@santipalenque these are e2e test ran using playwright we use this new convention to differentiate it from the unit test using jest
| const [ticket, setTicket] = useState(null); | ||
| // The code whose pre-selection has already been made. Applying a code | ||
| // decides once; the catalog is rebuilt whenever any sales window opens or | ||
| // closes, and deciding again then would overrule what the user chose after. |
There was a problem hiding this comment.
Too long! this should be something like "We must keep selection between sales windows" or something
| }, [ticket, quantity, maxQuantity]) | ||
|
|
||
| // A type on public sale that a prepaid code also claims comes back twice, | ||
| // as the regular offer and the prepaid one. Identity is the pair. |
| // only decides while nothing is chosen. | ||
| const toSelect = ticketToPreSelect(); | ||
| const mayMove = !ticket || !isDiscoveredCode; | ||
| if (toSelect && mayMove && !isSameOffer(toSelect, ticket)) { |
There was a problem hiding this comment.
@gcutrini This pre-selection re-fires on every mount and overrides the ticket and quantity restored from an existing reservation.
preSelectedFor is a ref initialised to null per mount, so a reload at the personal-info or payment step re-decides for the persisted promo code (promoCode is not blacklisted in src/store.js, and getTicketTypes re-filters by it at src/actions.js:152-161). The restore effect at :80-85 and this one run in the same commit, so ticket is still null here — !isSameOffer(toSelect, null) is true even when toSelect IS the reserved ticket, and handleTicketChange resets quantity to minQuantity.
Verified against a probe render (reservation of 3× the affected ticket, typed code): this branch reports ticketType: Discounted Ticket, ticketQuantity: 1, while the same probe on main reports quantity: 3. The payment-step summary then renders ${ticket.name} (1) above reservation totals for 3, and going Back re-reserves 1 ticket instead of 3.
Careful with the obvious fix: seeding preSelectedFor.current = promoCode in the restore effect repairs the quantity but unmasks the id-only lookup at :82 (see my other comment) — a prepaid claim then binds the Regular offer at full price instead of the PrePaid one at 0. Both sites need to move together.
| } | ||
| const updatedCurrentTicket = originalTicketTypes.find(t => t?.id === ticket.id); | ||
| if (!ticket) return; | ||
| const updatedCurrentTicket = originalTicketTypes.find(t => isSameOffer(t, ticket)); |
There was a problem hiding this comment.
@gcutrini This PR establishes (id, sub_type) as offer identity and applies it here and at :140, but the third comparison site was missed: :82 still matches the reservation on t.id === reservation.tickets[0].ticket_type_id.
With the prepaid duplicates this PR documents, array_merge($regular_ticket_types, array_values($prepaid_ticket_types)) in summit-api's PrePaidPromoCodeTicketTypesStrategy puts the regular offer first, so find returns the paid one. Today it is masked in the common case — the pre-selection overwrites it with the correct PrePaid offer — but it stands whenever a prepaid code claims two or more types (pre-selection suppressed): verified bound to General / Regular / cost 700 instead of General [PREPAID] / cost 0.
Note BaseSummitAttendeeTicketSerializer:32 exposes only ticket_type_id, no subtype, so this needs either a new serializer field or a different disambiguator (the ticket's cost, or expanding promo_code on it) — not a one-line swap to isSameOffer.
| // Three marks the catalog cannot carry unless a code produced them. The | ||
| // subtype is safe to key on because it is derived, not stored: nothing but | ||
| // the prepaid decorator can report one. | ||
| const isAffectedByPromoCode = (t) => |
There was a problem hiding this comment.
@gcutrini The audience === WithPromoCode marker is not exclusively produced by the applied code.
RegularPromoCodeTicketTypesStrategy::getTicketTypes merges $invitation->getRemainingAllowedTicketTypes() with no audience or canSell filter, and SummitRegistrationInvitationService::add:220-235 validates only that the type exists. So an invitation listing a WithPromoCode-audience type returns it regardless of any code, and it would be counted as affected forever: a typed code that marks nothing pre-selects it (count 1), or a code that legitimately marks one ticket is suppressed by it (count 2), with the follow-up onRevalidate then surfacing INVALID for a code that is fine.
I could not confirm this configuration exists in the field — it is self-contradictory, and nothing in the code produces it, only fails to prevent it. If you want the marker airtight, count a WithPromoCode type only when it was absent from the pre-apply catalog.
| // Closing Soon must actually leave the list, or this proves nothing: the | ||
| // catalog has to be rebuilt for the re-decide it guards against to be | ||
| // possible at all. | ||
| await page.waitForTimeout(12000); |
There was a problem hiding this comment.
@gcutrini This hardcoded sleep puts the test at 85% of the Playwright timeout on a green CI run.
Run 32494588576 (PR head 9b6700f) records this test at 25.5s against timeout: 30000 in playwright.config.js:4 — 4.5s of headroom, while the other 13 tests in the file sit at 6-13s. The 12s is pure additive wait, so any contention on the 2-worker runner tips it over.
Suggest replacing it with the condition the test already asserts right after: await expect(page.locator('[data-testid="ticket-list"]')).not.toContainText('Closing Soon', { timeout: 20000 }), dropping the sleep — or test.setTimeout() for this case if the sleep has to stay.
ref: https://app.clickup.com/t/86bbeawv8
Applying a promo code now selects the ticket it affected. Before, only suggested codes did, because they arrive with a list of their tickets. A typed code selected nothing unless the summit had a single ticket type.
The API already marks what a code did, so the widget reads that instead of comparing the catalog before and after:
One affected ticket is selected. Several selects nothing, since that is a real choice. Applying a code also moves an existing selection, per JP. A ticket outside its sales window is never selected.
Suggested codes keep using their own list and are unchanged.
Also here: a type on public sale that a prepaid code claims comes back twice, as two offers sharing an id. Identity is (id, sub_type), so selecting, re-syncing and the dropdown keys all compare both.