Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-pass notifications API endpoints to the backend so clients can list notifications, fetch unread counts, and mark notifications as read.
Changes:
- Introduces
/v1/notifications,/v1/notifications/unread-count, and/v1/notifications/{notification_id}/readendpoints. - Adds controller tests and a test data generator helper for notifications.
- Updates the generated OpenAPI schema and registers the new router.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/tests/data_generator.py | Adds gen_notification() to generate Notification records for tests. |
| backend/tests/controllers/notifications/test_notifications.py | Adds test coverage for listing, unread count, and marking notifications as read. |
| backend/test_observer/controllers/router.py | Registers the notifications router under /v1/notifications. |
| backend/test_observer/controllers/notifications/notifications.py | Implements notification listing, unread-count, and mark-as-read endpoints. |
| backend/test_observer/controllers/notifications/init.py | Exposes the notifications router. |
| backend/schemata/openapi.json | Updates OpenAPI paths/schemas for the new notification endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
backend/test_observer/controllers/notifications/notifications.py
Outdated
Show resolved
Hide resolved
backend/test_observer/controllers/notifications/notifications.py
Outdated
Show resolved
Hide resolved
backend/test_observer/controllers/notifications/notifications.py
Outdated
Show resolved
Hide resolved
rpbritton
left a comment
There was a problem hiding this comment.
Overall I think this looks pretty good! A couple comments though.
backend/test_observer/controllers/notifications/notifications.py
Outdated
Show resolved
Hide resolved
backend/test_observer/controllers/notifications/notifications.py
Outdated
Show resolved
Hide resolved
rpbritton
left a comment
There was a problem hiding this comment.
Another comment, what about prefixing these with:
GET /v1/users/{id}/notifications
GET /v1/users/me/notifications
That would make expand the general use case of these endpoints
| router.include_router(teams.router, prefix="/v1/teams") | ||
| router.include_router(permissions.router, prefix="/v1/permissions") | ||
| router.include_router(applications.router, prefix="/v1/applications") | ||
| router.include_router(notifications.router, prefix="/v1/users/me/notifications") |
There was a problem hiding this comment.
That's a incomplete to only hardcode me. If you go this route I think you should at least file a ticket to make the endpoint /v1/users/{id}/notifications, with an option for me.
Description
Added API functionality related to notifications
Resolved issues
Part of TO-291
Web service API changes
Added new endpoints for editing notifications