From 209c3b53f730dc77697e93c31c5ead68107bd8b5 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:02:35 -0400 Subject: [PATCH 01/10] add support for feed scoped notifications --- api/src/shared/database/users_database.py | 24 ++- .../notification_subscription_impl.py | 5 + .../user_service/impl/subscription_helpers.py | 11 ++ api/src/user_service/impl/users_api_impl.py | 51 ++++++- .../user_service/test_subscription_feeds.py | 144 ++++++++++++++++++ .../test_subscriptions_api_impl.py | 12 ++ .../user_service/test_users_api_impl.py | 96 +++++++++++- docs/UserServiceAPI.yaml | 20 +++ liquibase/changelog_user.xml | 4 + liquibase/changes_user/feat_1778.sql | 26 ++++ liquibase/test_data/users_test_data.sql | 13 +- 11 files changed, 394 insertions(+), 12 deletions(-) create mode 100644 api/tests/unittest/user_service/test_subscription_feeds.py create mode 100644 liquibase/changes_user/feat_1778.sql diff --git a/api/src/shared/database/users_database.py b/api/src/shared/database/users_database.py index 397545457..b573bc2b4 100644 --- a/api/src/shared/database/users_database.py +++ b/api/src/shared/database/users_database.py @@ -119,11 +119,17 @@ def wrapper(*args, **kwargs): @event.listens_for(mapper, "mapper_configured") def _configure_users_passive_deletes(mapper_, class_): - """Enable ``passive_deletes`` on ``NotificationSubscription.notification_logs``. + """Enable ``delete-orphan`` + ``passive_deletes`` on ``NotificationSubscription`` children. - Deleting a subscription then relies on the database ``ON DELETE CASCADE`` to remove its - ``notification_log`` rows, instead of SQLAlchemy trying to NULL the NOT NULL - ``notification_log.subscription_id`` (which would raise a NotNullViolation). + Applies to both child collections: + * ``notification_logs`` — delivery history rows. + * ``notification_subscription_feeds`` — feed-scoped subscription targets (issue #1778). + + Deleting a subscription then relies on the database ``ON DELETE CASCADE`` to remove these + rows, instead of SQLAlchemy trying to NULL the NOT NULL child foreign keys + (``notification_log.subscription_id`` / ``notification_subscription_feed.subscription_id``), + which would raise a NotNullViolation. ``delete-orphan`` additionally lets a feed be dropped + from a subscription simply by removing it from the in-memory collection. This mirrors the ``set_cascade`` mechanism in ``shared.database.database`` but is scoped to the users models, so ``users_database`` stays independent of the feeds ``database_gen``. @@ -132,7 +138,9 @@ def _configure_users_passive_deletes(mapper_, class_): module does not require ``shared.users_database_gen`` to be present (functions that symlink ``shared`` without the users models must still be able to import ``users_database``). """ - if class_.__name__ == "NotificationSubscription" and "notification_logs" in mapper_.relationships: - rel = mapper_.relationships["notification_logs"] - rel.cascade = "all, delete-orphan" - rel.passive_deletes = True + if class_.__name__ == "NotificationSubscription": + for rel_name in ("notification_logs", "notification_subscription_feeds"): + if rel_name in mapper_.relationships: + rel = mapper_.relationships[rel_name] + rel.cascade = "all, delete-orphan" + rel.passive_deletes = True diff --git a/api/src/shared/db_models/notification_subscription_impl.py b/api/src/shared/db_models/notification_subscription_impl.py index cec6ae93d..4c61fe1c8 100644 --- a/api/src/shared/db_models/notification_subscription_impl.py +++ b/api/src/shared/db_models/notification_subscription_impl.py @@ -14,10 +14,15 @@ class Config: def from_orm(cls, sub: NotificationSubscriptionOrm | None) -> NotificationSubscription | None: if not sub: return None + # feed_ids is the set of feed stable IDs this subscription targets, taken from the + # notification_subscription_feed join table. Sorted for a stable response; None when + # the subscription targets no feeds (i.e. non feed-scoped types). + feed_ids = sorted(f.feed_stable_id for f in sub.notification_subscription_feeds) return cls( id=sub.id, user_id=sub.user_id, notification_id=sub.notification_type_id, active=sub.active, created_at=sub.created_at, + feed_ids=feed_ids or None, ) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index abd99b4f1..0e170b9df 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -27,6 +27,17 @@ ANNOUNCEMENTS_NOTIFICATION_TYPE_ID = "api.announcements" +# Notification types that are scoped to specific feeds. A subscription to any of +# these must carry a non-empty list of feed stable IDs (persisted in the +# notification_subscription_feed join table); other types must not carry feeds. +FEED_SCOPED_NOTIFICATION_TYPE_IDS = frozenset( + { + "feed.url_updated", + "feed.url_availability", + "feed.coverage", + } +) + def sync_announcements(email: str, subscribe: bool, subscription_id: str | None = None) -> None: """Sync an api.announcements subscription with Brevo, mapping provider errors to 502.""" diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index eb3f5a4c2..cc2e1ab40 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -32,10 +32,12 @@ FeatureFlag, UserFeatureFlag, NotificationSubscription as NotificationSubscriptionOrm, + NotificationSubscriptionFeed as NotificationSubscriptionFeedOrm, NotificationType, ) from user_service.impl.subscription_helpers import ( ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, + FEED_SCOPED_NOTIFICATION_TYPE_IDS, sync_announcements, ) from user_service_gen.apis.users_api_base import BaseUsersApi @@ -130,6 +132,7 @@ def get_user_subscriptions(self, db_session=None) -> List[NotificationSubscripti user_id = self._require_user_id() subs = ( db_session.query(NotificationSubscriptionOrm) + .options(selectinload(NotificationSubscriptionOrm.notification_subscription_feeds)) .filter(NotificationSubscriptionOrm.user_id == user_id) .order_by(NotificationSubscriptionOrm.created_at) .all() @@ -140,13 +143,35 @@ def get_user_subscriptions(self, db_session=None) -> List[NotificationSubscripti def create_user_subscription( self, create_notification_subscription_request: CreateNotificationSubscriptionRequest, db_session=None ) -> NotificationSubscription: - """Subscribes the authenticated user to a notification type (idempotent).""" + """Subscribes the authenticated user to a notification type (idempotent). + + For feed-scoped notification types (``feed.url_updated``, ``feed.url_availability``, + ``feed.coverage``) a non-empty ``feed_ids`` list is required and is persisted in the + ``notification_subscription_feed`` join table; the single (user, type) subscription's + feed set is replaced with the supplied feeds. ``feed_ids`` must not be supplied for + other notification types. + """ user_id = self._require_user_id() notification_id = create_notification_subscription_request.notification_id + # De-duplicate while preserving order; treat null as no feeds. + feed_ids = list(dict.fromkeys(create_notification_subscription_request.feed_ids or [])) if db_session.get(NotificationType, notification_id) is None: raise HTTPException(status_code=400, detail=f"Unknown notification type '{notification_id}'.") + # Feed scoping is validated in code (the OpenAPI 3.0 schema keeps feed_ids optional). + is_feed_scoped = notification_id in FEED_SCOPED_NOTIFICATION_TYPE_IDS + if is_feed_scoped and not feed_ids: + raise HTTPException( + status_code=400, + detail=f"feed_ids is required for notification type '{notification_id}'.", + ) + if not is_feed_scoped and feed_ids: + raise HTTPException( + status_code=400, + detail=f"feed_ids is not supported for notification type '{notification_id}'.", + ) + user = db_session.get(AppUser, user_id) if user is None: raise HTTPException(status_code=404, detail="User not found.") @@ -168,6 +193,10 @@ def create_user_subscription( ) sub.active = True + # Replace the subscription's feed set with the supplied feeds (no-op for non-scoped + # types, whose feed_ids is always empty here). + self._sync_subscription_feeds(sub, feed_ids) + if notification_id == ANNOUNCEMENTS_NOTIFICATION_TYPE_ID: sync_announcements(user.email, subscribe=True, subscription_id=sub.id) @@ -226,6 +255,26 @@ def _require_user_id() -> str: raise HTTPException(status_code=403, detail="Guest users cannot perform this action.") return user_id + @staticmethod + def _sync_subscription_feeds(sub: NotificationSubscriptionOrm, feed_ids: List[str]) -> None: + """Make the subscription's feed set exactly ``feed_ids``. + + Relies on the ``delete-orphan`` cascade configured on + ``NotificationSubscription.notification_subscription_feeds`` (see + ``shared.database.users_database``): removing a row from the collection deletes it and + appending one inserts it on flush. + """ + desired = set(feed_ids) + current = {row.feed_stable_id: row for row in sub.notification_subscription_feeds} + + for stable_id, row in current.items(): + if stable_id not in desired: + sub.notification_subscription_feeds.remove(row) + + for stable_id in feed_ids: + if stable_id not in current: + sub.notification_subscription_feeds.append(NotificationSubscriptionFeedOrm(feed_stable_id=stable_id)) + @staticmethod def _get_owned_subscription(db_session, sub_id: str, user_id: str) -> NotificationSubscriptionOrm: sub = db_session.get(NotificationSubscriptionOrm, sub_id) diff --git a/api/tests/unittest/user_service/test_subscription_feeds.py b/api/tests/unittest/user_service/test_subscription_feeds.py new file mode 100644 index 000000000..fe1dbf570 --- /dev/null +++ b/api/tests/unittest/user_service/test_subscription_feeds.py @@ -0,0 +1,144 @@ +# +# MobilityData 2026 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""DB-backed tests for feed-scoped notification subscriptions (issue #1778). + +These exercise the full create path (validation, persistence into the +``notification_subscription_feed`` join table, idempotent feed-set replacement) and the +``ON DELETE CASCADE`` / ``delete-orphan`` behaviour against the real users test database. +""" + +import uuid + +import pytest +from fastapi import HTTPException +from sqlalchemy.orm import configure_mappers + +from middleware.request_context import _request_context + +# Importing the module registers the mapper_configured listener under test. +from shared.database.users_database import UsersDatabase +from shared.users_database_gen.sqlacodegen_models import ( + AppUser, + NotificationSubscription, + NotificationSubscriptionFeed, + NotificationType, +) +from user_service.impl.users_api_impl import UsersApiImpl +from user_service_gen.models.create_notification_subscription_request import ( + CreateNotificationSubscriptionRequest, +) + +FEED_SCOPED_TYPE = "feed.url_updated" + + +def _reset_singleton(): + UsersDatabase.instance = None + UsersDatabase.initialized = False + + +def test_feed_scoped_relationship_uses_delete_orphan_and_passive_deletes(): + """Regression guard: the listener configures the feed join collection for DB-driven cascade.""" + configure_mappers() + rel = NotificationSubscription.__mapper__.relationships["notification_subscription_feeds"] + assert rel.passive_deletes is True + assert "delete-orphan" in rel.cascade + + +@pytest.fixture +def api_session(users_test_database_url): + """A real users-DB session plus seeded user/type, always rolled back.""" + _reset_singleton() + db = UsersDatabase() + suffix = uuid.uuid4().hex + user_id = f"feed-user-{suffix}" + session = db.Session() + session.add(AppUser(id=user_id, email=f"{user_id}@test.org")) + # The feed-scoped type may already be seeded in the test DB; only insert if absent. + if session.get(NotificationType, FEED_SCOPED_TYPE) is None: + session.add(NotificationType(id=FEED_SCOPED_TYPE, description="url updated")) + session.flush() + _request_context.set({"user_id": user_id, "user_email": f"{user_id}@test.org", "is_guest": False}) + try: + yield UsersApiImpl(), session, user_id + finally: + session.rollback() + session.close() + _reset_singleton() + _request_context.set({}) + + +def _feed_rows(session, sub_id): + return { + r.feed_stable_id for r in session.query(NotificationSubscriptionFeed).filter_by(subscription_id=sub_id).all() + } + + +def test_create_persists_feed_ids(api_session): + api, session, _ = api_session + + result = api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=FEED_SCOPED_TYPE, feed_ids=["mdb-2", "mdb-1", "mdb-2"]), + db_session=session, + ) + + assert result.notification_id == FEED_SCOPED_TYPE + assert result.feed_ids == ["mdb-1", "mdb-2"] # sorted + deduped + assert _feed_rows(session, result.id) == {"mdb-1", "mdb-2"} + + +def test_create_replaces_feed_set_idempotently(api_session): + api, session, _ = api_session + + first = api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=FEED_SCOPED_TYPE, feed_ids=["mdb-1", "mdb-2"]), + db_session=session, + ) + second = api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=FEED_SCOPED_TYPE, feed_ids=["mdb-3"]), + db_session=session, + ) + + # Same single subscription, feed set replaced. + assert second.id == first.id + assert second.feed_ids == ["mdb-3"] + assert _feed_rows(session, first.id) == {"mdb-3"} + + +def test_delete_subscription_cascades_to_feed_rows(api_session): + api, session, _ = api_session + + sub = api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=FEED_SCOPED_TYPE, feed_ids=["mdb-1", "mdb-2"]), + db_session=session, + ) + assert _feed_rows(session, sub.id) == {"mdb-1", "mdb-2"} + + # Must not raise NotNullViolation; the DB ON DELETE CASCADE removes the feed rows. + session.delete(session.get(NotificationSubscription, sub.id)) + session.flush() + + assert _feed_rows(session, sub.id) == set() + + +def test_create_requires_feed_ids_for_feed_scoped_type(api_session): + api, session, _ = api_session + + with pytest.raises(HTTPException) as exc: + api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=FEED_SCOPED_TYPE), + db_session=session, + ) + assert exc.value.status_code == 400 diff --git a/api/tests/unittest/user_service/test_subscriptions_api_impl.py b/api/tests/unittest/user_service/test_subscriptions_api_impl.py index a860a7df4..c75ed8fc9 100644 --- a/api/tests/unittest/user_service/test_subscriptions_api_impl.py +++ b/api/tests/unittest/user_service/test_subscriptions_api_impl.py @@ -8,6 +8,7 @@ from shared.users_database_gen.sqlacodegen_models import ( AppUser, NotificationSubscription as NotificationSubscriptionOrm, + NotificationSubscriptionFeed as NotificationSubscriptionFeedOrm, ) from user_service.impl.subscriptions_api_impl import SubscriptionsApiImpl @@ -61,6 +62,17 @@ def test_get_does_not_touch_brevo(self): rem.assert_not_called() add.assert_not_called() + def test_returns_feed_ids_for_feed_scoped_subscription(self): + sub = _make_sub(notification_type_id="feed.url_updated") + sub.notification_subscription_feeds.append(NotificationSubscriptionFeedOrm(feed_stable_id="mdb-9")) + sub.notification_subscription_feeds.append(NotificationSubscriptionFeedOrm(feed_stable_id="mdb-2")) + self.mock_session.get.return_value = sub + + result = self.api.get_subscription("sub-1", db_session=self.mock_session) + + # feed_ids come from the join table, sorted. + self.assertEqual(result.feed_ids, ["mdb-2", "mdb-9"]) + def test_missing_returns_404(self): self.mock_session.get.return_value = None with self.assertRaises(HTTPException) as ctx: diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index 04cc0f035..b26c37401 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -241,13 +241,16 @@ def _make_sub(self, **kwargs): def test_get_user_subscriptions_returns_user_subs(self): sub = self._make_sub() query = self.mock_session.query.return_value - query.filter.return_value.order_by.return_value.all.return_value = [sub] + # The list query eager-loads feeds via .options(selectinload(...)). + query.options.return_value.filter.return_value.order_by.return_value.all.return_value = [sub] result = self.api.get_user_subscriptions(db_session=self.mock_session) self.assertEqual(len(result), 1) self.assertEqual(result[0].id, "sub-1") self.assertEqual(result[0].notification_id, "feed.published") + # A non feed-scoped subscription reports no feeds. + self.assertIsNone(result[0].feed_ids) def test_get_user_subscriptions_guest_403(self): _set_context(is_guest=True) @@ -319,6 +322,97 @@ def test_create_idempotent_reactivates_existing(self): self.mock_session.add.assert_not_called() self.assertEqual(result.id, "sub-1") + # ── create: feed-scoped (issue #1778) ── + def test_create_feed_scoped_persists_feed_ids(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.url_updated") if model is NotificationType else _make_user() + ) + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + result = self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.url_updated", feed_ids=["mdb-2", "mdb-1"]), + db_session=self.mock_session, + ) + + self.mock_session.add.assert_called_once() + added_sub = self.mock_session.add.call_args[0][0] + # Both feeds are attached to the new subscription's join collection. + self.assertEqual({f.feed_stable_id for f in added_sub.notification_subscription_feeds}, {"mdb-1", "mdb-2"}) + # Response feed_ids are sorted and deduped. + self.assertEqual(result.feed_ids, ["mdb-1", "mdb-2"]) + + def test_create_feed_scoped_dedupes_feed_ids(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.coverage") if model is NotificationType else _make_user() + ) + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + result = self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.coverage", feed_ids=["mdb-1", "mdb-1"]), + db_session=self.mock_session, + ) + + self.assertEqual(result.feed_ids, ["mdb-1"]) + + def test_create_feed_scoped_requires_feed_ids(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.url_availability") if model is NotificationType else _make_user() + ) + + for req in ( + CreateNotificationSubscriptionRequest(notification_id="feed.url_availability"), + CreateNotificationSubscriptionRequest(notification_id="feed.url_availability", feed_ids=[]), + ): + with self.assertRaises(HTTPException) as ctx: + self.api.create_user_subscription(req, db_session=self.mock_session) + self.assertEqual(ctx.exception.status_code, 400) + self.mock_session.add.assert_not_called() + + def test_create_non_scoped_rejects_feed_ids(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.published") if model is NotificationType else _make_user() + ) + + with self.assertRaises(HTTPException) as ctx: + self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.published", feed_ids=["mdb-1"]), + db_session=self.mock_session, + ) + self.assertEqual(ctx.exception.status_code, 400) + self.mock_session.add.assert_not_called() + + def test_create_feed_scoped_replaces_existing_feeds(self): + from shared.users_database_gen.sqlacodegen_models import ( + NotificationType, + NotificationSubscriptionFeed as FeedOrm, + ) + + existing = self._make_sub(notification_type_id="feed.url_updated", active=False) + existing.notification_subscription_feeds.append(FeedOrm(feed_stable_id="mdb-old")) + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.url_updated") if model is NotificationType else _make_user() + ) + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = existing + + result = self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.url_updated", feed_ids=["mdb-new"]), + db_session=self.mock_session, + ) + + self.mock_session.add.assert_not_called() + self.assertTrue(existing.active) + # The old feed was dropped and the new one attached (delete-orphan handles the DB side). + self.assertEqual({f.feed_stable_id for f in existing.notification_subscription_feeds}, {"mdb-new"}) + self.assertEqual(result.feed_ids, ["mdb-new"]) + # ── update ── def test_update_deactivate_announcement_removes_brevo(self): sub = self._make_sub(notification_type_id="api.announcements", active=True) diff --git a/docs/UserServiceAPI.yaml b/docs/UserServiceAPI.yaml index 3a60e9691..40f680ea0 100644 --- a/docs/UserServiceAPI.yaml +++ b/docs/UserServiceAPI.yaml @@ -390,6 +390,16 @@ components: type: string format: date-time description: Timestamp when the subscription was created. + feed_ids: + type: array + items: + type: string + nullable: true + description: > + Feed stable IDs this subscription targets. Populated for the + feed-scoped notification types (feed.url_updated, + feed.url_availability, feed.coverage); empty for other types. + example: ["mdb-1", "mdb-42"] CreateNotificationSubscriptionRequest: type: object @@ -400,6 +410,16 @@ components: type: string description: The notification type to subscribe to. example: "feed.published" + feed_ids: + type: array + items: + type: string + description: > + Feed stable IDs to subscribe to. Required (non-empty) for the + feed-scoped notification types feed.url_updated, + feed.url_availability and feed.coverage; must be omitted or empty + for other types. Validation is enforced in code, not the schema. + example: ["mdb-1", "mdb-42"] UpdateNotificationSubscriptionRequest: type: object diff --git a/liquibase/changelog_user.xml b/liquibase/changelog_user.xml index 885a4be50..d1cd2e5bd 100644 --- a/liquibase/changelog_user.xml +++ b/liquibase/changelog_user.xml @@ -27,4 +27,8 @@ + + diff --git a/liquibase/changes_user/feat_1778.sql b/liquibase/changes_user/feat_1778.sql new file mode 100644 index 000000000..be80371ca --- /dev/null +++ b/liquibase/changes_user/feat_1778.sql @@ -0,0 +1,26 @@ +-- liquibase formatted sql + +-- changeset MobilityData:feat_1778_subscription_feed +-- comment: Add notification_subscription_feed join table linking a subscription to the feeds it targets (feed.url_updated, feed.url_availability, feed.coverage). Issue #1778. + +-- A notification_subscription can target one-or-more feeds for the feed-scoped +-- notification types. This mirrors the existing notification_event_feed table: +-- feeds are referenced by their public stable_id and there is deliberately NO +-- foreign key to the feeds table, because feeds live in a separate database +-- (see docs/notifications.md). +CREATE TABLE IF NOT EXISTS notification_subscription_feed ( + subscription_id TEXT NOT NULL + REFERENCES notification_subscription(id) ON DELETE CASCADE, + -- Feed referenced by its public stable_id, e.g. 'mdb-1' (matches + -- notification_event_feed.feed_stable_id). No FK: feeds are in another DB. + feed_stable_id TEXT NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + -- One row per (subscription, feed); the PK also serves the + -- "which feeds does this subscription target?" lookup. + PRIMARY KEY (subscription_id, feed_stable_id) +); + +-- Reverse fan-out: "which subscriptions target this feed?" when matching a +-- feed.* event to its subscribers. +CREATE INDEX IF NOT EXISTS idx_notification_subscription_feed_feed_stable_id + ON notification_subscription_feed (feed_stable_id); diff --git a/liquibase/test_data/users_test_data.sql b/liquibase/test_data/users_test_data.sql index ae42158cc..4e7d6d607 100644 --- a/liquibase/test_data/users_test_data.sql +++ b/liquibase/test_data/users_test_data.sql @@ -7,7 +7,8 @@ INSERT INTO notification_type (id, description) VALUES ('feed.published', 'A new feed has been published'), ('feed.updated', 'An existing feed has new data available'), ('feed.deprecated', 'A feed has been marked deprecated'), - ('validation.failed', 'Validation failed for a tracked feed') + ('validation.failed', 'Validation failed for a tracked feed'), + ('feed.url_updated', 'A tracked feed''s URL changed (feed-scoped)') ON CONFLICT (id) DO NOTHING; -- App users ------------------------------------------------------------------ @@ -25,9 +26,17 @@ INSERT INTO notification_subscription (id, user_id, notification_type_id, filter ('sub_0000000000000000000000000002', 'test_user_alice_000000000001', 'feed.updated', '{"feed_ids": ["mdb-1", "mdb-42"]}'::jsonb, true), ('sub_0000000000000000000000000003', 'test_user_bob_00000000000002', 'feed.published', '{"country": "CA"}'::jsonb, true), ('sub_0000000000000000000000000004', 'test_user_bob_00000000000002', 'validation.failed', '{"feed_ids": ["mdb-7"]}'::jsonb, false), - ('sub_0000000000000000000000000005', 'test_user_dan_00000000000004', 'feed.deprecated', NULL, true) + ('sub_0000000000000000000000000005', 'test_user_dan_00000000000004', 'feed.deprecated', NULL, true), + ('sub_0000000000000000000000000006', 'test_user_alice_000000000001', 'feed.url_updated', NULL, true) ON CONFLICT (id) DO NOTHING; +-- Feed-scoped subscription targets (issue #1778) ---------------------------- +-- Alice's feed.url_updated subscription targets two feeds by their stable_id. +INSERT INTO notification_subscription_feed (subscription_id, feed_stable_id) VALUES + ('sub_0000000000000000000000000006', 'mdb-1'), + ('sub_0000000000000000000000000006', 'mdb-42') +ON CONFLICT (subscription_id, feed_stable_id) DO NOTHING; + -- Notification log ---------------------------------------------------------- INSERT INTO notification_log (id, subscription_id, sent_at, channel, status, error_message) VALUES ('log_0000000000000000000000000001', 'sub_0000000000000000000000000001', now() - interval '5 days', 'email', 'sent', NULL), From acd1d6ce16a51fc8256255af37d32f4b940c89c1 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:08:52 -0400 Subject: [PATCH 02/10] implement notifications get --- .../impl/notifications_api_impl.py | 16 +++--- .../test_notifications_api_impl.py | 57 +++++++++++++++++-- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/api/src/user_service/impl/notifications_api_impl.py b/api/src/user_service/impl/notifications_api_impl.py index 4f6722053..76f360916 100644 --- a/api/src/user_service/impl/notifications_api_impl.py +++ b/api/src/user_service/impl/notifications_api_impl.py @@ -15,16 +15,18 @@ # from typing import List -from fastapi import HTTPException - +from shared.database.users_database import with_users_db_session +from shared.db_models.notification_type_impl import NotificationTypeImpl +from shared.users_database_gen.sqlacodegen_models import NotificationType as NotificationTypeOrm from user_service_gen.apis.notifications_api_base import BaseNotificationsApi from user_service_gen.models.notification_type import NotificationType -_NOT_IMPLEMENTED = "Not yet implemented." - class NotificationsApiImpl(BaseNotificationsApi): - """Stub implementation — scheduled for a follow-up issue.""" + """Implementation of the User Service notifications API.""" - def get_notifications(self) -> List[NotificationType]: - raise HTTPException(status_code=501, detail=_NOT_IMPLEMENTED) + @with_users_db_session + def get_notifications(self, db_session=None) -> List[NotificationType]: + """Returns all predefined notification types users can subscribe to, ordered by id.""" + types = db_session.query(NotificationTypeOrm).order_by(NotificationTypeOrm.id).all() + return [NotificationTypeImpl.from_orm(t) for t in types] diff --git a/api/tests/unittest/user_service/test_notifications_api_impl.py b/api/tests/unittest/user_service/test_notifications_api_impl.py index abfd21654..aeeb6c609 100644 --- a/api/tests/unittest/user_service/test_notifications_api_impl.py +++ b/api/tests/unittest/user_service/test_notifications_api_impl.py @@ -1,18 +1,63 @@ import unittest +import uuid +from unittest.mock import MagicMock -from fastapi import HTTPException - +from shared.database.users_database import UsersDatabase +from shared.users_database_gen.sqlacodegen_models import NotificationType as NotificationTypeOrm from user_service.impl.notifications_api_impl import NotificationsApiImpl +from user_service_gen.models.notification_type import NotificationType class TestNotificationsApiImpl(unittest.TestCase): def setUp(self): self.api = NotificationsApiImpl() + self.mock_session = MagicMock() + + def test_get_notifications_returns_mapped_types(self): + self.mock_session.query.return_value.order_by.return_value.all.return_value = [ + NotificationTypeOrm(id="api.announcements", description="Announcements"), + NotificationTypeOrm(id="feed.url_updated", description=None), + ] + + result = self.api.get_notifications(db_session=self.mock_session) + + self.mock_session.query.assert_called_once_with(NotificationTypeOrm) + self.assertTrue(all(isinstance(t, NotificationType) for t in result)) + self.assertEqual([t.id for t in result], ["api.announcements", "feed.url_updated"]) + self.assertEqual(result[0].description, "Announcements") + self.assertIsNone(result[1].description) + + def test_get_notifications_empty(self): + self.mock_session.query.return_value.order_by.return_value.all.return_value = [] + self.assertEqual(self.api.get_notifications(db_session=self.mock_session), []) + + +def _reset_singleton(): + UsersDatabase.instance = None + UsersDatabase.initialized = False + + +def test_get_notifications_db_backed(users_test_database_url): + """End-to-end against the real users test DB: an inserted type appears in the result.""" + _reset_singleton() + db = UsersDatabase() + type_id = f"test.type-{uuid.uuid4().hex}" + session = db.Session() + try: + session.add(NotificationTypeOrm(id=type_id, description="db-backed test type")) + session.flush() + + result = NotificationsApiImpl().get_notifications(db_session=session) - def test_get_notifications_returns_501(self): - with self.assertRaises(HTTPException) as ctx: - self.api.get_notifications() - self.assertEqual(ctx.exception.status_code, 501) + by_id = {t.id: t for t in result} + assert type_id in by_id + assert by_id[type_id].description == "db-backed test type" + # Result is sorted by id. + assert [t.id for t in result] == sorted(t.id for t in result) + finally: + session.rollback() + session.close() + _reset_singleton() if __name__ == "__main__": From ee8ccea7070598b34688598a391999abfa5d2042 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:54:37 -0400 Subject: [PATCH 03/10] implement get notifications --- .../db_models/notification_type_impl.py | 20 ++++ .../user_service/impl/subscription_helpers.py | 4 + api/src/user_service/impl/users_api_impl.py | 26 +++++ .../user_service/test_subscription_feeds.py | 8 ++ .../user_service/test_users_api_impl.py | 97 +++++++++++++++++++ 5 files changed, 155 insertions(+) create mode 100644 api/src/shared/db_models/notification_type_impl.py diff --git a/api/src/shared/db_models/notification_type_impl.py b/api/src/shared/db_models/notification_type_impl.py new file mode 100644 index 000000000..c39348ae5 --- /dev/null +++ b/api/src/shared/db_models/notification_type_impl.py @@ -0,0 +1,20 @@ +from shared.users_database_gen.sqlacodegen_models import NotificationType as NotificationTypeOrm +from user_service_gen.models.notification_type import NotificationType + + +class NotificationTypeImpl(NotificationType): + """Implementation of the NotificationType model. + Converts a SQLAlchemy NotificationType ORM object to a Pydantic NotificationType model. + """ + + class Config: + from_attributes = True + + @classmethod + def from_orm(cls, notification_type: NotificationTypeOrm | None) -> NotificationType | None: + if not notification_type: + return None + return cls( + id=notification_type.id, + description=notification_type.description, + ) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 0e170b9df..380d61099 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -27,6 +27,10 @@ ANNOUNCEMENTS_NOTIFICATION_TYPE_ID = "api.announcements" +# Feature flag (feature_flag.id) that gates notification-subscription management: a user must +# have this boolean flag resolved to true to create (POST) or update (PATCH) subscriptions. +NOTIFICATIONS_FEATURE_FLAG_ID = "isNotificationEnabled" + # Notification types that are scoped to specific feeds. A subscription to any of # these must carry a non-empty list of feed stable IDs (persisted in the # notification_subscription_feed join table); other types must not carry feeds. diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index cc2e1ab40..de504066f 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -38,6 +38,7 @@ from user_service.impl.subscription_helpers import ( ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, FEED_SCOPED_NOTIFICATION_TYPE_IDS, + NOTIFICATIONS_FEATURE_FLAG_ID, sync_announcements, ) from user_service_gen.apis.users_api_base import BaseUsersApi @@ -152,6 +153,7 @@ def create_user_subscription( other notification types. """ user_id = self._require_user_id() + self._require_notifications_enabled(db_session, user_id) notification_id = create_notification_subscription_request.notification_id # De-duplicate while preserving order; treat null as no feeds. feed_ids = list(dict.fromkeys(create_notification_subscription_request.feed_ids or [])) @@ -211,6 +213,7 @@ def update_user_subscription( ) -> NotificationSubscription: """Activates or deactivates a notification subscription by ID.""" user_id = self._require_user_id() + self._require_notifications_enabled(db_session, user_id) sub = self._get_owned_subscription(db_session, id, user_id) active = update_notification_subscription_request.active @@ -255,6 +258,29 @@ def _require_user_id() -> str: raise HTTPException(status_code=403, detail="Guest users cannot perform this action.") return user_id + @classmethod + def _require_notifications_enabled(cls, db_session, user_id: str) -> None: + """Gate: only users with the ``isNotificationEnabled`` feature flag may manage subscriptions. + + Raises 403 unless the flag resolves to true for this user. + """ + if not cls._notifications_enabled(db_session, user_id): + raise HTTPException(status_code=403, detail="Notifications are not enabled for this user.") + + @staticmethod + def _notifications_enabled(db_session, user_id: str) -> bool: + """Resolve the boolean ``isNotificationEnabled`` flag for a user. + + A globally disabled or missing flag denies everyone; otherwise the user's override wins, + falling back to the flag's default value. + """ + flag = db_session.get(FeatureFlag, NOTIFICATIONS_FEATURE_FLAG_ID) + if flag is None or flag.disabled: + return False + override = db_session.get(UserFeatureFlag, (user_id, NOTIFICATIONS_FEATURE_FLAG_ID)) + value = override.value if override is not None and override.value is not None else flag.default_value + return value is True + @staticmethod def _sync_subscription_feeds(sub: NotificationSubscriptionOrm, feed_ids: List[str]) -> None: """Make the subscription's feed set exactly ``feed_ids``. diff --git a/api/tests/unittest/user_service/test_subscription_feeds.py b/api/tests/unittest/user_service/test_subscription_feeds.py index fe1dbf570..d4ad9e0b4 100644 --- a/api/tests/unittest/user_service/test_subscription_feeds.py +++ b/api/tests/unittest/user_service/test_subscription_feeds.py @@ -32,9 +32,11 @@ from shared.database.users_database import UsersDatabase from shared.users_database_gen.sqlacodegen_models import ( AppUser, + FeatureFlag, NotificationSubscription, NotificationSubscriptionFeed, NotificationType, + UserFeatureFlag, ) from user_service.impl.users_api_impl import UsersApiImpl from user_service_gen.models.create_notification_subscription_request import ( @@ -69,6 +71,12 @@ def api_session(users_test_database_url): # The feed-scoped type may already be seeded in the test DB; only insert if absent. if session.get(NotificationType, FEED_SCOPED_TYPE) is None: session.add(NotificationType(id=FEED_SCOPED_TYPE, description="url updated")) + # Subscription management is gated by the isNotificationEnabled flag (default off); enable it + # for this user. The flag row is seeded by migration, but insert-if-absent keeps the test + # self-contained. + if session.get(FeatureFlag, "isNotificationEnabled") is None: + session.add(FeatureFlag(id="isNotificationEnabled", value_type="boolean", default_value=False)) + session.add(UserFeatureFlag(user_id=user_id, feature_flag_id="isNotificationEnabled", value=True)) session.flush() _request_context.set({"user_id": user_id, "user_email": f"{user_id}@test.org", "is_guest": False}) try: diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index b26c37401..ab7e59dc9 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -223,6 +223,10 @@ def setUp(self): self.api = UsersApiImpl() self.mock_session = MagicMock() _set_context() + # These tests exercise create/update logic, not the feature-flag gate; enable it. + gate_patcher = patch.object(UsersApiImpl, "_require_notifications_enabled", return_value=None) + gate_patcher.start() + self.addCleanup(gate_patcher.stop) def _make_sub(self, **kwargs): from shared.users_database_gen.sqlacodegen_models import NotificationSubscription as Orm @@ -510,6 +514,99 @@ def test_brevo_failure_raises_502(self): self.assertEqual(ctx.exception.status_code, 502) +class TestSubscriptionGate(unittest.TestCase): + """The isNotificationEnabled feature flag gates POST/PATCH subscriptions.""" + + def setUp(self): + self.api = UsersApiImpl() + self.mock_session = MagicMock() + _set_context() + + def _configure_gate(self, *, default=False, disabled=False, has_override=False, override_value=None): + from shared.users_database_gen.sqlacodegen_models import ( + FeatureFlag, + NotificationType, + UserFeatureFlag, + ) + + flag = FeatureFlag(id="isNotificationEnabled", value_type="boolean", default_value=default, disabled=disabled) + override = ( + UserFeatureFlag(user_id="uid-123", feature_flag_id="isNotificationEnabled", value=override_value) + if has_override + else None + ) + + def _get(model, key): + if model is FeatureFlag: + return flag + if model is UserFeatureFlag: + return override + if model is NotificationType: + return NotificationType(id="feed.published") + return _make_user() + + self.mock_session.get.side_effect = _get + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + def _create(self): + return self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.published"), db_session=self.mock_session + ) + + def test_create_denied_when_default_false(self): + self._configure_gate(default=False) + with self.assertRaises(HTTPException) as ctx: + self._create() + self.assertEqual(ctx.exception.status_code, 403) + self.mock_session.add.assert_not_called() + + def test_create_allowed_when_default_true(self): + self._configure_gate(default=True) + result = self._create() + self.assertEqual(result.notification_id, "feed.published") + self.mock_session.add.assert_called_once() + + def test_create_allowed_with_user_override_true(self): + self._configure_gate(default=False, has_override=True, override_value=True) + self.assertEqual(self._create().notification_id, "feed.published") + + def test_create_denied_with_user_override_false(self): + self._configure_gate(default=True, has_override=True, override_value=False) + with self.assertRaises(HTTPException) as ctx: + self._create() + self.assertEqual(ctx.exception.status_code, 403) + + def test_create_denied_when_flag_globally_disabled(self): + self._configure_gate(default=True, disabled=True) + with self.assertRaises(HTTPException) as ctx: + self._create() + self.assertEqual(ctx.exception.status_code, 403) + + def test_create_denied_when_flag_missing(self): + from shared.users_database_gen.sqlacodegen_models import FeatureFlag, NotificationType + + def _get(model, key): + if model is FeatureFlag: + return None + if model is NotificationType: + return NotificationType(id="feed.published") + return _make_user() + + self.mock_session.get.side_effect = _get + with self.assertRaises(HTTPException) as ctx: + self._create() + self.assertEqual(ctx.exception.status_code, 403) + + def test_update_denied_when_default_false(self): + self._configure_gate(default=False) + with self.assertRaises(HTTPException) as ctx: + self.api.update_user_subscription( + "sub-1", UpdateNotificationSubscriptionRequest(active=False), db_session=self.mock_session + ) + self.assertEqual(ctx.exception.status_code, 403) + self.mock_session.delete.assert_not_called() + + if __name__ == "__main__": unittest.main() From 24288aee4f9d18860772097057fa31dad40244a0 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:19:49 -0400 Subject: [PATCH 04/10] gate admin summary subscription under feature flag --- .../user_service/impl/subscription_helpers.py | 6 ++ api/src/user_service/impl/users_api_impl.py | 33 +++++-- .../user_service/test_users_api_impl.py | 88 +++++++++++++++++++ 3 files changed, 122 insertions(+), 5 deletions(-) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 380d61099..221b250fc 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -31,6 +31,12 @@ # have this boolean flag resolved to true to create (POST) or update (PATCH) subscriptions. NOTIFICATIONS_FEATURE_FLAG_ID = "isNotificationEnabled" +# The admin dispatch-summary notification type and the additional feature flag required to +# subscribe to (or manage a subscription for) it. This is layered on top of the general +# NOTIFICATIONS_FEATURE_FLAG_ID gate. +ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID = "admin.event_summary" +ADMIN_SUMMARY_FEATURE_FLAG_ID = "isAdminSummarySubscriptionEnabled" + # Notification types that are scoped to specific feeds. A subscription to any of # these must carry a non-empty list of feed stable IDs (persisted in the # notification_subscription_feed join table); other types must not carry feeds. diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index de504066f..5819f703e 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -36,6 +36,8 @@ NotificationType, ) from user_service.impl.subscription_helpers import ( + ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID, + ADMIN_SUMMARY_FEATURE_FLAG_ID, ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, FEED_SCOPED_NOTIFICATION_TYPE_IDS, NOTIFICATIONS_FEATURE_FLAG_ID, @@ -161,6 +163,10 @@ def create_user_subscription( if db_session.get(NotificationType, notification_id) is None: raise HTTPException(status_code=400, detail=f"Unknown notification type '{notification_id}'.") + # The admin dispatch-summary type is gated by an additional feature flag. + if notification_id == ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID: + self._require_admin_summary_enabled(db_session, user_id) + # Feed scoping is validated in code (the OpenAPI 3.0 schema keeps feed_ids optional). is_feed_scoped = notification_id in FEED_SCOPED_NOTIFICATION_TYPE_IDS if is_feed_scoped and not feed_ids: @@ -216,6 +222,10 @@ def update_user_subscription( self._require_notifications_enabled(db_session, user_id) sub = self._get_owned_subscription(db_session, id, user_id) + # Managing an admin.event_summary subscription requires the additional admin flag. + if sub.notification_type_id == ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID: + self._require_admin_summary_enabled(db_session, user_id) + active = update_notification_subscription_request.active if sub.notification_type_id == ANNOUNCEMENTS_NOTIFICATION_TYPE_ID: user = db_session.get(AppUser, user_id) @@ -264,20 +274,33 @@ def _require_notifications_enabled(cls, db_session, user_id: str) -> None: Raises 403 unless the flag resolves to true for this user. """ - if not cls._notifications_enabled(db_session, user_id): + if not cls._feature_flag_enabled(db_session, user_id, NOTIFICATIONS_FEATURE_FLAG_ID): raise HTTPException(status_code=403, detail="Notifications are not enabled for this user.") + @classmethod + def _require_admin_summary_enabled(cls, db_session, user_id: str) -> None: + """Gate: the ``admin.event_summary`` type additionally requires ``isAdminSummarySubscriptionEnabled``. + + Layered on top of the general notifications gate. Raises 403 unless the flag resolves to + true for this user. + """ + if not cls._feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID): + raise HTTPException( + status_code=403, + detail=f"The '{ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID}' subscription is not enabled for this user.", + ) + @staticmethod - def _notifications_enabled(db_session, user_id: str) -> bool: - """Resolve the boolean ``isNotificationEnabled`` flag for a user. + def _feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: + """Resolve a boolean feature flag for a user. A globally disabled or missing flag denies everyone; otherwise the user's override wins, falling back to the flag's default value. """ - flag = db_session.get(FeatureFlag, NOTIFICATIONS_FEATURE_FLAG_ID) + flag = db_session.get(FeatureFlag, flag_id) if flag is None or flag.disabled: return False - override = db_session.get(UserFeatureFlag, (user_id, NOTIFICATIONS_FEATURE_FLAG_ID)) + override = db_session.get(UserFeatureFlag, (user_id, flag_id)) value = override.value if override is not None and override.value is not None else flag.default_value return value is True diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index ab7e59dc9..4c556d74b 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -607,6 +607,94 @@ def test_update_denied_when_default_false(self): self.mock_session.delete.assert_not_called() +class TestAdminSummaryGate(unittest.TestCase): + """admin.event_summary additionally requires the isAdminSummarySubscriptionEnabled flag.""" + + ADMIN_TYPE = "admin.event_summary" + + def setUp(self): + self.api = UsersApiImpl() + self.mock_session = MagicMock() + _set_context() + + def _configure(self, *, notifications=True, admin=True, sub=None): + """Wire get() to resolve both gate flags (enabled per args) and, optionally, a subscription.""" + from shared.users_database_gen.sqlacodegen_models import ( + FeatureFlag, + NotificationSubscription as Orm, + NotificationType, + UserFeatureFlag, + ) + + flags = { + "isNotificationEnabled": FeatureFlag( + id="isNotificationEnabled", value_type="boolean", default_value=notifications, disabled=False + ), + "isAdminSummarySubscriptionEnabled": FeatureFlag( + id="isAdminSummarySubscriptionEnabled", value_type="boolean", default_value=admin, disabled=False + ), + } + + def _get(model, key): + if model is FeatureFlag: + return flags.get(key) + if model is UserFeatureFlag: + return None + if model is NotificationType: + return NotificationType(id=key) + if model is Orm: + return sub + return _make_user() + + self.mock_session.get.side_effect = _get + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + def _create(self, notification_id): + return self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id=notification_id), db_session=self.mock_session + ) + + def test_create_admin_allowed_when_admin_flag_true(self): + self._configure(notifications=True, admin=True) + self.assertEqual(self._create(self.ADMIN_TYPE).notification_id, self.ADMIN_TYPE) + + def test_create_admin_denied_when_admin_flag_false(self): + self._configure(notifications=True, admin=False) + with self.assertRaises(HTTPException) as ctx: + self._create(self.ADMIN_TYPE) + self.assertEqual(ctx.exception.status_code, 403) + self.mock_session.add.assert_not_called() + + def test_create_admin_denied_when_notifications_flag_false(self): + # The general gate runs first, so it fails even with the admin flag on. + self._configure(notifications=False, admin=True) + with self.assertRaises(HTTPException) as ctx: + self._create(self.ADMIN_TYPE) + self.assertEqual(ctx.exception.status_code, 403) + + def test_create_non_admin_type_ignores_admin_flag(self): + # A non-admin type is unaffected by the admin flag being off. + self._configure(notifications=True, admin=False) + self.assertEqual(self._create("feed.published").notification_id, "feed.published") + + def test_update_admin_denied_when_admin_flag_false(self): + from shared.users_database_gen.sqlacodegen_models import NotificationSubscription as Orm + + sub = Orm( + id="sub-1", + user_id="uid-123", + notification_type_id=self.ADMIN_TYPE, + active=True, + created_at=FIXED_NOW, + ) + self._configure(notifications=True, admin=False, sub=sub) + with self.assertRaises(HTTPException) as ctx: + self.api.update_user_subscription( + "sub-1", UpdateNotificationSubscriptionRequest(active=False), db_session=self.mock_session + ) + self.assertEqual(ctx.exception.status_code, 403) + + if __name__ == "__main__": unittest.main() From 6d87d17815eaa5ff2f86a9fa118f3f038d56ebc7 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:00:30 -0400 Subject: [PATCH 05/10] add admin summary flag to get notifications --- .../impl/notifications_api_impl.py | 17 +++- .../user_service/impl/subscription_helpers.py | 15 +++ api/src/user_service/impl/users_api_impl.py | 19 +--- .../test_notifications_api_impl.py | 99 ++++++++++++++++--- 4 files changed, 122 insertions(+), 28 deletions(-) diff --git a/api/src/user_service/impl/notifications_api_impl.py b/api/src/user_service/impl/notifications_api_impl.py index 76f360916..4491e9b30 100644 --- a/api/src/user_service/impl/notifications_api_impl.py +++ b/api/src/user_service/impl/notifications_api_impl.py @@ -15,9 +15,15 @@ # from typing import List +from middleware.request_context import get_request_context from shared.database.users_database import with_users_db_session from shared.db_models.notification_type_impl import NotificationTypeImpl from shared.users_database_gen.sqlacodegen_models import NotificationType as NotificationTypeOrm +from user_service.impl.subscription_helpers import ( + ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID, + ADMIN_SUMMARY_FEATURE_FLAG_ID, + feature_flag_enabled, +) from user_service_gen.apis.notifications_api_base import BaseNotificationsApi from user_service_gen.models.notification_type import NotificationType @@ -27,6 +33,15 @@ class NotificationsApiImpl(BaseNotificationsApi): @with_users_db_session def get_notifications(self, db_session=None) -> List[NotificationType]: - """Returns all predefined notification types users can subscribe to, ordered by id.""" + """Returns the notification types the caller can subscribe to, ordered by id. + + ``admin.event_summary`` is only included for users with the + ``isAdminSummarySubscriptionEnabled`` flag, mirroring the create/update gate. + """ types = db_session.query(NotificationTypeOrm).order_by(NotificationTypeOrm.id).all() + + user_id = get_request_context().get("user_id") + if not (user_id and feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID)): + types = [t for t in types if t.id != ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID] + return [NotificationTypeImpl.from_orm(t) for t in types] diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 221b250fc..3c69c622e 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -22,6 +22,7 @@ import sib_api_v3_sdk import urllib3 from shared.common.brevo import add_contact_to_list, get_announcements_list_id, remove_contact_from_list +from shared.users_database_gen.sqlacodegen_models import FeatureFlag, UserFeatureFlag logger = logging.getLogger(__name__) @@ -49,6 +50,20 @@ ) +def feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: + """Resolve a boolean feature flag for a user. + + A globally disabled or missing flag denies everyone; otherwise the user's override wins, + falling back to the flag's default value. + """ + flag = db_session.get(FeatureFlag, flag_id) + if flag is None or flag.disabled: + return False + override = db_session.get(UserFeatureFlag, (user_id, flag_id)) + value = override.value if override is not None and override.value is not None else flag.default_value + return value is True + + def sync_announcements(email: str, subscribe: bool, subscription_id: str | None = None) -> None: """Sync an api.announcements subscription with Brevo, mapping provider errors to 502.""" try: diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index 5819f703e..e5823dc22 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -41,6 +41,7 @@ ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, FEED_SCOPED_NOTIFICATION_TYPE_IDS, NOTIFICATIONS_FEATURE_FLAG_ID, + feature_flag_enabled, sync_announcements, ) from user_service_gen.apis.users_api_base import BaseUsersApi @@ -274,7 +275,7 @@ def _require_notifications_enabled(cls, db_session, user_id: str) -> None: Raises 403 unless the flag resolves to true for this user. """ - if not cls._feature_flag_enabled(db_session, user_id, NOTIFICATIONS_FEATURE_FLAG_ID): + if not feature_flag_enabled(db_session, user_id, NOTIFICATIONS_FEATURE_FLAG_ID): raise HTTPException(status_code=403, detail="Notifications are not enabled for this user.") @classmethod @@ -284,26 +285,12 @@ def _require_admin_summary_enabled(cls, db_session, user_id: str) -> None: Layered on top of the general notifications gate. Raises 403 unless the flag resolves to true for this user. """ - if not cls._feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID): + if not feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID): raise HTTPException( status_code=403, detail=f"The '{ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID}' subscription is not enabled for this user.", ) - @staticmethod - def _feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: - """Resolve a boolean feature flag for a user. - - A globally disabled or missing flag denies everyone; otherwise the user's override wins, - falling back to the flag's default value. - """ - flag = db_session.get(FeatureFlag, flag_id) - if flag is None or flag.disabled: - return False - override = db_session.get(UserFeatureFlag, (user_id, flag_id)) - value = override.value if override is not None and override.value is not None else flag.default_value - return value is True - @staticmethod def _sync_subscription_feeds(sub: NotificationSubscriptionOrm, feed_ids: List[str]) -> None: """Make the subscription's feed set exactly ``feed_ids``. diff --git a/api/tests/unittest/user_service/test_notifications_api_impl.py b/api/tests/unittest/user_service/test_notifications_api_impl.py index aeeb6c609..aa4cb5b6f 100644 --- a/api/tests/unittest/user_service/test_notifications_api_impl.py +++ b/api/tests/unittest/user_service/test_notifications_api_impl.py @@ -2,22 +2,54 @@ import uuid from unittest.mock import MagicMock +from middleware.request_context import _request_context from shared.database.users_database import UsersDatabase -from shared.users_database_gen.sqlacodegen_models import NotificationType as NotificationTypeOrm +from shared.users_database_gen.sqlacodegen_models import ( + FeatureFlag, + NotificationType as NotificationTypeOrm, + UserFeatureFlag, +) from user_service.impl.notifications_api_impl import NotificationsApiImpl from user_service_gen.models.notification_type import NotificationType +ADMIN_TYPE = "admin.event_summary" -class TestNotificationsApiImpl(unittest.TestCase): + +def _set_context(user_id="uid-123"): + _request_context.set({"user_id": user_id, "user_email": "u@e.com", "is_guest": False}) + + +class TestGetNotifications(unittest.TestCase): def setUp(self): self.api = NotificationsApiImpl() self.mock_session = MagicMock() + _set_context() + + def _set_types(self, types): + self.mock_session.query.return_value.order_by.return_value.all.return_value = types + + def _set_admin_flag(self, enabled): + flag = FeatureFlag( + id="isAdminSummarySubscriptionEnabled", value_type="boolean", default_value=enabled, disabled=False + ) - def test_get_notifications_returns_mapped_types(self): - self.mock_session.query.return_value.order_by.return_value.all.return_value = [ - NotificationTypeOrm(id="api.announcements", description="Announcements"), - NotificationTypeOrm(id="feed.url_updated", description=None), - ] + def _get(model, key): + if model is FeatureFlag: + return flag + if model is UserFeatureFlag: + return None + return None + + self.mock_session.get.side_effect = _get + + def test_returns_mapped_types(self): + self._set_types( + [ + NotificationTypeOrm(id="api.announcements", description="Announcements"), + NotificationTypeOrm(id="feed.url_updated", description=None), + ] + ) + self._set_admin_flag(False) result = self.api.get_notifications(db_session=self.mock_session) @@ -27,8 +59,50 @@ def test_get_notifications_returns_mapped_types(self): self.assertEqual(result[0].description, "Announcements") self.assertIsNone(result[1].description) - def test_get_notifications_empty(self): - self.mock_session.query.return_value.order_by.return_value.all.return_value = [] + def test_admin_type_hidden_when_flag_off(self): + self._set_types( + [ + NotificationTypeOrm(id="api.announcements", description=None), + NotificationTypeOrm(id=ADMIN_TYPE, description="admin"), + ] + ) + self._set_admin_flag(False) + + result = self.api.get_notifications(db_session=self.mock_session) + + self.assertEqual([t.id for t in result], ["api.announcements"]) + + def test_admin_type_visible_when_flag_on(self): + self._set_types( + [ + NotificationTypeOrm(id="api.announcements", description=None), + NotificationTypeOrm(id=ADMIN_TYPE, description="admin"), + ] + ) + self._set_admin_flag(True) + + result = self.api.get_notifications(db_session=self.mock_session) + + self.assertEqual([t.id for t in result], ["api.announcements", ADMIN_TYPE]) + + def test_admin_type_hidden_for_anonymous_without_flag_lookup(self): + _request_context.set({}) # no user_id + self._set_types( + [ + NotificationTypeOrm(id="api.announcements", description=None), + NotificationTypeOrm(id=ADMIN_TYPE, description="admin"), + ] + ) + + result = self.api.get_notifications(db_session=self.mock_session) + + self.assertEqual([t.id for t in result], ["api.announcements"]) + # Without a user there is no flag to resolve. + self.mock_session.get.assert_not_called() + + def test_empty(self): + self._set_types([]) + self._set_admin_flag(False) self.assertEqual(self.api.get_notifications(db_session=self.mock_session), []) @@ -38,7 +112,7 @@ def _reset_singleton(): def test_get_notifications_db_backed(users_test_database_url): - """End-to-end against the real users test DB: an inserted type appears in the result.""" + """End-to-end against the real users test DB: an inserted type appears; admin type is hidden.""" _reset_singleton() db = UsersDatabase() type_id = f"test.type-{uuid.uuid4().hex}" @@ -46,18 +120,21 @@ def test_get_notifications_db_backed(users_test_database_url): try: session.add(NotificationTypeOrm(id=type_id, description="db-backed test type")) session.flush() + _request_context.set({}) # anonymous → admin.event_summary filtered out result = NotificationsApiImpl().get_notifications(db_session=session) by_id = {t.id: t for t in result} assert type_id in by_id assert by_id[type_id].description == "db-backed test type" - # Result is sorted by id. + # admin.event_summary is seeded (feat_1723) but hidden without the admin flag. + assert ADMIN_TYPE not in by_id assert [t.id for t in result] == sorted(t.id for t in result) finally: session.rollback() session.close() _reset_singleton() + _request_context.set({}) if __name__ == "__main__": From ca8653202ac81f9cc9f5bd2578f2f5ec5eed4ed1 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:30:59 -0400 Subject: [PATCH 06/10] Fix gated endpoints add documentation --- .../user_service/impl/subscription_helpers.py | 7 +++++-- .../user_service/impl/subscriptions_api_impl.py | 9 +++++++++ api/src/user_service/impl/users_api_impl.py | 8 +++++--- .../user_service/test_event_loop_not_blocked.py | 8 +++++--- .../user_service/test_subscription_feeds.py | 8 ++++---- .../user_service/test_subscriptions_api_impl.py | 15 +++++++++++++++ .../user_service/test_users_api_impl.py | 17 ++++++++++++----- docs/UserServiceAPI.yaml | 10 +++++----- 8 files changed, 60 insertions(+), 22 deletions(-) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 3c69c622e..45845c527 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -26,11 +26,14 @@ logger = logging.getLogger(__name__) +# Shared 403 detail returned when a user lacks the feature flag gating a subscription action. +ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED = "This feature is not enabled for the user." + ANNOUNCEMENTS_NOTIFICATION_TYPE_ID = "api.announcements" # Feature flag (feature_flag.id) that gates notification-subscription management: a user must -# have this boolean flag resolved to true to create (POST) or update (PATCH) subscriptions. -NOTIFICATIONS_FEATURE_FLAG_ID = "isNotificationEnabled" +# have this boolean flag resolved to true to create, update or delete subscriptions. +NOTIFICATIONS_FEATURE_FLAG_ID = "isNotificationsEnabled" # The admin dispatch-summary notification type and the additional feature flag required to # subscribe to (or manage a subscription for) it. This is layered on top of the general diff --git a/api/src/user_service/impl/subscriptions_api_impl.py b/api/src/user_service/impl/subscriptions_api_impl.py index 355a0ec95..611a887ba 100644 --- a/api/src/user_service/impl/subscriptions_api_impl.py +++ b/api/src/user_service/impl/subscriptions_api_impl.py @@ -24,6 +24,9 @@ ) from user_service.impl.subscription_helpers import ( ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, + ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED, + NOTIFICATIONS_FEATURE_FLAG_ID, + feature_flag_enabled, sync_announcements, ) from user_service_gen.apis.subscriptions_api_base import BaseSubscriptionsApi @@ -53,6 +56,12 @@ def delete_subscription(self, id: str, db_session=None) -> None: if sub is None: raise HTTPException(status_code=404, detail="Subscription not found.") + # Gated by the subscription owner's isNotificationsEnabled flag (this endpoint is + # unauthenticated; the subscription UUID is the capability, so we resolve the flag for + # the subscription's owner rather than a request user). + if not feature_flag_enabled(db_session, sub.user_id, NOTIFICATIONS_FEATURE_FLAG_ID): + raise HTTPException(status_code=403, detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED) + if sub.notification_type_id == ANNOUNCEMENTS_NOTIFICATION_TYPE_ID: user = db_session.get(AppUser, sub.user_id) email = user.email if user is not None else None diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index e5823dc22..35ed7f266 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -39,6 +39,7 @@ ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID, ADMIN_SUMMARY_FEATURE_FLAG_ID, ANNOUNCEMENTS_NOTIFICATION_TYPE_ID, + ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED, FEED_SCOPED_NOTIFICATION_TYPE_IDS, NOTIFICATIONS_FEATURE_FLAG_ID, feature_flag_enabled, @@ -243,6 +244,7 @@ def delete_user_subscription(self, id: str, db_session=None) -> None: The announcements subscription cannot be deleted; it is disabled instead. """ user_id = self._require_user_id() + self._require_notifications_enabled(db_session, user_id) sub = self._get_owned_subscription(db_session, id, user_id) if sub.notification_type_id == ANNOUNCEMENTS_NOTIFICATION_TYPE_ID: @@ -271,12 +273,12 @@ def _require_user_id() -> str: @classmethod def _require_notifications_enabled(cls, db_session, user_id: str) -> None: - """Gate: only users with the ``isNotificationEnabled`` feature flag may manage subscriptions. + """Gate: only users with the ``isNotificationsEnabled`` feature flag may manage subscriptions. Raises 403 unless the flag resolves to true for this user. """ if not feature_flag_enabled(db_session, user_id, NOTIFICATIONS_FEATURE_FLAG_ID): - raise HTTPException(status_code=403, detail="Notifications are not enabled for this user.") + raise HTTPException(status_code=403, detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED) @classmethod def _require_admin_summary_enabled(cls, db_session, user_id: str) -> None: @@ -288,7 +290,7 @@ def _require_admin_summary_enabled(cls, db_session, user_id: str) -> None: if not feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID): raise HTTPException( status_code=403, - detail=f"The '{ADMIN_EVENT_SUMMARY_NOTIFICATION_TYPE_ID}' subscription is not enabled for this user.", + detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED, ) @staticmethod diff --git a/api/tests/unittest/user_service/test_event_loop_not_blocked.py b/api/tests/unittest/user_service/test_event_loop_not_blocked.py index db4f3e4f0..7d82bcdcc 100644 --- a/api/tests/unittest/user_service/test_event_loop_not_blocked.py +++ b/api/tests/unittest/user_service/test_event_loop_not_blocked.py @@ -92,9 +92,11 @@ def _slow_remove(*_args, **_kwargs): app.dependency_overrides[get_token_Authentication] = lambda: "test-token" try: - with patch("shared.database.users_database.UsersDatabase", return_value=fake_db), patch.object( - helpers, "remove_contact_from_list", side_effect=_slow_remove - ), patch.object(helpers, "get_announcements_list_id", return_value=1): + with patch("shared.database.users_database.UsersDatabase", return_value=fake_db), patch( + "user_service.impl.subscriptions_api_impl.feature_flag_enabled", return_value=True + ), patch.object(helpers, "remove_contact_from_list", side_effect=_slow_remove), patch.object( + helpers, "get_announcements_list_id", return_value=1 + ): response, ticks_during = asyncio.run(_delete_with_heartbeat(app)) finally: app.dependency_overrides.pop(get_token_Authentication, None) diff --git a/api/tests/unittest/user_service/test_subscription_feeds.py b/api/tests/unittest/user_service/test_subscription_feeds.py index d4ad9e0b4..bb41a9fb8 100644 --- a/api/tests/unittest/user_service/test_subscription_feeds.py +++ b/api/tests/unittest/user_service/test_subscription_feeds.py @@ -71,12 +71,12 @@ def api_session(users_test_database_url): # The feed-scoped type may already be seeded in the test DB; only insert if absent. if session.get(NotificationType, FEED_SCOPED_TYPE) is None: session.add(NotificationType(id=FEED_SCOPED_TYPE, description="url updated")) - # Subscription management is gated by the isNotificationEnabled flag (default off); enable it + # Subscription management is gated by the isNotificationsEnabled flag (default off); enable it # for this user. The flag row is seeded by migration, but insert-if-absent keeps the test # self-contained. - if session.get(FeatureFlag, "isNotificationEnabled") is None: - session.add(FeatureFlag(id="isNotificationEnabled", value_type="boolean", default_value=False)) - session.add(UserFeatureFlag(user_id=user_id, feature_flag_id="isNotificationEnabled", value=True)) + if session.get(FeatureFlag, "isNotificationsEnabled") is None: + session.add(FeatureFlag(id="isNotificationsEnabled", value_type="boolean", default_value=False)) + session.add(UserFeatureFlag(user_id=user_id, feature_flag_id="isNotificationsEnabled", value=True)) session.flush() _request_context.set({"user_id": user_id, "user_email": f"{user_id}@test.org", "is_guest": False}) try: diff --git a/api/tests/unittest/user_service/test_subscriptions_api_impl.py b/api/tests/unittest/user_service/test_subscriptions_api_impl.py index c75ed8fc9..e5ccb7023 100644 --- a/api/tests/unittest/user_service/test_subscriptions_api_impl.py +++ b/api/tests/unittest/user_service/test_subscriptions_api_impl.py @@ -84,6 +84,21 @@ class TestPublicDeleteSubscription(unittest.TestCase): def setUp(self): self.api = SubscriptionsApiImpl() self.mock_session = MagicMock() + # Delete is gated by the owner's isNotificationsEnabled flag; enable it for these tests. + gate_patcher = patch("user_service.impl.subscriptions_api_impl.feature_flag_enabled", return_value=True) + gate_patcher.start() + self.addCleanup(gate_patcher.stop) + + def test_delete_denied_when_flag_off(self): + sub = _make_sub(notification_type_id="feed.published") + self.mock_session.get.return_value = sub + + with patch("user_service.impl.subscriptions_api_impl.feature_flag_enabled", return_value=False): + with self.assertRaises(HTTPException) as ctx: + self.api.delete_subscription("sub-1", db_session=self.mock_session) + + self.assertEqual(ctx.exception.status_code, 403) + self.mock_session.delete.assert_not_called() def test_delete_non_announcement_no_brevo(self): sub = _make_sub(notification_type_id="feed.published") diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index 4c556d74b..6b6903961 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -515,7 +515,7 @@ def test_brevo_failure_raises_502(self): class TestSubscriptionGate(unittest.TestCase): - """The isNotificationEnabled feature flag gates POST/PATCH subscriptions.""" + """The isNotificationsEnabled feature flag gates POST/PATCH subscriptions.""" def setUp(self): self.api = UsersApiImpl() @@ -529,9 +529,9 @@ def _configure_gate(self, *, default=False, disabled=False, has_override=False, UserFeatureFlag, ) - flag = FeatureFlag(id="isNotificationEnabled", value_type="boolean", default_value=default, disabled=disabled) + flag = FeatureFlag(id="isNotificationsEnabled", value_type="boolean", default_value=default, disabled=disabled) override = ( - UserFeatureFlag(user_id="uid-123", feature_flag_id="isNotificationEnabled", value=override_value) + UserFeatureFlag(user_id="uid-123", feature_flag_id="isNotificationsEnabled", value=override_value) if has_override else None ) @@ -606,6 +606,13 @@ def test_update_denied_when_default_false(self): self.assertEqual(ctx.exception.status_code, 403) self.mock_session.delete.assert_not_called() + def test_delete_denied_when_default_false(self): + self._configure_gate(default=False) + with self.assertRaises(HTTPException) as ctx: + self.api.delete_user_subscription("sub-1", db_session=self.mock_session) + self.assertEqual(ctx.exception.status_code, 403) + self.mock_session.delete.assert_not_called() + class TestAdminSummaryGate(unittest.TestCase): """admin.event_summary additionally requires the isAdminSummarySubscriptionEnabled flag.""" @@ -627,8 +634,8 @@ def _configure(self, *, notifications=True, admin=True, sub=None): ) flags = { - "isNotificationEnabled": FeatureFlag( - id="isNotificationEnabled", value_type="boolean", default_value=notifications, disabled=False + "isNotificationsEnabled": FeatureFlag( + id="isNotificationsEnabled", value_type="boolean", default_value=notifications, disabled=False ), "isAdminSummarySubscriptionEnabled": FeatureFlag( id="isAdminSummarySubscriptionEnabled", value_type="boolean", default_value=admin, disabled=False diff --git a/docs/UserServiceAPI.yaml b/docs/UserServiceAPI.yaml index 40f680ea0..c71eff327 100644 --- a/docs/UserServiceAPI.yaml +++ b/docs/UserServiceAPI.yaml @@ -117,7 +117,7 @@ paths: /v1/user/subscriptions: get: summary: List the current user's notification subscriptions - description: Returns all notification subscriptions for the authenticated user. + description: Returns all notification subscriptions for the authenticated user. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. operationId: getUserSubscriptions tags: - "users" @@ -138,7 +138,7 @@ paths: description: Not yet implemented. post: summary: Create a notification subscription - description: Subscribes the authenticated user to a notification type. + description: Subscribes the authenticated user to a notification type. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. operationId: createUserSubscription tags: - "users" @@ -167,7 +167,7 @@ paths: /v1/user/subscriptions/{id}: patch: summary: Toggle a notification subscription - description: Activates or deactivates a notification subscription by ID. + description: Activates or deactivates a notification subscription by ID. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. operationId: updateUserSubscription tags: - "users" @@ -204,7 +204,7 @@ paths: description: >- Removes a notification subscription by ID. The announcements subscription (`api.announcements`) cannot be deleted; calling this endpoint for it - disables the subscription (sets it inactive) instead of removing it. + disables the subscription (sets it inactive) instead of removing it. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. operationId: deleteUserSubscription tags: - "users" @@ -257,7 +257,7 @@ paths: Removes a notification subscription identified by its ID. The announcements subscription (`api.announcements`) cannot be deleted; calling this endpoint for it disables the subscription (sets it inactive) - instead of removing it. + instead of removing it. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. operationId: deleteSubscription tags: - "subscriptions" From da04a48f00086dbea2598ac8295ba69a24f1f1a2 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:18:46 -0400 Subject: [PATCH 07/10] Allow disabled feature flags to be subscribed by users allowed users --- .../user_service/impl/subscription_helpers.py | 22 +++++++++++++++---- .../impl/subscriptions_api_impl.py | 10 +++++++++ api/src/user_service/impl/users_api_impl.py | 10 +++++++++ .../user_service/test_users_api_impl.py | 11 +++++----- 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 45845c527..133c74abf 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -56,15 +56,29 @@ def feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: """Resolve a boolean feature flag for a user. - A globally disabled or missing flag denies everyone; otherwise the user's override wins, - falling back to the flag's default value. + The user's override wins, falling back to the flag's default value; a missing flag denies + (nothing to resolve). The ``disabled`` column is deliberately NOT consulted here: it only + controls consumer-facing *exposure* (whether the flag is surfaced in the user profile's + ``features`` list) — it is a backend concern and must not block a user who has been granted + access from subscribing. Otherwise a disabled-but-granted flag would leave no way to subscribe. """ flag = db_session.get(FeatureFlag, flag_id) - if flag is None or flag.disabled: + if flag is None: + logger.debug("feature flag %r not found; denying user %s", flag_id, user_id) return False override = db_session.get(UserFeatureFlag, (user_id, flag_id)) value = override.value if override is not None and override.value is not None else flag.default_value - return value is True + enabled = value is True + logger.debug( + "feature flag %r for user %s: override=%r default=%r resolved=%r enabled=%s", + flag_id, + user_id, + getattr(override, "value", None), + flag.default_value, + value, + enabled, + ) + return enabled def sync_announcements(email: str, subscribe: bool, subscription_id: str | None = None) -> None: diff --git a/api/src/user_service/impl/subscriptions_api_impl.py b/api/src/user_service/impl/subscriptions_api_impl.py index 611a887ba..47737af06 100644 --- a/api/src/user_service/impl/subscriptions_api_impl.py +++ b/api/src/user_service/impl/subscriptions_api_impl.py @@ -14,6 +14,8 @@ # limitations under the License. # +import logging + from fastapi import HTTPException from shared.database.users_database import with_users_db_session @@ -32,6 +34,8 @@ from user_service_gen.apis.subscriptions_api_base import BaseSubscriptionsApi from user_service_gen.models.notification_subscription import NotificationSubscription +logger = logging.getLogger(__name__) + class SubscriptionsApiImpl(BaseSubscriptionsApi): """Public, unauthenticated subscription management. @@ -60,6 +64,12 @@ def delete_subscription(self, id: str, db_session=None) -> None: # unauthenticated; the subscription UUID is the capability, so we resolve the flag for # the subscription's owner rather than a request user). if not feature_flag_enabled(db_session, sub.user_id, NOTIFICATIONS_FEATURE_FLAG_ID): + logger.info( + "Public delete denied for subscription %s (owner %s): feature flag %r not enabled.", + id, + sub.user_id, + NOTIFICATIONS_FEATURE_FLAG_ID, + ) raise HTTPException(status_code=403, detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED) if sub.notification_type_id == ANNOUNCEMENTS_NOTIFICATION_TYPE_ID: diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index 35ed7f266..8d609b31d 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -278,6 +278,11 @@ def _require_notifications_enabled(cls, db_session, user_id: str) -> None: Raises 403 unless the flag resolves to true for this user. """ if not feature_flag_enabled(db_session, user_id, NOTIFICATIONS_FEATURE_FLAG_ID): + logger.info( + "Subscription action denied for user %s: feature flag %r not enabled.", + user_id, + NOTIFICATIONS_FEATURE_FLAG_ID, + ) raise HTTPException(status_code=403, detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED) @classmethod @@ -288,6 +293,11 @@ def _require_admin_summary_enabled(cls, db_session, user_id: str) -> None: true for this user. """ if not feature_flag_enabled(db_session, user_id, ADMIN_SUMMARY_FEATURE_FLAG_ID): + logger.info( + "Subscription action denied for user %s: feature flag %r not enabled.", + user_id, + ADMIN_SUMMARY_FEATURE_FLAG_ID, + ) raise HTTPException( status_code=403, detail=ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED, diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index 6b6903961..2a38cfc15 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -576,11 +576,12 @@ def test_create_denied_with_user_override_false(self): self._create() self.assertEqual(ctx.exception.status_code, 403) - def test_create_denied_when_flag_globally_disabled(self): - self._configure_gate(default=True, disabled=True) - with self.assertRaises(HTTPException) as ctx: - self._create() - self.assertEqual(ctx.exception.status_code, 403) + def test_create_allowed_when_flag_disabled_but_granted(self): + # `disabled` only hides the flag from the consumer profile; it must NOT block a granted + # user from subscribing (backend-only concern). + self._configure_gate(default=False, disabled=True, has_override=True, override_value=True) + self.assertEqual(self._create().notification_id, "feed.published") + self.mock_session.add.assert_called_once() def test_create_denied_when_flag_missing(self): from shared.users_database_gen.sqlacodegen_models import FeatureFlag, NotificationType From 6b564841c974cdf42c6c1e085c10b3a29fb432a8 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:51:50 -0400 Subject: [PATCH 08/10] Add feed id validation and unit tests --- .../user_service/impl/subscription_helpers.py | 23 ++++++++++++ api/src/user_service/impl/users_api_impl.py | 9 +++++ .../user_service/test_subscription_feeds.py | 4 ++- .../user_service/test_subscription_helpers.py | 35 +++++++++++++++++++ .../user_service/test_users_api_impl.py | 23 ++++++++++++ 5 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 api/tests/unittest/user_service/test_subscription_helpers.py diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 133c74abf..65a85f7f4 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -22,6 +22,8 @@ import sib_api_v3_sdk import urllib3 from shared.common.brevo import add_contact_to_list, get_announcements_list_id, remove_contact_from_list +from shared.database.database import Database +from shared.database_gen.sqlacodegen_models import Feed from shared.users_database_gen.sqlacodegen_models import FeatureFlag, UserFeatureFlag logger = logging.getLogger(__name__) @@ -81,6 +83,27 @@ def feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: return enabled +def find_unknown_feed_ids(feed_ids, feeds_db_session=None) -> list: + """Return the supplied feed stable IDs that do not exist as a ``Feed`` in the feeds DB. + + Order is preserved and duplicates collapsed. Feeds live in a separate database from + subscriptions, so this opens its own feeds-DB session unless one is injected (tests). + """ + unique_ids = list(dict.fromkeys(feed_ids or [])) + if not unique_ids: + return [] + + def _missing(session) -> list: + rows = session.query(Feed.stable_id).filter(Feed.stable_id.in_(unique_ids)).all() + existing = {stable_id for (stable_id,) in rows} + return [feed_id for feed_id in unique_ids if feed_id not in existing] + + if feeds_db_session is not None: + return _missing(feeds_db_session) + with Database().start_db_session() as session: + return _missing(session) + + def sync_announcements(email: str, subscribe: bool, subscription_id: str | None = None) -> None: """Sync an api.announcements subscription with Brevo, mapping provider errors to 502.""" try: diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index 8d609b31d..3851296cd 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -43,6 +43,7 @@ FEED_SCOPED_NOTIFICATION_TYPE_IDS, NOTIFICATIONS_FEATURE_FLAG_ID, feature_flag_enabled, + find_unknown_feed_ids, sync_announcements, ) from user_service_gen.apis.users_api_base import BaseUsersApi @@ -182,6 +183,14 @@ def create_user_subscription( detail=f"feed_ids is not supported for notification type '{notification_id}'.", ) + # Every supplied feed must exist (feeds live in a separate DB, referenced by stable_id). + unknown_feed_ids = find_unknown_feed_ids(feed_ids) + if unknown_feed_ids: + raise HTTPException( + status_code=400, + detail=f"Unknown feed stable IDs: {', '.join(unknown_feed_ids)}.", + ) + user = db_session.get(AppUser, user_id) if user is None: raise HTTPException(status_code=404, detail="User not found.") diff --git a/api/tests/unittest/user_service/test_subscription_feeds.py b/api/tests/unittest/user_service/test_subscription_feeds.py index bb41a9fb8..f3a7c743b 100644 --- a/api/tests/unittest/user_service/test_subscription_feeds.py +++ b/api/tests/unittest/user_service/test_subscription_feeds.py @@ -60,8 +60,10 @@ def test_feed_scoped_relationship_uses_delete_orphan_and_passive_deletes(): @pytest.fixture -def api_session(users_test_database_url): +def api_session(users_test_database_url, monkeypatch): """A real users-DB session plus seeded user/type, always rolled back.""" + # Feed existence is checked against the separate feeds DB; treat the test feed ids as valid. + monkeypatch.setattr("user_service.impl.users_api_impl.find_unknown_feed_ids", lambda *a, **k: []) _reset_singleton() db = UsersDatabase() suffix = uuid.uuid4().hex diff --git a/api/tests/unittest/user_service/test_subscription_helpers.py b/api/tests/unittest/user_service/test_subscription_helpers.py new file mode 100644 index 000000000..4e478ee53 --- /dev/null +++ b/api/tests/unittest/user_service/test_subscription_helpers.py @@ -0,0 +1,35 @@ +import unittest +from unittest.mock import MagicMock + +from user_service.impl.subscription_helpers import find_unknown_feed_ids + + +class TestFindUnknownFeedIds(unittest.TestCase): + def _session_with_existing(self, existing_ids): + session = MagicMock() + session.query.return_value.filter.return_value.all.return_value = [(fid,) for fid in existing_ids] + return session + + def test_empty_returns_empty_without_query(self): + session = MagicMock() + self.assertEqual(find_unknown_feed_ids([], feeds_db_session=session), []) + session.query.assert_not_called() + + def test_all_known(self): + session = self._session_with_existing(["mdb-1", "mdb-2"]) + self.assertEqual(find_unknown_feed_ids(["mdb-1", "mdb-2"], feeds_db_session=session), []) + + def test_returns_only_unknown_preserving_order(self): + session = self._session_with_existing(["mdb-1"]) + self.assertEqual( + find_unknown_feed_ids(["mdb-1", "mdba-100", "mdb-nope"], feeds_db_session=session), + ["mdba-100", "mdb-nope"], + ) + + def test_dedupes_before_checking(self): + session = self._session_with_existing([]) + self.assertEqual(find_unknown_feed_ids(["mdba-100", "mdba-100"], feeds_db_session=session), ["mdba-100"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index 2a38cfc15..733c04d31 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -227,6 +227,10 @@ def setUp(self): gate_patcher = patch.object(UsersApiImpl, "_require_notifications_enabled", return_value=None) gate_patcher.start() self.addCleanup(gate_patcher.stop) + # Feed existence is checked against the (separate) feeds DB; treat all feeds as valid here. + feed_patcher = patch("user_service.impl.users_api_impl.find_unknown_feed_ids", return_value=[]) + feed_patcher.start() + self.addCleanup(feed_patcher.stop) def _make_sub(self, **kwargs): from shared.users_database_gen.sqlacodegen_models import NotificationSubscription as Orm @@ -362,6 +366,25 @@ def test_create_feed_scoped_dedupes_feed_ids(self): self.assertEqual(result.feed_ids, ["mdb-1"]) + def test_create_feed_scoped_rejects_unknown_feed_ids(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.url_updated") if model is NotificationType else _make_user() + ) + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + with patch("user_service.impl.users_api_impl.find_unknown_feed_ids", return_value=["mdba-100"]): + with self.assertRaises(HTTPException) as ctx: + self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.url_updated", feed_ids=["mdba-100"]), + db_session=self.mock_session, + ) + + self.assertEqual(ctx.exception.status_code, 400) + self.assertIn("mdba-100", ctx.exception.detail) + self.mock_session.add.assert_not_called() + def test_create_feed_scoped_requires_feed_ids(self): from shared.users_database_gen.sqlacodegen_models import NotificationType From af9e7b20874b953de7ac931c3b4e4bb7dc0dae7f Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:11:56 -0400 Subject: [PATCH 09/10] remove 501 from all implemented endpoints --- docs/UserServiceAPI.yaml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/docs/UserServiceAPI.yaml b/docs/UserServiceAPI.yaml index c71eff327..f7735085e 100644 --- a/docs/UserServiceAPI.yaml +++ b/docs/UserServiceAPI.yaml @@ -111,8 +111,6 @@ paths: $ref: "#/components/schemas/NotificationType" "401": description: Unauthorized. - "501": - description: Not yet implemented. /v1/user/subscriptions: get: @@ -134,8 +132,6 @@ paths: $ref: "#/components/schemas/NotificationSubscription" "401": description: Unauthorized. - "501": - description: Not yet implemented. post: summary: Create a notification subscription description: Subscribes the authenticated user to a notification type. This feature is available to selected users. To request access or learn more, contact us at api@mobilitydata.org. @@ -161,8 +157,6 @@ paths: description: Invalid request. "401": description: Unauthorized. - "501": - description: Not yet implemented. /v1/user/subscriptions/{id}: patch: @@ -197,8 +191,6 @@ paths: description: Unauthorized. "404": description: Subscription not found. - "501": - description: Not yet implemented. delete: summary: Delete or disable a notification subscription description: >- @@ -224,8 +216,6 @@ paths: description: Unauthorized. "404": description: Subscription not found. - "501": - description: Not yet implemented. /v1/subscriptions/{id}: get: From e87d7135ef6f5df66f251dbf42b0c9459d783ce4 Mon Sep 17 00:00:00 2001 From: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:06:30 -0400 Subject: [PATCH 10/10] add feeds metadata to the subscription endpoint --- api/.openapi-generator/FILES | 1 + .../notification_subscription_impl.py | 27 +++++++++--- .../user_service/impl/subscription_helpers.py | 41 ++++++++++++------ .../impl/subscriptions_api_impl.py | 5 ++- api/src/user_service/impl/users_api_impl.py | 28 +++++++++---- .../user_service/test_subscription_feeds.py | 7 ++-- .../user_service/test_subscription_helpers.py | 32 +++++++++++--- .../test_subscriptions_api_impl.py | 15 +++++-- .../user_service/test_users_api_impl.py | 39 ++++++++++++++--- docs/UserServiceAPI.yaml | 42 +++++++++++++++---- 10 files changed, 185 insertions(+), 52 deletions(-) diff --git a/api/.openapi-generator/FILES b/api/.openapi-generator/FILES index 35fcc3da8..7774a80bc 100644 --- a/api/.openapi-generator/FILES +++ b/api/.openapi-generator/FILES @@ -62,3 +62,4 @@ src/user_service_gen/models/update_notification_subscription_request.py src/user_service_gen/models/update_user_request.py src/user_service_gen/models/user_profile.py src/user_service_gen/security_api.py +src/user_service_gen/models/subscription_feed.py diff --git a/api/src/shared/db_models/notification_subscription_impl.py b/api/src/shared/db_models/notification_subscription_impl.py index 4c61fe1c8..5e1cfdc6f 100644 --- a/api/src/shared/db_models/notification_subscription_impl.py +++ b/api/src/shared/db_models/notification_subscription_impl.py @@ -1,5 +1,6 @@ from shared.users_database_gen.sqlacodegen_models import NotificationSubscription as NotificationSubscriptionOrm from user_service_gen.models.notification_subscription import NotificationSubscription +from user_service_gen.models.subscription_feed import SubscriptionFeed class NotificationSubscriptionImpl(NotificationSubscription): @@ -11,18 +12,32 @@ class Config: from_attributes = True @classmethod - def from_orm(cls, sub: NotificationSubscriptionOrm | None) -> NotificationSubscription | None: + def from_orm( + cls, + sub: NotificationSubscriptionOrm | None, + feed_metadata: dict | None = None, + ) -> NotificationSubscription | None: if not sub: return None - # feed_ids is the set of feed stable IDs this subscription targets, taken from the - # notification_subscription_feed join table. Sorted for a stable response; None when - # the subscription targets no feeds (i.e. non feed-scoped types). - feed_ids = sorted(f.feed_stable_id for f in sub.notification_subscription_feeds) + # The subscription's targeted feeds come from the notification_subscription_feed join + # table (sorted by stable ID for a stable response). Each feed is enriched with + # data_type/provider/feed_name resolved (by the caller) from the feeds DB; metadata is + # absent for a feed that no longer exists, so its fields stay null. + feed_metadata = feed_metadata or {} + feeds = [ + SubscriptionFeed( + feed_id=stable_id, + data_type=feed_metadata.get(stable_id, {}).get("data_type"), + provider=feed_metadata.get(stable_id, {}).get("provider"), + feed_name=feed_metadata.get(stable_id, {}).get("feed_name"), + ) + for stable_id in sorted(f.feed_stable_id for f in sub.notification_subscription_feeds) + ] return cls( id=sub.id, user_id=sub.user_id, notification_id=sub.notification_type_id, active=sub.active, created_at=sub.created_at, - feed_ids=feed_ids or None, + feeds=feeds or None, ) diff --git a/api/src/user_service/impl/subscription_helpers.py b/api/src/user_service/impl/subscription_helpers.py index 212ff5579..e6fae7c5c 100644 --- a/api/src/user_service/impl/subscription_helpers.py +++ b/api/src/user_service/impl/subscription_helpers.py @@ -28,7 +28,7 @@ get_announcements_list_id, remove_contact_from_list, ) -from shared.database.database import Database, generate_unique_id +from shared.database.database import generate_unique_id, with_db_session from shared.database_gen.sqlacodegen_models import Feed from shared.users_database_gen.sqlacodegen_models import ( AppUser, @@ -94,25 +94,42 @@ def feature_flag_enabled(db_session, user_id: str, flag_id: str) -> bool: return enabled -def find_unknown_feed_ids(feed_ids, feeds_db_session=None) -> list: +@with_db_session +def find_unknown_feed_ids(feed_ids, db_session=None) -> list: """Return the supplied feed stable IDs that do not exist as a ``Feed`` in the feeds DB. - Order is preserved and duplicates collapsed. Feeds live in a separate database from - subscriptions, so this opens its own feeds-DB session unless one is injected (tests). + Order is preserved and duplicates collapsed. ``db_session`` is the feeds-DB session injected + by ``@with_db_session`` (feeds live in a separate database from subscriptions). """ unique_ids = list(dict.fromkeys(feed_ids or [])) if not unique_ids: return [] + rows = db_session.query(Feed.stable_id).filter(Feed.stable_id.in_(unique_ids)).all() + existing = {stable_id for (stable_id,) in rows} + return [feed_id for feed_id in unique_ids if feed_id not in existing] - def _missing(session) -> list: - rows = session.query(Feed.stable_id).filter(Feed.stable_id.in_(unique_ids)).all() - existing = {stable_id for (stable_id,) in rows} - return [feed_id for feed_id in unique_ids if feed_id not in existing] - if feeds_db_session is not None: - return _missing(feeds_db_session) - with Database().start_db_session() as session: - return _missing(session) +@with_db_session +def resolve_feed_metadata(stable_ids, db_session=None) -> dict: + """Resolve display metadata for feeds by stable ID from the feeds DB. + + Returns ``{stable_id: {"data_type", "provider", "feed_name"}}`` for the feeds that exist. + Feed metadata is intentionally NOT persisted on the subscription; it is resolved at read + time so responses always reflect current feed data. ``db_session`` is the feeds-DB session + injected by ``@with_db_session``. + """ + unique_ids = list(dict.fromkeys(stable_ids or [])) + if not unique_ids: + return {} + rows = ( + db_session.query(Feed.stable_id, Feed.data_type, Feed.provider, Feed.feed_name) + .filter(Feed.stable_id.in_(unique_ids)) + .all() + ) + return { + row.stable_id: {"data_type": row.data_type, "provider": row.provider, "feed_name": row.feed_name} + for row in rows + } def sync_announcements( diff --git a/api/src/user_service/impl/subscriptions_api_impl.py b/api/src/user_service/impl/subscriptions_api_impl.py index a0020ba55..dbca3e08e 100644 --- a/api/src/user_service/impl/subscriptions_api_impl.py +++ b/api/src/user_service/impl/subscriptions_api_impl.py @@ -29,6 +29,7 @@ ERROR_MESSAGE_USER_FEATURE_NOT_ENABLED, NOTIFICATIONS_FEATURE_FLAG_ID, feature_flag_enabled, + resolve_feed_metadata, set_announcements_optin, ) from user_service_gen.apis.subscriptions_api_base import BaseSubscriptionsApi @@ -48,7 +49,9 @@ def get_subscription(self, id: str, db_session=None) -> NotificationSubscription sub = db_session.get(NotificationSubscriptionOrm, id) if sub is None: raise HTTPException(status_code=404, detail="Subscription not found.") - return NotificationSubscriptionImpl.from_orm(sub) + stable_ids = [f.feed_stable_id for f in sub.notification_subscription_feeds] + feed_metadata = resolve_feed_metadata(stable_ids) if stable_ids else {} + return NotificationSubscriptionImpl.from_orm(sub, feed_metadata) @with_users_db_session def delete_subscription(self, id: str, db_session=None) -> None: diff --git a/api/src/user_service/impl/users_api_impl.py b/api/src/user_service/impl/users_api_impl.py index 4d7ceced3..414e7303d 100644 --- a/api/src/user_service/impl/users_api_impl.py +++ b/api/src/user_service/impl/users_api_impl.py @@ -44,6 +44,7 @@ NOTIFICATIONS_FEATURE_FLAG_ID, feature_flag_enabled, find_unknown_feed_ids, + resolve_feed_metadata, set_announcements_optin, ) from user_service_gen.apis.users_api_base import BaseUsersApi @@ -162,7 +163,11 @@ def get_user_subscriptions(self, db_session=None) -> List[NotificationSubscripti .order_by(NotificationSubscriptionOrm.created_at) .all() ) - return [NotificationSubscriptionImpl.from_orm(s) for s in subs] + # Resolve feed metadata for every targeted feed in a single feeds-DB query (skipped + # entirely when the user has no feed-scoped subscriptions). + stable_ids = [f.feed_stable_id for s in subs for f in s.notification_subscription_feeds] + feed_metadata = resolve_feed_metadata(stable_ids) if stable_ids else {} + return [NotificationSubscriptionImpl.from_orm(s, feed_metadata) for s in subs] @with_users_db_session def create_user_subscription( @@ -203,12 +208,13 @@ def create_user_subscription( ) # Every supplied feed must exist (feeds live in a separate DB, referenced by stable_id). - unknown_feed_ids = find_unknown_feed_ids(feed_ids) - if unknown_feed_ids: - raise HTTPException( - status_code=400, - detail=f"Unknown feed stable IDs: {', '.join(unknown_feed_ids)}.", - ) + if feed_ids: + unknown_feed_ids = find_unknown_feed_ids(feed_ids) + if unknown_feed_ids: + raise HTTPException( + status_code=400, + detail=f"Unknown feed stable IDs: {', '.join(unknown_feed_ids)}.", + ) user = db_session.get(AppUser, user_id) if user is None: @@ -242,7 +248,9 @@ def create_user_subscription( db_session.add(sub) db_session.flush() - return NotificationSubscriptionImpl.from_orm(sub) + stable_ids = [f.feed_stable_id for f in sub.notification_subscription_feeds] + feed_metadata = resolve_feed_metadata(stable_ids) if stable_ids else {} + return NotificationSubscriptionImpl.from_orm(sub, feed_metadata) @with_users_db_session def update_user_subscription( @@ -264,7 +272,9 @@ def update_user_subscription( else: sub.active = active db_session.flush() - return NotificationSubscriptionImpl.from_orm(sub) + stable_ids = [f.feed_stable_id for f in sub.notification_subscription_feeds] + feed_metadata = resolve_feed_metadata(stable_ids) if stable_ids else {} + return NotificationSubscriptionImpl.from_orm(sub, feed_metadata) @with_users_db_session def delete_user_subscription(self, id: str, db_session=None) -> None: diff --git a/api/tests/unittest/user_service/test_subscription_feeds.py b/api/tests/unittest/user_service/test_subscription_feeds.py index f3a7c743b..58408d01e 100644 --- a/api/tests/unittest/user_service/test_subscription_feeds.py +++ b/api/tests/unittest/user_service/test_subscription_feeds.py @@ -62,8 +62,9 @@ def test_feed_scoped_relationship_uses_delete_orphan_and_passive_deletes(): @pytest.fixture def api_session(users_test_database_url, monkeypatch): """A real users-DB session plus seeded user/type, always rolled back.""" - # Feed existence is checked against the separate feeds DB; treat the test feed ids as valid. + # Feed existence + metadata are resolved against the separate feeds DB; stub both here. monkeypatch.setattr("user_service.impl.users_api_impl.find_unknown_feed_ids", lambda *a, **k: []) + monkeypatch.setattr("user_service.impl.users_api_impl.resolve_feed_metadata", lambda *a, **k: {}) _reset_singleton() db = UsersDatabase() suffix = uuid.uuid4().hex @@ -105,7 +106,7 @@ def test_create_persists_feed_ids(api_session): ) assert result.notification_id == FEED_SCOPED_TYPE - assert result.feed_ids == ["mdb-1", "mdb-2"] # sorted + deduped + assert [f.feed_id for f in result.feeds] == ["mdb-1", "mdb-2"] # sorted + deduped assert _feed_rows(session, result.id) == {"mdb-1", "mdb-2"} @@ -123,7 +124,7 @@ def test_create_replaces_feed_set_idempotently(api_session): # Same single subscription, feed set replaced. assert second.id == first.id - assert second.feed_ids == ["mdb-3"] + assert [f.feed_id for f in second.feeds] == ["mdb-3"] assert _feed_rows(session, first.id) == {"mdb-3"} diff --git a/api/tests/unittest/user_service/test_subscription_helpers.py b/api/tests/unittest/user_service/test_subscription_helpers.py index 4e478ee53..349f72b8e 100644 --- a/api/tests/unittest/user_service/test_subscription_helpers.py +++ b/api/tests/unittest/user_service/test_subscription_helpers.py @@ -1,7 +1,8 @@ import unittest +from types import SimpleNamespace from unittest.mock import MagicMock -from user_service.impl.subscription_helpers import find_unknown_feed_ids +from user_service.impl.subscription_helpers import find_unknown_feed_ids, resolve_feed_metadata class TestFindUnknownFeedIds(unittest.TestCase): @@ -12,23 +13,44 @@ def _session_with_existing(self, existing_ids): def test_empty_returns_empty_without_query(self): session = MagicMock() - self.assertEqual(find_unknown_feed_ids([], feeds_db_session=session), []) + self.assertEqual(find_unknown_feed_ids([], db_session=session), []) session.query.assert_not_called() def test_all_known(self): session = self._session_with_existing(["mdb-1", "mdb-2"]) - self.assertEqual(find_unknown_feed_ids(["mdb-1", "mdb-2"], feeds_db_session=session), []) + self.assertEqual(find_unknown_feed_ids(["mdb-1", "mdb-2"], db_session=session), []) def test_returns_only_unknown_preserving_order(self): session = self._session_with_existing(["mdb-1"]) self.assertEqual( - find_unknown_feed_ids(["mdb-1", "mdba-100", "mdb-nope"], feeds_db_session=session), + find_unknown_feed_ids(["mdb-1", "mdba-100", "mdb-nope"], db_session=session), ["mdba-100", "mdb-nope"], ) def test_dedupes_before_checking(self): session = self._session_with_existing([]) - self.assertEqual(find_unknown_feed_ids(["mdba-100", "mdba-100"], feeds_db_session=session), ["mdba-100"]) + self.assertEqual(find_unknown_feed_ids(["mdba-100", "mdba-100"], db_session=session), ["mdba-100"]) + + +class TestResolveFeedMetadata(unittest.TestCase): + def _session_with_rows(self, rows): + session = MagicMock() + session.query.return_value.filter.return_value.all.return_value = [ + SimpleNamespace(stable_id=sid, data_type=dt, provider=pr, feed_name=fn) for (sid, dt, pr, fn) in rows + ] + return session + + def test_empty_returns_empty_without_query(self): + session = MagicMock() + self.assertEqual(resolve_feed_metadata([], db_session=session), {}) + session.query.assert_not_called() + + def test_maps_stable_id_to_metadata(self): + session = self._session_with_rows([("mdb-1", "gtfs", "MTA", "Subway")]) + result = resolve_feed_metadata(["mdb-1", "mdb-missing"], db_session=session) + self.assertEqual(result, {"mdb-1": {"data_type": "gtfs", "provider": "MTA", "feed_name": "Subway"}}) + # A missing feed simply has no entry (caller fills nulls). + self.assertNotIn("mdb-missing", result) if __name__ == "__main__": diff --git a/api/tests/unittest/user_service/test_subscriptions_api_impl.py b/api/tests/unittest/user_service/test_subscriptions_api_impl.py index e5ccb7023..d0ad652d5 100644 --- a/api/tests/unittest/user_service/test_subscriptions_api_impl.py +++ b/api/tests/unittest/user_service/test_subscriptions_api_impl.py @@ -68,10 +68,17 @@ def test_returns_feed_ids_for_feed_scoped_subscription(self): sub.notification_subscription_feeds.append(NotificationSubscriptionFeedOrm(feed_stable_id="mdb-2")) self.mock_session.get.return_value = sub - result = self.api.get_subscription("sub-1", db_session=self.mock_session) - - # feed_ids come from the join table, sorted. - self.assertEqual(result.feed_ids, ["mdb-2", "mdb-9"]) + metadata = {"mdb-9": {"data_type": "gtfs", "provider": "P9", "feed_name": "F9"}} + with patch("user_service.impl.subscriptions_api_impl.resolve_feed_metadata", return_value=metadata): + result = self.api.get_subscription("sub-1", db_session=self.mock_session) + + # feeds come from the join table, sorted by id. + self.assertEqual([f.feed_id for f in result.feeds], ["mdb-2", "mdb-9"]) + # feeds carries resolved metadata per feed (null where unresolved). + by_id = {f.feed_id: f for f in result.feeds} + self.assertEqual(by_id["mdb-9"].data_type, "gtfs") + self.assertEqual(by_id["mdb-9"].provider, "P9") + self.assertIsNone(by_id["mdb-2"].data_type) def test_missing_returns_404(self): self.mock_session.get.return_value = None diff --git a/api/tests/unittest/user_service/test_users_api_impl.py b/api/tests/unittest/user_service/test_users_api_impl.py index 3a45651e6..c2a4840d7 100644 --- a/api/tests/unittest/user_service/test_users_api_impl.py +++ b/api/tests/unittest/user_service/test_users_api_impl.py @@ -295,6 +295,10 @@ def setUp(self): feed_patcher = patch("user_service.impl.users_api_impl.find_unknown_feed_ids", return_value=[]) feed_patcher.start() self.addCleanup(feed_patcher.stop) + # Feed metadata is resolved from the feeds DB; default to none resolved in these tests. + meta_patcher = patch("user_service.impl.users_api_impl.resolve_feed_metadata", return_value={}) + meta_patcher.start() + self.addCleanup(meta_patcher.stop) def _make_sub(self, **kwargs): from shared.users_database_gen.sqlacodegen_models import NotificationSubscription as Orm @@ -322,7 +326,7 @@ def test_get_user_subscriptions_returns_user_subs(self): self.assertEqual(result[0].id, "sub-1") self.assertEqual(result[0].notification_id, "feed.published") # A non feed-scoped subscription reports no feeds. - self.assertIsNone(result[0].feed_ids) + self.assertIsNone(result[0].feeds) def test_get_user_subscriptions_guest_403(self): _set_context(is_guest=True) @@ -412,8 +416,33 @@ def test_create_feed_scoped_persists_feed_ids(self): added_sub = self.mock_session.add.call_args[0][0] # Both feeds are attached to the new subscription's join collection. self.assertEqual({f.feed_stable_id for f in added_sub.notification_subscription_feeds}, {"mdb-1", "mdb-2"}) - # Response feed_ids are sorted and deduped. - self.assertEqual(result.feed_ids, ["mdb-1", "mdb-2"]) + # Response feeds are sorted by id and deduped. + self.assertEqual([f.feed_id for f in result.feeds], ["mdb-1", "mdb-2"]) + + def test_create_feed_scoped_returns_resolved_feed_metadata(self): + from shared.users_database_gen.sqlacodegen_models import NotificationType + + self.mock_session.get.side_effect = lambda model, key: ( + NotificationType(id="feed.url_updated") if model is NotificationType else _make_user() + ) + self.mock_session.query.return_value.filter.return_value.one_or_none.return_value = None + + metadata = {"mdb-1": {"data_type": "gtfs", "provider": "MTA", "feed_name": "Subway"}} + with patch("user_service.impl.users_api_impl.resolve_feed_metadata", return_value=metadata): + result = self.api.create_user_subscription( + CreateNotificationSubscriptionRequest(notification_id="feed.url_updated", feed_ids=["mdb-1", "mdb-2"]), + db_session=self.mock_session, + ) + + by_id = {f.feed_id: f for f in result.feeds} + self.assertEqual(set(by_id), {"mdb-1", "mdb-2"}) + self.assertEqual( + (by_id["mdb-1"].data_type, by_id["mdb-1"].provider, by_id["mdb-1"].feed_name), ("gtfs", "MTA", "Subway") + ) + # A feed with no resolved metadata (e.g. deleted) still appears with null fields. + self.assertEqual( + (by_id["mdb-2"].data_type, by_id["mdb-2"].provider, by_id["mdb-2"].feed_name), (None, None, None) + ) def test_create_feed_scoped_dedupes_feed_ids(self): from shared.users_database_gen.sqlacodegen_models import NotificationType @@ -428,7 +457,7 @@ def test_create_feed_scoped_dedupes_feed_ids(self): db_session=self.mock_session, ) - self.assertEqual(result.feed_ids, ["mdb-1"]) + self.assertEqual([f.feed_id for f in result.feeds], ["mdb-1"]) def test_create_feed_scoped_rejects_unknown_feed_ids(self): from shared.users_database_gen.sqlacodegen_models import NotificationType @@ -502,7 +531,7 @@ def test_create_feed_scoped_replaces_existing_feeds(self): self.assertTrue(existing.active) # The old feed was dropped and the new one attached (delete-orphan handles the DB side). self.assertEqual({f.feed_stable_id for f in existing.notification_subscription_feeds}, {"mdb-new"}) - self.assertEqual(result.feed_ids, ["mdb-new"]) + self.assertEqual([f.feed_id for f in result.feeds], ["mdb-new"]) # ── update ── def test_update_deactivate_announcement_removes_brevo(self): diff --git a/docs/UserServiceAPI.yaml b/docs/UserServiceAPI.yaml index f7735085e..c2169bb08 100644 --- a/docs/UserServiceAPI.yaml +++ b/docs/UserServiceAPI.yaml @@ -380,16 +380,44 @@ components: type: string format: date-time description: Timestamp when the subscription was created. - feed_ids: + feeds: type: array - items: - type: string nullable: true description: > - Feed stable IDs this subscription targets. Populated for the - feed-scoped notification types (feed.url_updated, - feed.url_availability, feed.coverage); empty for other types. - example: ["mdb-1", "mdb-42"] + The feeds this subscription targets (feed-scoped notification types: + feed.url_updated, feed.url_availability, feed.coverage), each with + its resolved metadata. Resolved from the feeds database at read time + using the stable feed IDs — not persisted on the subscription. + Consumers can build a human-readable description from these fields. + items: + $ref: "#/components/schemas/SubscriptionFeed" + + SubscriptionFeed: + type: object + description: > + Metadata for a feed targeted by a notification subscription, resolved + from the feed's stable ID at read time (not persisted). + required: + - feed_id + properties: + feed_id: + type: string + description: The feed's stable ID. + example: "mdb-1" + data_type: + type: string + nullable: true + description: The feed's data type. + example: "gtfs" + provider: + type: string + nullable: true + description: The transit/mobility data provider name. + example: "Metropolitan Transit Authority" + feed_name: + type: string + nullable: true + description: The feed's display name. CreateNotificationSubscriptionRequest: type: object