diff --git a/README.md b/README.md index 4308659..dab9747 100644 --- a/README.md +++ b/README.md @@ -93,6 +93,18 @@ Post-deploy checks: * scheduler can still hit backend internally * Grafana still loads +## Optix Login Bootstrap + +`POST /api/optix-token` is the Optix-authenticated login/bootstrap path. + +On each successful Optix login: + +* refresh Optix-owned user fields only when the incoming values changed +* refresh existing team names and team mailbox display names only when the incoming values changed +* preserve Avenu-owned user preferences such as `notifPrefs` + +This keeps local identity data current without rewriting unchanged rows on every login. + ## Learn more * Architecture overview: `docs/01-architecture/overview.md` diff --git a/backend/repositories/teams_repository.py b/backend/repositories/teams_repository.py index a3cfb36..3045e8c 100644 --- a/backend/repositories/teams_repository.py +++ b/backend/repositories/teams_repository.py @@ -8,7 +8,7 @@ from config import teams_collection, users_collection from errors import APIError -from models import build_mailbox_doc, build_team_create +from models import build_mailbox_doc, build_team_create, build_team_patch from .common import run_in_transaction from .mail_repository import delete_by_mailbox @@ -103,7 +103,14 @@ def work(session: Any) -> None: def ensure_team_from_external_identity(*, optix_id: int, name: str) -> dict[str, Any]: existing = find_team_by_optix_id(optix_id) if existing is not None: - return existing + candidate_patch = build_team_patch({"name": name}) + if existing.get("name") == candidate_patch["name"]: + return existing + patch = { + "name": candidate_patch["name"], + "updatedAt": candidate_patch["updatedAt"], + } + return update_team_with_mailbox_sync(team_id=existing["_id"], patch=patch) team_doc = build_team_create({"optixId": optix_id, "name": name}) mailbox_doc = build_mailbox_doc(owner_type="team", ref_id=ObjectId(), display_name=team_doc["name"]) diff --git a/backend/repositories/users_repository.py b/backend/repositories/users_repository.py index fc4c2cc..7d32424 100644 --- a/backend/repositories/users_repository.py +++ b/backend/repositories/users_repository.py @@ -14,6 +14,9 @@ from .mail_repository import delete_by_mailbox from .mailboxes_repository import delete_mailbox, find_owner_mailbox, insert_mailbox, update_owner_display_name from .teams_repository import count_by_ids +from services.user_preferences import normalize_effective_notification_state + +DEFAULT_EXTERNAL_IDENTITY_NOTIF_PREFS = ["email"] def list_users() -> list[dict[str, Any]]: @@ -152,7 +155,6 @@ def upsert_user_from_external_identity( phone: str, is_admin: bool, team_ids: list[ObjectId], - notif_prefs: list[str], ) -> tuple[dict[str, Any], bool]: existing = find_user_by_optix_id(optix_id) if existing is None: @@ -164,7 +166,7 @@ def upsert_user_from_external_identity( "phone": phone, "isAdmin": is_admin, "teamIds": team_ids, - "notifPrefs": notif_prefs, + "notifPrefs": DEFAULT_EXTERNAL_IDENTITY_NOTIF_PREFS, } ) mailbox_doc = build_mailbox_doc(owner_type="user", ref_id=ObjectId(), display_name=user_doc["fullname"]) @@ -174,16 +176,37 @@ def upsert_user_from_external_identity( except DuplicateKeyError as exc: raise APIError(409, "user with same optixId or email already exists") from exc - patch = build_user_patch( + candidate_patch = build_user_patch( { "fullname": fullname, "email": email, "phone": phone, "isAdmin": is_admin, "teamIds": team_ids, - "notifPrefs": notif_prefs, } ) + patch: dict[str, Any] = {} + for key in ("fullname", "email", "phone", "isAdmin"): + if existing.get(key) != candidate_patch.get(key): + patch[key] = candidate_patch[key] + + if "phone" in patch: + normalized = normalize_effective_notification_state( + current_user=existing, + phone_patch=candidate_patch["phone"], + ) + if normalized["notifPrefs"] != existing.get("notifPrefs", []): + patch["notifPrefs"] = normalized["notifPrefs"] + + current_team_ids = {str(team_id) for team_id in existing.get("teamIds", [])} + next_team_ids = {str(team_id) for team_id in candidate_patch.get("teamIds", [])} + if current_team_ids != next_team_ids: + patch["teamIds"] = candidate_patch["teamIds"] + + if not patch: + return existing, False + + patch["updatedAt"] = candidate_patch["updatedAt"] try: updated = update_user_with_mailbox_sync(user_id=existing["_id"], patch=patch) return updated, False diff --git a/backend/services/identity_sync_service.py b/backend/services/identity_sync_service.py index c5a1aac..5a96955 100644 --- a/backend/services/identity_sync_service.py +++ b/backend/services/identity_sync_service.py @@ -99,6 +99,5 @@ def sync_optix_identity(*, token: str) -> tuple[bool, dict[str, Any]]: phone=user_info.get("phone", ""), is_admin=bool(user_info.get("is_admin", False)), team_ids=team_ids, - notif_prefs=["email"], ) return created, user_doc diff --git a/backend/tests/unit/test_identity_sync_service.py b/backend/tests/unit/test_identity_sync_service.py index 4045e91..62cd7f8 100644 --- a/backend/tests/unit/test_identity_sync_service.py +++ b/backend/tests/unit/test_identity_sync_service.py @@ -78,6 +78,7 @@ def test_sync_optix_identity_creates_new_user_and_team(self): kwargs = upsert_user_mock.call_args.kwargs self.assertEqual(kwargs["optix_id"], 123) self.assertEqual(kwargs["team_ids"], [team_id]) + self.assertNotIn("notif_prefs", kwargs) def test_sync_optix_identity_updates_existing_user(self): user_id = ObjectId() @@ -109,6 +110,53 @@ def test_sync_optix_identity_updates_existing_user(self): self.assertFalse(created) self.assertEqual(user_doc["_id"], user_id) + def test_sync_optix_identity_forwards_updated_phone_and_team_membership_for_existing_user(self): + user_id = ObjectId() + first_team_id = ObjectId() + second_team_id = ObjectId() + + with patch( + "services.identity_sync_service.requests.post", + return_value=FakeResponse( + status_code=200, + payload={ + "data": { + "me": { + "user": { + "user_id": 123, + "email": "member@example.com", + "fullname": "Updated Name", + "phone": "+15550001111", + "is_admin": True, + "teams": [ + {"team_id": 99, "name": "Team One"}, + {"team_id": 100, "name": "Team Two"}, + ], + } + } + } + }, + ), + ), patch( + "services.identity_sync_service.ensure_team_from_external_identity", + side_effect=[ + {"_id": first_team_id, "optixId": 99, "name": "Team One"}, + {"_id": second_team_id, "optixId": 100, "name": "Team Two"}, + ], + ), patch( + "services.identity_sync_service.upsert_user_from_external_identity", + return_value=({"_id": user_id, "optixId": 123, "email": "member@example.com"}, False), + ) as upsert_user_mock: + created, user_doc = sync_optix_identity(token="optix-token") + + self.assertFalse(created) + self.assertEqual(user_doc["_id"], user_id) + kwargs = upsert_user_mock.call_args.kwargs + self.assertEqual(kwargs["fullname"], "Updated Name") + self.assertEqual(kwargs["phone"], "+15550001111") + self.assertEqual(kwargs["team_ids"], [first_team_id, second_team_id]) + self.assertNotIn("notif_prefs", kwargs) + def test_sync_optix_identity_raises_on_optix_failure_and_does_not_write_local_state(self): with patch( "services.identity_sync_service.requests.post", diff --git a/backend/tests/unit/test_teams_repository.py b/backend/tests/unit/test_teams_repository.py new file mode 100644 index 0000000..fa087e3 --- /dev/null +++ b/backend/tests/unit/test_teams_repository.py @@ -0,0 +1,55 @@ +import os +import unittest +from unittest.mock import patch + +from bson import ObjectId + +os.environ.setdefault("MONGO_URI", "mongodb://localhost:27017") + +from repositories.teams_repository import ensure_team_from_external_identity + + +class TeamsRepositoryExternalIdentityTests(unittest.TestCase): + def test_ensure_team_from_external_identity_creates_new_team_when_missing(self): + created_team = {"_id": ObjectId(), "optixId": 99, "name": "Team One"} + + with patch("repositories.teams_repository.find_team_by_optix_id", return_value=None), patch( + "repositories.teams_repository.create_team_with_mailbox", + return_value=created_team, + ) as create_mock: + team_doc = ensure_team_from_external_identity(optix_id=99, name="Team One") + + self.assertEqual(team_doc, created_team) + self.assertEqual(create_mock.call_args.kwargs["team_doc"]["name"], "Team One") + + def test_ensure_team_from_external_identity_refreshes_existing_team_name_when_changed(self): + team_id = ObjectId() + existing = {"_id": team_id, "optixId": 99, "name": "Old Team"} + updated = {"_id": team_id, "optixId": 99, "name": "New Team"} + + with patch("repositories.teams_repository.find_team_by_optix_id", return_value=existing), patch( + "repositories.teams_repository.update_team_with_mailbox_sync", + return_value=updated, + ) as update_mock: + team_doc = ensure_team_from_external_identity(optix_id=99, name="New Team") + + self.assertEqual(team_doc, updated) + patch_payload = update_mock.call_args.kwargs["patch"] + self.assertEqual(patch_payload["name"], "New Team") + self.assertIn("updatedAt", patch_payload) + + def test_ensure_team_from_external_identity_skips_write_when_team_name_is_unchanged(self): + team_id = ObjectId() + existing = {"_id": team_id, "optixId": 99, "name": "Team One"} + + with patch("repositories.teams_repository.find_team_by_optix_id", return_value=existing), patch( + "repositories.teams_repository.update_team_with_mailbox_sync" + ) as update_mock: + team_doc = ensure_team_from_external_identity(optix_id=99, name="Team One") + + self.assertEqual(team_doc, existing) + update_mock.assert_not_called() + + +if __name__ == "__main__": + unittest.main() diff --git a/backend/tests/unit/test_users_repository.py b/backend/tests/unit/test_users_repository.py new file mode 100644 index 0000000..72b205d --- /dev/null +++ b/backend/tests/unit/test_users_repository.py @@ -0,0 +1,135 @@ +import os +import unittest +from unittest.mock import patch + +from bson import ObjectId + +os.environ.setdefault("MONGO_URI", "mongodb://localhost:27017") + +from repositories.users_repository import upsert_user_from_external_identity + + +class UsersRepositoryExternalIdentityTests(unittest.TestCase): + def test_upsert_user_from_external_identity_creates_new_user_with_default_notification_prefs(self): + created_user = {"_id": ObjectId(), "optixId": 42, "fullname": "Member User"} + + with patch("repositories.users_repository.find_user_by_optix_id", return_value=None), patch( + "repositories.users_repository.create_user_with_mailbox", + return_value=created_user, + ) as create_mock: + user_doc, created = upsert_user_from_external_identity( + optix_id=42, + fullname="Member User", + email="member@example.com", + phone="+15550001111", + is_admin=False, + team_ids=[], + ) + + self.assertTrue(created) + self.assertEqual(user_doc, created_user) + created_payload = create_mock.call_args.kwargs["user_doc"] + self.assertEqual(created_payload["notifPrefs"], ["email"]) + + def test_upsert_user_from_external_identity_updates_existing_user_without_overwriting_notification_prefs(self): + user_id = ObjectId() + team_id = ObjectId() + existing = { + "_id": user_id, + "optixId": 42, + "fullname": "Old Name", + "email": "old@example.com", + "phone": None, + "isAdmin": False, + "teamIds": [], + "notifPrefs": ["text"], + } + updated = {**existing, "fullname": "New Name", "email": "new@example.com", "teamIds": [team_id]} + + with patch("repositories.users_repository.find_user_by_optix_id", return_value=existing), patch( + "repositories.users_repository.update_user_with_mailbox_sync", + return_value=updated, + ) as update_mock: + user_doc, created = upsert_user_from_external_identity( + optix_id=42, + fullname="New Name", + email="new@example.com", + phone=None, + is_admin=False, + team_ids=[team_id], + ) + + self.assertFalse(created) + self.assertEqual(user_doc, updated) + patch_payload = update_mock.call_args.kwargs["patch"] + self.assertEqual(patch_payload["fullname"], "New Name") + self.assertEqual(patch_payload["email"], "new@example.com") + self.assertEqual(patch_payload["teamIds"], [team_id]) + self.assertNotIn("notifPrefs", patch_payload) + + def test_upsert_user_from_external_identity_skips_write_when_optix_payload_is_unchanged(self): + user_id = ObjectId() + team_id = ObjectId() + existing = { + "_id": user_id, + "optixId": 42, + "fullname": "Member User", + "email": "member@example.com", + "phone": "+15550001111", + "isAdmin": True, + "teamIds": [team_id], + "notifPrefs": ["text"], + } + + with patch("repositories.users_repository.find_user_by_optix_id", return_value=existing), patch( + "repositories.users_repository.update_user_with_mailbox_sync" + ) as update_mock: + user_doc, created = upsert_user_from_external_identity( + optix_id=42, + fullname="Member User", + email="member@example.com", + phone="+15550001111", + is_admin=True, + team_ids=[team_id], + ) + + self.assertFalse(created) + self.assertEqual(user_doc, existing) + update_mock.assert_not_called() + + def test_upsert_user_from_external_identity_removes_sms_pref_when_optix_clears_phone(self): + user_id = ObjectId() + existing = { + "_id": user_id, + "optixId": 42, + "fullname": "Member User", + "email": "member@example.com", + "phone": "+15550001111", + "isAdmin": False, + "teamIds": [], + "notifPrefs": ["email", "text"], + } + updated = {**existing, "phone": None, "notifPrefs": ["email"]} + + with patch("repositories.users_repository.find_user_by_optix_id", return_value=existing), patch( + "repositories.users_repository.update_user_with_mailbox_sync", + return_value=updated, + ) as update_mock: + user_doc, created = upsert_user_from_external_identity( + optix_id=42, + fullname="Member User", + email="member@example.com", + phone="", + is_admin=False, + team_ids=[], + ) + + self.assertFalse(created) + self.assertEqual(user_doc, updated) + patch_payload = update_mock.call_args.kwargs["patch"] + self.assertIsNone(patch_payload["phone"]) + self.assertEqual(patch_payload["notifPrefs"], ["email"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/01-architecture/api/member-api-contract.md b/docs/01-architecture/api/member-api-contract.md index 95192bf..42fb6e9 100644 --- a/docs/01-architecture/api/member-api-contract.md +++ b/docs/01-architecture/api/member-api-contract.md @@ -15,6 +15,7 @@ This endpoint is the source of truth for: - Admin vs member routing - User identity in frontend state - Default notification preference state +- Whether SMS notifications can be enabled without another profile fetch ### Auth - Requires valid session cookie @@ -35,14 +36,17 @@ This endpoint is the source of truth for: "fullname": "Jane Doe", "isAdmin": false, "teamIds": [""], - "emailNotifications": true + "emailNotifications": true, + "smsNotifications": false, + "hasPhone": true } ``` ### Notes -* `emailNotifications` is the only public preference field for session hydration. +* Session hydration exposes effective notification booleans only: `emailNotifications`, `smsNotifications`, and `hasPhone`. * Internal storage (for example `notifPrefs`) is intentionally hidden from clients. +* Optix login/bootstrap may refresh Optix-owned identity fields before this endpoint is read, but it must not overwrite Avenu-managed notification preferences. * No sensitive fields exposed. --- @@ -109,7 +113,7 @@ Both are required. ### Purpose -Allow a member to toggle email notifications. +Allow a member to update notification preferences. This endpoint updates notification preferences for the authenticated user only. @@ -122,14 +126,18 @@ This endpoint updates notification preferences for the authenticated user only. ```json { - "emailNotifications": true + "emailNotifications": true, + "smsNotifications": false } ``` ### Backend Behavior -* Map `emailNotifications` to internal `notifPrefs` representation. +* Treat `emailNotifications` and `smsNotifications` as independently optional patch fields. +* Map public booleans to internal `notifPrefs` representation. * Do not expose enum or storage format to frontend. +* Reject SMS enablement when the effective phone number is missing. +* Auto-remove SMS preference when the effective phone number is missing. * Persist updated preferences. ### Response Shape @@ -137,7 +145,9 @@ This endpoint updates notification preferences for the authenticated user only. ```json { "id": "", - "emailNotifications": true + "emailNotifications": true, + "smsNotifications": false, + "hasPhone": true } ``` diff --git a/docs/01-architecture/data-model.md b/docs/01-architecture/data-model.md index 2b1a479..3df29cf 100644 --- a/docs/01-architecture/data-model.md +++ b/docs/01-architecture/data-model.md @@ -34,6 +34,7 @@ If `teams` field in users is non-empty: * Upsert each team first, keyed on `team_id` * Then upsert the user with team references +* Refresh existing team `name` and the corresponding team mailbox `displayName` only when the incoming Optix name changed This guarantees referential consistency during hydration. @@ -52,6 +53,30 @@ db.users.updateOne( ) ``` +### Ownership Boundaries + +Fields owned by Optix and refreshed during successful `/api/optix-token` login/bootstrap: + +* `users.fullname` +* `users.email` +* `users.phone` +* `users.isAdmin` +* `users.teamIds` +* `teams.name` +* team mailbox `displayName` + +Fields owned by Avenu and preserved across Optix login/bootstrap: + +* `users.notifPrefs` + +### Change Detection + +Hydration should compare incoming Optix values against stored MongoDB values before writing: + +* unchanged user identity payloads must not rewrite `users` or user mailbox records +* unchanged team names must not rewrite `teams` or team mailbox records +* first-time local creation still applies Avenu defaults such as initial notification preferences + --- ## Uniqueness Invariants @@ -215,4 +240,3 @@ Indexes: db.ocr_queue_items.createIndex({ jobId: 1, index: 1 }) db.ocr_queue_items.createIndex({ status: 1 }) ``` - diff --git a/docs/02-plans/plan-fix-optix-rehydration.md b/docs/02-plans/plan-fix-optix-rehydration.md new file mode 100644 index 0000000..8c9e05c --- /dev/null +++ b/docs/02-plans/plan-fix-optix-rehydration.md @@ -0,0 +1,55 @@ +# Open Questions +- None. + +# Task Checklist +## Phase 1 +- ☑ Preserve app-owned `notifPrefs` for existing users during Optix sync. +- ☑ Keep re-hydrating Optix-owned user fields (`fullname`, `email`, `phone`, `isAdmin`, `teamIds`) on each successful Optix login/bootstrap request to `/api/optix-token`. +- ☑ Apply change detection so unchanged Optix user data does not trigger redundant local writes. +- ☑ Add unit coverage for create-vs-update sync behavior and notification-preference ownership. + +## Phase 2 +- ☑ Refresh existing team names and team mailbox display names on each successful Optix login/bootstrap request to `/api/optix-token`. +- ☑ Apply change detection so unchanged Optix team/mailbox data does not trigger redundant local writes. +- ☑ Add regression coverage that Optix bootstrap still returns the correct create/update response and establishes the session. + +## Phase 3 +- ☑ Document the Optix login/bootstrap re-hydration rules and ownership boundaries after the sync change. +- ☑ Update any architecture or API docs that describe how Optix data populates local user/team records. + +## Phase 1: User Re-hydration Ownership Boundaries +Affected files and changes +- `backend/services/identity_sync_service.py`: stop treating Avenu-managed notification preferences as Optix-owned data during login/bootstrap; pass only Optix-owned user fields into the persistence layer and keep the create/update flow explicit so existing users are re-hydrated without resetting app preferences. +- `backend/repositories/users_repository.py`: split external-identity insert defaults from external-identity update behavior so new users still receive the current default `notifPrefs`, while existing users only update Optix-owned fields (`fullname`, `email`, `phone`, `isAdmin`, `teamIds`) and retain stored Avenu preferences; build the update patch from actual field diffs so unchanged Optix payloads do not rewrite the user or mailbox rows. +- `backend/tests/unit/test_identity_sync_service.py`: add assertions that sync for existing users does not overwrite `notifPrefs`, still forwards updated phone/team data, and still reports `created=False` for update flows. + +Unit tests +- `backend/tests/unit/test_identity_sync_service.py` + - `test_sync_optix_identity_creates_new_user_with_default_notification_prefs` + - `test_sync_optix_identity_updates_existing_user_without_overwriting_notification_prefs` + - `test_sync_optix_identity_forwards_updated_phone_and_team_membership_for_existing_user` + +## Phase 2: Other Optix-Owned Record Re-hydration +Affected files and changes +- `backend/repositories/teams_repository.py`: update `ensure_team_from_external_identity(...)` so existing teams refresh their name and mailbox display name when Optix returns different values, while unchanged payloads return without redundant writes. +- `backend/services/identity_sync_service.py`: rely on the refreshed team ensure path so every bootstrap reconciles both the user record and any referenced team records from the latest Optix payload. +- `backend/tests/unit/test_identity_sync_service.py`: extend sync coverage to assert team refresh behavior when Optix returns a renamed team. +- `backend/tests/unit/test_identity_controller.py`: keep controller-level regression coverage focused on the bootstrap contract by asserting the route still returns `200`/`201` correctly and establishes the session after the sync changes. + +Unit tests +- `backend/tests/unit/test_identity_sync_service.py` + - `test_sync_optix_identity_refreshes_existing_team_name_when_optix_payload_changes` + - `test_sync_optix_identity_skips_team_write_when_optix_team_name_is_unchanged` + - `test_sync_optix_identity_raises_on_optix_failure_and_does_not_write_local_state` +- `backend/tests/unit/test_identity_controller.py` + - `test_optix_token_route_returns_updated_200` + - `test_optix_token_route_returns_created_201` + +## Phase 3: Docs Alignment +Affected files and changes +- `docs/01-architecture/api/member-api-contract.md`: update any session/bootstrap contract notes that imply Optix login overwrites Avenu-managed notification preferences or omit the re-hydration behavior for existing users. +- `docs/01-architecture/data-model.md`: clarify ownership boundaries between Optix-owned identity fields and Avenu-owned user preferences, plus note that team names/mailbox display names are refreshed from Optix during login only when changed. +- `README.md`: if the current login/bootstrap description mentions Optix sync, align it with the new change-detection and ownership behavior so local onboarding docs match the implementation. + +Unit tests +- None.