Notification system #3216
Notification system #3216
Conversation
|
I have added the command to the |
|
Yes notifications for all of those will be good. Thanks! |
Head branch was pushed to by a user without write access
|
Fixed all the issues, please verify! |
|
Also added the checkin_reminder_notification command in the run_daily schedule |
|
@DonnieBLT Can this please be merged since it frequently gets merge conflicts due to the model addition |
|
I merged the conflict, hopefully that was right. |
|
Thanks a lot for that! I'll make sure to finish this up as soon as possible |
SahilDhillon21
force-pushed
the
notification-prompts
branch
from
1291766 to
e84e4e6
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
|
Does this look fine @DonnieBLT ? Since users are already connected to organizations, there was no need to connect the DailyStatusReport directly to orgs as we could access it from report -> user -> send notif if user's org has checkins enabled |
|
@coderabbitai is there a new pr for this? Can we close this one? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to check if there's a new PR that replaces this one before answering. 🏁 Script executed: Length of output: 631 @DonnieBLT Based on my investigation, PR #3216 "Notification system" is still open and appears to be the current PR for implementing the notification system. I don't see any newer PRs that specifically replace this functionality. The only other open PR mentioning notifications (#3809) is about Bounty Bot enhancements and Slack integration, which seems to be a different feature. From the comment chain, it appears SahilDhillon21 has been working on addressing your feedback and was waiting for your final approval. Unless you're aware of another PR that specifically replaces this functionality, I would recommend keeping this PR open and reviewing the current implementation to determine if it's ready to be merged. |
|
Oh @SahilDhillon21 I see this is current! Will review |
WalkthroughThis pull request integrates a notification system into the application. It introduces new URL endpoints and view functions to fetch, mark as read, and delete notifications, along with corresponding JavaScript functionality in the header for real-time user interaction. Additionally, it creates a new Notification model (with its database migration), extends the Organization model, and registers Notification in the admin interface. New management commands and Django signal receivers have been added to generate notifications—whether from daily check-ins or various user events—ensuring that notifications are created and managed throughout the system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Server as Django View
participant DB as Database
User->>Browser: Clicks notification button
Browser->>Server: GET /notifications/fetch/
Server->>DB: Query unread notifications
DB-->>Server: Return notifications list
Server-->>Browser: JSON response with notifications
Browser->>User: Renders notification dropdown
User->>Browser: Click delete on a notification
Browser->>Server: DELETE /notifications/delete_notification/<id>
Server->>DB: Mark notification as deleted
DB-->>Server: Confirmation
Server-->>Browser: Success response
Browser->>User: Update notification list
sequenceDiagram
participant CLI
participant RunDailyCmd as run_daily Command
participant DailyCheck as daily_checkin_reminder Command
participant DB as Database
CLI->>RunDailyCmd: Execute run_daily command
RunDailyCmd->>DailyCheck: Call daily_checkin_reminder (within try block)
DailyCheck->>DB: Query user profiles with check-ins enabled
DB-->>DailyCheck: Return eligible user list
DailyCheck->>DB: Bulk create Notification records
DB-->>DailyCheck: Confirmation
DailyCheck-->>RunDailyCmd: Return success message
RunDailyCmd->>CLI: Print final status
sequenceDiagram
participant Event as Trigger (e.g., Kudos/Post/etc.)
participant SignalReceiver as Django Signal Handler
participant DB as Database
Event->>SignalReceiver: Emit Django signal on event creation
SignalReceiver->>DB: Create a Notification record
DB-->>SignalReceiver: Notification saved
SignalReceiver-->>Event: (Notification available for later fetch)
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai please review and approve if it looks good |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
website/management/commands/daily_checkin_reminder.py (1)
6-23: Command implementation looks good with a few potential improvements.The command follows Django's management command pattern and correctly creates notification objects for users who belong to teams with check-ins enabled.
Some suggestions to consider:
- Add validation to handle the case when
userprofiles_with_checkinsis empty- Consider adding a timestamp check to prevent duplicate reminders on the same day
- Use Django's
reverse()function for generating the URL instead of hardcoding "/add-sizzle-checkin/"- Consider checking if users already have pending reminders before creating new ones
def handle(self, *args, **options): userprofiles_with_checkins = UserProfile.objects.filter(team__check_ins_enabled=True) + if not userprofiles_with_checkins.exists(): + self.stdout.write(self.style.WARNING("No users found with check-ins enabled.")) + return + notifications = [ Notification( user=userprofile.user, message=f"This is a reminder to add your daily check-in for {userprofile.team.name}", notification_type="reminder", - link="/add-sizzle-checkin/", + link=reverse("add_sizzle_checkin"), ) for userprofile in userprofiles_with_checkins ] Notification.objects.bulk_create(notifications) self.stdout.write( self.style.SUCCESS(f"Sent check-in reminder notifications to {len(userprofiles_with_checkins)} users.") )blt/urls.py (1)
638-640: URL patterns for notification endpoints look good with minor consistency issue.The notification endpoints are properly configured. For consistency with other URL patterns in this file, consider adding a trailing slash to the
mark_all_readendpoint.path("notifications/fetch/", fetch_notifications, name="fetch_notifications"), -path("notifications/mark_all_read", mark_as_read, name="mark_all_read"), +path("notifications/mark_all_read/", mark_as_read, name="mark_all_read"), path("notifications/delete_notification/<int:notification_id>", delete_notification, name="delete_notification"),website/models.py (1)
2304-2329: Notification model implementation is well-structuredThe Notification model has all the necessary fields to support the notification system requirements, with appropriate field types and constraints. The soft delete implementation is a good practice for preserving data while allowing users to "delete" notifications from their view.
One suggestion for improvement:
Consider adding an index on the
userandis_deletedfields since these will be frequently queried together:class Meta: ordering = ["is_read", "-created_at"] + indexes = [ + models.Index(fields=["user", "is_deleted"], name="notif_user_deleted_idx"), + ]website/views/user.py (3)
1124-1141: Consider implementing pagination for notificationsThe
fetch_notificationsfunction returns all notifications for a user in a single JSON response. For users with many notifications, this could lead to performance issues.Consider implementing pagination to limit the number of notifications fetched at once:
@login_required def fetch_notifications(request): + page = int(request.GET.get('page', 1)) + page_size = int(request.GET.get('page_size', 10)) + + start = (page - 1) * page_size + end = page * page_size + notifications = Notification.objects.filter(user=request.user, is_deleted=False).order_by("is_read", "-created_at") + total_count = notifications.count() + notifications = notifications[start:end] notifications_data = [ { "id": notification.id, "message": notification.message, "created_at": notification.created_at, "is_read": notification.is_read, "notification_type": notification.notification_type, "link": notification.link, } for notification in notifications ] - return JsonResponse({"notifications": notifications_data}, safe=False) + return JsonResponse({ + "notifications": notifications_data, + "pagination": { + "total": total_count, + "page": page, + "page_size": page_size, + "total_pages": (total_count + page_size - 1) // page_size + } + }, safe=False)
1143-1161: Simplify mark_as_read implementationThe function has unnecessary JSON parsing that's not used in the function logic.
@login_required def mark_as_read(request): if request.method == "PATCH": try: - if request.body and request.content_type == "application/json": - try: - json.loads(request.body) - except json.JSONDecodeError: - return JsonResponse({"status": "error", "message": "Invalid JSON in request body"}, status=400) notifications = Notification.objects.filter(user=request.user, is_read=False) notifications.update(is_read=True) return JsonResponse({"status": "success"}) except Exception as e: logger.error(f"Error marking notifications as read: {e}") return JsonResponse( {"status": "error", "message": "An error occured while marking notifications as read"}, status=400 )
1163-1180: Consider using transaction for delete_notificationUsing a database transaction would ensure atomicity for this operation.
@login_required def delete_notification(request, notification_id): if request.method == "DELETE": try: + with transaction.atomic(): notification = get_object_or_404(Notification, id=notification_id, user=request.user) notification.is_deleted = True notification.save() return JsonResponse({"status": "success", "message": "Notification deleted successfully"}) except Exception as e: logger.error(f"Error deleting notification: {e}") return JsonResponse( {"status": "error", "message": "An error occured while deleting notification, please try again."}, status=400, ) else: return JsonResponse({"status": "error", "message": "Invalid request method"}, status=405)Additionally, don't forget to import the transaction module:
from django.db.models import Count, F, Q, Sum +from django.db import transaction from django.db.models.functions import ExtractMonthwebsite/templates/includes/header.html (5)
123-144: UI implementation looks good but needs cross-browser testingThe notification dropdown UI structure is well-organized using appropriate HTML elements and Tailwind CSS classes. The bell icon with an unread count badge is a standard pattern that users will recognize.
Consider adding ARIA attributes for accessibility:
-<button id="notificationButton" - class="relative p-2 rounded-full hover:bg-gray-200 transition-colors duration-200"> +<button id="notificationButton" + class="relative p-2 rounded-full hover:bg-gray-200 transition-colors duration-200" + aria-label="Notifications" + aria-haspopup="true" + aria-expanded="false">Please verify this implementation works correctly across different browsers, especially for older versions of Safari and Firefox.
707-720: Notification type icon mapping is well-implementedThe function provides appropriate icons for different notification types.
Consider adding a fallback default icon at the end of the function to handle any future notification types:
function getNotificationIcon(type) { switch (type) { case "alert": return '<i class="fa fa-exclamation-circle text-red-500 fa-lg"></i>'; case "reminder": return '<i class="fa fa-clock text-yellow-500 fa-lg"></i>'; case "promo": return '<i class="fa fa-tag text-green-500 fa-lg"></i>'; case "reward": return '<i class="fa fa-gift text-purple-500 fa-lg"></i>'; default: return '<i class="fa fa-info-circle text-blue-500 fa-lg"></i>'; } }
745-781: Delete notification implementation uses proper error handlingThe function properly handles various error cases and provides appropriate feedback to users.
Remove the console.log statement on line 698 as it's not needed in production code:
-console.log("Deleting notification with ID:", notificationId);
785-817: Effective event listeners for notification dropdownThe code properly handles opening/closing the dropdown and marks notifications as read when viewed.
Consider adding keyboard navigation support for accessibility. Users should be able to tab through notifications and use Enter to interact with them.
document.addEventListener("DOMContentLoaded", function () { // Existing code... + // Add keyboard support for notifications + notificationList.addEventListener("keydown", function(event) { + if (event.key === "Enter" && event.target.classList.contains("notification-item")) { + if (event.target.dataset.link) { + window.location.href = event.target.dataset.link; + } + } + }); });
662-689: Good notification item creation, but consider extracting as a separate functionThe notification item creation logic is well implemented but embedded within the fetchNotifications function.
Consider extracting this logic into a separate function for better maintainability:
+function createNotificationItem(notification) { + const notificationItem = document.createElement("div"); + notificationItem.classList.add( + "flex", "items-start", "gap-3", "p-3", "rounded-lg", "hover:bg-gray-100", "transition-colors", + !notification.is_read ? "bg-gray-50" : "bg-white", + !notification.is_read ? "font-semibold" : "font-normal" + ); + + if (notification.link) { + notificationItem.addEventListener("click", () => { + window.location.href = notification.link; + }); + notificationItem.classList.add("cursor-pointer"); + } + + notificationItem.innerHTML = ` + <div class="flex-shrink-0"> + ${getNotificationIcon(notification.notification_type)} + </div> + <div class="flex-1"> + <p class="text-sm">${notification.message}</p> + <p class="text-xs text-gray-500">${timeSince(notification.created_at)} ago</p> + </div> + <button class="delete-notification p-1 text-gray-400 hover:text-red-500 transition-colors" data-notification-id="${notification.id}"> + <i class="fa fa-times"></i> + </button> + `; + + return notificationItem; +} // Then in fetchNotifications: - notifications.forEach(notification => { - const notificationItem = document.createElement("div"); - // ... (existing code) - notificationList.appendChild(notificationItem); - }); + notifications.forEach(notification => { + const notificationItem = createNotificationItem(notification); + notificationList.appendChild(notificationItem); + });website/notification_signals.py (4)
34-35: Redundant list conversionConverting to a set and then back to a list can be simplified.
-recipients = list(set(recipients)) +recipients = list(set(recipients)) # Remove duplicatesWhile this line works correctly to remove duplicates, adding a comment would help clarify the purpose.
70-81: Typo in function nameThere's a spelling error in the function name.
-def notify_user_on_badge_recieved(sender, instance, created, **kwargs): +def notify_user_on_badge_received(sender, instance, created, **kwargs):
77-79: Inconsistent URL construction approachSome functions use string formatting for URLs while others use reverse().
Use the reverse() function consistently for all URL generation:
-link = f"/profile/{instance.user.username}" +link = reverse("profile", kwargs={"username": instance.user.username})This makes URL generation consistent and ensures URLs will be updated if routes change.
1-81: Overall signal handler design considerationThe signal handlers are well structured, but they lack error handling which can cause issues if notification creation fails.
Wrap the notification creation in try-except blocks to prevent main operations from failing:
@receiver(post_save, sender=UserBadge) def notify_user_on_badge_received(sender, instance, created, **kwargs): """ Signal to notify users when they receive a new badge. """ if created: try: message = f"You have just earned a new badge: {instance.badge.title}!" link = reverse("profile", kwargs={"username": instance.user.username}) Notification.objects.create( user=instance.user, message=message, notification_type="reward", link=link ) except Exception as e: logger.error(f"Failed to create badge notification: {e}") # Main operation (badge creation) should still succeedFor production systems, consider moving notification creation to asynchronous tasks using Celery or Django Q to prevent notifications from impacting the performance of main operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
blt/urls.py(2 hunks)website/admin.py(2 hunks)website/management/commands/daily_checkin_reminder.py(1 hunks)website/management/commands/run_daily.py(1 hunks)website/migrations/0231_organization_check_ins_enabled_notification.py(1 hunks)website/models.py(2 hunks)website/notification_signals.py(1 hunks)website/templates/includes/header.html(3 hunks)website/views/user.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
website/management/commands/run_daily.py (1)
44-47: LGTM! Integration for the daily check-in reminders.The integration of the daily check-in reminder command follows the established pattern in this file. The error handling is consistent with other commands and the error message is clear.
website/admin.py (2)
44-44: LGTM! Notification model import added correctly.The Notification model is properly added to the imports list.
762-762: LGTM! Notification model registered correctly.The Notification model is properly registered with the Django admin site.
blt/urls.py (1)
302-302: LGTM! Proper import of notification-related view functions.The view functions for handling notifications are properly imported.
Also applies to: 304-304, 310-310
website/models.py (1)
175-177: LGTM: Organization model extension looks goodAdding
check_ins_enabledflag to the Organization model properly supports the check-in reminder feature requirement.website/migrations/0231_organization_check_ins_enabled_notification.py (1)
1-56: Migration file correctly reflects model changesThe migration properly adds the
check_ins_enabledfield to the Organization model and creates the new Notification model with all necessary fields and meta options.website/views/user.py (1)
45-45: LGTM: Correctly imported Notification modelThe Notification model is properly imported to support the new notification functionality.
website/templates/includes/header.html (2)
623-705: Robust notification fetching implementation with proper error handlingThe
fetchNotificationsfunction is well-structured with thorough error handling and data validation. It properly updates the UI based on the fetched data.
722-744: Effective time formatting functionThe
timeSincefunction effectively formats timestamps into human-readable relative time strings.website/notification_signals.py (2)
1-6: Good imports organizationThe imports are appropriately organized, with Django core imports first followed by application imports.
8-21: Points reward notification implementation is effectiveThe signal handler properly creates a notification when points are awarded, with a clear message that includes the points and reason.
🧩 Analysis chain@DonnieBLT I've reviewed the notification system implementation in PR #3216 and found it to be well-structured and comprehensive. Here's my assessment: Strengths
Minor Concerns
Overall, the implementation meets the requirements specified in the PR objectives and appears to be production-ready. The code is well-organized with proper separation of concerns. I recommend approving this PR. 🏁 Script executed: Length of output: 5640 🏁 Script executed: Length of output: 334 🏁 Script executed: Length of output: 2211 🏁 Script executed: Length of output: 2769 @DonnieBLT Upon further review of the PR #3216, I can confirm the notification system looks well-implemented: What's Implemented Well
Potential ImprovementsI noticed that notification_signals.py might not be imported in the WebsiteConfig.ready() method in apps.py (alongside the other signal imports). This could potentially prevent the signals from being registered. However, this is a minor issue that can be addressed in a follow-up PR if needed. The core notification functionality appears to be implemented correctly and meets the requirements described in the PR objectives. I'm approving this PR as it adds valuable functionality to the application. ✅ Actions performedComments resolved and changes approved. |
|
That's such a cool bot 😦 |
User description
fixes #2560
A notification system is created for the users that looks like this:
screen-capture (1).webm
However, in order to fully close this issue we need to create a cron job via some scheduler to send these reminders to users.
If you can also review #3209 then it can be easily incorporated
PR Type
Enhancement, Tests
Description
Introduced a notification system with dynamic dropdown UI.
Added backend support for fetching and marking notifications as read.
Implemented daily check-in reminders via a management command.
Created notification signals for various user activities.
Changes walkthrough 📝
5 files
Added notification dropdown UI and dynamic behavior.Added management command for daily check-in reminders.Added Notification model and organization check-ins field.Implemented signals to create notifications for user activities.Added views for fetching and marking notifications as read.2 files
Added routes for fetching and marking notifications as read.Registered Notification model in admin panel.1 files
Created migration for Notification model and organization check-ins.Summary by CodeRabbit