feat: add Notification Center with filtering, mark-read, and delete#966
Open
Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Open
feat: add Notification Center with filtering, mark-read, and delete#966Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Conversation
Contributor
WalkthroughAdds a Notification Center: new template and views to list, filter, mark-read, and delete notifications; URL routes and AJAX endpoints; a context processor and settings registration for unread counts; base template updates to surface the bell/unread badge; and tests covering behavior and permissions. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant Views
participant Model as NotificationModel
participant DB
User->>Browser: GET /account/notifications/
Browser->>Views: HTTP GET (request)
Views->>DB: Query notifications (filter, paginate)
DB-->>Model: Notification rows
Model-->>Views: objects
Views-->>Browser: Render notification_center.html (notifications, counts)
User->>Browser: POST /account/notifications/{id}/read/ (AJAX)
Browser->>Views: mark read request
Views->>DB: Update notification.read = True
DB-->>Views: Success
Views-->>Browser: JSON {status: "ok"}
User->>Browser: POST /account/notifications/{id}/delete/ (AJAX)
Browser->>Views: delete request
Views->>DB: Delete notification
DB-->>Views: Success
Views-->>Browser: JSON {status: "ok"}
User->>Browser: POST /account/notifications/mark-all-read/
Browser->>Views: mark-all request
Views->>DB: Update all user notifications read=True
DB-->>Views: Success
Views-->>Browser: Redirect/JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_trackers.py`:
- Around line 60-62: The Notification import is currently placed between test
classes; move the "from web.models import Notification" line into the top import
block alongside the other model imports (i.e., the file's import section) and
remove the in-body import between test classes so Notification is imported only
once and all imports are grouped consistently.
- Around line 152-154: The test name test_unread_count_context_processor is
misleading because it asserts response.context["unread_count"] produced by the
notification_center view; either rename the test to
test_unread_count_in_view_context (update the function name and any references)
to reflect it checks the view-provided unread_count, or change the assertion to
exercise the actual context processor variable unread_notifications_count (e.g.,
call the context processor or render a template with RequestContext and assert
unread_notifications_count is present) so the test name matches what is being
tested; update only the test function test_unread_count_context_processor (or
rename to test_unread_count_in_view_context) and its assertion accordingly.
In `@web/context_processors.py`:
- Around line 26-32: Add a brief docstring to the context processor function
unread_notifications describing its purpose and return value: explain that when
request.user.is_authenticated it returns a dict with
"unread_notifications_count" equal to the number of unread Notification objects
for that user, otherwise returns the dict with count 0; place the docstring
immediately under the def unread_notifications(request) line following the
project's docstring style.
In `@web/templates/account/notification_center.html`:
- Around line 166-196: The fetch call in function markAsRead currently hardcodes
the /en/ locale prefix; change it to build the URL dynamically using a
template-provided value (e.g., read a data attribute or a JS constant rendered
by Django) instead of "/en/". Specifically, add a data attribute on the
notifications container like data-mark-read-url with a placeholder ID (or render
a JS NOTIFICATION_URLS.markRead function) in the template, then update
markAsRead to read that value (e.g.,
document.getElementById('notification-container').dataset.markReadUrl) and
replace the placeholder with notificationId before calling fetch; keep the rest
of the response handling (element id notification-{id}, badge update, aria-label
selectors) unchanged.
In `@web/templates/base.html`:
- Around line 497-500: The "Notifications" menu item label no longer matches its
target URL name notification_preferences; either restore the original label
"Notification Preferences" in the anchor text to match the existing URL, or
change the href to point to the notification_center URL (and/or add a separate
link to notification_center) so that the label "Notifications" leads to the
notification center; update the anchor text or the URL name in the template
snippet containing the anchor with class "block px-4 py-2 ..." accordingly.
In `@web/views.py`:
- Around line 6980-7030: Add explicit type hints to the four view functions:
annotate the request parameter as django.http.HttpRequest for
notification_center, mark_notification_read, mark_all_notifications_read, and
delete_notification; annotate notification_id parameters as int for
mark_notification_read and delete_notification; set return types to
django.http.HttpResponse for notification_center, django.http.JsonResponse for
mark_notification_read and delete_notification, and
django.http.HttpResponseRedirect (or django.http.HttpResponse) for
mark_all_notifications_read; also add the necessary imports (HttpRequest,
HttpResponse, JsonResponse, HttpResponseRedirect) at the top of the module.
- Around line 6980-7003: The view notification_center returns the entire
Notification queryset; change it to use Django's Paginator: import
Paginator/EmptyPage/PageNotAnInteger, build a Paginator from the ordered
notifications queryset (e.g., per_page=20), read page number from
request.GET.get("page", "1"), and replace the raw queryset in the context with
the page object (e.g., page_obj or notifications_page) so templates render only
the current page; also handle invalid/empty page numbers by falling back to page
1 or the last page and keep unread_count/total_count as-is so pagination
metadata stays available to the template.
- Around line 23-25: Replace the internal import of
send_verification_email_for_user with the public allauth API: import
send_email_confirmation from allauth.account.utils and update any usages
(previously calling send_email_confirmation alias) to use the public signature
send_email_confirmation(request, user, signup=False); specifically change the
import statement and adjust the call sites that pass only a user to also pass
the current request and signup=False to match the documented
send_email_confirmation(request, user, signup=False) function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
tests/test_trackers.pyweb/context_processors.pyweb/settings.pyweb/templates/account/notification_center.htmlweb/templates/base.htmlweb/urls.pyweb/views.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/templates/base.html (1)
541-864: 🧹 Nitpick | 🔵 TrivialConsider adding notifications link to mobile menu.
The desktop navigation has the notification bell icon and dropdown menu entry, but the mobile menu section doesn't appear to include a direct link to the notification center. Mobile users may have difficulty accessing their notifications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/base.html` around lines 541 - 864, Mobile menu (element id "mobile-menu") is missing a link to the notifications center; add a Notifications entry inside the mobile menu content (near the User Account Section or alongside the Cart link) so mobile users can access it. Create a link similar to the Cart entry using the notifications URL (e.g., {% url 'notifications' %}), include a bell icon (fa-bell) and, if desired, reuse the same badge logic used for Cart (request.user notifications count or session helper) to show an unread count; ensure the new item follows the same classes/structure as existing mobile items and appears before the dark mode toggle or within the authenticated "Your Account" block for consistency.
♻️ Duplicate comments (1)
web/views.py (1)
23-25:⚠️ Potential issue | 🟠 MajorReplace internal allauth import with the public API
allauth.account.internal.*is a private/unstable module. This can break on allauth upgrades; use the public helper import instead and keep existing call sites (Line 1337, Line 1367) unchanged.♻️ Proposed fix
-from allauth.account.internal.flows.email_verification import ( - send_verification_email_for_user as send_email_confirmation, -) +from allauth.account.utils import send_email_confirmation#!/bin/bash set -euo pipefail # Verify current allauth import source and call sites rg -n "allauth\.account\.(internal|utils)" web/views.py -C2 rg -n "send_email_confirmation\(" web/views.py -C2 # Check declared allauth dependency version (if present) fd -a "pyproject.toml|requirements.*\\.txt|setup.cfg|setup.py|Pipfile" | xargs -I{} sh -c 'echo "== {} =="; rg -n "allauth|django-allauth" "{}" || true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 23 - 25, Replace the private internal import with the public allauth helper: stop importing send_verification_email_for_user from allauth.account.internal.flows.email_verification and instead import the public send_email_confirmation from allauth.account.utils (keep the existing local name send_email_confirmation and do not change the call sites where send_email_confirmation(...) is used). Update the import line that currently references send_verification_email_for_user to import send_email_confirmation from allauth.account.utils so existing calls (send_email_confirmation at the previous call sites) continue to work with the stable public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_trackers.py`:
- Around line 63-152: Add three focused tests inside NotificationCenterTests:
one that posts to mark_notification_read with a non-existent id (use
reverse("mark_notification_read", args=[99999]) or similar) and asserts a 404,
one that GETs reverse("notification_center") with an invalid filter parameter
(e.g., "?filter=invalid") and asserts the view returns 200 and falls back to the
default behavior (contains expected notification(s) and correct unread_count),
and one that exercises an AJAX endpoint (e.g., post to
reverse("mark_notification_read", args=[self.notification.id]) with
HTTP_X_REQUESTED_WITH='XMLHttpRequest') and asserts the response status and JSON
body shape/fields (use response.json() and check keys like "success" or "read").
Ensure tests reference NotificationCenterTests methods and the reverse names
("notification_center", "mark_notification_read") so they locate the same views.
In `@web/context_processors.py`:
- Around line 26-33: Add type hints to the context processor function
unread_notifications: annotate the request parameter as django.http.HttpRequest
(or HttpRequest from django.http) and the return type as dict[str, int] (or
typing.Dict[str, int] if supporting older Py versions); update the function
signature in web/context_processors.py and add any necessary imports
(HttpRequest or typing.Dict) so static checkers and IDEs can validate usages.
In `@web/templates/account/notification_center.html`:
- Around line 162-173: The template currently loops over
page_obj.paginator.page_range and can render every page link; update the
pagination block to use a truncated/elided range instead (use Django's
get_elided_page_range on page_obj.paginator or a custom helper) so the UI shows
first/last pages, the current window, and ellipses for the middle; modify the
loop that references page_obj.paginator.page_range to iterate over the elided
range (e.g., call page_obj.paginator.get_elided_page_range(page_obj.number,
on_each_side=1, on_ends=1) or a similar helper) and keep the existing
link/active-state logic that references page_obj.number and filter_type.
- Line 91: The h3 element currently always includes "font-semibold" and
conditionally "font-bold" when notification.read is false, causing redundant
overrides; update the class logic in the h3 tag (the one using {% if not
notification.read %}) so the base weight isn't overridden—either remove
"font-semibold" and rely solely on the conditional "font-bold" for unread items,
or make the weight fully conditional (e.g., choose "font-bold" when not read and
"font-normal" when read) so only one font weight class applies at a time.
- Around line 204-256: Add robust error handling to both markAsRead and
deleteNotification: check response.ok before calling response.json and handle
non-2xx responses (log error and show user feedback), add a .catch(...) to catch
network/parse errors and display/log an error message, and ensure UI updates
(badge decrement and element removal in markAsRead/deleteNotification) only
occur on successful responses; reference the functions markAsRead and
deleteNotification and the DOM IDs like notification-{notificationId} and
notification-badge when implementing these checks and user-facing error
messaging.
- Around line 246-256: Replace the inline style animation with toggling Tailwind
classes on the element found via
document.getElementById(`notification-${notificationId}`): ensure the element
initially has the visible classes (e.g., 'opacity-100', 'translate-x-0',
'transition-opacity', 'transition-transform', 'duration-300'), then when
data.success is true use el.classList.remove('opacity-100','translate-x-0') and
el.classList.add('opacity-0','translate-x-5') and keep the existing
setTimeout(() => el.remove(), 300) to remove the node after the transition
completes; update any template markup to include the initial Tailwind classes if
missing.
---
Outside diff comments:
In `@web/templates/base.html`:
- Around line 541-864: Mobile menu (element id "mobile-menu") is missing a link
to the notifications center; add a Notifications entry inside the mobile menu
content (near the User Account Section or alongside the Cart link) so mobile
users can access it. Create a link similar to the Cart entry using the
notifications URL (e.g., {% url 'notifications' %}), include a bell icon
(fa-bell) and, if desired, reuse the same badge logic used for Cart
(request.user notifications count or session helper) to show an unread count;
ensure the new item follows the same classes/structure as existing mobile items
and appears before the dark mode toggle or within the authenticated "Your
Account" block for consistency.
---
Duplicate comments:
In `@web/views.py`:
- Around line 23-25: Replace the private internal import with the public allauth
helper: stop importing send_verification_email_for_user from
allauth.account.internal.flows.email_verification and instead import the public
send_email_confirmation from allauth.account.utils (keep the existing local name
send_email_confirmation and do not change the call sites where
send_email_confirmation(...) is used). Update the import line that currently
references send_verification_email_for_user to import send_email_confirmation
from allauth.account.utils so existing calls (send_email_confirmation at the
previous call sites) continue to work with the stable public API.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
tests/test_trackers.pyweb/context_processors.pyweb/templates/account/notification_center.htmlweb/templates/base.htmlweb/views.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/views.py (1)
23-25:⚠️ Potential issue | 🟠 MajorUse the public allauth API instead of
allauth.account.internal.*Line 23 imports an internal django-allauth flow. Internal modules are not stable API and can break on upgrades; use
allauth.account.utils.send_email_confirmationinstead (this also covers the calls at Line 1337 and Line 1367).♻️ Proposed fix
-from allauth.account.internal.flows.email_verification import ( - send_verification_email_for_user as send_email_confirmation, -) +from allauth.account.utils import send_email_confirmation @@ - send_email_confirmation(request, user) + send_email_confirmation(request, user, signup=False) @@ - send_email_confirmation(request, user) + send_email_confirmation(request, user, signup=False)#!/bin/bash # Verify current allauth version and all internal-flow usages in repository. rg -n "django-allauth|allauth" pyproject.toml rg -n "allauth\\.account\\.internal\\.flows\\.email_verification|send_verification_email_for_user|send_email_confirmation\\(" -C2For django-allauth 65.3.1, is `allauth.account.internal.flows.email_verification.send_verification_email_for_user` a supported public API, and what is the recommended public function for sending email confirmations?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 23 - 25, Replace the unstable internal import and usages of send_verification_email_for_user with the public API allauth.account.utils.send_email_confirmation: update the import in web/views.py to import send_email_confirmation from allauth.account.utils, then change any calls that reference send_verification_email_for_user (and existing send_email_confirmation alias) at the sites mentioned (e.g., the places currently calling send_email_confirmation around the previous Line 1337 and Line 1367) to call the imported allauth.account.utils.send_email_confirmation with the same arguments so the public, supported function is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/views.py`:
- Around line 23-25: Replace the unstable internal import and usages of
send_verification_email_for_user with the public API
allauth.account.utils.send_email_confirmation: update the import in web/views.py
to import send_email_confirmation from allauth.account.utils, then change any
calls that reference send_verification_email_for_user (and existing
send_email_confirmation alias) at the sites mentioned (e.g., the places
currently calling send_email_confirmation around the previous Line 1337 and Line
1367) to call the imported allauth.account.utils.send_email_confirmation with
the same arguments so the public, supported function is used.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
tests/test_trackers.pyweb/context_processors.pyweb/templates/account/notification_center.htmlweb/templates/base.htmlweb/views.py
Author
|
@A1L13N could you please review it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a Notification Center UI. The
Notificationmodel existed but users had no way to view their notifications. Now they can see, filter, mark as read, and delete notifications from a dedicated page. The navbar bell icon shows an unread count badge. Includes 9 tests and dark mode support.Related issues
Fixes #965
Checklist
Summary by CodeRabbit
New Features
Tests