feat(promo-codes): domain-authorized promo codes for early registration access#525
Open
caseylocker wants to merge 22 commits intomainfrom
Open
feat(promo-codes): domain-authorized promo codes for early registration access#525caseylocker wants to merge 22 commits intomainfrom
caseylocker wants to merge 22 commits intomainfrom
Conversation
…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>
- 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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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>
- 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>
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>
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>
…ity_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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sion, 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds domain-authorized promo/discount code types with email-domain validation and per-account limits, adds an auto-apply flag across promo types, exposes a discover endpoint for authenticated members, enforces promo-code-only ticket purchases and per-account limits during reservation, and updates serializers, repository logic, migrations, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Authenticated Client
participant Controller as OAuth2 Controller
participant Service as PromoCodeService
participant Repository as PromoCodeRepo
participant DB as Database
Client->>Controller: GET /promo-codes/all/discover
Controller->>Controller: Load Summit & Current User
Controller->>Service: discoverPromoCodes(summit, member)
Service->>Repository: getDiscoverableByEmailForSummit(summit, email)
Repository->>DB: Query promo/discount codes by email/domain/subtype
DB-->>Repository: Return matching codes
Repository-->>Service: codes[]
loop Filter Each Code
Service->>Service: Check isLive() & hasQuantityAvailable()
Service->>Repository: getTicketCountByMemberAndPromoCode() (for domain-authorized)
Repository-->>Service: usedCount
Service->>Service: Enforce per-account limit & set remaining_quantity_per_account
end
Service-->>Controller: Filtered codes[]
Controller->>Controller: Serialize each code (expand/fields/relations)
Controller-->>Client: JSON {data: [], total, per_page, current_page, last_page}
sequenceDiagram
participant Order as OrderService
participant PreProcess as PreProcessReservationTask
participant Reserve as ReserveOrderTask
participant Apply as ApplyPromoCodeTask
participant Repo as PromoCodeRepo
participant Promo as PromoCode Model
Order->>PreProcess: run() - validate tickets
PreProcess->>PreProcess: Reject if isPromoCodeOnly() and no promo_code
PreProcess-->>Order: reservations state
Order->>Reserve: run() - reserve tickets
Reserve-->>Order: order & tickets created
Order->>Apply: run() - apply promo code
Apply->>Repo: Load promo code
Repo-->>Apply: PromoCode
Apply->>Promo: If IDomainAuthorizedPromoCode?
alt Domain-Authorized
Apply->>Repo: getTicketCountByMemberAndPromoCode()
Repo-->>Apply: usedCount
Apply->>Promo: Check usedCount < getQuantityPerAccount()
alt Over Limit
Apply-->>Order: ValidationException
else Under Limit
Apply->>Promo: Apply redemption
Apply-->>Order: Success
end
else Regular Code
Apply->>Promo: Apply redemption
Apply-->>Order: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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-525/ This page is automatically updated on each push to this PR. |
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
…ted 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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Services/Model/Imp/SummitOrderService.php (2)
284-296:⚠️ Potential issue | 🔴 CriticalThe saga state is dropped before
ApplyPromoCodeTask.After this reorder,
ApplyPromoCodeTask::run()no longer receivespromo_codes_usagebecauseReserveOrderTask::run()replaces the state with['order' => $order]. That means promo-code redemption/enforcement now runs with missing input, and any failure happens after the order was already persisted whileReserveOrderTask::undo()is still empty.🛠️ Minimal state fix
- return ['order' => $order]; + return array_merge($this->formerState, ['order' => $order]);Also applies to: 667-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Model/Imp/SummitOrderService.php` around lines 284 - 296, ReserveOrderTask::run() currently replaces the saga state (losing keys like promo_codes_usage) so ApplyPromoCodeTask::run() no longer receives required promo data; fix by preserving/merging state instead of overwriting (ensure ReserveOrderTask::run() returns array_merge($existingState, ['order' => $order]) or similar), or alternatively move the ApplyPromoCodeTask invocation to run before ReserveOrderTask so promo redemption happens prior to persisting the order; also apply the same fix at the other occurrence referenced (lines ~667-674) and ensure undo() implementations handle partial persistence.
728-742:⚠️ Potential issue | 🟠 MajorGuest/public reservations can bypass
QuantityPerAccount.This check is skipped whenever the saga was built with a null
$owner, butReserveOrderTaskcan later attach the order to an existing member viaowner_email. In that flow, the per-account limit is never enforced even though the order now belongs to an account. Resolve the owner from the reserved order (or byowner_email) before counting usage.Also applies to: 791-809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Model/Imp/SummitOrderService.php` around lines 728 - 742, The constructor currently stores a nullable $owner causing per-account checks (QuantityPerAccount) to be skipped; update the reserve flow so that before enforcing QuantityPerAccount (in ReserveOrderTask and the methods around the QuantityPerAccount check referenced at lines ~791-809) you resolve the actual owner from the reserved order (or by owner_email) instead of relying on the initial $this->owner being non-null; specifically, locate the QuantityPerAccount enforcement code and change it to obtain the owner via the reserved Order object (or lookup by owner_email) and then perform the per-account usage count against that resolved Member before allowing the reservation to proceed.
🧹 Nitpick comments (10)
app/Models/Foundation/Summit/Summit.php (1)
5555-5568: Keep promo-code visibility separate from purchase authorization.Returning
truehere meansmodels\summit\Summit::canBuyRegistrationTicketByType()no longer answers “can this email buy this ticket type?” formodels\summit\SummitTicketType::Audience_With_Promo_Code; it only says the ticket is not invitation-gated. That makes the helper easy to reuse incorrectly later and accidentally bypass the promo-code requirement. Consider threading promo-code context into this check, or splitting visibility from authorization into separate helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Summit/Summit.php` around lines 5555 - 5568, The current Summit::canBuyRegistrationTicketByType() unconditionally returns true for SummitTicketType::Audience_With_Promo_Code which conflates visibility with purchase authorization; instead remove the unconditional return and either (a) add a promo-context parameter (e.g. $promoCodeProvided or $promoContext) to canBuyRegistrationTicketByType and check promo validity before granting purchase, or (b) split responsibilities by creating a separate visibility helper (e.g. isTicketTypeVisibleToEmail or canViewTicketType) that returns true for Audience_With_Promo_Code while leaving canBuyRegistrationTicketByType to enforce promo-code checks; update callers to pass the promo context or use the new visibility helper as appropriate.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php (1)
1585-1602: OpenAPI annotation missingfieldsandrelationsquery parameters.The implementation at lines 1617-1622 reads
fieldsandrelationsfrom the request, but the OpenAPI annotation only documents theexpandparameter. Consider adding the missing parameters for complete API documentation.parameters: [ new OA\Parameter(name: "id", in: "path", required: true, schema: new OA\Schema(type: "integer")), new OA\Parameter(name: "expand", in: "query", required: false, schema: new OA\Schema(type: "string")), + new OA\Parameter(name: "fields", in: "query", required: false, schema: new OA\Schema(type: "string")), + new OA\Parameter(name: "relations", in: "query", required: false, schema: new OA\Schema(type: "string")), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php` around lines 1585 - 1602, The OpenAPI annotation for the controller method (the OA\Get block above discoverPromoCodesBySummit in OAuth2SummitPromoCodesApiController) is missing the query parameters used in the implementation; add two OA\Parameter entries for "fields" and "relations" (both in="query", required=false, schema type="string") with brief descriptions (e.g., "Comma-separated list of fields to include" and "Comma-separated list of relations to expand") so the documented parameters match the code that reads fields and relations from the request.app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php (1)
668-721: Discovery filtering logic is correct but has minor duplication.The method correctly:
- Normalizes email to lowercase
- Uses
INSTANCE OFto fetch only discoverable code types- Applies type-specific matching (domain vs. owner email)
- Checks
isLive()for all codesThe member and speaker code blocks (lines 702-716) share identical logic. Consider extracting to reduce duplication:
♻️ Optional refactor to reduce duplication
- // Email-linked types: match by associated member/speaker email - if ($code instanceof MemberSummitRegistrationPromoCode || $code instanceof MemberSummitRegistrationDiscountCode) { - $ownerEmail = $code->getOwnerEmail(); - if (!empty($ownerEmail) && strtolower($ownerEmail) === $email && $code->isLive()) { - $results[] = $code; - } - continue; - } - - if ($code instanceof SpeakerSummitRegistrationPromoCode || $code instanceof SpeakerSummitRegistrationDiscountCode) { - $ownerEmail = $code->getOwnerEmail(); - if (!empty($ownerEmail) && strtolower($ownerEmail) === $email && $code->isLive()) { - $results[] = $code; - } - continue; - } + // Email-linked types: match by associated member/speaker email + if (method_exists($code, 'getOwnerEmail')) { + $ownerEmail = $code->getOwnerEmail(); + if (!empty($ownerEmail) && strtolower($ownerEmail) === $email && $code->isLive()) { + $results[] = $code; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php` around lines 668 - 721, The member and speaker owner-email checks inside getDiscoverableByEmailForSummit are duplicated; extract the shared logic into a small private helper (e.g., private function isOwnerEmailMatch($code, string $email): bool) that reads $code->getOwnerEmail(), lowercases and compares to $email and checks $code->isLive(), then replace the two separate blocks (the checks against MemberSummitRegistrationPromoCode, MemberSummitRegistrationDiscountCode and against SpeakerSummitRegistrationPromoCode, SpeakerSummitRegistrationDiscountCode) with either a single instanceof-any conditional that calls the helper or two concise calls to the helper, leaving domain-authorized logic unchanged.app/Rules/AllowedEmailDomainsArray.php (1)
70-73:trans()used with literal string instead of translation key.The
trans()function expects a translation key, not a literal message. As written, it will return the string as-is. For proper i18n support:- return trans('The :attribute must be an array of valid email domain patterns (`@domain.com`, .tld, or user@example.com).'); + return 'The :attribute must be an array of valid email domain patterns (`@domain.com`, .tld, or user@example.com).';Or define a proper translation key if i18n is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Rules/AllowedEmailDomainsArray.php` around lines 70 - 73, The message() method in AllowedEmailDomainsArray currently calls trans() with a literal string; replace it to use a translation key (e.g. use __('validation.allowed_email_domains_array') or trans('validation.allowed_email_domains_array')) in the message() method and add the corresponding key/value in your language files (resources/lang/{locale}/validation.php) so i18n works correctly; reference: AllowedEmailDomainsArray::message().app/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCodeSerializer.php (1)
21-48: Consider addingexpand_mappingsforallowed_ticket_types.The promo code model includes an
allowed_ticket_typesrelationship (inherited fromSummitRegistrationPromoCode), and the parallelDomainAuthorizedSummitRegistrationDiscountCodeSerializerincludesexpand_mappingsfor this field. For API consistency, add the same mapping to the promo code serializer:+ protected static $expand_mappings = [ + 'allowed_ticket_types' => [ + 'type' => \Libs\ModelSerializers\Many2OneExpandSerializer::class, + 'getter' => 'getAllowedTicketTypes', + ], + ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCodeSerializer.php` around lines 21 - 48, The serializer DomainAuthorizedSummitRegistrationPromoCodeSerializer is missing an expand_mappings entry for the inherited allowed_ticket_types relation; add a protected static $expand_mappings property (matching the pattern used in DomainAuthorizedSummitRegistrationDiscountCodeSerializer) that maps 'allowed_ticket_types' to the appropriate expansion key/serializer (e.g., the allowed_ticket_types expansion that returns the ticket types collection using the same serializer used elsewhere for ticket types), so API consumers can expand allowed_ticket_types when calling the serialize method.app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.php (1)
122-134: Add PHPDoc documentation for$companyparameter — it's part of the interface contract across all promo code implementations.The
$companyparameter is unused in this method but is retained for consistency with the base class signatureSummitRegistrationPromoCode::checkSubject(), which is polymorphically implemented across all promo code trait variants (Member, Speaker, Speakers, Sponsor, DomainAuthorized). Add a brief PHPDoc note explaining its retention for interface compatibility:/** * `@param` string $email * `@param` null|string $company Retained for interface compatibility; not used in domain-authorized validation * `@return` bool * `@throws` ValidationException */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.php` around lines 122 - 134, Add a PHPDoc block to the DomainAuthorizedPromoCodeTrait::checkSubject method documenting the unused $company parameter for interface compatibility; update the docblock to include `@param` string $email, `@param` null|string $company Retained for interface compatibility; not used in domain-authorized validation, `@return` bool and `@throws` ValidationException so the method signature matches the SummitRegistrationPromoCode::checkSubject contract and clarifies why $company is intentionally unused.app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCode.php (1)
51-58: This override doesn't implement the documented audience bypass.Right now it only forwards to
parent::addAllowedTicketType(), so the “regardless of audience value” behavior in the comment is not actually expressed here. Either remove the override/comment or put the intended bypass logic in this method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCode.php` around lines 51 - 58, The override addAllowedTicketType in DomainAuthorizedSummitRegistrationPromoCode currently just calls parent::addAllowedTicketType and does not implement the claimed "regardless of audience" behavior; either remove this override and its comment, or implement the bypass by directly adding the SummitTicketType to this promo's allowed ticket collection without performing the parent's audience check (e.g., perform the collection add/update inside addAllowedTicketType on DomainAuthorizedSummitRegistrationPromoCode rather than calling parent::addAllowedTicketType). Ensure you update or remove the doc comment to match the actual behavior.tests/oauth2/OAuth2SummitPromoCodesApiTest.php (1)
1085-1116: These skipped checkout tests won't catch regressions.Right now the new quantity-per-account checkout path has no protection from this file because all three enforcement tests are permanently skipped. If the full OAuth/saga harness is too heavy, a narrower test around
ApplyPromoCodeTask::run()would still cover the new guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oauth2/OAuth2SummitPromoCodesApiTest.php` around lines 1085 - 1116, The three checkout tests in OAuth2SummitPromoCodesApiTest.php are skipped and leave the new quantity-per-account enforcement untested; add focused unit tests that call ApplyPromoCodeTask::run() directly (instead of the full saga) to cover over-limit, under-limit, and basic concurrency behavior: create tests that construct an ApplyPromoCodeTask with mocked dependencies (e.g., promo repository, order repository/count query, and any lock/persistence layer), stub the member's existing redemptions count to simulate over-limit and under-limit scenarios and assert the task throws the expected rejection or completes successfully, and for concurrency simulate the reservation/count behavior via mocks to assert the pessimistic-lock path; place these tests in OAuth2SummitPromoCodesApiTest.php replacing the skipped methods (testCheckoutRejectsOverLimitQuantityPerAccount, testCheckoutSucceedsUnderLimitQuantityPerAccount, testCheckoutConcurrentEnforcement) so ApplyPromoCodeTask::run() is directly exercised.tests/Unit/Services/DomainAuthorizedPromoCodeTest.php (1)
291-305: Vacuously true assertion reduces test value.Line 304 asserts that id
99is not in the result, but no ticket type with id99was created in this test. The assertion will always pass regardless of the actual filtering behavior.To strengthen this test, either:
- Remove the misleading assertion about id 99 (the test still proves the strategy returns results via the id 30 check), or
- Add a WithPromoCode ticket type to the promo code mock and verify the strategy correctly excludes it when no promo code is provided.
♻️ Suggested fix — remove misleading assertion
$ids = array_map(fn($tt) => $tt->getId(), $result); // Audience_All type IS returned (non-vacuous: proves the strategy produces results) $this->assertContains(30, $ids, 'Audience_All ticket type should be returned without a promo code'); - // WithPromoCode type (id 99) is NOT returned — it only lives in promo_code->getAllowedTicketTypes() - $this->assertNotContains(99, $ids, 'WithPromoCode ticket types should not be returned without a promo code');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Services/DomainAuthorizedPromoCodeTest.php` around lines 291 - 305, The assertion is vacuous because no ticket type with id 99 exists in this test; update the testWithPromoCodeAudienceNoPromoCodeNotReturned to actually create a "WithPromoCode" ticket type (e.g., via buildMockTicketType(99, SummitTicketType::Audience_Promo) or similar) and attach it only to a mocked promo code's getAllowedTicketTypes() so that RegularPromoCodeTicketTypesStrategy($summit, $member, null)->getTicketTypes() has both id 30 and id 99 available in the source data and you can assert that 30 is returned while 99 is not; alternatively, if you prefer the simpler change, remove the misleading assertNotContains(99, $ids) line entirely to avoid the vacuous check.app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php (1)
53-60: Remove this no-op override or clarify intent with accurate documentation.The parent
SummitRegistrationPromoCode::addAllowedTicketType()contains no audience-based restrictions—it simply adds the ticket type to the collection. This override merely delegates to the parent without any custom logic, making it redundant. The docblock's claim that it "allows any ticket type regardless of audience value" is misleading, as the parent already permits any ticket type. Either remove the override or, if it serves a semantic purpose in the inheritance hierarchy, update the docblock to accurately reflect that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php` around lines 53 - 60, The addAllowedTicketType method in DomainAuthorizedSummitRegistrationDiscountCode is a redundant no-op that only calls parent::addAllowedTicketType (the parent SummitRegistrationPromoCode already permits any ticket type); either remove this override entirely to avoid dead code, or if you want an explicit semantic override, keep the method but replace the misleading docblock with a concise note like "Intentional passthrough to parent to signal inherited behavior" and ensure the method body remains a direct parent call (referencing DomainAuthorizedSummitRegistrationDiscountCode::addAllowedTicketType and SummitRegistrationPromoCode::addAllowedTicketType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Foundation/Summit/Factories/SummitPromoCodeFactory.php`:
- Around line 295-296: Normalize the allowed_email_domains input to an array
before calling the typed setter setAllowedEmailDomains: if
$data['allowed_email_domains'] is a string, split on commas (or another
delimiter used by CSV), trim each entry and remove empty values so you always
pass an array to SummitPromoCodeFactory::setAllowedEmailDomains; apply the same
normalization pattern for the other CSV-driven fields that expect arrays (the
other typed setters in this factory, e.g., setAllowedTicketTypes /
setTicketTypesRules or any similar setter calls around the nearby block) so
string CSV values don't throw and get silently skipped.
- Around line 201-202: Replace uses of boolval(...) for the auto_apply field
with filter_var($data['auto_apply'], FILTER_VALIDATE_BOOLEAN,
FILTER_NULL_ON_FAILURE) so string values like "false" are parsed correctly;
specifically update every branch that calls
$promo_code->setAutoApply(boolval($data['auto_apply'])) (all occurrences
handling auto_apply in SummitPromoCodeFactory.php) to first compute the boolean
via filter_var and then pass that result to $promo_code->setAutoApply, applying
the same change to each auto_apply branch mentioned (the multiple setAutoApply
call sites).
In
`@app/ModelSerializers/Summit/Registration/PromoCodes/MemberSummitRegistrationDiscountCodeSerializer.php`:
- Line 98: Replace the hardcoded null assignment to
$values['remaining_quantity_per_account'] with the actual value from the
domain-authorized discount model (e.g. call the model's getter such as
getRemainingQuantityPerAccount() on the serialized object or
$domainAuthorizedDiscount/$this->object) and only fall back to null if that
getter returns an unset/empty value; update
MemberSummitRegistrationDiscountCodeSerializer to read and serialize the real
remaining-per-account value instead of forcing null.
In `@app/Rules/AllowedEmailDomainsArray.php`:
- Around line 46-58: The .tld branch in AllowedEmailDomainsArray (the elseif
that checks str_starts_with($element, '.')) uses a regex that only allows a
single label after the leading dot; update that regex to accept multi-level
suffixes by matching a leading dot followed by one label and then zero or more
additional ".label" segments (i.e., allow repeated dot-separated labels) so
patterns like ".co.uk" or ".ac.jp" are accepted; replace the existing /^\.\w+$/
pattern in that branch accordingly.
In `@database/migrations/model/Version20260401150000.php`:
- Around line 99-117: In the down() migration, before reverting the ClassName
ENUM on SummitRegistrationPromoCode, remove or update any rows that reference
the domain-authorized discriminators so the ALTER TABLE won't fail;
specifically, after dropping the joined tables
DomainAuthorizedSummitRegistrationPromoCode and
DomainAuthorizedSummitRegistrationDiscountCode, delete (or UPDATE ClassName to a
valid value) any SummitRegistrationPromoCode rows whose ClassName equals the
domain-authorized values (e.g., the discriminator values introduced for
domain-authorized promo/discount codes) and only then run the ALTER TABLE ...
MODIFY ClassName ENUM(...) DEFAULT 'SummitRegistrationPromoCode' to narrow the
enum safely.
In `@routes/api_v1.php`:
- Around line 1953-1956: The discover route is currently rate-limited but
unauthenticated; update the route definition for the 'all/discover' endpoint to
include the authentication middleware so only authenticated users can call
OAuth2SummitPromoCodesApiController@discover. Modify the middleware array on the
Route::get for 'discover' to add 'auth.user' alongside 'rate.limit:25,1' (e.g.,
['middleware' => ['auth.user', 'rate.limit:25,1']]) so the route enforces
authentication at the routing layer before controller execution.
---
Outside diff comments:
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Around line 284-296: ReserveOrderTask::run() currently replaces the saga state
(losing keys like promo_codes_usage) so ApplyPromoCodeTask::run() no longer
receives required promo data; fix by preserving/merging state instead of
overwriting (ensure ReserveOrderTask::run() returns array_merge($existingState,
['order' => $order]) or similar), or alternatively move the ApplyPromoCodeTask
invocation to run before ReserveOrderTask so promo redemption happens prior to
persisting the order; also apply the same fix at the other occurrence referenced
(lines ~667-674) and ensure undo() implementations handle partial persistence.
- Around line 728-742: The constructor currently stores a nullable $owner
causing per-account checks (QuantityPerAccount) to be skipped; update the
reserve flow so that before enforcing QuantityPerAccount (in ReserveOrderTask
and the methods around the QuantityPerAccount check referenced at lines
~791-809) you resolve the actual owner from the reserved order (or by
owner_email) instead of relying on the initial $this->owner being non-null;
specifically, locate the QuantityPerAccount enforcement code and change it to
obtain the owner via the reserved Order object (or lookup by owner_email) and
then perform the per-account usage count against that resolved Member before
allowing the reservation to proceed.
---
Nitpick comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php`:
- Around line 1585-1602: The OpenAPI annotation for the controller method (the
OA\Get block above discoverPromoCodesBySummit in
OAuth2SummitPromoCodesApiController) is missing the query parameters used in the
implementation; add two OA\Parameter entries for "fields" and "relations" (both
in="query", required=false, schema type="string") with brief descriptions (e.g.,
"Comma-separated list of fields to include" and "Comma-separated list of
relations to expand") so the documented parameters match the code that reads
fields and relations from the request.
In
`@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.php`:
- Around line 122-134: Add a PHPDoc block to the
DomainAuthorizedPromoCodeTrait::checkSubject method documenting the unused
$company parameter for interface compatibility; update the docblock to include
`@param` string $email, `@param` null|string $company Retained for interface
compatibility; not used in domain-authorized validation, `@return` bool and
`@throws` ValidationException so the method signature matches the
SummitRegistrationPromoCode::checkSubject contract and clarifies why $company is
intentionally unused.
In
`@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php`:
- Around line 53-60: The addAllowedTicketType method in
DomainAuthorizedSummitRegistrationDiscountCode is a redundant no-op that only
calls parent::addAllowedTicketType (the parent SummitRegistrationPromoCode
already permits any ticket type); either remove this override entirely to avoid
dead code, or if you want an explicit semantic override, keep the method but
replace the misleading docblock with a concise note like "Intentional
passthrough to parent to signal inherited behavior" and ensure the method body
remains a direct parent call (referencing
DomainAuthorizedSummitRegistrationDiscountCode::addAllowedTicketType and
SummitRegistrationPromoCode::addAllowedTicketType).
In
`@app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCode.php`:
- Around line 51-58: The override addAllowedTicketType in
DomainAuthorizedSummitRegistrationPromoCode currently just calls
parent::addAllowedTicketType and does not implement the claimed "regardless of
audience" behavior; either remove this override and its comment, or implement
the bypass by directly adding the SummitTicketType to this promo's allowed
ticket collection without performing the parent's audience check (e.g., perform
the collection add/update inside addAllowedTicketType on
DomainAuthorizedSummitRegistrationPromoCode rather than calling
parent::addAllowedTicketType). Ensure you update or remove the doc comment to
match the actual behavior.
In `@app/Models/Foundation/Summit/Summit.php`:
- Around line 5555-5568: The current Summit::canBuyRegistrationTicketByType()
unconditionally returns true for SummitTicketType::Audience_With_Promo_Code
which conflates visibility with purchase authorization; instead remove the
unconditional return and either (a) add a promo-context parameter (e.g.
$promoCodeProvided or $promoContext) to canBuyRegistrationTicketByType and check
promo validity before granting purchase, or (b) split responsibilities by
creating a separate visibility helper (e.g. isTicketTypeVisibleToEmail or
canViewTicketType) that returns true for Audience_With_Promo_Code while leaving
canBuyRegistrationTicketByType to enforce promo-code checks; update callers to
pass the promo context or use the new visibility helper as appropriate.
In
`@app/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCodeSerializer.php`:
- Around line 21-48: The serializer
DomainAuthorizedSummitRegistrationPromoCodeSerializer is missing an
expand_mappings entry for the inherited allowed_ticket_types relation; add a
protected static $expand_mappings property (matching the pattern used in
DomainAuthorizedSummitRegistrationDiscountCodeSerializer) that maps
'allowed_ticket_types' to the appropriate expansion key/serializer (e.g., the
allowed_ticket_types expansion that returns the ticket types collection using
the same serializer used elsewhere for ticket types), so API consumers can
expand allowed_ticket_types when calling the serialize method.
In `@app/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.php`:
- Around line 668-721: The member and speaker owner-email checks inside
getDiscoverableByEmailForSummit are duplicated; extract the shared logic into a
small private helper (e.g., private function isOwnerEmailMatch($code, string
$email): bool) that reads $code->getOwnerEmail(), lowercases and compares to
$email and checks $code->isLive(), then replace the two separate blocks (the
checks against MemberSummitRegistrationPromoCode,
MemberSummitRegistrationDiscountCode and against
SpeakerSummitRegistrationPromoCode, SpeakerSummitRegistrationDiscountCode) with
either a single instanceof-any conditional that calls the helper or two concise
calls to the helper, leaving domain-authorized logic unchanged.
In `@app/Rules/AllowedEmailDomainsArray.php`:
- Around line 70-73: The message() method in AllowedEmailDomainsArray currently
calls trans() with a literal string; replace it to use a translation key (e.g.
use __('validation.allowed_email_domains_array') or
trans('validation.allowed_email_domains_array')) in the message() method and add
the corresponding key/value in your language files
(resources/lang/{locale}/validation.php) so i18n works correctly; reference:
AllowedEmailDomainsArray::message().
In `@tests/oauth2/OAuth2SummitPromoCodesApiTest.php`:
- Around line 1085-1116: The three checkout tests in
OAuth2SummitPromoCodesApiTest.php are skipped and leave the new
quantity-per-account enforcement untested; add focused unit tests that call
ApplyPromoCodeTask::run() directly (instead of the full saga) to cover
over-limit, under-limit, and basic concurrency behavior: create tests that
construct an ApplyPromoCodeTask with mocked dependencies (e.g., promo
repository, order repository/count query, and any lock/persistence layer), stub
the member's existing redemptions count to simulate over-limit and under-limit
scenarios and assert the task throws the expected rejection or completes
successfully, and for concurrency simulate the reservation/count behavior via
mocks to assert the pessimistic-lock path; place these tests in
OAuth2SummitPromoCodesApiTest.php replacing the skipped methods
(testCheckoutRejectsOverLimitQuantityPerAccount,
testCheckoutSucceedsUnderLimitQuantityPerAccount,
testCheckoutConcurrentEnforcement) so ApplyPromoCodeTask::run() is directly
exercised.
In `@tests/Unit/Services/DomainAuthorizedPromoCodeTest.php`:
- Around line 291-305: The assertion is vacuous because no ticket type with id
99 exists in this test; update the
testWithPromoCodeAudienceNoPromoCodeNotReturned to actually create a
"WithPromoCode" ticket type (e.g., via buildMockTicketType(99,
SummitTicketType::Audience_Promo) or similar) and attach it only to a mocked
promo code's getAllowedTicketTypes() so that
RegularPromoCodeTicketTypesStrategy($summit, $member, null)->getTicketTypes()
has both id 30 and id 99 available in the source data and you can assert that 30
is returned while 99 is not; alternatively, if you prefer the simpler change,
remove the misleading assertNotContains(99, $ids) line entirely to avoid the
vacuous check.
🪄 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: e384ac84-d886-48f3-8deb-ae6c4bca21ae
📒 Files selected for processing (38)
app/Http/Controllers/Apis/Protected/Summit/Factories/Registration/PromoCodesValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.phpapp/ModelSerializers/SerializerRegistry.phpapp/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCodeSerializer.phpapp/ModelSerializers/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCodeSerializer.phpapp/ModelSerializers/Summit/Registration/PromoCodes/MemberSummitRegistrationDiscountCodeSerializer.phpapp/ModelSerializers/Summit/Registration/PromoCodes/MemberSummitRegistrationPromoCodeSerializer.phpapp/ModelSerializers/Summit/Registration/PromoCodes/SpeakerSummitRegistrationDiscountCodeSerializer.phpapp/ModelSerializers/Summit/Registration/PromoCodes/SpeakerSummitRegistrationPromoCodeSerializer.phpapp/Models/Foundation/Summit/Factories/SummitPromoCodeFactory.phpapp/Models/Foundation/Summit/Registration/PromoCodes/AutoApplyPromoCodeTrait.phpapp/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedPromoCodeTrait.phpapp/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationPromoCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/IDomainAuthorizedPromoCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/MemberSummitRegistrationDiscountCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/MemberSummitRegistrationPromoCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/PromoCodesConstants.phpapp/Models/Foundation/Summit/Registration/PromoCodes/SpeakerSummitRegistrationDiscountCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/SpeakerSummitRegistrationPromoCode.phpapp/Models/Foundation/Summit/Registration/PromoCodes/Strategies/RegularPromoCodeTicketTypesStrategy.phpapp/Models/Foundation/Summit/Registration/PromoCodes/SummitRegistrationPromoCode.phpapp/Models/Foundation/Summit/Registration/SummitTicketType.phpapp/Models/Foundation/Summit/Repositories/ISummitRegistrationPromoCodeRepository.phpapp/Models/Foundation/Summit/Summit.phpapp/Repositories/Summit/DoctrineSummitRegistrationPromoCodeRepository.phpapp/Rules/AllowedEmailDomainsArray.phpapp/Services/Model/ISummitPromoCodeService.phpapp/Services/Model/Imp/SummitOrderService.phpapp/Services/Model/Imp/SummitPromoCodeService.phpdatabase/migrations/model/Version20260401150000.phpdatabase/seeders/ApiEndpointsSeeder.phpdoc/promo-codes-for-early-registration-access.mdroutes/api_v1.phptests/Unit/Services/DomainAuthorizedPromoCodeTest.phptests/Unit/Services/PreProcessReservationTaskTest.phptests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.phptests/oauth2/OAuth2SummitPromoCodesApiTest.php
...erializers/Summit/Registration/PromoCodes/MemberSummitRegistrationDiscountCodeSerializer.php
Show resolved
Hide resolved
… 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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Services/Model/Imp/SummitPromoCodeService.php (1)
1031-1054: Batch per-code usage lookups for discovery.
getDiscoverableByEmailForSummit()loads the candidate codes, and then each domain-authorized result triggers anothergetTicketCountByMemberAndPromoCode()call. On a summit with many matching codes, this becomes a 1+N query pattern on the registration hot path. Consider a repository method that returns usage counts keyed by promo-code ID for the member.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Model/Imp/SummitPromoCodeService.php` around lines 1031 - 1054, The loop in SummitPromoCodeService uses getDiscoverableByEmailForSummit() then calls getTicketCountByMemberAndPromoCode() per code, causing 1+N queries; add a repository method such as getTicketCountsByMemberAndPromoCodes($member, array $promoCodeIds) that returns a map promoCodeId=>usedCount, call it once after retrieving $codes, then in the foreach over $codes (same loop referencing IDomainAuthorizedPromoCode, getQuantityPerAccount, setRemainingQuantityPerAccount) read the used count from that map to decide exhaustion and set remaining per-account quantity instead of making per-code repository calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/Model/Imp/SummitPromoCodeService.php`:
- Around line 645-647: The CSV import currently uses isset() then explode('|',
$row['allowed_email_domains']) which yields [''] for blank cells and leaves
whitespace—update both occurrences handling allowed_email_domains in
SummitPromoCodeService.php (the two blocks around the shown diff and the block
at the other path mentioned) to: after exploding by '|' trim each element and
remove any empty strings (e.g., array_map('trim', ...) followed by
array_filter(...) or equivalent) so the resulting $row['allowed_email_domains']
is an array of non-empty, trimmed domains or omitted if none remain.
In `@database/migrations/model/Version20260401150000.php`:
- Around line 103-107: Replace the hard DELETE that removes base-table rows from
SummitRegistrationPromoCode with a safe UPDATE that remaps the discriminator to
the base ClassName so rows (and their IDs/foreign links) are preserved;
specifically modify the SQL in Version20260401150000.php that targets ClassName
IN
('DomainAuthorizedSummitRegistrationDiscountCode','DomainAuthorizedSummitRegistrationPromoCode')
to run an UPDATE setting ClassName = 'SummitRegistrationPromoCode' (or the
appropriate surviving base type) instead of DELETE, ensuring child table cascade
behavior and external FK integrity remain intact.
---
Nitpick comments:
In `@app/Services/Model/Imp/SummitPromoCodeService.php`:
- Around line 1031-1054: The loop in SummitPromoCodeService uses
getDiscoverableByEmailForSummit() then calls
getTicketCountByMemberAndPromoCode() per code, causing 1+N queries; add a
repository method such as getTicketCountsByMemberAndPromoCodes($member, array
$promoCodeIds) that returns a map promoCodeId=>usedCount, call it once after
retrieving $codes, then in the foreach over $codes (same loop referencing
IDomainAuthorizedPromoCode, getQuantityPerAccount,
setRemainingQuantityPerAccount) read the used count from that map to decide
exhaustion and set remaining per-account quantity instead of making per-code
repository calls.
🪄 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: 629c3bf1-4fa4-415f-bf98-f0555ef818a6
📒 Files selected for processing (2)
app/Services/Model/Imp/SummitPromoCodeService.phpdatabase/migrations/model/Version20260401150000.php
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
…n 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/Model/Imp/SummitPromoCodeService.php (1)
1037-1056: Push discovery availability checks down into the repository.
discoverPromoCodes()now compensates forgetDiscoverableByEmailForSummit()by post-filtering exhausted codes and then issuing one ticket-count lookup per returned domain-authorized code. Folding those predicates into the repository would keep discovery to one query path and avoid O(N) follow-up calls as matching code volume grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Model/Imp/SummitPromoCodeService.php` around lines 1037 - 1056, discoverPromoCodes() is doing post-filtering for global exhaustion and per-account remaining quantity by iterating results from getDiscoverableByEmailForSummit() and calling getTicketCountByMemberAndPromoCode() for each IDomainAuthorizedPromoCode; move those predicates into the repository to avoid O(N) follow-ups. Update getDiscoverableByEmailForSummit() to (a) exclude globally exhausted codes (apply hasQuantityAvailable semantics in the query), and (b) for domain-authorized promo codes compute remaining per-account quantity in the same query (via a join/subquery that counts tickets per member) and return that value (or include it in the returned entity/DTO) so discoverPromoCodes() no longer needs to call getTicketCountByMemberAndPromoCode() or call setRemainingQuantityPerAccount() itself; keep discoverPromoCodes() logic but remove the post-filtering loop that checks hasQuantityAvailable() and quantityPerAccount usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/migrations/model/Version20260401150000.php`:
- Around line 94-97: The rollback widens restricted inventory and codes by
setting SummitTicketType.Audience = 'All' and altering enums for promo/discount
code tables; instead, modify the down() migration to first detect any rows that
would lose restrictions (e.g., SummitTicketType rows where
Audience='WithPromoCode' and promo/discount code rows with domain or invitation
constraints), and if any exist, abort the down() with an explicit
exception/error indicating manual cleanup required, or alternatively move those
rows to a designated inactive table/flag for manual migration; update the down()
implementation that touches SummitTicketType and the promo/discount code enum
revert logic to perform this pre-check and fail-fast or relocate restricted rows
rather than broadening access.
---
Nitpick comments:
In `@app/Services/Model/Imp/SummitPromoCodeService.php`:
- Around line 1037-1056: discoverPromoCodes() is doing post-filtering for global
exhaustion and per-account remaining quantity by iterating results from
getDiscoverableByEmailForSummit() and calling
getTicketCountByMemberAndPromoCode() for each IDomainAuthorizedPromoCode; move
those predicates into the repository to avoid O(N) follow-ups. Update
getDiscoverableByEmailForSummit() to (a) exclude globally exhausted codes (apply
hasQuantityAvailable semantics in the query), and (b) for domain-authorized
promo codes compute remaining per-account quantity in the same query (via a
join/subquery that counts tickets per member) and return that value (or include
it in the returned entity/DTO) so discoverPromoCodes() no longer needs to call
getTicketCountByMemberAndPromoCode() or call setRemainingQuantityPerAccount()
itself; keep discoverPromoCodes() logic but remove the post-filtering loop that
checks hasQuantityAvailable() and quantityPerAccount usage.
🪄 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: 3c17ca05-176f-485e-9fee-749764e0c825
📒 Files selected for processing (2)
app/Services/Model/Imp/SummitPromoCodeService.phpdatabase/migrations/model/Version20260401150000.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
smarcet
reviewed
Apr 12, 2026
|
|
||
| $codes = $this->promo_code_service->discoverPromoCodes($summit, $current_member); | ||
|
|
||
| $expand = Request::input('expand', ''); |
Collaborator
There was a problem hiding this comment.
@caseylocker this not follow api patterns please check other controllers to see how we are doing it
SerializerUtils::getExpand(),
SerializerUtils::getFields(),
SerializerUtils::getRelations()
smarcet
reviewed
Apr 12, 2026
| return Saga::start() | ||
| ->addTask(new PreOrderValidationTask($summit, $payload, $this->ticket_type_repository, $this->tx_service)) | ||
| ->addTask(new PreProcessReservationTask($summit, $payload)) | ||
| ->addTask(new PreProcessReservationTask($summit, $payload, $owner, $this->promo_code_repository)) |
Collaborator
There was a problem hiding this comment.
@caseylocker this need to be reviewed
what is the rationale of this change ?
seems to me a breaking change without a clear regression test
Before this PR — saga order was safe
PreOrderValidationTask
→ PreProcessReservationTask (validates ticket types, aggregates promo code usage)
→ ReserveTicketsTask (decrements ticket type inventory with locks)
→ ApplyPromoCodeTask (validates checkSubject, canBeAppliedTo, adds usage)
→ ReserveOrderTask (creates Order + Tickets in DB, applies discount via applyTo)
If ApplyPromoCodeTask threw (e.g. checkSubject rejects the user's email domain), the saga compensation ran:
- ApplyPromoCodeTask::undo() ( nothing to undo ,no usage was added)
- ReserveTicketsTask::undo() (restores ticket type quantities)
- PreProcessReservationTask::undo() (no-op)
- PreOrderValidationTask::undo() (no-op)
No order existed yet. ReserveOrderTask never ran, so its empty undo() was irrelevant. Clean failure.
Contributor
Author
There was a problem hiding this comment.
@smarcet you're right that the rationale wasn't clear and there was no regression test — apologies. Pushed c4bcdef which addresses all three of your comments.
Saga reorder rationale (deviation D4 in the PR description): the pre-existing order had a TOCTOU gap on QuantityPerAccount. DoctrineSummitRegistrationPromoCodeRepository::getTicketCountByMemberAndPromoCode filters on o.Status IN ('Reserved', 'Paid', 'Confirmed'), so running ApplyPromoCodeTask before ReserveOrderTask meant the count query couldn't see the current checkout's in-flight rows. Two concurrent checkouts could each pass the per-account check and then both add $qty tickets. Running ApplyPromoCodeTask after the Reserved order is persisted — and inside the existing promo-code usage lock — makes the count atomic with respect to concurrent orders.
The reorder exposed two real bugs that I also fixed in this commit:
ReserveOrderTask::undo()was a TODO stub, so any downstream failure (invalid code / domain reject /canBeAppliedTo/QuantityPerAccount) was leaving orphaned Order+Ticket rows. Now implemented: detaches tickets from attendees,Summit::removeOrder, thenISummitOrderRepository::delete(Doctrinecascade: ['remove']+orphanRemoval: trueonSummitOrder::$ticketsdrops the ticket rows).Event::dispatch(new CreatedSummitRegistrationOrder(...))was firing insideReserveOrderTask::run(), so listeners would observe orders that later got rolled back. Moved toSummitOrderService::reserve()after the full saga succeeds. No listener exists today, but the contract is now correct.
Regression tests in tests/Unit/Services/SagaCompensationTest.php: Saga::abort() invokes undo() in reverse order, and ReserveOrderTask::undo() removes the order and detaches tickets.
Other two comments addressed in the same commit:
discover()now usesSerializerUtils::getExpand/getFields/getRelations.- Added
database/migrations/config/Version20260412000000.phpfollowing the PR feat: add new endpoint to retrieve member by external id #526 template. FYI it uses theAPIEndpointsMigrationHelpertrait from main — will rebase onto main before your re-review so CI picks it up.
One unrelated bug I noticed while fixing the seeder: the get-sponsorship entry had IGroup::Sponsors in its scopes array instead of authz_groups. Fixed in the same commit — happy to split into a separate PR if you'd prefer.
smarcet
requested changes
Apr 12, 2026
Collaborator
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@caseylocker please review
smarcet
reviewed
Apr 12, 2026
| SummitScopes::ReadAllSummitData | ||
| ] | ||
| ], | ||
| [ |
Collaborator
There was a problem hiding this comment.
@caseylocker
need to create a migration for the new endpoint too
check https://github.com/OpenStackweb/summit-api/pull/526/changes#diff-8aedb5d1857bdf6419aebd732cfff1de8ef7352a5bb15cad6831b0ab03cd2014R48
…r 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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php`:
- Line 1591: The OpenAPI security annotation for the discover endpoint is
missing the ReadAllSummitData scope; update the security array used in the
discover endpoint's annotation (the line with security:
[['summit_promo_codes_oauth2' => [SummitScopes::ReadSummitData]]]) to include
both SummitScopes::ReadSummitData and SummitScopes::ReadAllSummitData so the
documented scopes match the endpoint's actual authorization configuration for
the discover handler.
In `@app/Services/Model/Imp/SummitOrderService.php`:
- Around line 666-667: The current return of a fresh array with only 'order'
drops previously accumulated saga state (e.g., promo_codes_usage) and breaks
subsequent steps like ApplyPromoCodeTask; update the handler that sets
$this->formerState['order'] to preserve and return the full saga state by
merging or returning $this->formerState (i.e., ensure the method returns the
previously accumulated $this->formerState with the updated 'order' key instead
of a new ['order' => $order] array) so ApplyPromoCodeTask can access
promo_codes_usage and other prior state.
🪄 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: 60e94142-914f-48aa-9cce-0a468efca022
📒 Files selected for processing (7)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.phpapp/Services/Model/Imp/SummitOrderService.phpdatabase/migrations/config/Version20260412000000.phpdatabase/seeders/ApiEndpointsSeeder.phpdoc/promo-codes-for-early-registration-access.mdroutes/api_v1.phptests/Unit/Services/SagaCompensationTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- routes/api_v1.php
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php
Outdated
Show resolved
Hide resolved
- 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>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-525/ This page is automatically updated on each push to this PR. |
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
2 participants
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.
ref: https://app.clickup.com/t/86b952pgc
Summary
DomainAuthorizedSummitRegistrationDiscountCode(with discount) andDomainAuthorizedSummitRegistrationPromoCode(access-only)WithPromoCodeaudience value onSummitTicketTypefor promo-code-only ticket typesGET /api/v1/summits/{id}/promo-codes/all/discover) that finds matching codes for the authenticated user's emailauto_applysupport to domain-authorized types and existing email-linked types (Member/Speaker)QuantityPerAccountenforcement at both discovery and checkout timeSDS Implementation (12 tasks)
All 12 tasks implemented. All review follow-ups resolved. Two open deviations remain:
allowed_email_domainsvalidation needs custom rule (currentlysometimes|json)QuantityPerAccountcheckout enforcement —ApplyPromoCodeTaskneeds to move afterReserveOrderTaskin saga chainFiles changed (35 files, +3127/-53)
DomainAuthorizedSummitRegistrationDiscountCode,DomainAuthorizedSummitRegistrationPromoCodeDomainAuthorizedPromoCodeTrait,AutoApplyPromoCodeTraitIDomainAuthorizedPromoCodeVersion20260401150000ApplyPromoCodeTaskDomainAuthorizedPromoCodeTest(30 tests)OAuth2SummitPromoCodesApiTestTest plan
php artisan test --filter=DomainAuthorizedPromoCodeTest— all unit tests passphp artisan test --filter="OAuth2SummitPromoCodesApiTest::testDiscover"— discovery integration tests passupanddownrun cleanly🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests