Skip to content

Commit b87cefd

Browse files
caseylockerclaude
caseylocker
and
claude
committed
fix(promo-codes): guard WithPromoCode reservations and exclude exhausted 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>
1 parent 138c1f8 commit b87cefd

5 files changed

Lines changed: 330 additions & 0 deletions

File tree

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,6 +1032,14 @@ public function run(array $formerState): array
10321032

10331033
$promo_code_value = isset($ticket_dto['promo_code']) ? strtoupper(trim($ticket_dto['promo_code'])) : null;
10341034

1035+
// WithPromoCode audience ticket types are never purchasable without a qualifying promo code.
1036+
// canBeAppliedTo (below) rejects wrong codes; this guards the no-code case.
1037+
if ($ticket_type->isPromoCodeOnly() && empty($promo_code_value)) {
1038+
throw new ValidationException(
1039+
sprintf("Ticket type %s requires a promo code.", $ticket_type->getName())
1040+
);
1041+
}
1042+
10351043
if (!isset($reservations[$type_id]))
10361044
$reservations[$type_id] = 0;
10371045

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,13 @@ public function discoverPromoCodes(Summit $summit, Member $member): array
10241024
$results = [];
10251025

10261026
foreach ($codes as $code) {
1027+
// Global exhaustion: finite code with quantity_used >= quantity_available.
1028+
// The repository filter uses isLive() (dates only), so exhausted codes leak through.
1029+
// Skip them here so discovery matches checkout's validate() behavior.
1030+
if (!$code->hasQuantityAvailable()) {
1031+
continue;
1032+
}
1033+
10271034
// QuantityPerAccount enforcement: exclude exhausted codes
10281035
if ($code instanceof IDomainAuthorizedPromoCode) {
10291036
$quantityPerAccount = $code->getQuantityPerAccount();

‎tests/Unit/Services/PreProcessReservationTaskTest.php‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
<?php namespace Tests\Unit\Services;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use App\Services\Model\PreProcessReservationTask;
16+
use Mockery;
17+
use models\exceptions\ValidationException;
18+
use models\summit\Summit;
19+
use models\summit\SummitTicketType;
20+
use PHPUnit\Framework\TestCase;
21+
22+
/**
23+
* Class PreProcessReservationTaskTest
24+
*
25+
* Regression unit tests for the WithPromoCode reservation guard in
26+
* {@see PreProcessReservationTask}. Uses Mockery on concrete Summit /
27+
* SummitTicketType so the tests do not require Laravel, DB, or Redis.
28+
*
29+
* @package Tests\Unit\Services
30+
*/
31+
class PreProcessReservationTaskTest extends TestCase
32+
{
33+
protected function tearDown(): void
34+
{
35+
Mockery::close();
36+
parent::tearDown();
37+
}
38+
39+
/**
40+
* WithPromoCode audience + no promo code → ValidationException.
41+
*
42+
* This is the regression: previously, PreProcessReservationTask::run()
43+
* only validated promo codes when one was supplied, letting
44+
* audience=WithPromoCode tickets be reserved with just a type_id.
45+
*/
46+
public function testRejectsPromoCodeOnlyTicketTypeWithoutPromoCode(): void
47+
{
48+
$ticket_type = Mockery::mock(SummitTicketType::class);
49+
$ticket_type->shouldReceive('getId')->andReturn(42);
50+
$ticket_type->shouldReceive('getName')->andReturn('VIP_PROMO_ONLY');
51+
$ticket_type->shouldReceive('isLive')->andReturn(true);
52+
$ticket_type->shouldReceive('isPromoCodeOnly')->andReturn(true);
53+
54+
$summit = Mockery::mock(Summit::class);
55+
$summit->shouldReceive('getTicketTypeById')->with(42)->andReturn($ticket_type);
56+
57+
$payload = [
58+
'tickets' => [
59+
['type_id' => 42], // no promo_code
60+
],
61+
];
62+
63+
$task = new PreProcessReservationTask($summit, $payload);
64+
65+
$this->expectException(ValidationException::class);
66+
$this->expectExceptionMessage('Ticket type VIP_PROMO_ONLY requires a promo code.');
67+
68+
$task->run([]);
69+
}
70+
71+
/**
72+
* Non-WithPromoCode audience + no promo code → allowed (guard does not overreach).
73+
*/
74+
public function testAllowsNonPromoCodeOnlyTicketTypeWithoutPromoCode(): void
75+
{
76+
$ticket_type = Mockery::mock(SummitTicketType::class);
77+
$ticket_type->shouldReceive('getId')->andReturn(7);
78+
$ticket_type->shouldReceive('getName')->andReturn('GENERAL_ADMISSION');
79+
$ticket_type->shouldReceive('isLive')->andReturn(true);
80+
$ticket_type->shouldReceive('isPromoCodeOnly')->andReturn(false);
81+
82+
$summit = Mockery::mock(Summit::class);
83+
$summit->shouldReceive('getTicketTypeById')->with(7)->andReturn($ticket_type);
84+
85+
$payload = [
86+
'tickets' => [
87+
['type_id' => 7],
88+
],
89+
];
90+
91+
$task = new PreProcessReservationTask($summit, $payload);
92+
$state = $task->run([]);
93+
94+
$this->assertEquals([7 => 1], $state['reservations']);
95+
$this->assertEquals([], $state['promo_codes_usage']);
96+
$this->assertEquals([7], $state['ticket_types_ids']);
97+
}
98+
}

‎tests/Unit/Services/SummitPromoCodeServiceDiscoveryTest.php‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
<?php namespace Tests\Unit\Services;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use App\Services\Model\SummitPromoCodeService;
16+
use App\Services\Utils\ILockManagerService;
17+
use libs\utils\ITransactionService;
18+
use Mockery;
19+
use models\main\ICompanyRepository;
20+
use models\main\IMemberRepository;
21+
use models\main\ITagRepository;
22+
use models\main\Member;
23+
use models\summit\DomainAuthorizedSummitRegistrationPromoCode;
24+
use models\summit\ISpeakerRepository;
25+
use models\summit\ISummitAttendeeTicketRepository;
26+
use models\summit\ISummitRegistrationPromoCodeRepository;
27+
use models\summit\ISummitRepository;
28+
use models\summit\Summit;
29+
use PHPUnit\Framework\TestCase;
30+
31+
/**
32+
* Class SummitPromoCodeServiceDiscoveryTest
33+
*
34+
* Regression unit tests for {@see SummitPromoCodeService::discoverPromoCodes}
35+
* filtering logic. Focuses on the global-exhaustion guard added to match
36+
* checkout's validate() behavior.
37+
*
38+
* @package Tests\Unit\Services
39+
*/
40+
class SummitPromoCodeServiceDiscoveryTest extends TestCase
41+
{
42+
protected function tearDown(): void
43+
{
44+
Mockery::close();
45+
parent::tearDown();
46+
}
47+
48+
private function buildService(ISummitRegistrationPromoCodeRepository $repository): SummitPromoCodeService
49+
{
50+
return new SummitPromoCodeService(
51+
Mockery::mock(IMemberRepository::class),
52+
Mockery::mock(ICompanyRepository::class),
53+
Mockery::mock(ISpeakerRepository::class),
54+
Mockery::mock(ISummitRepository::class),
55+
Mockery::mock(ISummitAttendeeTicketRepository::class),
56+
Mockery::mock(ITagRepository::class),
57+
$repository,
58+
Mockery::mock(ITransactionService::class),
59+
Mockery::mock(ILockManagerService::class)
60+
);
61+
}
62+
63+
/**
64+
* Regression: a finite domain-authorized code whose quantity_used has
65+
* reached quantity_available must not appear in discovery. Previously,
66+
* the repository filter used isLive() (dates only), so globally
67+
* exhausted codes leaked through — frontend auto-apply would then hit
68+
* a hard checkout failure.
69+
*/
70+
public function testDiscoverExcludesGloballyExhaustedDomainAuthorizedCode(): void
71+
{
72+
$exhausted = Mockery::mock(DomainAuthorizedSummitRegistrationPromoCode::class);
73+
$exhausted->shouldReceive('getCode')->andReturn('GLOBAL_EXHAUSTED');
74+
$exhausted->shouldReceive('hasQuantityAvailable')->andReturn(false);
75+
// getQuantityPerAccount should never be reached if the global-exhaustion
76+
// guard is in place — but define it defensively so a regression would
77+
// surface as a quota check, not an uncaught Mockery error.
78+
$exhausted->shouldReceive('getQuantityPerAccount')->andReturn(0);
79+
$exhausted->shouldReceive('setRemainingQuantityPerAccount')->andReturn(null);
80+
81+
$summit = Mockery::mock(Summit::class);
82+
$member = Mockery::mock(Member::class);
83+
$member->shouldReceive('getEmail')->andReturn('new-buyer@acme.com');
84+
$member->shouldReceive('getId')->andReturn(99);
85+
86+
$repository = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
87+
// Repository filter is isLive()-only, so it would pass the exhausted
88+
// code through — simulate that by returning it.
89+
$repository->shouldReceive('getDiscoverableByEmailForSummit')
90+
->with($summit, 'new-buyer@acme.com')
91+
->andReturn([$exhausted]);
92+
93+
$service = $this->buildService($repository);
94+
$result = $service->discoverPromoCodes($summit, $member);
95+
96+
$this->assertSame([], $result,
97+
'Globally exhausted domain-authorized code must not appear in discovery');
98+
}
99+
100+
/**
101+
* A healthy domain-authorized code (has global quantity, unlimited quota)
102+
* passes through. Guards against over-filtering: the exhaustion guard must
103+
* not drop valid codes.
104+
*/
105+
public function testDiscoverReturnsHealthyDomainAuthorizedCode(): void
106+
{
107+
$healthy = Mockery::mock(DomainAuthorizedSummitRegistrationPromoCode::class);
108+
$healthy->shouldReceive('getCode')->andReturn('HEALTHY');
109+
$healthy->shouldReceive('hasQuantityAvailable')->andReturn(true);
110+
$healthy->shouldReceive('getQuantityPerAccount')->andReturn(0);
111+
$healthy->shouldReceive('setRemainingQuantityPerAccount')->with(null)->once();
112+
113+
$summit = Mockery::mock(Summit::class);
114+
$member = Mockery::mock(Member::class);
115+
$member->shouldReceive('getEmail')->andReturn('buyer@acme.com');
116+
$member->shouldReceive('getId')->andReturn(42);
117+
118+
$repository = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
119+
$repository->shouldReceive('getDiscoverableByEmailForSummit')
120+
->with($summit, 'buyer@acme.com')
121+
->andReturn([$healthy]);
122+
123+
$service = $this->buildService($repository);
124+
$result = $service->discoverPromoCodes($summit, $member);
125+
126+
$this->assertCount(1, $result);
127+
$this->assertSame('HEALTHY', $result[0]->getCode());
128+
}
129+
130+
/**
131+
* Mixed case: exhausted code is dropped while a healthy sibling survives.
132+
* This proves the guard uses per-code `continue`, not a scalar short-circuit.
133+
*/
134+
public function testDiscoverMixedHealthyAndExhaustedCodes(): void
135+
{
136+
$exhausted = Mockery::mock(DomainAuthorizedSummitRegistrationPromoCode::class);
137+
$exhausted->shouldReceive('getCode')->andReturn('EXHAUSTED');
138+
$exhausted->shouldReceive('hasQuantityAvailable')->andReturn(false);
139+
$exhausted->shouldReceive('getQuantityPerAccount')->andReturn(0);
140+
$exhausted->shouldReceive('setRemainingQuantityPerAccount')->andReturn(null);
141+
142+
$healthy = Mockery::mock(DomainAuthorizedSummitRegistrationPromoCode::class);
143+
$healthy->shouldReceive('getCode')->andReturn('HEALTHY');
144+
$healthy->shouldReceive('hasQuantityAvailable')->andReturn(true);
145+
$healthy->shouldReceive('getQuantityPerAccount')->andReturn(0);
146+
$healthy->shouldReceive('setRemainingQuantityPerAccount')->with(null)->once();
147+
148+
$summit = Mockery::mock(Summit::class);
149+
$member = Mockery::mock(Member::class);
150+
$member->shouldReceive('getEmail')->andReturn('buyer@acme.com');
151+
$member->shouldReceive('getId')->andReturn(7);
152+
153+
$repository = Mockery::mock(ISummitRegistrationPromoCodeRepository::class);
154+
$repository->shouldReceive('getDiscoverableByEmailForSummit')
155+
->with($summit, 'buyer@acme.com')
156+
->andReturn([$exhausted, $healthy]);
157+
158+
$service = $this->buildService($repository);
159+
$result = $service->discoverPromoCodes($summit, $member);
160+
161+
$this->assertCount(1, $result);
162+
$this->assertSame('HEALTHY', $result[0]->getCode());
163+
}
164+
}

‎tests/oauth2/OAuth2SummitPromoCodesApiTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1022,6 +1022,59 @@ public function testDiscoverExcludesExhaustedCodes()
10221022
'Exhausted domain-authorized code (quantity_per_account reached) should not appear in discovery');
10231023
}
10241024

