Skip to content

Commit 383eb12

Browse files
caseylockercaseylocker
authored
feat(promo-codes): Track 1 telemetry + lookup-build for DA discover (#546)
* feat(promo-codes): add AllowedEmailDomainsLookup DTO + builder for Track 1 * test(promo-codes): move AllowedEmailDomainsLookupBuilderTest to Unit/Services + cover malformed-drop * test(promo-codes): align test namespace + drop tautological hash assert + cover malformed silent-drop * feat(promo-codes): add matchesEmailDomainViaLookup parity matcher * feat(promo-codes): Track 1 repo-decouple + lookup-driven discover + metric * docs(promo-codes): mark getDiscoverableByEmailForSummit @deprecated for Track 2 cleanup * fix(promo-codes): unrestricted flag preserves legacy parity for malformed-only arrays * fix(promo-codes): trim email before empty guard in aggregator (CodeRabbit) * fix(promo-codes): apply smarcet review on discoverPromoCodes (PR #546) Addresses all 5 review findings from @smarcet on the Track-1 discover hot path in SummitPromoCodeService::discoverPromoCodes. 1. Whitespace-only email bypassed the empty() guard (empty(" ") === false), causing strtolower(trim(...)) -> "" to flow into the email-linked repo query and potentially match codes with NULL/empty email columns. Guard now normalizes first: if (empty(trim($email))) return []. 2. Per-code lookup rebuild defeated the SDS-promised "per-request micro-opt" — N codes meant N full normalize+sort+sha1 passes. Added $lookupCache keyed by sha1(implode("\0", $patterns)) so sibling codes sharing byte-identical pattern arrays reuse a single built lookup. Cache pre-hash is order- and case-sensitive (acceptable for upstream-generated arrays; future T1-Sanity can surface lower-than-expected hit rate if needed). 3. new \DateTime() replaced with new \DateTime('now', new DateTimeZone('UTC')) to match the repository convention at DoctrineSummitRegistrationPromoCode Repository.php:699. Note: config/app.php sets timezone => 'UTC' and PHP DateTime comparison normalizes via Unix timestamp, so runtime behavior is unchanged — change is defensive consistency. 4. valid_until_date_passed had two semantic bugs at the metric emit: - empty($candidates) short-circuited to true (zero DA codes != all expired) - open-ended codes (null valid_until_date) collapsed the reduce accumulator to false, masking the all-finite-expired case Filter to finite candidates first via array_filter; default empty-finite case to false; simplify reduce predicate since filter pre-enforces the invariants. 5. Test coverage gaps: all 4 existing discover tests mocked matchesEmailDomainViaLookup -> true, so a regression there would silently leak all in-date DA codes to every member. Added two tests: - testDomainNonMatchingDACodeIsExcluded — uses real AllowedEmailDomainsLookupBuilder + makePartial() so the real matchesEmailDomainViaLookup trait method fires; member email user@other.com against a code allowing @acme.com asserts exclusion. - testDiscoverReturnsEmptyForWhitespaceOnlyEmail — pins fix 1 with shouldNotReceive on both repo methods so any guard regression fails. vendor/bin/phpunit tests/Unit/Services -> 30/30 passing (was 28; +2 from the new discovery tests). Builder 12 + parity 11 + Discovery 7 = 30.
1 parent 12a29e2 commit 383eb12

11 files changed

Lines changed: 666 additions & 53 deletions

‎app/Models/Foundation/Summit/Registration/PromoCodes/AllowedEmailDomainsLookup.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php namespace models\summit;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
/**
16+
* Class AllowedEmailDomainsLookup
17+
* @package models\summit
18+
*
19+
* Immutable per-request DTO used by discovery / matching paths to avoid
20+
* recomputing strtolower(trim(...)) for every allowed-email-domain pattern
21+
* on every candidate code. Partitions normalized patterns into:
22+
*
23+
* - $exactSet: O(1) lookup map for "@domain.tld" and "user@domain.tld" patterns
24+
* - $suffixList: array of ".tld"-style suffixes for endsWith / str_ends_with checks
25+
* - $patternsHash: stable sha1 over the sorted normalized pattern set; identifies
26+
* the pattern set for change detection / equality comparisons.
27+
* - $unrestricted: true iff the input pattern array was empty. Distinguishes
28+
* "no patterns configured" (legacy "no restriction") from
29+
* "patterns configured but all malformed" (which the legacy
30+
* matcher treats as no match — see
31+
* DomainAuthorizedPromoCodeTrait::matchesEmailDomain parity contract).
32+
*/
33+
final class AllowedEmailDomainsLookup
34+
{
35+
public function __construct(
36+
public readonly array $exactSet,
37+
public readonly array $suffixList,
38+
public readonly string $patternsHash,
39+
public readonly bool $unrestricted
40+
) {
41+
}
42+
}

‎app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,41 @@ public function matchesEmailDomain(string $email): bool
110110
return false;
111111
}
112112

113+
/**
114+
* Lookup-driven sibling of matchesEmailDomain().
115+
* Consumes a precomputed AllowedEmailDomainsLookup so the caller does not
116+
* pay strtolower/trim per pattern when matching many codes against the
117+
* same pattern set. Must return parity with matchesEmailDomain().
118+
*
119+
* @param string $email
120+
* @param AllowedEmailDomainsLookup $lookup
121+
* @return bool
122+
*/
123+
public function matchesEmailDomainViaLookup(string $email, AllowedEmailDomainsLookup $lookup): bool
124+
{
125+
// Parity with legacy matchesEmailDomain(): only return "match anything"
126+
// when the stored pattern array was actually empty. A non-empty array
127+
// whose patterns all dropped as malformed must still return false here.
128+
if ($lookup->unrestricted) return true;
129+
130+
$email = strtolower(trim($email));
131+
if ($email === '') return false;
132+
133+
$atPos = strpos($email, '@');
134+
if ($atPos === false) return false;
135+
136+
$emailDomain = substr($email, $atPos);
137+
138+
if (isset($lookup->exactSet[$emailDomain])) return true;
139+
if (isset($lookup->exactSet[$email])) return true;
140+
141+
foreach ($lookup->suffixList as $suffix) {
142+
if (str_ends_with($emailDomain, $suffix)) return true;
143+
}
144+
145+
return false;
146+
}
147+
113148
/**
114149
* Validates email against allowed_email_domains.
115150
* Throws ValidationException if no match.

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,20 @@ public function getQuantityPerAccount(): int;
3737
*/
3838
public function matchesEmailDomain(string $email): bool;
3939

40+
/**
41+
* Lookup-driven sibling of matchesEmailDomain().
42+
*
43+
* Consumes a precomputed AllowedEmailDomainsLookup (normalized + partitioned
44+
* exactSet / suffixList) so callers iterating many candidate codes against
45+
* the same pattern set avoid re-normalizing patterns per code. Must return
46+
* the SAME boolean as matchesEmailDomain() for any (patterns, email) pair.
47+
*
48+
* @param string $email
49+
* @param AllowedEmailDomainsLookup $lookup
50+
* @return bool
51+
*/
52+
public function matchesEmailDomainViaLookup(string $email, AllowedEmailDomainsLookup $lookup): bool;
53+
4054
/**
4155
* @param int|null $remaining
4256
* @return void

‎app/Models/Foundation/Summit/Repositories/ISummitRegistrationPromoCodeRepository.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,14 @@ public function getBySummitAndCode(Summit $summit, string $code):?SummitRegistra
8181
public function getDiscoverableByEmailForSummit(Summit $summit, string $email): array;
8282

8383
/**
84+
* Returns date-windowed DOMAIN_AUTHORIZED_* candidates for the summit.
85+
* Filtering by email moved to SummitPromoCodeService::discoverPromoCodes
86+
* under option (b) of the Track-1 repository-decouple (SDS Task T1-Service).
87+
*
8488
* @param Summit $summit
85-
* @param string $email
8689
* @return SummitRegistrationPromoCode[]
8790
*/
88-
public function getDomainAuthorizedDiscoverableForSummit(Summit $summit, string $email): array;
91+
public function getDomainAuthorizedDiscoverableForSummit(Summit $summit): array;
8992

9093
/**
9194
* @param Summit $summit

‎app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -661,32 +661,43 @@ public function getBySummitAndCode(Summit $summit, string $code):?SummitRegistra
661661
* Returns domain-authorized types (matched by email domain) and
662662
* existing email-linked types (member/speaker, matched by associated email).
663663
*
664+
* @deprecated Track 1 (SDS task T1-Service) moved the discover hot path to
665+
* SummitPromoCodeService::discoverPromoCodes calling the two leaf methods
666+
* directly. This aggregator is retained only to preserve its public
667+
* by-email contract for any non-grepped caller. Remove once Track 2 ships
668+
* and a `grep -rn getDiscoverableByEmailForSummit` confirms zero callers.
669+
*
664670
* @param Summit $summit
665671
* @param string $email
666672
* @return SummitRegistrationPromoCode[]
667673
*/
668674
public function getDiscoverableByEmailForSummit(Summit $summit, string $email): array
669675
{
670-
if (empty($email)) return [];
676+
$normalized = strtolower(trim($email));
677+
if ($normalized === '') return [];
671678

672-
$email = strtolower(trim($email));
679+
$daCandidates = $this->getDomainAuthorizedDiscoverableForSummit($summit);
673680

674-
return array_merge(
675-
$this->getDomainAuthorizedDiscoverableForSummit($summit, $email),
676-
$this->getEmailLinkedDiscoverableForSummit($summit, $email)
677-
);
681+
$daMatched = [];
682+
foreach ($daCandidates as $code) {
683+
if ($code instanceof IDomainAuthorizedPromoCode && $code->matchesEmailDomain($normalized)) {
684+
$daMatched[] = $code;
685+
}
686+
}
687+
688+
$emailLinked = $this->getEmailLinkedDiscoverableForSummit($summit, $normalized);
689+
return array_merge($daMatched, $emailLinked);
678690
}
679691

680692
/**
681693
* @param Summit $summit
682-
* @param string $email
683694
* @return SummitRegistrationPromoCode[]
684695
*/
685-
public function getDomainAuthorizedDiscoverableForSummit(Summit $summit, string $email): array
696+
public function getDomainAuthorizedDiscoverableForSummit(Summit $summit): array
686697
{
687-
$em = $this->getEntityManager();
698+
$em = $this->getEntityManager();
688699
$now = new \DateTime('now', new \DateTimeZone('UTC'));
689-
$daPromoClass = DomainAuthorizedSummitRegistrationPromoCode::class;
700+
$daPromoClass = DomainAuthorizedSummitRegistrationPromoCode::class;
690701
$daDiscountClass = DomainAuthorizedSummitRegistrationDiscountCode::class;
691702

692703
$qb = $em->createQueryBuilder();
@@ -702,16 +713,7 @@ public function getDomainAuthorizedDiscoverableForSummit(Summit $summit, string
702713
->setParameter('summit_id', $summit->getId())
703714
->setParameter('now', $now);
704715

705-
$candidates = $qb->getQuery()->getResult();
706-
$results = [];
707-
708-
foreach ($candidates as $code) {
709-
if ($code instanceof IDomainAuthorizedPromoCode && $code->matchesEmailDomain($email)) {
710-
$results[] = $code;
711-
}
712-
}
713-
714-
return $results;
716+
return $qb->getQuery()->getResult();
715717
}
716718

717719
/**

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
<?php namespace App\Services\Model;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use models\summit\AllowedEmailDomainsLookup;
16+
17+
/**
18+
* Class AllowedEmailDomainsLookupBuilder
19+
* @package App\Services\Model
20+
*
21+
* Pure builder: turns a raw list of allowed-email-domain patterns into an
22+
* immutable AllowedEmailDomainsLookup DTO. No DB, no cache, no Doctrine.
23+
*
24+
* Normalization rules:
25+
* - strtolower(trim((string) $raw)) on every pattern
26+
* - drop empty values
27+
* - case-insensitive dedup (first occurrence wins)
28+
*
29+
* Partition rules:
30+
* - leading '@' -> exactSet[$p] = true (e.g. "@acme.com")
31+
* - leading '.' -> suffixList[] = $p (e.g. ".edu")
32+
* - contains '@' (not at start) -> exactSet[$p] = true (e.g. "user@acme.com")
33+
* - otherwise -> dropped silently
34+
*
35+
* patternsHash = sha1(implode('|', $sortedNormalizedPatterns)) — stable
36+
* regardless of input order, so callers can use it for change detection / equality.
37+
*/
38+
final class AllowedEmailDomainsLookupBuilder
39+
{
40+
public function build(array $patterns): AllowedEmailDomainsLookup
41+
{
42+
// Capture "no patterns configured" from the RAW input BEFORE any
43+
// normalization or partitioning. Required for legacy parity: a
44+
// non-empty input whose patterns all drop as malformed must NOT
45+
// be treated as unrestricted.
46+
$unrestricted = count($patterns) === 0;
47+
48+
$exactSet = [];
49+
$suffixList = [];
50+
$seen = [];
51+
$normalized = [];
52+
53+
foreach ($patterns as $raw) {
54+
$p = strtolower(trim((string) $raw));
55+
if ($p === '') {
56+
continue;
57+
}
58+
if (isset($seen[$p])) {
59+
continue;
60+
}
61+
$seen[$p] = true;
62+
63+
$first = $p[0];
64+
if ($first === '@') {
65+
$exactSet[$p] = true;
66+
$normalized[] = $p;
67+
continue;
68+
}
69+
if ($first === '.') {
70+
$suffixList[] = $p;
71+
$normalized[] = $p;
72+
continue;
73+
}
74+
if (strpos($p, '@') !== false) {
75+
$exactSet[$p] = true;
76+
$normalized[] = $p;
77+
continue;
78+
}
79+
// otherwise: dropped silently
80+
}
81+
82+
sort($normalized);
83+
$patternsHash = sha1(implode('|', $normalized));
84+
85+
return new AllowedEmailDomainsLookup($exactSet, $suffixList, $patternsHash, $unrestricted);
86+
}
87+
}

0 commit comments

Comments
 (0)