Skip to content

Commit 283f576

Browse files
caseylockercaseylocker
committed
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.
1 parent 93bc180 commit 283f576

7 files changed

Lines changed: 381 additions & 45 deletions

‎app/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCodeSerializer.php‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,8 @@ public function serialize($expand = null, array $fields = [], array $relations =
4444
if (!$code instanceof DomainAuthorizedSummitRegistrationDiscountCode) return [];
4545
$values = parent::serialize($expand, $fields, $relations, $params);
4646

47-
// RE-ADD allowed_ticket_types (parent discount serializer unsets it).
48-
// Check both relations (default serialization) and expand (explicit ?expand= request).
49-
$needs_allowed_ticket_types = in_array('allowed_ticket_types', $relations)
50-
|| (!empty($expand) && str_contains($expand, 'allowed_ticket_types'));
51-
if ($needs_allowed_ticket_types && !isset($values['allowed_ticket_types'])) {
52-
$ticket_types = [];
53-
foreach ($code->getAllowedTicketTypes() as $ticket_type) {
54-
$ticket_types[] = $ticket_type->getId();
55-
}
56-
$values['allowed_ticket_types'] = $ticket_types;
57-
}
47+
// See parent::restoreAllowedTicketTypes() docblock for why this call is needed.
48+
$this->restoreAllowedTicketTypes($values, $expand, $relations);
5849

5950
// Transient remaining_quantity_per_account (set by service layer)
6051
$values['remaining_quantity_per_account'] = $code->getRemainingQuantityPerAccount();

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -84,16 +84,8 @@ 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-
}
87+
// See parent::restoreAllowedTicketTypes() docblock for why this call is needed.
88+
$this->restoreAllowedTicketTypes($values, $expand, $relations);
9789

9890
$values['remaining_quantity_per_account'] = null;
9991

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

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

81-
// Re-add allowed_ticket_types (parent discount serializer unsets it).
82-
$needs_allowed_ticket_types = in_array('allowed_ticket_types', $relations)
83-
|| (!empty($expand) && str_contains($expand, 'allowed_ticket_types'));
84-
if ($needs_allowed_ticket_types && !isset($values['allowed_ticket_types'])) {
85-
$ticket_types = [];
86-
foreach ($code->getAllowedTicketTypes() as $ticket_type) {
87-
$ticket_types[] = $ticket_type->getId();
88-
}
89-
$values['allowed_ticket_types'] = $ticket_types;
90-
}
81+
// See parent::restoreAllowedTicketTypes() docblock for why this call is needed.
82+
$this->restoreAllowedTicketTypes($values, $expand, $relations);
9183

9284
$values['remaining_quantity_per_account'] = null;
9385

‎app/ModelSerializers/Summit/Registration/PromoCodes/SummitRegistrationDiscountCodeSerializer.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ public function serialize($expand = null, array $fields = [], array $relations =
4343
if (!$code instanceof SummitRegistrationDiscountCode) return [];
4444
$values = parent::serialize($expand, $fields, $relations, $params);
4545

46+
// Discount codes express ticket-type coverage via `ticket_types_rules`
47+
// (which carries rate/amount per type), so the grandparent's flat
48+
// `allowed_ticket_types` is redundant and ambiguous for a generic
49+
// discount code. Subclasses that DO need the flat list (e.g., admin
50+
// widgets that gate on ticket-type selection without rate semantics)
51+
// must call self::restoreAllowedTicketTypes() after parent::serialize().
4652
unset($values['allowed_ticket_types']);
4753

4854
if (in_array('ticket_types_rules', $relations)) {
@@ -80,4 +86,26 @@ public function serialize($expand = null, array $fields = [], array $relations =
8086

8187
return $values;
8288
}
89+
90+
/**
91+
* Re-adds the flat `allowed_ticket_types` field (unset by this class on
92+
* purpose) for subclasses that need it in addition to `ticket_types_rules`.
93+
* Call after parent::serialize() in the subclass.
94+
*/
95+
protected function restoreAllowedTicketTypes(array &$values, $expand, array $relations): void
96+
{
97+
$code = $this->object;
98+
if (!$code instanceof SummitRegistrationDiscountCode) return;
99+
100+
$needs = in_array('allowed_ticket_types', $relations)
101+
|| (!empty($expand) && str_contains($expand, 'allowed_ticket_types'));
102+
103+
if ($needs && !isset($values['allowed_ticket_types'])) {
104+
$ticket_types = [];
105+
foreach ($code->getAllowedTicketTypes() as $ticket_type) {
106+
$ticket_types[] = $ticket_type->getId();
107+
}
108+
$values['allowed_ticket_types'] = $ticket_types;
109+
}
110+
}
83111
}

‎app/Services/Model/Imp/SummitOrderService.php‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -813,8 +813,26 @@ public function run(array $formerState): array
813813
}
814814
}
815815

