diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index f488d63ab..4314094fb 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -772,6 +772,17 @@ final class ApplyPromoCodeTask extends AbstractTask */ private $owner; + /** + * Usages applied (and committed) by this task instance, one entry per promo code. + * Filled by applyUsages() right after each code's transaction commits and consumed + * by undo(), which releases each entry exactly once. Tracking it on the instance + * (formerState is only updated when run() completes) lets run() compensate the + * codes it already applied when a later one is rejected. + * + * @var array + */ + private $applied = []; + /** * ApplyPromoCodeTask constructor. * @param Summit $summit @@ -815,6 +826,37 @@ public function run(array $formerState): array $this->summit = $summit_repository->getById($this->summit->getId()); $owner_company_name = TaskUtils::getOwnerCompanyName($this->summit, $this->payload); + // Saga::run() only marks a task as "ran" AFTER run() returns. If code A is applied + // (its own transaction already committed) and code B is then rejected, the exception + // propagates before markAsRan(), so Saga::abort() never calls this task's undo() and + // code A's usage would leak. Release what this run applied before rethrowing. + try { + $this->applyUsages($promo_codes_usage, $owner_email, $owner_company_name); + } catch (\Exception $ex) { + try { + $this->undo(); + } catch (\Throwable $undo_ex) { + // never mask the exception that rejected the reservation + Log::error(sprintf("ApplyPromoCodeTask::run undo failed: %s", $undo_ex->getMessage())); + Log::error($undo_ex); + } + throw $ex; + } + + return $this->formerState; + } + + /** + * Applies each promo code usage on its own transaction, recording it in $this->applied + * as soon as it commits. + * + * @param array $promo_codes_usage + * @param string $owner_email + * @param string|null $owner_company_name + * @throws \Exception + */ + private function applyUsages(array $promo_codes_usage, string $owner_email, ?string $owner_company_name): void + { foreach ($promo_codes_usage as $promo_code_value => $info) { $this->tx_service->transaction(function () use ($owner_email, $owner_company_name, $promo_code_value, $info) { @@ -859,45 +901,62 @@ public function run(array $formerState): array }, 30); }); - // mark a done - $promo_codes_usage[$promo_code_value]['redeem'] = true; + // mark as done: visible to undo() from now on + $this->applied[] = ['code' => $promo_code_value, 'qty' => intval($info['qty'])]; } - // update state - $this->formerState['promo_codes_usage'] = $promo_codes_usage; - - return $this->formerState; } + /** + * Releases the usages applied by this task instance. Idempotent: each entry is dropped + * from $applied once released, so the local release done by run() on failure plus a later + * Saga::abort() can not release the same usage twice. A release that fails is logged and + * kept in $applied, so it does not block the remaining codes and a later undo() retries + * only the entries that did not go through. + */ public function undo() { Log::debug ( sprintf ( - "ApplyPromoCodeTask::undo: compensating transaction former state %s payload %s", - json_encode($this->formerState), + "ApplyPromoCodeTask::undo: compensating transaction applied %s payload %s", + json_encode($this->applied), json_encode($this->payload) ) ); - $promo_codes_usage = $this->formerState['promo_codes_usage']; $owner_email = $this->payload['owner_email']; - foreach ($promo_codes_usage as $code => $info) { - Log::debug(sprintf("ApplyPromoCodeTask::undo undoing promo code %s info %s owner_email %s", $code, json_encode($info), $owner_email)); + // a failing release must not block the release of the remaining codes: + // failed entries are kept so a later undo() can retry them + $remaining = []; - $this->tx_service->transaction(function () use ($code, $info, $owner_email) { - $promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $code); - if (is_null($promo_code)) return; + foreach ($this->applied as $entry) { + $code = $entry['code']; + Log::debug(sprintf("ApplyPromoCodeTask::undo undoing promo code %s qty %s owner_email %s", $code, $entry['qty'], $owner_email)); - if (!isset($info['redeem'])) return; + try { + // reload the code: instances from the failed transaction are detached + // once its rollback cleared the EntityManager + $this->tx_service->transaction(function () use ($code, $entry, $owner_email) { + $promo_code = $this->promo_code_repository->getByValueExclusiveLock($this->summit, $code); + if (is_null($promo_code)) return; - $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $info, $owner_email) { - $promo_code->removeUsage(intval($info['qty']), $owner_email); - }, 30); + $this->lock_service->lock('promocode.' . $promo_code->getId() . '.usage.lock', function () use ($promo_code, $entry, $owner_email) { + $promo_code->removeUsage($entry['qty'], $owner_email); + }, 30); - }); + }); + } catch (\Throwable $ex) { + Log::warning(sprintf( + "ApplyPromoCodeTask::undo failed to release %s × %s: %s", + $code, $entry['qty'], $ex->getMessage() + )); + $remaining[] = $entry; + } } + + $this->applied = $remaining; } } diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index 24c42ffee..a07b0cd4d 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -44,6 +44,9 @@ use models\main\ITagRepository; use App\Models\Foundation\Summit\PromoCodes\PromoCodesConstants; use models\summit\SpeakersRegistrationDiscountCode; +use models\summit\SummitRegistrationDiscountCode; +use models\summit\SummitRegistrationPromoCode; +use models\summit\PresentationSpeaker; use models\summit\ISummitAttendeeRepository; use models\summit\ISummitAttendeeTicketRepository; use models\summit\ISummitRegistrationPromoCodeRepository; @@ -762,6 +765,137 @@ public function testReserveRejectedByPromoCodeLeavesNoOrderAndRestoresStock() $this->assertEquals($initial_quantity_sold, $reFetchedTicketType->getQuantitySold()); } + /** + * Each promo code is applied on its own transaction. When a later code of the same + * order is rejected, the usage already applied for the first one must be released + * together with the deleted order. + */ + public function testReserveRejectedOnSecondPromoCodeReleasesFirstCodeUsage() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $regular_code = new SummitRegistrationDiscountCode(); + $regular_code->setCode('TEST_DC_' . uniqid()); + $regular_code->setAmount(15); + $regular_code->setQuantityAvailable(10); + $regular_code->addAllowedTicketType(self::$default_ticket_type); + self::$summit->addPromoCode($regular_code); + + // the buyer is not assigned to this speakers code: ApplyPromoCodeTask rejects it + $speakers_code = new SpeakersRegistrationDiscountCode(); + $speakers_code->setCode('TEST_SPK_DC_' . uniqid()); + $speakers_code->setType(PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypes[0]); + $speakers_code->setAmount(100); + $speakers_code->addAllowedTicketType(self::$default_ticket_type); + self::$summit->addPromoCode($speakers_code); + + self::$em->persist(self::$summit); + self::$em->flush(); + + $regular_code_id = $regular_code->getId(); + $ticket_type_id = self::$default_ticket_type->getId(); + $initial_quantity_used = $regular_code->getQuantityUsed(); + + $payload = [ + "owner_email" => sprintf('not-a-speaker-%s@test.com', uniqid()), + "owner_first_name" => "Not", + "owner_last_name" => "Speaker", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => $ticket_type_id, "promo_code" => $regular_code->getCode()], + ["type_id" => $ticket_type_id, "promo_code" => $speakers_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(); + $re_fetched_code = self::$em->find(SummitRegistrationPromoCode::class, $regular_code_id); + $this->assertEquals($initial_quantity_used, $re_fetched_code->getQuantityUsed()); + } + + /** + * A speaker whose reservation was rejected because of another ticket's promo code + * must keep their speakers code unredeemed, so they can reserve again right away. + */ + public function testReserveRejectedOnSecondPromoCodeLeavesSpeakersCodeRedeemable() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $buyer_email = self::$defaultSpeaker->getEmail(); + + $speakers_code = new SpeakersRegistrationDiscountCode(); + $speakers_code->setCode('TEST_SPK_DC_' . uniqid()); + $speakers_code->setType(PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypes[0]); + $speakers_code->setAmount(100); + $speakers_code->addAllowedTicketType(self::$default_ticket_type); + $speakers_code->assignSpeaker(self::$defaultSpeaker); + self::$summit->addPromoCode($speakers_code); + + // expired: PreProcessReservationTask does not check the validity period, so it is + // ApplyPromoCodeTask that rejects it, after the speakers code was already applied + $now = new \DateTime('now', new \DateTimeZone('UTC')); + $expired_code = new SummitRegistrationDiscountCode(); + $expired_code->setCode('TEST_DC_' . uniqid()); + $expired_code->setAmount(15); + $expired_code->setQuantityAvailable(10); + $expired_code->setValidSinceDate((clone $now)->sub(new \DateInterval('P10D'))); + $expired_code->setValidUntilDate((clone $now)->sub(new \DateInterval('P5D'))); + $expired_code->addAllowedTicketType(self::$default_ticket_type); + self::$summit->addPromoCode($expired_code); + + self::$em->persist(self::$summit); + self::$em->flush(); + + $speakers_code_id = $speakers_code->getId(); + $speaker_id = self::$defaultSpeaker->getId(); + $ticket_type_id = self::$default_ticket_type->getId(); + + $payload = [ + "owner_email" => $buyer_email, + "owner_first_name" => "Sebastian", + "owner_last_name" => "Marcet", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => $ticket_type_id, "promo_code" => $speakers_code->getCode()], + ["type_id" => $ticket_type_id, "promo_code" => $expired_code->getCode()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('is not a valid code', $ex->getMessage()); + } + + self::$em->clear(); + $re_fetched_code = self::$em->find(SummitRegistrationPromoCode::class, $speakers_code_id); + $speaker = self::$em->find(PresentationSpeaker::class, $speaker_id); + $assignment = $re_fetched_code->getSpeakerAssignment($speaker); + $this->assertNotNull($assignment); + $this->assertNull($assignment->getRedeemedAt()); + + // the speaker can retry right away with their own code + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $retry_payload = $payload; + $retry_payload["tickets"] = [ + ["type_id" => $ticket_type_id, "promo_code" => $speakers_code->getCode()], + ]; + $order = $service->reserve(null, self::$summit, $retry_payload); + $this->assertNotNull($order); + + // release the seat so the fixture teardown can delete the ticket type + $service->cancel(self::$summit, $order->getHash()); + } + /** * ReserveTicketsTask rejects orders mixing ticket types of different currencies. */ diff --git a/tests/Unit/Services/SagaCompensationTest.php b/tests/Unit/Services/SagaCompensationTest.php index b671a5309..376621694 100644 --- a/tests/Unit/Services/SagaCompensationTest.php +++ b/tests/Unit/Services/SagaCompensationTest.php @@ -14,9 +14,15 @@ use App\Models\Foundation\Summit\Repositories\ISummitOrderRepository; use App\Services\Model\AbstractTask; +use App\Services\Model\ApplyPromoCodeTask; use App\Services\Model\ReserveOrderTask; use App\Services\Model\Saga; +use App\Services\Utils\ILockManagerService; use libs\utils\ITransactionService; +use models\exceptions\ValidationException; +use models\summit\ISummitRegistrationPromoCodeRepository; +use models\summit\SummitRegistrationPromoCode; +use models\summit\SummitTicketType; use Mockery; use models\main\Member; use models\summit\ISummitRepository; @@ -231,6 +237,247 @@ public function undo() { /* never runs — it threw in run() */ } $this->assertSame(['run:first', 'undo:first'], $order_of_calls); } + /** + * Saga::run() only marks a task as ran after run() returns, so when the second + * promo code is rejected the saga never undoes this task. run() must release the + * usage it already applied for the first code and rethrow the original exception. + */ + public function testApplyPromoCodeReleasesAppliedCodesWhenALaterCodeIsRejected(): void + { + $rejection = new ValidationException('code B is not valid for the buyer'); + + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('addUsage')->once()->with('buyer@test.com', 2); + $code_a->shouldReceive('removeUsage')->once()->with(2, 'buyer@test.com'); + + $code_b = $this->buildPromoCode(2, 'CODE_B'); + $code_b->shouldReceive('validate')->once()->andThrow($rejection); + $code_b->shouldReceive('addUsage')->never(); + $code_b->shouldReceive('removeUsage')->never(); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a, 'CODE_B' => $code_b]); + + try { + $task->run($this->promoCodesFormerState()); + $this->fail('Expected the rejection of the second promo code to propagate'); + } catch (ValidationException $ex) { + $this->assertSame($rejection, $ex); + } + + // a later Saga::abort() (or any extra undo) must not release the usage twice + $task->undo(); + } + + public function testApplyPromoCodeDoesNotReleaseAnythingWhenTheFirstCodeIsRejected(): void + { + $rejection = new ValidationException('code A is not valid for the buyer'); + + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('validate')->once()->andThrow($rejection); + $code_a->shouldReceive('addUsage')->never(); + $code_a->shouldReceive('removeUsage')->never(); + + $code_b = $this->buildPromoCode(2, 'CODE_B'); + $code_b->shouldReceive('addUsage')->never(); + $code_b->shouldReceive('removeUsage')->never(); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a, 'CODE_B' => $code_b]); + + try { + $task->run($this->promoCodesFormerState()); + $this->fail('Expected the rejection of the first promo code to propagate'); + } catch (ValidationException $ex) { + $this->assertSame($rejection, $ex); + } + } + + /** + * A successful run leaves every usage applied; if a later saga task fails the + * saga undoes this one, releasing each code exactly once however often undo() runs. + */ + public function testApplyPromoCodeUndoAfterSuccessfulRunReleasesEachCodeOnce(): void + { + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('addUsage')->once()->with('buyer@test.com', 2); + $code_a->shouldReceive('removeUsage')->once()->with(2, 'buyer@test.com'); + + $code_b = $this->buildPromoCode(2, 'CODE_B'); + $code_b->shouldReceive('addUsage')->once()->with('buyer@test.com', 1); + $code_b->shouldReceive('removeUsage')->once()->with(1, 'buyer@test.com'); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a, 'CODE_B' => $code_b]); + + $task->run($this->promoCodesFormerState()); + $task->undo(); + $task->undo(); + + // the once() expectations above are verified on Mockery::close() + $this->addToAssertionCount(Mockery::getContainer()->mockery_getExpectationCount()); + } + + /** + * Without a successful run there is nothing to release: an undo() must never + * touch a usage that belongs to another order of the same buyer. + */ + public function testApplyPromoCodeUndoWithoutRunReleasesNothing(): void + { + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('removeUsage')->never(); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a]); + $this->setPrivate($task, 'formerState', $this->promoCodesFormerState()); + + $task->undo(); + + $this->addToAssertionCount(Mockery::getContainer()->mockery_getExpectationCount()); + } + + /** + * If releasing the first code fails while compensating, the caller still gets + * the exception that rejected the reservation, not the compensation failure. + */ + public function testApplyPromoCodeFailingReleaseDoesNotMaskTheRejection(): void + { + $rejection = new ValidationException('code B is not valid for the buyer'); + + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('addUsage')->once(); + $code_a->shouldReceive('removeUsage')->once()->andThrow(new \RuntimeException('release failed')); + + $code_b = $this->buildPromoCode(2, 'CODE_B'); + $code_b->shouldReceive('validate')->once()->andThrow($rejection); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a, 'CODE_B' => $code_b]); + + try { + $task->run($this->promoCodesFormerState()); + $this->fail('Expected the rejection of the second promo code to propagate'); + } catch (ValidationException $ex) { + $this->assertSame($rejection, $ex); + } + } + + /** + * A release that fails for one code must not leak the usage of the other codes + * applied by the same run, and a later undo() retries only the failed one. + */ + public function testApplyPromoCodeFailingReleaseDoesNotBlockTheOtherCodes(): void + { + $rejection = new ValidationException('code C is not valid for the buyer'); + + $release_attempts = 0; + $code_a = $this->buildPromoCode(1, 'CODE_A'); + $code_a->shouldReceive('addUsage')->once()->with('buyer@test.com', 2); + $code_a->shouldReceive('removeUsage')->twice()->with(2, 'buyer@test.com') + ->andReturnUsing(function () use (&$release_attempts) { + if (++$release_attempts === 1) throw new \RuntimeException('release failed'); + }); + + $code_b = $this->buildPromoCode(2, 'CODE_B'); + $code_b->shouldReceive('addUsage')->once()->with('buyer@test.com', 1); + $b_released = false; + $code_b->shouldReceive('removeUsage')->once()->with(1, 'buyer@test.com') + ->andReturnUsing(function () use (&$b_released) { + $b_released = true; + }); + + $code_c = $this->buildPromoCode(3, 'CODE_C'); + $code_c->shouldReceive('validate')->once()->andThrow($rejection); + $code_c->shouldReceive('addUsage')->never(); + $code_c->shouldReceive('removeUsage')->never(); + + $task = $this->buildApplyPromoCodeTask(['CODE_A' => $code_a, 'CODE_B' => $code_b, 'CODE_C' => $code_c]); + + $former_state = $this->promoCodesFormerState(); + $former_state['promo_codes_usage']['CODE_C'] = ['qty' => 1, 'types' => [10]]; + + try { + $task->run($former_state); + $this->fail('Expected the rejection of the third promo code to propagate'); + } catch (ValidationException $ex) { + $this->assertSame($rejection, $ex); + } + + // CODE_B is released by run() itself, despite CODE_A failing before it + $this->assertTrue($b_released); + $this->assertSame(1, $release_attempts); + + // retries CODE_A only: CODE_B was already released + $task->undo(); + // nothing left to release + $task->undo(); + + $this->assertSame(2, $release_attempts); + } + + private function promoCodesFormerState(): array + { + return [ + 'promo_codes_usage' => [ + 'CODE_A' => ['qty' => 2, 'types' => [10]], + 'CODE_B' => ['qty' => 1, 'types' => [10]], + ], + ]; + } + + /** + * @return \Mockery\MockInterface|SummitRegistrationPromoCode + */ + private function buildPromoCode(int $id, string $code) + { + $promo_code = Mockery::mock(SummitRegistrationPromoCode::class); + $promo_code->shouldReceive('getId')->andReturn($id); + $promo_code->shouldReceive('getCode')->andReturn($code); + $promo_code->shouldReceive('getSummitId')->andReturn(77); + $promo_code->shouldReceive('validate')->andReturnNull()->byDefault(); + $promo_code->shouldReceive('canBeAppliedTo')->andReturn(true); + return $promo_code; + } + + /** + * @param array $promo_codes by code value + */ + private function buildApplyPromoCodeTask(array $promo_codes): ApplyPromoCodeTask + { + $ticket_type = Mockery::mock(SummitTicketType::class); + $ticket_type->shouldReceive('getName')->andReturn('Ticket'); + + $summit = Mockery::mock(Summit::class); + $summit->shouldReceive('getId')->andReturn(77); + $summit->shouldReceive('getTicketTypeById')->andReturn($ticket_type); + + // run() re attaches the summit through the repository + $summit_repo = Mockery::mock(ISummitRepository::class); + $summit_repo->shouldReceive('getById')->with(77)->andReturn($summit); + $container = \Illuminate\Support\Facades\Facade::getFacadeApplication(); + $container->instance(ISummitRepository::class, $summit_repo); + + $promo_code_repo = Mockery::mock(ISummitRegistrationPromoCodeRepository::class); + $promo_code_repo->shouldReceive('getByValueExclusiveLock') + ->andReturnUsing(function ($summit, $code) use ($promo_codes) { + return $promo_codes[$code] ?? null; + }); + + $tx_service = Mockery::mock(ITransactionService::class); + $tx_service->shouldReceive('transaction')->andReturnUsing(function ($fn) { + return $fn(); + }); + + $lock_service = Mockery::mock(ILockManagerService::class); + $lock_service->shouldReceive('lock')->andReturnUsing(function ($name, $fn) { + return $fn(); + }); + + return new ApplyPromoCodeTask( + $summit, + ['owner_email' => 'buyer@test.com', 'owner_company' => 'Acme'], + null, + $promo_code_repo, + $tx_service, + $lock_service + ); + } + /** * Construct a ReserveOrderTask with only the fields undo() needs. run() is * not exercised here, so most collaborators can be plain Mockery doubles.