diff --git a/.env.example b/.env.example index 8d4fa1b52..5e3de2850 100644 --- a/.env.example +++ b/.env.example @@ -34,6 +34,7 @@ SESSION_COOKIE_SECURE=false QUEUE_DRIVER=database QUEUE_CONN= QUEUE_DATABASE= +DB_QUEUE_RETRY_AFTER=1800 MAIL_DRIVER=sendgrid SENDGRID_API_KEY='YOUR_SENDGRID_API_KEY' diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php index 4fb0c181e..d47d7b9a1 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php @@ -303,54 +303,10 @@ public function getSpeakers($summit_id) return $this->_getAll( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]; + return \services\model\ISpeakerFilterFields::OPERATORS; }, function () { - return [ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::VALIDATION_RULES; }, function () { return [ @@ -443,55 +399,11 @@ public function getSpeakersActivitiesCount($summit_id) $filter = null; if (Request::has('filter')) { - $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]); + $filter = FilterParser::parse(Request::input('filter'), \services\model\ISpeakerFilterFields::OPERATORS); } if (!is_null($filter)) { - $filter->validate([ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]); + $filter->validate(\services\model\ISpeakerFilterFields::VALIDATION_RULES); } $count = $this->speaker_repository->getUniqueActivitiesCountBySummit($summit, $filter); @@ -584,54 +496,10 @@ public function getSpeakersCSV($summit_id) return $this->_getAllCSV( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]; + return \services\model\ISpeakerFilterFields::OPERATORS; }, function () { - return [ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::VALIDATION_RULES; }, function () { return [ @@ -865,7 +733,7 @@ function ($page, $per_page, $filter, $order, $applyExtraFilters) use ($summit) { ), new OA\Parameter( name: 'filter', - description: 'Filter by id, first_name, last_name, email, full_name, member_id', + description: 'Filter by id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id. This endpoint is not summit-scoped, so presentation-related filter fields (available on the per-summit speaker endpoints) do not apply here.', in: 'query', required: false, schema: new OA\Schema(type: 'string') @@ -912,29 +780,12 @@ public function getAll() { return $this->_getAll( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '==','@@'], - 'last_name' => ['=@', '==','@@'], - 'email' => ['=@', '==','@@'], - 'full_name' => ['=@', '==','@@'], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - ]; + // getAll() is summit-independent - only the GLOBAL_* subset is safe here, + // see GLOBAL_OPERATORS' docblock on the shared filter-fields interface for why. + return \services\model\ISpeakerFilterFields::GLOBAL_OPERATORS; }, function () { - return [ - - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::GLOBAL_VALIDATION_RULES; }, function () { return [ @@ -3203,7 +3054,7 @@ public function deleteSpeakerBigPhoto($speaker_id) ), new OA\Parameter( name: 'filter', - description: 'Filter speakers by id, first_name, last_name, email, full_name, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_track_group_id, presentations_selection_plan_id, presentations_type_id, presentations_title, presentations_abstract, presentations_submitter_full_name, presentations_submitter_email, has_media_upload_with_type, has_not_media_upload_with_type', + description: 'Filter speakers by id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_track_group_id, presentations_selection_plan_id, presentations_type_id, presentations_title, presentations_abstract, presentations_submitter_full_name, presentations_submitter_email, has_media_upload_with_type, has_not_media_upload_with_type', in: 'query', required: false, schema: new OA\Schema(type: 'string') @@ -3259,53 +3110,13 @@ public function send($summit_id) $filter = null; if (Request::has('filter')) { - $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]); + $filter = FilterParser::parse(Request::input('filter'), \services\model\ISpeakerFilterFields::OPERATORS); } if (is_null($filter)) $filter = new Filter(); - $filter->validate([ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]); + $filter->validate(\services\model\ISpeakerFilterFields::VALIDATION_RULES); $this->service->triggerSendEmails($summit, $payload, Request::input('filter')); diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index 9df2b20aa..0510e85a5 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -11,13 +11,19 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessExcerptEmail; +use App\Jobs\Utils\JobDispatcher; +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\ISpeakerService; use utils\FilterParser; /** @@ -79,28 +85,121 @@ public function handle ) ); - $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]) : null; + $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, \services\model\ISpeakerFilterFields::OPERATORS) : null; $service->sendEmails($this->summit_id, $this->payload, $filter); } + + /** + * Invoked by the queue worker once this job is marked failed. With tries = 1 that includes a + * chunk whose worker was killed mid-run: the job sits reserved until the connection's + * retry_after elapses, is re-served, and is failed without re-running. Nothing else reports + * that loss - the outcome excerpt is only sent when sendEmails() runs to completion - so + * without this hook a dead chunk leaves no trace beyond a queue_failed_jobs row. + * + * Log the chunk's speaker 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. The chunk is processed one speaker + * per transaction, so a worker killed mid-run has already e-mailed (and written the "already + * sent" proof for) the speakers before the kill: the ids are an upper bound on what was lost, + * not confirmed misses, and both messages say so and tell the operator to re-send with + * should_resend=false so the resend guard skips the speakers that already have a proof. The + * excerpt goes through JobDispatcher::withDbFallback (primary, then the database queue, then + * an inline run) and is best-effort: a failure there must not mask the original failure. + * + * @param \Throwable $e + */ + public function failed(\Throwable $e): void + { + $speaker_ids = $this->payload['speaker_ids'] ?? []; + $flow_event = $this->payload['email_flow_event'] ?? ''; + $ids_list = implode(', ', $speaker_ids); + + $resend_hint = "Re-send these ids with should_resend=false so the speakers already e-mailed are skipped"; + if (isset($this->payload['promo_code_spec'])) { + // AutomaticMultiSpeakerPromoCodeStrategy::getPromoCode() generates a fresh code on every + // call, before the resend guard runs, so should_resend=false does not prevent this one. + $resend_hint .= "; this send auto-generates promo codes and a re-send creates a new code for every speaker in the list, including the ones already e-mailed"; + } + + Log::error + ( + sprintf + ( + "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) failed (%s: %s); up to %s of them may not have been processed. Speaker ids in the chunk: [%s] filter fields %s. %s.", + $this->summit_id, + $flow_event, + count($speaker_ids), + get_class($e), + $e->getMessage(), + count($speaker_ids), + $ids_list, + json_encode($this->redactFilterFieldNames($this->filter)), + $resend_hint + ) + ); + + $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("ProcessSpeakersEmailRequestJob::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 speaker(s) failed (%s); up to %s of them may not have been processed. Speaker ids in the chunk: %s. %s", + count($speaker_ids), + $e->getMessage(), + count($speaker_ids), + $ids_list, + $resend_hint + ), + ], + [ + '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 (SpeakerService::triggerSendEmails): 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 PresentationSpeakerSelectionProcessExcerptEmail($summit, $outcome_email_recipient, $report), + logContext: ['summit_id' => $this->summit_id, 'speaker_count' => count($speaker_ids)], + primaryConnection: Config::get('queue.default') + ); + } + catch (\Throwable $ex) { + Log::error($ex); + } + } + + /** + * Reduces a raw filter (["email==foo@bar.com", "presentations_selection_plan_id==78"]) to + * just its field names (["email", "presentations_selection_plan_id"]) so error logs never + * carry filter values that may be PII - email and full_name are valid filter fields + * (ISpeakerFilterFields::OPERATORS). + * + * @param mixed $filter + * @return string[] + */ + private function redactFilterFieldNames($filter): array + { + if (empty($filter) || !is_array($filter)) return []; + return array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $filter); + } } diff --git a/app/Services/Model/ISpeakerFilterFields.php b/app/Services/Model/ISpeakerFilterFields.php new file mode 100644 index 000000000..ce4d43f52 --- /dev/null +++ b/app/Services/Model/ISpeakerFilterFields.php @@ -0,0 +1,115 @@ + ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '@@', '=='], + 'last_name' => ['=@', '@@', '=='], + 'email' => ['=@', '@@', '=='], + 'full_name' => ['=@', '@@', '=='], + 'member_id' => ['=='], + 'member_user_external_id' => ['=='], + 'has_accepted_presentations' => ['=='], + 'has_alternate_presentations' => ['=='], + 'has_rejected_presentations' => ['=='], + 'presentations_track_id' => ['=='], + 'presentations_track_group_id' => ['=='], + 'presentations_selection_plan_id' => ['=='], + 'presentations_type_id' => ['=='], + 'presentations_title' => ['=@', '@@', '=='], + 'presentations_abstract' => ['=@', '@@', '=='], + 'presentations_submitter_full_name' => ['=@', '@@', '=='], + 'presentations_submitter_email' => ['=@', '@@', '=='], + 'has_media_upload_with_type' => ['=='], + 'has_not_media_upload_with_type' => ['=='], + ]; + + const VALIDATION_RULES = [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'email' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'member_user_external_id' => 'sometimes|integer', + 'has_accepted_presentations' => 'sometimes|string|in:true,false', + 'has_alternate_presentations' => 'sometimes|string|in:true,false', + 'has_rejected_presentations' => 'sometimes|string|in:true,false', + 'presentations_track_id' => 'sometimes|integer', + 'presentations_track_group_id' => 'sometimes|integer', + 'presentations_selection_plan_id' => 'sometimes|integer', + 'presentations_type_id' => 'sometimes|integer', + 'presentations_title' => 'sometimes|string', + 'presentations_abstract' => 'sometimes|string', + 'presentations_submitter_full_name' => 'sometimes|string', + 'presentations_submitter_email' => 'sometimes|string', + 'has_media_upload_with_type' => 'sometimes|integer', + 'has_not_media_upload_with_type' => 'sometimes|integer', + ]; + + /** + * The subset of OPERATORS safe for a summit-independent query (e.g. + * OAuth2SummitSpeakersApiController::getAll(), which uses + * DoctrineSpeakerRepository::getAllByPage() / getAllIdsByPage()). + * + * The 13 fields excluded here (has_accepted/alternate/rejected_presentations, + * every presentations_* field, has_(not_)media_upload_with_type) all hard-code a + * ":summit" bound parameter inside DoctrineSpeakerRepository::getFilterMappings()'s + * DQL templates (e.g. "__p41_:i.summit = :summit"), because that method is shared + * verbatim by the summit-scoped query methods (getSpeakersBySummit, + * getSpeakersIdsBySummit), which bind that parameter on their own base query. + * getAllByPage()/getAllIdsByPage() never bind it - there is no single summit to + * scope by on a global listing - so applying any of those 13 fields there throws + * Doctrine\ORM\Query\QueryException ("too few parameters"), not a silent no-op. + * Verified by reproducing the exception directly against getAllByPage() with a + * presentations_track_id filter. Removing the hard-coded :summit clause from those + * DQL templates would need it to become conditional depending on caller context - + * real repository work, out of scope for a filter-whitelist unification. + */ + const GLOBAL_OPERATORS = [ + 'id' => ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '@@', '=='], + 'last_name' => ['=@', '@@', '=='], + 'email' => ['=@', '@@', '=='], + 'full_name' => ['=@', '@@', '=='], + 'member_id' => ['=='], + 'member_user_external_id' => ['=='], + ]; + + const GLOBAL_VALIDATION_RULES = [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'email' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'member_user_external_id' => 'sometimes|integer', + ]; +} diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index 42166a44b..e47018633 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -18,6 +18,7 @@ use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionRejectedEmail; use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionRequestedEmail; use App\Jobs\Emails\ProcessSpeakersEmailRequestJob; +use App\Jobs\Utils\JobDispatcher; use App\Jobs\Emails\Registration\PromoCodes\PromoCodeEmailFactory; use App\Models\Foundation\Main\CountryCodes; use App\Models\Foundation\Main\Repositories\ILanguageRepository; @@ -34,6 +35,7 @@ use App\Services\Model\Strategies\EmailActions\SpeakerActionsEmailStrategy; use App\Services\Model\Strategies\PromoCodes\IPromoCodeStrategyFactory; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -56,6 +58,7 @@ use models\summit\Summit; use utils\Filter; use utils\FilterParser; +use utils\PagingInfo; /** * Class SpeakerService @@ -1227,13 +1230,110 @@ public function deleteSpeakerBigPhoto($speaker_id): void }); } + /** * @inheritDoc */ public function triggerSendEmails(Summit $summit, array $payload, $filter = null): void { - Log::debug(sprintf("SpeakerService::triggerSendEmails summit %s payload %s", $summit->getId(), json_encode($payload))); - ProcessSpeakersEmailRequestJob::dispatch($summit->getId(), $payload, $filter); + $process_db_chunk_size = intval(Config::get('emails.speakers_process_db_chunk_size', 500)); + $process_jon_chunk_size = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + + Log::debug + ( + sprintf + ( + "SpeakerService::triggerSendEmails summit %s email_flow_event %s speaker_ids_count %s has_filter %s process_db_chunk_size %s process_jon_chunk_size %s", + $summit->getId(), + $payload['email_flow_event'] ?? '', + isset($payload['speaker_ids']) ? count($payload['speaker_ids']) : 0, + !is_null($filter) ? 'yes' : 'no', + $process_db_chunk_size, + $process_jon_chunk_size + ) + ); + + if (isset($payload['speaker_ids'])) { + $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); + } + + if (isset($payload['excluded_speaker_ids'])) { + $ids = array_diff($ids, $payload['excluded_speaker_ids']); + } + + $ids = array_values(array_unique($ids)); + + if (empty($ids)) { + Log::debug(sprintf("SpeakerService::triggerSendEmails summit %s no speakers matched, nothing dispatched", $summit->getId())); + return; + } + + Log::debug(sprintf("SpeakerService::triggerSendEmail got %s speakers 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 (should_resend + // defaults to true and the admin UI never sends it, so today's re-runs are not + // deduplicated). withDbFallback() fails over to the database queue (and runs sync on a + // double failure) so the loop completes. Same pattern as + // PresentationSubmissionReopenService::notify's per-recipient dispatch loop. + $chunk_nbr = 1; + foreach (array_chunk($ids, $process_jon_chunk_size) as $chunk) { + $chunkPayload = $payload; + $chunkPayload['speaker_ids'] = $chunk; + unset($chunkPayload['excluded_speaker_ids']); + + Log::debug + ( + sprintf + ( + "SpeakerService::triggerSendEmails sending summit id %s chunk %s speakers count %s", + $summit->getId(), + $chunk_nbr, + count($chunk) + ) + ); + + try { + JobDispatcher::withDbFallback( + job: new ProcessSpeakersEmailRequestJob($summit->getId(), $chunkPayload, $filter), + logContext: ['summit_id' => $summit->getId(), 'speaker_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 + ( + "SpeakerService::triggerSendEmails summit %s: chunk of %s speaker(s) failed every dispatch tier (%s: %s). Unprocessed speaker ids: [%s]", + $summit->getId(), + count($chunk), + get_class($ex), + $ex->getMessage(), + implode(', ', $chunk) + ), + ['summit_id' => $summit->getId(), 'speaker_ids' => $chunk, 'exception' => $ex] + ); + } + } } /** @@ -1316,26 +1416,7 @@ function ); $original_filter = count($original_filter) > 0 ? - FilterParser::parse($original_filter, [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]) : null; + FilterParser::parse($original_filter, ISpeakerFilterFields::OPERATORS) : null; } catch (\Exception $ex){ Log::warning($ex); diff --git a/config/emails.php b/config/emails.php new file mode 100644 index 000000000..5db39bb72 --- /dev/null +++ b/config/emails.php @@ -0,0 +1,20 @@ + 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) +]; diff --git a/config/queue.php b/config/queue.php index 6a175b9bb..abbe0b127 100644 --- a/config/queue.php +++ b/config/queue.php @@ -43,7 +43,10 @@ 'driver' => 'database', 'table' => 'queue_jobs', 'queue' => 'default', - 'expire' => 60, + // Same window as the redis primary: this is the failover tier for the same jobs + // (JobDispatcher::withDbFallback), so a reserved job must not be re-served before + // a chunk-sized job can finish. Laravel 12 ignores the old 'expire' key. + 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 1800), ], 'redis' => [ diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php new file mode 100644 index 000000000..cfdfdc781 --- /dev/null +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -0,0 +1,220 @@ + failed() plumbing can't be driven end to end without a real queue + * connection. That plumbing is Laravel's (Illuminate\Queue\Jobs\Job::fail() -> + * CallQueuedHandler::failed() -> $command->failed($e)); what this class pins is what OUR hook does. + * + * Class ProcessSpeakersEmailRequestJobFailedHookTest + */ +class ProcessSpeakersEmailRequestJobFailedHookTest extends ProtectedApiTestCase +{ + use InsertSummitTestData; + + protected function setUp(): void + { + parent::setUp(); + 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 ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22, 33], + 'outcome_email_recipient' => 'outcome@example.com', + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::class, 1); + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::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 speaker id in the chunk'); + $this->assertStringContainsString('worker killed mid-chunk', $errorLines[0], 'the ERROR line must carry the failure reason'); + // The chunk runs one speaker per transaction, so a mid-run kill has already e-mailed + // part of it: the line must not present the ids as confirmed misses, and it must tell + // the operator how to re-send without duplicating those. + $this->assertStringContainsString('up to 3 of them may not have been processed', $errorLines[0]); + $this->assertStringContainsString('should_resend=false', $errorLines[0], 'the ERROR line must tell the operator to re-send with should_resend=false'); + $this->assertStringNotContainsString('promo code', $errorLines[0], 'no promo-code caveat when the send carries no promo_code_spec'); + $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') + && str_contains($message, 'should_resend=false')) + ->once(); + } + + public function testFailedChunkWithPromoCodeSpecWarnsThatAResendCreatesNewCodes(): void + { + // AutomaticMultiSpeakerPromoCodeStrategy::getPromoCode() generates a fresh code on every + // call and runs before the resend guard, so should_resend=false does not stop a re-send + // from handing a second code to the speakers already e-mailed. The operator has to know. + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22], + 'outcome_email_recipient' => 'outcome@example.com', + 'promo_code_spec' => ['class_name' => 'SPEAKERS_PROMO_CODE', 'type' => 'ACCEPTED'], + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::class, function ($excerpt) { + $lines = $this->jobProperty($excerpt, 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + $this->assertCount(1, $errorLines); + $this->assertStringContainsString('should_resend=false', $errorLines[0]); + $this->assertStringContainsString('auto-generates promo codes', $errorLines[0], 'a promo_code_spec send must warn that a re-send creates new codes'); + return true; + }); + } + + 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(PresentationSpeakerSelectionProcessExcerptEmail::class); + // Only the excerpt dispatches are scripted (primary throws, fallback captures); every + // other dispatch - the fixture teardown queues hundreds - 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 ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_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(PresentationSpeakerSelectionProcessExcerptEmail::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 speaker id in the chunk'); + } + + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [44, 55], + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertNothingPushed(); + + // Exactly one error line, and it names the ids. The "exactly one" half is what + // distinguishes "skipped the excerpt on purpose" from "tried to build it without a + // recipient and swallowed the resulting exception into a second error entry". + Log::shouldHaveReceived('error')->once(); + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) && str_contains($message, '44, 55')) + ->once(); + } + + public function testFailedChunkLogsFilterFieldNamesButNotTheirValues(): void + { + // email and full_name are valid speaker filter fields (ISpeakerFilterFields::OPERATORS), + // so the raw filter can carry PII. The error log must still say which fields were used + // (useful to reproduce the failed send) without writing the PII value itself. + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22], + ], ['email==foo@bar.com', 'presentations_selection_plan_id==78']); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'email') + && str_contains($message, 'presentations_selection_plan_id') + && !str_contains($message, 'foo@bar.com')) + ->once(); + } +} diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php new file mode 100644 index 000000000..77ee73a25 --- /dev/null +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -0,0 +1,374 @@ + 'ACCEPTED_ALTERNATE']; + } + + /** + * @return ProcessSpeakersEmailRequestJob[] + */ + private function pushedJobs(): array + { + $jobs = []; + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, function ($job) use (&$jobs) { + $jobs[] = $job; + return true; + }); + return $jobs; + } + + private function jobProperty(ProcessSpeakersEmailRequestJob $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.speakers_process_job_chunk_size', 200)); + $remainder = 50; + $ids = range(1, 2 * $chunkSize + $remainder); + $payload = $this->basePayload(); + $payload['speaker_ids'] = $ids; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 3); + + $jobs = $this->pushedJobs(); + $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $jobs); + + $this->assertCount($chunkSize, $slices[0]); + $this->assertCount($chunkSize, $slices[1]); + $this->assertCount($remainder, $slices[2]); + + $covered = array_merge(...$slices); + sort($covered); + $this->assertEquals($ids, $covered, 'the slices together must cover every matched id exactly once'); + } + + public function testDispatchesExactlyOneJobWhenMatchedCountEqualsChunkSize(): void + { + Queue::fake(); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + $ids = range(1, $chunkSize); + $payload = $this->basePayload(); + $payload['speaker_ids'] = $ids; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + } + + public function testDispatchesNothingWhenSpeakerIdsIsEmpty(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = []; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertNotPushed(ProcessSpeakersEmailRequestJob::class); + } + + public function testExcludedSpeakerIdsAreRemovedBeforeChunking(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = range(1, 10); + $payload['excluded_speaker_ids'] = [3, 7]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $slice = $this->jobProperty($jobs[0], 'payload')['speaker_ids']; + sort($slice); + + $this->assertEquals([1, 2, 4, 5, 6, 8, 9, 10], $slice); + $this->assertArrayNotHasKey( + 'excluded_speaker_ids', + $this->jobProperty($jobs[0], 'payload'), + 'excluded_speaker_ids must not be carried forward once already applied' + ); + } + + public function testDuplicateExplicitIdsAreDedupedBeforeDispatch(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = [5, 5, 6]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $slice = $this->jobProperty($jobs[0], 'payload')['speaker_ids']; + sort($slice); + + $this->assertEquals([5, 6], $slice, 'a duplicate id must only appear once across all dispatched jobs'); + } + + public function testExplicitFilterRawValueReachesEveryChunkUnchanged(): void + { + Queue::fake(); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + $payload = $this->basePayload(); + $payload['speaker_ids'] = range(1, $chunkSize + 1); + $rawFilter = ['first_name=@Test']; + + $this->service()->triggerSendEmails(self::$summit, $payload, $rawFilter); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 2); + foreach ($this->pushedJobs() as $job) { + $this->assertSame( + $rawFilter, + $this->jobProperty($job, 'filter'), + "every chunk's private filter constructor argument must be identical (===) to the raw filter triggerSendEmails received" + ); + } + } + + public function testChunkPayloadCarriesOtherPayloadKeysThrough(): void + { + Queue::fake(); + $payload = [ + 'email_flow_event' => 'ACCEPTED_ALTERNATE', + 'speaker_ids' => [1, 2, 3], + 'original_filter' => ['id==1||2||3'], + 'promo_code_spec' => ['class_name' => 'SPEAKERS_PROMO_CODE', 'type' => 'ACCEPTED'], + 'test_email_recipient' => 'test@example.com', + 'outcome_email_recipient' => 'outcome@example.com', + 'should_resend' => false, + 'should_send_copy_2_submitter' => true, + ]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertEquals($payload['original_filter'], $chunkPayload['original_filter']); + $this->assertEquals($payload['promo_code_spec'], $chunkPayload['promo_code_spec']); + $this->assertEquals($payload['test_email_recipient'], $chunkPayload['test_email_recipient']); + $this->assertEquals($payload['outcome_email_recipient'], $chunkPayload['outcome_email_recipient']); + $this->assertEquals($payload['should_resend'], $chunkPayload['should_resend']); + $this->assertEquals($payload['should_send_copy_2_submitter'], $chunkPayload['should_send_copy_2_submitter']); + $this->assertEquals($payload['email_flow_event'], $chunkPayload['email_flow_event']); + $this->assertEquals([1, 2, 3], $chunkPayload['speaker_ids']); + } + + public function testFilterBasedSelectionResolvesRealMatchingSpeakersAndChunks(): void + { + Queue::fake(); + + // self::$defaultSpeaker already has 20 presentations attached to self::$summit + // (via InsertSummitTestData), so it matches getSpeakersIdsBySummit with no filter. + $payload = $this->basePayload(); + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertContains( + self::$defaultSpeaker->getId(), + $chunkPayload['speaker_ids'], + 'a speaker with a presentation in this summit must be selected by the filter-based path' + ); + $this->assertNull( + $this->jobProperty($jobs[0], 'filter'), + 'the raw filter (null, in this case) must still reach the dispatched job unchanged' + ); + } + + public function testMemberIdFilterSelectsOnlyTheMatchingSpeaker(): void + { + Queue::fake(); + + $prefix = str_random(10); + $member = new Member(); + $member->setEmail("chunk-test+{$prefix}@test.com"); + $member->setActive(true); + $member->setFirstName("Chunk"); + $member->setLastName("Test"); + $member->setEmailVerified(true); + $member->setUserExternalId(mt_rand()); + self::$em->persist($member); + + $memberSpeaker = new PresentationSpeaker(); + $memberSpeaker->setFirstName("Chunk"); + $memberSpeaker->setLastName("Test Speaker"); + $memberSpeaker->setMember($member); + self::$em->persist($memberSpeaker); + + $presentation = new \models\summit\Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle("Chunk test presentation {$prefix}"); + $presentation->setAbstract("Abstract {$prefix}"); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType(self::$defaultPresentationType); + $presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + $presentation->addSpeaker($memberSpeaker); + self::$em->persist($presentation); + + self::$em->flush(); + + $rawFilter = ['member_id==' . $member->getId()]; + + $payload = $this->basePayload(); + + $this->service()->triggerSendEmails(self::$summit, $payload, $rawFilter); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertEquals( + [$memberSpeaker->getId()], + $chunkPayload['speaker_ids'], + 'member_id must select only the speaker belonging to that member, not the default fixture speaker' + ); + } + + public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedIdExactlyOnce(): void + { + // The filter-based path is the only one summit-admin drives (it always sends filter[], + // never speaker_ids). Seed one more speaker than the configured job chunk size sharing a + // unique first name on one presentation of this summit, so dispatch has to split them + // into two job chunks. self::$defaultSpeaker (with its own presentations) is the control + // the first_name filter must leave out. A page that is skipped, re-read, or overwritten + // instead of merged breaks the exact-set assertion below. + Queue::fake(); + + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + + $firstName = 'PageSeed' . str_random(8); + + $presentation = new \models\summit\Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle("Multi-page chunk test {$firstName}"); + $presentation->setAbstract("Abstract {$firstName}"); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType(self::$defaultPresentationType); + $presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + + $seeded = []; + for ($i = 0; $i < $chunkSize + 1; $i++) { + $speaker = new PresentationSpeaker(); + $speaker->setFirstName($firstName); + $speaker->setLastName("Speaker {$i}"); + self::$em->persist($speaker); + $presentation->addSpeaker($speaker); + $seeded[] = $speaker; + } + self::$em->persist($presentation); + self::$em->flush(); + + $seededIds = array_map(fn($s) => $s->getId(), $seeded); + sort($seededIds); + + $this->service()->triggerSendEmails(self::$summit, $this->basePayload(), ["first_name=={$firstName}"]); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 2); + + $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $this->pushedJobs()); + $sizes = array_map('count', $slices); + rsort($sizes); + $this->assertEquals([$chunkSize, 1], $sizes, 'chunkSize + 1 matched ids must become one full chunk and one single-id chunk'); + + $covered = array_merge(...$slices); + sort($covered); + $this->assertEquals($seededIds, $covered, 'the chunks together must cover every seeded id exactly once, across both pages'); + $this->assertNotContains(self::$defaultSpeaker->getId(), $covered, 'the first_name filter must leave the control speaker out'); + } + + 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 all 3 chunks fail through all 3 tiers; one Log::error + // per chunk proves the loop reached every chunk instead of aborting on the first. + \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['speaker_ids'] = range(1, 250); // 3 chunks + + $this->service()->triggerSendEmails(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); + } +} diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index 6e57e5760..90f1a130d 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -495,6 +495,101 @@ public function testGetCurrentSummitSpeakersOrderByIDAndFilteredBySelPlan() $this->assertTrue(count($speakers[0]->accepted_presentations) == 20); } + public function testGetAllSpeakersFilteredByMemberId() + { + // getAll() is global/non-summit-scoped. Its member_id/member_user_external_id + // support predates this change; this proves swapping its inline whitelist for + // ISpeakerFilterFields::GLOBAL_OPERATORS/GLOBAL_VALIDATION_RULES didn't break it. + // + // Widening getAll() to the full ISpeakerFilterFields (like the summit-scoped + // endpoints) was attempted and reverted: DoctrineSpeakerRepository::getFilterMappings() + // hard-codes a ":summit" bound parameter into every presentations_*/has_*_presentations + // DQL template (shared verbatim with the summit-scoped query methods, which bind it on + // their own base query). getAllByPage()/getAllIdsByPage() never bind it - reproduced + // directly: Doctrine\ORM\Query\QueryException "too few parameters" applying + // presentations_track_id to getAllByPage(). Not a silent no-op, a hard 500. See + // ISpeakerFilterFields::GLOBAL_OPERATORS's docblock for the full field-by-field + // breakdown of what is/isn't summit-independent. + $control = new PresentationSpeaker(); + $control->setFirstName("NoMember"); + $control->setLastName("Control " . str_random(8)); + self::$em->persist($control); + self::$em->flush(); + + $params = [ + 'page' => 1, + 'per_page' => 100, + 'filter' => [ + sprintf('member_id==%s', self::$defaultMember->getId()), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getAll", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(200); + $speakers_response = json_decode($response->getContent()); + $this->assertTrue(!is_null($speakers_response)); + $ids = array_map(fn($s) => $s->id, $speakers_response->data); + + $this->assertContains( + self::$defaultSpeaker->getId(), + $ids, + 'the speaker belonging to the filtered member must be included' + ); + $this->assertNotContains( + $control->getId(), + $ids, + 'a speaker with no member (or a different one) must not match' + ); + } + + public function testGetAllSpeakersRejectsPresentationScopedFilter() + { + // getAll() is not summit-scoped and was never wired to accept presentation-related + // fields (see ISpeakerFilterFields::GLOBAL_OPERATORS's docblock: those fields hard-code + // a :summit bound parameter in the shared repository mapping that a global query can + // never bind). This must stay a clean validation error, not a 500. + $params = [ + 'page' => 1, + 'per_page' => 10, + 'filter' => [ + sprintf('presentations_track_id==%s', self::$defaultTrack->getId()), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getAll", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(412); + $content = json_decode($response->getContent(), true); + $this->assertStringContainsString('presentations_track_id', $content['errors'][0]); + } + public function testGetCurrentSummitSpeakersOrderByIDAndFilteredByMediaUploadType() { $media_upload_ids =array_map(function($v){ @@ -800,6 +895,94 @@ public function testExportCurrentSummitSpeakersWithAcceptedPresentations() $this->assertTrue(!empty($csv)); } + public function testSendSpeakersBulkEmailFilteredByMemberUserExternalId() { + // member_user_external_id was previously rejected by send()'s own whitelist (it only + // existed on the listing endpoints). This drives the real HTTP action, so it exercises + // the controller's FilterParser::parse + Filter::validate against the shared + // ISpeakerFilterFields constants AND the service's id resolution, end to end. + // + // The fixture summit has a single speaker with presentations (self::$defaultSpeaker), + // so on its own an exact-equality assertion would pass for the same reason an + // in_array one does. Seed a control speaker - different member, with a presentation + // in this summit - so "exactly [defaultSpeaker]" actually proves the filter narrowed. + Queue::fake(); + + $prefix = str_random(10); + $control_member = new Member(); + $control_member->setEmail("send-control+{$prefix}@test.com"); + $control_member->setActive(true); + $control_member->setFirstName("Control"); + $control_member->setLastName("Member"); + $control_member->setEmailVerified(true); + $control_member->setUserExternalId(mt_rand()); + self::$em->persist($control_member); + + $control_speaker = new PresentationSpeaker(); + $control_speaker->setFirstName("Control"); + $control_speaker->setLastName("Speaker {$prefix}"); + $control_speaker->setMember($control_member); + self::$em->persist($control_speaker); + + $control_presentation = new Presentation(); + self::$summit->addEvent($control_presentation); + $control_presentation->setTitle("Send control presentation {$prefix}"); + $control_presentation->setAbstract("Abstract {$prefix}"); + $control_presentation->setCategory(self::$defaultTrack); + $control_presentation->setType(self::$defaultPresentationType); + $control_presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $control_presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + $control_presentation->addSpeaker($control_speaker); + self::$em->persist($control_presentation); + + self::$em->flush(); + + $params = [ + 'id' => self::$summit->getId(), + 'filter' => [ + 'member_user_external_id==' . self::$member->getUserExternalId(), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $data = [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + ]; + + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@send", + $params, + [], + [], + [], + $headers, + json_encode($data) + ); + + $this->assertResponseStatus(200); + + Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, 1); + Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, function ($job) use ($control_speaker) { + $ref = new \ReflectionObject($job); + $prop = $ref->getProperty('payload'); + $prop->setAccessible(true); + $payload = $prop->getValue($job); + $this->assertEquals( + [self::$defaultSpeaker->getId()], + $payload['speaker_ids'] ?? [], + sprintf( + 'the chunk must contain exactly the speaker of the filtered member, not the control speaker %s', + $control_speaker->getId() + ) + ); + return true; + }); + } + public function testSendSpeakersBulkEmail() { $params = [ 'id' => self::$summit->getId(),