feat(promo-codes): Track 1 telemetry + lookup-build for DA discover#546
Conversation
…Services + cover malformed-drop
…rt + cover malformed silent-drop
…or Track 2 cleanup
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a precomputed AllowedEmailDomainsLookup DTO and builder, implements a lookup-based matcher, removes email filtering from the repository contract, and updates SummitPromoCodeService to filter domain-authorized candidates in-memory using the lookup (merged with email-linked codes) while emitting basic telemetry. ChangesDomain Lookup & Promo Code Discovery Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-546/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-546/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php`:
- Around line 676-689: Normalize the incoming $email first (e.g., $normalized =
strtolower(trim($email))) and use that for the empty guard (if
empty($normalized) return []), then use $normalized throughout: pass $normalized
to IDomainAuthorizedPromoCode::matchesEmailDomain(...) instead of $email and to
getEmailLinkedDiscoverableForSummit($summit, $normalized); keep the same calls
to getDomainAuthorizedDiscoverableForSummit and IDomainAuthorizedPromoCode
checks but operate on the normalized value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db438cef-5002-4763-bead-9c32100f6c8a
📒 Files selected for processing (11)
app/Models/Foundation/Summit/Registration/PromoCodes/AllowedEmailDomainsLookup.phpapp/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.phpapp/Models/Foundation/Summit/Registration/PromoCodes/IDomainAuthorizedPromoCode.phpapp/Models/Foundation/Summit/Repositories/ISummitRegistrationPromoCodeRepository.phpapp/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.phpapp/Services/Model/Imp/AllowedEmailDomainsLookupBuilder.phpapp/Services/Model/Imp/SummitPromoCodeService.phpapp/Services/ModelServicesProvider.phptests/Unit/Services/AllowedEmailDomainsLookupBuilderTest.phptests/Unit/Services/MatchesEmailDomainViaLookupTest.phptests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-546/ This page is automatically updated on each push to this PR. |
| @@ -1024,34 +1033,69 @@ public function discoverPromoCodes(Summit $summit, Member $member): array | |||
| $email = $member->getEmail(); | |||
| if (empty($email)) return []; | |||
There was a problem hiding this comment.
[BLOCKER] Whitespace-only email bypasses guard → live DB query with email=\'
empty(" ") returns false in PHP, so a member whose getEmail() returns whitespace passes this check. $normalizedEmail becomes "" and getEmailLinkedDiscoverableForSummit($summit, "") executes a real DB query binding email=''.
The prior aggregator had an explicit guard at DoctrineSummitRegistrationPromoCodeRepository.php:677 (if ($normalized === '' ) return []) that was removed without a replacement here.
Fix:
if (empty(trim($email))) return []; ```
Also add a unit test in `SummitPromoCodeServiceDiscoveryTest` where `$member->getEmail()` returns `" "`, asserting the result is `[]` and no repository method is called.There was a problem hiding this comment.
Fixed in d7cdc3b. SummitPromoCodeService.php:1034 now reads if (empty(trim($email))) return []; per your suggested form — normalize-then-test mirrors the legacy aggregator at DoctrineSummitRegistrationPromoCodeRepository.php:677. Coverage added in SummitPromoCodeServiceDiscoveryTest::testDiscoverReturnsEmptyForWhitespaceOnlyEmail — mocks getEmail() to return " \t\n " and uses shouldNotReceive on both repo methods so any guard regression fails immediately.
| $matched = []; | ||
| foreach ($candidates as $code) { | ||
| if (!$code instanceof IDomainAuthorizedPromoCode) continue; | ||
| $lookup = $this->lookup_builder->build($code->getAllowedEmailDomains()); |
There was a problem hiding this comment.
[BLOCKER] Lookup rebuilt on every loop iteration — O(N) optimisation does not materialise
$this->lookup_builder->build($code->getAllowedEmailDomains()) is called once per candidate inside the foreach. For a summit with 50 DA codes × 1,400 patterns each, this triggers 50 full rebuilds — each running 1,400 normalizations, a dedup pass, sort(), and sha1(). The performance gain described in the PR only applies when the lookup is built once per unique pattern-set and reused.
Fix: add a local patternsHash-keyed cache before calling build():
$lookupCache = [];
foreach ($candidates as $code) {
if (!$code instanceof IDomainAuthorizedPromoCode) continue;
$patterns = $code->getAllowedEmailDomains();
$hash = sha1(implode("\0", $patterns)); // stable, cheap pre-hash
$lookup = $lookupCache[$hash] ?? ($lookupCache[$hash] = $this->lookup_builder->build($patterns));
if ($code->matchesEmailDomainViaLookup($normalizedEmail, $lookup)) {
$matched[] = $code;
}
}This reduces N lookups to at most K unique pattern-sets.
There was a problem hiding this comment.
Fixed in d7cdc3b. SummitPromoCodeService.php:1042-1054 adds a $lookupCache keyed by sha1(implode("\0", $patterns)) per your suggested form. Caveat worth flagging: the pre-hash is order- and case-sensitive (no normalization before hashing), so two codes whose pattern arrays differ only in case or order won't share a cache entry. Acceptable for the OCP case where sibling codes are populated from the same upstream pattern array — but if the T1-Sanity measurement shows lower-than-expected hit rate, the fix is to normalize patterns before hashing.
| // valid_until_date_passed signals indexed-SQL self-termination per the | ||
| // SDS Verified Ops Precondition. | ||
| $duration_ms = (microtime(true) - $started) * 1000; | ||
| $now = new \DateTime(); |
There was a problem hiding this comment.
[BLOCKER] new \DateTime() without UTC → valid_until_date_passed wrong on non-UTC servers
The repository query (line 699) creates $now as new \DateTime('now', new \DateTimeZone('UTC')). This line uses new \DateTime() which inherits the PHP process system timezone. On any server where the default timezone is not UTC, $c->getValidUntilDate() < $now compares a UTC-stored DateTime against wall-clock local time — producing incorrect flag values and potentially triggering wrong ops self-termination signals.
Fix:
$now = new \DateTime('now', new \DateTimeZone('UTC'));There was a problem hiding this comment.
Fixed in d7cdc3b. SummitPromoCodeService.php:1083 now uses new \DateTime('now', new \DateTimeZone('UTC')) matching the repository at DoctrineSummitRegistrationPromoCodeRepository.php:699.
Note for the record: config/app.php:70 sets 'timezone' => 'UTC' and PHP \DateTime comparison normalizes via Unix timestamp regardless of TZ, so the runtime comparison result was already correct on this app. Applied as defensive consistency — matches the repo convention and future-proofs against any config drift.
| && $c->getValidUntilDate() !== null | ||
| && $c->getValidUntilDate() < $now, | ||
| true | ||
| ); |
There was a problem hiding this comment.
[BLOCKER] valid_until_date_passed has two semantic bugs
Bug A — empty candidates emits true
When $candidates is empty (no DA codes configured for the summit), the flag short-circuits to true. The SDS defines this flag as "all DA codes have passed their expiry window." Zero candidates means no DA codes exist at all — not that they expired. Ops tooling acting on true will incorrectly self-terminate DA-code processing for summits that never had any DA codes.
Fix: change the empty branch to false (or null if a third state is needed):
$valid_until_date_passed = empty($candidates)
? false // no DA codes configured — not the same as all expired
: array_reduce(...);Bug B — open-ended codes (null valid_until_date) force flag to false
The reduce requires $c->getValidUntilDate() !== null. Any open-ended code (perpetual, no expiry set) short-circuits the accumulator to false. A summit with one open-ended code and several fully-expired finite codes will emit valid_until_date_passed = false, hiding the fact that all finite codes are past their window.
Fix: compute the flag only over codes that have a non-null valid_until_date:
$finiteCandidates = array_filter($candidates, fn($c) => method_exists($c, 'getValidUntilDate') && $c->getValidUntilDate() !== null);
$valid_until_date_passed = empty($finiteCandidates)
? false
: array_reduce(
$finiteCandidates,
fn($acc, $c) => $acc && $c->getValidUntilDate() < $now,
true
);There was a problem hiding this comment.
Fixed in d7cdc3b. SummitPromoCodeService.php:1084-1094 applies your combined fix:
$finiteCandidates = array_filter(
$candidates,
fn($c) => method_exists($c, 'getValidUntilDate') && $c->getValidUntilDate() !== null
);
$valid_until_date_passed = empty($finiteCandidates)
? false
: array_reduce(
$finiteCandidates,
fn($acc, $c) => $acc && $c->getValidUntilDate() < $now,
true
);Filter to finite codes pre-enforces the non-null invariant so the reduce predicate simplifies. Empty-finite case defaults to false (resolves Bug A — no DA codes configured ≠ all expired). Open-ended codes no longer drag the accumulator to false (resolves Bug B).
| foreach ($candidates as $code) { | ||
| if (!$code instanceof IDomainAuthorizedPromoCode) continue; | ||
| $lookup = $this->lookup_builder->build($code->getAllowedEmailDomains()); | ||
| if ($code->matchesEmailDomainViaLookup($normalizedEmail, $lookup)) { |
There was a problem hiding this comment.
[FOLLOW-UP] Domain-non-matching DA code exclusion not tested at service level
All four unit tests in SummitPromoCodeServiceDiscoveryTest mock matchesEmailDomainViaLookup to return true. No test verifies that a DA code returned by the repository but not matching the member's email domain is excluded from results.
A regression in this build-and-filter path would silently expose all in-date DA codes to every member.
Suggested test:
public function testDomainNonMatchingDACodeIsExcluded(): void
{
// DA code with @acme.com pattern, member has @other.com email
$daCode = $this->buildDAPromoCode(allowedDomains: [@acme.com]); $this->repository->shouldReceive('getDomainAuthorizedDiscoverableForSummit')->andReturn([$daCode]); $this->repository->shouldReceive('getEmailLinkedDiscoverableForSummit')->andReturn([]); $member = $this->buildMember(email: 'user@other.com'); $result = $this->service->discoverPromoCodes($this->summit, $member); $this->assertEmpty($result); }Use real AllowedEmailDomainsLookupBuilder (already wired in buildService()) so the test exercises the actual lookup path, not a mock.
There was a problem hiding this comment.
Fixed in d7cdc3b. Added SummitPromoCodeServiceDiscoveryTest::testDomainNonMatchingDACodeIsExcluded — uses the real AllowedEmailDomainsLookupBuilder already wired into buildService() at L80 and lets the real matchesEmailDomainViaLookup trait method fire (via Mockery's makePartial()). Member email user@other.com against a code allowing @acme.com asserts the code is excluded from results. shouldNotReceive('setRemainingQuantityPerAccount') additionally proves the code is dropped at the email-match step, not later at the quantity loop.
Hot-path suite now 30/30 (Builder 12 + parity 11 + Discovery 7).
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@caseylocker please review
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.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-546/ This page is automatically updated on each push to this PR. |
2 participants
ref: https://app.clickup.com/t/86ba0b8tz
Summary
Ships Track 1 of the Domain-Authorized Promo Codes storage-and-lookup SDS per ftn-docsnsklz PR #15. No schema change, no Redis. Two new classes, one new trait method, a repository decouple that drops
$emailfrom the leaf signature and moves email filtering into the service, plus a single observability emit that becomes the Track-2 trigger instrumentation.models\summit\AllowedEmailDomainsLookup(final readonly DTO:exactSet/suffixList/patternsHash/unrestricted).App\Services\Model\AllowedEmailDomainsLookupBuilder(pure builder, no DB / cache / Doctrine).IDomainAuthorizedPromoCode::matchesEmailDomainViaLookup(string $email, AllowedEmailDomainsLookup $lookup): bool— O(1) exact-set + linear suffix scan. Parity-tested against legacymatchesEmailDomainfor every pattern shape and casing.getDomainAuthorizedDiscoverableForSummitdrops$emailand the PHP filter loop; returns raw date-windowed candidates. AggregatorgetDiscoverableByEmailForSummitrewritten with an inline legacy filter so its public by-email contract is preserved (marked@deprecatedfor Track 2 cleanup).SummitPromoCodeService::discoverPromoCodesrewritten — calls the two leaf methods directly, builds per-codeAllowedEmailDomainsLookupwith a pattern-hash-keyed cache so sibling codes sharing pattern arrays reuse a single normalized lookup, filters viamatchesEmailDomainViaLookup. Eliminates the ~7,000strtolower(trim())calls per OCP-volume discover request (~35 ms → sub-ms).Log::info('promo_code.discover.track1', [...])with exactly the 4 attrs the SDS specifies:summit_id,code_count,duration_ms,valid_until_date_passed. Thevalid_until_date_passedflag is the indexed-SQL self-termination signal JP's verified ops precondition relies on.Companion PR: fntechgit/summit-admin#947 (Tier 1 bulk-input frontend) — same prod push, week of 2026-05-19.
Out of scope (Track 2, decoupled — target branch cut 2026-06-18):
PromoCodeEmailDomainPatternchild table + migration, candidate-decomposition matcher (EmailDomainCandidateBuilder), batchedgetTicketCountsByMemberAndPromoCodes, trait getter/setter delegation. No Redis at any track under pass-4.ClickUp: https://app.clickup.com/t/86ba0b8tz
smarcet review pass — 2026-05-19 (commit
d7cdc3bb5)All 5 review findings applied in one commit on
discoverPromoCodes. Inline replies tying each fix to file:line landed on the original review threads.empty()guard (empty(" ") === false, so the empty-string flowed into the email-linked repo query). Guard now readsif (empty(trim($email))) return [];at L1034, mirroring the legacy aggregator's normalize-first order atDoctrineSummitRegistrationPromoCodeRepository.php:677.$lookupCachekeyed bysha1(implode("\0", $patterns))at L1042-1054 so sibling codes with byte-identical pattern arrays reuse a single built lookup. Pre-hash is order- and case-sensitive — acceptable for upstream-generated arrays; future T1-Sanity can surface lower-than-expected hit rate if a refinement to normalize patterns before hashing is needed.new \DateTime()not explicitly UTC. L1083 now usesnew \DateTime('now', new \DateTimeZone('UTC'))matching the repository at L699. Note:config/app.php:70sets'timezone' => 'UTC'and PHP\DateTimecomparison normalizes via Unix timestamp regardless of TZ, so the runtime behavior was already correct — change is defensive consistency.valid_until_date_passedtwo semantic bugs. L1084-1094 now filters$candidatesto a$finiteCandidatesset (non-nullgetValidUntilDate()); empty-finite case defaults tofalseinstead oftrue(resolves Bug A — zero DA codes ≠ all expired); open-ended codes no longer collapse the accumulator (resolves Bug B). The reduce predicate simplifies to$acc && $c->getValidUntilDate() < $nowsince the filter pre-enforces the invariants.matchesEmailDomainViaLookup→true. AddedtestDomainNonMatchingDACodeIsExcludedusing the realAllowedEmailDomainsLookupBuilder(already wired intobuildService()at L80) plus MockerymakePartial()so the realmatchesEmailDomainViaLookuptrait method fires. Also addedtestDiscoverReturnsEmptyForWhitespaceOnlyEmailwithshouldNotReceiveon both repo methods to pin the fix-1 guard.Test plan
vendor/bin/phpunitgreen for the 3 hot-path suites — 30/30 as ofd7cdc3bb5:tests/Unit/Services/AllowedEmailDomainsLookupBuilderTest.php(12/12)tests/Unit/Services/MatchesEmailDomainViaLookupTest.php(11/11)tests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.php(7/7 — was 5; +2 from smarcet review pass)matchesEmailDomain(calls (aggregator exempt):Track 1 staging sanity measurement (REQUIRED before prod merge)
Per SDS Task T1-Sanity — paste results here before approving.
valid_until_datein future, ~1,400 patterns, ~100%@domainper JP's OCP data).GET /api/v1/summits/{id}/promo-codes/all/discoveras a member whose email matches a pattern; tail laravel.log forpromo_code.discover.track1.duration_ms ≈ 35 ms,valid_until_date_passed = false,code_count = 1.valid_until_dateto the past, re-hit the endpoint; confirmvalid_until_date_passedflips totrueandcode_count → 0.If anything is unexpected (duration wildly higher than ~35 ms, or
valid_until_date_passeddoesn't flip), pause and escalate to the Track-2 owner — the Track-1 posture assumption is broken.Summary by CodeRabbit
New Features
Improvements
Tests