From eac186f47ccafd47e3b6730964c26e92e0207ce5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:03:25 -0300 Subject: [PATCH 01/19] fix(repositories): add deterministic ORDER BY to getAllIdsByPage pagination getAllIdsByPage applied setFirstResult/setMaxResults with no ORDER BY, so MySQL was free to return a different row order per page. Paging through a filtered result set could silently skip or repeat rows across pages. Route through the existing getParametrizedAllIdsByPage helper with a default ORDER BY e.id ASC fallback when no explicit Order is given - the same pattern DoctrineSpeakerRepository::getSpeakersIdsBySummit already uses. Affects all 8 services calling this shared method. --- app/Repositories/DoctrineRepository.php | 47 +++---- .../DoctrineSummitAttendeeRepositoryTest.php | 115 ++++++++++++++++++ 2 files changed, 135 insertions(+), 27 deletions(-) create mode 100644 tests/DoctrineSummitAttendeeRepositoryTest.php diff --git a/app/Repositories/DoctrineRepository.php b/app/Repositories/DoctrineRepository.php index 76c74119d..ecea26e6b 100644 --- a/app/Repositories/DoctrineRepository.php +++ b/app/Repositories/DoctrineRepository.php @@ -341,39 +341,32 @@ public function getFastCount(Filter $filter = null, Order $order = null){ } /** + * With no explicit $order, pagination falls back to ORDER BY e.id ASC so paging through a + * filtered result set is deterministic - LIMIT/OFFSET with no ORDER BY at all lets MySQL + * return a different row order per page, silently skipping or repeating rows across pages. + * Routed through getParametrizedAllIdsByPage so every caller (8 services across the + * codebase) gets the same join/filter handling as before, plus this one default-order + * fallback - the same contract getParametrizedAllByPage below already documents. + * * @param PagingInfo $paging_info * @param Filter|null $filter * @param Order|null $order * @return array */ public function getAllIdsByPage(PagingInfo $paging_info, Filter $filter = null, Order $order = null):array { - - $query = $this->getEntityManager() - ->createQueryBuilder() - ->distinct(true) - ->select("e.id") - ->from($this->getBaseEntity(), "e"); - - $query = $this->applyExtraJoins($query, $filter, $order); - - $query = $this->applyExtraSelects($query, $filter, $order); - - if(!is_null($filter)){ - $filter->apply2Query($query, $this->getFilterMappings($filter)); - } - - $query = $this->applyExtraFilters($query); - - if(!is_null($order)){ - $order->apply2Query($query, $this->getOrderMappings($filter)); - } - - $query = $query - ->setFirstResult($paging_info->getOffset()) - ->setMaxResults($paging_info->getPerPage()); - - $res = $query->getQuery()->getArrayResult(); - return array_column($res, 'id'); + return $this->getParametrizedAllIdsByPage(function () { + return $this->getEntityManager() + ->createQueryBuilder() + ->distinct(true) + ->select("e.id") + ->from($this->getBaseEntity(), "e"); + }, + $paging_info, + $filter, + $order, + function ($query) { + return $query->addOrderBy("e.id", 'ASC'); + }); } /** diff --git a/tests/DoctrineSummitAttendeeRepositoryTest.php b/tests/DoctrineSummitAttendeeRepositoryTest.php new file mode 100644 index 000000000..c29465fc8 --- /dev/null +++ b/tests/DoctrineSummitAttendeeRepositoryTest.php @@ -0,0 +1,115 @@ +addFilterCondition(FilterElement::makeEqual('summit_id', self::$summit->getId())); + + $captured_sql = []; + $logger = new class($captured_sql) implements SQLLogger { + private array $sink; + + public function __construct(array &$sink) + { + $this->sink = &$sink; + } + + public function startQuery($sql, ?array $params = null, ?array $types = null) + { + $this->sink[] = $sql; + } + + public function stopQuery() + { + } + }; + + $connection = Registry::getManager(SilverstripeBaseModel::EntityManager)->getConnection(); + $previous_logger = $connection->getConfiguration()->getSQLLogger(); + $connection->getConfiguration()->setSQLLogger($logger); + + try { + $repository->getAllIdsByPage(new PagingInfo(1, 2), $filter); + } finally { + $connection->getConfiguration()->setSQLLogger($previous_logger); + } + + $this->assertNotEmpty($captured_sql, 'expected at least one query to be logged for getAllIdsByPage'); + + $select_queries = array_values(array_filter($captured_sql, function ($sql) { + return stripos($sql, 'SELECT') === 0; + })); + + $this->assertNotEmpty($select_queries, 'expected the paginated attendee id SELECT to be logged'); + + $matched = array_filter($select_queries, function ($sql) { + return stripos($sql, 'ORDER BY') !== false; + }); + + $this->assertNotEmpty( + $matched, + sprintf( + 'getAllIdsByPage with no explicit Order must still generate an ORDER BY clause so pagination is deterministic across pages; captured SQL: %s', + implode(" | ", $select_queries) + ) + ); + } +} From 8accd35109c508e5f528cb4a46a6ae469ccef10d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:17:21 -0300 Subject: [PATCH 02/19] feat(attendees): add SummitAttendeeAnnouncementEmail sent-proof entity Attendees had no per-recipient, per-email-type, timestamped proof of a sent email - InvitationEmailSentDate only covers the invitation path and carries no type dimension. This is the prerequisite for a retry-safe bulk send (a resumed chunk needs to know who it already reached). SummitAttendeeAnnouncementEmail mirrors SpeakerAnnouncementSummitEmail, adapted for the one shape speakers don't have: SummitAttendeeTicketEmailStrategy sends up to one email per ticket, not one per attendee, so this carries an optional ticket association. SummitAttendee gains the EXTRA_LAZY collection, addAnnouncementEmail/ removeAnnouncementEmail, and hasAnnouncementEmailTypeSentSince - a bounded matching() query, not a full hydration. --- .../Registration/Attendees/SummitAttendee.php | 52 ++++++ .../SummitAttendeeAnnouncementEmail.php | 129 ++++++++++++++ .../model/Version20260908190443.php | 61 +++++++ tests/SummitAttendeeAnnouncementEmailTest.php | 157 ++++++++++++++++++ 4 files changed, 399 insertions(+) create mode 100644 app/Models/Foundation/Summit/Registration/Attendees/SummitAttendeeAnnouncementEmail.php create mode 100644 database/migrations/model/Version20260908190443.php create mode 100644 tests/SummitAttendeeAnnouncementEmailTest.php diff --git a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php index 9fd768724..f0b717387 100644 --- a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php +++ b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php @@ -216,6 +216,12 @@ public function getSummitHallCheckedIn() #[ORM\ManyToMany(targetEntity: \models\main\Tag::class, cascade: ['persist'], fetch: 'EXTRA_LAZY')] private $tags; + /** + * @var SummitAttendeeAnnouncementEmail[] + */ + #[ORM\OneToMany(targetEntity: \models\summit\SummitAttendeeAnnouncementEmail::class, mappedBy: 'attendee', cascade: ['persist', 'remove'], orphanRemoval: true, fetch: 'EXTRA_LAZY')] + private $announcement_emails; + /** * @param bool $summit_hall_checked_in */ @@ -297,6 +303,51 @@ public function getFirstTicket():?SummitAttendeeTicket{ return $this->tickets->first(); } + public function addAnnouncementEmail(SummitAttendeeAnnouncementEmail $announcementEmail) + { + if ($this->announcement_emails->contains($announcementEmail)) return; + $this->announcement_emails->add($announcementEmail); + $announcementEmail->setAttendee($this); + } + + public function removeAnnouncementEmail(SummitAttendeeAnnouncementEmail $announcementEmail) + { + if (!$this->announcement_emails->contains($announcementEmail)) return; + $this->announcement_emails->removeElement($announcementEmail); + $announcementEmail->clearAttendee(); + } + + /** + * Per-recipient, per-email-type, timestamped proof check backing the bulk-send resume + * predicate (AbstractEmailAction::alreadySentSince). Bounded matching() query on the + * EXTRA_LAZY announcement_emails collection - does not hydrate it. $ticket distinguishes + * SummitAttendeeTicketEmailStrategy's per-ticket proofs (one email per active paid ticket) + * from every other strategy's single per-attendee proof (ticket left null). + * + * @param Summit $summit + * @param string $type + * @param \DateTime $since + * @param SummitAttendeeTicket|null $ticket + * @return bool + */ + public function hasAnnouncementEmailTypeSentSince(Summit $summit, string $type, \DateTime $since, ?SummitAttendeeTicket $ticket = null): bool + { + $criteria = Criteria::create(); + + $criteria + ->where(Criteria::expr()->eq('summit', $summit)) + ->andWhere(Criteria::expr()->eq('type', $type)) + ->andWhere(Criteria::expr()->gte('send_date', $since)); + + if (!is_null($ticket)) { + $criteria->andWhere(Criteria::expr()->eq('ticket', $ticket)); + } else { + $criteria->andWhere(Criteria::expr()->isNull('ticket')); + } + + return $this->announcement_emails->matching($criteria)->count() > 0; + } + /** * @param SummitAttendeeTicket $ticket */ @@ -349,6 +400,7 @@ public function __construct() $this->managed_attendees = new ArrayCollection(); $this->manager = null; $this->rsvp_invitations = new ArrayCollection(); + $this->announcement_emails = new ArrayCollection(); } public function isVirtualCheckedIn(): bool diff --git a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendeeAnnouncementEmail.php b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendeeAnnouncementEmail.php new file mode 100644 index 000000000..59c1ca591 --- /dev/null +++ b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendeeAnnouncementEmail.php @@ -0,0 +1,129 @@ +type; + } + + /** + * @param string $type + */ + public function setType(string $type) + { + $this->type = $type; + } + + /** + * @return DateTime + */ + public function getSendDate():?DateTime + { + return $this->send_date; + } + + public function isSent():bool{ + return !is_null($this->send_date); + } + + public function markAsSent():void{ + $this->send_date = new DateTime('now', new \DateTimeZone('UTC')); + } + + /** + * @return SummitAttendee + */ + public function getAttendee():?SummitAttendee + { + return $this->attendee; + } + + /** + * @param SummitAttendee $attendee + */ + public function setAttendee($attendee) + { + $this->attendee = $attendee; + } + + public function clearAttendee():void{ + $this->attendee = null; + } + + /** + * @return SummitAttendeeTicket|null + */ + public function getTicket():?SummitAttendeeTicket + { + return $this->ticket; + } + + /** + * @param SummitAttendeeTicket|null $ticket + */ + public function setTicket(?SummitAttendeeTicket $ticket):void + { + $this->ticket = $ticket; + } + +} diff --git a/database/migrations/model/Version20260908190443.php b/database/migrations/model/Version20260908190443.php new file mode 100644 index 000000000..dc9025e2a --- /dev/null +++ b/database/migrations/model/Version20260908190443.php @@ -0,0 +1,61 @@ +string("AnnouncementEmailTypeSent")->setNotnull(true); + $table->timestamp("AnnouncementEmailSentDate")->setNotnull(true); + + // FK + $table->integer("AttendeeID", false, false)->setNotnull(true); + $table->index("AttendeeID", "AttendeeID"); + $table->foreign("SummitAttendee", "AttendeeID", "ID", ["onDelete" => "CASCADE"]); + + // FK + $table->integer("TicketID", false, false)->setNotnull(false)->setDefault(null); + $table->index("TicketID", "TicketID"); + $table->foreign("SummitAttendeeTicket", "TicketID", "ID", ["onDelete" => "CASCADE"]); + + // FK + $table->integer("SummitID", false, false)->setNotnull(true); + $table->index("SummitID", "SummitID"); + $table->foreign("Summit", "SummitID", "ID", ["onDelete" => "CASCADE"]); + + $table->index(["AttendeeID", "AnnouncementEmailTypeSent"], "AttendeeID_AnnouncementEmailTypeSent"); + }); + } + + /** + * @param Schema $schema + */ + public function down(Schema $schema): void + { + $schema->dropTable(self::TableName); + } +} diff --git a/tests/SummitAttendeeAnnouncementEmailTest.php b/tests/SummitAttendeeAnnouncementEmailTest.php new file mode 100644 index 000000000..a479fc8b7 --- /dev/null +++ b/tests/SummitAttendeeAnnouncementEmailTest.php @@ -0,0 +1,157 @@ +getAttendees()->first(); + $this->assertNotNull($attendee); + return [self::$summit->getId(), $attendee->getId()]; + } + + /** + * @return array{0: Summit, 1: SummitAttendee} + */ + private function reloadFresh(int $summit_id, int $attendee_id): array + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $summit = $em->getRepository(Summit::class)->find($summit_id); + $attendee = $em->getRepository(SummitAttendee::class)->find($attendee_id); + return [$summit, $attendee]; + } + + public function testAddAnnouncementEmailPersistsViaCascadeAndSetsAttendeeAndSummit(): void + { + [$summit_id, $attendee_id] = $this->fixtureIds(); + [$summit, $attendee] = $this->reloadFresh($summit_id, $attendee_id); + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + + $proof = new SummitAttendeeAnnouncementEmail(); + $proof->setType('SUMMIT_REGISTRATION_GENERIC_EMAIL'); + $proof->setSummit($summit); + $attendee->addAnnouncementEmail($proof); + $proof->markAsSent(); + + $em->persist($proof); + $em->flush(); + + $this->assertNotNull($proof->getId()); + $this->assertTrue($proof->isSent()); + $this->assertSame($attendee_id, $proof->getAttendee()->getId()); + $this->assertSame($summit_id, $proof->getSummit()->getId()); + $this->assertNull($proof->getTicket()); + } + + public function testHasAnnouncementEmailTypeSentSinceMatchesOnAttendeeTypeAndDate(): void + { + [$summit_id, $attendee_id] = $this->fixtureIds(); + [$summit, $attendee] = $this->reloadFresh($summit_id, $attendee_id); + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + + $type = 'SUMMIT_REGISTRATION_GENERIC_EMAIL'; + + $before = new \DateTime('now', new \DateTimeZone('UTC')); + $before->sub(new \DateInterval('PT1M')); + + $proof = new SummitAttendeeAnnouncementEmail(); + $proof->setType($type); + $proof->setSummit($summit); + $attendee->addAnnouncementEmail($proof); + $proof->markAsSent(); + + $em->persist($proof); + $em->flush(); + + $this->assertTrue($attendee->hasAnnouncementEmailTypeSentSince($summit, $type, $before)); + + $after = new \DateTime('now', new \DateTimeZone('UTC')); + $after->add(new \DateInterval('PT1M')); + $this->assertFalse($attendee->hasAnnouncementEmailTypeSentSince($summit, $type, $after)); + + $this->assertFalse($attendee->hasAnnouncementEmailTypeSentSince($summit, 'A_DIFFERENT_TYPE', $before)); + } + + public function testHasAnnouncementEmailTypeSentSinceDistinguishesByTicket(): void + { + [$summit_id, $attendee_id] = $this->fixtureIds(); + [$summit, $attendee] = $this->reloadFresh($summit_id, $attendee_id); + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $ticket = $attendee->getTickets()->first(); + $this->assertNotNull($ticket); + + $type = 'SUMMIT_REGISTRATION_TICKET_EMAIL'; + $before = new \DateTime('now', new \DateTimeZone('UTC')); + $before->sub(new \DateInterval('PT1M')); + + $proof = new SummitAttendeeAnnouncementEmail(); + $proof->setType($type); + $proof->setSummit($summit); + $proof->setTicket($ticket); + $attendee->addAnnouncementEmail($proof); + $proof->markAsSent(); + + $em->persist($proof); + $em->flush(); + + $this->assertTrue($attendee->hasAnnouncementEmailTypeSentSince($summit, $type, $before, $ticket)); + // the attendee-level (no ticket) check must not match a ticket-scoped proof + $this->assertFalse($attendee->hasAnnouncementEmailTypeSentSince($summit, $type, $before)); + } +} From 5d72132f54547a7ad351aaacdde5fb5881865697 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:29:24 -0300 Subject: [PATCH 03/19] feat(attendees): wire the sent-proof resume check into every email strategy AbstractEmailAction and its four concrete strategies (Generic, AllCurrentTickets, RegistrationIncompleteReminder, Ticket) now share a resume-check/record pattern backed by SummitAttendeeAnnouncementEmail: before dispatching, skip a recipient already reached by this run (resume_since set and a matching proof exists); after dispatching, record the proof. SummitAttendeeTicketEmailStrategy is the one shape speakers don't have - up to one email per ticket, not one per attendee - so the check/record happens per ticket, keyed on the flow_event requested at the top of the loop rather than the value the complete-branch transiently mutates mid-loop. AttendeeService::send's processCurrentId closure declared 8 params while ParametrizedSendEmails invokes it with 9, silently dropping the info callback; now declares and forwards all 9, plus resume_since read from the payload. This task alone changes no observable behavior - resume_since is only ever set once the chunk job (Task 4) exists to set it. --- app/Services/Model/AttendeeService.php | 8 +- .../EmailActions/AbstractEmailAction.php | 61 ++++- .../EmailActionsStrategyFactory.php | 12 +- .../IEmailActionsStrategyFactory.php | 4 +- ...AttendeeAllCurrentTicketsEmailStrategy.php | 30 ++- .../SummitAttendeeGenericEmailStrategy.php | 32 ++- ...RegistrationIncompleteReminderStrategy.php | 30 ++- .../SummitAttendeeTicketEmailStrategy.php | 39 ++- tests/AttendeeServiceResumeSendEmailsTest.php | 222 ++++++++++++++++++ 9 files changed, 410 insertions(+), 28 deletions(-) create mode 100644 tests/AttendeeServiceResumeSendEmailsTest.php diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 445c8121f..cdc53cccd 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -639,8 +639,10 @@ function $announcement_email_config, $filter, $onDispatchSuccess, + $onDispatchInfo, $onDispatchError) use ($payload) { try { + $resume_since = $payload['resume_since'] ?? null; $this->tx_service->transaction(function () use ( $summit, $flow_event, @@ -648,7 +650,9 @@ function $test_email_recipient, $filter, $onDispatchSuccess, + $onDispatchInfo, $onDispatchError, + $resume_since, $payload ) { Log::debug(sprintf("AttendeeService::send processing attendee id %s", $attendee_id)); @@ -658,9 +662,9 @@ function return; $emailActionsStrategyFactory = new EmailActionsStrategyFactory(); - $strategy = $emailActionsStrategyFactory->build($flow_event); + $strategy = $emailActionsStrategyFactory->build($summit, $flow_event); if ($strategy != null) { - $strategy->process($attendee, $test_email_recipient, $onDispatchSuccess, $onDispatchError); + $strategy->process($attendee, $test_email_recipient, $onDispatchSuccess, $onDispatchInfo, $onDispatchError, $resume_since); } }); } catch (\Exception $ex) { diff --git a/app/Services/Model/Strategies/EmailActions/AbstractEmailAction.php b/app/Services/Model/Strategies/EmailActions/AbstractEmailAction.php index afaa9926e..c269e9f56 100644 --- a/app/Services/Model/Strategies/EmailActions/AbstractEmailAction.php +++ b/app/Services/Model/Strategies/EmailActions/AbstractEmailAction.php @@ -13,7 +13,11 @@ * limitations under the License. **/ +use models\summit\Summit; use models\summit\SummitAttendee; +use models\summit\SummitAttendeeAnnouncementEmail; +use models\summit\SummitAttendeeTicket; +use DateTime; /** * Class AbstractEmailAction @@ -21,6 +25,11 @@ */ abstract class AbstractEmailAction { + /** + * @var Summit + */ + protected $summit; + /** * @var String */ @@ -28,10 +37,12 @@ abstract class AbstractEmailAction /** * AbstractEmailAction constructor. + * @param Summit $summit * @param String $flow_event */ - public function __construct(String $flow_event) + public function __construct(Summit $summit, String $flow_event) { + $this->summit = $summit; $this->flow_event = $flow_event; } @@ -39,7 +50,9 @@ public function __construct(String $flow_event) * @param SummitAttendee $attendee * @param string|null $test_email_recipient * @param callable|null $onSuccess + * @param callable|null $onInfo * @param callable|null $onError + * @param int|null $resume_since * @return mixed */ public abstract function process @@ -47,6 +60,48 @@ public abstract function process SummitAttendee $attendee, ?string $test_email_recipient = null, callable $onSuccess = null, - callable $onError = null + callable $onInfo = null, + callable $onError = null, + ?int $resume_since = null ); -} \ No newline at end of file + + /** + * Resume-check backing every concrete strategy's skip decision: on a retry (resume_since + * set), an (attendee[, ticket]) that already has a proof for this flow event written at or + * after resume_since was already reached by this run before the kill - skip before any side + * effect. First attempt (resume_since null) always returns false, so this task alone changes + * no observable behavior until the chunk job that sets resume_since exists. + * + * @param SummitAttendee $attendee + * @param int|null $resume_since + * @param SummitAttendeeTicket|null $ticket + * @param string|null $type defaults to $this->flow_event - pass explicitly when the caller's + * own flow_event has been transiently mutated (SummitAttendeeTicketEmailStrategy) + * @return bool + */ + protected function alreadySentSince(SummitAttendee $attendee, ?int $resume_since, ?SummitAttendeeTicket $ticket = null, ?string $type = null): bool + { + if (is_null($resume_since)) return false; + return $attendee->hasAnnouncementEmailTypeSentSince($this->summit, $type ?? $this->flow_event, new DateTime('@' . $resume_since), $ticket); + } + + /** + * Records the sent-proof for an email this strategy actually dispatched. Call after the + * dispatch, never before - a proof only means "sent", not "about to send". + * + * @param SummitAttendee $attendee + * @param SummitAttendeeTicket|null $ticket + * @param string|null $type defaults to $this->flow_event - pass explicitly when the caller's + * own flow_event has been transiently mutated (SummitAttendeeTicketEmailStrategy) + * @return void + */ + protected function recordSent(SummitAttendee $attendee, ?SummitAttendeeTicket $ticket = null, ?string $type = null): void + { + $proof = new SummitAttendeeAnnouncementEmail(); + $proof->setType($type ?? $this->flow_event); + $proof->setSummit($this->summit); + $attendee->addAnnouncementEmail($proof); + $proof->setTicket($ticket); + $proof->markAsSent(); + } +} diff --git a/app/Services/Model/Strategies/EmailActions/EmailActionsStrategyFactory.php b/app/Services/Model/Strategies/EmailActions/EmailActionsStrategyFactory.php index bbc37db7e..a3f77ddc8 100644 --- a/app/Services/Model/Strategies/EmailActions/EmailActionsStrategyFactory.php +++ b/app/Services/Model/Strategies/EmailActions/EmailActionsStrategyFactory.php @@ -17,6 +17,7 @@ use App\Jobs\Emails\SummitAttendeeAllTicketsEditionEmail; use App\Jobs\Emails\SummitAttendeeRegistrationIncompleteReminderEmail; use App\Jobs\Emails\SummitAttendeeTicketRegenerateHashEmail; +use models\summit\Summit; /** * Class IEmailActionsStrategyFactory @@ -25,20 +26,21 @@ final class EmailActionsStrategyFactory implements IEmailActionsStrategyFactory { /** + * @param Summit $summit * @param String $flow_event * @return AbstractEmailAction|null */ - public function build(String $flow_event): ?AbstractEmailAction { + public function build(Summit $summit, String $flow_event): ?AbstractEmailAction { switch ($flow_event) { case SummitAttendeeTicketRegenerateHashEmail::EVENT_SLUG: case InviteAttendeeTicketEditionMail::EVENT_SLUG: - return new SummitAttendeeTicketEmailStrategy($flow_event); + return new SummitAttendeeTicketEmailStrategy($summit, $flow_event); case SummitAttendeeAllTicketsEditionEmail::EVENT_SLUG: - return new SummitAttendeeAllCurrentTicketsEmailStrategy($flow_event); + return new SummitAttendeeAllCurrentTicketsEmailStrategy($summit, $flow_event); case SummitAttendeeRegistrationIncompleteReminderEmail::EVENT_SLUG: - return new SummitAttendeeRegistrationIncompleteReminderStrategy($flow_event); + return new SummitAttendeeRegistrationIncompleteReminderStrategy($summit, $flow_event); case GenericSummitAttendeeEmail::EVENT_SLUG: - return new SummitAttendeeGenericEmailStrategy($flow_event); + return new SummitAttendeeGenericEmailStrategy($summit, $flow_event); default: return null; } diff --git a/app/Services/Model/Strategies/EmailActions/IEmailActionsStrategyFactory.php b/app/Services/Model/Strategies/EmailActions/IEmailActionsStrategyFactory.php index f2a553912..df9f0e9c8 100644 --- a/app/Services/Model/Strategies/EmailActions/IEmailActionsStrategyFactory.php +++ b/app/Services/Model/Strategies/EmailActions/IEmailActionsStrategyFactory.php @@ -13,6 +13,7 @@ * limitations under the License. **/ +use models\summit\Summit; /** * Interface IEmailActionsStrategyFactory @@ -21,8 +22,9 @@ interface IEmailActionsStrategyFactory { /** + * @param Summit $summit * @param String $flow_event * @return AbstractEmailAction|null */ - public function build(String $flow_event): ?AbstractEmailAction; + public function build(Summit $summit, String $flow_event): ?AbstractEmailAction; } \ No newline at end of file diff --git a/app/Services/Model/Strategies/EmailActions/SummitAttendeeAllCurrentTicketsEmailStrategy.php b/app/Services/Model/Strategies/EmailActions/SummitAttendeeAllCurrentTicketsEmailStrategy.php index 66757bcbd..c87b551e4 100644 --- a/app/Services/Model/Strategies/EmailActions/SummitAttendeeAllCurrentTicketsEmailStrategy.php +++ b/app/Services/Model/Strategies/EmailActions/SummitAttendeeAllCurrentTicketsEmailStrategy.php @@ -15,6 +15,7 @@ use App\Jobs\Emails\SummitAttendeeAllTicketsEditionEmail; use App\Services\utils\IEmailExcerptService; use Illuminate\Support\Facades\Log; +use models\summit\Summit; use models\summit\SummitAttendee; /** @@ -25,18 +26,21 @@ final class SummitAttendeeAllCurrentTicketsEmailStrategy extends AbstractEmailAc { /** * SummitAttendeeAllCurrentTicketsEmailStrategy constructor. + * @param Summit $summit * @param String $flow_event */ - public function __construct(String $flow_event) + public function __construct(Summit $summit, string $flow_event) { - parent::__construct($flow_event); + parent::__construct($summit, $flow_event); } /** * @param SummitAttendee $attendee * @param string|null $test_email_recipient * @param callable|null $onSuccess + * @param callable|null $onInfo * @param callable|null $onError + * @param int|null $resume_since * @return void */ public function process @@ -44,9 +48,26 @@ public function process SummitAttendee $attendee, ?string $test_email_recipient = null, callable $onSuccess = null, - callable $onError = null + callable $onInfo = null, + callable $onError = null, + ?int $resume_since = null ) { + if ($this->alreadySentSince($attendee, $resume_since)) { + if (!is_null($onInfo)) { + $onInfo + ( + sprintf + ( + "Attendee %s (%s) already processed by this run before the retry, skipped.", + $attendee->getEmail(), + $attendee->getId() + ) + ); + } + return; + } + Log::debug ( sprintf @@ -57,9 +78,10 @@ public function process ) ); SummitAttendeeAllTicketsEditionEmail::dispatch($attendee, $test_email_recipient); + $this->recordSent($attendee); if (!is_null($onSuccess)) { $onSuccess($attendee->getEmail(), IEmailExcerptService::EmailLineType, $this->flow_event); } } -} \ No newline at end of file +} diff --git a/app/Services/Model/Strategies/EmailActions/SummitAttendeeGenericEmailStrategy.php b/app/Services/Model/Strategies/EmailActions/SummitAttendeeGenericEmailStrategy.php index 1b2bcc217..8a3eb2fe6 100644 --- a/app/Services/Model/Strategies/EmailActions/SummitAttendeeGenericEmailStrategy.php +++ b/app/Services/Model/Strategies/EmailActions/SummitAttendeeGenericEmailStrategy.php @@ -15,6 +15,7 @@ use App\Jobs\Emails\Registration\Attendees\GenericSummitAttendeeEmail; use App\Services\utils\IEmailExcerptService; use Illuminate\Support\Facades\Log; +use models\summit\Summit; use models\summit\SummitAttendee; /** @@ -24,19 +25,22 @@ final class SummitAttendeeGenericEmailStrategy extends AbstractEmailAction { /** - * SummitAttendeeTicketEmailStrategy constructor. + * SummitAttendeeGenericEmailStrategy constructor. + * @param Summit $summit * @param String $flow_event */ - public function __construct(string $flow_event) + public function __construct(Summit $summit, string $flow_event) { - parent::__construct($flow_event); + parent::__construct($summit, $flow_event); } /** * @param SummitAttendee $attendee * @param string|null $test_email_recipient * @param callable|null $onSuccess + * @param callable|null $onInfo * @param callable|null $onError + * @param int|null $resume_since * @return void */ public function process @@ -44,9 +48,26 @@ public function process SummitAttendee $attendee, ?string $test_email_recipient = null, callable $onSuccess = null, - callable $onError = null + callable $onInfo = null, + callable $onError = null, + ?int $resume_since = null ) { + if ($this->alreadySentSince($attendee, $resume_since)) { + if (!is_null($onInfo)) { + $onInfo + ( + sprintf + ( + "Attendee %s (%s) already processed by this run before the retry, skipped.", + $attendee->getEmail(), + $attendee->getId() + ) + ); + } + return; + } + Log::debug ( sprintf @@ -58,9 +79,10 @@ public function process ); GenericSummitAttendeeEmail::dispatch($attendee, $test_email_recipient); + $this->recordSent($attendee); if (!is_null($onSuccess)) { $onSuccess($attendee->getEmail(), IEmailExcerptService::EmailLineType, $this->flow_event); } } -} \ No newline at end of file +} diff --git a/app/Services/Model/Strategies/EmailActions/SummitAttendeeRegistrationIncompleteReminderStrategy.php b/app/Services/Model/Strategies/EmailActions/SummitAttendeeRegistrationIncompleteReminderStrategy.php index 70c5eb978..5f3b45b75 100644 --- a/app/Services/Model/Strategies/EmailActions/SummitAttendeeRegistrationIncompleteReminderStrategy.php +++ b/app/Services/Model/Strategies/EmailActions/SummitAttendeeRegistrationIncompleteReminderStrategy.php @@ -15,6 +15,7 @@ use App\Jobs\Emails\SummitAttendeeRegistrationIncompleteReminderEmail; use App\Services\utils\IEmailExcerptService; use Illuminate\Support\Facades\Log; +use models\summit\Summit; use models\summit\SummitAttendee; /** @@ -25,18 +26,21 @@ final class SummitAttendeeRegistrationIncompleteReminderStrategy extends Abstrac { /** * SummitAttendeeRegistrationIncompleteReminderStrategy constructor. + * @param Summit $summit * @param String $flow_event */ - public function __construct(string $flow_event) + public function __construct(Summit $summit, string $flow_event) { - parent::__construct($flow_event); + parent::__construct($summit, $flow_event); } /** * @param SummitAttendee $attendee * @param string|null $test_email_recipient * @param callable|null $onSuccess + * @param callable|null $onInfo * @param callable|null $onError + * @param int|null $resume_since * @return void */ public function process @@ -44,10 +48,27 @@ public function process SummitAttendee $attendee, ?string $test_email_recipient = null, callable $onSuccess = null, - callable $onError = null + callable $onInfo = null, + callable $onError = null, + ?int $resume_since = null ) { if (!$attendee->isComplete()) { + if ($this->alreadySentSince($attendee, $resume_since)) { + if (!is_null($onInfo)) { + $onInfo + ( + sprintf + ( + "Attendee %s (%s) already processed by this run before the retry, skipped.", + $attendee->getEmail(), + $attendee->getId() + ) + ); + } + return; + } + Log::debug ( sprintf @@ -58,6 +79,7 @@ public function process ) ); SummitAttendeeRegistrationIncompleteReminderEmail::dispatch($attendee, $test_email_recipient); + $this->recordSent($attendee); if (!is_null($onSuccess)) { $onSuccess($attendee->getEmail(), IEmailExcerptService::EmailLineType, $this->flow_event); @@ -74,4 +96,4 @@ public function process ) ); } -} \ No newline at end of file +} diff --git a/app/Services/Model/Strategies/EmailActions/SummitAttendeeTicketEmailStrategy.php b/app/Services/Model/Strategies/EmailActions/SummitAttendeeTicketEmailStrategy.php index 4dd3df4f6..d0faeb8b9 100644 --- a/app/Services/Model/Strategies/EmailActions/SummitAttendeeTicketEmailStrategy.php +++ b/app/Services/Model/Strategies/EmailActions/SummitAttendeeTicketEmailStrategy.php @@ -16,6 +16,7 @@ use App\Jobs\Emails\SummitAttendeeTicketRegenerateHashEmail; use App\Services\utils\IEmailExcerptService; use Illuminate\Support\Facades\Log; +use models\summit\Summit; use models\summit\SummitAttendee; /** @@ -26,18 +27,21 @@ class SummitAttendeeTicketEmailStrategy extends AbstractEmailAction { /** * SummitAttendeeTicketEmailStrategy constructor. + * @param Summit $summit * @param String $flow_event */ - public function __construct(String $flow_event) + public function __construct(Summit $summit, String $flow_event) { - parent::__construct($flow_event); + parent::__construct($summit, $flow_event); } /** * @param SummitAttendee $attendee * @param string|null $test_email_recipient * @param callable|null $onSuccess + * @param callable|null $onInfo * @param callable|null $onError + * @param int|null $resume_since * @return void */ public function process @@ -45,13 +49,39 @@ public function process SummitAttendee $attendee, ?string $test_email_recipient = null, callable $onSuccess = null, - callable $onError = null + callable $onInfo = null, + callable $onError = null, + ?int $resume_since = null ) { + // Captured once, before the loop, and used for every ticket's resume-check and proof - + // never $this->flow_event read mid-loop, which the complete-branch below transiently + // mutates to a different value. Sending resumes/records against the flow event the + // caller actually requested, not whichever mail class ended up dispatched for a given + // ticket. + $requested_flow_event = $this->flow_event; + foreach ($attendee->getTickets() as $ticket) { try { if(!$ticket->isActive()) continue; if(!$ticket->isPaid()) continue; + + if ($this->alreadySentSince($attendee, $resume_since, $ticket, $requested_flow_event)) { + if (!is_null($onInfo)) { + $onInfo + ( + sprintf + ( + "Attendee %s (%s) ticket %s already processed by this run before the retry, skipped.", + $attendee->getEmail(), + $attendee->getId(), + $ticket->getId() + ) + ); + } + continue; + } + $is_complete = $attendee->isComplete(); $original_flow_event = $this->flow_event; Log::debug @@ -87,6 +117,7 @@ public function process $attendee->sendInvitationEmail($ticket, true, [], $test_email_recipient); } $this->flow_event = $original_flow_event; + $this->recordSent($attendee, $ticket, $requested_flow_event); if (!is_null($onSuccess)) { $onSuccess($attendee->getEmail(), IEmailExcerptService::EmailLineType, $this->flow_event); @@ -100,4 +131,4 @@ public function process } } } -} \ No newline at end of file +} diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php new file mode 100644 index 000000000..b0177d33f --- /dev/null +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -0,0 +1,222 @@ +em(); + $attendee = $em->getRepository(\models\summit\SummitAttendee::class)->find($attendee_id); + $summit = $em->getRepository(\models\summit\Summit::class)->find(self::$summit->getId()); + + $proof = new SummitAttendeeAnnouncementEmail(); + $proof->setType($type); + $proof->setSummit($summit); + if (!is_null($ticket_id)) { + $ticket = $em->getRepository(\models\summit\SummitAttendeeTicket::class)->find($ticket_id); + $proof->setTicket($ticket); + } + $attendee->addAnnouncementEmail($proof); + + if (is_null($backdateTo)) { + $proof->markAsSent(); + } else { + $prop = new ReflectionProperty(SummitAttendeeAnnouncementEmail::class, 'send_date'); + $prop->setAccessible(true); + $prop->setValue($proof, $backdateTo); + } + + $em->persist($proof); + $em->flush(); + } + + private function proofCount(int $attendee_id, string $type, ?int $ticket_id = null): int + { + $em = $this->em(); + $qb = $em->createQueryBuilder() + ->select('COUNT(p.id)') + ->from(SummitAttendeeAnnouncementEmail::class, 'p') + ->where('p.attendee = :attendee_id') + ->andWhere('p.type = :type') + ->setParameter('attendee_id', $attendee_id) + ->setParameter('type', $type); + if (is_null($ticket_id)) { + $qb->andWhere('p.ticket IS NULL'); + } else { + $qb->andWhere('p.ticket = :ticket_id')->setParameter('ticket_id', $ticket_id); + } + return (int) $qb->getQuery()->getSingleScalarResult(); + } + + public function testFirstAttemptWithNoResumeSinceDispatchesAndRecordsExactlyOneProof(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => [$attendee_id], + ]); + + Queue::assertPushed(GenericSummitAttendeeEmail::class, 1); + $this->assertSame(1, $this->proofCount($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG)); + } + + public function testResumedRunSkipsAttendeeWithProofSinceDispatchAndDoesNotDuplicateProof(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + + $dispatchedAt = time() - 600; + + // proof written "now" (after dispatch) - this run already reached this attendee + $this->givenAttendeeHasProof($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG, null); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => [$attendee_id], + 'dispatched_at' => $dispatchedAt, + 'resume_since' => $dispatchedAt, + ]); + + Queue::assertNotPushed(GenericSummitAttendeeEmail::class); + // still exactly one proof (the pre-existing one) - the resume check did not duplicate it + $this->assertSame(1, $this->proofCount($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG)); + } + + public function testResumedRunStillProcessesAttendeeWithProofBeforeDispatch(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + + $dispatchedAt = time() - 600; + + // proof from 30 days ago - an earlier, unrelated campaign, not this run + $this->givenAttendeeHasProof($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG, new \DateTime('-30 days', new \DateTimeZone('UTC'))); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => [$attendee_id], + 'dispatched_at' => $dispatchedAt, + 'resume_since' => $dispatchedAt, + ]); + + Queue::assertPushed(GenericSummitAttendeeEmail::class, 1); + // the old proof plus the new one from this run + $this->assertSame(2, $this->proofCount($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG)); + } + + public function testResumedRunSkipsOnlyTicketWithProofSinceDispatchNotOthers(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + $tickets = $attendee->getTickets(); + $this->assertGreaterThanOrEqual(2, $tickets->count(), 'fixture must seed at least 2 tickets for this attendee'); + $ticket_ids = []; + foreach ($tickets as $t) { + $ticket_ids[] = $t->getId(); + } + $first_ticket_id = $ticket_ids[0]; + + $type = SummitAttendeeTicketRegenerateHashEmail::EVENT_SLUG; + $dispatchedAt = time() - 600; + + // only the first ticket already has a proof since dispatch + $this->givenAttendeeHasProof($attendee_id, $type, null, $first_ticket_id); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => $type, + 'attendees_ids' => [$attendee_id], + 'dispatched_at' => $dispatchedAt, + 'resume_since' => $dispatchedAt, + ]); + + // the first ticket's proof is untouched (still exactly one - not duplicated) + $this->assertSame(1, $this->proofCount($attendee_id, $type, $first_ticket_id)); + // every other ticket in the fixture must have been processed (one proof each) - the + // resume check is per-ticket, not per-attendee, so it must not skip the whole attendee + for ($i = 1; $i < count($ticket_ids); $i++) { + $this->assertSame( + 1, + $this->proofCount($attendee_id, $type, $ticket_ids[$i]), + sprintf('ticket %s (not the resume-skipped one) must still have been processed', $ticket_ids[$i]) + ); + } + } +} From 809c4d240fd5f4ddde3d2299ad5f8cf27988a241 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:48:07 -0300 Subject: [PATCH 04/19] feat(attendees): chunk the bulk email send instead of one unbounded job AttendeeService::triggerSend replaced the single unbounded ProcessAttendeesEmailRequestJob::dispatch(...) with the id-list chunk-loop pattern SpeakerService::triggerSendEmails already uses: resolve the full matched id set (explicit attendees_ids or a paginated filter query), dedup, drop excluded ids, then dispatch one job per attendees_process_job_chunk_size-sized group via JobDispatcher::withDbFallback (primary connection, database fallback, sync as a last resort - one chunk failing every tier does not block its siblings). ProcessAttendeesEmailRequestJob gains the ResumableChunkJob trait (tries=2, timeout=1200s, strictly below every queue retry_after) and calls activateResumeIfRetrying() so a retry resumes via Task 3's resume-skip rather than re-emailing everyone. IAttendeeEmailFilterFields centralizes the FilterParser operator allow-list shared by the controller, triggerSend, and the job's own retry-path parse - previously duplicated inline, about to be duplicated a third time. Carries only OPERATORS, not a VALIDATION_RULES constant like ISpeakerFilterFields: three of this endpoint's fields validate via "new Boolean()" rule instances, and PHP does not allow "new" inside a class constant value. attendees_process_job_chunk_size defaults to 200, matching the speaker precedent, rather than the originally-planned 2000 - the larger value was never validated against real per-attendee timing. --- .../OAuth2SummitAttendeesApiController.php | 38 +-- .../ProcessAttendeesEmailRequestJob.php | 54 +--- app/Services/Model/AttendeeService.php | 110 +++++++- .../Model/IAttendeeEmailFilterFields.php | 77 ++++++ config/emails.php | 6 +- tests/AttendeeServiceBulkSendChunkingTest.php | 242 ++++++++++++++++++ 6 files changed, 448 insertions(+), 79 deletions(-) create mode 100644 app/Services/Model/IAttendeeEmailFilterFields.php create mode 100644 tests/AttendeeServiceBulkSendChunkingTest.php diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php index 7ca7ba688..c0c2a5c12 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitAttendeesApiController.php @@ -48,6 +48,7 @@ use utils\Filter; use utils\FilterElement; use Illuminate\Support\Facades\Validator; +use services\model\IAttendeeEmailFilterFields; use utils\FilterParser; /** @@ -1485,42 +1486,7 @@ public function send($summit_id) $filter = null; if (Request::has('filter')) { - $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '=='], - 'last_name' => ['=@', '=='], - 'full_name' => ['=@', '=='], - 'company' => ['=@', '=='], - 'has_company' => ['=='], - 'email' => ['=@', '=='], - 'external_order_id' => ['=@', '=='], - 'external_attendee_id' => ['=@', '=='], - 'member_id' => ['==', '>'], - 'ticket_type' => ['=@', '==', '@@'], - 'ticket_type_id' => ['=='], - 'badge_type' => ['=@', '==', '@@'], - 'badge_type_id' => ['=='], - 'features' => ['=@', '==', '@@'], - 'features_id' => ['=='], - 'access_levels' => ['=@', '==', '@@'], - 'access_levels_id' => ['=='], - 'status' => ['=@', '=='], - 'has_member' => ['=='], - 'has_tickets' => ['=='], - 'has_virtual_checkin' => ['=='], - 'has_checkin' => ['=='], - 'tickets_count' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_date' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_count' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_track_group_id' => ['=='], - 'summit_hall_checked_in_date' => ['==', '>=', '<=', '>', '<','[]'], - 'tags' => ['=@', '==', '@@'], - 'tags_id' => ['=='], - 'notes' => ['=@', '@@'], - 'has_notes' => ['=='], - 'has_manager' => ['=='], - ]); + $filter = FilterParser::parse(Request::input('filter'), IAttendeeEmailFilterFields::OPERATORS); } if (is_null($filter)) diff --git a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php index 018fd95d2..779e1cfcd 100644 --- a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php +++ b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\Traits\ResumableChunkJob; use App\Services\Model\IAttendeeService; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -19,6 +20,7 @@ use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\Log; use models\summit\Summit; +use services\model\IAttendeeEmailFilterFields; use utils\FilterParser; /** * Class ProcessAttendeesEmailRequestJob @@ -26,11 +28,10 @@ */ final class ProcessAttendeesEmailRequestJob implements ShouldQueue { - public $timeout = 0; - - public $tries = 1; - - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + // $timeout/$tries/$backoff and the resume-on-retry mechanics come from ResumableChunkJob - + // see that trait's doc comment for why timeout must stay below every retry_after / worker + // --timeout this job can run under. + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, ResumableChunkJob; private $summit_id; @@ -63,42 +64,13 @@ public function handle(IAttendeeService $service){ ) ); - $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '=='], - 'last_name' => ['=@', '=='], - 'full_name' => ['=@', '=='], - 'company' => ['=@', '=='], - 'has_company' => ['=='], - 'email' => ['=@', '=='], - 'external_order_id' => ['=@', '=='], - 'external_attendee_id' => ['=@', '=='], - 'member_id' => ['==', '>'], - 'ticket_type' => ['=@', '==', '@@'], - 'ticket_type_id' => ['=='], - 'badge_type' => ['=@', '==', '@@'], - 'badge_type_id' => ['=='], - 'features' => ['=@', '==', '@@'], - 'features_id' => ['=='], - 'access_levels' => ['=@', '==', '@@'], - 'access_levels_id' => ['=='], - 'status' => ['=@', '=='], - 'has_member' => ['=='], - 'has_tickets' => ['=='], - 'has_virtual_checkin' => ['=='], - 'has_checkin' => ['=='], - 'tickets_count' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_date' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_count' => ['==', '>=', '<=', '>', '<'], - 'presentation_votes_track_group_id' => ['=='], - 'summit_hall_checked_in_date' => ['==', '>=', '<=', '>', '<','[]'], - 'tags' => ['=@', '==', '@@'], - 'tags_id' => ['=='], - 'notes' => ['=@', '@@'], - 'has_notes' => ['=='], - 'has_manager' => ['=='] - ]) : null; + // ResumableChunkJob::activateResumeIfRetrying(): resume, not resend. On a retry it sets + // resume_since = dispatched_at in $this->payload so AttendeeService::send skips only the + // attendees (or, for the ticket flow event, attendee+ticket pairs) whose proof for this + // run was written by THIS run. + $this->activateResumeIfRetrying(); + + $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, IAttendeeEmailFilterFields::OPERATORS) : null; $service->send($this->summit_id, $this->payload, $filter); } diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index cdc53cccd..2521cddb2 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -14,12 +14,14 @@ use App\Jobs\Emails\ProcessAttendeesEmailRequestJob; use App\Jobs\Emails\Registration\Attendees\SummitAttendeeExcerptEmail; +use App\Jobs\Utils\JobDispatcher; use App\Models\Foundation\Summit\Repositories\ISummitAttendeeBadgeRepository; use App\Services\Apis\ExternalRegistrationFeeds\IExternalRegistrationFeedFactory; use App\Services\Model\Imp\Traits\ParametrizedSendEmails; use App\Services\Model\Strategies\EmailActions\EmailActionsStrategyFactory; use App\Utils\AES; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -42,8 +44,10 @@ use models\summit\SummitAttendeeNote; use models\summit\SummitAttendeeTicket; use services\apis\IEventbriteAPI; +use services\model\IAttendeeEmailFilterFields; use utils\Filter; use utils\FilterElement; +use utils\FilterParser; use utils\PagingInfo; /** @@ -605,7 +609,111 @@ public function regenerateAttendeeBadgesQRCodes(SummitAttendee $attendee): void */ public function triggerSend(Summit $summit, array $payload, $filter = null): void { - ProcessAttendeesEmailRequestJob::dispatch($summit, $payload, $filter); + $process_db_chunk_size = intval(Config::get('emails.attendees_process_db_chunk_size', 2000)); + $process_job_chunk_size = intval(Config::get('emails.attendees_process_job_chunk_size', 2000)); + + Log::debug + ( + sprintf + ( + "AttendeeService::triggerSend summit %s email_flow_event %s attendees_ids_count %s has_filter %s process_db_chunk_size %s process_job_chunk_size %s", + $summit->getId(), + $payload['email_flow_event'] ?? '', + isset($payload['attendees_ids']) ? count($payload['attendees_ids']) : 0, + !is_null($filter) ? 'yes' : 'no', + $process_db_chunk_size, + $process_job_chunk_size + ) + ); + + if (isset($payload['attendees_ids'])) { + $ids = $payload['attendees_ids']; + } else { + $parsedFilter = !is_null($filter) ? FilterParser::parse($filter, IAttendeeEmailFilterFields::OPERATORS) : new Filter(); + if (!$parsedFilter->hasFilter("summit_id")) + $parsedFilter->addFilterCondition(FilterElement::makeEqual('summit_id', $summit->getId())); + $ids = []; + $page = 1; + do { + $currentPage = $this->tx_service->transaction(function () use ($page, $parsedFilter, $process_db_chunk_size) { + return $this->attendee_repository->getAllIdsByPage(new PagingInfo($page, $process_db_chunk_size), $parsedFilter); + }); + $ids = array_merge($ids, $currentPage); + $page++; + } while (count($currentPage) > 0); + } + + if (isset($payload['excluded_attendees_ids'])) { + $ids = array_diff($ids, $payload['excluded_attendees_ids']); + } + + $ids = array_values(array_unique($ids)); + + if (empty($ids)) { + Log::debug(sprintf("AttendeeService::triggerSend summit %s no attendees matched, nothing dispatched", $summit->getId())); + return; + } + + Log::debug(sprintf("AttendeeService::triggerSend got %s attendees to process", count($ids))); + + // JobDispatcher, not ::dispatch(): a queue-backend failure part-way through this loop + // would otherwise abort the request with some chunks already queued and the rest lost, + // and an operator retry would re-email the chunks that already went out. withDbFallback() + // fails over to the database queue (and runs sync on a double failure) so the loop + // completes. Same pattern as SpeakerService::triggerSendEmails. + // Stamped once per run (not per chunk) so every chunk's resume check, if the chunk is + // ever retried, compares against the same instant this run started. + $dispatched_at = time(); + $chunk_nbr = 1; + foreach (array_chunk($ids, $process_job_chunk_size) as $chunk) { + $chunkPayload = $payload; + $chunkPayload['attendees_ids'] = $chunk; + $chunkPayload['dispatched_at'] = $dispatched_at; + unset($chunkPayload['excluded_attendees_ids']); + // resume_since is set only by ProcessAttendeesEmailRequestJob::handle() on a retry - + // a caller-supplied value must never reach a first-attempt chunk. + unset($chunkPayload['resume_since']); + + Log::debug + ( + sprintf + ( + "AttendeeService::triggerSend sending summit id %s chunk %s attendees count %s", + $summit->getId(), + $chunk_nbr, + count($chunk) + ) + ); + + try { + JobDispatcher::withDbFallback( + job: new ProcessAttendeesEmailRequestJob($summit, $chunkPayload, $filter), + logContext: ['summit_id' => $summit->getId(), 'attendee_count' => count($chunk)], + primaryConnection: Config::get('queue.default') + ); + $chunk_nbr++; + } + catch (\Throwable $ex){ + // withDbFallback already exhausted the primary connection, the database + // fallback, and a synchronous run - reaching here means all three failed for + // this chunk. Log at error (not warning) so it surfaces to alerting; keep the + // loop going so a bad chunk doesn't also block every sibling chunk that would + // otherwise succeed. + Log::error + ( + sprintf + ( + "AttendeeService::triggerSend summit %s: chunk of %s attendee(s) failed every dispatch tier (%s: %s). Unprocessed attendee ids: [%s]", + $summit->getId(), + count($chunk), + get_class($ex), + $ex->getMessage(), + implode(', ', $chunk) + ), + ['summit_id' => $summit->getId(), 'attendee_ids' => $chunk, 'exception' => $ex] + ); + } + } } use ParametrizedSendEmails; diff --git a/app/Services/Model/IAttendeeEmailFilterFields.php b/app/Services/Model/IAttendeeEmailFilterFields.php new file mode 100644 index 000000000..5882a368f --- /dev/null +++ b/app/Services/Model/IAttendeeEmailFilterFields.php @@ -0,0 +1,77 @@ +validate() call - so there is no duplication to remove by relocating it; it stays + * inline in OAuth2SummitAttendeesApiController::send. + * + * Scoped to the email-send endpoint only - the general attendee listing endpoint + * (DoctrineSummitAttendeeRepository::getFilterMappings) supports a richer field set this + * interface does not attempt to unify with. + * + * @package services\model + */ +interface IAttendeeEmailFilterFields +{ + const OPERATORS = [ + 'id' => ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '=='], + 'last_name' => ['=@', '=='], + 'full_name' => ['=@', '=='], + 'company' => ['=@', '=='], + 'has_company' => ['=='], + 'email' => ['=@', '=='], + 'external_order_id' => ['=@', '=='], + 'external_attendee_id' => ['=@', '=='], + 'member_id' => ['==', '>'], + 'ticket_type' => ['=@', '==', '@@'], + 'ticket_type_id' => ['=='], + 'badge_type' => ['=@', '==', '@@'], + 'badge_type_id' => ['=='], + 'features' => ['=@', '==', '@@'], + 'features_id' => ['=='], + 'access_levels' => ['=@', '==', '@@'], + 'access_levels_id' => ['=='], + 'status' => ['=@', '=='], + 'has_member' => ['=='], + 'has_tickets' => ['=='], + 'has_virtual_checkin' => ['=='], + 'has_checkin' => ['=='], + 'tickets_count' => ['==', '>=', '<=', '>', '<'], + 'presentation_votes_date' => ['==', '>=', '<=', '>', '<'], + 'presentation_votes_count' => ['==', '>=', '<=', '>', '<'], + 'presentation_votes_track_group_id' => ['=='], + 'summit_hall_checked_in_date' => ['==', '>=', '<=', '>', '<', '[]'], + 'tags' => ['=@', '==', '@@'], + 'tags_id' => ['=='], + 'notes' => ['=@', '@@'], + 'has_notes' => ['=='], + 'has_manager' => ['=='], + ]; +} diff --git a/config/emails.php b/config/emails.php index 5db39bb72..6532bd954 100644 --- a/config/emails.php +++ b/config/emails.php @@ -16,5 +16,9 @@ // size of the chunk of db processing for speakers email 'speakers_process_db_chunk_size' => env('EMAILS_SPEAKERS_PROCESS_DB_CHUNK', 500), // size of the chunk of job processing for speakers email - 'speakers_process_job_chunk_size' => env('EMAILS_SPEAKERS_PROCESS_JOB_CHUNK', 200) + 'speakers_process_job_chunk_size' => env('EMAILS_SPEAKERS_PROCESS_JOB_CHUNK', 200), + // size of the chunk of db processing for attendees email + 'attendees_process_db_chunk_size' => env('EMAILS_ATTENDEES_PROCESS_DB_CHUNK', 2000), + // size of the chunk of job processing for attendees email + 'attendees_process_job_chunk_size' => env('EMAILS_ATTENDEES_PROCESS_JOB_CHUNK', 200) ]; diff --git a/tests/AttendeeServiceBulkSendChunkingTest.php b/tests/AttendeeServiceBulkSendChunkingTest.php new file mode 100644 index 000000000..2bf19c29e --- /dev/null +++ b/tests/AttendeeServiceBulkSendChunkingTest.php @@ -0,0 +1,242 @@ + 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL']; + } + + /** + * @return ProcessAttendeesEmailRequestJob[] + */ + private function pushedJobs(): array + { + $jobs = []; + Queue::assertPushed(ProcessAttendeesEmailRequestJob::class, function ($job) use (&$jobs) { + $jobs[] = $job; + return true; + }); + return $jobs; + } + + private function jobProperty(ProcessAttendeesEmailRequestJob $job, string $name) + { + $reflection = new ReflectionObject($job); + $property = $reflection->getProperty($name); + $property->setAccessible(true); + return $property->getValue($job); + } + + public function testDispatchesOneChunkPerConfiguredSizeWithNoOverlap(): void + { + Queue::fake(); + $chunkSize = intval(Config::get('emails.attendees_process_job_chunk_size', 2000)); + $remainder = 50; + $ids = range(1, 2 * $chunkSize + $remainder); + $payload = $this->basePayload(); + $payload['attendees_ids'] = $ids; + + $this->service()->triggerSend(self::$summit, $payload, null); + + Queue::assertPushed(ProcessAttendeesEmailRequestJob::class, 3); + + $jobs = $this->pushedJobs(); + $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['attendees_ids'], $jobs); + + $this->assertCount($chunkSize, $slices[0]); + $this->assertCount($chunkSize, $slices[1]); + $this->assertCount($remainder, $slices[2]); + + $reassembled = array_merge($slices[0], $slices[1], $slices[2]); + sort($reassembled); + $this->assertSame($ids, $reassembled, 'chunks must partition the id set with no overlap and no gap'); + } + + public function testDispatchesExactlyOneJobWhenMatchedCountEqualsChunkSize(): void + { + Queue::fake(); + $chunkSize = intval(Config::get('emails.attendees_process_job_chunk_size', 2000)); + $payload = $this->basePayload(); + $payload['attendees_ids'] = range(1, $chunkSize); + + $this->service()->triggerSend(self::$summit, $payload, null); + + Queue::assertPushed(ProcessAttendeesEmailRequestJob::class, 1); + } + + public function testDispatchesNothingWhenAttendeeIdsIsEmpty(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['attendees_ids'] = []; + + $this->service()->triggerSend(self::$summit, $payload, null); + + Queue::assertNothingPushed(); + } + + public function testExcludedAttendeeIdsAreRemovedBeforeChunking(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['attendees_ids'] = [1, 2, 3, 4, 5]; + $payload['excluded_attendees_ids'] = [2, 4]; + + $this->service()->triggerSend(self::$summit, $payload, null); + + $jobs = $this->pushedJobs(); + $this->assertCount(1, $jobs); + $ids = $this->jobProperty($jobs[0], 'payload')['attendees_ids']; + sort($ids); + $this->assertSame([1, 3, 5], $ids); + + // excluded_attendees_ids must not leak into the chunk payload itself + $this->assertArrayNotHasKey('excluded_attendees_ids', $this->jobProperty($jobs[0], 'payload')); + } + + public function testDuplicateExplicitIdsAreDedupedBeforeDispatch(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['attendees_ids'] = [1, 2, 2, 3, 1]; + + $this->service()->triggerSend(self::$summit, $payload, null); + + $jobs = $this->pushedJobs(); + $this->assertCount(1, $jobs); + $ids = $this->jobProperty($jobs[0], 'payload')['attendees_ids']; + sort($ids); + $this->assertSame([1, 2, 3], $ids); + } + + public function testChunkPayloadCarriesOtherPayloadKeysThroughAndStripsCallerSuppliedResumeSince(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['attendees_ids'] = [1, 2, 3]; + $payload['outcome_email_recipient'] = 'ops@example.com'; + $payload['resume_since'] = 123456; // caller-supplied - must never reach a first-attempt chunk + + $this->service()->triggerSend(self::$summit, $payload, null); + + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertSame('ops@example.com', $chunkPayload['outcome_email_recipient']); + $this->assertArrayHasKey('dispatched_at', $chunkPayload); + $this->assertArrayNotHasKey('resume_since', $chunkPayload); + } + + public function testFilterBasedSelectionResolvesRealMatchingAttendeesAndChunks(): void + { + Queue::fake(); + $payload = $this->basePayload(); + // no attendees_ids - triggers the paginated getAllIdsByPage path + + $this->service()->triggerSend(self::$summit, $payload, null); + + $jobs = $this->pushedJobs(); + $this->assertGreaterThanOrEqual(1, count($jobs)); + + $ids = []; + foreach ($jobs as $job) { + $ids = array_merge($ids, $this->jobProperty($job, 'payload')['attendees_ids']); + } + sort($ids); + + $expected = []; + foreach (self::$summit->getAttendees() as $attendee) { + $expected[] = $attendee->getId(); + } + sort($expected); + + $this->assertSame($expected, $ids, 'every fixture attendee must be covered exactly once'); + } + + public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): void + { + // Per-chunk failure isolation: JobDispatcher::withDbFallback tries the primary + // connection, the database fallback, and a synchronous run. When ALL THREE fail for a + // chunk, the per-chunk try/catch must log at error level and keep the loop going, so a + // bad chunk cannot also block every sibling chunk. Forcing every Bus dispatch to throw + // makes every chunk fail through all 3 tiers; one Log::error per chunk proves the loop + // reached every chunk instead of aborting on the first. + Config::set('emails.attendees_process_job_chunk_size', 100); + + \Illuminate\Support\Facades\Bus::shouldReceive('dispatch') + ->andThrow(new \RuntimeException('queue backend down')); + \Illuminate\Support\Facades\Bus::shouldReceive('dispatchSync') + ->andThrow(new \RuntimeException('sync run failed')); + \Illuminate\Support\Facades\Log::spy(); + + $payload = $this->basePayload(); + $payload['attendees_ids'] = range(1, 250); // 3 chunks of 100/100/50 + + $this->service()->triggerSend(self::$summit, $payload, null); + + // At least one Log::error per chunk (JobDispatcher logs its own on the database + // fallback failing, plus the per-chunk catch). If the try/catch moved outside the + // loop, only the first chunk would ever be attempted (< 3 errors); if the catch + // were removed, the exception would propagate and fail this test outright. + \Illuminate\Support\Facades\Log::shouldHaveReceived('error')->atLeast()->times(3); + } +} From cabd31a970ff79eec7c7075020ce0fb6e4e7c7b7 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:53:57 -0300 Subject: [PATCH 05/19] feat(attendees): add failed() hook and fix the extra-questions N+1 ProcessAttendeesEmailRequestJob::failed() mirrors ProcessSpeakersEmailRequestJob::failed(): once both ResumableChunkJob attempts are exhausted, log the chunk's attendee ids at error with the exception class and message, and - when outcome_email_recipient was supplied - dispatch a SummitAttendeeExcerptEmail naming them, routed through JobDispatcher::withDbFallback same as the chunk itself. Nothing else reports this loss beyond a queue_failed_jobs row. Filter values are redacted to field names only before logging. Summit::getMainOrderExtraQuestionsByUsage() gains an instance-level memo. It has exactly one caller (SummitAttendee::getExtraQuestions()) and the same Summit PHP instance is reused for every attendee in a chunk's send loop, so every attendee on the invitation flow event was issuing an identical, uncached DQL query. Collapses N queries per chunk to 1. --- .../ProcessAttendeesEmailRequestJob.php | 107 +++++++++++++ app/Models/Foundation/Summit/Summit.php | 22 ++- ...AttendeesEmailRequestJobFailedHookTest.php | 145 ++++++++++++++++++ tests/SummitExtraQuestionsMemoizationTest.php | 103 +++++++++++++ 4 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 tests/ProcessAttendeesEmailRequestJobFailedHookTest.php create mode 100644 tests/SummitExtraQuestionsMemoizationTest.php diff --git a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php index 779e1cfcd..234129d63 100644 --- a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php +++ b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php @@ -11,14 +11,20 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\Registration\Attendees\SummitAttendeeExcerptEmail; use App\Jobs\Emails\Traits\ResumableChunkJob; +use App\Jobs\Utils\JobDispatcher; use App\Services\Model\IAttendeeService; +use App\Services\utils\IEmailExcerptService; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; +use models\summit\ISummitRepository; use models\summit\Summit; use services\model\IAttendeeEmailFilterFields; use utils\FilterParser; @@ -75,4 +81,105 @@ public function handle(IAttendeeService $service){ $service->send($this->summit_id, $this->payload, $filter); } + /** + * Invoked by the queue worker once this job is marked failed - with ResumableChunkJob's + * tries = 2 and the resume check inside AttendeeService::send (Task 3), that means BOTH + * attempts failed: a chunk whose worker was merely killed mid-run (rolling deploy, OOM, + * scale-down) is re-served and automatically resumed once, skipping only the attendees this + * run already reached. This hook only fires when that automatic resume itself also failed to + * finish the chunk. Nothing else reports that loss - the outcome excerpt is only sent when + * send() runs to completion - so without this hook a dead chunk leaves no trace beyond a + * queue_failed_jobs row. + * + * Log the chunk's attendee ids at error, and when the operator asked for an outcome e-mail + * send one naming them, so the chunk can be re-sent by id. Mirrors + * ProcessSpeakersEmailRequestJob::failed(), simplified: attendees generate no promo codes on + * this path, so there is no should_resend/promo-code resend caveat to add. + * + * @param \Throwable $e + */ + public function failed(\Throwable $e): void + { + $attendee_ids = $this->payload['attendees_ids'] ?? []; + $flow_event = $this->payload['email_flow_event'] ?? ''; + $ids_list = implode(', ', $attendee_ids); + + Log::error + ( + sprintf + ( + "ProcessAttendeesEmailRequestJob::failed summit %s flow_event %s: chunk of %s attendee(s) failed (%s: %s); up to %s of them may not have been processed. Attendee ids in the chunk: [%s] filter fields %s.", + $this->summit_id, + $flow_event, + count($attendee_ids), + get_class($e), + $e->getMessage(), + count($attendee_ids), + $ids_list, + json_encode($this->redactFilterFieldNames($this->filter)) + ) + ); + + $outcome_email_recipient = $this->payload['outcome_email_recipient'] ?? null; + if (empty($outcome_email_recipient)) return; + + try { + $summit = App::make(ISummitRepository::class)->getById($this->summit_id); + if (!$summit instanceof Summit) { + Log::warning(sprintf("ProcessAttendeesEmailRequestJob::failed summit %s not found, outcome excerpt not sent", $this->summit_id)); + return; + } + + // Same line types AbstractExcerptEmailJob renders for a completed run, so the + // operator's inbox reads the same either way. + $report = [ + [ + 'type' => IEmailExcerptService::InfoType, + 'message' => sprintf("Processing EMAIL %s for Summit %s", $flow_event, $this->summit_id), + ], + [ + 'type' => IEmailExcerptService::ErrorType, + 'message' => sprintf + ( + "Chunk of %s attendee(s) failed (%s); up to %s of them may not have been processed. Attendee ids in the chunk: %s.", + count($attendee_ids), + $e->getMessage(), + count($attendee_ids), + $ids_list + ), + ], + [ + 'type' => IEmailExcerptService::InfoType, + 'message' => "TOTAL processed for this chunk is unknown, the job did not run to completion", + ], + ]; + + // Same failover route as the chunk itself (AttendeeService::triggerSend): a chunk + // runs on the database fallback worker precisely when the redis primary was down at + // dispatch time, so a bare ::dispatch() here would throw into the catch below and lose + // the report in the one scenario it exists for. + JobDispatcher::withDbFallback( + job: new SummitAttendeeExcerptEmail($summit, $outcome_email_recipient, $report), + logContext: ['summit_id' => $this->summit_id, 'attendee_count' => count($attendee_ids)], + primaryConnection: Config::get('queue.default') + ); + } + catch (\Throwable $ex) { + Log::error($ex); + } + } + + /** + * Reduces a raw filter (["email==foo@bar.com", "first_name==Jane"]) to just its field names + * (["email", "first_name"]) so error logs never carry filter values that may be PII - email + * and first_name are valid filter fields (IAttendeeEmailFilterFields::OPERATORS). + * + * @param mixed $filter + * @return string[] + */ + private function redactFilterFieldNames($filter): array + { + if (empty($filter) || !is_array($filter)) return []; + return array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $filter); + } } diff --git a/app/Models/Foundation/Summit/Summit.php b/app/Models/Foundation/Summit/Summit.php index 0c6160e96..f2a7dfc41 100644 --- a/app/Models/Foundation/Summit/Summit.php +++ b/app/Models/Foundation/Summit/Summit.php @@ -4475,12 +4475,28 @@ public function getMandatoryOrderExtraQuestionsByUsage(string $usage) return $this->order_extra_questions->matching($criteria); } + /** + * Instance-level memo keyed by $usage. Not initialized in the constructor - Doctrine + * proxies may not run it - so the inline default is what guarantees this is always an + * array. getMainOrderExtraQuestionsByUsage() has exactly one caller in the codebase + * (SummitAttendee::getExtraQuestions()), and the same Summit PHP instance is reused for + * every attendee in a bulk email send's do-while loop (ParametrizedSendEmails::_sendEmails + * fetches the root entity once), so this turns N identical queries per chunk into 1. + * + * @var array + */ + private $main_order_extra_questions_cache = []; + /** * @param string $usage * @return array */ public function getMainOrderExtraQuestionsByUsage(string $usage): array { + if (isset($this->main_order_extra_questions_cache[$usage])) { + return $this->main_order_extra_questions_cache[$usage]; + } + $dql = <<createQuery($dql); - return $query + $result = $query ->setParameter('summit_id', $this->getIdentifier()) ->setParameter('usage1', $usage) ->setParameter('usage2', SummitOrderExtraQuestionTypeConstants::BothQuestionUsage) ->getResult(); + + $this->main_order_extra_questions_cache[$usage] = $result; + + return $result; } /** diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php new file mode 100644 index 000000000..02b75d31f --- /dev/null +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -0,0 +1,145 @@ + failed() + * plumbing can't be driven end to end without a real queue connection. What this class pins is + * what OUR hook does. + * + * Class ProcessAttendeesEmailRequestJobFailedHookTest + */ +final class ProcessAttendeesEmailRequestJobFailedHookTest extends TestCase +{ + use InsertSummitTestData; + + use InsertMemberTestData; + + protected function setUp(): void + { + parent::setUp(); + self::insertMemberTestData(IGroup::TrackChairs); + self::$defaultMember = self::$member; + self::insertSummitTestData(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + } + + private function jobProperty(object $job, string $name) + { + $reflection = new ReflectionObject($job); + while ($reflection && !$reflection->hasProperty($name)) { + $reflection = $reflection->getParentClass(); + } + $property = $reflection->getProperty($name); + $property->setAccessible(true); + return $property->getValue($job); + } + + public function testFailedChunkWithOutcomeRecipientSendsExcerptNamingTheUnprocessedIds(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [11, 22, 33], + 'outcome_email_recipient' => 'outcome@example.com', + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertPushed(SummitAttendeeExcerptEmail::class, 1); + Queue::assertPushed(SummitAttendeeExcerptEmail::class, function ($excerpt) { + $this->assertEquals('outcome@example.com', $this->jobProperty($excerpt, 'to_email')); + + $lines = $this->jobProperty($excerpt, 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + + $this->assertCount(1, $errorLines, 'the excerpt must carry exactly one ERROR line for the lost chunk'); + $this->assertStringContainsString('11, 22, 33', $errorLines[0], 'the ERROR line must name every attendee id in the chunk'); + $this->assertStringContainsString('worker killed mid-chunk', $errorLines[0], 'the ERROR line must carry the failure reason'); + $this->assertEmpty( + array_filter($lines, fn($l) => str_starts_with($l, 'Email type')), + 'a failed chunk must not report any e-mail as sent' + ); + return true; + }); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, (string) self::$summit->getId()) + && str_contains($message, '11, 22, 33')) + ->once(); + } + + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [44, 55], + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertNotPushed(SummitAttendeeExcerptEmail::class); + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) && str_contains($message, '44, 55')) + ->once(); + } + + public function testFailedChunkLogsFilterFieldNamesButNotTheirValues(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [1], + ], ['email==someone-private@example.com', 'first_name==Jane']); + + $job->failed(new \RuntimeException('boom')); + + Log::shouldHaveReceived('error') + ->withArgs(function ($message) { + if (!is_string($message)) return false; + $this->assertStringNotContainsString('someone-private@example.com', $message, 'filter VALUES must never reach the log'); + $this->assertStringNotContainsString('Jane', $message); + return str_contains($message, 'email') && str_contains($message, 'first_name'); + }) + ->once(); + } +} diff --git a/tests/SummitExtraQuestionsMemoizationTest.php b/tests/SummitExtraQuestionsMemoizationTest.php new file mode 100644 index 000000000..69d1faedc --- /dev/null +++ b/tests/SummitExtraQuestionsMemoizationTest.php @@ -0,0 +1,103 @@ + + * getMainOrderExtraQuestionsByUsage()) issued an identical, uncached DQL query - the same + * summit's question set does not change mid-request. This asserts the SQL directly (a + * black-box, non-test-only-API technique - matching DoctrineSummitAttendeeRepositoryTest's + * approach in Task 1), the only reliable way to distinguish "queried once" from "queried twice + * and both times returned the same rows". + * + * Class SummitExtraQuestionsMemoizationTest + */ +final class SummitExtraQuestionsMemoizationTest extends TestCase +{ + use InsertSummitTestData; + + use InsertMemberTestData; + + protected function setUp(): void + { + parent::setUp(); + self::insertMemberTestData(IGroup::TrackChairs); + self::$defaultMember = self::$member; + self::insertSummitTestData(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + } + + public function testSecondCallWithSameUsageIssuesNoAdditionalQuery(): void + { + $captured_sql = []; + $logger = new class($captured_sql) implements SQLLogger { + private array $sink; + + public function __construct(array &$sink) + { + $this->sink = &$sink; + } + + public function startQuery($sql, ?array $params = null, ?array $types = null) + { + $this->sink[] = $sql; + } + + public function stopQuery() + { + } + }; + + $connection = Registry::getManager(SilverstripeBaseModel::EntityManager)->getConnection(); + $previous_logger = $connection->getConfiguration()->getSQLLogger(); + $connection->getConfiguration()->setSQLLogger($logger); + + try { + self::$summit->getMainOrderExtraQuestionsByUsage(SummitOrderExtraQuestionTypeConstants::TicketQuestionUsage); + $countAfterFirstCall = count(array_filter($captured_sql, fn($sql) => stripos($sql, 'SummitOrderExtraQuestionType') !== false)); + + self::$summit->getMainOrderExtraQuestionsByUsage(SummitOrderExtraQuestionTypeConstants::TicketQuestionUsage); + $countAfterSecondCall = count(array_filter($captured_sql, fn($sql) => stripos($sql, 'SummitOrderExtraQuestionType') !== false)); + } finally { + $connection->getConfiguration()->setSQLLogger($previous_logger); + } + + $this->assertSame(1, $countAfterFirstCall, 'the first call must issue exactly one query'); + $this->assertSame( + $countAfterFirstCall, + $countAfterSecondCall, + 'a second call with the same usage must not issue an additional query' + ); + } + + public function testResultsAreIdenticalAcrossCalls(): void + { + $first = self::$summit->getMainOrderExtraQuestionsByUsage(SummitOrderExtraQuestionTypeConstants::TicketQuestionUsage); + $second = self::$summit->getMainOrderExtraQuestionsByUsage(SummitOrderExtraQuestionTypeConstants::TicketQuestionUsage); + + $this->assertSame($first, $second, 'the cached result must be the same data the uncached query would have returned'); + } +} From d0aafc7a4a641e75277a80d7fe7f7e470389ad0e Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 16:57:33 -0300 Subject: [PATCH 06/19] test(attendees): add job-level regression tests for resume and failure reporting ProcessAttendeesEmailRequestJobResumeTest mirrors ProcessSpeakersEmailRequestJobResumeTest (minus should_resend, which attendees don't use): first attempt sets no resume_since, a second attempt with dispatched_at sets resume_since, a second attempt without dispatched_at (pre-deploy-window chunk) sets none, and job timeout stays strictly below every queue connection's retry_after. Rounds out ProcessAttendeesEmailRequestJobFailedHookTest with the database-fallback failover case that was left out when the failed() hook itself landed: when the primary Bus dispatch of the outcome excerpt throws, it must retry on the database connection rather than losing the report. Red-green verified testHandleOnSecondAttemptSetsResumeSince by temporarily disabling activateResumeIfRetrying() - the test fails, then passes again once restored. --- ...AttendeesEmailRequestJobFailedHookTest.php | 41 ++++++ ...cessAttendeesEmailRequestJobResumeTest.php | 122 ++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 tests/ProcessAttendeesEmailRequestJobResumeTest.php diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php index 02b75d31f..68f0efd7e 100644 --- a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -16,8 +16,10 @@ use App\Jobs\Emails\ProcessAttendeesEmailRequestJob; use App\Jobs\Emails\Registration\Attendees\SummitAttendeeExcerptEmail; use App\Models\Foundation\Main\IGroup; +use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; +use Mockery; use ReflectionObject; /** @@ -103,6 +105,45 @@ public function testFailedChunkWithOutcomeRecipientSendsExcerptNamingTheUnproces ->once(); } + public function testFailedChunkExcerptFailsOverToTheDatabaseQueueWhenThePrimaryDispatchFails(): void + { + // A chunk lands on the database fallback worker precisely when the redis primary was down + // at dispatch time. If that chunk then fails while redis is still down, a bare ::dispatch() + // of the excerpt throws, the best-effort catch swallows it, and the operator report is lost + // in the one scenario it exists for. The excerpt must take the same failover route as the + // chunk itself (JobDispatcher::withDbFallback): primary throws, database gets the job. + Log::spy(); + $captured = []; + $excerpt = Mockery::type(SummitAttendeeExcerptEmail::class); + // Only the excerpt dispatches are scripted (primary throws, fallback captures); every + // other dispatch goes to the real dispatcher. + $realBus = Bus::getFacadeRoot(); + Bus::shouldReceive('dispatch')->with($excerpt)->once()->andThrow(new \RuntimeException('redis down')); + Bus::shouldReceive('dispatch')->with($excerpt)->once()->andReturnUsing(function ($job) use (&$captured) { + $captured[] = $job; + return null; + }); + Bus::shouldReceive('dispatch')->andReturnUsing(fn($job) => $realBus->dispatch($job)); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [11, 22], + 'outcome_email_recipient' => 'outcome@example.com', + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + $this->assertCount(1, $captured, 'after the primary dispatch fails the excerpt must be re-dispatched on the fallback connection, not swallowed'); + $this->assertInstanceOf(SummitAttendeeExcerptEmail::class, $captured[0]); + $this->assertSame('database', $captured[0]->connection, 'the retry must target the database fallback connection'); + $this->assertEquals('outcome@example.com', $this->jobProperty($captured[0], 'to_email')); + + $lines = $this->jobProperty($captured[0], 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + $this->assertCount(1, $errorLines); + $this->assertStringContainsString('11, 22', $errorLines[0], 'the failed-over excerpt must still name every attendee id in the chunk'); + } + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void { Queue::fake(); diff --git a/tests/ProcessAttendeesEmailRequestJobResumeTest.php b/tests/ProcessAttendeesEmailRequestJobResumeTest.php new file mode 100644 index 000000000..57f3515f3 --- /dev/null +++ b/tests/ProcessAttendeesEmailRequestJobResumeTest.php @@ -0,0 +1,122 @@ + + * 1) with a dispatched_at already stamped in the payload, handle() must add resume_since = + * dispatched_at before calling send(). Attendees have no should_resend concept (unlike speakers - + * see Task 3's Context for Implementer), so this covers only the resume_since half of the + * speaker precedent. + * + * attempts() is driven via InteractsWithQueue::setJob() with a mocked + * Illuminate\Contracts\Queue\Job, since $this->attempts() returns 1 whenever no job instance is + * set (dispatchSync, direct calls) and there is no other way to simulate a second delivery + * without a real queue connection. + * + * Class ProcessAttendeesEmailRequestJobResumeTest + */ +final class ProcessAttendeesEmailRequestJobResumeTest extends TestCase +{ + private function jobWithAttempts(array $payload, int $attempts): ProcessAttendeesEmailRequestJob + { + $job = new ProcessAttendeesEmailRequestJob(new Summit(), $payload, null); + + $queueJob = Mockery::mock(QueueJobContract::class); + $queueJob->shouldReceive('attempts')->andReturn($attempts); + $job->setJob($queueJob); + + return $job; + } + + public function testHandleOnSecondAttemptSetsResumeSince(): void + { + $payload = [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [11, 22], + 'dispatched_at' => 1700000000, + ]; + $job = $this->jobWithAttempts($payload, 2); + + $service = Mockery::mock(IAttendeeService::class); + $service->shouldReceive('send')->once()->withArgs(function ($summit_id, $sentPayload) { + return ($sentPayload['resume_since'] ?? null) === 1700000000; + }); + + $job->handle($service); + } + + public function testHandleOnFirstAttemptSetsNoResumeSince(): void + { + $payload = [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [11, 22], + 'dispatched_at' => 1700000000, + ]; + $job = $this->jobWithAttempts($payload, 1); + + $service = Mockery::mock(IAttendeeService::class); + $service->shouldReceive('send')->once()->withArgs(function ($summit_id, $sentPayload) { + return !array_key_exists('resume_since', $sentPayload); + }); + + $job->handle($service); + } + + public function testHandleOnSecondAttemptWithoutDispatchedAtSetsNoResumeSince(): void + { + // A chunk queued by a pod running the previous version of this job (before dispatched_at + // existed) must not attempt a resume it cannot correctly compute - it retries as a full + // re-run instead. Same deploy-window gap the speaker precedent documents. + $payload = [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [11, 22], + ]; + $job = $this->jobWithAttempts($payload, 2); + + $service = Mockery::mock(IAttendeeService::class); + $service->shouldReceive('send')->once()->withArgs(function ($summit_id, $sentPayload) { + return !array_key_exists('resume_since', $sentPayload); + }); + + $job->handle($service); + } + + /** + * Regression guard for ResumableChunkJob's core safety argument: the job's own $timeout must + * stay strictly below every queue connection's retry_after, or a retried attempt (tries=2) + * could be re-served to a second worker while the first is still running it - true concurrent + * execution of the same chunk, which the resume_since check cannot protect against. + */ + public function testTimeoutStaysStrictlyBelowRetryAfterForEveryQueueConnection(): void + { + $job = new ProcessAttendeesEmailRequestJob(new Summit(), [], null); + + $this->assertLessThan( + config('queue.connections.database.retry_after'), + $job->timeout, + 'job timeout must stay strictly below the database queue retry_after, or a retried attempt can run concurrently with a still-live earlier attempt' + ); + $this->assertLessThan( + config('queue.connections.redis.retry_after'), + $job->timeout, + 'job timeout must stay strictly below the redis queue retry_after, or a retried attempt can run concurrently with a still-live earlier attempt' + ); + } +} From 7c9c1b3fb79347e47b8a26ea9d3572c6898dcbd4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 17:03:41 -0300 Subject: [PATCH 07/19] test(attendees): strengthen the filter-based chunking test to span pages AttendeeServiceResumeSendEmailsTest and AttendeeServiceBulkSendChunkingTest already satisfied Task 7's requirements from Tasks 3 and 4 - both built alongside the production code they cover, TDD RED-first. The one gap: the filter-based selection test resolved the fixture's small attendee count in a single DB page (default page size 2000), so it never exercised the multi-page merge logic in triggerSend's id-resolution loop, despite the plan calling for a test that spans several pages - the exact scenario Task 1's ordering fix exists for. Renamed to testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedIdExactlyOnce and forces the DB chunk size down to 1 for the duration of the test, so a page that is skipped, re-read, or overwritten instead of merged would break the exact-set assertion. Full plan-wide regression: 55 tests, 261 assertions across all 9 test files created or touched by this plan, plus the HTTP controller suite - one pre-existing failure (testRedeemPromoCodes, unrelated, confirmed against unmodified code in Task 1) and nothing else. --- tests/AttendeeServiceBulkSendChunkingTest.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/AttendeeServiceBulkSendChunkingTest.php b/tests/AttendeeServiceBulkSendChunkingTest.php index 2bf19c29e..05d18adb1 100644 --- a/tests/AttendeeServiceBulkSendChunkingTest.php +++ b/tests/AttendeeServiceBulkSendChunkingTest.php @@ -186,9 +186,17 @@ public function testChunkPayloadCarriesOtherPayloadKeysThroughAndStripsCallerSup $this->assertArrayNotHasKey('resume_since', $chunkPayload); } - public function testFilterBasedSelectionResolvesRealMatchingAttendeesAndChunks(): void + public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedIdExactlyOnce(): void { + // The filter-based path is the only one summit-admin drives (it always sends filter[], + // never attendees_ids). Force the id-resolution loop's DB page size down to 1 so it must + // span several pages even over the fixture's small attendee count - a page that is + // skipped, re-read, or overwritten instead of merged breaks the exact-set assertion + // below. This is what exercises Task 1's ordering fix end-to-end: without a deterministic + // ORDER BY, a 1-row-per-page scan is exactly where duplicate/missing rows would surface. Queue::fake(); + Config::set('emails.attendees_process_db_chunk_size', 1); + $payload = $this->basePayload(); // no attendees_ids - triggers the paginated getAllIdsByPage path @@ -209,7 +217,8 @@ public function testFilterBasedSelectionResolvesRealMatchingAttendeesAndChunks() } sort($expected); - $this->assertSame($expected, $ids, 'every fixture attendee must be covered exactly once'); + $this->assertGreaterThanOrEqual(2, count($expected), 'fixture must seed at least 2 attendees for a 1-per-page scan to span pages'); + $this->assertSame($expected, $ids, 'every fixture attendee must be covered exactly once, with no id skipped or repeated across pages'); } public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): void From af1b9feae5c8cc6c1274fea905c6e0688a0f8ed9 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 17:13:58 -0300 Subject: [PATCH 08/19] fix(attendees): correct the $announcement_emails PHPDoc collection type @var SummitAttendeeAnnouncementEmail[] described it as a plain array; at runtime it's a Doctrine Collection (implements Selectable), which is why matching() already works on it. Same PHPStan gap speakers had (PR #598, commit cab6991ce) for the equivalent $announcement_summit_emails property - matched here for parity. Docblock-only change, no behavior change. --- .../Foundation/Summit/Registration/Attendees/SummitAttendee.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php index f0b717387..913341e66 100644 --- a/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php +++ b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php @@ -217,7 +217,7 @@ public function getSummitHallCheckedIn() private $tags; /** - * @var SummitAttendeeAnnouncementEmail[] + * @var \Doctrine\Common\Collections\Collection */ #[ORM\OneToMany(targetEntity: \models\summit\SummitAttendeeAnnouncementEmail::class, mappedBy: 'attendee', cascade: ['persist', 'remove'], orphanRemoval: true, fetch: 'EXTRA_LAZY')] private $announcement_emails; From 5ed82d820f49044054ca350fbf712ca0bd58802d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 17:29:22 -0300 Subject: [PATCH 09/19] fix(attendees): correct the stale 2000 fallback for the job chunk size Config::get('emails.attendees_process_job_chunk_size', 2000)'s fallback literal still said 2000 after the default moved to 200 in config/emails.php - dead code under normal operation (the key is always defined), but a real inconsistency if that config entry were ever removed. Found by the changes-review agent. --- app/Services/Model/AttendeeService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 2521cddb2..2e8e04319 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -610,7 +610,7 @@ public function regenerateAttendeeBadgesQRCodes(SummitAttendee $attendee): void public function triggerSend(Summit $summit, array $payload, $filter = null): void { $process_db_chunk_size = intval(Config::get('emails.attendees_process_db_chunk_size', 2000)); - $process_job_chunk_size = intval(Config::get('emails.attendees_process_job_chunk_size', 2000)); + $process_job_chunk_size = intval(Config::get('emails.attendees_process_job_chunk_size', 200)); Log::debug ( From aea09e9e8e11b6e08c5fcb2517e655bef6817757 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 18:02:26 -0300 Subject: [PATCH 10/19] fix(attendees): route resume-skip notices to the excerpt as INFO lines, not ERROR AttendeeService::send's processCurrentId closure declared its last two callbacks as ($onDispatchInfo, $onDispatchError), but ParametrizedSendEmails::_sendEmails passes them positionally as (success, error, info) - the order SpeakerService's closure already uses. Every resume-skip notice therefore reached the outcome excerpt through EmailExcerpt::addErrorMessage as an ERROR line, and every strategy error through addInfoMessage as an INFO line. Reorder the closure's parameters (and the inner use list) to match the positional contract, and add a test asserting a resumed run reports the skipped attendee as exactly one INFO line and no ERROR lines. --- app/Services/Model/AttendeeService.php | 10 ++-- tests/AttendeeServiceResumeSendEmailsTest.php | 48 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 2e8e04319..730de33b3 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -746,9 +746,13 @@ function $test_email_recipient, $announcement_email_config, $filter, + // Positional contract with ParametrizedSendEmails::_sendEmails: it invokes + // processCurrentId with (success, error, info) - same order SpeakerService + // declares. Swapping the last two routes every resume-skip notice into the + // excerpt as an ERROR line and every strategy error as an INFO line. $onDispatchSuccess, - $onDispatchInfo, - $onDispatchError) use ($payload) { + $onDispatchError, + $onDispatchInfo) use ($payload) { try { $resume_since = $payload['resume_since'] ?? null; $this->tx_service->transaction(function () use ( @@ -758,8 +762,8 @@ function $test_email_recipient, $filter, $onDispatchSuccess, - $onDispatchInfo, $onDispatchError, + $onDispatchInfo, $resume_since, $payload ) { diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php index b0177d33f..a2d9153b7 100644 --- a/tests/AttendeeServiceResumeSendEmailsTest.php +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -16,6 +16,8 @@ use App\Jobs\Emails\SummitAttendeeTicketRegenerateHashEmail; use App\Models\Foundation\Main\IGroup; use App\Services\Model\IAttendeeService; +use App\Services\utils\IEmailExcerptService; +use App\Services\Utils\Facades\EmailExcerpt; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\Registry; @@ -156,6 +158,52 @@ public function testResumedRunSkipsAttendeeWithProofSinceDispatchAndDoesNotDupli $this->assertSame(1, $this->proofCount($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG)); } + /** + * The resume-skip notice is informational: it must reach the outcome excerpt through the + * INFO callback, never the ERROR one. ParametrizedSendEmails::_sendEmails hands + * processCurrentId its callbacks positionally as (success, error, info), so a closure that + * declares them in a different order silently routes every skip into + * EmailExcerpt::addErrorMessage and the operator's report shows a wall of errors for a run + * that did exactly what it should. + */ + public function testResumedRunReportsTheSkipAsAnInfoLineNotAnError(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + $attendee_email = $attendee->getEmail(); + + $dispatchedAt = time() - 600; + + $this->givenAttendeeHasProof($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG, null); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => [$attendee_id], + 'dispatched_at' => $dispatchedAt, + 'resume_since' => $dispatchedAt, + ]); + + Queue::assertNotPushed(GenericSummitAttendeeEmail::class); + + $report = EmailExcerpt::getReport(); + + $skipLines = array_values(array_filter( + $report, + fn($line) => str_contains($line['message'] ?? '', $attendee_email) + )); + $this->assertCount(1, $skipLines, 'exactly one excerpt line must name the resume-skipped attendee'); + $this->assertSame( + IEmailExcerptService::InfoType, + $skipLines[0]['type'], + 'the resume-skip notice must be an INFO line, not an ERROR line' + ); + + $errorLines = array_filter($report, fn($line) => ($line['type'] ?? null) === IEmailExcerptService::ErrorType); + $this->assertCount(0, $errorLines, 'a resumed run that only skipped an already-reached attendee must report no errors'); + } + public function testResumedRunStillProcessesAttendeeWithProofBeforeDispatch(): void { Queue::fake(); From 1f7bff3452fa4301355d383a76c20883fa9a0270 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 8 Sep 2026 18:07:08 -0300 Subject: [PATCH 11/19] fix(attendees): skip explicit attendee ids that belong to another summit AttendeeService::send loaded each id with getByIdExclusiveLock - a bare find() by primary key - and nothing upstream verified that an explicit attendees_ids entry belongs to the summit the send was requested for: auth.user only checks the endpoint's global groups, and CurrentSummitFinderStrategy only resolves the summit. A foreign id was emailed under the wrong summit's context and, since the sent-proof was introduced, its proof row was stamped with the requesting summit's id. Guard right after the lock: when the attendee's summit differs from the requested one, log a warning, add one ERROR line to the outcome excerpt naming the attendee, and return before any side effect. Covered by a test that sends a summit-1 attendee id against summit 2 and asserts no email is pushed, no proof is written, and exactly one ERROR line is reported. --- app/Services/Model/AttendeeService.php | 31 ++++++++++++++++++ tests/AttendeeServiceResumeSendEmailsTest.php | 32 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 730de33b3..a119af16f 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -773,6 +773,37 @@ function if (!$attendee instanceof SummitAttendee) return; + // getByIdExclusiveLock is a bare find() by primary key, and nothing upstream + // (route middleware, CurrentSummitFinderStrategy) checks that an explicit + // attendees_ids entry belongs to the summit this send was requested for. + // Skip before any side effect: the email would go out under the wrong + // summit's context and the sent-proof would be stamped with its id. + if ($attendee->getSummitId() !== $summit->getId()) { + Log::warning + ( + sprintf + ( + "AttendeeService::send attendee %s belongs to summit %s, not to requested summit %s, skipped", + $attendee_id, + $attendee->getSummitId(), + $summit->getId() + ) + ); + if (!is_null($onDispatchError)) { + $onDispatchError + ( + sprintf + ( + "Attendee %s (%s) does not belong to summit %s, skipped.", + $attendee->getEmail(), + $attendee_id, + $summit->getId() + ) + ); + } + return; + } + $emailActionsStrategyFactory = new EmailActionsStrategyFactory(); $strategy = $emailActionsStrategyFactory->build($summit, $flow_event); if ($strategy != null) { diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php index a2d9153b7..fd150369a 100644 --- a/tests/AttendeeServiceResumeSendEmailsTest.php +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -204,6 +204,38 @@ public function testResumedRunReportsTheSkipAsAnInfoLineNotAnError(): void $this->assertCount(0, $errorLines, 'a resumed run that only skipped an already-reached attendee must report no errors'); } + /** + * send() loads each id with getByIdExclusiveLock, a bare find() by primary key. An explicit + * attendees_ids payload can therefore name an attendee of a different summit than the one + * the request was made for; nothing upstream (route middleware, CurrentSummitFinderStrategy) + * checks that. The attendee must be skipped before any side effect: no email dispatched, no + * sent-proof written (it would be stamped with the requesting summit's id), and the operator's + * excerpt must carry exactly one ERROR line naming the attendee. + */ + public function testSendSkipsAnAttendeeThatDoesNotBelongToTheRequestedSummit(): void + { + Queue::fake(); + + $attendee = self::$summit->getAttendees()->first(); + $attendee_id = $attendee->getId(); + $this->assertNotSame(self::$summit->getId(), self::$summit2->getId(), 'fixture must provide a second, distinct summit'); + + $this->service()->send(self::$summit2->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => [$attendee_id], + ]); + + Queue::assertNotPushed(GenericSummitAttendeeEmail::class); + $this->assertSame(0, $this->proofCount($attendee_id, GenericSummitAttendeeEmail::EVENT_SLUG), 'no sent-proof may be written for an attendee of another summit'); + + $errorLines = array_values(array_filter( + EmailExcerpt::getReport(), + fn($line) => ($line['type'] ?? null) === IEmailExcerptService::ErrorType + )); + $this->assertCount(1, $errorLines, 'the excerpt must carry exactly one ERROR line for the foreign attendee'); + $this->assertStringContainsString(sprintf('(%s)', $attendee_id), $errorLines[0]['message'], 'the ERROR line must name the skipped attendee'); + } + public function testResumedRunStillProcessesAttendeeWithProofBeforeDispatch(): void { Queue::fake(); From b363a5cc8bb0f40ff35ca7fef95b9a99920771c9 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 00:43:18 -0300 Subject: [PATCH 12/19] fix(emails): resolve bulk-send ids inside one REPEATABLE READ transaction AttendeeService::triggerSend and SpeakerService::triggerSendEmails paged the matching ids with LIMIT/OFFSET, opening one READ COMMITTED transaction per page. A row that was deleted or stopped matching the filter between two page reads shifted every later row left by one, silently dropping one recipient. Wrap the whole scan in a single root transaction at REPEATABLE READ so every page reads the same InnoDB snapshot. The transaction service defaults to READ COMMITTED, which takes a fresh snapshot per statement, so the level is passed explicitly. Reads only, no locks held; chunk dispatch stays outside the transaction. Regression test in both chunking test classes asserts, on the captured SQL, exactly one START TRANSACTION preceded by a REPEATABLE READ isolation statement across a multi-page scan. --- app/Services/Model/AttendeeService.php | 25 ++++--- app/Services/Model/Imp/SpeakerService.php | 25 ++++--- tests/AttendeeServiceBulkSendChunkingTest.php | 70 +++++++++++++++++++ tests/SpeakerServiceBulkSendChunkingTest.php | 70 +++++++++++++++++++ 4 files changed, 172 insertions(+), 18 deletions(-) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index a119af16f..879cddc19 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -20,6 +20,7 @@ use App\Services\Model\Imp\Traits\ParametrizedSendEmails; use App\Services\Model\Strategies\EmailActions\EmailActionsStrategyFactory; use App\Utils\AES; +use Doctrine\DBAL\TransactionIsolationLevel; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; @@ -632,15 +633,21 @@ public function triggerSend(Summit $summit, array $payload, $filter = null): voi $parsedFilter = !is_null($filter) ? FilterParser::parse($filter, IAttendeeEmailFilterFields::OPERATORS) : new Filter(); if (!$parsedFilter->hasFilter("summit_id")) $parsedFilter->addFilterCondition(FilterElement::makeEqual('summit_id', $summit->getId())); - $ids = []; - $page = 1; - do { - $currentPage = $this->tx_service->transaction(function () use ($page, $parsedFilter, $process_db_chunk_size) { - return $this->attendee_repository->getAllIdsByPage(new PagingInfo($page, $process_db_chunk_size), $parsedFilter); - }); - $ids = array_merge($ids, $currentPage); - $page++; - } while (count($currentPage) > 0); + // One root transaction at REPEATABLE READ around the whole scan, not one per page: every + // LIMIT/OFFSET page then reads the same InnoDB snapshot, so an attendee that is deleted or + // stops matching the filter while the loop runs cannot shift later rows and silently drop + // one id. READ COMMITTED (the transaction service default) takes a fresh snapshot per + // statement and would leave that cross-page drift in place. Reads only - no locks held. + $ids = $this->tx_service->transaction(function () use ($parsedFilter, $process_db_chunk_size) { + $ids = []; + $page = 1; + do { + $currentPage = $this->attendee_repository->getAllIdsByPage(new PagingInfo($page, $process_db_chunk_size), $parsedFilter); + $ids = array_merge($ids, $currentPage); + $page++; + } while (count($currentPage) > 0); + return $ids; + }, TransactionIsolationLevel::REPEATABLE_READ); } if (isset($payload['excluded_attendees_ids'])) { diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index 216407a28..e10a927d0 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -13,6 +13,7 @@ **/ use App\Http\Utils\IFileUploader; +use Doctrine\DBAL\TransactionIsolationLevel; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessExcerptEmail; use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionApprovedEmail; use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionRejectedEmail; @@ -1258,15 +1259,21 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null $ids = $payload['speaker_ids']; } else { $parsedFilter = !is_null($filter) ? FilterParser::parse($filter, ISpeakerFilterFields::OPERATORS) : null; - $ids = []; - $page = 1; - do { - $currentPage = $this->tx_service->transaction(function () use ($summit, $page, $parsedFilter, $process_db_chunk_size) { - return $this->speaker_repository->getSpeakersIdsBySummit($summit, new PagingInfo($page, $process_db_chunk_size), $parsedFilter); - }); - $ids = array_merge($ids, $currentPage); - $page++; - } while (count($currentPage) > 0); + // One root transaction at REPEATABLE READ around the whole scan, not one per page: every + // LIMIT/OFFSET page then reads the same InnoDB snapshot, so a speaker that is deleted or + // stops matching the filter while the loop runs cannot shift later rows and silently drop + // one id. READ COMMITTED (the transaction service default) takes a fresh snapshot per + // statement and would leave that cross-page drift in place. Reads only - no locks held. + $ids = $this->tx_service->transaction(function () use ($summit, $parsedFilter, $process_db_chunk_size) { + $ids = []; + $page = 1; + do { + $currentPage = $this->speaker_repository->getSpeakersIdsBySummit($summit, new PagingInfo($page, $process_db_chunk_size), $parsedFilter); + $ids = array_merge($ids, $currentPage); + $page++; + } while (count($currentPage) > 0); + return $ids; + }, TransactionIsolationLevel::REPEATABLE_READ); } if (isset($payload['excluded_speaker_ids'])) { diff --git a/tests/AttendeeServiceBulkSendChunkingTest.php b/tests/AttendeeServiceBulkSendChunkingTest.php index 05d18adb1..80f362a73 100644 --- a/tests/AttendeeServiceBulkSendChunkingTest.php +++ b/tests/AttendeeServiceBulkSendChunkingTest.php @@ -15,9 +15,12 @@ use App\Jobs\Emails\ProcessAttendeesEmailRequestJob; use App\Models\Foundation\Main\IGroup; use App\Services\Model\IAttendeeService; +use Doctrine\DBAL\Logging\SQLLogger; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Queue; +use LaravelDoctrine\ORM\Facades\Registry; +use models\utils\SilverstripeBaseModel; use ReflectionObject; /** @@ -248,4 +251,71 @@ public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): // were removed, the exception would propagate and fail this test outright. \Illuminate\Support\Facades\Log::shouldHaveReceived('error')->atLeast()->times(3); } + + /** + * The id-resolution loop pages with LIMIT/OFFSET. If each page were read in its own + * transaction - or in one READ COMMITTED transaction, where every statement takes a fresh + * snapshot - a attendee that is deleted or stops matching the filter between two page reads + * shifts every later row left by one and exactly one attendee is silently skipped. Every + * page must therefore be read from the same InnoDB snapshot: one root transaction, opened at + * REPEATABLE READ before the first page. Asserted on the SQL the connection actually issues + * (the same technique the ORDER BY regression test uses): a concurrent-writer reproduction + * would need a second DB session and a seam between pages the service deliberately does not + * expose. + */ + public function testFilterBasedIdResolutionReadsEveryPageInsideOneRepeatableReadTransaction(): void + { + Queue::fake(); + Config::set('emails.attendees_process_db_chunk_size', 1); + + $captured_sql = []; + $logger = new class($captured_sql) implements SQLLogger { + private array $sink; + + public function __construct(array &$sink) + { + $this->sink = &$sink; + } + + public function startQuery($sql, ?array $params = null, ?array $types = null) + { + $this->sink[] = $sql; + } + + public function stopQuery() + { + } + }; + + $connection = Registry::getManager(SilverstripeBaseModel::EntityManager)->getConnection(); + $previous_logger = $connection->getConfiguration()->getSQLLogger(); + $connection->getConfiguration()->setSQLLogger($logger); + + try { + $this->service()->triggerSend(self::$summit, $this->basePayload(), null); + } finally { + $connection->getConfiguration()->setSQLLogger($previous_logger); + } + + $page_selects = array_keys(array_filter( + $captured_sql, + fn($sql) => stripos($sql, 'SELECT') === 0 && stripos($sql, 'LIMIT') !== false + )); + $this->assertGreaterThanOrEqual(2, count($page_selects), 'a 1-row-per-page scan over the fixture must issue several page SELECTs'); + + $begins = array_keys(array_filter($captured_sql, fn($sql) => stripos($sql, 'START TRANSACTION') !== false)); + $this->assertCount( + 1, + $begins, + sprintf('every id page must be read inside ONE transaction; captured SQL: %s', implode(' | ', $captured_sql)) + ); + $this->assertLessThan(min($page_selects), $begins[0], 'the transaction must be opened before the first page is read'); + + $repeatable_reads = array_keys(array_filter( + $captured_sql, + fn($sql) => stripos($sql, 'TRANSACTION ISOLATION LEVEL REPEATABLE READ') !== false + )); + $this->assertCount(1, $repeatable_reads, 'the id-resolution transaction must run at REPEATABLE READ so every page reads the same snapshot'); + $this->assertLessThan($begins[0], $repeatable_reads[0], 'the isolation level must be set before the transaction starts'); + } } diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php index 77ee73a25..6e245d3ee 100644 --- a/tests/SpeakerServiceBulkSendChunkingTest.php +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -13,11 +13,14 @@ **/ use App\Jobs\Emails\ProcessSpeakersEmailRequestJob; +use Doctrine\DBAL\Logging\SQLLogger; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Queue; +use LaravelDoctrine\ORM\Facades\Registry; use models\main\Member; use models\summit\PresentationSpeaker; +use models\utils\SilverstripeBaseModel; use ReflectionObject; use services\model\ISpeakerService; @@ -371,4 +374,71 @@ public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): // were removed, the exception would propagate and fail this test outright. \Illuminate\Support\Facades\Log::shouldHaveReceived('error')->atLeast()->times(3); } + + /** + * The id-resolution loop pages with LIMIT/OFFSET. If each page were read in its own + * transaction - or in one READ COMMITTED transaction, where every statement takes a fresh + * snapshot - a speaker that is deleted or stops matching the filter between two page reads + * shifts every later row left by one and exactly one speaker is silently skipped. Every + * page must therefore be read from the same InnoDB snapshot: one root transaction, opened at + * REPEATABLE READ before the first page. Asserted on the SQL the connection actually issues + * (the same technique the ORDER BY regression test uses): a concurrent-writer reproduction + * would need a second DB session and a seam between pages the service deliberately does not + * expose. + */ + public function testFilterBasedIdResolutionReadsEveryPageInsideOneRepeatableReadTransaction(): void + { + Queue::fake(); + Config::set('emails.speakers_process_db_chunk_size', 1); + + $captured_sql = []; + $logger = new class($captured_sql) implements SQLLogger { + private array $sink; + + public function __construct(array &$sink) + { + $this->sink = &$sink; + } + + public function startQuery($sql, ?array $params = null, ?array $types = null) + { + $this->sink[] = $sql; + } + + public function stopQuery() + { + } + }; + + $connection = Registry::getManager(SilverstripeBaseModel::EntityManager)->getConnection(); + $previous_logger = $connection->getConfiguration()->getSQLLogger(); + $connection->getConfiguration()->setSQLLogger($logger); + + try { + $this->service()->triggerSendEmails(self::$summit, $this->basePayload(), null); + } finally { + $connection->getConfiguration()->setSQLLogger($previous_logger); + } + + $page_selects = array_keys(array_filter( + $captured_sql, + fn($sql) => stripos($sql, 'SELECT') === 0 && stripos($sql, 'LIMIT') !== false + )); + $this->assertGreaterThanOrEqual(2, count($page_selects), 'a 1-row-per-page scan over the fixture must issue several page SELECTs'); + + $begins = array_keys(array_filter($captured_sql, fn($sql) => stripos($sql, 'START TRANSACTION') !== false)); + $this->assertCount( + 1, + $begins, + sprintf('every id page must be read inside ONE transaction; captured SQL: %s', implode(' | ', $captured_sql)) + ); + $this->assertLessThan(min($page_selects), $begins[0], 'the transaction must be opened before the first page is read'); + + $repeatable_reads = array_keys(array_filter( + $captured_sql, + fn($sql) => stripos($sql, 'TRANSACTION ISOLATION LEVEL REPEATABLE READ') !== false + )); + $this->assertCount(1, $repeatable_reads, 'the id-resolution transaction must run at REPEATABLE READ so every page reads the same snapshot'); + $this->assertLessThan($begins[0], $repeatable_reads[0], 'the isolation level must be set before the transaction starts'); + } } From 91051afe02af7a81f8bce73c9021edae4dcc4a45 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 00:48:53 -0300 Subject: [PATCH 13/19] chore(ci): run the bulk-email chunk/resume test classes in the push matrix No job in the integration-tests matrix runs the tests/ root, only its subdirectories and the explicitly listed files, so the attendee bulk-email test classes added by this branch and the speaker chunk/resume classes added by #595/#598 never executed in CI. Add two path-named shards, one per subject, listing those files. tests/AttendeeServiceTest.php stays out on purpose: its pre-existing testRedeemPromoCodes hardcodes summit id 24 and fails on a fresh database. --- .github/workflows/push.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 71c143955..363f61390 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -80,6 +80,8 @@ jobs: # subdirectories - a file added there runs nowhere unless it is listed here. - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php tests/PresentationSubmissionReopenedEmailTest.php" } - { name: "SpeakerSubmitterPublishedFilter", filter: "tests/ProcessSpeakersEmailRequestJobTest.php tests/ProcessSubmittersEmailRequestJobTest.php tests/SpeakerRepositoryTest.php tests/SubmitterRepositoryTest.php tests/SpeakerServiceOriginalFilterTest.php tests/SubmitterServiceOriginalFilterTest.php" } + - { name: "SpeakerBulkEmailChunkResume", filter: "tests/SpeakerServiceBulkSendChunkingTest.php tests/SpeakerServiceResumeSendEmailsTest.php tests/ProcessSpeakersEmailRequestJobResumeTest.php tests/ProcessSpeakersEmailRequestJobFailedHookTest.php" } + - { name: "AttendeeBulkEmailChunkResume", filter: "tests/AttendeeServiceBulkSendChunkingTest.php tests/AttendeeServiceResumeSendEmailsTest.php tests/ProcessAttendeesEmailRequestJobResumeTest.php tests/ProcessAttendeesEmailRequestJobFailedHookTest.php tests/SummitAttendeeAnnouncementEmailTest.php tests/DoctrineSummitAttendeeRepositoryTest.php tests/SummitExtraQuestionsMemoizationTest.php" } - { name: "Repositories", filter: "tests/Repositories/" } - { name: "Services", filter: "tests/Unit/Services/" } - { name: "Integration", filter: "tests/Integration/" } From d27d7fb04e948862dfb3c2906b74a95d5f55d318 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 00:50:59 -0300 Subject: [PATCH 14/19] fix(emails): redact field names of a scalar filter in the failed-chunk log ProcessAttendeesEmailRequestJob::failed() and ProcessSpeakersEmailRequestJob:: failed() log the filter's field names (never its values, which can be PII) so a lost chunk can be reproduced. redactFilterFieldNames() returned [] for anything that was not an array, but FiltersParams::getFilterParam() passes the raw request value through: filter[] arrives as an array, a bare filter= as a string, and FilterParser::parse accepts both by wrapping the scalar. A scalar filter therefore logged "filter fields []". Wrap a scalar into a one-element array before redacting, keep only scalar conditions, and reindex. Originally flagged by CodeRabbit on #600. --- .../ProcessSpeakersEmailRequestJob.php | 9 +++++-- .../ProcessAttendeesEmailRequestJob.php | 9 +++++-- ...AttendeesEmailRequestJobFailedHookTest.php | 25 +++++++++++++++++++ ...sSpeakersEmailRequestJobFailedHookTest.php | 25 +++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index e35381f4c..617311258 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -210,7 +210,12 @@ public function failed(\Throwable $e): void */ private function redactFilterFieldNames($filter): array { - if (empty($filter) || !is_array($filter)) return []; - return array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $filter); + if (empty($filter)) return []; + // FiltersParams::getFilterParam() passes the raw request value through: filter[] arrives + // as an array, a bare filter= as a string. FilterParser::parse accepts both by wrapping the + // scalar, so the redaction accepts the same shape instead of dropping the field names. + if (!is_array($filter)) $filter = [$filter]; + $conditions = array_filter($filter, 'is_scalar'); + return array_values(array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $conditions)); } } diff --git a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php index 234129d63..b34fe3efe 100644 --- a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php +++ b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php @@ -179,7 +179,12 @@ public function failed(\Throwable $e): void */ private function redactFilterFieldNames($filter): array { - if (empty($filter) || !is_array($filter)) return []; - return array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $filter); + if (empty($filter)) return []; + // FiltersParams::getFilterParam() passes the raw request value through: filter[] arrives + // as an array, a bare filter= as a string. FilterParser::parse accepts both by wrapping the + // scalar, so the redaction accepts the same shape instead of dropping the field names. + if (!is_array($filter)) $filter = [$filter]; + $conditions = array_filter($filter, 'is_scalar'); + return array_values(array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $conditions)); } } diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php index 68f0efd7e..a01569da3 100644 --- a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -183,4 +183,29 @@ public function testFailedChunkLogsFilterFieldNamesButNotTheirValues(): void }) ->once(); } + + /** + * FiltersParams::getFilterParam() hands the job whatever the request carried: summit-admin + * always sends filter[] (an array), but a direct API caller can send filter= as a bare string, + * which FilterParser::parse accepts by wrapping it. The redaction must accept the same shape, + * or the failed-chunk log silently drops the field names it exists to print. + */ + public function testFailedChunkLogsFilterFieldNamesForAScalarFilterToo(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [1], + ], 'email==someone-private@example.com'); + + $job->failed(new \RuntimeException('boom')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'filter fields ["email"]') + && !str_contains($message, 'someone-private@example.com')) + ->once(); + } } diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index a9347e8c0..69bb75f40 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -221,4 +221,29 @@ public function testFailedChunkLogsFilterFieldNamesButNotTheirValues(): void && !str_contains($message, 'foo@bar.com')) ->once(); } + + /** + * FiltersParams::getFilterParam() hands the job whatever the request carried: summit-admin + * always sends filter[] (an array), but a direct API caller can send filter= as a bare string, + * which FilterParser::parse accepts by wrapping it. The redaction must accept the same shape, + * or the failed-chunk log silently drops the field names it exists to print. + */ + public function testFailedChunkLogsFilterFieldNamesForAScalarFilterToo(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [1], + ], 'email==someone-private@example.com'); + + $job->failed(new \RuntimeException('boom')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'filter fields ["email"]') + && !str_contains($message, 'someone-private@example.com')) + ->once(); + } } From 6bc5346ad3c64dcbef52d3dbfcfa621b7c704e2c Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 10:34:39 -0300 Subject: [PATCH 15/19] fix(emails): redact range and set operator operands in failed-chunk logs redactFilterFieldNames() in ProcessAttendeesEmailRequestJob and ProcessSpeakersEmailRequestJob cut a filter condition only at =<>@!, so a range (summit_hall_checked_in_date[]a&&b) or set (field()x||y) condition was logged verbatim, operands included, in the failed-chunk error line. Cut at [ and ( too, covering every operator FilterParser::filterExpresion recognizes. Covered by testFailedChunkLogsFilterFieldNamesButNotRangeOperatorValues in both failed-hook test classes. --- .../ProcessSpeakersEmailRequestJob.php | 5 +++- .../ProcessAttendeesEmailRequestJob.php | 5 +++- ...AttendeesEmailRequestJobFailedHookTest.php | 25 +++++++++++++++++++ ...sSpeakersEmailRequestJobFailedHookTest.php | 25 +++++++++++++++++++ 4 files changed, 58 insertions(+), 2 deletions(-) diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index 617311258..3f1dfe9b0 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -216,6 +216,9 @@ private function redactFilterFieldNames($filter): array // scalar, so the redaction accepts the same shape instead of dropping the field names. if (!is_array($filter)) $filter = [$filter]; $conditions = array_filter($filter, 'is_scalar'); - return array_values(array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $conditions)); + // Cut at the first character of ANY operator FilterParser::filterExpresion recognizes + // (==, =@, @@, <>, <, >, <=, >=, [] range, () set) - [ and ( included, or a range/set + // condition's operands are logged verbatim. + return array_values(array_map(fn($condition) => preg_replace('/[=<>@!\[(].*/', '', (string)$condition), $conditions)); } } diff --git a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php index b34fe3efe..752be774d 100644 --- a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php +++ b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php @@ -185,6 +185,9 @@ private function redactFilterFieldNames($filter): array // scalar, so the redaction accepts the same shape instead of dropping the field names. if (!is_array($filter)) $filter = [$filter]; $conditions = array_filter($filter, 'is_scalar'); - return array_values(array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $conditions)); + // Cut at the first character of ANY operator FilterParser::filterExpresion recognizes + // (==, =@, @@, <>, <, >, <=, >=, [] range, () set) - [ and ( included, or a range/set + // condition's operands are logged verbatim. + return array_values(array_map(fn($condition) => preg_replace('/[=<>@!\[(].*/', '', (string)$condition), $conditions)); } } diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php index a01569da3..22f3adc96 100644 --- a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -208,4 +208,29 @@ public function testFailedChunkLogsFilterFieldNamesForAScalarFilterToo(): void && !str_contains($message, 'someone-private@example.com')) ->once(); } + + /** + * FilterParser::filterExpresion also recognizes the range ([]) and set (()) operators, and + * summit_hall_checked_in_date accepts [] (IAttendeeEmailFilterFields::OPERATORS). The + * redaction must cut at those operators too, or the range bounds are logged verbatim. + */ + public function testFailedChunkLogsFilterFieldNamesButNotRangeOperatorValues(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessAttendeesEmailRequestJob(self::$summit, [ + 'email_flow_event' => 'SUMMIT_REGISTRATION_GENERIC_ATTENDEE_EMAIL', + 'attendees_ids' => [1], + ], ['summit_hall_checked_in_date[]1700000000&&1700003600', 'tags_id()7||9']); + + $job->failed(new \RuntimeException('boom')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'filter fields ["summit_hall_checked_in_date","tags_id"]') + && !str_contains($message, '1700000000') + && !str_contains($message, '7||9')) + ->once(); + } } diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index 69bb75f40..78190d9a7 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -246,4 +246,29 @@ public function testFailedChunkLogsFilterFieldNamesForAScalarFilterToo(): void && !str_contains($message, 'someone-private@example.com')) ->once(); } + + /** + * FilterParser::filterExpresion also recognizes the range ([]) and set (()) operators. The + * redaction runs on the raw request filter, before any whitelist check, so it must cut at + * those operators too or the operands are logged verbatim. + */ + public function testFailedChunkLogsFilterFieldNamesButNotRangeOperatorValues(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [1], + ], ['created[]1700000000&&1700003600', 'id()7||9']); + + $job->failed(new \RuntimeException('boom')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'filter fields ["created","id"]') + && !str_contains($message, '1700000000') + && !str_contains($message, '7||9')) + ->once(); + } } From d0438937a869db337101247780dec3aa6cabfa79 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 11:07:58 -0300 Subject: [PATCH 16/19] fix(attendees): build the email strategy from the attendee's managed summit AttendeeService::send built each attendee's email strategy with the root Summit that ParametrizedSendEmails::_sendEmails fetches once, outside the per-attendee transaction. After any attendee's transaction failed (a transient queue push error, a retryable DB error), DoctrineTransactionService cleared or replaced the EntityManager and that Summit became detached, so every later attendee's sent-proof failed at flush with "A new entity was found through the relationship 'SummitAttendeeAnnouncementEmail#summit'" - after its email had already been dispatched. A retried chunk then re-emailed everyone processed after the failure while the excerpt reported them as sent. Build the strategy with $attendee->getSummit(), the managed association of the attendee this transaction just loaded (same id, enforced by the summit guard right above), so the proof always references a live entity of the current EntityManager. Regression test: one failing dispatch mid-chunk, every later attendee still gets exactly one proof. --- app/Services/Model/AttendeeService.php | 13 +++- tests/AttendeeServiceResumeSendEmailsTest.php | 69 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 879cddc19..6abb213e5 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -811,8 +811,19 @@ function return; } + // The strategy records the sent-proof against the Summit it is built with. + // $summit is the root entity _sendEmails fetched once, outside this + // transaction: after any earlier attendee's transaction failed, + // DoctrineTransactionService cleared (or replaced) the EntityManager and + // that instance is detached, so a proof referencing it fails at flush + // ("A new entity was found through the relationship ...#summit") AFTER + // the email was already dispatched - a retried chunk would then re-email + // everyone processed after the failure. $attendee->getSummit() is the + // managed association of the attendee this transaction just loaded (same + // id, the guard above enforces it), so the proof always references a live + // entity of the current EntityManager. $emailActionsStrategyFactory = new EmailActionsStrategyFactory(); - $strategy = $emailActionsStrategyFactory->build($summit, $flow_event); + $strategy = $emailActionsStrategyFactory->build($attendee->getSummit(), $flow_event); if ($strategy != null) { $strategy->process($attendee, $test_email_recipient, $onDispatchSuccess, $onDispatchInfo, $onDispatchError, $resume_since); } diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php index fd150369a..558e4684f 100644 --- a/tests/AttendeeServiceResumeSendEmailsTest.php +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -19,6 +19,7 @@ use App\Services\utils\IEmailExcerptService; use App\Services\Utils\Facades\EmailExcerpt; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\Registry; use models\summit\SummitAttendeeAnnouncementEmail; @@ -299,4 +300,72 @@ public function testResumedRunSkipsOnlyTicketWithProofSinceDispatchNotOthers(): ); } } + + const SimulatedDispatchFailure = 'simulated transient queue push failure'; + + /** + * @return int[] every attendee of the fixture summit, in the order send() will process them + */ + private function fixtureAttendeeIds(): array + { + $ids = []; + foreach (self::$summit->getAttendees() as $attendee) { + $ids[] = $attendee->getId(); + } + return $ids; + } + + /** + * Fails the FIRST GenericSummitAttendeeEmail push with a transient queue error and lets every + * later push succeed. The strategies dispatch mail jobs with a bare ::dispatch(), so the + * failure surfaces inside that attendee's own send() transaction - exactly where a redis + * outage would surface in production. + * + * @param array $dispatched receives the class name of every job dispatched, in order + */ + private function givenTheFirstGenericEmailDispatchFails(array &$dispatched): void + { + Bus::shouldReceive('dispatch')->andReturnUsing(function ($job) use (&$dispatched) { + $dispatched[] = get_class($job); + $generic = count(array_filter($dispatched, fn($class) => $class === GenericSummitAttendeeEmail::class)); + if ($job instanceof GenericSummitAttendeeEmail && $generic === 1) { + throw new \RuntimeException(self::SimulatedDispatchFailure); + } + return null; + }); + } + + /** + * Regression: the strategy used to be built with the root Summit that _sendEmails fetches once, + * outside the per-attendee transaction. After any attendee's transaction failed, + * DoctrineTransactionService cleared the EntityManager, that Summit became detached, and every + * later attendee's proof failed at flush ("A new entity was found through the relationship + * ...#summit") AFTER its email had already been dispatched - so a retried chunk re-emailed + * everyone processed after the failure while the excerpt reported them as sent. + */ + public function testAFailingAttendeeDoesNotPreventProofRecordingForTheRestOfTheChunk(): void + { + $dispatched = []; + $this->givenTheFirstGenericEmailDispatchFails($dispatched); + + $ids = $this->fixtureAttendeeIds(); + $this->assertGreaterThanOrEqual(2, count($ids), 'fixture must seed at least 2 attendees on the summit'); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => $ids, + ]); + + $generic_dispatches = count(array_filter($dispatched, fn($class) => $class === GenericSummitAttendeeEmail::class)); + $this->assertSame(count($ids), $generic_dispatches, 'every attendee must still be attempted after the failure'); + + $this->assertSame(0, $this->proofCount($ids[0], GenericSummitAttendeeEmail::EVENT_SLUG), 'the attendee whose dispatch failed must have no proof'); + foreach (array_slice($ids, 1) as $id) { + $this->assertSame( + 1, + $this->proofCount($id, GenericSummitAttendeeEmail::EVENT_SLUG), + sprintf('attendee %s, processed after the failure, must have exactly one proof', $id) + ); + } + } } From bff2b4fa73ab11de42e97ee36a727f572022cce8 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 11:09:42 -0300 Subject: [PATCH 17/19] fix(attendees): report a per-attendee send failure as an ERROR line in the excerpt AttendeeService::send caught any exception thrown while processing one attendee and only logged it, so the operator's outcome excerpt for a run that silently skipped an attendee read exactly like a clean one. The excerpt is the only signal the operator gets, and SpeakerService::send already routes the same failure to the ERROR callback. Call $onDispatchError with the exception message from that catch, matching the speaker path. Regression test: one failing dispatch mid-chunk produces exactly one ERROR line carrying the failure reason, while every other attendee is still reported as sent. --- app/Services/Model/AttendeeService.php | 5 +++ tests/AttendeeServiceResumeSendEmailsTest.php | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 6abb213e5..b0dee3d57 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -830,6 +830,11 @@ function }); } catch (\Exception $ex) { Log::warning($ex); + // Same as SpeakerService::send: an attendee this chunk could not process must + // show in the operator's outcome excerpt as an ERROR line, not only in the log, + // or a run that silently skipped an attendee reads exactly like a clean one. + if (!is_null($onDispatchError)) + $onDispatchError($ex->getMessage()); } }, function($summit, $outcome_email_recipient, $report){ diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php index 558e4684f..ff0e56ae3 100644 --- a/tests/AttendeeServiceResumeSendEmailsTest.php +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -368,4 +368,39 @@ public function testAFailingAttendeeDoesNotPreventProofRecordingForTheRestOfTheC ); } } + + /** + * An attendee this chunk could not process must show in the operator's outcome excerpt as an + * ERROR line: the excerpt is the only signal the operator gets, and SpeakerService::send + * already reports it that way. Previously the exception was only logged, so the excerpt of a + * run that silently skipped an attendee read exactly like a clean one. + */ + public function testAFailingAttendeeIsReportedAsAnErrorLineInTheExcerpt(): void + { + $dispatched = []; + $this->givenTheFirstGenericEmailDispatchFails($dispatched); + + $ids = $this->fixtureAttendeeIds(); + $this->assertGreaterThanOrEqual(2, count($ids), 'fixture must seed at least 2 attendees on the summit'); + + $this->service()->send(self::$summit->getId(), [ + 'email_flow_event' => GenericSummitAttendeeEmail::EVENT_SLUG, + 'attendees_ids' => $ids, + ]); + + $report = EmailExcerpt::getReport(); + + $errorLines = array_values(array_filter( + $report, + fn($line) => ($line['type'] ?? null) === IEmailExcerptService::ErrorType + )); + $this->assertCount(1, $errorLines, 'exactly one ERROR line, for the attendee whose dispatch failed'); + $this->assertStringContainsString(self::SimulatedDispatchFailure, $errorLines[0]['message'], 'the ERROR line must carry the failure reason'); + + $emailLines = array_filter( + $report, + fn($line) => ($line['type'] ?? null) === IEmailExcerptService::EmailLineType + ); + $this->assertCount(count($ids) - 1, $emailLines, 'every other attendee must still be reported as sent'); + } } From df5dc7ea76d47810d1d3898794b12f9ca8863707 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 11:37:08 -0300 Subject: [PATCH 18/19] test(attendees): seed the email flow types the resume and failed-hook tests need AttendeeServiceResumeSendEmailsTest and ProcessAttendeesEmailRequestJobFailedHookTest extended Tests\TestCase, the plain Laravel base that runs no seeder. Every mail job they push resolves its template through Summit::getEmailIdentifierPerEmailEventFlowSlug, which reads SummitEmailEventFlowType - a table that is empty on a database built from initial_schema.sql plus migrations, as CI does: the migrations that seed those types are pre-marked in initial_migrations.sql or return early without their parent flow. The job constructor then threw "missing template_identifier value", the catch swallowed it, and CI reported "pushed 0 times" / no proof written in 5 tests while the same shard passed locally against an already-seeded database. Extend ProtectedApiTestCase instead, as the four speaker equivalents do: its BrowserKitTestCase base runs SummitEmailFlowTypeSeeder once per process. It also inserts and clears the member fixture itself, so the classes' own member fixture calls are dropped. Validated against a fresh model database created the way the CI job creates it. --- tests/AttendeeServiceResumeSendEmailsTest.php | 7 +------ tests/ProcessAttendeesEmailRequestJobFailedHookTest.php | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/tests/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php index ff0e56ae3..5954d5444 100644 --- a/tests/AttendeeServiceResumeSendEmailsTest.php +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -14,7 +14,6 @@ use App\Jobs\Emails\Registration\Attendees\GenericSummitAttendeeEmail; use App\Jobs\Emails\SummitAttendeeTicketRegenerateHashEmail; -use App\Models\Foundation\Main\IGroup; use App\Services\Model\IAttendeeService; use App\Services\utils\IEmailExcerptService; use App\Services\Utils\Facades\EmailExcerpt; @@ -36,17 +35,14 @@ * * Class AttendeeServiceResumeSendEmailsTest */ -final class AttendeeServiceResumeSendEmailsTest extends TestCase +final class AttendeeServiceResumeSendEmailsTest extends ProtectedApiTestCase { use InsertSummitTestData; - use InsertMemberTestData; - protected function setUp(): void { parent::setUp(); - self::insertMemberTestData(IGroup::TrackChairs); self::$defaultMember = self::$member; self::insertSummitTestData(); } @@ -54,7 +50,6 @@ protected function setUp(): void protected function tearDown(): void { self::clearSummitTestData(); - self::clearMemberTestData(); parent::tearDown(); } diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php index 22f3adc96..09dc073b9 100644 --- a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -15,7 +15,6 @@ use App\Jobs\Emails\IMailTemplatesConstants; use App\Jobs\Emails\ProcessAttendeesEmailRequestJob; use App\Jobs\Emails\Registration\Attendees\SummitAttendeeExcerptEmail; -use App\Models\Foundation\Main\IGroup; use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; @@ -36,16 +35,13 @@ * * Class ProcessAttendeesEmailRequestJobFailedHookTest */ -final class ProcessAttendeesEmailRequestJobFailedHookTest extends TestCase +final class ProcessAttendeesEmailRequestJobFailedHookTest extends ProtectedApiTestCase { use InsertSummitTestData; - use InsertMemberTestData; - protected function setUp(): void { parent::setUp(); - self::insertMemberTestData(IGroup::TrackChairs); self::$defaultMember = self::$member; self::insertSummitTestData(); } @@ -53,7 +49,6 @@ protected function setUp(): void protected function tearDown(): void { self::clearSummitTestData(); - self::clearMemberTestData(); parent::tearDown(); } From 874fa87bb17ae1f530df9f9ac694f6637c713eb2 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 9 Sep 2026 12:14:33 -0300 Subject: [PATCH 19/19] fix(speakers): resolve the summit inside each per-speaker send transaction SpeakerService::sendEmails built each speaker's email strategy, resume check, promo code strategy and assistance with the root Summit that ParametrizedSendEmails::_sendEmails fetches once, outside the per-speaker transaction. After any speaker's transaction failed - a speaker id that no longer exists throws EntityNotFoundException right there; a queue push or a retryable DB error can throw too - DoctrineTransactionService cleared or replaced the EntityManager and that Summit became detached, so every later speaker's sent-proof (and any promo code or assistance generated for it) failed at flush with "A new entity was found through the relationship 'SpeakerAnnouncementSummitEmail#summit'" after its email had already been dispatched. A retried chunk then re-emailed everyone processed after the failure. Re-resolve the summit from the current EntityManager at the top of the transaction: an identity-map hit on the normal path, one query only after a clear. Same defect and fix as the attendee path (d0438937a); speakers have no owning summit to read it from, hence the repository lookup. Regression test: a missing speaker id first in the chunk, the next speaker still gets its email and exactly one proof. --- app/Services/Model/Imp/SpeakerService.php | 16 ++++++++ tests/SpeakerServiceResumeSendEmailsTest.php | 42 ++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index e10a927d0..efdd468af 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -1393,6 +1393,22 @@ function $onDispatchInfo, $payload ) { + // $summit is the root entity _sendEmails fetched once, outside this + // transaction. After an earlier speaker's transaction failed (a missing + // speaker id throws EntityNotFoundException below; a queue push or a + // retryable DB error can throw too), DoctrineTransactionService cleared or + // replaced the EntityManager and that instance is detached: every write + // below that references it - the sent-proof, an auto-generated promo code, + // a speaker assistance - would then fail at flush ("A new entity was found + // through the relationship ...#summit") AFTER the email was dispatched, and + // a retried chunk would re-email everyone processed after the failure. + // Re-resolve it from the current EntityManager: an identity-map hit on the + // normal path, one query only after a clear. + $summit = $this->summit_repository->getById($summit->getId()); + if (!$summit instanceof Summit) { + throw new EntityNotFoundException('Summit not found'); + } + $email_strategy = new SpeakerActionsEmailStrategy($summit, $flow_event); Log::debug(sprintf("SpeakerService::send processing speaker id %s payload %s", $speaker_id, json_encode($payload))); diff --git a/tests/SpeakerServiceResumeSendEmailsTest.php b/tests/SpeakerServiceResumeSendEmailsTest.php index 21336a944..515fe9bb9 100644 --- a/tests/SpeakerServiceResumeSendEmailsTest.php +++ b/tests/SpeakerServiceResumeSendEmailsTest.php @@ -49,6 +49,9 @@ class SpeakerServiceResumeSendEmailsTest extends ProtectedApiTestCase private const FLOW_EVENT = PresentationSpeakerSelectionProcessAcceptedAlternateEmail::EVENT_SLUG; private const EMAIL_TYPE = SpeakerAnnouncementSummitEmail::TypeAcceptedAlternate; + // max signed INT: PresentationSpeaker.ID is an auto-increment INT, so no row can carry it + private const MissingSpeakerId = 2147483647; + protected function setUp(): void { parent::setUp(); @@ -383,4 +386,43 @@ public function testResumeCheckSkipsBeforeGeneratingSpeakerAssistance(): void 'generateSpeakerAssistance() must not run for the resume-skipped speaker - the return happens before it, same as before getPromoCode()' ); } + + private function proofCount(PresentationSpeaker $speaker): int + { + return (int) self::$em->createQueryBuilder() + ->select('COUNT(p.id)') + ->from(SpeakerAnnouncementSummitEmail::class, 'p') + ->where('p.speaker = :speaker_id') + ->andWhere('p.summit = :summit_id') + ->andWhere('p.type = :type') + ->setParameter('speaker_id', $speaker->getId()) + ->setParameter('summit_id', self::$summit->getId()) + ->setParameter('type', self::EMAIL_TYPE) + ->getQuery()->getSingleScalarResult(); + } + + /** + * Regression: the per-speaker transaction built its strategy, resume check, promo code + * strategy and assistance with the root Summit that _sendEmails fetches once, outside the + * transaction. After any speaker's transaction failed - a speaker id that no longer exists + * throws EntityNotFoundException right there - DoctrineTransactionService cleared the + * EntityManager, that Summit became detached, and every later speaker's proof failed at flush + * ("A new entity was found through the relationship ...#summit") AFTER its email had already + * been dispatched, so a retried chunk re-emailed everyone processed after the failure. + */ + public function testAFailingSpeakerDoesNotPreventProofRecordingForTheRestOfTheChunk(): void + { + Queue::fake(); + + $speaker = $this->newFixtureSpeaker('after-failure'); + self::$em->flush(); + + $this->service()->sendEmails(self::$summit->getId(), [ + 'email_flow_event' => self::FLOW_EVENT, + 'speaker_ids' => [self::MissingSpeakerId, $speaker->getId()], + ], null); + + $this->assertSame(1, $this->proofCount($speaker), 'the speaker processed after the failure must have exactly one proof'); + Queue::assertPushed(PresentationSpeakerSelectionProcessAcceptedAlternateEmail::class, 1); + } }