1025+
/**
1026+
* Discovery excludes codes where global quantity_available is exhausted
1027+
* (quantity_used >= quantity_available), independent of quantity_per_account.
1028+
* Regression: isLive() is dates-only, so the repository filter does not
1029+
* catch globally-exhausted-but-still-in-date codes.
1030+
*/
1031+
public function testDiscoverExcludesGloballyExhaustedCodes()
1032+
{
1033+
$memberEmail = self::$member->getEmail();
1034+
$domain = '@' . substr($memberEmail, strpos($memberEmail, '@') + 1);
1035+
1036+
$code = new DomainAuthorizedSummitRegistrationPromoCode();
1037+
$code->setCode('DISC_GLOBAL_EXHAUST_' . str_random(8));
1038+
$code->setAllowedEmailDomains([$domain]);
1039+
$code->setQuantityAvailable(1);
1040+
// quantity_per_account = 0 (unlimited) isolates the global exhaustion path
1041+
$code->setQuantityPerAccount(0);
1042+
self::$summit->addPromoCode($code);
1043+
self::$em->persist(self::$summit);
1044+
self::$em->flush();
1045+
1046+
// Globally exhaust: quantity_used becomes 1, matches quantity_available=1.
1047+
// isLive() is dates-only and will still return true, so without the
1048+
// service-layer guard this code would leak into the discovery results.
1049+
$code->addUsage('someone-else@example.com', 1);
1050+
self::$em->persist($code);
1051+
self::$em->flush();
1052+
1053+
$params = [
1054+
'id' => self::$summit->getId(),
1055+
];
1056+
1057+
$headers = ["HTTP_Authorization" => " Bearer " . $this->access_token];
1058+
1059+
$response = $this->action(
1060+
"GET",
1061+
"OAuth2SummitPromoCodesApiController@discover",
1062+
$params,
1063+
[],
1064+
[],
1065+
[],
1066+
$headers
1067+
);
1068+
1069+
$content = $response->getContent();
1070+
$this->assertResponseStatus(200);
1071+
$result = json_decode($content, true);
1072+
1073+
$codes = array_column($result['data'], 'code');
1074+
$this->assertNotContains($code->getCode(), $codes,
1075+
'Globally exhausted domain-authorized code (quantity_used >= quantity_available) should not appear in discovery');
1076+
}
1077+
10251078
// -----------------------------------------------------------------------
10261079
// Checkout enforcement — Task 12 follow-up #6
10271080
// -----------------------------------------------------------------------

0 commit comments

Comments
 (0)