From 10ca31e6985a7bc4b6dec9f6ebbce2169c154cc5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 10:48:04 -0300 Subject: [PATCH 1/7] feat: add CFP reopen speaker/submitter notification endpoint Admin viewing a presentation with a live CFP reopen grant can select any combination of submitter, speakers and moderator, and trigger PUT .../submission-period/reopen/notify to queue one reopen-notification email per selected, distinct recipient (deduped by lowercased email). Response reports how many were queued and how many were skipped for a missing email. - New PresentationSubmissionReopenedEmail job, mirroring PresentationSpeakerNotificationEmail but role-independent. - notify() on PresentationSubmissionReopenService: builds the allowed recipient set from the presentation's own getSpeakers()/getModerator() (never by looking speakers up from the request), intersects against the caller's selection, dedupes by email. - New controller action + route, rate-limited via rate.limit:30,60 middleware (matching the discover/preValidatePromoCode precedent). - Additive config + model migrations registering the endpoint and the email flow event type for already-deployed environments, plus the matching fresh-install seeder entries. ClickUp: https://app.clickup.com/t/9014802374/86bbkbrue --- .../OAuth2PresentationApiController.php | 69 ++++++++ ...EmailTemplatesSchemaSerializerRegistry.php | 2 + app/Jobs/Emails/IMailTemplatesConstants.php | 1 + .../PresentationSubmissionReopenedEmail.php | 101 +++++++++++ .../IPresentationSubmissionReopenService.php | 31 ++++ .../PresentationSubmissionReopenService.php | 113 ++++++++++++ .../config/Version20260824100000.php | 82 +++++++++ .../model/Version20260824090000.php | 89 ++++++++++ database/seeders/ApiEndpointsSeeder.php | 15 ++ .../seeders/SummitEmailFlowTypeSeeder.php | 6 + routes/api_v1.php | 1 + tests/PresentationReopenApiTest.php | 126 +++++++++++++ tests/PresentationReopenAuthzTest.php | 46 +++++ ...resentationSubmissionReopenedEmailTest.php | 164 +++++++++++++++++ ...resentationSubmissionReopenServiceTest.php | 167 ++++++++++++++++++ 15 files changed, 1013 insertions(+) create mode 100644 app/Jobs/Emails/PresentationSubmissions/PresentationSubmissionReopenedEmail.php create mode 100644 database/migrations/config/Version20260824100000.php create mode 100644 database/migrations/model/Version20260824090000.php create mode 100644 tests/PresentationSubmissionReopenedEmailTest.php diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php index f3e382778..63922e57f 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2PresentationApiController.php @@ -641,6 +641,75 @@ public function closeSubmissionPeriod($summit_id, $presentation_id) }); } + #[OA\Put( + path: "/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen/notify", + summary: "Admin-only: notify selected recipients (submitter/speakers/moderator) that the submission period has been reopened", + operationId: "notifySubmissionReopened", + security: [['summit_presentations_auth' => [SummitScopes::WriteSummitData, SummitScopes::WriteEventData, SummitScopes::WritePresentationData]]], + tags: ['Presentations'], + parameters: [ + new OA\Parameter(name: 'id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + new OA\Parameter(name: 'presentation_id', in: 'path', required: true, schema: new OA\Schema(type: 'integer')), + ], + requestBody: new OA\RequestBody( + required: false, + content: new OA\JsonContent( + properties: [ + new OA\Property(property: 'speaker_ids', type: 'array', items: new OA\Items(type: 'integer')), + new OA\Property(property: 'include_submitter', type: 'boolean'), + ] + ) + ), + responses: [ + new OA\Response( + response: Response::HTTP_OK, + description: "OK", + content: new OA\JsonContent( + properties: [ + new OA\Property(property: 'recipients', type: 'integer'), + new OA\Property(property: 'skipped', type: 'integer'), + ] + ) + ), + new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), + new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), + new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Not Found"), + new OA\Response(response: Response::HTTP_PRECONDITION_FAILED, description: "Validation Error"), + new OA\Response(response: Response::HTTP_INTERNAL_SERVER_ERROR, description: "Server Error"), + ] + )] + public function notifySubmissionReopened($summit_id, $presentation_id) + { + return $this->processRequest(function () use ($summit_id, $presentation_id) { + + $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->resource_server_context)->find($summit_id); + if (is_null($summit)) return $this->error404(); + + $current_member = $this->resource_server_context->getCurrentUser(); + if (is_null($current_member)) return $this->error403(); + + $isAdmin = $current_member->isAdmin() + || $current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators); + if (!$isAdmin) return $this->error403(); + + $payload = $this->getJsonPayload([ + 'speaker_ids' => 'sometimes|array', + 'speaker_ids.*' => 'integer', + 'include_submitter' => 'sometimes|boolean', + ]); + + $result = $this->presentation_submission_reopen_service->notify( + $summit, + intval($presentation_id), + $payload['speaker_ids'] ?? [], + boolval($payload['include_submitter'] ?? false), + $current_member + ); + + return $this->ok(['recipients' => $result['queued'], 'skipped' => $result['skipped']]); + }); + } + #[OA\Put( path: "/api/v1/summits/{id}/presentations/{presentation_id}/completed", summary: "Mark a presentation submission as completed", diff --git a/app/Jobs/Emails/EmailTemplatesSchemaSerializerRegistry.php b/app/Jobs/Emails/EmailTemplatesSchemaSerializerRegistry.php index fc709bc6f..8e892983b 100644 --- a/app/Jobs/Emails/EmailTemplatesSchemaSerializerRegistry.php +++ b/app/Jobs/Emails/EmailTemplatesSchemaSerializerRegistry.php @@ -29,6 +29,7 @@ use App\Jobs\Emails\PresentationSubmissions\PresentationCreatorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationModeratorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationSpeakerNotificationEmail; +use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedAlternateEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedOnlyEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedRejectedEmail; @@ -146,6 +147,7 @@ private function __construct() $this->registry[PresentationCreatorNotificationEmail::EVENT_SLUG] = PresentationCreatorNotificationEmail::class; $this->registry[PresentationModeratorNotificationEmail::EVENT_SLUG] = PresentationModeratorNotificationEmail::class; $this->registry[PresentationSpeakerNotificationEmail::EVENT_SLUG] = PresentationSpeakerNotificationEmail::class; + $this->registry[PresentationSubmissionReopenedEmail::EVENT_SLUG] = PresentationSubmissionReopenedEmail::class; $this->registry[SpeakerCreationEmail::EVENT_SLUG] = SpeakerCreationEmail::class; $this->registry[SpeakerEditPermissionApprovedEmail::EVENT_SLUG] = SpeakerEditPermissionApprovedEmail::class; $this->registry[SpeakerEditPermissionRejectedEmail::EVENT_SLUG] = SpeakerEditPermissionRejectedEmail::class; diff --git a/app/Jobs/Emails/IMailTemplatesConstants.php b/app/Jobs/Emails/IMailTemplatesConstants.php index e11aac0ef..aa6a83318 100644 --- a/app/Jobs/Emails/IMailTemplatesConstants.php +++ b/app/Jobs/Emails/IMailTemplatesConstants.php @@ -174,6 +174,7 @@ interface IMailTemplatesConstants const summit_reassign_ticket_till_date = 'summit_reassign_ticket_till_date'; const summit_schedule_url = 'summit_schedule_url'; const summit_site_url = 'summit_site_url'; + const summit_slug = 'summit_slug'; const summit_schedule_default_event_detail_url = 'summit_schedule_default_event_detail_url'; const summit_virtual_site_oauth2_client_id = 'summit_virtual_site_oauth2_client_id'; const summit_virtual_site_url = 'summit_virtual_site_url'; diff --git a/app/Jobs/Emails/PresentationSubmissions/PresentationSubmissionReopenedEmail.php b/app/Jobs/Emails/PresentationSubmissions/PresentationSubmissionReopenedEmail.php new file mode 100644 index 000000000..f416431a6 --- /dev/null +++ b/app/Jobs/Emails/PresentationSubmissions/PresentationSubmissionReopenedEmail.php @@ -0,0 +1,101 @@ +getSummit(); + $selection_plan = $presentation->getSelectionPlan(); + + if (is_null($selection_plan)) + throw new \InvalidArgumentException('Presentation selection plan is null.'); + + $support_email = $summit->getSupportEmail(); + $support_email = !empty($support_email) ? $support_email : Config::get("cfp.support_email", null); + + if (empty($support_email)) + throw new \InvalidArgumentException('cfp.support_email is null.'); + + $payload = []; + + $payload[IMailTemplatesConstants::full_name] = $to_full_name; + $payload[IMailTemplatesConstants::presentation_title] = $presentation->getTitle(); + $payload[IMailTemplatesConstants::selection_plan_name] = $selection_plan->getName(); + $payload[IMailTemplatesConstants::summit_slug] = $summit->getRawSlug(); + $payload[IMailTemplatesConstants::selection_plan_id] = $selection_plan->getId(); + $payload[IMailTemplatesConstants::presentation_id] = $presentation->getId(); + $payload[IMailTemplatesConstants::support_email] = $support_email; + + // until_date deliberately breaks the sibling format (date-only): a reopen window is + // measured in hours, so render summit-local date, time and zone label. + $until = $presentation->getSubmissionReopenedUntil(); + $local = $selection_plan->convertDateFromUTC2TimeZone($until); + $payload[IMailTemplatesConstants::until_date] = is_null($local) + ? $until->format('F d, Y g:i a') . ' UTC' + : $local->format('F d, Y g:i a') . ' ' . $summit->getTimeZoneLabel(); + + $template_identifier = $this->getEmailTemplateIdentifierFromEmailEvent($summit); + + parent::__construct($summit, $payload, $template_identifier, $to_email); + } + + /** + * @return array + */ + public static function getEmailTemplateSchema(): array + { + $payload = parent::getEmailTemplateSchema(); + + $payload[IMailTemplatesConstants::full_name]['type'] = 'string'; + $payload[IMailTemplatesConstants::presentation_title]['type'] = 'string'; + $payload[IMailTemplatesConstants::until_date]['type'] = 'string'; + $payload[IMailTemplatesConstants::selection_plan_name]['type'] = 'string'; + $payload[IMailTemplatesConstants::summit_slug]['type'] = 'string'; + $payload[IMailTemplatesConstants::support_email]['type'] = 'string'; + $payload[IMailTemplatesConstants::selection_plan_id]['type'] = 'int'; + $payload[IMailTemplatesConstants::presentation_id]['type'] = 'int'; + + return $payload; + } +} diff --git a/app/Services/Model/IPresentationSubmissionReopenService.php b/app/Services/Model/IPresentationSubmissionReopenService.php index e162de225..123a4f46d 100644 --- a/app/Services/Model/IPresentationSubmissionReopenService.php +++ b/app/Services/Model/IPresentationSubmissionReopenService.php @@ -45,4 +45,35 @@ public function reopen(Summit $summit, int $presentation_id, ?int $hours, Member * @throws EntityNotFoundException if the presentation is not in $summit */ public function closeNow(Summit $summit, int $presentation_id, Member $actor): void; + + /** + * Queues one reopen-notification email per SELECTED, distinct recipient for the presentation's + * CURRENTLY ACTIVE grant. + * + * The admin chooses who is notified. $speaker_ids names speakers and/or the moderator (the + * moderator IS a PresentationSpeaker, so it needs no separate parameter); $include_submitter + * covers the submitter -- SummitEvent::getCreatedBy(), a Member with no speaker id. Every id is + * verified to belong to THIS presentation -- see the trust-boundary note in the implementation. + * + * Not a delivery count. PresentationSubmissionReopenedEmail is a ShouldQueue job, so this + * returns before any mail has been handed to mailing-api, let alone sent. Delivery outcome + * lives in mailing-api's Mail rows. + * + * Repeatable by design, with a different selection each time if the admin wants: there is no + * once-only marker and no persisted selection. + * + * @return array{queued: int, skipped: int} queued = distinct recipients with a usable email + * that were queued; skipped = selected recipients dropped for a missing email. + * @throws EntityNotFoundException if the presentation is not in $summit + * @throws ValidationException if no grant is in force, if the selection is empty, if any id + * is not attached to this presentation, or if no selected + * recipient has an email + */ + public function notify( + Summit $summit, + int $presentation_id, + array $speaker_ids, + bool $include_submitter, + Member $actor + ): array; } diff --git a/app/Services/Model/Imp/PresentationSubmissionReopenService.php b/app/Services/Model/Imp/PresentationSubmissionReopenService.php index 2d6e2f2ad..caec94bde 100644 --- a/app/Services/Model/Imp/PresentationSubmissionReopenService.php +++ b/app/Services/Model/Imp/PresentationSubmissionReopenService.php @@ -12,6 +12,7 @@ * limitations under the License. **/ +use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use App\Services\Model\AbstractService; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; @@ -123,4 +124,116 @@ public function closeNow(Summit $summit, int $presentation_id, Member $actor): v ) ); } + + public function notify( + Summit $summit, + int $presentation_id, + array $speaker_ids, + bool $include_submitter, + Member $actor + ): array { + // Read inside the transaction, dispatch outside it. Same reasoning as closeNow()'s + // deferred audit line: flush/commit happen after the closure and a retryable failure + // re-runs it, so dispatching inside would queue mail for a read that had not committed + // and could queue it more than once. + [$presentation, $recipients, $skipped, $deadline] = $this->tx_service->transaction( + function () use ($summit, $presentation_id, $speaker_ids, $include_submitter) { + + // summit-scoped unconditionally, matching reopen() and closeNow() + $presentation = $summit->getEvent($presentation_id); + if (!$presentation instanceof Presentation) + throw new EntityNotFoundException(sprintf("Presentation %s not found.", $presentation_id)); + + // The single grant gate. isSubmissionReopened() is false both when no grant exists + // AND when the plan's submission_end_date has since been extended past now -- in + // that second case the speaker is editing under normal open-window rules and the + // grant is not what is letting them in, so there is no reopen deadline to announce. + if (!$presentation->isSubmissionReopened()) + throw new ValidationException("Submission is not currently reopened for this presentation."); + + $speaker_ids = array_values(array_unique(array_map('intval', $speaker_ids))); + + if (empty($speaker_ids) && !$include_submitter) + throw new ValidationException("Select at least one recipient."); + + // --------------------------------------------------------------------------- + // TRUST BOUNDARY. The caller now names recipients, so the set of people this + // endpoint may mail must be derived from the PRESENTATION, never from the request. + // Without this check the endpoint mails any speaker id in the system on behalf of + // any summit admin: an authenticated mail relay. Build the allowed map first, then + // intersect -- do not look speakers up by id from the repository. + // --------------------------------------------------------------------------- + $allowed = []; // speaker id => PresentationSpeaker + $roles = []; // speaker id => 'speaker' | 'moderator' | 'speaker, moderator' + foreach ($presentation->getSpeakers() as $speaker) { + $allowed[$speaker->getId()] = $speaker; + $roles[$speaker->getId()] = 'speaker'; + } + + // Separate association, NOT necessarily a member of getSpeakers(). Dropping this + // line makes every moderator-only recipient fail the intersect below as "not on + // this presentation". + $moderator = $presentation->getModerator(); + if (!is_null($moderator)) { + $allowed[$moderator->getId()] = $moderator; + $roles[$moderator->getId()] = isset($roles[$moderator->getId()]) + ? 'speaker, moderator' : 'moderator'; + } + + $unknown = array_diff($speaker_ids, array_keys($allowed)); + if (!empty($unknown)) + throw new ValidationException(sprintf( + "Speaker(s) %s are not on this presentation.", implode(', ', $unknown) + )); + + // keyed by normalized email -> display name + $recipients = []; $skipped = 0; + + $add = function (?string $email, ?string $name, string $role) use (&$recipients, &$skipped) { + $key = strtolower(trim($email ?? '')); + if ($key === '') { + $skipped++; + // Logged, never fatal: one incomplete record must not block the others. + Log::warning(sprintf("PresentationSubmissionReopenService::notify: %s has no usable email; skipped.", $role)); + return; + } + if (!array_key_exists($key, $recipients)) $recipients[$key] = $name; + }; + + // Submitter first: on a self-submitted talk they are also a speaker, and + // first-write wins in $add, so the submitter's own name is the one used and they + // get ONE email even when both boxes are ticked. + if ($include_submitter) { + // getCreatedBy(), NOT getCreator(): the latter is @deprecated. + $submitter = $presentation->getCreatedBy(); + if (is_null($submitter)) + throw new ValidationException("This presentation has no submitter to notify."); + $add($submitter->getEmail(), $submitter->getFullName(), sprintf('submitter (member %s)', $submitter->getId())); + } + + foreach ($speaker_ids as $id) + $add($allowed[$id]->getEmail(), $allowed[$id]->getFullName(), sprintf('%s %s', $roles[$id], $id)); + + if (empty($recipients)) + throw new ValidationException("None of the selected recipients has an email address."); + + return [$presentation, $recipients, $skipped, $presentation->getSubmissionReopenedUntil()]; + } + ); + + foreach ($recipients as $email => $name) + PresentationSubmissionReopenedEmail::dispatch($presentation, $email, $name ?? ''); + + // Report queued and skipped, NOT "queued of selected". Selection is counted in rows by the + // client and in ids by the server, and one merged row (a submitter who is also a speaker) + // sets two channels, so "1 of 2 selected" would be a true statement about a single ticked + // box. Queued plus skipped is unambiguous at both ends. + Log::info(sprintf( + "PresentationSubmissionReopenService::notify summit %s presentation %s queued %s recipient(s), %s skipped for missing email, by member %s (window ends %s).", + $summit->getId(), $presentation_id, count($recipients), $skipped, $actor->getId(), + $deadline->setTimezone(new \DateTimeZone('UTC'))->format('Y-m-d\TH:i:s\Z') + )); + + return ['queued' => count($recipients), 'skipped' => $skipped]; + } } diff --git a/database/migrations/config/Version20260824100000.php b/database/migrations/config/Version20260824100000.php new file mode 100644 index 000000000..f58ac5f21 --- /dev/null +++ b/database/migrations/config/Version20260824100000.php @@ -0,0 +1,82 @@ + 'notify-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen/notify', + 'http_method' => 'PUT', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData, + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ], + ], + ]; + + public function getDescription(): string + { + return 'Register the CFP reopen notification endpoint.'; + } + + public function up(Schema $schema): void + { + $this->registerEndpoints(self::API_NAME, self::ENDPOINTS); + } + + public function down(Schema $schema): void + { + $this->unregisterEndpoints(self::API_NAME, array_column(self::ENDPOINTS, 'name')); + } +} diff --git a/database/migrations/model/Version20260824090000.php b/database/migrations/model/Version20260824090000.php new file mode 100644 index 000000000..5292a9b70 --- /dev/null +++ b/database/migrations/model/Version20260824090000.php @@ -0,0 +1,89 @@ +delete(), ..."SummitEmailEventFlowType")->delete() and + * ..."SummitEmailEventFlow")->delete() -- that third table holds every summit's per-event + * template overrides, so a seeder run silently discards every show's customized email wiring. + * This migration is therefore the only safe way to register the type on a deployed database. + * + * On a fresh install this correctly does nothing, and that is not a bug: migrations run before + * seeders, so the "Presentation Submissions" flow does not exist yet at migration time -- the + * is_null($flow) guard below simply returns, and SummitEmailFlowTypeSeeder creates both the flow + * and this event type together afterwards. + */ +final class Version20260824090000 extends AbstractMigration +{ + public function getDescription(): string + { + return 'Seed new SummitEmailEventFlowType (CFP reopen notification)'; + } + + public function up(Schema $schema): void + { + DB::setDefaultConnection("model"); + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $repository = $em->getRepository(SummitEmailFlowType::class); + $flow = $repository->findOneBy([ + "name" => "Presentation Submissions" + ]); + if (is_null($flow)) return; + + SummitEmailFlowTypeSeeder::createEventsTypes( + [ + [ + 'name' => PresentationSubmissionReopenedEmail::EVENT_NAME, + 'slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG, + 'default_email_template' => PresentationSubmissionReopenedEmail::DEFAULT_TEMPLATE + ], + ], + $flow + ); + + $em->persist($flow); + $em->flush(); + } + + /** + * Deliberate no-op, matching Version20250812201257.php's precedent. Deleting the + * SummitEmailEventFlowType row would cascade to any SummitEmailEventFlow override a show has + * already customized against it (SummitEmailEventFlow.SummitEmailEventFlowTypeID ... ON + * DELETE CASCADE), destroying an operator's per-summit wording. An unused type row costs + * nothing; a destroyed override costs an operator their copy. + */ + public function down(Schema $schema): void + { + } +} diff --git a/database/seeders/ApiEndpointsSeeder.php b/database/seeders/ApiEndpointsSeeder.php index 1fd7dc720..5fc5be1f0 100644 --- a/database/seeders/ApiEndpointsSeeder.php +++ b/database/seeders/ApiEndpointsSeeder.php @@ -7122,6 +7122,21 @@ private function seedSummitEndpoints() IGroup::SummitAdministrators, ] ], + [ + 'name' => 'notify-presentation-submission-period', + 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/submission-period/reopen/notify', + 'http_method' => 'PUT', + 'scopes' => [ + SummitScopes::WriteSummitData, + SummitScopes::WriteEventData, + SummitScopes::WritePresentationData + ], + 'authz_groups' => [ + IGroup::SuperAdmins, + IGroup::Administrators, + IGroup::SummitAdministrators, + ], + ], // presentation speakers [ 'name' => 'add-presentation-speaker', diff --git a/database/seeders/SummitEmailFlowTypeSeeder.php b/database/seeders/SummitEmailFlowTypeSeeder.php index 17e2636cc..c676ffa14 100644 --- a/database/seeders/SummitEmailFlowTypeSeeder.php +++ b/database/seeders/SummitEmailFlowTypeSeeder.php @@ -27,6 +27,7 @@ use App\Jobs\Emails\PresentationSubmissions\PresentationCreatorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationModeratorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationSpeakerNotificationEmail; +use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedAlternateEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedOnlyEmail; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessAcceptedRejectedEmail; @@ -343,6 +344,11 @@ public static function seed(){ 'slug' => PresentationSpeakerNotificationEmail::EVENT_SLUG, 'default_email_template' => PresentationSpeakerNotificationEmail::DEFAULT_TEMPLATE ], + [ + 'name' => PresentationSubmissionReopenedEmail::EVENT_NAME, + 'slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG, + 'default_email_template' => PresentationSubmissionReopenedEmail::DEFAULT_TEMPLATE + ], [ 'name' => PresentationModeratorNotificationEmail::EVENT_NAME, 'slug' => PresentationModeratorNotificationEmail::EVENT_SLUG, diff --git a/routes/api_v1.php b/routes/api_v1.php index ba3f0a43b..0132c00b1 100644 --- a/routes/api_v1.php +++ b/routes/api_v1.php @@ -867,6 +867,7 @@ Route::group(['prefix' => 'reopen'], function () { Route::put('', ['middleware' => 'auth.user', 'uses' => 'OAuth2PresentationApiController@reopenSubmissionPeriod']); Route::delete('', ['middleware' => 'auth.user', 'uses' => 'OAuth2PresentationApiController@closeSubmissionPeriod']); + Route::put('notify', ['middleware' => ['auth.user', 'rate.limit:30,60'], 'uses' => 'OAuth2PresentationApiController@notifySubmissionReopened']); }); }); diff --git a/tests/PresentationReopenApiTest.php b/tests/PresentationReopenApiTest.php index eb46d61a8..043eb7b8f 100644 --- a/tests/PresentationReopenApiTest.php +++ b/tests/PresentationReopenApiTest.php @@ -12,10 +12,13 @@ * limitations under the License. **/ +use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\Registry; use ModelSerializers\SerializerRegistry; use models\summit\Presentation; +use models\summit\PresentationSpeaker; use models\summit\SummitEvent; use models\utils\SilverstripeBaseModel; @@ -97,6 +100,34 @@ protected function closeNow() ); } + protected function notify(array $payload) + { + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => self::$presentation->getId(), + ]; + $headers = $this->getAuthHeaders(); // includes CONTENT_TYPE: application/json + + return $this->action( + "PUT", "OAuth2PresentationApiController@notifySubmissionReopened", + $params, [], [], [], $headers, json_encode($payload) + ); + } + + /** + * A speaker with no linked Member and no registration_request -- PresentationSpeaker::getEmail() + * (PresentationSpeaker.php:1953-1969) returns null for both branches, so this is the "no usable + * email" fixture the notify tests need without touching the blank@blank.com sentinel path. + */ + private function speakerWithNoEmail(): PresentationSpeaker + { + $speaker = new PresentationSpeaker(); + $speaker->setFirstName("Katherine"); + $speaker->setLastName("Johnson"); + self::$em->persist($speaker); + return $speaker; + } + /** * Reload from the DB rather than reading the response body. * @@ -986,4 +1017,99 @@ public function testAllowedMediaUploadTypesAreUnchangedUnderReopen() $this->assertEquals($before, $after); } + + // ------------------------------------------------------------------------- + // notify() endpoint + // ------------------------------------------------------------------------- + + public function testNotifyQueuesSelectedRecipientsAndReturns200WithCounts() + { + Queue::fake(); + $this->grantWindow(24); + + // self::$speaker is self::$member's own speaker profile (InsertMemberTestData.php:144-149), + // so selecting it AND include_submitter=true exercises the submitter+speaker merge -- + // ONE dispatch, not two -- in the same request as the moderator-accepted assertion below. + $this->attachSpeaker(); + $moderator = $this->speakerWithNoEmail(); + self::$presentation->setModerator($moderator); + self::$em->flush(); + + $response = $this->notify([ + 'speaker_ids' => [self::$speaker->getId(), $moderator->getId()], + 'include_submitter' => true, + ]); + + $this->assertResponseStatus(200); + $body = json_decode($response->getContent(), true); + // submitter + self::$speaker share self::$member's email -> merge into 1; moderator has no + // email -> skipped. + $this->assertEquals(1, $body['recipients']); + $this->assertEquals(1, $body['skipped']); + Queue::assertPushed(PresentationSubmissionReopenedEmail::class, 1); + } + + public function testNotifyOnlyQueuesTheSelectedRecipientsNotEveryoneOnTheTalk() + { + Queue::fake(); + $this->grantWindow(24); + $this->attachSpeaker(); + $unselected = $this->speakerWithNoEmail(); + $unselected->setFirstName("Alan"); + $unselected->setLastName("Turing"); + self::$presentation->addSpeaker($unselected); + self::$em->flush(); + + // only self::$speaker selected -- $unselected must NOT be queued even though it is on the + // presentation. Regression test for "falls back to notifying everyone". + $response = $this->notify(['speaker_ids' => [self::$speaker->getId()]]); + + $this->assertResponseStatus(200); + $body = json_decode($response->getContent(), true); + $this->assertEquals(1, $body['recipients']); + $this->assertEquals(0, $body['skipped']); + Queue::assertPushed(PresentationSubmissionReopenedEmail::class, 1); + } + + public function testNotifyBlankEmailRecipientIsSkippedWhileOthersAreDispatched() + { + Queue::fake(); + $this->grantWindow(24); + $this->attachSpeaker(); + $blank = $this->speakerWithNoEmail(); + self::$presentation->addSpeaker($blank); + self::$em->flush(); + + $response = $this->notify([ + 'speaker_ids' => [self::$speaker->getId(), $blank->getId()], + ]); + + $this->assertResponseStatus(200); + $body = json_decode($response->getContent(), true); + $this->assertEquals(1, $body['recipients']); + $this->assertEquals(1, $body['skipped']); + Queue::assertPushed(PresentationSubmissionReopenedEmail::class, 1); + } + + public function testNotifyRejectsASpeakerIdNotOnThisPresentationAndQueuesNothing() + { + Queue::fake(); + $this->grantWindow(24); + + $response = $this->notify(['speaker_ids' => [999999]]); + + $this->assertResponseStatus(412); + Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); + } + + public function testNotifyRejectsAnEmptySelection() + { + Queue::fake(); + $this->grantWindow(24); + + $response = $this->notify([]); + + $this->assertResponseStatus(412); + Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); + } } diff --git a/tests/PresentationReopenAuthzTest.php b/tests/PresentationReopenAuthzTest.php index 667e55aa5..94127770a 100644 --- a/tests/PresentationReopenAuthzTest.php +++ b/tests/PresentationReopenAuthzTest.php @@ -12,9 +12,11 @@ * limitations under the License. **/ +use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use App\Models\Foundation\Main\IGroup; use App\Models\ResourceServer\IAccessTokenService; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\Registry; use models\summit\Presentation; use models\utils\SilverstripeBaseModel; @@ -355,6 +357,50 @@ public function testSpeakerOnThePresentationStillCannotClose() ); } + /** + * Same coarse-gate refusal as reopen/close, exercised on the notify endpoint: a member with no + * summit-admin permission never reaches the service, so nothing is queued. + */ + public function testMemberWithNoSummitAdminPermissionCannotNotify() + { + Queue::fake(); + $this->grantWindow(self::$presentation); + + $this->action( + "PUT", "OAuth2PresentationApiController@notifySubmissionReopened", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders(), json_encode(['include_submitter' => true]) + ); + $this->assertResponseStatus(403); + + Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); + } + + /** + * Persona (b) on the notify endpoint too: being a recipient is not being an operator. The + * member is the creator AND an assigned speaker (memberCanEdit() true), and is still refused -- + * §4 gives no speaker path to the reopen controls, notify included. + */ + public function testSpeakerOnThePresentationStillCannotNotify() + { + Queue::fake(); + self::$presentation->addSpeaker(self::$speaker); + $this->grantWindow(self::$presentation); + + $reloaded = $this->reload(self::$presentation->getId()); + $this->assertTrue($reloaded->memberCanEdit(self::$member), 'speaker/creator link did not persist'); + $this->assertTrue($reloaded->isSubmissionReopened(), 'grant did not persist'); + + $this->action( + "PUT", "OAuth2PresentationApiController@notifySubmissionReopened", + ['id' => self::$summit->getId(), 'presentation_id' => self::$presentation->getId()], + [], [], [], $this->getAuthHeaders(), json_encode(['include_submitter' => true]) + ); + $this->assertResponseStatus(403); + + Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); + } + // --------------------------------------------------------------------------------------------- // The _by fields are admin-only ON THE WIRE. // diff --git a/tests/PresentationSubmissionReopenedEmailTest.php b/tests/PresentationSubmissionReopenedEmailTest.php new file mode 100644 index 000000000..20fca5dc3 --- /dev/null +++ b/tests/PresentationSubmissionReopenedEmailTest.php @@ -0,0 +1,164 @@ +setTitle("REOPEN NOTIFICATION EMAIL TEST"); + self::$presentation->setType(self::$defaultPresentationType); + self::$presentation->setSelectionPlan(self::$default_selection_plan); + self::$presentation->setCreatedBy(self::$member); + self::$presentation->setCategory(self::$defaultTrack); + self::$summit->addEvent(self::$presentation); + + self::$summit->setTimeZoneId("America/Chicago"); + self::$default_selection_plan->setIsEnabled(true); + self::$default_selection_plan->setSubmissionBeginDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P10D')) + ); + self::$default_selection_plan->setSubmissionEndDate( + (new \DateTime('now', new \DateTimeZone('UTC')))->sub(new \DateInterval('P1D')) + ); + + // live grant: 24h window from now + self::$presentation->reopenSubmission(24, self::$member); + + self::$em->persist(self::$summit); + self::$em->flush(); + + $this->ensureEmailEventFlowTypeRegistered(); + } + + /** + * The job's constructor refuses to build without a resolvable template identifier + * (AbstractEmailJob::__construct throws on an empty one), which requires a + * SummitEmailEventFlowType row for this event slug attached to the existing + * "Presentation Submissions" SummitEmailFlowType. That row is normally created once by + * Task 5's migration (deployed envs) or Task 6's SummitEmailFlowTypeSeeder entry (fresh + * installs / this suite's one-time process-wide seed in BrowserKitTestCase::prepareForTests()). + * Guarded so this test is self-contained regardless of whether those tasks have landed yet, + * and a no-op once they have (the seeder/migration already created the row). + */ + private function ensureEmailEventFlowTypeRegistered(): void + { + $repo = self::$em->getRepository(SummitEmailEventFlowType::class); + $existing = $repo->findOneBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]); + if (!is_null($existing)) return; + + $flow = self::$em->getRepository(SummitEmailFlowType::class)->findOneBy(['name' => 'Presentation Submissions']); + $this->assertNotNull($flow, '"Presentation Submissions" SummitEmailFlowType must already be seeded by prepareForTests().'); + + $event_type = new SummitEmailEventFlowType(); + $event_type->setFlow($flow); + $event_type->setName(PresentationSubmissionReopenedEmail::EVENT_NAME); + $event_type->setSlug(PresentationSubmissionReopenedEmail::EVENT_SLUG); + $event_type->setDefaultEmailTemplate(PresentationSubmissionReopenedEmail::DEFAULT_TEMPLATE); + + self::$em->persist($event_type); + self::$em->flush(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + parent::tearDown(); + } + + private function readPayload(PresentationSubmissionReopenedEmail $job): array + { + $prop = new ReflectionProperty($job, 'payload'); + $prop->setAccessible(true); + return $prop->getValue($job); + } + + public function testPayloadContainsAllKeysWithLocalTimeZone() + { + $job = new PresentationSubmissionReopenedEmail(self::$presentation, 'speaker@example.com', 'Grace Hopper'); + $payload = $this->readPayload($job); + + $this->assertNotEmpty($payload[IMailTemplatesConstants::summit_slug]); + $this->assertNotEmpty($payload[IMailTemplatesConstants::selection_plan_id]); + $this->assertNotEmpty($payload[IMailTemplatesConstants::presentation_id]); + $this->assertEquals('Grace Hopper', $payload[IMailTemplatesConstants::full_name]); + $this->assertEquals('REOPEN NOTIFICATION EMAIL TEST', $payload[IMailTemplatesConstants::presentation_title]); + $this->assertEquals(self::$default_selection_plan->getName(), $payload[IMailTemplatesConstants::selection_plan_name]); + $this->assertEquals(self::$summit->getRawSlug(), $payload[IMailTemplatesConstants::summit_slug]); + $this->assertEquals(self::$default_selection_plan->getId(), $payload[IMailTemplatesConstants::selection_plan_id]); + $this->assertEquals(self::$presentation->getId(), $payload[IMailTemplatesConstants::presentation_id]); + $this->assertNotEmpty($payload[IMailTemplatesConstants::support_email]); + + // date + time + zone label, not date-only + $until = self::$presentation->getSubmissionReopenedUntil(); + $local = self::$default_selection_plan->convertDateFromUTC2TimeZone($until); + $expected = $local->format('F d, Y g:i a') . ' ' . self::$summit->getTimeZoneLabel(); + $this->assertEquals($expected, $payload[IMailTemplatesConstants::until_date]); + } + + public function testUnparseableTimeZoneFallsBackToUtcWithoutThrow() + { + self::$summit->setTimeZoneId('Not/A/Real/Zone'); + self::$em->persist(self::$summit); + self::$em->flush(); + + $job = new PresentationSubmissionReopenedEmail(self::$presentation, 'speaker@example.com', 'Grace Hopper'); + $payload = $this->readPayload($job); + + $until = self::$presentation->getSubmissionReopenedUntil(); + $expected = $until->format('F d, Y g:i a') . ' UTC'; + $this->assertEquals($expected, $payload[IMailTemplatesConstants::until_date]); + } + + public function testGetEmailTemplateSchemaDeclaresAllKeys() + { + $schema = PresentationSubmissionReopenedEmail::getEmailTemplateSchema(); + + $this->assertEquals('string', $schema[IMailTemplatesConstants::full_name]['type']); + $this->assertEquals('string', $schema[IMailTemplatesConstants::presentation_title]['type']); + $this->assertEquals('string', $schema[IMailTemplatesConstants::until_date]['type']); + $this->assertEquals('string', $schema[IMailTemplatesConstants::selection_plan_name]['type']); + $this->assertEquals('string', $schema[IMailTemplatesConstants::summit_slug]['type']); + $this->assertEquals('string', $schema[IMailTemplatesConstants::support_email]['type']); + $this->assertEquals('int', $schema[IMailTemplatesConstants::selection_plan_id]['type']); + $this->assertEquals('int', $schema[IMailTemplatesConstants::presentation_id]['type']); + } +} diff --git a/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php index 728896218..f65a6b401 100644 --- a/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php +++ b/tests/Unit/Services/PresentationSubmissionReopenServiceTest.php @@ -22,6 +22,7 @@ use models\exceptions\ValidationException; use models\main\Member; use models\summit\Presentation; +use models\summit\PresentationSpeaker; use models\summit\Summit; use models\summit\SummitEvent; use PHPUnit\Framework\TestCase; @@ -150,6 +151,26 @@ private function presentation(?SelectionPlan $plan = null): Presentation return $presentation; } + private function speaker(int $id, string $email = 'speaker@example.com', string $full_name = 'Grace Hopper'): PresentationSpeaker + { + $speaker = Mockery::mock(PresentationSpeaker::class); + $speaker->shouldReceive('getId')->andReturn($id); + $speaker->shouldReceive('getEmail')->andReturn($email); + $speaker->shouldReceive('getFullName')->andReturn($full_name); + return $speaker; + } + + /** + * A presentation with a live reopen grant, ready for notify() tests. $plan defaults to one + * whose submission window already ended (the precondition isSubmissionReopened() also checks). + */ + private function reopenedPresentation(?SelectionPlan $plan = null): Presentation + { + $presentation = $this->presentation($plan ?? $this->plan($this->utc('-1 hour'))); + $presentation->reopenSubmission(24, $this->member()); + return $presentation; + } + /** * @param Presentation|SummitEvent|null $event what getEvent() hands back */ @@ -555,6 +576,152 @@ public function testCloseNowThrowsEntityNotFoundWhenTheEventIsNotAPresentation() } } + // ------------------------------------------------------------------------- + // notify() + // ------------------------------------------------------------------------- + + public function testNotifyThrowsEntityNotFoundWhenTheEventDoesNotExistInTheSummit(): void + { + $service = $this->makeService(); + + $this->expectException(EntityNotFoundException::class); + $this->expectExceptionMessage('Presentation 1234 not found.'); + + try { + $service->notify($this->summit(null), 1234, [], true, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionWhenNoGrantIsActive(): void + { + $service = $this->makeService(); + // no reopenSubmission() call -- no grant + $presentation = $this->presentation($this->plan($this->utc('-1 hour'))); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Submission is not currently reopened for this presentation.'); + + try { + $service->notify($this->summit($presentation), 1234, [], true, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionWhenThePlanWasExtendedPastNow(): void + { + $service = $this->makeService(); + // submission_end_date in the FUTURE: the grant exists but is not what is admitting the + // speaker anymore -- isSubmissionReopened() must refuse. + $presentation = $this->reopenedPresentation($this->plan($this->utc('+1 hour'))); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Submission is not currently reopened for this presentation.'); + + try { + $service->notify($this->summit($presentation), 1234, [], true, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionOnEmptySelection(): void + { + $service = $this->makeService(); + $presentation = $this->reopenedPresentation(); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Select at least one recipient.'); + + try { + $service->notify($this->summit($presentation), 1234, [], false, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionWhenASpeakerIdIsNotOnThisPresentation(): void + { + // The trust-boundary regression test: id 999 is never added via addSpeaker()/setModerator(), + // so it must not resolve to any recipient even though the request names it explicitly. + $service = $this->makeService(); + $presentation = $this->reopenedPresentation(); + $presentation->addSpeaker($this->speaker(7)); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Speaker(s) 999 are not on this presentation.'); + + try { + $service->notify($this->summit($presentation), 1234, [999], false, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionWhenIncludeSubmitterHasNoCreator(): void + { + $service = $this->makeService(); + $presentation = $this->reopenedPresentation(); + // no setCreatedBy() call -- getCreatedBy() returns null + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('This presentation has no submitter to notify.'); + + try { + $service->notify($this->summit($presentation), 1234, [], true, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyThrowsValidationExceptionWhenEverySelectedRecipientIsBlank(): void + { + $service = $this->makeService(); + $presentation = $this->reopenedPresentation(); + $presentation->addSpeaker($this->speaker(7, '')); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('None of the selected recipients has an email address.'); + + try { + $service->notify($this->summit($presentation), 1234, [7], false, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + public function testNotifyAcceptsAModeratorIdEvenThoughTheModeratorIsNotInGetSpeakers(): void + { + // The moderator is a SEPARATE association from getSpeakers() (Presentation.php:840) -- + // regression test for the exact failure the SDS calls out: dropping the getModerator() + // branch makes every moderator-only id fail the trust-boundary intersect as "not on this + // presentation" even though the moderator genuinely is. A blank email on the moderator (not + // added via addSpeaker()) still reaches the "no usable email" branch rather than the + // trust-boundary one, which is only possible if id 9 was recognized as allowed. + $service = $this->makeService(); + $presentation = $this->reopenedPresentation(); + $presentation->setModerator($this->speaker(9, '')); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('None of the selected recipients has an email address.'); + + try { + $service->notify($this->summit($presentation), 1234, [9], false, $this->member()); + } finally { + $this->assertClosureRan(); + } + } + + // Success-path assertions (queued count, skipped count, "only selected people queued", + // moderator/submitter dedup) are NOT tested here: PresentationSubmissionReopenedEmail's + // constructor calls Presentation::getSummit() and, via AbstractSummitEmailJob::__construct(), + // App::make(ISummitRepository::class) -- a service-locator dependency chain this bare-container + // Mockery harness cannot satisfy. Matches the SDS's own placement rationale (per-file docblock + // above): queueing assertions live in tests/PresentationReopenApiTest.php, which boots the full + // app and can Queue::fake(). + // ------------------------------------------------------------------------- // Guard on the harness itself // ------------------------------------------------------------------------- From 9a011f0711144a144e4f6353d33cbf869cb298dc Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 11:45:31 -0300 Subject: [PATCH 2/7] fix: correct notify rate-limit window to a real hour rate.limit:30,60 was 30 per MINUTE, not per hour. RateLimitMiddleware overrides handle() and passes its $decayMinutes argument straight into RateLimiter::hit($key, $decaySeconds), skipping the 60 * $decayMinutes conversion Laravel's own ThrottleRequests::handle() performs, so the second rate.limit argument is effectively seconds in this codebase. Verified against Redis: hit(key, 60) yields a 60s TTL, hit(key, 3600) yields 3600s. Changed to rate.limit:30,3600 for the 30-per-hour ceiling the SDS specifies, and documented the seconds semantics at both the route and the migration docblock so it does not get 'corrected' back to 60. --- database/migrations/config/Version20260824100000.php | 10 +++++++++- routes/api_v1.php | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/database/migrations/config/Version20260824100000.php b/database/migrations/config/Version20260824100000.php index f58ac5f21..4204030d4 100644 --- a/database/migrations/config/Version20260824100000.php +++ b/database/migrations/config/Version20260824100000.php @@ -29,13 +29,21 @@ * * Rate limiting for this endpoint (unlike reopen/close, which are idempotent state changes, * this one sends mail to real people and is re-sendable by design) is enforced via the - * `rate.limit:30,60` middleware on the route itself (routes/api_v1.php), NOT via the + * `rate.limit:30,3600` middleware on the route itself (routes/api_v1.php), NOT via the * api_endpoints.rate_limit/rate_limit_decay columns -- RateLimitMiddleware.php's block that * would read those columns off the matched endpoint is commented out (dead code), so setting * them here would have no runtime effect. Route-level rate.limit is the only mechanism this * codebase actually enforces (precedent: routes/api_v1.php's `discover` and * `preValidatePromoCode` promo-code routes). * + * The `3600` is SECONDS and is deliberate: RateLimitMiddleware overrides handle() and passes its + * $decayMinutes argument straight into RateLimiter::hit($key, $decaySeconds), skipping the + * `60 * $decayMinutes` conversion Laravel's own ThrottleRequests::handle() performs. So the + * second rate.limit argument is effectively seconds in this codebase, and `30,60` -- which is + * what "30 per hour" looks like if you trust the parameter name -- would actually cap at 30 per + * MINUTE. The two existing `rate.limit:25,1` promo-code routes are 25-per-second for the same + * reason. + * * DEPLOY ORDER: this migration must land with or before the application. auth.user reads these * rows, and an endpoint registered without them 403s every authenticated member. * diff --git a/routes/api_v1.php b/routes/api_v1.php index 0132c00b1..537d27590 100644 --- a/routes/api_v1.php +++ b/routes/api_v1.php @@ -867,7 +867,10 @@ Route::group(['prefix' => 'reopen'], function () { Route::put('', ['middleware' => 'auth.user', 'uses' => 'OAuth2PresentationApiController@reopenSubmissionPeriod']); Route::delete('', ['middleware' => 'auth.user', 'uses' => 'OAuth2PresentationApiController@closeSubmissionPeriod']); - Route::put('notify', ['middleware' => ['auth.user', 'rate.limit:30,60'], 'uses' => 'OAuth2PresentationApiController@notifySubmissionReopened']); + // 30 sends per hour. The second arg is SECONDS, not minutes: RateLimitMiddleware + // passes it straight into RateLimiter::hit($key, $decaySeconds) without the + // 60x that Laravel's own ThrottleRequests applies. Do not "correct" 3600 to 60. + Route::put('notify', ['middleware' => ['auth.user', 'rate.limit:30,3600'], 'uses' => 'OAuth2PresentationApiController@notifySubmissionReopened']); }); }); From 8f15ebee1dfdf3669b9fe0367b253a932ed29b70 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 12:10:58 -0300 Subject: [PATCH 3/7] test: run the reopen notification email test in CI tests/PresentationSubmissionReopenedEmailTest.php sits at the tests/ root, and no job in the matrix runs that root - only its subdirectories - so the file ran nowhere in CI despite passing locally. The workflow already carries a comment warning about exactly this; the new file just was not added to the list it points at. Added to the PresentationMediaUploads shard alongside the other reopen test files. Full shard verified locally: 70 tests, 467 assertions, green. --- .github/workflows/push.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 0e665c5b0..07c8c2f3d 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -69,7 +69,7 @@ jobs: - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } # Named by path because no job in this matrix runs the tests/ root, only its # 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" } + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php tests/PresentationReopenModelTest.php tests/PresentationReopenApiTest.php tests/PresentationReopenAuthzTest.php tests/PresentationSubmissionReopenedEmailTest.php" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing From c227ead2baa78d8aec2be671e383b1cbac5fea5d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 12:18:41 -0300 Subject: [PATCH 4/7] test: prove the notify trust boundary with a real foreign speaker The existing trust-boundary test passes a nonexistent speaker id, which cannot detect the regression it exists for. SDS section 10 calls this out: 'Include a same-summit foreign speaker, not just a nonexistent id.' The regression is a refactor of notify() that resolves speaker_ids through the repository instead of intersecting them against the presentation's own speakers and moderator. A nonexistent id is refused under both the correct and the broken implementation, so that test stays green either way. A speaker that genuinely resolves separates them: under a repository lookup it would resolve and be mailed, which is the authenticated-mail-relay failure the trust boundary prevents. Verified by mutation: with the intersect replaced by a repository lookup, the nonexistent-id test still passes while this one fails with 200 instead of 412, having mailed the foreign speaker. --- tests/PresentationReopenApiTest.php | 72 +++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/tests/PresentationReopenApiTest.php b/tests/PresentationReopenApiTest.php index 043eb7b8f..2a004e3d9 100644 --- a/tests/PresentationReopenApiTest.php +++ b/tests/PresentationReopenApiTest.php @@ -17,6 +17,7 @@ use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\Registry; use ModelSerializers\SerializerRegistry; +use models\main\Member; use models\summit\Presentation; use models\summit\PresentationSpeaker; use models\summit\SummitEvent; @@ -128,6 +129,30 @@ private function speakerWithNoEmail(): PresentationSpeaker return $speaker; } + /** + * A speaker backed by its own Member, so PresentationSpeaker::getEmail() returns a real + * address. The local part is randomized on purpose: Member.Email carries a case-insensitive + * unique index, and clearSummitTestData() does not remove Members, so a fixed address would + * collide with the row left behind by a previous run of this suite against the same database. + */ + private function speakerWithEmail(string $first_name, string $last_name): PresentationSpeaker + { + $member = new Member(); + $member->setEmail(sprintf("%s@example.com", strtolower(str_random(16)))); + $member->setActive(true); + $member->setFirstName($first_name); + $member->setLastName($last_name); + $member->setEmailVerified(true); + $member->setUserExternalId(mt_rand()); + + $speaker = new PresentationSpeaker(); + $speaker->setMember($member); + + self::$em->persist($member); + self::$em->persist($speaker); + return $speaker; + } + /** * Reload from the DB rather than reading the response body. * @@ -1102,6 +1127,53 @@ public function testNotifyRejectsASpeakerIdNotOnThisPresentationAndQueuesNothing Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); } + /** + * The trust-boundary test proper, and the reason the nonexistent-id case above is not enough + * on its own (SDS §10: "Include a same-summit foreign speaker, not just a nonexistent id"). + * + * The regression this guards is a refactor of notify() that resolves $speaker_ids through the + * speaker repository instead of intersecting them against the presentation's own people. A + * nonexistent id cannot detect that: the repository returns null for it either way, so the + * request is refused under both the correct and the broken implementation and the test stays + * green. Only a speaker that genuinely resolves separates them -- under a repository lookup + * this one would resolve, pass whatever null check replaced the intersect, and BE MAILED, + * which is the authenticated-mail-relay failure the trust boundary exists to prevent. + * + * Same summit and attached to a real sibling presentation, so nothing but "not on THIS + * presentation" can be what refuses it. + */ + public function testNotifyRejectsARealSpeakerFromAnotherPresentationInTheSameSummit() + { + Queue::fake(); + $this->grantWindow(24); + + $foreign = $this->speakerWithEmail("Foreign", "Speaker"); + + $sibling = new Presentation(); + $sibling->setTitle("ANOTHER PRESENTATION IN THIS SUMMIT"); + $sibling->setType(self::$defaultPresentationType); + $sibling->setSelectionPlan(self::$default_selection_plan); + $sibling->setCategory(self::$defaultTrack); + $sibling->addSpeaker($foreign); + self::$summit->addEvent($sibling); + self::$em->flush(); + + // Preconditions, or the 412 below could be the "no usable email" branch (or a plain + // unknown id) rather than the trust boundary actually doing its job. + $this->assertNotEmpty($foreign->getId(), 'foreign speaker did not persist'); + $this->assertNotEmpty($foreign->getEmail(), 'foreign speaker has no email; it would be skipped, not refused'); + $this->assertNotEquals( + self::$presentation->getId(), + $sibling->getId(), + 'the foreign speaker must be on a DIFFERENT presentation' + ); + + $response = $this->notify(['speaker_ids' => [$foreign->getId()]]); + + $this->assertResponseStatus(412); + Queue::assertNotPushed(PresentationSubmissionReopenedEmail::class); + } + public function testNotifyRejectsAnEmptySelection() { Queue::fake(); From d23a91ba1a84b9a997f09d6cfd9ec6ebd6853389 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 13:20:24 -0300 Subject: [PATCH 5/7] fix: make the reopen notification flow-type migration re-run-safe SummitEmailFlowTypeSeeder::createEventsTypes() inserts unconditionally and SummitEmailEventFlowType.Slug carries no unique index, so executing Version20260824090000 a second time (migrations:execute --up, a restored doctrine_migration_versions table) left two rows for the slug and the Email Flow Events page listed the event twice. The PR described both migrations as safe to re-run; only the config one was. Guard on the slug before calling the seeder helper. Regression test starts from the deployed precondition (no row), runs up() twice and expects one row, so a guard that always returned would fail it too. Verified live against the local model database: rows 0 -> 1 -> 1 across two executions, both recorded in DoctrineMigration. --- .../model/Version20260824090000.php | 12 +++++++ ...resentationSubmissionReopenedEmailTest.php | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/database/migrations/model/Version20260824090000.php b/database/migrations/model/Version20260824090000.php index 5292a9b70..c9d19b023 100644 --- a/database/migrations/model/Version20260824090000.php +++ b/database/migrations/model/Version20260824090000.php @@ -13,6 +13,7 @@ **/ use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; +use App\Models\Foundation\Summit\EmailFlows\SummitEmailEventFlowType; use App\Models\Foundation\Summit\EmailFlows\SummitEmailFlowType; use Database\Seeders\SummitEmailFlowTypeSeeder; use Doctrine\DBAL\Schema\Schema; @@ -43,6 +44,12 @@ * seeders, so the "Presentation Submissions" flow does not exist yet at migration time -- the * is_null($flow) guard below simply returns, and SummitEmailFlowTypeSeeder creates both the flow * and this event type together afterwards. + * + * Re-run-safe, unlike the precedent: createEventsTypes() inserts unconditionally and + * SummitEmailEventFlowType.Slug has no unique index, so a second execution (migrations:execute + * --up, a restored doctrine_migration_versions table) would leave two rows for the slug and the + * Email Flow Events page would list the event twice. The slug lookup below makes the second run a + * no-op. Regression test: PresentationSubmissionReopenedEmailTest::testModelMigrationInsertsOnce... */ final class Version20260824090000 extends AbstractMigration { @@ -61,6 +68,11 @@ public function up(Schema $schema): void ]); if (is_null($flow)) return; + $existing = $em->getRepository(SummitEmailEventFlowType::class)->findOneBy([ + "slug" => PresentationSubmissionReopenedEmail::EVENT_SLUG + ]); + if (!is_null($existing)) return; + SummitEmailFlowTypeSeeder::createEventsTypes( [ [ diff --git a/tests/PresentationSubmissionReopenedEmailTest.php b/tests/PresentationSubmissionReopenedEmailTest.php index 20fca5dc3..e15f428c8 100644 --- a/tests/PresentationSubmissionReopenedEmailTest.php +++ b/tests/PresentationSubmissionReopenedEmailTest.php @@ -16,7 +16,10 @@ use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; use App\Models\Foundation\Summit\EmailFlows\SummitEmailEventFlowType; use App\Models\Foundation\Summit\EmailFlows\SummitEmailFlowType; +use Database\Migrations\Model\Version20260824090000; +use Doctrine\DBAL\Schema\Schema; use models\summit\Presentation; +use Psr\Log\NullLogger; use ReflectionProperty; /** @@ -148,6 +151,37 @@ public function testUnparseableTimeZoneFallsBackToUtcWithoutThrow() $this->assertEquals($expected, $payload[IMailTemplatesConstants::until_date]); } + /** + * The deployed-database registration path must be re-run-safe. SummitEmailFlowTypeSeeder:: + * createEventsTypes() inserts unconditionally and SummitEmailEventFlowType.Slug carries no + * unique index, so an unguarded migration executed twice (migrations:execute --up, a restored + * doctrine_migration_versions table) leaves two rows for the slug and the Email Flow Events + * page lists the event twice. Starts from the deployed precondition (no row for the slug), so + * it also proves the first run inserts -- a guard that always returns would fail here too. + */ + public function testModelMigrationInsertsOnceAndDoesNotDuplicateTheEventTypeOnRerun() + { + $repo = self::$em->getRepository(SummitEmailEventFlowType::class); + $flow = self::$em->getRepository(SummitEmailFlowType::class)->findOneBy(['name' => 'Presentation Submissions']); + $this->assertNotNull($flow); + + // orphanRemoval on SummitEmailFlowType::$flow_event_types deletes the row through the ORM, + // keeping the identity map consistent (a DQL delete would leave a stale managed entity). + foreach ($repo->findBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]) as $existing) + $flow->removeFlowEventType($existing); + self::$em->flush(); + $this->assertCount(0, $repo->findBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG])); + + // migration classes are discovered by Doctrine's finder, not composer's autoloader + require_once base_path('database/migrations/model/Version20260824090000.php'); + $migration = new Version20260824090000(self::$em->getConnection(), new NullLogger()); + $migration->up(new Schema()); + $this->assertCount(1, $repo->findBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]), 'first run must insert the row'); + + $migration->up(new Schema()); + $this->assertCount(1, $repo->findBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]), 'second run must not duplicate the row'); + } + public function testGetEmailTemplateSchemaDeclaresAllKeys() { $schema = PresentationSubmissionReopenedEmail::getEmailTemplateSchema(); From 892eb6aa8145168c95e50ff3bdda022814bb323a Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 13:20:30 -0300 Subject: [PATCH 6/7] fix: fail over reopen notification dispatch to the database queue A queue-backend failure part-way through notify()'s dispatch loop aborted the request with some recipients already queued, so the operator's retry mailed them twice (CodeRabbit, PR #590). Dispatch through JobDispatcher::withDbFallback(), the codebase's existing helper, which falls over to the database queue and runs the job synchronously on a double failure, so the loop completes and the returned count stays true. The job is still constructed before any push, so a missing SummitEmailEventFlowType row or cfp.support_email fails the request loudly instead of reporting a false "queued" -- the reason a bare try/catch was not the fix. primaryConnection follows queue.default so this mail is routed like every other AbstractEmailJob rather than the helper's hardcoded redis. --- .../Imp/PresentationSubmissionReopenService.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/app/Services/Model/Imp/PresentationSubmissionReopenService.php b/app/Services/Model/Imp/PresentationSubmissionReopenService.php index caec94bde..27f6ec34c 100644 --- a/app/Services/Model/Imp/PresentationSubmissionReopenService.php +++ b/app/Services/Model/Imp/PresentationSubmissionReopenService.php @@ -13,6 +13,7 @@ **/ use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; +use App\Jobs\Utils\JobDispatcher; use App\Services\Model\AbstractService; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; @@ -221,8 +222,20 @@ function () use ($summit, $presentation_id, $speaker_ids, $include_submitter) { } ); + // JobDispatcher, not ::dispatch(): a queue-backend failure part-way through this loop would + // otherwise abort the request with some recipients already queued, and the operator's retry + // would mail them twice. withDbFallback() fails over to the database queue (and runs sync on + // a double failure) so the loop completes and the returned count stays true. The job is + // constructed here, before any push, so a missing SummitEmailEventFlowType row or + // cfp.support_email still fails the request loudly rather than reporting a false "queued". + // primaryConnection follows queue.default so this mail is routed like every other + // AbstractEmailJob instead of JobDispatcher's hardcoded redis primary. foreach ($recipients as $email => $name) - PresentationSubmissionReopenedEmail::dispatch($presentation, $email, $name ?? ''); + JobDispatcher::withDbFallback( + job: new PresentationSubmissionReopenedEmail($presentation, $email, $name ?? ''), + logContext: ['summit_id' => $summit->getId(), 'presentation_id' => $presentation_id], + primaryConnection: Config::get('queue.default') + ); // Report queued and skipped, NOT "queued of selected". Selection is counted in rows by the // client and in ids by the server, and one merged row (a submitter who is also a speaker) From b29ca46313a51cb61e9ff5d0c4961622affb413d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 25 Aug 2026 14:10:03 -0300 Subject: [PATCH 7/7] feat: backfill the reopen notification email event onto every summit Version20260824090000 registers the SummitEmailEventFlowType, which is enough for the email to send (getEmailIdentifierPerEmailEventFlowSlug falls back to the type's default template), but not for it to be listed or overridable per show: GET /summits/{id}/email-flows-events lists per-summit SummitEmailEventFlow rows, not types, so existing shows never saw the new event on their Email Flow Events page. Every earlier event-type migration ships with a seedDefaultEmailFlowEvents() backfill companion (Version20250812201257 + Version20250812201307, the precedent the type migration copies); this adds the missing half. The seeder path the PR called "optional" never runs on a k8s deploy, so the migration is the only way existing summits get the row. Idempotent: seedDefaultEmailFlowEvents() creates a row only when getEmailEventByType() is null, so a re-run is a no-op and existing overrides are untouched. Regression test starts from a summit with no row, runs up() twice and expects 0 -> 1 -> 1. --- .../model/Version20260824090001.php | 69 +++++++++++++++++++ ...resentationSubmissionReopenedEmailTest.php | 46 +++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 database/migrations/model/Version20260824090001.php diff --git a/database/migrations/model/Version20260824090001.php b/database/migrations/model/Version20260824090001.php new file mode 100644 index 000000000..1f8a9ef5b --- /dev/null +++ b/database/migrations/model/Version20260824090001.php @@ -0,0 +1,69 @@ +getRepository(Summit::class); + $summits = $repository->findAll(); + foreach ($summits as $summit) { + $summit->seedDefaultEmailFlowEvents(); + $em->persist($summit); + } + $em->flush(); + } + + /** + * Deliberate no-op, matching Version20250812201307 and the type migration's down(): deleting + * SummitEmailEventFlow rows would destroy any per-summit template override an operator has + * already authored against them. + */ + public function down(Schema $schema): void + { + } +} diff --git a/tests/PresentationSubmissionReopenedEmailTest.php b/tests/PresentationSubmissionReopenedEmailTest.php index e15f428c8..2a3edc604 100644 --- a/tests/PresentationSubmissionReopenedEmailTest.php +++ b/tests/PresentationSubmissionReopenedEmailTest.php @@ -14,9 +14,11 @@ use App\Jobs\Emails\IMailTemplatesConstants; use App\Jobs\Emails\PresentationSubmissions\PresentationSubmissionReopenedEmail; +use App\Models\Foundation\Summit\EmailFlows\SummitEmailEventFlow; use App\Models\Foundation\Summit\EmailFlows\SummitEmailEventFlowType; use App\Models\Foundation\Summit\EmailFlows\SummitEmailFlowType; use Database\Migrations\Model\Version20260824090000; +use Database\Migrations\Model\Version20260824090001; use Doctrine\DBAL\Schema\Schema; use models\summit\Presentation; use Psr\Log\NullLogger; @@ -182,6 +184,50 @@ public function testModelMigrationInsertsOnceAndDoesNotDuplicateTheEventTypeOnRe $this->assertCount(1, $repo->findBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]), 'second run must not duplicate the row'); } + /** + * The per-summit backfill companion to the type migration above. seedDefaultEmailFlowEvents() + * creates a SummitEmailEventFlow only when getEmailEventByType() is null, so this proves both + * halves the deploy relies on: the first run gives an existing summit its row for the new event + * (what makes it visible on that show's Email Flow Events page), and a second run leaves that + * row alone rather than adding a duplicate or replacing an operator's override. + */ + public function testPerSummitBackfillMigrationCreatesTheEventFlowOnceAndDoesNotDuplicateOnRerun() + { + $type = self::$em->getRepository(SummitEmailEventFlowType::class) + ->findOneBy(['slug' => PresentationSubmissionReopenedEmail::EVENT_SLUG]); + $this->assertNotNull($type, 'type row must exist (seeded or created by setUp())'); + + // deployed precondition: an existing summit with no per-summit row for the new event + $existing = self::$summit->getEmailEventByType($type); + if (!is_null($existing)) self::$summit->removeEmailEventFlow($existing); // orphanRemoval deletes it + self::$em->flush(); + self::$em->clear(); + + $countFor = function () use ($type): int { + return count(self::$em->getRepository(SummitEmailEventFlow::class)->findBy([ + 'summit' => self::$summit->getId(), + 'event_type' => $type->getId(), + ])); + }; + $this->assertSame(0, $countFor()); + + require_once base_path('database/migrations/model/Version20260824090001.php'); + $migration = new Version20260824090001(self::$em->getConnection(), new NullLogger()); + $migration->up(new Schema()); + self::$em->clear(); + $this->assertSame(1, $countFor(), 'first run must create the per-summit row'); + + $row = self::$em->getRepository(SummitEmailEventFlow::class)->findOneBy([ + 'summit' => self::$summit->getId(), + 'event_type' => $type->getId(), + ]); + $this->assertEquals(PresentationSubmissionReopenedEmail::DEFAULT_TEMPLATE, $row->getEmailTemplateIdentifier()); + + $migration->up(new Schema()); + self::$em->clear(); + $this->assertSame(1, $countFor(), 'second run must not duplicate the per-summit row'); + } + public function testGetEmailTemplateSchemaDeclaresAllKeys() { $schema = PresentationSubmissionReopenedEmail::getEmailTemplateSchema();