816-
// QuantityPerAccount enforcement for domain-authorized promo codes
817-
// Runs inside the locked transaction, after ReserveOrderTask has created ticket rows
816+
// QuantityPerAccount enforcement for domain-authorized promo codes.
817+
//
818+
// IMPORTANT — the condition below is `>`, NOT `>=`, and we do NOT add $qty.
819+
// That is because $existingCount ALREADY INCLUDES the current order's
820+
// tickets. The saga runs ReserveOrderTask before ApplyPromoCodeTask;
821+
// ReserveOrderTask calls $promo_code->applyTo($ticket), which sets
822+
// PromoCodeID on each new ticket, and its transaction commits before
823+
// this task's transaction opens. getTicketCountByMemberAndPromoCode
824+
// is a raw SQL count over SummitAttendeeTicket joined to SummitOrder
825+
// with status IN ('Reserved','Paid','Confirmed'), so the in-flight
826+
// order's freshly-reserved tickets are already counted.
827+
//
828+
// Examples (limit = 2, prior tickets = 0):
829+
// buying 2 -> existingCount=2 -> 2 > 2 false -> allowed (exactly at cap)
830+
// buying 3 -> existingCount=3 -> 3 > 2 true -> rejected
831+
// Examples (limit = 2, prior tickets = 2):
832+
// buying 1 -> existingCount=3 -> 3 > 2 true -> rejected
833+
//
834+
// If the saga order changes, or PromoCodeID assignment moves out of
835+
// ReserveOrderTask, the semantics here break — revisit this check.
818836
if ($promo_code instanceof IDomainAuthorizedPromoCode
819837
&& !is_null($this->owner)
820838
) {

‎app/Services/Model/Imp/SummitPromoCodeService.php‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -642,12 +642,7 @@ public function importPromoCodes(Summit $summit, UploadedFile $csv_file, ?Member
642642
$row['tags'] = explode('|', $row['tags']);
643643
}
644644

645-
if(isset($row['allowed_email_domains'])){
646-
$domains = array_map('trim', explode('|', $row['allowed_email_domains']));
647-
$domains = array_values(array_filter($domains, fn($d) => $d !== ''));
648-
$row['allowed_email_domains'] = !empty($domains) ? $domains : null;
649-
if(is_null($row['allowed_email_domains'])) unset($row['allowed_email_domains']);
650-
}
645+
$this->parseDelimitedDomains($row);
651646

652647
if(isset($row['ticket_types_rules']) && (isset($row['amount']) || isset($row['rate']))){
653648

@@ -752,12 +747,7 @@ public function importSponsorPromoCodes(Summit $summit, UploadedFile $csv_file,
752747
$row['tags'] = explode('|', $row['tags']);
753748
}
754749

755-
if(isset($row['allowed_email_domains'])){
756-
$domains = array_map('trim', explode('|', $row['allowed_email_domains']));
757-
$domains = array_values(array_filter($domains, fn($d) => $d !== ''));
758-
$row['allowed_email_domains'] = !empty($domains) ? $domains : null;
759-
if(is_null($row['allowed_email_domains'])) unset($row['allowed_email_domains']);
760-
}
750+
$this->parseDelimitedDomains($row);
761751

762752
if(isset($row['ticket_types_rules']) && (isset($row['amount']) || isset($row['rate']))){
763753

@@ -1064,4 +1054,17 @@ public function discoverPromoCodes(Summit $summit, Member $member): array
10641054

10651055
return $results;
10661056
}
1057+
1058+
/**
1059+
* Parses a pipe-delimited `allowed_email_domains` value from a CSV import row
1060+
* into a trimmed, non-empty array — or removes the key when no valid domains remain.
1061+
*/
1062+
private function parseDelimitedDomains(array &$row): void
1063+
{
1064+
if (!isset($row['allowed_email_domains'])) return;
1065+
$domains = array_map('trim', explode('|', $row['allowed_email_domains']));
1066+
$domains = array_values(array_filter($domains, fn($d) => $d !== ''));
1067+
$row['allowed_email_domains'] = !empty($domains) ? $domains : null;
1068+
if (is_null($row['allowed_email_domains'])) unset($row['allowed_email_domains']);
1069+
}
10671070
}

0 commit comments

Comments
 (0)