Skip to content

fix(promo-codes): restrict implicit "apply to all" to Audience=All only#541

Open
caseylocker wants to merge 2 commits into
mainfrom
feature/promo-code-apply-to-all-audience-restriction
Open

fix(promo-codes): restrict implicit "apply to all" to Audience=All only#541
caseylocker wants to merge 2 commits into
mainfrom
feature/promo-code-apply-to-all-audience-restriction

Conversation

@caseylocker
Copy link
Copy Markdown
Contributor

@caseylocker caseylocker commented May 11, 2026

ref: https://app.clickup.com/t/86b9vrpxp

Summary

Tightens SummitRegistrationPromoCode::canBeAppliedTo() so the implicit "apply to all" branch (empty allowed_ticket_types) only matches ticket types with Audience = All. Other audiences (WithInvitation, WithoutInvitation, WithPromoCode) must be opted in via explicit allowed_ticket_types membership.

What changed

  • app/Models/Foundation/Summit/Registration/PromoCodes/SummitRegistrationPromoCode.php:483-495 — empty-collection branch returns $ticketType->getAudience() === SummitTicketType::Audience_All. Subclass overrides (SummitRegistrationDiscountCode::canBeAppliedTo, DomainAuthorizedSummitRegistrationDiscountCode::canBeAppliedTo) delegate to the patched base — no override changes needed.
  • tests/Unit/Services/PromoCodeCanBeAppliedToAudienceTest.php — new unit tests, dataProvider over all 4 audiences × {empty, present, absent} + a smoke test for the domain-authorized override.
  • doc/promo-code-apply-to-all-audience-restriction.md — dedicated SDS.
  • app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php:73 — tightened a stale comment now that the empty branch is audience-conditional.

Caller audit

canBeAppliedTo() is the single gate for every production path:

  • RegularPromoCodeTicketTypesStrategy::applyPromo2TicketType() line 82
  • SummitOrderService.php:812 and :1093 (order/checkout pipeline)
  • RegularTicketTypePromoCodeValidationStrategy.php:91

No code path independently treats empty allowed_ticket_types as "applies to all audiences" for promo codes. The out-of-scope sibling SummitAttendee.php:1367 is a different domain (invitations).

Behavior change (intentional)

Existing discount codes with empty allowed_ticket_types that today implicitly cover WithInvitation / WithoutInvitation tickets will stop covering them after deploy. This is called out in the ticket: "Existing discount codes with 'Apply to all' checked continue to work correctly for Audience = All ticket types — no regression."

AC5 wording note

The ClickUp acceptance criterion #5 says "submitting apply_to_all_ticket_types = true resolves to Audience = All only, regardless of frontend." No such payload field exists in this API — it's a conceptual name. The actual enforcement is canBeAppliedTo(), the single gate every consumer flows through. A hostile client cannot bypass it because there's no apply_to_all_* field to honor or ignore.

Test plan

  • Unit: ./vendor/bin/phpunit tests/Unit/Services/PromoCodeCanBeAppliedToAudienceTest.php — pass
  • Unit suite: full tests/Unit/Services/ passes (53 tests, 77 assertions, no deprecations)
  • Integration: CI's Run Unit Tests On Push workflow ran tests/oauth2/ (OAuth2Tests shard) on the PR head — green. The previously-flagged OAuth2SummitTicketTypesApiTest::testGetAllowedTicketTypesApplyingRegularDiscountCodeToAllOfThem (:218) still passes: default_ticket_type is Audience=All per tests/InsertSummitTestData.php:355, so it remains discount-eligible after the empty branch is gated, and '+id' ordering keeps it at data[0] for the assertion.
  • Manual: against a deploy carrying this patch, create a SUMMIT_DISCOUNT_CODE with empty rules, verify it does NOT apply to a WithPromoCode ticket type; add the WithPromoCode type to allowed_ticket_types, verify it then applies. Deferred: dev/staging do not yet carry this change. Reviewer please run after first deploy.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected promo code applicability logic to properly enforce audience restrictions when codes apply to all ticket types, ensuring codes only apply to unrestricted audience types.
  • Tests

    • Added comprehensive test coverage validating promo code applicability across different audience configurations and ticket type scenarios.
  • Documentation

    • Added specification document detailing promo code audience restriction requirements and behavior.

Review Change Stack

When SummitRegistrationPromoCode::allowed_ticket_types is empty, canBeAppliedTo()
previously returned true for any ticket type — including WithPromoCode-audience
tickets that are deliberately hidden from the public. This change tightens the
implicit-sweep branch to require Audience = All; non-All audiences must be
opted in via explicit allowed_ticket_types membership.

Adds 11 unit tests (4 audience values × empty/explicit) and a smoke test
confirming DomainAuthorizedSummitRegistrationDiscountCode's override still
applies to its explicitly-allowed WithPromoCode tickets.

