Skip to content

Commit 5dd1ad7

Browse files
caseylockerclaude
andcommitted
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>
1 parent 2967746 commit 5dd1ad7

7 files changed

Lines changed: 61 additions & 9 deletions

app/ModelSerializers/Summit/Registration/PromoCodes/MemberSummitRegistrationDiscountCodeSerializer.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,19 @@ public function serialize($expand = null, array $fields = [], array $relations =
8484
}
8585
}
8686

87+
// Re-add allowed_ticket_types (parent discount serializer unsets it).
88+
$needs_allowed_ticket_types = in_array('allowed_ticket_types', $relations)
89+
|| (!empty($expand) && str_contains($expand, 'allowed_ticket_types'));
90+
if ($needs_allowed_ticket_types && !isset($values['allowed_ticket_types'])) {
91+
$ticket_types = [];
92+
foreach ($code->getAllowedTicketTypes() as $ticket_type) {
93+
$ticket_types[] = $ticket_type->getId();
94+
}
95+
$values['allowed_ticket_types'] = $ticket_types;
96+
}
97+
98+
$values['remaining_quantity_per_account'] = null;
99+
87100
return $values;
88101
}
89102
}

app/ModelSerializers/Summit/Registration/PromoCodes/MemberSummitRegistrationPromoCodeSerializer.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ public function serialize($expand = null, array $fields = [], array $relations =
8282
}
8383
}
8484

85+
$values['remaining_quantity_per_account'] = null;
86+
8587
return $values;
8688
}
8789
}

app/ModelSerializers/Summit/Registration/PromoCodes/SpeakerSummitRegistrationDiscountCodeSerializer.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,19 @@ public function serialize($expand = null, array $fields = [], array $relations =
7777
}
7878
}
7979

80+
// Re-add allowed_ticket_types (parent discount serializer unsets it).
81+
$needs_allowed_ticket_types = in_array('allowed_ticket_types', $relations)
82+
|| (!empty($expand) && str_contains($expand, 'allowed_ticket_types'));
83+
if ($needs_allowed_ticket_types && !isset($values['allowed_ticket_types'])) {
84+
$ticket_types = [];
85+
foreach ($code->getAllowedTicketTypes() as $ticket_type) {
86+
$ticket_types[] = $ticket_type->getId();
87+
}
88+
$values['allowed_ticket_types'] = $ticket_types;
89+
}
90+
91+
$values['remaining_quantity_per_account'] = null;
92+
8093
return $values;
8194
}
8295
}

app/ModelSerializers/Summit/Registration/PromoCodes/SpeakerSummitRegistrationPromoCodeSerializer.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ public function serialize($expand = null, array $fields = [], array $relations =
7878
}
7979
}
8080

81+
$values['remaining_quantity_per_account'] = null;
82+
8183
return $values;
8284
}
8385
}

app/Models/Foundation/Summit/Registration/PromoCodes/IDomainAuthorizedPromoCode.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,15 @@ public function getQuantityPerAccount(): int;
3636
* @return bool
3737
*/
3838
public function matchesEmailDomain(string $email): bool;
39+
40+
/**
41+
* @param int|null $remaining
42+
* @return void
43+
*/
44+
public function setRemainingQuantityPerAccount(?int $remaining): void;
45+
46+
/**
47+
* @return int|null
48+
*/
49+
public function getRemainingQuantityPerAccount(): ?int;
3950
}

app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -684,7 +684,7 @@ public function getDiscoverableByEmailForSummit(Summit $summit, string $email):
684684
->from($this->getBaseEntity(), 'e')
685685
->leftJoin('e.summit', 's')
686686
->where('s.id = :summit_id')
687-
->andWhere("e INSTANCE OF {$daDiscountClass} OR e INSTANCE OF {$daPromoClass} OR e INSTANCE OF {$memberPromoClass} OR e INSTANCE OF {$memberDiscountClass} OR e INSTANCE OF {$speakerPromoClass} OR e INSTANCE OF {$speakerDiscountClass}")
687+
->andWhere("(e INSTANCE OF {$daDiscountClass} OR e INSTANCE OF {$daPromoClass} OR e INSTANCE OF {$memberPromoClass} OR e INSTANCE OF {$memberDiscountClass} OR e INSTANCE OF {$speakerPromoClass} OR e INSTANCE OF {$speakerDiscountClass})")
688688
->setParameter('summit_id', $summit->getId());
689689

690690
$candidates = $qb->getQuery()->getResult();
@@ -709,12 +709,9 @@ public function getDiscoverableByEmailForSummit(Summit $summit, string $email):
709709
}
710710

711711
if ($code instanceof SpeakerSummitRegistrationPromoCode || $code instanceof SpeakerSummitRegistrationDiscountCode) {
712-
$speaker = $code->getSpeaker();
713-
if (!is_null($speaker) && $speaker->hasMember()) {
714-
$member = $speaker->getMember();
715-
if (!is_null($member) && strtolower($member->getEmail()) === $email && $code->isLive()) {
716-
$results[] = $code;
717-
}
712+
$ownerEmail = $code->getOwnerEmail();
713+
if (!empty($ownerEmail) && strtolower($ownerEmail) === $email && $code->isLive()) {
714+
$results[] = $code;
718715
}
719716
continue;
720717
}
@@ -739,6 +736,7 @@ public function getTicketCountByMemberAndPromoCode(Member $member, SummitRegistr
739736
WHERE t.PromoCodeID = :promo_code_id
740737
AND o.OwnerID = :member_id
741738
AND o.Status IN ('Paid', 'Confirmed')
739+
AND t.Status != 'Cancelled'
742740
SQL;
743741
$stm = $this->getEntityManager()->getConnection()->executeQuery($sql, [
744742
'promo_code_id' => $code->getId(),

doc/promo-codes-for-early-registration-access.md

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,9 @@ Deviations from the SDS captured during implementation. Each entry is either **O
573573
- Unit test for discovery query
574574

575575
**Review Follow-ups:**
576-
- None
576+
- [x] **Summit scoping lost in DQL OR chain (MUST-FIX):** `getDiscoverableByEmailForSummit()` at line 683 builds `->where('s.id = :summit_id')->andWhere("e INSTANCE OF A OR e INSTANCE OF B OR ...")`. Doctrine's `andWhere()` wraps existing + new conditions in an `Andx` composite that renders as `(s.id = :summit_id AND e INSTANCE OF A OR e INSTANCE OF B OR ...)`. Due to SQL/DQL operator precedence (AND before OR), only the first `INSTANCE OF` branch is summit-scoped; all remaining branches match those types from any summit, leaking cross-summit promo codes into discovery results. **Fix:** wrap the entire `INSTANCE OF` chain in an extra pair of parentheses so it is treated as a single group: `->andWhere("(e INSTANCE OF {$daDiscountClass} OR e INSTANCE OF {$daPromoClass} OR e INSTANCE OF {$memberPromoClass} OR e INSTANCE OF {$memberDiscountClass} OR e INSTANCE OF {$speakerPromoClass} OR e INSTANCE OF {$speakerDiscountClass})")`. File: `app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php`, line 687.
577+
- [x] **Speaker email matching misses speakers without a linked Member (SHOULD-FIX):** `getDiscoverableByEmailForSummit()` at lines 711–720 guards on `$speaker->hasMember()` then accesses `$speaker->getMember()->getEmail()`. However, `PresentationSpeaker::getEmail()` (Speakers/PresentationSpeaker.php:1924) already falls through to `$this->registration_request->getEmail()` when no Member association exists. `SpeakerSummitRegistrationPromoCode::getOwnerEmail()` and `SpeakerSummitRegistrationDiscountCode::getOwnerEmail()` both call `$this->getSpeaker()->getEmail()` which uses this fallback. `SpeakerPromoCodeTrait::checkSubject()` validates via `getOwnerEmail()`. The discovery code and `checkSubject` are inconsistent: a speaker code whose speaker has only a `SpeakerRegistrationRequest` (no Member) passes checkout validation but is never returned by discovery. **Fix:** replace the `hasMember()` guard + `getMember()->getEmail()` path with a direct call to `$code->getOwnerEmail()` (which already exists on both speaker promo code types via `IOwnablePromoCode`): `$ownerEmail = $code->getOwnerEmail(); if (!empty($ownerEmail) && strtolower($ownerEmail) === $email && $code->isLive()) { $results[] = $code; }`. File: `app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php`, lines 711–719.
578+
- [x] **`getTicketCountByMemberAndPromoCode` counts cancelled tickets (SHOULD-FIX):** The raw SQL at lines 735–742 filters by `o.Status IN ('Paid', 'Confirmed')` (order status) but does not filter by ticket status. `SummitAttendeeTicket` has its own `Status` column — `isCancelled()` at SummitAttendeeTicket.php:559 checks against `IOrderConstants::CancelledStatus`. A ticket can be individually cancelled within a paid order without changing the order status. Such cancelled tickets are still counted toward `quantity_per_account`, over-inflating the count and potentially blocking users who cancelled and want to repurchase. **Fix:** add `AND t.Status != 'Cancelled'` to the WHERE clause (or equivalently `AND t.Status = 'Paid'` if only Paid is a valid active status for tickets). The constant value is `IOrderConstants::CancelledStatus = 'Cancelled'`. File: `app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php`, line 741.
577579

578580
---
579581

@@ -697,7 +699,18 @@ Deviations from the SDS captured during implementation. Each entry is either **O
697699
- Integration test calling the endpoint
698700

699701
**Review Follow-ups:**
700-
- None
702+
703+
- [x] **`remaining_quantity_per_account` absent from member/speaker serializer output (MUST-FIX):**
704+
All four member/speaker serializers (`MemberSummitRegistrationPromoCodeSerializer`, `MemberSummitRegistrationDiscountCodeSerializer`, `SpeakerSummitRegistrationPromoCodeSerializer`, `SpeakerSummitRegistrationDiscountCodeSerializer`) do not output `remaining_quantity_per_account`. The DoD requires every discover result to include this field, and the SDS sample response shows `"remaining_quantity_per_account": null` for `MEMBER_DISCOUNT_CODE` and `SPEAKER_PROMO_CODE`. The domain-authorized serializers correctly set it from a transient property; member/speaker serializers must emit `null` unconditionally (these types have no per-account limit concept).
705+
**Fix:** In the `serialize()` override of each of the four member/speaker serializers, add `$values['remaining_quantity_per_account'] = null;` before returning `$values`. No entity change required — member/speaker entities do not need a transient property; the value is always `null` for these types.
706+
707+
- [x] **`allowed_ticket_types` absent from member/speaker discount code responses (MUST-FIX):**
708+
`SummitRegistrationDiscountCodeSerializer::serialize()` unconditionally calls `unset($values['allowed_ticket_types'])` (line 46). `MemberSummitRegistrationDiscountCodeSerializer` and `SpeakerSummitRegistrationDiscountCodeSerializer` both extend this class and never re-add the key, so `MEMBER_DISCOUNT_CODE` and `SPEAKER_DISCOUNT_CODE` results from the discover endpoint are missing `allowed_ticket_types`. `DomainAuthorizedSummitRegistrationDiscountCodeSerializer` already demonstrates the correct fix pattern at lines 47–56: check `in_array('allowed_ticket_types', $relations)` and rebuild the array from `$code->getAllowedTicketTypes()`.
709+
**Fix:** In `MemberSummitRegistrationDiscountCodeSerializer::serialize()` and `SpeakerSummitRegistrationDiscountCodeSerializer::serialize()`, after calling `parent::serialize()`, re-add `allowed_ticket_types` using the same pattern as `DomainAuthorizedSummitRegistrationDiscountCodeSerializer.php:47–56`. The controller's default `$relations` already includes `'allowed_ticket_types'`, so no controller change is needed.
710+
711+
- [x] **`IDomainAuthorizedPromoCode` interface missing `setRemainingQuantityPerAccount` / `getRemainingQuantityPerAccount` declarations (SHOULD-FIX):**
712+
`SummitPromoCodeService::discoverPromoCodes()` narrows a code to `IDomainAuthorizedPromoCode` via `instanceof`, then calls `$code->setRemainingQuantityPerAccount(...)` (service lines 1035, 1037). The interface (`IDomainAuthorizedPromoCode.php`) declares only `getAllowedEmailDomains()`, `getQuantityPerAccount()`, and `matchesEmailDomain()` — neither setter nor getter is declared. PHP resolves the call dynamically at runtime (both concrete classes `DomainAuthorizedSummitRegistrationPromoCode` and `DomainAuthorizedSummitRegistrationDiscountCode` implement both methods), but static analysis tools (PHPStan/Psalm) will flag this as a call on an undefined method of the interface type.
713+
**Fix:** Add `public function setRemainingQuantityPerAccount(?int $remaining): void;` and `public function getRemainingQuantityPerAccount(): ?int;` to `IDomainAuthorizedPromoCode.php`. Both concrete classes already implement these methods, so no implementation change is needed — only the interface declaration.
701714

702715
---
703716

0 commit comments

Comments
 (0)