Conversation
* added temporary links feature * updated LV version Signed-off-by: smarcet@gmail.com <smarcet@gmail.com> Change-Id: I1b868b8318f431d67819911efda6381f7ae923ee
caseylocker
added a commit
that referenced
this pull request
Apr 16, 2026
Addresses smarcet's PR #525 review items #8 and #9: - Widen the .tld branch regex from /^\.\w+$/ to /^\.[a-z0-9]+(?:\.[a-z0-9]+)*$/i so admins can register suffixes like .co.uk, .com.au, .ac.uk via the API. The runtime matcher in DomainAuthorizedPromoCodeTrait::matchesEmailDomain already supported these via str_ends_with — the validator was incorrectly narrower than the runtime. - Add tests/Unit/Rules/AllowedEmailDomainsArrayTest covering 31 cases: valid single/multi-label TLDs, @Domain, exact email, uppercase, trimmed whitespace, mixed valid; plus rejection of non-arrays, empty and whitespace-only entries, non-string elements, nested arrays, and malformed patterns (., ..com, .com., .co..uk, .-edu, @, @.com, bare tokens, one-bad-apple arrays). Uses @dataProvider doc-comment style to match the existing precedent (AbstractOAuth2ApiScopesTest).
caseylocker
added a commit
that referenced
this pull request
Apr 16, 2026
Addresses smarcet's PR #525 review items #8 and #9: - Widen the .tld branch regex from /^\.\w+$/ to /^\.[a-z0-9]+(?:\.[a-z0-9]+)*$/i so admins can register suffixes like .co.uk, .com.au, .ac.uk via the API. The runtime matcher in DomainAuthorizedPromoCodeTrait::matchesEmailDomain already supported these via str_ends_with — the validator was incorrectly narrower than the runtime. - Add tests/Unit/Rules/AllowedEmailDomainsArrayTest covering 31 cases: valid single/multi-label TLDs, @Domain, exact email, uppercase, trimmed whitespace, mixed valid; plus rejection of non-arrays, empty and whitespace-only entries, non-string elements, nested arrays, and malformed patterns (., ..com, .com., .co..uk, .-edu, @, @.com, bare tokens, one-bad-apple arrays). Uses @dataProvider doc-comment style to match the existing precedent (AbstractOAuth2ApiScopesTest).
smarcet
added a commit
that referenced
this pull request
Apr 17, 2026
…on access (#525) * feat(promo-codes): implement domain-authorized promo codes for early registration access Adds two new promo code subtypes (DomainAuthorizedSummitRegistrationDiscountCode and DomainAuthorizedSummitRegistrationPromoCode) enabling domain-based early registration access. Adds WithPromoCode ticket type audience value for promo-code-only distribution. Includes auto-discovery endpoint, per-account quantity enforcement at checkout, and auto_apply support for existing member/speaker promo code types. SDS: doc/promo-codes-for-early-registration-access.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address review follow-ups for Tasks 1–3 - Task 1: Add ClassName discriminator ENUM widening in migration, add data guard before narrowing Audience ENUM in down() - Task 2: Guard matchesEmailDomain() against emails missing @ to prevent false-positive suffix matches - Task 3: Replace canBeAppliedTo() with direct collection membership check in addTicketTypeRule() (Truth #4), override removeTicketTypeRule() to prevent parent from re-adding to allowed_ticket_types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(promo-codes): add Task 4 review follow-up note for no-op override Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(promo-codes): add review follow-ups for Tasks 5 and 7 Task 5: accepted NITs for constant naming, interface gap, and pre-existing edge cases. Task 7: MUST-FIX — canBuyRegistrationTicketByType() missing WithPromoCode branch blocks checkout for promo-code-only tickets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address Task 6 review follow-ups - Replace 'sometimes|json' with custom AllowedEmailDomainsArray rule for allowed_email_domains validation — accepts pre-decoded PHP array and validates each entry against @domain.com/.tld/user@email formats - Remove json_decode() from factory populate for both domain-authorized types — value is already a PHP array after request decoding - Fix expand=allowed_ticket_types silently dropping field on DomainAuthorizedSummitRegistrationDiscountCodeSerializer — extend re-add guard to check both $relations and $expand - Rename json_array → json_string_array in both new serializers Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address Task 7 review follow-ups Add WithPromoCode branch to canBuyRegistrationTicketByType() so promo-code-only ticket types are not rejected at checkout for both invited and non-invited users. Replace isSoldOut() with canSell() in the strategy's WithPromoCode loop to align listing visibility with checkout enforcement. Add 5 unit tests for audience-based filtering scenarios required by Task 7 DoD. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address review follow-ups for Tasks 8 and 9 Task 8: wrap INSTANCE OF chain in parentheses to preserve summit scoping, simplify speaker email matching via getOwnerEmail(), and exclude cancelled tickets from quantity-per-account count. Task 9: add remaining_quantity_per_account (null) to all four member/speaker serializers, re-add allowed_ticket_types to member and speaker discount code serializers, and declare setter/getter on IDomainAuthorizedPromoCode interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address Task 10 review follow-ups — race-safe quantity_per_account Move ApplyPromoCodeTask after ReserveOrderTask in the saga chain so ticket rows exist when the count query fires. Broaden getTicketCountByMemberAndPromoCode to include 'Reserved' orders, ensuring concurrent checkouts correctly see each other's reservations. Remove the TOCTOU-vulnerable pre-check from PreProcessReservationTask and relocate it inside ApplyPromoCodeTask's locked transaction, where it naturally fires once per unique promo code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(promo-codes): add Task 11 review follow-up notes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address Task 12 review follow-ups — tests for collision, canBeAppliedTo, discovery, checkout - Fix base class: extend Tests\TestCase instead of PHPUnit\Framework\TestCase (boots Laravel facades) - Add 3 collision avoidance tests for DomainAuthorizedSummitRegistrationDiscountCode - Add 2 canBeAppliedTo override tests (free ticket guard bypass) - Add 4 auto_apply tests for existing email-linked types (Member/Speaker promo/discount) - Fix vacuous testWithPromoCodeAudienceNoPromoCodeNotReturned (now asserts on real data) - Add 3 serializer tests (auto_apply, remaining_quantity_per_account, email-linked type) - Rename misleading test to testWithPromoCodeAudienceLivePromoCodeReturned - Add 5 discovery endpoint integration tests in OAuth2SummitPromoCodesApiTest - Add 3 checkout enforcement test stubs (2 need order pipeline harness, 1 blocked by D4) - Mark all 9 review follow-ups complete in SDS doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): register discover endpoint in ApiEndpointsSeeder The GET /api/v1/summits/{id}/promo-codes/all/discover route was added in Task 12 but never seeded into the api_endpoints table. The OAuth2 bearer token validator middleware rejects any unregistered route with a 400 "API endpoint does not exits" error, causing 5 discover-endpoint integration tests in OAuth2SummitPromoCodesApiTest to fail. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): use rate.limit instead of auth.user on discover route The discover endpoint's seeder entry intentionally omits authz_groups per SDS Task 9 ("any authenticated user with read scope"). The auth.user middleware requires at least one matching group, so every request fell through to a 403. Switch to rate.limit:25,1 to match the adjacent pre-validate-promo-code route, which has the same "any authenticated user" profile. OAuth2 bearer auth and scope enforcement are still applied via the parent 'api' middleware group. All 5 discover integration tests now pass (verified locally). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): guard WithPromoCode reservations and exclude exhausted discovery codes Two review findings on the promo-codes branch. P1 — `POST /orders` allowed reserving audience=WithPromoCode ticket types with just a `type_id` and no `promo_code`. `Summit::canBuyRegistrationTicketByType` unconditionally returns true for that audience, and `PreProcessReservationTask::run` only validated a promo code when one was supplied. Add an explicit `isPromoCodeOnly() && empty($promo_code_value)` guard that throws ValidationException; reuses the existing `SummitTicketType::isPromoCodeOnly()` helper. P2 — Promo code discovery endpoint returned globally exhausted finite codes (`quantity_used >= quantity_available`). The repository filter uses `isLive()` which is dates-only, and the service layer only enforced the per-account quota. Add a `hasQuantityAvailable()` short-circuit at the top of `SummitPromoCodeService::discoverPromoCodes` so discovery matches `validate()` behavior at checkout. Regression tests: - `tests/Unit/Services/PreProcessReservationTaskTest.php` — pure PHPUnit unit tests for the WithPromoCode guard (reject + non-overreach). - `tests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.php` — pure PHPUnit unit tests for the global exhaustion filter (reject, healthy passes, mixed batch). - `tests/oauth2/OAuth2SummitPromoCodesApiTest.php` — `testDiscoverExcludesGloballyExhaustedCodes`, sibling to the existing per-account exhaustion integration test. Mutation-verified: temporarily reverted both fixes, confirmed that 3 of 5 new unit tests fail as expected, then restored. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * test(promo-codes): add mixed-payload and infinite-code regression tests Follow-up to b87cefd addressing Codex review suggestions #2 and #4. PreProcessReservationTaskTest: add two mixed-payload tests exercising the per-ticket guard in heterogeneous reservations (promo-only + Audience_All), both orderings. The original tests only covered single-ticket payloads. - testRejectsMixedPayloadWithPromoCodeOnlyFirst — guard fires on first iter. - testRejectsMixedPayloadWithPromoCodeOnlySecond — guard fires after prior aggregation; proves the exception short-circuits cleanly. SummitPromoCodeServiceDiscoveryTest: add an infinite-code overreach test that pins the `quantity_available == 0` semantics — `hasQuantityAvailable()` short-circuits to true for infinite codes, so the exhaustion guard must not drop them. - testDiscoverReturnsInfiniteDomainAuthorizedCode. Mutation-verified: reverting the production fixes causes the 3 reject tests to fail while the infinite-code and healthy-code tests still pass, as expected for overreach guards. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): fix serializer tests and resolve D3 deviation Serializer unit tests (testSerializerAutoApplyField, testSerializerRemainingQuantityPerAccount, testSerializerAutoApplyEmailLinkedType) were failing because bare model instances lacked a Summit association, causing getSummitId() to call getId() on null. Added buildMockSummitForSerializer() helper and setSummit() calls in all three tests. Updated D3 deviation status to RESOLVED — AllowedEmailDomainsArray custom rule was already implemented. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address CodeRabbit findings — CSV domain import and migration rollback CodeRabbit flagged 6 issues on PR #525. After independent validation (Codex), 2 were confirmed as real bugs, 2 were false positives, and 2 were informational/misframed. Fixed (validated as real): - **CSV import TypeError:** `allowed_email_domains` was not exploded from its pipe-delimited CSV string before reaching `setAllowedEmailDomains(array)`, causing a TypeError on domain-authorized code import. Added the same `explode('|', ...)` normalization used by all other CSV list fields in both the add and update import paths. - **Migration down() failure:** Dropping the joined domain-authorized tables did not remove orphaned base-table rows, so narrowing the ClassName ENUM would fail if any domain-authorized promo codes existed. Added a DELETE statement before the ALTER TABLE. Dismissed (validated as false positives): - `remaining_quantity_per_account = null` in MemberDiscountCode serializer is correct — Member types do not have per-account quantity. - Discover route already has OAuth2 auth via the `api` middleware group and an explicit controller-level null-member guard. Adding `auth.user` would break it (requires authz_groups, intentionally removed in 138c1f8). Deferred: - `boolval("false")` pattern is pre-existing across the factory (not introduced by this PR); warrants a separate cleanup. - Multi-level TLD validation regex (`.co.uk`) is an enhancement, not a bug in the current domain-matching logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): harden CSV domain import and migration rollback safety CSV import — blank allowed_email_domains cells produced [''] after explode, which passed the empty() check on the array but caused matchesEmailDomain() to reject every email (empty pattern is skipped, no match found, returns false). Now trims whitespace, filters empty strings, and unsets the key if no valid domains remain. Migration down() — replaced DELETE with UPDATE to remap domain-authorized rows to base types (discount→SummitRegistrationDiscountCode, promo→SummitRegistrationPromoCode). DELETE would silently cascade through SummitAttendeeTicket.PromoCodeID (ON DELETE CASCADE), destroying ticket history. UPDATE preserves FK references while safely narrowing the ENUM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(promo-codes): add D10/D11 deviations for CSV import and migration rollback D10: blank CSV cell for allowed_email_domains produced [''] which silently bricked promo codes by rejecting all emails. D11: migration down() DELETE cascaded through SummitAttendeeTicket FK, destroying ticket history. Replaced with UPDATE to base types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address smarcet review — saga compensation, discover serializer, endpoint migration - Implement ReserveOrderTask::undo() so ApplyPromoCodeTask failures (invalid code / canBeAppliedTo / domain reject / QuantityPerAccount) no longer leave orphaned Order+Ticket rows. Relies on SummitOrder::\$tickets cascade=remove + orphanRemoval=true to drop ticket rows. - Defer CreatedSummitRegistrationOrder event dispatch from ReserveOrderTask::run to SummitOrderService::reserve, so listeners only observe fully-validated reservations. - Use SerializerUtils::getExpand/getFields/getRelations in discover() to match the rest of the controller's API pattern. - Seed discover-promo-codes endpoint via config migration Version20260412000000.php so deployed environments get the endpoint row without re-running the seeder. - Fix ApiEndpointsSeeder: IGroup::Sponsors on get-sponsorship was in the scopes array; moved to authz_groups. - Add tests/Unit/Services/SagaCompensationTest covering undo() no-op when no order persisted, undo() removes order+detaches tickets, and Saga::abort invokes undo in reverse order. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(saga): clear resolved facade instances in setUp for test isolation When SagaCompensationTest runs after tests that bound the real Log facade, Facade::$resolvedInstance still caches the full LogManager. Clear it in setUp so the minimal container bound afterwards is honored. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address CodeRabbit findings on saga reorder - Critical: ReserveOrderTask::run now returns the accumulated formerState instead of a fresh ['order' => ...] array. After the reorder, ApplyPromoCodeTask runs downstream and reads promo_codes_usage / reservations / ticket_types_ids that earlier tasks populated; dropping state broke promo redemption and per-account enforcement for every promo-code checkout. - Minor: discover() OpenAPI security annotation now declares both ReadSummitData and ReadAllSummitData to match the seeded scopes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(promo-codes): address romanetar's PR #525 review comments - Document QuantityPerAccount guard semantics in ApplyPromoCodeTask (the existing `>` comparator is correct because $existingCount already includes the in-flight order's tickets via ReserveOrderTask's commit). - Extract SummitPromoCodeService::parseDelimitedDomains() helper to deduplicate allowed_email_domains parsing across importPromoCodes and importDiscountCodes paths. - Make the discount-code serializer's intentional unset of allowed_ticket_types explicit: add a doc comment on the parent and a protected restoreAllowedTicketTypes() helper; replace duplicated re-add blocks in DomainAuthorized/Member/Speaker discount-code serializers with the helper call. - Add ApplyPromoCodeTaskQuantityPerAccountTest unit regression coverage pinning the guard arithmetic (6 cases, mutation-tested against `>=` and `+$qty` variants suggested in review). No behavior change on the wire; verified via phpunit. * refactor(migrations): use Builder/Table API in Version20260401150000 Addresses smarcet's PR #525 review: - Replace raw CREATE TABLE / ADD COLUMN with LaravelDoctrine Builder + Table fluent API (matches Version20231208171355, Version20260407003923). - Reorder down(): remap ClassName discriminator before dropping joined tables so an interrupted rollback can't leave the enum pointing at non-existent tables (DROP TABLE implicit-commit hazard). - Keep ENUM MODIFY as raw addSql (Doctrine Schema has no MySQL ENUM support; matches Version20231208172204). - Verified with down/up round-trip on local model DB; Codex audit clean. * fix(rules): accept multi-level TLD suffixes in AllowedEmailDomainsArray Addresses smarcet's PR #525 review items #8 and #9: - Widen the .tld branch regex from /^\.\w+$/ to /^\.[a-z0-9]+(?:\.[a-z0-9]+)*$/i so admins can register suffixes like .co.uk, .com.au, .ac.uk via the API. The runtime matcher in DomainAuthorizedPromoCodeTrait::matchesEmailDomain already supported these via str_ends_with — the validator was incorrectly narrower than the runtime. - Add tests/Unit/Rules/AllowedEmailDomainsArrayTest covering 31 cases: valid single/multi-label TLDs, @Domain, exact email, uppercase, trimmed whitespace, mixed valid; plus rejection of non-arrays, empty and whitespace-only entries, non-string elements, nested arrays, and malformed patterns (., ..com, .com., .co..uk, .-edu, @, @.com, bare tokens, one-bad-apple arrays). Uses @dataProvider doc-comment style to match the existing precedent (AbstractOAuth2ApiScopesTest). * feat(promo-codes): add SummitPromoCodeMemberReservation entity (data layer) Schema + data layer for the per-member QuantityPerAccount counter that will fix the TOCTOU race smarcet flagged in PR #525 (and reproduced in PR #530). This commit is intentionally NOT wired into the order-reserve saga yet — the table sits unused until the follow-up commit that modifies PreProcessReservationTask. - Entity SummitPromoCodeMemberReservation (SilverstripeBaseModel) with unique (PromoCodeID, MemberID) and ManyToOne FKs cascading on delete. - ISummitPromoCodeMemberReservationRepository + Doctrine impl. Readers from the reservation path rely on the outer row lock already held on the parent promo code (getByValueExclusiveLock) for serialization, so the repo does not take its own PESSIMISTIC_WRITE. - Two migrations, split so CREATE TABLE commits before the backfill INSERT runs (Builder defers schema diff to end-of-migration, so INSERT-in-same-migration hits "table doesn't exist"): Version20260415191521 — create table via Builder/Table API. Version20260415191522 — backfill from existing committed tickets (ON DUPLICATE KEY UPDATE for idempotency). - RepositoriesProvider binding. - Verified: down/up round-trip on docker MySQL; php -l clean. * fix(promo-codes): harden reservation backfill per Codex review Three no-behavior-change safety nits from the step-1 audit: - Version20260415191522 backfill now uses GREATEST(QtyUsed, VALUES(QtyUsed)) on the ON DUPLICATE KEY path so a re-run after live saga writes can never clobber a newer counter with a stale historical count. - Version20260415191522.down() is now a documented no-op. The previous TRUNCATE would have silently zeroed live counters once step 2 lands. Roll back Version20260415191521 if a clean slate is needed. - ISummitPromoCodeMemberReservationRepository docblock is reworded so the 'outer lock' statement is explicit as a CALLER PRECONDITION, not something the repository guarantees or enforces. Migration down/up round-trip re-verified on docker MySQL. Remaining step-1 concern — undo idempotency of the entity's decrement helper — is deferred to step 2, where the saga caller is the right place to guard duplicate undo invocations (mirror the 'redeem' flag pattern already used by ApplyPromoCodeTask::undo). * feat(promo-codes): atomic per-member reserve in PreProcessReservationTask Step 2 of 3 for the TOCTOU fix. Wires SummitPromoCodeMemberReservation (added in b1f3abe) into the order-reserve saga. The post-facto check in ApplyPromoCodeTask stays in place as belt-and-suspenders for this commit; it is removed in step 3. - PreProcessReservationTask gains two optional collaborators (ISummitPromoCodeMemberReservationRepository, ITransactionService) plus a new protected reserveMemberQuotas() pass that runs after the existing validation. For each IDomainAuthorizedPromoCode in the payload with QuantityPerAccount > 0, it opens a short transaction that acquires PESSIMISTIC_WRITE on the parent promo code row (via the existing getByValueExclusiveLock), upserts the per-member counter, and rejects if the new total would exceed the limit. - The outer row lock is the serialization point: two concurrent sagas serialize on the lock, and the second observes the first's increment once the first commits. - undo() walks the reserved list and decrements each counter under the same lock. Idempotent via a local $undone flag; best-effort (logs and continues on failure so remaining codes still release). - SagaFactory and SummitOrderService ctors are extended to carry the new repo through from Laravel's container; PreProcessReservationTask ctor params stay nullable-optional so the existing 2-arg construction path in tests and the PrePaid subclass keeps working. Verified: docker exec summit-api vendor/bin/phpunit --filter "PreProcessReservationTaskTest|SagaCompensationTest|ApplyPromoCodeTaskQuantityPerAccountTest" → 13/13 pass (baseline backward compat) php -l clean. Step 3 (follow-up) will remove the post-facto check in ApplyPromoCodeTask, update smarcet's PR #530 test mocks to exercise the new reservation surface, and add dedicated coverage for the pre-reservation path. * fix(promo-codes): prevent partial-reservation leak on mid-loop failure Codex audit of ad113d5 caught a real BUG. Saga::run() at SummitOrderService.php:131-134 only calls markAsRan() AFTER a task's run() returns. If reserveMemberQuotas() succeeds for code A and then throws on code B, the exception propagates before PreProcessReservationTask is in $already_run_tasks, so saga abort() never invokes this task's undo() — leaking code A's counter increment on the durable reservation row. Guard reserveMemberQuotas() with a local try/catch in run() that calls $this->undo() (idempotent via the $undone flag) before rethrowing, so any partial progress is released whether or not the saga reaches us. Found by Codex review, patch as proposed. * chore(unit-test): add unit test to demo the toctou bug The output is .E. — tests 1 and 3 pass, test 2 (testDoubleRejection_BothReservedBeforeEitherValidates) fails with: ValidationException: Promo code DOMAIN-CODE-1 has reached the maximum of 1 tickets per account. Task A throws at SummitOrderService.php:843 (the $existingCount > $quantityPerAccount guard) (exactly the TOCTOU bug). The test asserts Task A should succeed (it's a valid first request), but the inflated count from both orders' tickets being visible causes it to reject. When the race condition is fixed, this test will start passing. * feat(promo-codes): finish TOCTOU fix — remove post-facto check, add tests Step 3 of 3. The per-member QuantityPerAccount check now lives solely in PreProcessReservationTask::reserveMemberQuotas (added in ad113d5, leak-guarded in 77c3059), where it runs atomically under the PESSIMISTIC_WRITE row lock on the parent promo code, BEFORE ReserveTicketsTask and ReserveOrderTask commit any tickets. Changes: - Remove the belt-and-suspenders post-facto check from ApplyPromoCodeTask::run. A comment now points at the pre-reservation location and links to smarcet's TOCTOU reproduction for context. The old check counted committed tickets and could not distinguish concurrent orders' rows — see the race narrative in PR #525. - Delete tests/Unit/Services/ApplyPromoCodeTaskConcurrencyTest.php. smarcet added this in the cherry-picked 2e0ef84 to prove the TOCTOU bug against the old check surface (static getTicketCountByMemberAndPromoCode). That surface is no longer in the write path, so the test targets removed code. The narrative is preserved — and extended — in the new file below. - Delete tests/Unit/Services/ApplyPromoCodeTaskQuantityPerAccountTest.php. All six cases validated the exact post-facto check that was just removed. - Add tests/Unit/Services/PreProcessReservationTaskConcurrencyTest.php with 6 cases exercising the new surface via reflection on reserveMemberQuotas: 1. First reserve succeeds when no prior row exists (repo `add` called with qty_used=1). 2. Second reserve rejects when the prior row's QtyUsed already sits at the limit (the serialized-second-request flow that replaces smarcet's TOCTOU reproduction). 3. Within-limit reserve increments the existing row. 4. Limit = 0 bypasses reservation entirely (unlimited per account). 5. Non-IDomainAuthorizedPromoCode codes are skipped (no reservation repo calls). 6. undo() decrements each reserved counter exactly once and is idempotent via the $undone guard. Verified: docker exec summit-api composer dump-autoload # pick up new entity docker exec summit-api vendor/bin/phpunit --filter "PreProcessReservationTask|SagaCompensationTest|ApplyPromoCodeTask" → 13/13 pass (3 PHPUnit deprecations match repo baseline). Outstanding from smarcet's PR #525 review: #7 (discoverPromoCodes N+1) is the only remaining item. * fix(promo-codes): step-3 review follow-ups Two findings from Codex audit of 999eec1: 1. BUG — ApplyPromoCodeTask's pointer comment referenced tests/Unit/Services/ApplyPromoCodeTaskConcurrencyTest, a file deleted in the same commit. Repoint at the new location, tests/Unit/Services/PreProcessReservationTaskConcurrencyTest. (Patch applied by Codex.) 2. CONCERN — coverage parity lost the "single order qty > limit with no prior reservation row" case from the deleted ApplyPromoCodeTaskQuantityPerAccountTest::testRejectsWhenOrderExceedsLimit. Restore that branch via a new PreProcessReservation test: testSingleOrderExceedingLimitRejects (limit=1, prior=null, qty=2 → reject; repo `add` must never be called). Now 7/7 passing in PreProcessReservationTaskConcurrencyTest. * refactor(promo-codes): split discover query into targeted per-subtype DQL getDiscoverableByEmailForSummit loaded ALL 6 discoverable subtypes for the entire summit, then filtered in PHP — O(all codes) hydrations. Split into two focused methods: - getDomainAuthorizedDiscoverableForSummit: fetches only the 2 DA types, filters by email domain in PHP (unavoidable pattern match) - getEmailLinkedDiscoverableForSummit: 4 DQL queries (one per Member/ Speaker × Promo/Discount subtype) push the email filter into the WHERE clause, including the MemberPromoCodeTrait owner-fallback and the PresentationSpeaker member/registration_request two-hop chain Both methods add isLive() date filtering in DQL (:now parameter), matching the codebase convention from DoctrineSummitRepository. The facade method delegates to both and merges, preserving the existing caller contract (discoverPromoCodes and its exhaustion/quota logic are untouched). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): address Codex review on discover query split - Switch INSTANCE OF from :param binding to inline class interpolation, matching the original code's pattern and avoiding Doctrine discriminator binding edge cases - Add explicit parentheses around isLive DQL condition for defensive clarity (andWhere already wraps, but now consistent with email-linked) - Fix member email fallback: PHP empty() matches both NULL and '', but DQL only checked IS NULL — now also checks e.email = '' to match MemberPromoCodeTrait::getEmail() exactly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(promo-codes): close reservation counter leak on cancel + fix undo retry Two bugs found by Codex review of the TOCTOU fix: 1. restoreTicketsPromoCodes (cancel/refund path) called removeUsage() on the promo code but never decremented the per-member reservation counter (SummitPromoCodeMemberReservation.QtyUsed). After a legitimate cancellation, discovery showed quota available but checkout rejected on the stale counter. Fix: add ?Member $owner parameter, decrement the reservation row after successful removeUsage() in the same try block. Non-ValidationException errors from the reservation path propagate and roll back the transaction. All 4 call sites updated. 2. PreProcessReservationTask::undo() set $undone=true before the loop, making partial failures unrecoverable — failed entries were cleared from $reserved unconditionally. Fix: build a $remaining list of failed entries, set $reserved=$remaining and $undone=empty($remaining) so a retry re-processes only the failed codes. New test file RestorePathReservationTest (6 cases) exercises the real restoreTicketsPromoCodes path via reflection: successful decrement, guest order skip, missing reservation row skip, over-decrement clamp, and non-ValidationException propagation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tests): add missing reservation repo mock to SummitOrderServiceTest The SummitOrderService constructor gained an ISummitPromoCodeMemberReservationRepository parameter in b1f3abe but this test was not updated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: smarcet <smarcet@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Signed-off-by: smarcet@gmail.com smarcet@gmail.com
Change-Id: I1b868b8318f431d67819911efda6381f7ae923ee