Feat/211 add notificaitons app#225
Conversation
|
Wcale nie vibecodowane |
There was a problem hiding this comment.
Pull request overview
Introduces a new notifications Django app providing a Notification model and a read-only-ish DRF endpoint (list/retrieve + PATCH to mark read, plus a mark-all-read action). The app is wired into INSTALLED_APPS and the root URL conf so other apps can create notifications and the frontend can fetch them.
Changes:
- New
notificationsapp:Notificationmodel (UUID PK,title/content/is_read/notification_type/timestamps/user FK),NotificationTypetext choices (email,in_app), initial migration. NotificationViewSet(auth-required, user-scoped queryset, DjangoFilterBackend onis_read/created_at/notification_type,mark-all-read@action),NotificationSerializer, router URL registration.- ORM-level tests for create/retrieve/update/delete and registration in
INSTALLED_APPS+ rooturls.py.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| testownik_core/settings.py | Registers NotificationsConfig in INSTALLED_APPS. |
| testownik_core/urls.py | Mounts notifications.urls under /api/. |
| notifications/apps.py | Standard AppConfig. |
| notifications/models.py | New Notification model + NotificationType choices. |
| notifications/migrations/0001_initial.py | Initial schema for the model. |
| notifications/serializers.py | NotificationSerializer (omits notification_type/created_at despite them being filterable). |
| notifications/views.py | ModelViewSet restricted via http_method_names; user-scoped queryset; mark-all-read action. |
| notifications/urls.py | Registers viewset on DefaultRouter. |
| notifications/admin.py | Empty placeholder file. |
| notifications/tests/test_notifications.py | ORM-only tests; no API-level tests; tests dir missing __init__.py. |
Comments suppressed due to low confidence (8)
notifications/tests/test_notifications.py:1
notifications/tests/is missing an__init__.pyfile. All other test packages in the codebase (e.g.quizzes/tests/__init__.py,users/tests/__init__.py) include one, and without it Django's default test discovery will not find these tests when runningpython manage.py test. As a result the new test suite will silently be skipped in CI.
from django.contrib.auth import get_user_model
notifications/tests/test_notifications.py:43
- The tests use
notification_type="info"andnotification_type="warning", butNotificationTypeonly definesEMAIL = "email"andIN_APP = "in_app". These values are not valid choices for the field. SinceModel.objects.create()doesn't callfull_clean(), the tests will pass with invalid data, masking the fact that the test fixtures do not represent realistic data. UseNotificationType.EMAIL/NotificationType.IN_APPconstants instead.
notification = Notification.objects.create(
user=self.user, content="Test notification message", notification_type="info"
)
self.assertEqual(notification.user, self.user)
self.assertEqual(notification.content, "Test notification message")
self.assertEqual(notification.notification_type, "info")
self.assertFalse(notification.is_read)
class NotificationRetrievalTestCase(TestCase):
"""Test notification retrieval."""
def setUp(self):
"""Setup test users and multiple notifications."""
User = get_user_model()
self.user1 = User.objects.create_user(email="[email protected]", password="pass123")
self.user2 = User.objects.create_user(email="[email protected]", password="pass123")
# Create notifications for user1
Notification.objects.create(user=self.user1, content="Notification 1", notification_type="info")
Notification.objects.create(
user=self.user1, content="Notification 2", notification_type="warning", is_read=True
)
# Create notification for user2
Notification.objects.create(user=self.user2, content="User 2 Notification", notification_type="info")
notifications/tests/test_notifications.py:19
- All tests instantiate
Notificationwithout providing atitle, buttitle = models.CharField(max_length=128)is required (noblank=True/null=True/default). The tests pass only because Django CharField silently coerces missing values to an empty string at the ORM level, not because empty titles are intentional. This hides the fact that there is no model-level enforcement that notifications have a title, and the tests don't actually validate thetitlefield at all. Consider either makingtitleoptional explicitly or providing titles in test fixtures and asserting on them.
notification = Notification.objects.create(
user=self.user, content="Test notification message", notification_type="info"
)
notifications/views.py:34
- The viewset is declared as
ModelViewSetwithhttp_method_namesrestricted to GET/PATCH/HEAD/OPTIONS to fake a read+mark-as-read API. This is fragile and surfaces unintended routes (e.g. DRF will still registerput/destroyhandlers; OpenAPI generation via drf-spectacular and the browsable API can become inconsistent). The conventional approach in this codebase is to composeReadOnlyModelViewSetwithmixins.UpdateModelMixin, or to use explicitgenerics.*views. Restricting viahttp_method_namesis a workaround rather than an API design.
class NotificationViewSet(viewsets.ModelViewSet):
"""
ViewSet for managing user notifications.
This ViewSet allows authenticated frontend users to read and manage their notifications.
Notifications are created by other apps and this endpoint provides read-only access
(with the ability to mark them as read).
Available actions:
- GET: Retrieve user's notifications
- PATCH: Update notification status (mark as read)
- mark-all-read: Custom action to mark all unread notifications as read
"""
# We override get_queryset to filter by user, so we start with an empty queryset here.
queryset = Notification.objects.none()
serializer_class = NotificationSerializer
permission_classes = [IsAuthenticated]
filter_backends = [DjangoFilterBackend]
filterset_fields = ["is_read", "created_at", "notification_type"]
# Only allow read and partial update
http_method_names = ["get", "patch", "head", "options"]
notifications/views.py:49
mark_all_readqueriesNotification.objects.filter(user=request.user, ...)directly instead of going throughself.get_queryset(). This duplicates the user-scoping logic and would silently bypass any future tightening ofget_queryset(e.g. soft-delete filtering, archived filtering). Preferself.get_queryset().filter(is_read=False).update(is_read=True)to keep a single source of truth for what notifications belong to the current user.
@action(detail=False, methods=["patch"], url_path="mark-all-read")
def mark_all_read(self, request):
updated_count = Notification.objects.filter(user=request.user, is_read=False).update(is_read=True)
return Response(
{
"message": f"{updated_count} notifications marked as read",
},
status=status.HTTP_200_OK,
)
notifications/models.py:42
- No default ordering is defined on the model (no
class Meta: ordering = [...]) and the viewset does not specifyordering/OrderingFilter. As a result the list endpoint will return notifications in an undefined/database-dependent order, which is a poor UX for a notifications feed. Add a default ordering such as["-created_at"]toNotification.Meta.
class Notification(models.Model):
"""
Represents a notification entity that can be sent to users.
Notifications can be sent via email or displayed in-app. Each notification
is associated with a user and tracks whether it has been read.
"""
id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
title = models.CharField(max_length=128)
content = models.TextField()
is_read = models.BooleanField(default=False)
notification_type = models.CharField(max_length=20, choices=NotificationType.choices)
created_at = models.DateTimeField(auto_now_add=True)
updated_at = models.DateTimeField(auto_now=True)
user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="notifications")
def __str__(self):
return f"Notification(id={self.id}, title={self.title}, user={self.user_id})"
notifications/serializers.py:10
useris exposed in serializerfieldsand marked read-only, but sinceget_querysetalready filters by the authenticated user, returning theuserUUID on every notification is redundant and leaks an internal identifier. Consider removinguserfrom the serialized output.
fields = ["id", "title", "content", "is_read", "user"]
read_only_fields = ["id", "title", "content", "user"]
notifications/views.py:49
- Other apps in the codebase decorate viewsets and custom
@actions with@extend_schema(drf-spectacular) so OpenAPI docs document the response shape and tags.NotificationViewSetand themark_all_readaction have no schema annotations, so the generated docs will be minimal/wrong (e.g.mark_all_readwill show theNotificationSerializeras request/response body when it actually returns{"message": "..."}). See e.g. usages inquizzes/views.pyfor the established pattern.
class NotificationViewSet(viewsets.ModelViewSet):
"""
ViewSet for managing user notifications.
This ViewSet allows authenticated frontend users to read and manage their notifications.
Notifications are created by other apps and this endpoint provides read-only access
(with the ability to mark them as read).
Available actions:
- GET: Retrieve user's notifications
- PATCH: Update notification status (mark as read)
- mark-all-read: Custom action to mark all unread notifications as read
"""
# We override get_queryset to filter by user, so we start with an empty queryset here.
queryset = Notification.objects.none()
serializer_class = NotificationSerializer
permission_classes = [IsAuthenticated]
filter_backends = [DjangoFilterBackend]
filterset_fields = ["is_read", "created_at", "notification_type"]
# Only allow read and partial update
http_method_names = ["get", "patch", "head", "options"]
def get_queryset(self):
user = self.request.user
return Notification.objects.filter(user=user)
@action(detail=False, methods=["patch"], url_path="mark-all-read")
def mark_all_read(self, request):
updated_count = Notification.objects.filter(user=request.user, is_read=False).update(is_read=True)
return Response(
{
"message": f"{updated_count} notifications marked as read",
},
status=status.HTTP_200_OK,
)
Antoni-Czaplicki
left a comment
There was a problem hiding this comment.
we napisz jeszcze ai niech zrobi żeby wszystkie powiadomienia szły przez tą aplikacje i się zapisywały a dopiero ta apka ma wysylac maile i push w przyszlosci
|
haj tam @WiktorGruszczynski ? |
|
Dodałem jedną funkcje od powiadomień. Teraz w testownikj zamiasy uzywac |
|
dobra to tak na przyszlosc daj resolve na komentare i rerequest review plss |
Antoni-Czaplicki
left a comment
There was a problem hiding this comment.
Kierunek jest ok, admin i wspólny send_notification są na plus. Zostawiłbym jeszcze changes requested, bo są tu dwie rzeczy do poprawienia przed mergem. Te stare wątki o testach API i polach w serializerze też dalej są sensowne.
Antoni-Czaplicki
left a comment
There was a problem hiding this comment.
Jeszcze z pozostałych rzeczy z review: stare wątki o testach API i polach w serializerze dalej są aktualne, więc odpisałem tam zamiast zakładać duble. Dodałem też jeden drobniejszy komentarz do mark-all-read.
|
@Antoni-Czaplicki są te zmiany ktore chciales |
00da75d to
47a43fd
Compare
1a04a83 to
92657e3
Compare
Powiadomienia
Mamy endpoint tylko do odczytu powiadomień i do oznaczenia ich jako przeczytane.
Przeznaczenie
Notificaitonjest takie, że tworzymy je w pozostałych aplikacjach i służy ono troche jak takie narzędzie dodatkowe.Testy też są :)