Refs: ClickUp 86b9vrpxp; parent feature SDS doc/promo-codes-for-early-registration-access.md
…case

testNonEmptyAllowedTicketTypesNotContainingTicketReturnsFalse previously
only ran for Audience=All. Convert it to a DataProvider so a regression
that accidentally returns true on the absent-from-list path for
WithInvitation / WithoutInvitation / WithPromoCode also gets caught.

Also tighten the stale "returns true when allowed_ticket_types is empty"
comment in DomainAuthorizedSummitRegistrationDiscountCode::addTicketTypeRule
to reflect the post-fix conditional, and refresh the SDS line-number cites
to match the current method body.

Refs: ClickUp 86b9vrpxp; Codex review of 8defe88.
@caseylocker caseylocker self-assigned this May 11, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1524bc84-448c-4fe0-b7f0-cf9e60799ce3

📥 Commits

Reviewing files that changed from the base of the PR and between 34e47f9 and e1c0a94.

📒 Files selected for processing (4)
  • app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php
  • app/Models/Foundation/Summit/Registration/PromoCodes/SummitRegistrationPromoCode.php
  • doc/promo-code-apply-to-all-audience-restriction.md
  • tests/Unit/Services/PromoCodeCanBeAppliedToAudienceTest.php

📝 Walkthrough

Walkthrough

This PR enforces audience-aware applicability for promo codes when allowed_ticket_types is empty: instead of unconditionally applying to all tickets, empty allowed_ticket_types now restricts application to Audience_All tickets only, rejecting With_Invitation, Without_Invitation, and With_Promo_Code audiences.

Changes

Promo Code Audience Restriction

Layer / File(s) Summary
Audience-based applicability in canBeAppliedTo()
app/Models/Foundation/Summit/Registration/PromoCodes/SummitRegistrationPromoCode.php, app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php
canBeAppliedTo() now checks ticket audience when allowed_ticket_types is empty, returning true only for Audience_All and false for other audiences. Comment in DomainAuthorizedSummitRegistrationDiscountCode clarified to explain why the base method's behavior matters.
Test coverage for audience restrictions
tests/Unit/Services/PromoCodeCanBeAppliedToAudienceTest.php
Comprehensive PHPUnit test suite validates the audience × allowed_ticket_types matrix across multiple SummitTicketType audiences, with additional coverage for SummitRegistrationDiscountCode and DomainAuthorizedSummitRegistrationDiscountCode subclasses.
Specification and documentation
doc/promo-code-apply-to-all-audience-restriction.md
SDS document defines the audience restriction change with goal, scope, authoritized constraints, task breakdown, manual test plan, and acceptance criteria. Establishes that empty allowed_ticket_types must apply only to All-audience tickets.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 A promo code's audience now finds its way,

No longer lumped in a generic fray,

With All it shines, but others must stay,

Tested and spec'd in this hardening play! 🎫✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: restricting promo code applicability when allowed_ticket_types is empty to only Audience=All ticket types, which is the core logic fix across the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/promo-code-apply-to-all-audience-restriction

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-541/

This page is automatically updated on each push to this PR.

@caseylocker caseylocker marked this pull request as ready for review May 12, 2026 15:48
@caseylocker caseylocker requested a review from smarcet May 12, 2026 15:49
@smarcet smarcet requested review from Copilot and romanetar May 12, 2026 15:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens promo-code applicability so the implicit “apply to all ticket types” behavior (empty allowed_ticket_types) only applies to ticket types whose audience is All, preventing unintended applicability to restricted audiences like WithPromoCode.

Changes:

  • Updated SummitRegistrationPromoCode::canBeAppliedTo() so an empty allowed_ticket_types collection returns true only for Audience_All.
  • Added a dedicated unit-test matrix covering all audiences across empty/non-empty allowed-ticket-type scenarios, plus subclass delegation smoke tests.
  • Added an SDS documenting the rationale, scope, and acceptance criteria; updated a related in-code comment to reflect the new audience-conditional empty-collection behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
app/Models/Foundation/Summit/Registration/PromoCodes/SummitRegistrationPromoCode.php Restricts empty allowed_ticket_types (“apply to all”) to Audience_All ticket types only.
tests/Unit/Services/PromoCodeCanBeAppliedToAudienceTest.php Adds unit coverage guarding the new audience restriction and verifying subclass delegation behavior.
doc/promo-code-apply-to-all-audience-restriction.md Documents the design decision, scope, and test plan for the audience restriction.
app/Models/Foundation/Summit/Registration/PromoCodes/DomainAuthorizedSummitRegistrationDiscountCode.php Updates a comment to align with the now audience-conditional empty-collection behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Collaborator

@romanetar romanetar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants