Skip to content

Commit b3dfeba

Browse files
committed
Address arch-review polish items
- Move the HistoryAuditMonitor import to the top of lib/galaxy/app — it's a pure Python class with no import-time side effects, so the conditional import added nothing. Drop the self._history_audit_monitor attribute and resolve the monitor via self[HistoryAuditMonitor] in the shutdown path so it stays off the app surface. - Inject NotificationManager into ShareableService directly instead of reaching self.notification_service.notification_manager from inside sharable.py. Thread the new param through HistoriesService, PagesService, VisualizationsService and WorkflowsService (all Lagom-resolved, so there are no manual call sites to update). - Add missing return annotations: EventsService.__init__ -> None, SSELineListener.__init__ -> None. Declare _PgListenAdapter._conn / .driver once at the class level so the psycopg / psycopg2 branches can assign without re-annotating the same attribute. - Tighten the historyStore idempotency test: assert expect(deltaAfterSecond).toBe(1) instead of toBeLessThanOrEqual(1) so a no-op second startWatchingHistory fails the test rather than trivially passing. Add a comment to the SSE-event test noting that the mocked refreshHistoryFromPush asserts dispatch only; the actual refresh is covered by the Selenium SSE integration tests.
1 parent a949308 commit b3dfeba

10 files changed

Lines changed: 46 additions & 21 deletions

File tree

client/src/stores/historyStore.test.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ describe("historyStore — config-driven SSE vs polling", () => {
114114
});
115115

116116
it("triggers refreshHistoryFromPush when an SSE event names the current history", async () => {
117+
// This test asserts the store's *decision* to refresh, not the refresh
118+
// itself — ``refreshHistoryFromPush`` is mocked so we can observe the
119+
// dispatch. The real refresh is covered end-to-end in the Selenium
120+
// SSE integration tests (see test/integration_selenium/test_history_sse.py).
117121
const store = useHistoryStore();
118122
await primeStore(() => store.startWatchingHistory());
119123
// Drive the store to a known current-history id so the handler has
@@ -181,8 +185,10 @@ describe("historyStore — config-driven SSE vs polling", () => {
181185
// not two.
182186
await vi.advanceTimersByTimeAsync(3000);
183187
await flushPromises();
188+
// Exactly one additional poll after the 3000ms advance — anything
189+
// else means a second independent polling loop was scheduled.
184190
const deltaAfterSecond = mockWatchHistory.mock.calls.length - pollsAfterFirst;
185-
expect(deltaAfterSecond).toBeLessThanOrEqual(1);
191+
expect(deltaAfterSecond).toBe(1);
186192
});
187193
});
188194
});

lib/galaxy/app/__init__.py

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
from galaxy.managers.folders import FolderManager
6666
from galaxy.managers.hdas import HDAManager
6767
from galaxy.managers.histories import HistoryManager
68+
from galaxy.managers.history_audit_monitor import HistoryAuditMonitor
6869
from galaxy.managers.interactivetool import InteractiveToolManager
6970
from galaxy.managers.jobs import (
7071
JobManager as JobQueryManager,
@@ -1018,10 +1019,8 @@ def __init__(self, **kwargs) -> None:
10181019
# up. start/stop are driven by heartbeat role transitions rather than
10191020
# postfork, so the monitor cleanly migrates when the leader dies.
10201021
if self.config.enable_sse_history_updates:
1021-
from galaxy.managers.history_audit_monitor import HistoryAuditMonitor
1022-
1023-
self._history_audit_monitor = self._register_singleton(HistoryAuditMonitor)
1024-
self.database_heartbeat.add_audit_monitor_change_callback(self._history_audit_monitor.on_role_change)
1022+
monitor = self._register_singleton(HistoryAuditMonitor)
1023+
self.database_heartbeat.add_audit_monitor_change_callback(monitor.on_role_change)
10251024

10261025
# Start web stack message handling
10271026
self.application_stack.register_postfork_function(self.application_stack.start)
@@ -1058,9 +1057,9 @@ def _shutdown_database_heartbeat(self):
10581057
self.database_heartbeat.shutdown()
10591058

10601059
def _shutdown_history_audit_monitor(self):
1061-
monitor = getattr(self, "_history_audit_monitor", None)
1062-
if monitor:
1063-
monitor.shutdown()
1060+
if not self.config.enable_sse_history_updates:
1061+
return
1062+
self[HistoryAuditMonitor].shutdown()
10641063

10651064
def _shutdown_scheduling_manager(self):
10661065
self.workflow_scheduling_manager.shutdown()

lib/galaxy/managers/history_audit_monitor.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,19 @@ class _PgListenAdapter:
5555
live for the lifetime of the monitor and never be returned to the pool.
5656
"""
5757

58+
# Typed once at the class level so both driver branches can assign without
59+
# re-annotating the same attribute.
60+
_conn: Any
61+
driver: str
62+
5863
def __init__(self, engine: Engine) -> None:
5964
# Strip the SA ``+driver`` suffix so the raw DBAPI libraries accept the URL.
6065
dsn = engine.url.set(drivername="postgresql").render_as_string(hide_password=False)
6166
driver = engine.dialect.driver
6267
if driver == "psycopg":
6368
import psycopg # conditional: psycopg3 driver
6469

65-
self._conn: Any = psycopg.connect(dsn, autocommit=True)
70+
self._conn = psycopg.connect(dsn, autocommit=True)
6671
self.driver = "psycopg3"
6772
else:
6873
import psycopg2 # conditional: psycopg2 driver

lib/galaxy/webapps/galaxy/services/events.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323

2424
class EventsService(ServiceBase):
25-
def __init__(self, sse_manager: SSEConnectionManager, notifications: NotificationService):
25+
def __init__(self, sse_manager: SSEConnectionManager, notifications: NotificationService) -> None:
2626
self.sse_manager = sse_manager
2727
self.notifications = notifications
2828

lib/galaxy/webapps/galaxy/services/histories.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@
9494
ServiceBase,
9595
tool_request_to_model,
9696
)
97+
from galaxy.managers.notification import NotificationManager
9798
from galaxy.webapps.galaxy.services.notifications import NotificationService
9899
from galaxy.webapps.galaxy.services.sharable import ShareableService
99100
from galaxy.workflow.extract import summarize
@@ -129,6 +130,7 @@ def __init__(
129130
filters: HistoryFilters,
130131
short_term_storage_allocator: ShortTermStorageAllocator,
131132
notification_service: NotificationService,
133+
notification_manager: NotificationManager,
132134
):
133135
super().__init__(security)
134136
self.manager = manager
@@ -138,7 +140,9 @@ def __init__(
138140
self.citations_manager = citations_manager
139141
self.history_export_manager = history_export_manager
140142
self.filters = filters
141-
self.shareable_service = ShareableHistoryService(self.manager, self.serializer, notification_service)
143+
self.shareable_service = ShareableHistoryService(
144+
self.manager, self.serializer, notification_service, notification_manager
145+
)
142146
self.short_term_storage_allocator = short_term_storage_allocator
143147

144148
def index(

lib/galaxy/webapps/galaxy/services/pages.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
ensure_celery_tasks_enabled,
3939
ServiceBase,
4040
)
41+
from galaxy.managers.notification import NotificationManager
4142
from galaxy.webapps.galaxy.services.notifications import NotificationService
4243
from galaxy.webapps.galaxy.services.sharable import ShareableService
4344

@@ -58,11 +59,14 @@ def __init__(
5859
serializer: PageSerializer,
5960
short_term_storage_allocator: ShortTermStorageAllocator,
6061
notification_service: NotificationService,
62+
notification_manager: NotificationManager,
6163
):
6264
super().__init__(security)
6365
self.manager = manager
6466
self.serializer = serializer
65-
self.shareable_service = ShareableService(self.manager, self.serializer, notification_service)
67+
self.shareable_service = ShareableService(
68+
self.manager, self.serializer, notification_service, notification_manager
69+
)
6670
self.short_term_storage_allocator = short_term_storage_allocator
6771

6872
def index(

lib/galaxy/webapps/galaxy/services/sharable.py

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
)
66

77
from galaxy.managers import base
8+
from galaxy.managers.notification import NotificationManager
89
from galaxy.managers.sharable import (
910
SharableModelManager,
1011
SharableModelSerializer,
@@ -62,10 +63,12 @@ def __init__(
6263
manager: SharableModelManager,
6364
serializer: SharableModelSerializer,
6465
notification_service: NotificationService,
66+
notification_manager: NotificationManager,
6567
) -> None:
6668
self.manager = manager
6769
self.serializer = serializer
6870
self.notification_service = notification_service
71+
self.notification_manager = notification_manager
6972

7073
def set_slug(self, trans, id: DecodedDatabaseIdField, payload: SetSlugPayload):
7174
item = self._get_item_by_id(trans, id)
@@ -177,17 +180,13 @@ def _get_users(self, trans, emails_or_ids: list[UserIdentifier]) -> tuple[set[Us
177180
def _send_notification_to_users(
178181
self, users_to_notify: set[User], item: SharableItem, status: ShareWithStatus, galaxy_url: Optional[str] = None
179182
):
180-
if (
181-
self.notification_service.notification_manager.notifications_enabled
182-
and not status.errors
183-
and users_to_notify
184-
):
183+
if self.notification_manager.notifications_enabled and not status.errors and users_to_notify:
185184
request = SharedItemNotificationFactory.build_notification_request(
186185
item, users_to_notify, status, galaxy_url
187186
)
188187
# We can set force_sync=True here because we already have the set of users to notify
189188
# and there is no need to resolve them asynchronously as no groups or roles are involved.
190-
self.notification_service.notification_manager.send_notification_internal(request, force_sync=True)
189+
self.notification_manager.send_notification_internal(request, force_sync=True)
191190

192191

193192
class SharedItemNotificationFactory:

lib/galaxy/webapps/galaxy/services/visualizations.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from galaxy import exceptions
1010
from galaxy.managers.base import security_check
1111
from galaxy.managers.context import ProvidesUserContext
12+
from galaxy.managers.notification import NotificationManager
1213
from galaxy.managers.sharable import (
1314
slug_exists,
1415
SlugBuilder,
@@ -62,11 +63,14 @@ def __init__(
6263
manager: VisualizationManager,
6364
serializer: VisualizationSerializer,
6465
notification_service: NotificationService,
66+
notification_manager: NotificationManager,
6567
):
6668
super().__init__(security)
6769
self.manager = manager
6870
self.serializer = serializer
69-
self.shareable_service = ShareableService(self.manager, self.serializer, notification_service)
71+
self.shareable_service = ShareableService(
72+
self.manager, self.serializer, notification_service, notification_manager
73+
)
7074

7175
# TODO: add the rest of the API actions here and call them directly from the API controller
7276

lib/galaxy/webapps/galaxy/services/workflows.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
web,
1313
)
1414
from galaxy.managers.context import ProvidesUserContext
15+
from galaxy.managers.notification import NotificationManager
1516
from galaxy.managers.workflows import (
1617
RefactorRequest,
1718
RefactorResponse,
@@ -57,11 +58,14 @@ def __init__(
5758
serializer: WorkflowSerializer,
5859
tool_shed_registry: Registry,
5960
notification_service: NotificationService,
61+
notification_manager: NotificationManager,
6062
):
6163
self._workflows_manager = workflows_manager
6264
self._workflow_contents_manager = workflow_contents_manager
6365
self._serializer = serializer
64-
self.shareable_service = ShareableService(workflows_manager, serializer, notification_service)
66+
self.shareable_service = ShareableService(
67+
workflows_manager, serializer, notification_service, notification_manager
68+
)
6569
self._tool_shed_registry = tool_shed_registry
6670

6771
def index(

lib/galaxy_test/base/sse.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ def __init__(
6464
api_key: str,
6565
headers: Optional[dict] = None,
6666
timeout: int = 30,
67-
):
67+
) -> None:
6868
self.url = url
6969
self.api_key = api_key
7070
self.headers = headers or {}

0 commit comments

Comments
 (0)