Skip to content

Commit c4bcdef

Browse files
caseylockerclaude
andcommitted
fix(promo-codes): address smarcet review — saga compensation, discover 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>
1 parent c2719e1 commit c4bcdef

5 files changed

Lines changed: 338 additions & 9 deletions

File tree

app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitPromoCodesApiController.php

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1614,12 +1614,9 @@ public function discover($summit_id)
16141614

16151615
$codes = $this->promo_code_service->discoverPromoCodes($summit, $current_member);
16161616

1617-
$expand = Request::input('expand', '');
1618-
$fields = Request::input('fields', '');
1619-
$relations = Request::input('relations', '');
1620-
1621-
$relations = !empty($relations) ? explode(',', $relations) : ['allowed_ticket_types', 'badge_features', 'tags', 'ticket_types_rules'];
1622-
$fields = !empty($fields) ? explode(',', $fields) : [];
1617+
$expand = SerializerUtils::getExpand();
1618+
$fields = SerializerUtils::getFields();
1619+
$relations = SerializerUtils::getRelations();
16231620

16241621
$data = [];
16251622
foreach ($codes as $code) {

app/Services/Model/Imp/SummitOrderService.php

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -663,14 +663,36 @@ public function run(array $formerState): array
663663
);
664664
$invitation->addOrder($order);
665665
}
666-
Event::dispatch(new CreatedSummitRegistrationOrder($order->getId()));
666+
$this->formerState['order'] = $order;
667667
return ['order' => $order];
668668
});
669669
}
670670

671671
public function undo()
672672
{
673-
// TODO: Implement undo() method.
673+
$order = $this->formerState['order'] ?? null;
674+
if (is_null($order)) {
675+
Log::warning("ReserveOrderTask::undo: no order in formerState, nothing to compensate");
676+
return;
677+
}
678+
679+
$this->tx_service->transaction(function () use ($order) {
680+
Log::info(sprintf("ReserveOrderTask::undo: removing reserved order id %s number %s",
681+
$order->getId(), $order->getNumber()));
682+
683+
// Detach tickets from their attendee owners so stale references don't survive.
684+
// The OneToMany(SummitAttendeeTicket, cascade: ['remove'], orphanRemoval: true) on
685+
// SummitOrder::$tickets then cascades ticket deletion when the order is removed.
686+
foreach ($order->getTickets() as $ticket) {
687+
$attendee = $ticket->getOwner();
688+
if (!is_null($attendee)) {
689+
$attendee->removeTicket($ticket);
690+
}
691+
}
692+
693+
$this->summit->removeOrder($order);
694+
App::make(ISummitOrderRepository::class)->delete($order);
695+
});
674696
}
675697
}
676698

@@ -1759,6 +1781,10 @@ public function reserve(?Member $owner, Summit $summit, array $payload): SummitO
17591781

17601782
$state = $saga_factory->build($owner, $summit, $payload)->run();
17611783

