From d1b9d032faa2ebe02657d7a67d3fb9a0c9c161b5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 6 Oct 2026 14:47:17 -0300 Subject: [PATCH 1/2] fix(tickets): adjust ticket type sold counters when a ticket's type is changed updateTicket reassigned the ticket type through upgradeTicketType() without touching quantity_sold, so the old type kept a sale it no longer had and the new type never counted it. Over time a type could look sold out while seats were still free, blocking promo codes that unlock it. When ticket_type_id differs from the current type, lock both types in ascending id order, sell(1) on the new one (a sold out type rejects the whole update) and restore(1) on the old one (a counter already out of sync is only logged). An unchanged ticket_type_id leaves the counters alone. --- app/Services/Model/Imp/SummitOrderService.php | 23 ++++ tests/SummitOrderServiceTest.php | 100 ++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 0c216b7c6..a03127519 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -4057,8 +4057,31 @@ public function updateTicket(Summit $summit, int $order_id, int $ticket_id, arra if (is_null($ticket_type)) throw new EntityNotFoundException("ticket type not found"); + $old_ticket_type_id = $ticket->getTicketTypeId(); + $ticket->upgradeTicketType($ticket_type); + // quantity_sold is a hand maintained counter: move the sale from the old type to the new one + if ($old_ticket_type_id !== $ticket_type->getId()) { + // lock in ascending id order so two opposite moves can not deadlock + $ids = [$old_ticket_type_id, $ticket_type->getId()]; + sort($ids); + $locked_types = []; + foreach ($ids as $id) { + $locked_types[$id] = $this->ticket_type_repository->getByIdExclusiveLock($id, true); + } + + // fails (and rolls back the whole update) if the new type has no seats left + $locked_types[$ticket_type->getId()]->sell(1); + + try { + $locked_types[$old_ticket_type_id]->restore(1); + } catch (ValidationException $ex) { + // old counter already out of sync (below the restored qty), do not block the admin + Log::warning($ex); + } + } + $shouldSendInvitationEmail = true; } diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index 85f1640bb..cd4b22331 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -1460,6 +1460,106 @@ public function testUpdateTicketBadgeTypeChangeDoesNotDuplicateBadge(){ $this->assertEquals('NEW BADGE TYPE', $badge->getType()->getName()); } + /** + * Resolves the real ticket of the default attendee (see the fixture note in + * testUpdateTicketReassignmentRegeneratesBadgeQRCode) and primes the sold counters: + * fixture tickets are created without SummitTicketType::sell(), so the old type is + * sold once (unless $sell_old_type is false, i.e. a counter already out of sync) to make a decrement observable. + * @return array [summit_id, order_id, ticket_id, old_type_id, new_type_id] + */ + private function prepareTicketTypeChange(int $new_type_quantity_2_sell = 100, int $new_type_sold = 0, bool $sell_old_type = true): array + { + $attendee = self::$summit->getAttendeeByMember(self::$defaultMember); + $this->assertNotNull($attendee); + $badge_id = $attendee->getTickets()->first()->getBadge()->getId(); + + $summit_id = self::$summit->getId(); + + $real_ticket = EntityManager::getRepository(SummitAttendeeBadge::class)->find($badge_id)->getTicket(); + $ticket_id = $real_ticket->getId(); + $order_id = $real_ticket->getOrder()->getId(); + $old_type = $real_ticket->getTicketType(); + $new_type = self::$default_ticket_type_2; + $this->assertNotEquals($old_type->getId(), $new_type->getId()); + + if ($sell_old_type) $old_type->sell(1); + $new_type->setQuantity2Sell($new_type_quantity_2_sell); + if ($new_type_sold > 0) $new_type->sell($new_type_sold); + self::$em->persist($old_type); + self::$em->persist($new_type); + self::$em->flush(); + + $ids = [$summit_id, $order_id, $ticket_id, $old_type->getId(), $new_type->getId()]; + EntityManager::clear(); + return $ids; + } + + public function testUpdateTicketTypeChangeMovesSoldCounter() + { + Queue::fake(); + list($summit_id, $order_id, $ticket_id, $old_type_id, $new_type_id) = $this->prepareTicketTypeChange(); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + App::make(ISummitOrderService::class) + ->updateTicket($summit, $order_id, $ticket_id, ['ticket_type_id' => $new_type_id]); + + EntityManager::clear(); + $ticket = EntityManager::getRepository(SummitAttendeeTicket::class)->find($ticket_id); + $this->assertEquals($new_type_id, $ticket->getTicketTypeId()); + $this->assertEquals(0, EntityManager::getRepository(SummitTicketType::class)->find($old_type_id)->getQuantitySold()); + $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($new_type_id)->getQuantitySold()); + } + + public function testUpdateTicketTypeChangeWithOldCounterAlreadyAtZeroStillMovesTheTicket() + { + Queue::fake(); + list($summit_id, $order_id, $ticket_id, $old_type_id, $new_type_id) = $this->prepareTicketTypeChange(100, 0, false); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + App::make(ISummitOrderService::class) + ->updateTicket($summit, $order_id, $ticket_id, ['ticket_type_id' => $new_type_id]); + + EntityManager::clear(); + $ticket = EntityManager::getRepository(SummitAttendeeTicket::class)->find($ticket_id); + $this->assertEquals($new_type_id, $ticket->getTicketTypeId()); + $this->assertEquals(0, EntityManager::getRepository(SummitTicketType::class)->find($old_type_id)->getQuantitySold()); + $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($new_type_id)->getQuantitySold()); + } + + public function testUpdateTicketSameTypeLeavesSoldCounterUntouched() + { + Queue::fake(); + list($summit_id, $order_id, $ticket_id, $old_type_id) = $this->prepareTicketTypeChange(); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + App::make(ISummitOrderService::class) + ->updateTicket($summit, $order_id, $ticket_id, ['ticket_type_id' => $old_type_id]); + + EntityManager::clear(); + $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($old_type_id)->getQuantitySold()); + } + + public function testUpdateTicketToSoldOutTypeFailsAndLeavesCountersUntouched() + { + Queue::fake(); + list($summit_id, $order_id, $ticket_id, $old_type_id, $new_type_id) = $this->prepareTicketTypeChange(1, 1); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + try { + App::make(ISummitOrderService::class) + ->updateTicket($summit, $order_id, $ticket_id, ['ticket_type_id' => $new_type_id]); + $this->fail('Moving a ticket to a sold out ticket type should be rejected.'); + } catch (ValidationException $ex) { + // expected + } + + EntityManager::clear(); + $ticket = EntityManager::getRepository(SummitAttendeeTicket::class)->find($ticket_id); + $this->assertEquals($old_type_id, $ticket->getTicketTypeId()); + $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($old_type_id)->getQuantitySold()); + $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($new_type_id)->getQuantitySold()); + } + public function testAddTicketsCommitsAcrossNestedTransaction() { $service = App::make(ISummitOrderService::class); From eabd3d8742424b0b3fb8014c18152ac58e5a743a Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 7 Oct 2026 13:14:11 -0300 Subject: [PATCH 2/2] fix(tickets): enqueue the revocation email only after the ticket update commits updateTicket dispatched RevocationTicketEmail inside the Doctrine transaction. The database queue is not transactional (after_commit is off and it writes through a separate connection), so when a later step rolled the update back (e.g. moving the ticket to a sold out type) the former owner was still told the ticket had been revoked, and the dedup cache key was burnt for 10 minutes. Capture the former owner inside the transaction and enqueue the email once it commits, through SummitAttendee::dispatchRevocationTicketEmail, which skips the current-owner guard that would otherwise turn the post-commit call into a no-op. --- .../Registration/Attendees/SummitAttendee.php | 11 ++++ app/Services/Model/Imp/SummitOrderService.php | 13 +++- tests/SummitOrderServiceTest.php | 66 +++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php index 913341e66..0453ec93c 100644 --- a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php +++ b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php @@ -528,6 +528,17 @@ public function sendRevocationTicketEmail(SummitAttendeeTicket $ticket) if (!$ticket->hasOwner()) return; if ($ticket->getOwner()->getId() != $this->getId()) return; + $this->dispatchRevocationTicketEmail($ticket); + } + + /** + * Enqueues the revocation email without checking the current ticket owner: use it once a + * reassignment is committed and $this is no longer the owner (the queue is not transactional, + * so enqueueing inside the transaction would notify the former owner even on rollback). + * @param SummitAttendeeTicket $ticket + */ + public function dispatchRevocationTicketEmail(SummitAttendeeTicket $ticket): void + { $email = $this->getEmail(); $key = md5($email); if (Cache::add(sprintf("%s_revoke_ticket", $key), true, 600)) { diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index a03127519..5f9f717e3 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -3953,7 +3953,7 @@ public function updateTicketById(Member $current_user, int $ticket_id, array $pa */ public function updateTicket(Summit $summit, int $order_id, int $ticket_id, array $payload): SummitAttendeeTicket { - list($ticket, $shouldSendInvitationEmail) = $this->tx_service->transaction(function () use ($summit, $order_id, $ticket_id, $payload) { + list($ticket, $shouldSendInvitationEmail, $revoked_owner) = $this->tx_service->transaction(function () use ($summit, $order_id, $ticket_id, $payload) { // lock and get the order $order = $this->order_repository->getByIdExclusiveLock($order_id); @@ -4026,9 +4026,12 @@ public function updateTicket(Summit $summit, int $order_id, int $ticket_id, arra } $shouldSendInvitationEmail = false; + $revoked_owner = null; // we are doing a reassignment from owner to new owner if (!is_null($owner) && !is_null($new_owner) && $owner->getId() !== $new_owner->getId()) { - $owner->sendRevocationTicketEmail($ticket); + // the queue is not transactional: the revocation email is enqueued after commit (see below), + // so a rollback further down (e.g. sold out ticket type) does not notify the former owner + $revoked_owner = $owner; $owner->removeTicket($ticket); $owner->updateStatus(); } @@ -4097,9 +4100,13 @@ public function updateTicket(Summit $summit, int $order_id, int $ticket_id, arra $ticket->setBadge($badge); } - return [$ticket, $shouldSendInvitationEmail]; + return [$ticket, $shouldSendInvitationEmail, $revoked_owner]; }); + // the reassignment is committed: now it is safe to tell the former owner + if (!is_null($revoked_owner)) + $revoked_owner->dispatchRevocationTicketEmail($ticket); + if ($shouldSendInvitationEmail && $summit->isRegistrationSendTicketEmailAutomatically() && $ticket->hasOwner()) $ticket->getOwner()->sendInvitationEmail($ticket); diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index cd4b22331..395616e36 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -14,6 +14,7 @@ use App\Jobs\Emails\Registration\Reminders\SummitOrderReminderEmail; use App\Jobs\Emails\Registration\Reminders\SummitTicketReminderEmail; +use App\Jobs\Emails\RevocationTicketEmail; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeConstants; use App\Models\Foundation\ExtraQuestions\ExtraQuestionTypeValue; use App\Models\Foundation\Main\IGroup; @@ -1560,6 +1561,71 @@ public function testUpdateTicketToSoldOutTypeFailsAndLeavesCountersUntouched() $this->assertEquals(1, EntityManager::getRepository(SummitTicketType::class)->find($new_type_id)->getQuantitySold()); } + /** + * The revocation email is deduplicated per attendee email with a 10 minutes cache key + * (SummitAttendee::dispatchRevocationTicketEmail); the test cache driver is not reset between + * tests, so the key must be cleared for the Queue assertions below to be meaningful. + */ + private function forgetRevocationEmailDedup(string $email): void + { + Cache::forget(sprintf("%s_revoke_ticket", md5($email))); + } + + public function testUpdateTicketReassignmentToSoldOutTypeDoesNotSendRevocationEmail() + { + Queue::fake(); + $old_owner_email = self::$defaultMember->getEmail(); + $new_owner_email = self::$member2->getEmail(); + $this->forgetRevocationEmailDedup($old_owner_email); + list($summit_id, $order_id, $ticket_id, $old_type_id, $new_type_id) = $this->prepareTicketTypeChange(1, 1); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + try { + App::make(ISummitOrderService::class)->updateTicket($summit, $order_id, $ticket_id, [ + 'attendee_email' => $new_owner_email, + 'ticket_type_id' => $new_type_id, + ]); + $this->fail('Moving a ticket to a sold out ticket type should be rejected.'); + } catch (ValidationException $ex) { + // expected + } + + // the whole update rolled back: the former owner must not be told the ticket was revoked + Queue::assertNotPushed(RevocationTicketEmail::class); + + EntityManager::clear(); + $ticket = EntityManager::getRepository(SummitAttendeeTicket::class)->find($ticket_id); + $this->assertEquals($old_type_id, $ticket->getTicketTypeId()); + $this->assertEquals($old_owner_email, $ticket->getOwner()->getEmail()); + } + + public function testUpdateTicketReassignmentWithTypeChangeSendsRevocationEmailAfterCommit() + { + Queue::fake(); + $old_owner_email = self::$defaultMember->getEmail(); + $new_owner_email = self::$member2->getEmail(); + $this->forgetRevocationEmailDedup($old_owner_email); + list($summit_id, $order_id, $ticket_id, , $new_type_id) = $this->prepareTicketTypeChange(); + + $summit = EntityManager::getRepository(Summit::class)->find($summit_id); + App::make(ISummitOrderService::class)->updateTicket($summit, $order_id, $ticket_id, [ + 'attendee_email' => $new_owner_email, + 'ticket_type_id' => $new_type_id, + ]); + + // the email must go to the former owner even though, once committed, the ticket belongs to the new one + Queue::assertPushed(RevocationTicketEmail::class, function (RevocationTicketEmail $job) use ($old_owner_email) { + $prop = new \ReflectionProperty(\App\Jobs\Emails\AbstractEmailJob::class, 'to_email'); + $prop->setAccessible(true); + return $prop->getValue($job) === $old_owner_email; + }); + + EntityManager::clear(); + $ticket = EntityManager::getRepository(SummitAttendeeTicket::class)->find($ticket_id); + $this->assertEquals($new_type_id, $ticket->getTicketTypeId()); + $this->assertEquals($new_owner_email, $ticket->getOwner()->getEmail()); + } + public function testAddTicketsCommitsAcrossNestedTransaction() { $service = App::make(ISummitOrderService::class);