Skip to content

Commit 6017887

Browse files
committed
fix: dispatch email channel when force_sync=True and celery is enabled
When `force_sync=True` is passed to `send_notification_internal`, the notification row was created and SSE events fired, but email (and any other async channel) was never dispatched because the channel plugins are only registered when celery tasks are enabled. This meant tool-request notifications — which the service layer explicitly forces to sync so the client gets a response ID — would not deliver email to admins. Changes: - Add `_dispatch_notification_via_channels` to `NotificationManager` that marks the row dispatched and then calls `_dispatch_notification_to_users`, mirroring the pattern in `dispatch_pending_notifications_via_channels`. - Call it inside `send_notification_internal` when `force_sync=True` and celery is enabled (i.e. channels are registered). - Deduplicate the sync/async branching that was duplicated between `NotificationService.send_notification_internal` and the manager: the service now delegates directly to the manager. - Make `contact_email` and `notification_settings_url` on `NotificationContext` `Optional` (the builder already produced `None` for both when config is absent; templates already guard with `{% if %}`). - Add unit test asserting email channel fires on `force_sync=True`. - Add integration test with mock SMTP verifying end-to-end email delivery for tool-request submissions when celery tasks are enabled.
1 parent b4fe427 commit 6017887

4 files changed

Lines changed: 84 additions & 16 deletions

File tree

lib/galaxy/managers/notification.py

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,9 @@ def send_notification_internal(
194194
) -> Union[NotificationCreatedResponse, AsyncTaskResultSummary]:
195195
"""Sends a notification to a list of recipients (users, groups or roles).
196196
197-
If `force_sync` is set to `True`, the notification recipients will be processed synchronously instead of
198-
in a background task.
197+
If `force_sync` is set to `True`, recipient association creation and any
198+
configured channel delivery will be processed synchronously instead of in a
199+
background task.
199200
200201
Note: This function is meant for internal use from other callers that don't need to check sender permissions.
201202
"""
@@ -208,11 +209,22 @@ def send_notification_internal(
208209
return async_task_summary(result)
209210

210211
notification, recipient_user_count = self.send_notification_to_recipients(request)
212+
if notification is None:
213+
raise RuntimeError("Expected notification to be created before delivery.")
214+
if self.can_send_notifications_async and force_sync:
215+
self._dispatch_notification_via_channels(notification)
211216
return NotificationCreatedResponse(
212217
total_notifications_sent=recipient_user_count,
213218
notification=NotificationResponse.model_validate(notification),
214219
)
215220

221+
def _dispatch_notification_via_channels(self, notification: Notification) -> None:
222+
# Mark dispatched before delivery — same trade-off as dispatch_pending_notifications_via_channels:
223+
# a transient SMTP failure leaves the row marked and won't be retried by the beat task.
224+
notification.dispatched = True
225+
self.sa_session.commit()
226+
self._dispatch_notification_to_users(notification)
227+
216228
def _create_associations(self, notification: Notification, users: list[User]) -> int:
217229
success_count = 0
218230
for user in users:
@@ -723,9 +735,9 @@ class NotificationContext(BaseModel):
723735
user_email: str
724736
date: str
725737
hostname: str
726-
contact_email: str
738+
contact_email: Optional[str] = None
727739
variant: str
728-
notification_settings_url: str
740+
notification_settings_url: Optional[str] = None
729741
content: AnyNotificationContent
730742
workflow_name: Optional[str] = None
731743
galaxy_url: Optional[str] = None

lib/galaxy/webapps/galaxy/services/notifications.py

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99
Union,
1010
)
1111

