From ba12b478136616b6f4535a6639d5f4060da8acb4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 24 Aug 2026 13:21:35 -0300 Subject: [PATCH 1/6] feat(presentations): notify support when a speaker or moderator changes on a published activity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a stopgap email notification whenever a speaker or moderator is added to or removed from an already-published presentation ("activity"), so support staff can manually track promo-code/ticket fallout until the automated sync described in the parent ticket exists. New PresentationActivitySpeakerChangeEmail job (extends AbstractSummitEmailJob) sent to a single platform-wide recipient read from a new cfp.speaker_change_notification_email config value (CFP_SPEAKER_CHANGE_NOTIFICATION_EMAIL) — no SummitEmailEventFlow entry, per explicit product guidance that per-summit customization isn't needed yet. Dispatch is hooked at every place a published presentation's speaker/ moderator set changes: - PresentationService::upsertPresentationSpeaker/removeSpeakerFromPresentation (single add/remove admin endpoints), on a genuine state transition only. - SummitService::addModerator2Presentation/removeModeratorFromPresentation (self-service moderator assign/unassign). - SummitService::saveOrUpdatePresentationData/saveOrUpdateEvent (bulk admin "save presentation" form), diffing old vs. new speaker IDs and moderator. All hooks are guarded by Presentation::isPublished() and dispatch only after their surrounding transaction commits — discovered mid-implementation that dispatching inline (which triggers AbstractSummitEmailJob's getByIdRefreshed(), a mid-request Doctrine EntityManager::refresh()) can corrupt entity state for later requests in the same process. Explicitly out of scope: MemberService::registerExternalUserByPayload's account-merge speaker reassignment (not a user-driven activity edit). A pre-existing, unrelated Doctrine entity-staleness bug in OAuth2PresentationApiController's PresentationSerializer admin-check (commit 28ae09584) was found during verification — reproduced identically on unmodified code via git stash — and documented but not fixed here. --- .env.example | 1 + app/Jobs/Emails/IMailTemplatesConstants.php | 2 + ...PresentationActivitySpeakerChangeEmail.php | 96 ++++++ .../Model/Imp/PresentationService.php | 43 ++- app/Services/Model/Imp/SummitService.php | 118 +++++++- config/cfp.php | 1 + ...entationActivitySpeakerChangeEmailTest.php | 68 +++++ tests/oauth2/OAuth2PresentationApiTest.php | 142 +++++++++ tests/oauth2/OAuth2SummitEventsApiTest.php | 285 ++++++++++++++++++ tests/oauth2/OAuth2SummitSpeakersApiTest.php | 166 ++++++++++ 10 files changed, 913 insertions(+), 9 deletions(-) create mode 100644 app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php create mode 100644 tests/Unit/Jobs/PresentationActivitySpeakerChangeEmailTest.php diff --git a/.env.example b/.env.example index d08ef75a8..8d4fa1b52 100644 --- a/.env.example +++ b/.env.example @@ -154,6 +154,7 @@ MAIL_API_OAUTH2_CLIENT_SCOPES= CFP_APP_BASE_URL= CFP_SUPPORT_EMAIL= +CFP_SPEAKER_CHANGE_NOTIFICATION_EMAIL= CFP_OAUTH2_SCOPES= CFP_OAUTH2_CLIENT_ID= # ceiling and default for an admin-granted per-presentation submission reopen window, in hours diff --git a/app/Jobs/Emails/IMailTemplatesConstants.php b/app/Jobs/Emails/IMailTemplatesConstants.php index d6a59b4d6..e11aac0ef 100644 --- a/app/Jobs/Emails/IMailTemplatesConstants.php +++ b/app/Jobs/Emails/IMailTemplatesConstants.php @@ -20,6 +20,8 @@ interface IMailTemplatesConstants { const accepted_moderated_presentations = 'accepted_moderated_presentations'; const accepted_presentations = 'accepted_presentations'; + const activity_change_action = 'activity_change_action'; + const activity_change_role = 'activity_change_role'; const admin_ticket_edit_url = 'admin_ticket_edit_url'; const alternate_moderated_presentations = 'alternate_moderated_presentations'; const alternate_presentations = 'alternate_presentations'; diff --git a/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php b/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php new file mode 100644 index 000000000..68b889253 --- /dev/null +++ b/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php @@ -0,0 +1,96 @@ +getSummit(); + + $payload = []; + $payload[IMailTemplatesConstants::speaker_full_name] = $speaker->getFullName(" "); + $payload[IMailTemplatesConstants::speaker_email] = $speaker->getEmail(); + $payload[IMailTemplatesConstants::presentation_title] = $presentation->getTitle(); + $payload[IMailTemplatesConstants::presentation_id] = $presentation->getId(); + $payload[IMailTemplatesConstants::presentation_edit_link] = $presentation->getEditLink(); + $payload[IMailTemplatesConstants::activity_change_role] = $role; + $payload[IMailTemplatesConstants::activity_change_action] = $action; + + $to_email = Config::get('cfp.speaker_change_notification_email'); + if (empty($to_email)) + throw new ValidationException('cfp.speaker_change_notification_email is not configured.'); + + parent::__construct($summit, $payload, self::DEFAULT_TEMPLATE, $to_email); + } + + /** + * @return array + */ + public static function getEmailTemplateSchema(): array{ + + $payload = parent::getEmailTemplateSchema(); + + $payload[IMailTemplatesConstants::speaker_full_name]['type'] = 'string'; + $payload[IMailTemplatesConstants::speaker_email]['type'] = 'string'; + $payload[IMailTemplatesConstants::presentation_title]['type'] = 'string'; + $payload[IMailTemplatesConstants::presentation_id]['type'] = 'int'; + $payload[IMailTemplatesConstants::presentation_edit_link]['type'] = 'string'; + $payload[IMailTemplatesConstants::activity_change_role]['type'] = 'string'; + $payload[IMailTemplatesConstants::activity_change_action]['type'] = 'string'; + + return $payload; + } +} diff --git a/app/Services/Model/Imp/PresentationService.php b/app/Services/Model/Imp/PresentationService.php index 4df302eea..1f3e3ebdd 100644 --- a/app/Services/Model/Imp/PresentationService.php +++ b/app/Services/Model/Imp/PresentationService.php @@ -17,6 +17,7 @@ use App\Http\Utils\FileUploadInfo; use App\Http\Utils\IFileUploader; use App\Jobs\Emails\PresentationSubmissions\PresentationCreatorNotificationEmail; +use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Models\Exceptions\AuthzException; use App\Models\Foundation\Summit\Events\Presentations\TrackChairs\PresentationTrackChairScore; use App\Models\Foundation\Summit\Events\Presentations\TrackChairs\PresentationTrackChairScoreType; @@ -1725,7 +1726,9 @@ public function processMediaUpload(int $summit_id, int $media_upload_type_id, ?s * @throws \Exception */ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, int $speaker_id, array $data): Presentation { - return $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data) { + $pending_notification = null; + + $presentation = $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data, &$pending_notification) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1737,6 +1740,14 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, if (!$presentation->isSpeaker($speaker)) { $presentation->addSpeaker($speaker); + if ($presentation->isPublished()) { + $pending_notification = [ + $presentation, + $speaker, + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Added + ]; + } } if (isset($data['order'])) { @@ -1747,6 +1758,14 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, return $presentation; }); + + // dispatched only after the transaction above has committed, so a queued notification + // never outlives a save that ends up rolling back + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } + + return $presentation; } /** @@ -1758,7 +1777,9 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, */ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_id, int $speaker_id): void { - $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id) { + $pending_notification = null; + + $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, &$pending_notification) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1768,8 +1789,24 @@ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_ if (is_null($speaker) || !($speaker instanceof PresentationSpeaker)) throw new EntityNotFoundException("Speaker {$speaker_id} not found."); - $presentation->removeSpeaker($speaker); + if ($presentation->isSpeaker($speaker)) { + $presentation->removeSpeaker($speaker); + if ($presentation->isPublished()) { + $pending_notification = [ + $presentation, + $speaker, + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + } }); + + // dispatched only after the transaction above has committed, so a queued notification + // never outlives a save that ends up rolling back + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } } } diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 155427fcd..ff5d2a87c 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -24,6 +24,7 @@ use App\Jobs\Emails\PresentationSubmissions\ImportEventSpeakerEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationModeratorNotificationEmail; use App\Jobs\Emails\PresentationSubmissions\PresentationSpeakerNotificationEmail; +use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Jobs\Emails\Schedule\ShareEventEmail; use App\Jobs\EncryptAllSummitBadgeQRCodes; use App\Jobs\ProcessEventDataImport; @@ -662,7 +663,9 @@ private function canPerformEventTypeTransition(SummitEventType $old_event_type, */ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null, bool $trigger_data_update = true, bool $saveAsIncomplete = false) { - return $this->tx_service->transaction(function () use ($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete) { + $pending_speaker_changes = []; + + $event = $this->tx_service->transaction(function () use ($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete, &$pending_speaker_changes) { Log::debug ( @@ -833,7 +836,7 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null } } - $this->saveOrUpdatePresentationData($event, $event_type, $data, $saveAsIncomplete); + $pending_speaker_changes = $this->saveOrUpdatePresentationData($event, $event_type, $data, $saveAsIncomplete); $this->saveOrUpdateSummitGroupEventData($event, $event_type, $data); if (!$event_type->isAllowsLocation()) @@ -861,6 +864,14 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null return $event; }); + + // dispatched only after the transaction above has committed, so a queued notification + // never outlives a save that ends up rolling back + foreach ($pending_speaker_changes as $change) { + PresentationActivitySpeakerChangeEmail::dispatch($event, $change['speaker'], $change['role'], $change['action']); + } + + return $event; } private function saveOrUpdateSummitGroupEventData(SummitEvent $event, SummitEventType $event_type, array $data) @@ -883,17 +894,29 @@ private function saveOrUpdateSummitGroupEventData(SummitEvent $event, SummitEven * @param SummitEventType $event_type * @param array $data * @param bool $saveAsIncomplete + * @return array list of pending speaker/moderator change notifications, each + * ['speaker' => PresentationSpeaker, 'role' => string, 'action' => string]. + * The caller must dispatch these only after its own transaction commits. * @throws EntityNotFoundException * @throws ValidationException */ - private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventType $event_type, array $data, bool $saveAsIncomplete = false) + private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventType $event_type, array $data, bool $saveAsIncomplete = false): array { - if (!$event instanceof Presentation) return; + if (!$event instanceof Presentation) return []; Log::debug(sprintf("SummitService::saveOrUpdatePresentationData presentation %s saveAsIncomplete %b", $event->getId(), $saveAsIncomplete)); if ($saveAsIncomplete && $event->isPublished()) throw new ValidationException('Cannot save a published event as incomplete.'); + // captured before any mutation below: a published presentation being edited is a + // "change", not a creation, so notifications are only considered when this was + // already true on entry + $was_published = $event->isPublished(); + $old_speaker_ids = array_map(fn(PresentationSpeaker $s) => $s->getId(), $event->getSpeakers()->toArray()); + $old_moderator = $event->hasModerator() ? $event->getModerator() : null; + + $pending_changes = []; + if (!$saveAsIncomplete || $event->isNew()) { // if we are creating the presentation from admin, then // we should mark it as received and complete @@ -929,6 +952,24 @@ private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventTyp $event->addSpeaker($speaker); } } + + if ($was_published) { + $new_speaker_ids = array_map(fn(PresentationSpeaker $s) => $s->getId(), $event->getSpeakers()->toArray()); + foreach (array_diff($new_speaker_ids, $old_speaker_ids) as $added_id) { + $pending_changes[] = [ + 'speaker' => $this->speaker_repository->getById($added_id), + 'role' => PresentationActivitySpeakerChangeEmail::Role_Speaker, + 'action' => PresentationActivitySpeakerChangeEmail::Action_Added + ]; + } + foreach (array_diff($old_speaker_ids, $new_speaker_ids) as $removed_id) { + $pending_changes[] = [ + 'speaker' => $this->speaker_repository->getById($removed_id), + 'role' => PresentationActivitySpeakerChangeEmail::Role_Speaker, + 'action' => PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + } } // moderator @@ -952,9 +993,34 @@ private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventTyp throw new EntityNotFoundException(sprintf('Moderator %s not found', $moderator_id)); $event->setModerator($moderator); } + + if ($was_published) { + $new_moderator = $event->hasModerator() ? $event->getModerator() : null; + $old_moderator_id = is_null($old_moderator) ? null : $old_moderator->getId(); + $new_moderator_id = is_null($new_moderator) ? null : $new_moderator->getId(); + + if ($old_moderator_id !== $new_moderator_id) { + if (!is_null($old_moderator)) { + $pending_changes[] = [ + 'speaker' => $old_moderator, + 'role' => PresentationActivitySpeakerChangeEmail::Role_Moderator, + 'action' => PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + if (!is_null($new_moderator)) { + $pending_changes[] = [ + 'speaker' => $new_moderator, + 'role' => PresentationActivitySpeakerChangeEmail::Role_Moderator, + 'action' => PresentationActivitySpeakerChangeEmail::Action_Added + ]; + } + } + } } PresentationFactory::populate($event, $data, true); + + return $pending_changes; } /** @@ -1833,7 +1899,9 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak */ public function addModerator2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - return $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id) { + $pending_notification = null; + + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1864,6 +1932,15 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i $presentation->setModerator($speaker); + if ($presentation->isPublished()) { + $pending_notification = [ + $presentation, + $speaker, + PresentationActivitySpeakerChangeEmail::Role_Moderator, + PresentationActivitySpeakerChangeEmail::Action_Added + ]; + } + // check is selection plan is private, if so add moderator to allowed members $selection_plan = $presentation->getSelectionPlan(); @@ -1877,6 +1954,14 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i return $presentation; }); + + // dispatched only after the transaction above has committed, so a queued notification + // never outlives a save that ends up rolling back + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } + + return $presentation; } /** @@ -1889,7 +1974,9 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i */ public function removeModeratorFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - return $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id) { + $pending_notification = null; + + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -1919,10 +2006,29 @@ public function removeModeratorFromPresentation(int $current_member_id, int $spe if (!$presentation->isCompleted()) $presentation->setProgress(Presentation::PHASE_SPEAKERS); + $previous_moderator = $presentation->hasModerator() ? $presentation->getModerator() : null; + $presentation->unsetModerator(); + if (!is_null($previous_moderator) && $presentation->isPublished()) { + $pending_notification = [ + $presentation, + $previous_moderator, + PresentationActivitySpeakerChangeEmail::Role_Moderator, + PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + return $presentation; }); + + // dispatched only after the transaction above has committed, so a queued notification + // never outlives a save that ends up rolling back + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } + + return $presentation; } /** diff --git a/config/cfp.php b/config/cfp.php index 54b8dc9ea..9f866e840 100644 --- a/config/cfp.php +++ b/config/cfp.php @@ -15,6 +15,7 @@ return [ 'base_url' => env('CFP_APP_BASE_URL', null), 'support_email' => env('CFP_SUPPORT_EMAIL', null), + 'speaker_change_notification_email' => env('CFP_SPEAKER_CHANGE_NOTIFICATION_EMAIL', null), 'client_id' => env('CFP_OAUTH2_CLIENT_ID', null), 'scopes' => env('CFP_OAUTH2_SCOPES', null), diff --git a/tests/Unit/Jobs/PresentationActivitySpeakerChangeEmailTest.php b/tests/Unit/Jobs/PresentationActivitySpeakerChangeEmailTest.php new file mode 100644 index 000000000..15e438ccf --- /dev/null +++ b/tests/Unit/Jobs/PresentationActivitySpeakerChangeEmailTest.php @@ -0,0 +1,68 @@ +expectException(\InvalidArgumentException::class); + + new PresentationActivitySpeakerChangeEmail( + new Presentation(), + new PresentationSpeaker(), + 'NotARole', + 'Added' + ); + } + + public function testConstructorRejectsInvalidAction(): void + { + $this->expectException(\InvalidArgumentException::class); + + new PresentationActivitySpeakerChangeEmail( + new Presentation(), + new PresentationSpeaker(), + 'Speaker', + 'NotAnAction' + ); + } + + public function testConstructorThrowsWhenRecipientNotConfigured(): void + { + Config::set('cfp.speaker_change_notification_email', null); + + $this->expectException(ValidationException::class); + + $presentation = new Presentation(); + $presentation->setTitle('Test Presentation'); + + new PresentationActivitySpeakerChangeEmail( + $presentation, + new PresentationSpeaker(), + 'Speaker', + 'Added' + ); + } +} diff --git a/tests/oauth2/OAuth2PresentationApiTest.php b/tests/oauth2/OAuth2PresentationApiTest.php index cf1683360..c5245ea0b 100644 --- a/tests/oauth2/OAuth2PresentationApiTest.php +++ b/tests/oauth2/OAuth2PresentationApiTest.php @@ -12,9 +12,11 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Models\Foundation\Main\IGroup; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Queue; /** * Class OAuth2PresentationApiTest */ @@ -36,6 +38,7 @@ protected function setUp(): void self::$current_track_chair = self::$summit->addTrackChair(self::$member, [ self::$defaultTrack ] ); self::$em->persist(self::$summit); self::$em->flush(); + Config::set('cfp.speaker_change_notification_email', 'speaker-changes@test.com'); } protected function tearDown(): void @@ -1249,6 +1252,145 @@ public function testUpdateSpeakerInPresentation() $this->assertResponseStatus(201); } + public function testAddSpeaker2PresentationQueuesChangeNotificationOnPublishedPresentation() + { + $presentation = self::$default_selection_plan->getPresentations()[0]; + $this->assertTrue($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => $presentation->getId(), + 'speaker_id' => self::$speaker->getId(), + ]; + + Queue::fake(); + + $response = $this->action( + "POST", + "OAuth2PresentationApiController@addSpeaker2Presentation", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['order' => 1]) + ); + + $this->assertResponseStatus(201); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + + // re-adding the same (already assigned) speaker is an order-only update: no additional email + $response = $this->action( + "POST", + "OAuth2PresentationApiController@addSpeaker2Presentation", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['order' => 1]) + ); + + $this->assertResponseStatus(201); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + + public function testRemoveSpeakerFromPresentationQueuesChangeNotificationOnPublishedPresentation() + { + $presentation = self::$default_selection_plan->getPresentations()[0]; + $this->assertTrue($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => $presentation->getId(), + 'speaker_id' => self::$speaker->getId(), + ]; + + // ensure the speaker is actually assigned first (outside the fake queue window) + $response = $this->action( + "POST", + "OAuth2PresentationApiController@addSpeaker2Presentation", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['order' => 1]) + ); + $this->assertResponseStatus(201); + + Queue::fake(); + + $response = $this->action( + "DELETE", + "OAuth2PresentationApiController@removeSpeakerFromPresentation", + $params, + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(204); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + + // removing an already-removed (not assigned) speaker queues nothing additional + $response = $this->action( + "DELETE", + "OAuth2PresentationApiController@removeSpeakerFromPresentation", + $params, + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(204); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + + public function testSpeakerChangeEndpointsDoNotQueueChangeNotificationOnNonPublishedPresentation() + { + $presentation = self::$default_selection_plan->getPresentations()[1]; + $presentation->unPublish(); + self::$em->persist($presentation); + self::$em->flush(); + $this->assertFalse($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => $presentation->getId(), + 'speaker_id' => self::$speaker->getId(), + ]; + + Queue::fake(); + + $response = $this->action( + "POST", + "OAuth2PresentationApiController@addSpeaker2Presentation", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['order' => 1]) + ); + $this->assertResponseStatus(201); + + $response = $this->action( + "DELETE", + "OAuth2PresentationApiController@removeSpeakerFromPresentation", + $params, + [], + [], + [], + $this->getAuthHeaders() + ); + $this->assertResponseStatus(204); + + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + // --- Comments --- public function testGetComment() diff --git a/tests/oauth2/OAuth2SummitEventsApiTest.php b/tests/oauth2/OAuth2SummitEventsApiTest.php index f00452cc2..e30f0010e 100644 --- a/tests/oauth2/OAuth2SummitEventsApiTest.php +++ b/tests/oauth2/OAuth2SummitEventsApiTest.php @@ -11,10 +11,16 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\IMailTemplatesConstants; +use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Models\Foundation\Main\IGroup; use App\Services\Model\ISummitService; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Queue; +use models\summit\PresentationSpeaker; +use models\summit\PresentationType; use models\utils\SilverstripeBaseModel; use services\model\IPresentationService; use models\summit\Presentation; @@ -35,6 +41,24 @@ protected function setUp():void self::$defaultMember2 = self::$member2; self::insertSummitTestData(); self::InsertOrdersTestData(); + Config::set('cfp.speaker_change_notification_email', 'speaker-changes@test.com'); + } + + private function collectDispatchedChangeActions(): array + { + $actions = []; + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, function ($job) use (&$actions) { + $ref = new \ReflectionClass($job); + $prop = $ref->getProperty('payload'); + $prop->setAccessible(true); + $payload = $prop->getValue($job); + $actions[] = [ + 'role' => $payload[IMailTemplatesConstants::activity_change_role], + 'action' => $payload[IMailTemplatesConstants::activity_change_action], + ]; + return true; + }); + return $actions; } public function tearDown():void @@ -480,6 +504,267 @@ public function testUpdateEvent() } + public function testUpdateEventSpeakersQueuesChangeNotificationOnPublishedPresentation() + { + $presentation = self::$summit->getPresentations()[0]; + $this->assertTrue($presentation->isPublished()); + $old_speaker_id = self::$defaultSpeaker->getId(); + + $newSpeaker = new PresentationSpeaker(); + $newSpeaker->setFirstName('New'); + $newSpeaker->setLastName('Speaker'); + $newSpeaker->setBio('New speaker bio'); + self::$em->persist($newSpeaker); + self::$em->flush(); + + $params = [ + 'id' => self::$summit->getId(), + 'event_id' => $presentation->getId(), + ]; + + $data = [ + 'speakers' => [$newSpeaker->getId()], + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitEventsApiController@updateEvent", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(200); + + $actions = $this->collectDispatchedChangeActions(); + $this->assertCount(2, $actions); + $this->assertContains(['role' => 'Speaker', 'action' => 'Added'], $actions); + $this->assertContains(['role' => 'Speaker', 'action' => 'Removed'], $actions); + + // re-saving the identical speakers array queues nothing additional + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitEventsApiController@updateEvent", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(200); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + + public function testUpdateEventModeratorQueuesChangeNotificationOnPublishedPresentation() + { + $moderatorType = new PresentationType(); + $moderatorType->setType('TEST MODERATED PRESENTATION TYPE ' . str_random(8)); + $moderatorType->setMinSpeakers(1); + $moderatorType->setMaxSpeakers(3); + $moderatorType->setMinModerators(0); + $moderatorType->setMaxModerators(1); + $moderatorType->setUseSpeakers(true); + $moderatorType->setShouldBeAvailableOnCfp(true); + $moderatorType->setAreSpeakersMandatory(false); + $moderatorType->setUseModerator(true); + $moderatorType->setIsModeratorMandatory(false); + $moderatorType->setAllowsLocationTimeframeCollision(true); + $moderatorType->setAllowsSpeakerEventCollision(true); + $moderatorType->setBlackoutTimes('Final'); + self::$summit->addEventType($moderatorType); + self::$em->persist(self::$summit); + self::$em->flush(); + + $oldModerator = new PresentationSpeaker(); + $oldModerator->setFirstName('Old'); + $oldModerator->setLastName('Moderator'); + $oldModerator->setBio('Old moderator bio'); + self::$em->persist($oldModerator); + + $newModerator = new PresentationSpeaker(); + $newModerator->setFirstName('New'); + $newModerator->setLastName('Moderator'); + $newModerator->setBio('New moderator bio'); + self::$em->persist($newModerator); + self::$em->flush(); + + $start_date = new \DateTime('now', new \DateTimeZone('UTC')); + $end_date = (clone $start_date)->add(new \DateInterval('PT1H')); + + $presentation = new Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle('Moderated Presentation ' . str_random(8)); + $presentation->setAbstract('Moderated presentation abstract'); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType($moderatorType); + $presentation->setProgress(Presentation::PHASE_COMPLETE); + $presentation->setStatus(Presentation::STATUS_RECEIVED); + $presentation->setStartDate($start_date); + $presentation->setEndDate($end_date); + $presentation->addSpeaker(self::$defaultSpeaker); + $presentation->setModerator($oldModerator); + self::$em->persist($presentation); + self::$em->flush(); + $presentation->publish(); + self::$em->persist($presentation); + self::$em->flush(); + + $this->assertTrue($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'event_id' => $presentation->getId(), + ]; + + $data = [ + 'moderator_speaker_id' => $newModerator->getId(), + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitEventsApiController@updateEvent", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(200); + + $actions = $this->collectDispatchedChangeActions(); + $this->assertCount(2, $actions); + $this->assertContains(['role' => 'Moderator', 'action' => 'Added'], $actions); + $this->assertContains(['role' => 'Moderator', 'action' => 'Removed'], $actions); + } + + public function testUpdateEventSpeakersDoNotQueueChangeNotificationOnNonPublishedPresentation() + { + $presentation = self::$summit->getPresentations()[2]; + $presentation->unPublish(); + self::$em->persist($presentation); + self::$em->flush(); + $this->assertFalse($presentation->isPublished()); + + $newSpeaker = new PresentationSpeaker(); + $newSpeaker->setFirstName('New'); + $newSpeaker->setLastName('SpeakerNotPublished'); + $newSpeaker->setBio('New speaker bio'); + self::$em->persist($newSpeaker); + self::$em->flush(); + + $params = [ + 'id' => self::$summit->getId(), + 'event_id' => $presentation->getId(), + ]; + + $data = [ + 'speakers' => [$newSpeaker->getId()], + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitEventsApiController@updateEvent", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(200); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + + public function testUpdateEventModeratorDoesNotQueueChangeNotificationOnNonPublishedPresentation() + { + $moderatorType = new PresentationType(); + $moderatorType->setType('TEST MODERATED PRESENTATION TYPE NOT PUBLISHED ' . str_random(8)); + $moderatorType->setMinSpeakers(1); + $moderatorType->setMaxSpeakers(3); + $moderatorType->setMinModerators(0); + $moderatorType->setMaxModerators(1); + $moderatorType->setUseSpeakers(true); + $moderatorType->setShouldBeAvailableOnCfp(true); + $moderatorType->setAreSpeakersMandatory(false); + $moderatorType->setUseModerator(true); + $moderatorType->setIsModeratorMandatory(false); + $moderatorType->setAllowsLocationTimeframeCollision(true); + $moderatorType->setAllowsSpeakerEventCollision(true); + $moderatorType->setBlackoutTimes('Final'); + self::$summit->addEventType($moderatorType); + self::$em->persist(self::$summit); + self::$em->flush(); + + $oldModerator = new PresentationSpeaker(); + $oldModerator->setFirstName('Old'); + $oldModerator->setLastName('ModeratorNotPublished'); + $oldModerator->setBio('Old moderator bio'); + self::$em->persist($oldModerator); + + $newModerator = new PresentationSpeaker(); + $newModerator->setFirstName('New'); + $newModerator->setLastName('ModeratorNotPublished'); + $newModerator->setBio('New moderator bio'); + self::$em->persist($newModerator); + self::$em->flush(); + + $presentation = new Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle('Non-Published Moderated Presentation ' . str_random(8)); + $presentation->setAbstract('Non-published moderated presentation abstract'); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType($moderatorType); + $presentation->setProgress(Presentation::PHASE_COMPLETE); + $presentation->setStatus(Presentation::STATUS_RECEIVED); + $presentation->addSpeaker(self::$defaultSpeaker); + $presentation->setModerator($oldModerator); + self::$em->persist($presentation); + self::$em->flush(); + + $this->assertFalse($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'event_id' => $presentation->getId(), + ]; + + $data = [ + 'moderator_speaker_id' => $newModerator->getId(), + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitEventsApiController@updateEvent", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(200); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + public function testUpdateDraftEventDoesNotCompleteIncompletePresentation() { $presentation = new Presentation(); diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index 5f0469753..37eebad74 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -11,11 +11,13 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Models\Foundation\Main\IGroup; use App\Models\Foundation\Summit\Speakers\SpeakerEditPermissionRequest; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Date; +use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\EntityManager; use models\summit\Presentation; use models\summit\PresentationSpeaker; @@ -35,6 +37,7 @@ protected function setUp(): void self::insertSummitTestData(); // Clean up stale edit permission requests from previous test runs/methods self::$em->getConnection()->executeStatement('DELETE FROM SpeakerEditPermissionRequest'); + Config::set('cfp.speaker_change_notification_email', 'speaker-changes@test.com'); } protected function tearDown(): void @@ -1832,6 +1835,169 @@ public function testRemoveModeratorFromMyPresentation() $this->assertResponseStatus(204); } + public function testAddModeratorToMyPresentationQueuesChangeNotificationOnPublishedPresentation() + { + $presentation = self::$presentations[0]; + $this->assertTrue($presentation->isPublished()); + + $moderator = new PresentationSpeaker(); + $moderator->setFirstName('Moderator'); + $moderator->setLastName('Notify'); + $moderator->setBio('Moderator bio'); + self::$em->persist($moderator); + self::$em->flush(); + + $presentation_id = $presentation->getId(); + $moderator_id = $moderator->getId(); + self::$em->clear(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $moderator_id, + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(201); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + + public function testRemoveModeratorFromMyPresentationQueuesChangeNotificationOnPublishedPresentation() + { + $ids = $this->testAddModeratorToMyPresentation(); + + $presentation = self::$summit->getEvent($ids['presentation_id']); + $this->assertTrue($presentation->isPublished()); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $ids['presentation_id'], + 'speaker_id' => $ids['speaker_id'], + ]; + + Queue::fake(); + + $response = $this->action( + "DELETE", + "OAuth2SummitSpeakersApiController@removeModeratorFromMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(204); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + + public function testModeratorEndpointsDoNotQueueChangeNotificationOnNonPublishedPresentation() + { + $presentation = self::$presentations[0]; + $presentation->unPublish(); + self::$em->persist($presentation); + self::$em->flush(); + $this->assertFalse($presentation->isPublished()); + + $moderator = new PresentationSpeaker(); + $moderator->setFirstName('Moderator'); + $moderator->setLastName('NotPublished'); + $moderator->setBio('Moderator bio'); + self::$em->persist($moderator); + self::$em->flush(); + + $presentation_id = $presentation->getId(); + $moderator_id = $moderator->getId(); + self::$em->clear(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $moderator_id, + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + $params, + [], + [], + [], + $headers + ); + $this->assertResponseStatus(201); + + $response = $this->action( + "DELETE", + "OAuth2SummitSpeakersApiController@removeModeratorFromMyPresentation", + $params, + [], + [], + [], + $headers + ); + $this->assertResponseStatus(204); + + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + + public function testRemoveModeratorFromMyPresentationWithNoModeratorSetQueuesNothing() + { + $presentation = self::$presentations[0]; + $this->assertTrue($presentation->isPublished()); + $this->assertFalse($presentation->hasModerator()); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $presentation->getId(), + 'speaker_id' => self::$defaultSpeaker->getId(), + ]; + + Queue::fake(); + + $response = $this->action( + "DELETE", + "OAuth2SummitSpeakersApiController@removeModeratorFromMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(204); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + // --- Approve/Decline Speaker Edit Permission --- public function testApproveSpeakerEditPermission() From a3a8c59ac5db938a5273ec547f4aeccc668cbaf5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 24 Aug 2026 18:18:30 -0300 Subject: [PATCH 2/6] fix(presentations): address CodeRabbit review findings on speaker-change notifications Three real gaps found by CodeRabbit's review of #589, all verified against current code before fixing: - SummitService::addSpeaker2Presentation/removeSpeakerFromPresentation (the self-service "my presentation" flow) never dispatched PresentationActivitySpeakerChangeEmail at all. Now guarded by isSpeaker() for genuine-transition-only, same pattern as PresentationService's version. - SummitService::addModerator2Presentation fired an unconditional "Added" notification on every call, even when reassigning the same moderator (no-op) or replacing an existing one (never notified the outgoing moderator). Now diffs old vs. new moderator: no notification when unchanged, Removed+Added when replaced. - updateEvents()/updateAndPublishEvents() wrap updateEvent() in an outer transaction; saveOrUpdateEvent() was dispatching right after its own *nested* transaction() call returned, but Doctrine's connection nests begin/commit via a counter, so that "commit" isn't real until the outermost transaction commits. A later item failing mid-batch rolled back an earlier item's DB changes while its notification had already gone out. Fixed by threading an optional by-reference $pending_notifications_out through saveOrUpdateEvent/updateEvent: bulk callers accumulate and dispatch only after their own outer transaction returns; standalone calls behave exactly as before. Root-caused while reproducing the last fix: DoctrineRepository::find() called EntityManager::refresh() unconditionally when $refresh=true, even when the lookup returned null - throwing a TypeError for any nonexistent ID passed to getByIdRefreshed(). DoctrineTransactionService::transaction() only catches \Exception, so that TypeError skipped rollback/cleanup entirely and left the DB transaction open indefinitely, holding locks across every table it had touched (reproduced live: a stuck transaction blocked the whole local DB for ~16 minutes until killed by hand). Fixed both: null-guard before refresh(), and widened the transaction service's catch to \Throwable so any future non-Exception error still rolls back and closes the connection. 11 new/extended tests covering all of the above, including a rollback regression test exercising the exact nested-transaction/failure path. --- app/Repositories/DoctrineRepository.php | 2 +- app/Services/Model/Imp/SummitService.php | 134 ++++++++-- .../Utils/DoctrineTransactionService.php | 12 +- tests/oauth2/OAuth2SummitEventsApiTest.php | 45 ++++ tests/oauth2/OAuth2SummitSpeakersApiTest.php | 238 ++++++++++++++++++ 5 files changed, 401 insertions(+), 30 deletions(-) diff --git a/app/Repositories/DoctrineRepository.php b/app/Repositories/DoctrineRepository.php index 62542e841..10ccc8802 100644 --- a/app/Repositories/DoctrineRepository.php +++ b/app/Repositories/DoctrineRepository.php @@ -457,7 +457,7 @@ public function find($id, $lockMode = null, $lockVersion = null, $refresh = fals { $em = $this->getEntityManager(); $res = $em->find($this->getEntityName(), $id, $lockMode, $lockVersion); - if($refresh) + if($refresh && !is_null($res)) $em->refresh($res); return $res; } diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index ff5d2a87c..27600b2d0 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -618,9 +618,9 @@ public function addEvent(Summit $summit, array $data) * @param array $data * @return SummitEvent */ - public function updateEvent(Summit $summit, $event_id, array $data, bool $trigger_data_update = true, bool $saveAsIncomplete = false) + public function updateEvent(Summit $summit, $event_id, array $data, bool $trigger_data_update = true, bool $saveAsIncomplete = false, ?array &$pending_notifications_out = null) { - return $this->saveOrUpdateEvent($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete); + return $this->saveOrUpdateEvent($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete, $pending_notifications_out); } /** @@ -661,7 +661,7 @@ private function canPerformEventTypeTransition(SummitEventType $old_event_type, * @return SummitEvent * @throws Exception */ - private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null, bool $trigger_data_update = true, bool $saveAsIncomplete = false) + private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null, bool $trigger_data_update = true, bool $saveAsIncomplete = false, ?array &$pending_notifications_out = null) { $pending_speaker_changes = []; @@ -865,10 +865,21 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null return $event; }); - // dispatched only after the transaction above has committed, so a queued notification - // never outlives a save that ends up rolling back - foreach ($pending_speaker_changes as $change) { - PresentationActivitySpeakerChangeEmail::dispatch($event, $change['speaker'], $change['role'], $change['action']); + // $pending_notifications_out !== null means a bulk caller (updateEvents/updateAndPublishEvents) + // is wrapping this call in its OWN outer transaction: this method's transaction() above just + // returned but has not really committed yet (Doctrine nests via a connection-level counter), so + // dispatching here would outlive a rollback triggered by a later item in that caller's batch. + // Hand the pending changes back instead and let the bulk caller dispatch after ITS OWN commit. + if ($pending_notifications_out !== null) { + foreach ($pending_speaker_changes as $change) { + $pending_notifications_out[] = [$event, $change['speaker'], $change['role'], $change['action']]; + } + } else { + // standalone call (addEvent/updateEvent used directly): the transaction() call above + // was the outermost one, so its commit was real and dispatching now is safe + foreach ($pending_speaker_changes as $change) { + PresentationActivitySpeakerChangeEmail::dispatch($event, $change['speaker'], $change['role'], $change['action']); + } } return $event; @@ -1553,17 +1564,28 @@ public function unPublishEvents(Summit $summit, array $data) */ public function updateAndPublishEvents(Summit $summit, array $data) { - return $this->tx_service->transaction(function () use ( + $pending_notifications = []; + + $result = $this->tx_service->transaction(function () use ( $summit, - $data + $data, + &$pending_notifications ) { foreach ($data['events'] as $event_data) { - $this->updateEvent($summit, intval($event_data['id']), $event_data); + $this->updateEvent($summit, intval($event_data['id']), $event_data, true, false, $pending_notifications); $this->publishEvent($summit, intval($event_data['id']), $event_data); } return true; }); + + // dispatched only after this (outermost) transaction has committed, so a queued + // notification from an earlier item in the batch never outlives a later item's failure + foreach ($pending_notifications as $notification) { + PresentationActivitySpeakerChangeEmail::dispatch(...$notification); + } + + return $result; } /** @@ -1576,17 +1598,28 @@ public function updateAndPublishEvents(Summit $summit, array $data) */ public function updateEvents(Summit $summit, array $data, bool $trigger_data_update = true) { - return $this->tx_service->transaction(function () use ( + $pending_notifications = []; + + $result = $this->tx_service->transaction(function () use ( $summit, $data, - $trigger_data_update + $trigger_data_update, + &$pending_notifications ) { foreach ($data['events'] as $event_data) { - $this->updateEvent($summit, intval($event_data['id']), $event_data, $trigger_data_update); + $this->updateEvent($summit, intval($event_data['id']), $event_data, $trigger_data_update, false, $pending_notifications); } return true; }); + + // dispatched only after this (outermost) transaction has committed, so a queued + // notification from an earlier item in the batch never outlives a later item's failure + foreach ($pending_notifications as $notification) { + PresentationActivitySpeakerChangeEmail::dispatch(...$notification); + } + + return $result; } /** @@ -1798,7 +1831,9 @@ public function deleteSummit($summit_id) */ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - return $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id) { + $pending_notification = null; + + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1826,7 +1861,17 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, if (!$presentation->isCompleted()) $presentation->setProgress(Presentation::PHASE_SPEAKERS); - $presentation->addSpeaker($speaker); + if (!$presentation->isSpeaker($speaker)) { + $presentation->addSpeaker($speaker); + if ($presentation->isPublished()) { + $pending_notification = [ + $presentation, + $speaker, + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Added + ]; + } + } // check is selection plan is private, if so add moderator to allowed members @@ -1841,6 +1886,14 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, return $presentation; }); + + // dispatched only after the transaction above has committed; addSpeaker2Presentation + // has no internal callers in this class, so this call is always the outermost one + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } + + return $presentation; } /** @@ -1853,7 +1906,9 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, */ public function removeSpeakerFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - return $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id) { + $pending_notification = null; + + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -1883,10 +1938,28 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak if (!$presentation->isCompleted()) $presentation->setProgress(Presentation::PHASE_SPEAKERS); - $presentation->removeSpeaker($speaker); + if ($presentation->isSpeaker($speaker)) { + $presentation->removeSpeaker($speaker); + if ($presentation->isPublished()) { + $pending_notification = [ + $presentation, + $speaker, + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + } return $presentation; }); + + // dispatched only after the transaction above has committed; removeSpeakerFromPresentation + // has no internal callers in this class, so this call is always the outermost one + if (!is_null($pending_notification)) { + PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + } + + return $presentation; } /** @@ -1899,9 +1972,9 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak */ public function addModerator2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notification = null; + $pending_notifications = []; - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1930,10 +2003,21 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i if (!$presentation->isCompleted()) $presentation->setProgress(Presentation::PHASE_SPEAKERS); + $previous_moderator = $presentation->hasModerator() ? $presentation->getModerator() : null; + $previous_moderator_id = is_null($previous_moderator) ? null : $previous_moderator->getId(); + $presentation->setModerator($speaker); - if ($presentation->isPublished()) { - $pending_notification = [ + if ($presentation->isPublished() && $previous_moderator_id !== $speaker->getId()) { + if (!is_null($previous_moderator)) { + $pending_notifications[] = [ + $presentation, + $previous_moderator, + PresentationActivitySpeakerChangeEmail::Role_Moderator, + PresentationActivitySpeakerChangeEmail::Action_Removed + ]; + } + $pending_notifications[] = [ $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Moderator, @@ -1955,10 +2039,10 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i return $presentation; }); - // dispatched only after the transaction above has committed, so a queued notification - // never outlives a save that ends up rolling back - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); + // dispatched only after the transaction above has committed; addModerator2Presentation + // has no internal callers in this class, so this call is always the outermost one + foreach ($pending_notifications as $notification) { + PresentationActivitySpeakerChangeEmail::dispatch(...$notification); } return $presentation; diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php index 0c30fb96c..ef66cda63 100644 --- a/app/Services/Utils/DoctrineTransactionService.php +++ b/app/Services/Utils/DoctrineTransactionService.php @@ -45,10 +45,10 @@ public function __construct($manager_name) } /** - * @param Exception $e + * @param \Throwable $e * @return bool */ - public function shouldReconnect(\Exception $e):bool + public function shouldReconnect(\Throwable $e):bool { Log::debug ( @@ -129,7 +129,7 @@ public function shouldReconnect(\Exception $e):bool * @param Closure $callback * @param int $isolationLevel * @return mixed|null - * @throws Exception + * @throws \Throwable */ public function transaction(Closure $callback, int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED) { @@ -151,7 +151,11 @@ public function transaction(Closure $callback, int $isolationLevel = Transactio $em->getConnection()->commit(); $done = true; } - catch (Exception $ex) { + // \Throwable, not \Exception: a \TypeError/\Error thrown inside the callback (e.g. a + // repository bug) must still roll back and close the connection - catching only + // \Exception let such errors skip cleanup entirely and leave the transaction open, + // holding locks indefinitely. + catch (\Throwable $ex) { $retry++; $em->getConnection()->close(); diff --git a/tests/oauth2/OAuth2SummitEventsApiTest.php b/tests/oauth2/OAuth2SummitEventsApiTest.php index e30f0010e..b65866feb 100644 --- a/tests/oauth2/OAuth2SummitEventsApiTest.php +++ b/tests/oauth2/OAuth2SummitEventsApiTest.php @@ -63,6 +63,7 @@ private function collectDispatchedChangeActions(): array public function tearDown():void { + \Mockery::close(); self::clearOrdersTestData(); self::clearSummitTestData(); parent::tearDown(); @@ -765,6 +766,50 @@ public function testUpdateEventModeratorDoesNotQueueChangeNotificationOnNonPubli Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); } + public function testUpdateEventsRollsBackWithoutQueuingChangeNotificationsWhenABatchMemberFails() + { + $presentation = self::$summit->getPresentations()[3]; + $this->assertTrue($presentation->isPublished()); + + $newSpeaker = new PresentationSpeaker(); + $newSpeaker->setFirstName('New'); + $newSpeaker->setLastName('SpeakerBulkRollback'); + $newSpeaker->setBio('New speaker bio'); + self::$em->persist($newSpeaker); + self::$em->flush(); + + $data = [ + 'events' => [ + [ + 'id' => $presentation->getId(), + 'speakers' => [$newSpeaker->getId()], + ], + [ + // nonexistent event id: forces the whole bulk transaction to roll back + 'id' => 999999999, + ], + ], + ]; + + Queue::fake(); + + $mock_context = \Mockery::mock(\models\oauth2\IResourceServerContext::class); + $mock_context->shouldReceive('getCurrentUser')->andReturn(self::$defaultMember2); + $this->app->instance('resource_server_context', $mock_context); + + $service = App::make(\services\model\ISummitService::class); + + $threw = false; + try { + $service->updateEvents(self::$summit, $data, false); + } catch (\Throwable $ex) { + $threw = true; + } + + $this->assertTrue($threw, 'updateEvents was expected to throw for the nonexistent event id in the batch.'); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + public function testUpdateDraftEventDoesNotCompleteIncompletePresentation() { $presentation = new Presentation(); diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index 37eebad74..606a22a69 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\IMailTemplatesConstants; use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; use App\Models\Foundation\Main\IGroup; use App\Models\Foundation\Summit\Speakers\SpeakerEditPermissionRequest; @@ -40,6 +41,23 @@ protected function setUp(): void Config::set('cfp.speaker_change_notification_email', 'speaker-changes@test.com'); } + private function collectDispatchedChangeActions(): array + { + $actions = []; + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, function ($job) use (&$actions) { + $ref = new \ReflectionClass($job); + $prop = $ref->getProperty('payload'); + $prop->setAccessible(true); + $payload = $prop->getValue($job); + $actions[] = [ + 'role' => $payload[IMailTemplatesConstants::activity_change_role], + 'action' => $payload[IMailTemplatesConstants::activity_change_action], + ]; + return true; + }); + return $actions; + } + protected function tearDown(): void { self::clearSummitTestData(); @@ -1767,6 +1785,106 @@ public function testRemoveSpeakerFromMyPresentation() $this->assertResponseStatus(204); } + public function testAddSpeakerToMyPresentationQueuesChangeNotificationOnPublishedPresentation() + { + $presentation = self::$presentations[0]; + $this->assertTrue($presentation->isPublished()); + + $new_speaker = new PresentationSpeaker(); + $new_speaker->setFirstName('New'); + $new_speaker->setLastName('SpeakerSelfService'); + $new_speaker->setBio('New speaker bio'); + self::$em->persist($new_speaker); + self::$em->flush(); + + $presentation_id = $presentation->getId(); + $speaker_id = $new_speaker->getId(); + self::$em->clear(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $speaker_id, + ]; + + Queue::fake(); + + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addSpeakerToMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(201); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + + // re-adding the same (already assigned) speaker queues nothing additional + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addSpeakerToMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(201); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + + public function testRemoveSpeakerFromMyPresentationQueuesChangeNotificationOnPublishedPresentation() + { + $ids = $this->testAddSpeakerToMyPresentation(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $ids['presentation_id'], + 'speaker_id' => $ids['speaker_id'], + ]; + + Queue::fake(); + + $response = $this->action( + "DELETE", + "OAuth2SummitSpeakersApiController@removeSpeakerFromMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(204); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + + // removing an already-removed speaker queues nothing additional + $response = $this->action( + "DELETE", + "OAuth2SummitSpeakersApiController@removeSpeakerFromMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(204); + Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); + } + public function testAddModeratorToMyPresentation() { // Create a new speaker to be moderator @@ -1877,6 +1995,126 @@ public function testAddModeratorToMyPresentationQueuesChangeNotificationOnPublis Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); } + public function testAddModeratorToMyPresentationReassigningSameModeratorQueuesNothing() + { + $presentation = self::$presentations[0]; + $this->assertTrue($presentation->isPublished()); + + $moderator = new PresentationSpeaker(); + $moderator->setFirstName('Moderator'); + $moderator->setLastName('Unchanged'); + $moderator->setBio('Moderator bio'); + self::$em->persist($moderator); + self::$em->flush(); + + $presentation_id = $presentation->getId(); + $moderator_id = $moderator->getId(); + self::$em->clear(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $params = [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $moderator_id, + ]; + + // first assignment: sets the moderator (not asserted here, exercised elsewhere) + $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + $params, + [], + [], + [], + $headers + ); + + Queue::fake(); + + // re-assigning the SAME moderator must not queue a notification + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(201); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + } + + public function testAddModeratorToMyPresentationReplacingModeratorQueuesRemovedAndAdded() + { + $presentation = self::$presentations[0]; + $this->assertTrue($presentation->isPublished()); + + $old_moderator = new PresentationSpeaker(); + $old_moderator->setFirstName('Old'); + $old_moderator->setLastName('ModeratorReplaced'); + $old_moderator->setBio('Old moderator bio'); + self::$em->persist($old_moderator); + + $new_moderator = new PresentationSpeaker(); + $new_moderator->setFirstName('New'); + $new_moderator->setLastName('ModeratorReplacing'); + $new_moderator->setBio('New moderator bio'); + self::$em->persist($new_moderator); + self::$em->flush(); + + $presentation_id = $presentation->getId(); + $old_moderator_id = $old_moderator->getId(); + $new_moderator_id = $new_moderator->getId(); + self::$em->clear(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + // assign the old moderator first (not asserted here) + $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $old_moderator_id, + ], + [], + [], + [], + $headers + ); + + Queue::fake(); + + // replace with the new moderator: must queue Removed (old) + Added (new) + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@addModeratorToMyPresentation", + [ + 'presentation_id' => $presentation_id, + 'speaker_id' => $new_moderator_id, + ], + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(201); + + $actions = $this->collectDispatchedChangeActions(); + $this->assertCount(2, $actions); + $this->assertContains(['role' => 'Moderator', 'action' => 'Added'], $actions); + $this->assertContains(['role' => 'Moderator', 'action' => 'Removed'], $actions); + } + public function testRemoveModeratorFromMyPresentationQueuesChangeNotificationOnPublishedPresentation() { $ids = $this->testAddModeratorToMyPresentation(); From 1fdde6cee8d0e8c0c70cbc94893b838486b19dad Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 24 Aug 2026 18:39:02 -0300 Subject: [PATCH 3/6] fix(presentations): keep speaker-change fix out of unrelated transaction infra Revert the DoctrineRepository/DoctrineTransactionService changes from the previous commit - out of scope for an email-notifications PR, and nested-transaction/rollback handling in those exact files is already being reworked in #533. Redesign the batch-rollback regression test to trigger the mid-batch failure via an existing event with a nonexistent track_id (EntityNotFoundException, already null-checked) instead of a nonexistent event id (which went through DoctrineRepository::find()'s unguarded refresh() call - the bug reported separately on #533). Same coverage of SummitService's own dispatch-timing fix, without touching the file #533 owns. --- app/Repositories/DoctrineRepository.php | 2 +- app/Services/Utils/DoctrineTransactionService.php | 12 ++++-------- tests/oauth2/OAuth2SummitEventsApiTest.php | 13 +++++++++---- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/app/Repositories/DoctrineRepository.php b/app/Repositories/DoctrineRepository.php index 10ccc8802..62542e841 100644 --- a/app/Repositories/DoctrineRepository.php +++ b/app/Repositories/DoctrineRepository.php @@ -457,7 +457,7 @@ public function find($id, $lockMode = null, $lockVersion = null, $refresh = fals { $em = $this->getEntityManager(); $res = $em->find($this->getEntityName(), $id, $lockMode, $lockVersion); - if($refresh && !is_null($res)) + if($refresh) $em->refresh($res); return $res; } diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php index ef66cda63..0c30fb96c 100644 --- a/app/Services/Utils/DoctrineTransactionService.php +++ b/app/Services/Utils/DoctrineTransactionService.php @@ -45,10 +45,10 @@ public function __construct($manager_name) } /** - * @param \Throwable $e + * @param Exception $e * @return bool */ - public function shouldReconnect(\Throwable $e):bool + public function shouldReconnect(\Exception $e):bool { Log::debug ( @@ -129,7 +129,7 @@ public function shouldReconnect(\Throwable $e):bool * @param Closure $callback * @param int $isolationLevel * @return mixed|null - * @throws \Throwable + * @throws Exception */ public function transaction(Closure $callback, int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED) { @@ -151,11 +151,7 @@ public function transaction(Closure $callback, int $isolationLevel = Transactio $em->getConnection()->commit(); $done = true; } - // \Throwable, not \Exception: a \TypeError/\Error thrown inside the callback (e.g. a - // repository bug) must still roll back and close the connection - catching only - // \Exception let such errors skip cleanup entirely and leave the transaction open, - // holding locks indefinitely. - catch (\Throwable $ex) { + catch (Exception $ex) { $retry++; $em->getConnection()->close(); diff --git a/tests/oauth2/OAuth2SummitEventsApiTest.php b/tests/oauth2/OAuth2SummitEventsApiTest.php index b65866feb..79674c39d 100644 --- a/tests/oauth2/OAuth2SummitEventsApiTest.php +++ b/tests/oauth2/OAuth2SummitEventsApiTest.php @@ -778,6 +778,8 @@ public function testUpdateEventsRollsBackWithoutQueuingChangeNotificationsWhenAB self::$em->persist($newSpeaker); self::$em->flush(); + $otherPresentation = self::$summit->getPresentations()[4]; + $data = [ 'events' => [ [ @@ -785,8 +787,11 @@ public function testUpdateEventsRollsBackWithoutQueuingChangeNotificationsWhenAB 'speakers' => [$newSpeaker->getId()], ], [ - // nonexistent event id: forces the whole bulk transaction to roll back - 'id' => 999999999, + // existing event, but a nonexistent track_id: SummitService::saveOrUpdateEvent + // throws a clean EntityNotFoundException here, forcing the whole bulk + // transaction to roll back + 'id' => $otherPresentation->getId(), + 'track_id' => 999999999, ], ], ]; @@ -802,11 +807,11 @@ public function testUpdateEventsRollsBackWithoutQueuingChangeNotificationsWhenAB $threw = false; try { $service->updateEvents(self::$summit, $data, false); - } catch (\Throwable $ex) { + } catch (\Exception $ex) { $threw = true; } - $this->assertTrue($threw, 'updateEvents was expected to throw for the nonexistent event id in the batch.'); + $this->assertTrue($threw, 'updateEvents was expected to throw for the nonexistent track_id in the batch.'); Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); } From 5a273251c5615a7743ebb03afd317f7af7588ef1 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 24 Aug 2026 19:45:30 -0300 Subject: [PATCH 4/6] refactor(presentations): dedupe speaker-change dispatch via shared trait Extracted the deferred-dispatch-after-transaction pattern repeated across PresentationService and SummitService (9 call sites) into DispatchesSpeakerChangeNotifications, following the existing ParametrizedSendEmails trait convention in app/Services/Model/Imp/Traits. --- .../Model/Imp/PresentationService.php | 27 ++++---- app/Services/Model/Imp/SummitService.php | 61 +++++++------------ .../DispatchesSpeakerChangeNotifications.php | 35 +++++++++++ 3 files changed, 67 insertions(+), 56 deletions(-) create mode 100644 app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php diff --git a/app/Services/Model/Imp/PresentationService.php b/app/Services/Model/Imp/PresentationService.php index 1f3e3ebdd..5c7ddfdf9 100644 --- a/app/Services/Model/Imp/PresentationService.php +++ b/app/Services/Model/Imp/PresentationService.php @@ -34,6 +34,7 @@ use App\Services\Filesystem\FileUploadStrategyFactory; use App\Services\Model\AbstractService; use App\Services\Model\IFolderService; +use App\Services\Model\Imp\Traits\DispatchesSpeakerChangeNotifications; use Illuminate\Http\Request as LaravelRequest; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Log; @@ -69,6 +70,8 @@ final class PresentationService extends AbstractService implements IPresentationService { + use DispatchesSpeakerChangeNotifications; + const LocalChunkSize = 1024; /** * @var ISummitEventRepository @@ -1726,9 +1729,9 @@ public function processMediaUpload(int $summit_id, int $media_upload_type_id, ?s * @throws \Exception */ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, int $speaker_id, array $data): Presentation { - $pending_notification = null; + $pending_notifications = []; - $presentation = $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data, &$pending_notification) { + $presentation = $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data, &$pending_notifications) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1741,7 +1744,7 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, if (!$presentation->isSpeaker($speaker)) { $presentation->addSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notification = [ + $pending_notifications[] = [ $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, @@ -1759,11 +1762,7 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, return $presentation; }); - // dispatched only after the transaction above has committed, so a queued notification - // never outlives a save that ends up rolling back - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $presentation; } @@ -1777,9 +1776,9 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, */ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_id, int $speaker_id): void { - $pending_notification = null; + $pending_notifications = []; - $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, &$pending_notification) { + $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, &$pending_notifications) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1792,7 +1791,7 @@ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_ if ($presentation->isSpeaker($speaker)) { $presentation->removeSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notification = [ + $pending_notifications[] = [ $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, @@ -1803,10 +1802,6 @@ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_ }); - // dispatched only after the transaction above has committed, so a queued notification - // never outlives a save that ends up rolling back - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); } } diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index 27600b2d0..acc531d94 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -53,6 +53,7 @@ use App\Services\FileSystem\IFileUploadStrategy; use App\Services\Model\AbstractPublishService; use App\Services\Model\IMemberService; +use App\Services\Model\Imp\Traits\DispatchesSpeakerChangeNotifications; use App\Services\Utils\Security\IEncryptionAES256KeysGenerator; use DateInterval; use DateTime; @@ -127,6 +128,8 @@ final class SummitService extends AbstractPublishService implements ISummitService { + use DispatchesSpeakerChangeNotifications; + /** * @var ISummitEventRepository */ @@ -877,9 +880,11 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null } else { // standalone call (addEvent/updateEvent used directly): the transaction() call above // was the outermost one, so its commit was real and dispatching now is safe + $pending_notifications = []; foreach ($pending_speaker_changes as $change) { - PresentationActivitySpeakerChangeEmail::dispatch($event, $change['speaker'], $change['role'], $change['action']); + $pending_notifications[] = [$event, $change['speaker'], $change['role'], $change['action']]; } + $this->dispatchSpeakerChangeNotifications($pending_notifications); } return $event; @@ -1579,11 +1584,7 @@ public function updateAndPublishEvents(Summit $summit, array $data) return true; }); - // dispatched only after this (outermost) transaction has committed, so a queued - // notification from an earlier item in the batch never outlives a later item's failure - foreach ($pending_notifications as $notification) { - PresentationActivitySpeakerChangeEmail::dispatch(...$notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $result; } @@ -1613,11 +1614,7 @@ public function updateEvents(Summit $summit, array $data, bool $trigger_data_upd return true; }); - // dispatched only after this (outermost) transaction has committed, so a queued - // notification from an earlier item in the batch never outlives a later item's failure - foreach ($pending_notifications as $notification) { - PresentationActivitySpeakerChangeEmail::dispatch(...$notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $result; } @@ -1831,9 +1828,9 @@ public function deleteSummit($summit_id) */ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notification = null; + $pending_notifications = []; - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1864,7 +1861,7 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, if (!$presentation->isSpeaker($speaker)) { $presentation->addSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notification = [ + $pending_notifications[] = [ $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, @@ -1887,11 +1884,7 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, return $presentation; }); - // dispatched only after the transaction above has committed; addSpeaker2Presentation - // has no internal callers in this class, so this call is always the outermost one - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $presentation; } @@ -1906,9 +1899,9 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, */ public function removeSpeakerFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notification = null; + $pending_notifications = []; - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -1941,7 +1934,7 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak if ($presentation->isSpeaker($speaker)) { $presentation->removeSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notification = [ + $pending_notifications[] = [ $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, @@ -1953,11 +1946,7 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak return $presentation; }); - // dispatched only after the transaction above has committed; removeSpeakerFromPresentation - // has no internal callers in this class, so this call is always the outermost one - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $presentation; } @@ -2039,11 +2028,7 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i return $presentation; }); - // dispatched only after the transaction above has committed; addModerator2Presentation - // has no internal callers in this class, so this call is always the outermost one - foreach ($pending_notifications as $notification) { - PresentationActivitySpeakerChangeEmail::dispatch(...$notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $presentation; } @@ -2058,9 +2043,9 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i */ public function removeModeratorFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notification = null; + $pending_notifications = []; - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notification) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -2095,7 +2080,7 @@ public function removeModeratorFromPresentation(int $current_member_id, int $spe $presentation->unsetModerator(); if (!is_null($previous_moderator) && $presentation->isPublished()) { - $pending_notification = [ + $pending_notifications[] = [ $presentation, $previous_moderator, PresentationActivitySpeakerChangeEmail::Role_Moderator, @@ -2106,11 +2091,7 @@ public function removeModeratorFromPresentation(int $current_member_id, int $spe return $presentation; }); - // dispatched only after the transaction above has committed, so a queued notification - // never outlives a save that ends up rolling back - if (!is_null($pending_notification)) { - PresentationActivitySpeakerChangeEmail::dispatch(...$pending_notification); - } + $this->dispatchSpeakerChangeNotifications($pending_notifications); return $presentation; } diff --git a/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php b/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php new file mode 100644 index 000000000..722f9a60a --- /dev/null +++ b/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php @@ -0,0 +1,35 @@ + Date: Mon, 24 Aug 2026 20:43:41 -0300 Subject: [PATCH 5/6] fix(presentations): never fail a committed save on a speaker-change notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Speaker-change notifications are dispatched after the caller's transaction commits, so anything they throw surfaces as an HTTP error on a request whose write already landed. With cfp.speaker_change_notification_email unset — the default, since config/cfp.php has no fallback and .env.example ships the key blank — PresentationActivitySpeakerChangeEmail's constructor throws ValidationException, which RequestProcessor maps to a 412. Every speaker or moderator change on a published activity returned an error while persisting the change. Guard the dispatch loop instead: - skip and log when the recipient is not configured, so an unconfigured deployment simply sends nothing; - wrap each dispatch in try/catch, so one failing notification neither propagates nor cancels the rest of a bulk batch. Adds PresentationActivitySpeakerChangeEmail::RecipientConfigKey so the guard and the constructor read one key rather than two copies of the literal. Regression test asserts the endpoint returns 201, queues nothing, and really persists the speaker when the recipient is unconfigured. Verified red/green: reverting the guard reproduces the 412. --- ...PresentationActivitySpeakerChangeEmail.php | 6 ++- .../DispatchesSpeakerChangeNotifications.php | 31 +++++++++++++- tests/oauth2/OAuth2PresentationApiTest.php | 42 +++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php b/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php index 68b889253..943dcc487 100644 --- a/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php +++ b/app/Jobs/Emails/Schedule/PresentationActivitySpeakerChangeEmail.php @@ -33,6 +33,8 @@ class PresentationActivitySpeakerChangeEmail extends AbstractSummitEmailJob const AllowedActions = [self::Action_Added, self::Action_Removed]; + const RecipientConfigKey = 'cfp.speaker_change_notification_email'; + protected function getEmailEventSlug(): string { return self::EVENT_SLUG; @@ -69,9 +71,9 @@ public function __construct(Presentation $presentation, PresentationSpeaker $spe $payload[IMailTemplatesConstants::activity_change_role] = $role; $payload[IMailTemplatesConstants::activity_change_action] = $action; - $to_email = Config::get('cfp.speaker_change_notification_email'); + $to_email = Config::get(self::RecipientConfigKey); if (empty($to_email)) - throw new ValidationException('cfp.speaker_change_notification_email is not configured.'); + throw new ValidationException(sprintf('%s is not configured.', self::RecipientConfigKey)); parent::__construct($summit, $payload, self::DEFAULT_TEMPLATE, $to_email); } diff --git a/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php b/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php index 722f9a60a..2a2e2e309 100644 --- a/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php +++ b/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php @@ -12,6 +12,8 @@ * limitations under the License. **/ use App\Jobs\Emails\Schedule\PresentationActivitySpeakerChangeEmail; +use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Log; /** * Trait DispatchesSpeakerChangeNotifications @@ -23,13 +25,40 @@ trait DispatchesSpeakerChangeNotifications * Dispatches queued speaker/moderator change notifications only after the caller's * enclosing transaction has committed, so a queued notification never outlives a * save that ends up rolling back. + * + * Because the caller's write is already durable by the time we get here, a notification + * failure must never propagate: it would surface as an HTTP error on a request that + * actually succeeded. The recipient is optional platform config, so an unconfigured + * deployment simply logs and sends nothing, and a single failing notification never + * cancels the rest of the batch. + * * @param array $pending_notifications * @return void */ private function dispatchSpeakerChangeNotifications(array $pending_notifications): void { + if (count($pending_notifications) === 0) return; + + if (empty(Config::get(PresentationActivitySpeakerChangeEmail::RecipientConfigKey))) { + Log::warning + ( + sprintf + ( + "DispatchesSpeakerChangeNotifications::dispatchSpeakerChangeNotifications %s is not configured, skipping %s speaker change notification(s).", + PresentationActivitySpeakerChangeEmail::RecipientConfigKey, + count($pending_notifications) + ) + ); + return; + } + foreach ($pending_notifications as $notification) { - PresentationActivitySpeakerChangeEmail::dispatch(...$notification); + try { + PresentationActivitySpeakerChangeEmail::dispatch(...$notification); + } catch (\Exception $ex) { + Log::warning("DispatchesSpeakerChangeNotifications::dispatchSpeakerChangeNotifications failed to dispatch speaker change notification."); + Log::warning($ex); + } } } } diff --git a/tests/oauth2/OAuth2PresentationApiTest.php b/tests/oauth2/OAuth2PresentationApiTest.php index c5245ea0b..1582dc6ec 100644 --- a/tests/oauth2/OAuth2PresentationApiTest.php +++ b/tests/oauth2/OAuth2PresentationApiTest.php @@ -17,6 +17,9 @@ use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Queue; +use LaravelDoctrine\ORM\Facades\Registry; +use models\summit\Presentation; +use models\utils\SilverstripeBaseModel; /** * Class OAuth2PresentationApiTest */ @@ -1295,6 +1298,45 @@ public function testAddSpeaker2PresentationQueuesChangeNotificationOnPublishedPr Queue::assertPushed(PresentationActivitySpeakerChangeEmail::class, 1); } + public function testAddSpeaker2PresentationSucceedsWhenNotificationRecipientNotConfigured() + { + // the recipient is optional platform config: an unconfigured deployment must still + // apply the speaker change, since the write has already committed by the time the + // notification is dispatched. + Config::set(PresentationActivitySpeakerChangeEmail::RecipientConfigKey, null); + + $presentation = self::$default_selection_plan->getPresentations()[0]; + $this->assertTrue($presentation->isPublished()); + + $params = [ + 'id' => self::$summit->getId(), + 'presentation_id' => $presentation->getId(), + 'speaker_id' => self::$speaker->getId(), + ]; + + Queue::fake(); + + $this->action( + "POST", + "OAuth2PresentationApiController@addSpeaker2Presentation", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode(['order' => 1]) + ); + + $this->assertResponseStatus(201); + Queue::assertNotPushed(PresentationActivitySpeakerChangeEmail::class); + + // the save must have really landed: re-read through a current entity manager, since a + // transaction-level reset during the request leaves the fixture's static one stale + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $persisted = $em->find(Presentation::class, $presentation->getId()); + $this->assertTrue($persisted->isSpeaker(self::$speaker)); + } + public function testRemoveSpeakerFromPresentationQueuesChangeNotificationOnPublishedPresentation() { $presentation = self::$default_selection_plan->getPresentations()[0]; From 098094b57d426e20afcd7a701ecc1e7c5cf3edc4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 24 Aug 2026 21:09:46 -0300 Subject: [PATCH 6/6] refactor(presentations): make speaker-change dispatch timing structural saveOrUpdateEvent decided whether to dispatch now or hand its pending notifications up to a bulk caller by testing `$pending_notifications_out !== null`. That made transactional semantics depend on whether the caller happened to INITIALIZE the out-param: passing an undeclared variable by reference yields null, which silently selects the dispatch-now branch and re-introduces the rolled-back-but-notified bug inside the outer transaction. It worked only because both call sites pre-initialized an array, and nothing enforced that. The root cause was updateEvent's double role - public entry point for the controllers and internal step of updateEvents/updateAndPublishEvents - so remove the double role instead of describing it with a flag. Introduce SpeakerChangeNotifications, a collector passed as a REQUIRED parameter. saveOrUpdateEvent and saveOrUpdatePresentationData only ever add to it; they never dispatch. Whoever constructs a collector is the one that dispatches it, right after its own transaction() returns - addEvent and updateEvent for the single paths, the bulk methods for their batch, which now call saveOrUpdateEvent directly rather than through updateEvent. Omitting the collector is a type error, not a null that quietly changes behaviour. The trait DispatchesSpeakerChangeNotifications is gone; its config guard and per-notification try/catch now live in the collector's dispatch(), which also empties itself so a double call cannot re-queue. updateEvent returns to its 5-parameter signature, so ISummitService:59 matches the implementation again with no change to the interface. Verified: OAuth2SummitEventsApiTest and OAuth2SummitSpeakersApiTest both match their pre-refactor results as full-file runs. Mutation-checked the invariant by routing the bulk loop back through updateEvent - the batch rollback regression test fails (2 queued, expected 0) and passes again once reverted. --- .../SpeakerChangeNotifications.php | 99 ++++++++++ .../Model/Imp/PresentationService.php | 28 +-- app/Services/Model/Imp/SummitService.php | 184 +++++++++--------- .../DispatchesSpeakerChangeNotifications.php | 64 ------ 4 files changed, 207 insertions(+), 168 deletions(-) create mode 100644 app/Services/Model/Imp/Notifications/SpeakerChangeNotifications.php delete mode 100644 app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php diff --git a/app/Services/Model/Imp/Notifications/SpeakerChangeNotifications.php b/app/Services/Model/Imp/Notifications/SpeakerChangeNotifications.php new file mode 100644 index 000000000..48c425604 --- /dev/null +++ b/app/Services/Model/Imp/Notifications/SpeakerChangeNotifications.php @@ -0,0 +1,99 @@ +transaction() returns. Methods that merely + * take a collector as a parameter only ever add to it, never dispatch it. That keeps a + * notification from outliving a save that ends up rolling back, without any caller having to + * describe its own transaction nesting. + * + * Class SpeakerChangeNotifications + * @package App\Services\Model\Imp\Notifications + */ +final class SpeakerChangeNotifications +{ + /** + * @var array + */ + private $pending = []; + + /** + * @param Presentation $presentation + * @param PresentationSpeaker $speaker + * @param string $role + * @param string $action + * @return void + */ + public function add(Presentation $presentation, PresentationSpeaker $speaker, string $role, string $action): void + { + $this->pending[] = [$presentation, $speaker, $role, $action]; + } + + /** + * @return bool + */ + public function isEmpty(): bool + { + return count($this->pending) === 0; + } + + /** + * Queues everything collected so far and empties the collector. + * + * By the time this runs the caller's write is already durable, so a notification failure + * must never propagate: it would surface as an HTTP error on a request that actually + * succeeded. The recipient is optional platform config, so an unconfigured deployment + * simply logs and sends nothing, and a single failing notification never cancels the rest. + * + * @return void + */ + public function dispatch(): void + { + $pending = $this->pending; + $this->pending = []; + + if (count($pending) === 0) return; + + if (empty(Config::get(PresentationActivitySpeakerChangeEmail::RecipientConfigKey))) { + Log::warning + ( + sprintf + ( + "SpeakerChangeNotifications::dispatch %s is not configured, skipping %s speaker change notification(s).", + PresentationActivitySpeakerChangeEmail::RecipientConfigKey, + count($pending) + ) + ); + return; + } + + foreach ($pending as $notification) { + try { + PresentationActivitySpeakerChangeEmail::dispatch(...$notification); + } catch (\Exception $ex) { + Log::warning("SpeakerChangeNotifications::dispatch failed to dispatch speaker change notification."); + Log::warning($ex); + } + } + } +} diff --git a/app/Services/Model/Imp/PresentationService.php b/app/Services/Model/Imp/PresentationService.php index 5c7ddfdf9..4d7dfb56f 100644 --- a/app/Services/Model/Imp/PresentationService.php +++ b/app/Services/Model/Imp/PresentationService.php @@ -34,7 +34,7 @@ use App\Services\Filesystem\FileUploadStrategyFactory; use App\Services\Model\AbstractService; use App\Services\Model\IFolderService; -use App\Services\Model\Imp\Traits\DispatchesSpeakerChangeNotifications; +use App\Services\Model\Imp\Notifications\SpeakerChangeNotifications; use Illuminate\Http\Request as LaravelRequest; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Log; @@ -70,8 +70,6 @@ final class PresentationService extends AbstractService implements IPresentationService { - use DispatchesSpeakerChangeNotifications; - const LocalChunkSize = 1024; /** * @var ISummitEventRepository @@ -1729,9 +1727,9 @@ public function processMediaUpload(int $summit_id, int $media_upload_type_id, ?s * @throws \Exception */ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, int $speaker_id, array $data): Presentation { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $presentation = $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data, &$pending_notifications) { + $presentation = $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $data, $notifications) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1744,12 +1742,13 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, if (!$presentation->isSpeaker($speaker)) { $presentation->addSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, PresentationActivitySpeakerChangeEmail::Action_Added - ]; + ); } } @@ -1762,7 +1761,8 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, return $presentation; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $presentation; } @@ -1776,9 +1776,9 @@ public function upsertPresentationSpeaker(Summit $summit, int $presentation_id, */ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_id, int $speaker_id): void { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, &$pending_notifications) { + $this->tx_service->transaction(function () use ($summit, $presentation_id, $speaker_id, $notifications) { $presentation = $summit->getEvent($presentation_id); if (!$presentation instanceof Presentation) @@ -1791,17 +1791,19 @@ public function removeSpeakerFromPresentation(Summit $summit, int $presentation_ if ($presentation->isSpeaker($speaker)) { $presentation->removeSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + ); } } }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); } } diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index acc531d94..4848e60b0 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -53,7 +53,7 @@ use App\Services\FileSystem\IFileUploadStrategy; use App\Services\Model\AbstractPublishService; use App\Services\Model\IMemberService; -use App\Services\Model\Imp\Traits\DispatchesSpeakerChangeNotifications; +use App\Services\Model\Imp\Notifications\SpeakerChangeNotifications; use App\Services\Utils\Security\IEncryptionAES256KeysGenerator; use DateInterval; use DateTime; @@ -128,8 +128,6 @@ final class SummitService extends AbstractPublishService implements ISummitService { - use DispatchesSpeakerChangeNotifications; - /** * @var ISummitEventRepository */ @@ -612,7 +610,11 @@ public function deleteMyEventFeedback(Member $member, Summit $summit, int $event */ public function addEvent(Summit $summit, array $data) { - return $this->saveOrUpdateEvent($summit, $data, null); + $notifications = new SpeakerChangeNotifications(); + $event = $this->saveOrUpdateEvent($summit, $data, $notifications, null); + // we own the collector, so our transaction() above was the outermost one + $notifications->dispatch(); + return $event; } /** @@ -621,9 +623,13 @@ public function addEvent(Summit $summit, array $data) * @param array $data * @return SummitEvent */ - public function updateEvent(Summit $summit, $event_id, array $data, bool $trigger_data_update = true, bool $saveAsIncomplete = false, ?array &$pending_notifications_out = null) + public function updateEvent(Summit $summit, $event_id, array $data, bool $trigger_data_update = true, bool $saveAsIncomplete = false) { - return $this->saveOrUpdateEvent($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete, $pending_notifications_out); + $notifications = new SpeakerChangeNotifications(); + $event = $this->saveOrUpdateEvent($summit, $data, $notifications, $event_id, $trigger_data_update, $saveAsIncomplete); + // we own the collector, so our transaction() above was the outermost one + $notifications->dispatch(); + return $event; } /** @@ -664,11 +670,9 @@ private function canPerformEventTypeTransition(SummitEventType $old_event_type, * @return SummitEvent * @throws Exception */ - private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null, bool $trigger_data_update = true, bool $saveAsIncomplete = false, ?array &$pending_notifications_out = null) + private function saveOrUpdateEvent(Summit $summit, array $data, SpeakerChangeNotifications $notifications, $event_id = null, bool $trigger_data_update = true, bool $saveAsIncomplete = false) { - $pending_speaker_changes = []; - - $event = $this->tx_service->transaction(function () use ($summit, $data, $event_id, $trigger_data_update, $saveAsIncomplete, &$pending_speaker_changes) { + return $this->tx_service->transaction(function () use ($summit, $data, $notifications, $event_id, $trigger_data_update, $saveAsIncomplete) { Log::debug ( @@ -839,7 +843,7 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null } } - $pending_speaker_changes = $this->saveOrUpdatePresentationData($event, $event_type, $data, $saveAsIncomplete); + $this->saveOrUpdatePresentationData($event, $event_type, $data, $notifications, $saveAsIncomplete); $this->saveOrUpdateSummitGroupEventData($event, $event_type, $data); if (!$event_type->isAllowsLocation()) @@ -867,27 +871,6 @@ private function saveOrUpdateEvent(Summit $summit, array $data, $event_id = null return $event; }); - - // $pending_notifications_out !== null means a bulk caller (updateEvents/updateAndPublishEvents) - // is wrapping this call in its OWN outer transaction: this method's transaction() above just - // returned but has not really committed yet (Doctrine nests via a connection-level counter), so - // dispatching here would outlive a rollback triggered by a later item in that caller's batch. - // Hand the pending changes back instead and let the bulk caller dispatch after ITS OWN commit. - if ($pending_notifications_out !== null) { - foreach ($pending_speaker_changes as $change) { - $pending_notifications_out[] = [$event, $change['speaker'], $change['role'], $change['action']]; - } - } else { - // standalone call (addEvent/updateEvent used directly): the transaction() call above - // was the outermost one, so its commit was real and dispatching now is safe - $pending_notifications = []; - foreach ($pending_speaker_changes as $change) { - $pending_notifications[] = [$event, $change['speaker'], $change['role'], $change['action']]; - } - $this->dispatchSpeakerChangeNotifications($pending_notifications); - } - - return $event; } private function saveOrUpdateSummitGroupEventData(SummitEvent $event, SummitEventType $event_type, array $data) @@ -909,16 +892,16 @@ private function saveOrUpdateSummitGroupEventData(SummitEvent $event, SummitEven * @param SummitEvent $event * @param SummitEventType $event_type * @param array $data + * @param SpeakerChangeNotifications $notifications collector this method only ever ADDS to; + * dispatching it belongs to whoever constructed it, after its own transaction commits. * @param bool $saveAsIncomplete - * @return array list of pending speaker/moderator change notifications, each - * ['speaker' => PresentationSpeaker, 'role' => string, 'action' => string]. - * The caller must dispatch these only after its own transaction commits. + * @return void * @throws EntityNotFoundException * @throws ValidationException */ - private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventType $event_type, array $data, bool $saveAsIncomplete = false): array + private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventType $event_type, array $data, SpeakerChangeNotifications $notifications, bool $saveAsIncomplete = false): void { - if (!$event instanceof Presentation) return []; + if (!$event instanceof Presentation) return; Log::debug(sprintf("SummitService::saveOrUpdatePresentationData presentation %s saveAsIncomplete %b", $event->getId(), $saveAsIncomplete)); if ($saveAsIncomplete && $event->isPublished()) @@ -931,8 +914,6 @@ private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventTyp $old_speaker_ids = array_map(fn(PresentationSpeaker $s) => $s->getId(), $event->getSpeakers()->toArray()); $old_moderator = $event->hasModerator() ? $event->getModerator() : null; - $pending_changes = []; - if (!$saveAsIncomplete || $event->isNew()) { // if we are creating the presentation from admin, then // we should mark it as received and complete @@ -972,18 +953,22 @@ private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventTyp if ($was_published) { $new_speaker_ids = array_map(fn(PresentationSpeaker $s) => $s->getId(), $event->getSpeakers()->toArray()); foreach (array_diff($new_speaker_ids, $old_speaker_ids) as $added_id) { - $pending_changes[] = [ - 'speaker' => $this->speaker_repository->getById($added_id), - 'role' => PresentationActivitySpeakerChangeEmail::Role_Speaker, - 'action' => PresentationActivitySpeakerChangeEmail::Action_Added - ]; + $notifications->add + ( + $event, + $this->speaker_repository->getById($added_id), + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Added + ); } foreach (array_diff($old_speaker_ids, $new_speaker_ids) as $removed_id) { - $pending_changes[] = [ - 'speaker' => $this->speaker_repository->getById($removed_id), - 'role' => PresentationActivitySpeakerChangeEmail::Role_Speaker, - 'action' => PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + $notifications->add + ( + $event, + $this->speaker_repository->getById($removed_id), + PresentationActivitySpeakerChangeEmail::Role_Speaker, + PresentationActivitySpeakerChangeEmail::Action_Removed + ); } } } @@ -1017,26 +1002,28 @@ private function saveOrUpdatePresentationData(SummitEvent $event, SummitEventTyp if ($old_moderator_id !== $new_moderator_id) { if (!is_null($old_moderator)) { - $pending_changes[] = [ - 'speaker' => $old_moderator, - 'role' => PresentationActivitySpeakerChangeEmail::Role_Moderator, - 'action' => PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + $notifications->add + ( + $event, + $old_moderator, + PresentationActivitySpeakerChangeEmail::Role_Moderator, + PresentationActivitySpeakerChangeEmail::Action_Removed + ); } if (!is_null($new_moderator)) { - $pending_changes[] = [ - 'speaker' => $new_moderator, - 'role' => PresentationActivitySpeakerChangeEmail::Role_Moderator, - 'action' => PresentationActivitySpeakerChangeEmail::Action_Added - ]; + $notifications->add + ( + $event, + $new_moderator, + PresentationActivitySpeakerChangeEmail::Role_Moderator, + PresentationActivitySpeakerChangeEmail::Action_Added + ); } } } } PresentationFactory::populate($event, $data, true); - - return $pending_changes; } /** @@ -1569,22 +1556,25 @@ public function unPublishEvents(Summit $summit, array $data) */ public function updateAndPublishEvents(Summit $summit, array $data) { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); $result = $this->tx_service->transaction(function () use ( $summit, $data, - &$pending_notifications + $notifications ) { foreach ($data['events'] as $event_data) { - $this->updateEvent($summit, intval($event_data['id']), $event_data, true, false, $pending_notifications); + // saveOrUpdateEvent directly, not updateEvent: updateEvent owns its own collector + // and would dispatch while this outer transaction is still open + $this->saveOrUpdateEvent($summit, $event_data, $notifications, intval($event_data['id'])); $this->publishEvent($summit, intval($event_data['id']), $event_data); } return true; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $result; } @@ -1599,22 +1589,25 @@ public function updateAndPublishEvents(Summit $summit, array $data) */ public function updateEvents(Summit $summit, array $data, bool $trigger_data_update = true) { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); $result = $this->tx_service->transaction(function () use ( $summit, $data, $trigger_data_update, - &$pending_notifications + $notifications ) { foreach ($data['events'] as $event_data) { - $this->updateEvent($summit, intval($event_data['id']), $event_data, $trigger_data_update, false, $pending_notifications); + // saveOrUpdateEvent directly, not updateEvent: updateEvent owns its own collector + // and would dispatch while this outer transaction is still open + $this->saveOrUpdateEvent($summit, $event_data, $notifications, intval($event_data['id']), $trigger_data_update); } return true; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $result; } @@ -1828,9 +1821,9 @@ public function deleteSummit($summit_id) */ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, $notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1861,12 +1854,13 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, if (!$presentation->isSpeaker($speaker)) { $presentation->addSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, PresentationActivitySpeakerChangeEmail::Action_Added - ]; + ); } } @@ -1884,7 +1878,8 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, return $presentation; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $presentation; } @@ -1899,9 +1894,9 @@ public function addSpeaker2Presentation(int $current_member_id, int $speaker_id, */ public function removeSpeakerFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, $notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -1934,19 +1929,21 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak if ($presentation->isSpeaker($speaker)) { $presentation->removeSpeaker($speaker); if ($presentation->isPublished()) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Speaker, PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + ); } } return $presentation; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $presentation; } @@ -1961,9 +1958,9 @@ public function removeSpeakerFromPresentation(int $current_member_id, int $speak */ public function addModerator2Presentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, $notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) throw new EntityNotFoundException(sprintf("Member %s not found.", $current_member_id)); @@ -1999,19 +1996,21 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i if ($presentation->isPublished() && $previous_moderator_id !== $speaker->getId()) { if (!is_null($previous_moderator)) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $previous_moderator, PresentationActivitySpeakerChangeEmail::Role_Moderator, PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + ); } - $pending_notifications[] = [ + $notifications->add + ( $presentation, $speaker, PresentationActivitySpeakerChangeEmail::Role_Moderator, PresentationActivitySpeakerChangeEmail::Action_Added - ]; + ); } // check is selection plan is private, if so add moderator to allowed members @@ -2028,7 +2027,8 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i return $presentation; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $presentation; } @@ -2043,9 +2043,9 @@ public function addModerator2Presentation(int $current_member_id, int $speaker_i */ public function removeModeratorFromPresentation(int $current_member_id, int $speaker_id, int $presentation_id): Presentation { - $pending_notifications = []; + $notifications = new SpeakerChangeNotifications(); - $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, &$pending_notifications) { + $presentation = $this->tx_service->transaction(function () use ($current_member_id, $speaker_id, $presentation_id, $notifications) { $current_member = $this->member_repository->getById($current_member_id); if (is_null($current_member) || !($current_member instanceof Member)) @@ -2080,18 +2080,20 @@ public function removeModeratorFromPresentation(int $current_member_id, int $spe $presentation->unsetModerator(); if (!is_null($previous_moderator) && $presentation->isPublished()) { - $pending_notifications[] = [ + $notifications->add + ( $presentation, $previous_moderator, PresentationActivitySpeakerChangeEmail::Role_Moderator, PresentationActivitySpeakerChangeEmail::Action_Removed - ]; + ); } return $presentation; }); - $this->dispatchSpeakerChangeNotifications($pending_notifications); + // we own the collector, so nothing goes out until OUR transaction has committed + $notifications->dispatch(); return $presentation; } diff --git a/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php b/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php deleted file mode 100644 index 2a2e2e309..000000000 --- a/app/Services/Model/Imp/Traits/DispatchesSpeakerChangeNotifications.php +++ /dev/null @@ -1,64 +0,0 @@ -