1784+
// Dispatch only after the full saga (including ApplyPromoCodeTask validation) succeeds,
1785+
// so listeners never observe orders that were rolled back by compensation.
1786+
Event::dispatch(new CreatedSummitRegistrationOrder($state['order']->getId()));
1787+
17621788
return $state['order'];
17631789
} catch (ValidationException $ex) {
17641790
Log::warning($ex);
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<?php namespace Database\Migrations\Config;
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 Doctrine\Migrations\AbstractMigration;
16+
use Doctrine\DBAL\Schema\Schema;
17+
use App\Security\SummitScopes;
18+
19+
/**
20+
* Seed the discover-promo-codes endpoint.
21+
*
22+
* Adds the endpoint row plus its two scope associations (ReadSummitData,
23+
* ReadAllSummitData). No authz groups — the endpoint is authenticated-user
24+
* scoped and serves the caller their own qualifying codes.
25+
*
26+
* Idempotent via WHERE NOT EXISTS in APIEndpointsMigrationHelper.
27+
*/
28+
final class Version20260412000000 extends AbstractMigration
29+
{
30+
use APIEndpointsMigrationHelper;
31+
32+
private const API_NAME = 'summits';
33+
private const ENDPOINT_NAME = 'discover-promo-codes';
34+
private const ENDPOINT_ROUTE = '/api/v1/summits/{id}/promo-codes/all/discover';
35+
36+
public function getDescription(): string
37+
{
38+
return 'Seed discover-promo-codes endpoint with ReadSummitData/ReadAllSummitData scopes';
39+
}
40+
41+
public function up(Schema $schema): void
42+
{
43+
$this->addSql($this->insertEndpoint(
44+
self::API_NAME,
45+
self::ENDPOINT_NAME,
46+
self::ENDPOINT_ROUTE,
47+
'GET'
48+
));
49+
50+
$this->addSql($this->insertEndpointScope(
51+
self::API_NAME,
52+
self::ENDPOINT_NAME,
53+
SummitScopes::ReadSummitData
54+
));
55+
56+
$this->addSql($this->insertEndpointScope(
57+
self::API_NAME,
58+
self::ENDPOINT_NAME,
59+
SummitScopes::ReadAllSummitData
60+
));
61+
}
62+
63+
public function down(Schema $schema): void
64+
{
65+
$this->addSql($this->deleteEndpoint(self::API_NAME, self::ENDPOINT_NAME));
66+
}
67+
}

database/seeders/ApiEndpointsSeeder.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2703,12 +2703,12 @@ private function seedSummitEndpoints()
27032703
'scopes' => [
27042704
SummitScopes::ReadSummitData,
27052705
SummitScopes::ReadAllSummitData,
2706-
IGroup::Sponsors,
27072706
],
27082707
'authz_groups' => [
27092708
IGroup::SuperAdmins,
27102709
IGroup::Administrators,
27112710
IGroup::SummitAdministrators,
2711+
IGroup::Sponsors,
27122712
]
27132713
],
27142714
[
Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
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\Models\Foundation\Summit\Repositories\ISummitOrderRepository;
16+
use App\Services\Model\AbstractTask;
17+
use App\Services\Model\ReserveOrderTask;
18+
use App\Services\Model\Saga;
19+
use libs\utils\ITransactionService;
20+
use Mockery;
21+
use models\main\Member;
22+
use models\summit\Summit;
23+
use models\summit\SummitAttendee;
24+
use models\summit\SummitAttendeeTicket;
25+
use models\summit\SummitOrder;
26+
use PHPUnit\Framework\TestCase;
27+
28+
/**
29+
* Class SagaCompensationTest
30+
*
31+
* Regression tests for the saga reorder introduced by the domain-authorized
32+
* promo code feature (ApplyPromoCodeTask moved after ReserveOrderTask). Two
33+
* concerns are exercised:
34+
*
35+
* 1. If any task downstream of ReserveOrderTask throws, Saga::abort() invokes
36+
* undo() on previously-run tasks in reverse order. ReserveOrderTask::undo()
37+
* must therefore remove the persisted order and its tickets.
38+
* 2. Tasks that have not yet run must not be undone.
39+
*
40+
* Uses Mockery on concrete classes; no Laravel, DB, or Redis required. The
41+
* actual dispatch of CreatedSummitRegistrationOrder after a successful
42+
* SummitOrderService::reserve() is covered by
43+
* OAuth2SummitPromoCodesApiTest/OAuth2SummitOrdersApiTest integration tests.
44+
*
45+
* @package Tests\Unit\Services
46+
*/
47+
class SagaCompensationTest extends TestCase
48+
{
49+
protected function setUp(): void
50+
{
51+
parent::setUp();
52+
// Minimal container so the Log/App facades the code under test touches
53+
// resolve to no-ops. No DB, no full Laravel bootstrap.
54+
$container = new \Illuminate\Container\Container();
55+
$container->instance('app', $container);
56+
$container->instance('log', new class {
57+
public function __call($name, $args) { /* silently swallow log calls */ }
58+
});
59+
\Illuminate\Support\Facades\Facade::setFacadeApplication($container);
60+
}
61+
62+
protected function tearDown(): void
63+
{
64+
\Illuminate\Support\Facades\Facade::clearResolvedInstances();
65+
\Illuminate\Support\Facades\Facade::setFacadeApplication(null);
66+
Mockery::close();
67+
parent::tearDown();
68+
}
69+
70+
/**
71+
* ReserveOrderTask::undo() with no order in formerState is a safe no-op
72+
* (run() may have thrown before persisting the order).
73+
*/
74+
public function testUndoIsNoOpWhenOrderWasNotPersisted(): void
75+
{
76+
$tx_service = Mockery::mock(ITransactionService::class);
77+
// transaction() must NOT be called — nothing to compensate.
78+
$tx_service->shouldNotReceive('transaction');
79+
80+
$task = $this->buildTask(
81+
$tx_service,
82+
Mockery::mock(Summit::class),
83+
Mockery::mock(Member::class)
84+
);
85+
86+
// formerState deliberately missing the 'order' key
87+
$this->invokeUndo($task, []);
88+
89+
// Assertion is implicit via Mockery expectations
90+
$this->addToAssertionCount(1);
91+
}
92+
93+
/**
94+
* When ReserveOrderTask::run() persisted an order, undo() must:
95+
* - detach each ticket from its attendee owner (so stale references don't linger)
96+
* - remove the order from the summit
97+
* - delete the order via the repository (cascade removes tickets via orphanRemoval)
98+
*/
99+
public function testUndoDeletesOrderAndDetachesTicketsFromAttendees(): void
100+
{
101+
$attendee1 = Mockery::mock(SummitAttendee::class);
102+
$attendee2 = Mockery::mock(SummitAttendee::class);
103+
104+
$ticket1 = Mockery::mock(SummitAttendeeTicket::class);
105+
$ticket1->shouldReceive('getOwner')->andReturn($attendee1);
106+
$ticket2 = Mockery::mock(SummitAttendeeTicket::class);
107+
$ticket2->shouldReceive('getOwner')->andReturn($attendee2);
108+
// Unassigned ticket: getOwner may return null, undo must not explode
109+
$ticket3 = Mockery::mock(SummitAttendeeTicket::class);
110+
$ticket3->shouldReceive('getOwner')->andReturn(null);
111+
112+
$attendee1->shouldReceive('removeTicket')->once()->with($ticket1);
113+
$attendee2->shouldReceive('removeTicket')->once()->with($ticket2);
114+
115+
$order = Mockery::mock(SummitOrder::class);
116+
$order->shouldReceive('getId')->andReturn(9001);
117+
$order->shouldReceive('getNumber')->andReturn('ORD-TEST-0001');
118+
$order->shouldReceive('getTickets')->andReturn([$ticket1, $ticket2, $ticket3]);
119+
120+
$summit = Mockery::mock(Summit::class);
121+
$summit->shouldReceive('removeOrder')->once()->with($order);
122+
123+
$owner = Mockery::mock(Member::class);
124+
125+
$order_repo = Mockery::mock(ISummitOrderRepository::class);
126+
$order_repo->shouldReceive('delete')->once()->with($order);
127+
128+
// Bind the repo into the container so App::make() inside undo() resolves it.
129+
$container = \Illuminate\Support\Facades\Facade::getFacadeApplication();
130+
$container->instance(ISummitOrderRepository::class, $order_repo);
131+
132+
$tx_service = Mockery::mock(ITransactionService::class);
133+
$tx_service->shouldReceive('transaction')->once()->andReturnUsing(function ($fn) {
134+
return $fn();
135+
});
136+
137+
$task = $this->buildTask($tx_service, $summit, $owner);
138+
$this->invokeUndo($task, ['order' => $order]);
139+
140+
$this->addToAssertionCount(1);
141+
}
142+
143+
/**
144+
* Integration-ish: drive a real Saga whose last task throws and verify
145+
* that a preceding "ReserveOrder-like" task has its undo() invoked exactly
146+
* once, in reverse order.
147+
*/
148+
public function testSagaAbortCallsUndoInReverseOrder(): void
149+
{
150+
$order_of_calls = [];
151+
152+
$first = new RecordingTask('first', $order_of_calls);
153+
$second = new RecordingTask('second', $order_of_calls);
154+
$failing = new class extends AbstractTask {
155+
public function run(array $formerState): array
156+
{
157+
throw new \RuntimeException('downstream failure');
158+
}
159+
public function undo() { /* never runs — it threw in run() */ }
160+
};
161+
162+
$saga = Saga::start()
163+
->addTask($first)
164+
->addTask($second)
165+
->addTask($failing);
166+
167+
try {
168+
$saga->run();
169+
$this->fail('Expected saga to propagate the downstream exception');
170+
} catch (\RuntimeException $ex) {
171+
$this->assertSame('downstream failure', $ex->getMessage());
172+
}
173+
174+
// run: first, second, (failing throws); undo: second, first
175+
$this->assertSame(
176+
['run:first', 'run:second', 'undo:second', 'undo:first'],
177+
$order_of_calls
178+
);
179+
}
180+
181+
/**
182+
* Construct a ReserveOrderTask with only the fields undo() needs. run() is
183+
* not exercised here, so most collaborators can be plain Mockery doubles.
184+
*/
185+
private function buildTask(ITransactionService $tx, Summit $summit, Member $owner): ReserveOrderTask
186+
{
187+
$reflector = new \ReflectionClass(ReserveOrderTask::class);
188+
/** @var ReserveOrderTask $task */
189+
$task = $reflector->newInstanceWithoutConstructor();
190+
191+
$this->setPrivate($task, 'tx_service', $tx);
192+
$this->setPrivate($task, 'summit', $summit);
193+
$this->setPrivate($task, 'owner', $owner);
194+
195+
return $task;
196+
}
197+
198+
private function setPrivate(object $instance, string $property, $value): void
199+
{
200+
$r = new \ReflectionClass($instance);
201+
$p = $r->getProperty($property);
202+
$p->setAccessible(true);
203+
$p->setValue($instance, $value);
204+
}
205+
206+
private function invokeUndo(ReserveOrderTask $task, array $formerState): void
207+
{
208+
$this->setPrivate($task, 'formerState', $formerState);
209+
$task->undo();
210+
}
211+
}
212+
213+
/**
214+
* Minimal AbstractTask implementation that records run/undo invocation order.
215+
* Declared at file scope (not inside the TestCase) so PHP can resolve the
216+
* AbstractTask parent at class-load time without coupling to test lifecycle.
217+
*/
218+
final class RecordingTask extends AbstractTask
219+
{
220+
private $label;
221+
private $log;
222+
223+
public function __construct(string $label, array &$log)
224+
{
225+
$this->label = $label;
226+
$this->log = &$log;
227+
}
228+
229+
public function run(array $formerState): array
230+
{
231+
$this->log[] = 'run:' . $this->label;
232+
return $formerState;
233+
}
234+
235+
public function undo()
236+
{
237+
$this->log[] = 'undo:' . $this->label;
238+
}
239+
}

0 commit comments

Comments
 (0)