12-
from galaxy.celery.helpers import async_task_summary
13-
from galaxy.celery.tasks import send_notification_to_recipients_async
1412
from galaxy.config import GalaxyAppConfiguration
1513
from galaxy.exceptions import (
1614
AdminRequiredException,
@@ -177,16 +175,7 @@ def send_notification_internal(
177175
178176
Note: This function is meant for internal use from other services that don't need to check sender permissions.
179177
"""
180-
if self.notification_manager.can_send_notifications_async and not force_sync:
181-
result = send_notification_to_recipients_async.delay(request)
182-
summary = async_task_summary(result)
183-
return summary
184-
185-
notification, recipient_user_count = self.notification_manager.send_notification_to_recipients(request)
186-
return NotificationCreatedResponse(
187-
total_notifications_sent=recipient_user_count,
188-
notification=NotificationResponse.model_validate(notification),
189-
)
178+
return self.notification_manager.send_notification_internal(request, force_sync=force_sync)
190179

191180
def broadcast(
192181
self, sender_context: ProvidesUserContext, payload: BroadcastNotificationCreateRequest

test/integration/test_tool_request_form.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
"""Integration tests for the tool-request-form feature (routed via POST /api/notifications)."""
22

3+
import json
4+
import os
5+
from typing import ClassVar
6+
37
from galaxy_test.base.api_util import ADMIN_TEST_USER
48
from galaxy_test.driver.integration_util import IntegrationTestCase
59

@@ -210,6 +214,48 @@ def test_workflow_install_request_with_tool_names(self):
210214
assert len(content["tool_names"]) == 2
211215

212216

217+
class TestToolRequestFormEmailDeliveryIntegration(ToolRequestFormIntegrationBase):
218+
email_directory: ClassVar[str]
219+
220+
@classmethod
221+
def handle_galaxy_config_kwds(cls, config):
222+
super().handle_galaxy_config_kwds(config)
223+
cls.email_directory = cls._test_driver.mkdtemp()
224+
config["enable_celery_tasks"] = True
225+
config["email_from"] = "galaxy-no-reply@example.com"
226+
config["smtp_server"] = f"mock_emails_to_path://{cls.email_directory}/email.json"
227+
228+
def test_submission_sends_admin_email_when_force_sync_is_required(self):
229+
"""Tool requests should still deliver email notifications when the API forces synchronous processing."""
230+
user = self._setup_user("tool_request_email_delivery@galaxy.test")
231+
with self._different_user(user["email"]):
232+
update_request = {
233+
"preferences": {
234+
"tool_request": {
235+
"enabled": True,
236+
"channels": {"push": True, "email": False},
237+
}
238+
}
239+
}
240+
update_response = self._put("notifications/preferences", data=update_request, json=True)
241+
self._assert_status_code_is_ok(update_response)
242+
243+
response = self._post("notifications", data=TOOL_REQUEST_NOTIFICATION_BODY, json=True)
244+
self._assert_status_code_is(response, 200)
245+
246+
with open(os.path.join(self.email_directory, "email.json")) as f:
247+
email = json.loads(f.read())
248+
249+
assert email["from"] == "galaxy-no-reply@example.com"
250+
assert email["to"] == ADMIN_TEST_USER
251+
assert email["subject"] == "[Galaxy] Tool installation request: FastQC"
252+
assert user["email"] in email["body"]
253+
assert "Quality control tool for high-throughput sequencing data." in email["body"]
254+
assert email["html"] is not None
255+
assert "Would be great for the genomics team." in email["html"]
256+
assert "/user/notifications" in email["html"]
257+
258+
213259
class TestToolRequestFormDisabledIntegration(IntegrationTestCase):
214260
"""Tests for when the tool request form feature is disabled."""
215261

test/unit/app/managers/test_NotificationManager.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@
2424
Role,
2525
User,
2626
)
27+
from galaxy.schema.fields import Security
2728
from galaxy.schema.notifications import (
2829
BroadcastNotificationContent,
2930
BroadcastNotificationCreateRequest,
3031
NotificationBroadcastUpdateRequest,
3132
NotificationCategorySettings,
3233
NotificationChannelSettings,
3334
NotificationCreateData,
35+
NotificationCreatedResponse,
3436
NotificationCreateRequest,
3537
NotificationRecipients,
3638
NotificationVariant,
@@ -39,6 +41,7 @@
3941
UserNotificationPreferences,
4042
UserNotificationUpdateRequest,
4143
)
44+
from galaxy.security.idencoding import IdEncodingHelper
4245
from .base import BaseTestCase
4346

4447

@@ -469,6 +472,24 @@ def validate_send_email(frm, to, subject, body, config, html=None):
469472
mock_send_mail.assert_called_once()
470473
assert len(emails_sent) == 1
471474

475+
def test_force_sync_dispatches_email_channel_when_async_is_enabled(self):
476+
user = self._create_test_user()
477+
Security.security = IdEncodingHelper(id_secret="testing")
478+
479+
request = NotificationCreateRequest(
480+
recipients=NotificationRecipients.model_construct(user_ids=[user.id]),
481+
notification=NotificationCreateData(**self._default_test_notification_data()),
482+
galaxy_url="https://test.galaxy.url",
483+
)
484+
485+
with patch("galaxy.util.send_mail") as mock_send_mail:
486+
response = self.notification_manager.send_notification_internal(request, force_sync=True)
487+
488+
assert isinstance(response, NotificationCreatedResponse)
489+
assert response.total_notifications_sent == 1
490+
mock_send_mail.assert_called_once()
491+
assert "email" in self.notification_manager.get_supported_channels()
492+
472493

473494
class TestNotificationRecipientResolver(NotificationsBaseTestCase):
474495
def test_default_resolution_strategy(self):

0 commit comments

Comments
 (0)