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 0c216b7c6..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(); } @@ -4057,8 +4060,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; } @@ -4074,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 85f1640bb..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; @@ -1460,6 +1461,171 @@ 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()); + } + + /** + * 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);