diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 0c216b7c6..f488d63ab 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -153,7 +153,14 @@ private function markAsRan(AbstractTask $task) private function abort() { foreach (array_reverse($this->already_run_tasks) as $task) { - $task->undo(); + // a failing compensation must neither mask the exception that aborted the saga + // nor prevent the remaining tasks (ticket stock, member quota) from being undone + try { + $task->undo(); + } catch (\Throwable $ex) { + Log::error(sprintf("Saga::abort undo failed for %s: %s", get_class($task), $ex->getMessage())); + Log::error($ex); + } } } } @@ -689,7 +696,21 @@ public function undo() return; } - $this->tx_service->transaction(function () use ($order) { + $order_id = $order->getId(); + + $this->tx_service->transaction(function () use ($order_id) { + // The order cached in formerState (and $this->summit) belong to the transaction that + // created them. When a later task fails, the root transaction rollback clears the + // EntityManager, so they are detached by now and can not be removed: reload them. + $order_repository = App::make(ISummitOrderRepository::class); + $order = $order_repository->getByIdExclusiveLock($order_id); + if (!$order instanceof SummitOrder) { + Log::warning(sprintf("ReserveOrderTask::undo: order id %s not found, nothing to compensate", $order_id)); + return; + } + // re attach + $summit = App::make(ISummitRepository::class)->getById($this->summit->getId()); + Log::info(sprintf("ReserveOrderTask::undo: removing reserved order id %s number %s", $order->getId(), $order->getNumber())); @@ -703,8 +724,8 @@ public function undo() } } - $this->summit->removeOrder($order); - App::make(ISummitOrderRepository::class)->delete($order); + $summit->removeOrder($order); + $order_repository->delete($order); }); } } diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index 85f1640bb..24c42ffee 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -42,6 +42,8 @@ use models\main\ICompanyRepository; use models\main\IMemberRepository; use models\main\ITagRepository; +use App\Models\Foundation\Summit\PromoCodes\PromoCodesConstants; +use models\summit\SpeakersRegistrationDiscountCode; use models\summit\ISummitAttendeeRepository; use models\summit\ISummitAttendeeTicketRepository; use models\summit\ISummitRegistrationPromoCodeRepository; @@ -710,6 +712,56 @@ public function testReserveFailsWhenTicketTypeSoldOut() $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); } + /** + * ApplyPromoCodeTask (last saga task) rejects a speakers promo code that is not + * assigned to the buyer. The saga compensation must then delete the order that + * ReserveOrderTask already committed and restore the ticket type stock, and the + * caller must see the original ValidationException, not a detached-entity error. + */ + public function testReserveRejectedByPromoCodeLeavesNoOrderAndRestoresStock() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $promo_code = new SpeakersRegistrationDiscountCode(); + $promo_code->setCode('TEST_SPK_DC_' . uniqid()); + $promo_code->setType(PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypes[0]); + $promo_code->setAmount(100); + $promo_code->addAllowedTicketType(self::$default_ticket_type); + self::$summit->addPromoCode($promo_code); + self::$em->persist(self::$summit); + self::$em->flush(); + + $ticket_type_id = self::$default_ticket_type->getId(); + $initial_quantity_sold = self::$default_ticket_type->getQuantitySold(); + $initial_order_count = self::$summit->getOrders()->count(); + $buyer_email = sprintf('not-a-speaker-%s@test.com', uniqid()); + + $payload = [ + "owner_email" => $buyer_email, + "owner_first_name" => "Not", + "owner_last_name" => "Speaker", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => $ticket_type_id, "promo_code" => $promo_code->getCode()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('is not valid for', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); + + $reFetchedTicketType = self::$em->find(SummitTicketType::class, $ticket_type_id); + $this->assertEquals($initial_quantity_sold, $reFetchedTicketType->getQuantitySold()); + } + /** * ReserveTicketsTask rejects orders mixing ticket types of different currencies. */ diff --git a/tests/Unit/Services/SagaCompensationTest.php b/tests/Unit/Services/SagaCompensationTest.php index a1beb7cbc..b671a5309 100644 --- a/tests/Unit/Services/SagaCompensationTest.php +++ b/tests/Unit/Services/SagaCompensationTest.php @@ -19,6 +19,7 @@ use libs\utils\ITransactionService; use Mockery; use models\main\Member; +use models\summit\ISummitRepository; use models\summit\Summit; use models\summit\SummitAttendee; use models\summit\SummitAttendeeTicket; @@ -119,16 +120,24 @@ public function testUndoDeletesOrderAndDetachesTicketsFromAttendees(): void $order->shouldReceive('getTickets')->andReturn([$ticket1, $ticket2, $ticket3]); $summit = Mockery::mock(Summit::class); + $summit->shouldReceive('getId')->andReturn(77); $summit->shouldReceive('removeOrder')->once()->with($order); $owner = Mockery::mock(Member::class); + // undo() reloads the order and the summit: the instances cached by run() are + // detached once the failing task's root transaction cleared the EntityManager. $order_repo = Mockery::mock(ISummitOrderRepository::class); + $order_repo->shouldReceive('getByIdExclusiveLock')->once()->with(9001)->andReturn($order); $order_repo->shouldReceive('delete')->once()->with($order); - // Bind the repo into the container so App::make() inside undo() resolves it. + $summit_repo = Mockery::mock(ISummitRepository::class); + $summit_repo->shouldReceive('getById')->once()->with(77)->andReturn($summit); + + // Bind the repos into the container so App::make() inside undo() resolves them. $container = \Illuminate\Support\Facades\Facade::getFacadeApplication(); $container->instance(ISummitOrderRepository::class, $order_repo); + $container->instance(ISummitRepository::class, $summit_repo); $tx_service = Mockery::mock(ITransactionService::class); $tx_service->shouldReceive('transaction')->once()->andReturnUsing(function ($fn) { @@ -179,6 +188,49 @@ public function undo() { /* never runs — it threw in run() */ } ); } + /** + * A compensation that itself fails must neither mask the exception that aborted + * the saga nor stop the remaining compensations (ticket stock, member quota) + * from running. + */ + public function testSagaAbortKeepsUndoingAndRethrowsOriginalWhenAnUndoFails(): void + { + $order_of_calls = []; + + $first = new RecordingTask('first', $order_of_calls); + $broken_undo = new class extends AbstractTask { + public function run(array $formerState): array + { + return $formerState; + } + public function undo() + { + throw new \LogicException('undo failure'); + } + }; + $failing = new class extends AbstractTask { + public function run(array $formerState): array + { + throw new \RuntimeException('downstream failure'); + } + public function undo() { /* never runs — it threw in run() */ } + }; + + $saga = Saga::start() + ->addTask($first) + ->addTask($broken_undo) + ->addTask($failing); + + try { + $saga->run(); + $this->fail('Expected saga to propagate the downstream exception'); + } catch (\RuntimeException $ex) { + $this->assertSame('downstream failure', $ex->getMessage()); + } + + $this->assertSame(['run:first', 'undo:first'], $order_of_calls); + } + /** * Construct a ReserveOrderTask with only the fields undo() needs. run() is * not exercised here, so most collaborators can be plain Mockery doubles.