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/" } 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/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index e35381f4c..3f1dfe9b0 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -210,7 +210,15 @@ 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'); + // 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 018fd95d2..752be774d 100644 --- a/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php +++ b/app/Jobs/Emails/Registration/Attendees/ProcessAttendeesEmailRequestJob.php @@ -11,14 +11,22 @@ * 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; /** * Class ProcessAttendeesEmailRequestJob @@ -26,11 +34,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,44 +70,124 @@ 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); } + /** + * 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)) 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'); + // 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/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php b/app/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.php index 9fd768724..913341e66 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 \Doctrine\Common\Collections\Collection + */ + #[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/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/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/app/Services/Model/AttendeeService.php b/app/Services/Model/AttendeeService.php index 445c8121f..b0dee3d57 100644 --- a/app/Services/Model/AttendeeService.php +++ b/app/Services/Model/AttendeeService.php @@ -14,12 +14,15 @@ 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 Doctrine\DBAL\TransactionIsolationLevel; 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 +45,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 +610,117 @@ 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', 200)); + + 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())); + // 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'])) { + $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; @@ -638,9 +753,15 @@ 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, - $onDispatchError) use ($payload) { + $onDispatchError, + $onDispatchInfo) use ($payload) { try { + $resume_since = $payload['resume_since'] ?? null; $this->tx_service->transaction(function () use ( $summit, $flow_event, @@ -649,6 +770,8 @@ function $filter, $onDispatchSuccess, $onDispatchError, + $onDispatchInfo, + $resume_since, $payload ) { Log::debug(sprintf("AttendeeService::send processing attendee id %s", $attendee_id)); @@ -657,14 +780,61 @@ 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; + } + + // 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($flow_event); + $strategy = $emailActionsStrategyFactory->build($attendee->getSummit(), $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) { 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/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/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index 216407a28..efdd468af 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'])) { @@ -1386,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/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/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/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/AttendeeServiceBulkSendChunkingTest.php b/tests/AttendeeServiceBulkSendChunkingTest.php new file mode 100644 index 000000000..80f362a73 --- /dev/null +++ b/tests/AttendeeServiceBulkSendChunkingTest.php @@ -0,0 +1,321 @@ + '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 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 + + $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->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 + { + // 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); + } + + /** + * 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/AttendeeServiceResumeSendEmailsTest.php b/tests/AttendeeServiceResumeSendEmailsTest.php new file mode 100644 index 000000000..5954d5444 --- /dev/null +++ b/tests/AttendeeServiceResumeSendEmailsTest.php @@ -0,0 +1,401 @@ +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)); + } + + /** + * 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'); + } + + /** + * 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(); + + $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]) + ); + } + } + + 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) + ); + } + } + + /** + * 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'); + } +} 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) + ) + ); + } +} diff --git a/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php new file mode 100644 index 000000000..09dc073b9 --- /dev/null +++ b/tests/ProcessAttendeesEmailRequestJobFailedHookTest.php @@ -0,0 +1,231 @@ + 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 ProtectedApiTestCase +{ + use InsertSummitTestData; + + protected function setUp(): void + { + parent::setUp(); + self::$defaultMember = self::$member; + self::insertSummitTestData(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + 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 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(); + 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(); + } + + /** + * 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(); + } + + /** + * 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/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' + ); + } +} diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index a9347e8c0..78190d9a7 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -221,4 +221,54 @@ 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(); + } + + /** + * 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(); + } } 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'); + } } 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); + } } 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)); + } +} 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'); + } +}