Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
=======================================
Coverage 57.05% 57.05%
=======================================
Files 14 14
Lines 475 475
Branches 37 37
=======================================
Hits 271 271
Misses 196 196
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
omgdory
left a comment
There was a problem hiding this comment.
🔍 Peer Review: Sessions Model + Set up for Issue #50
Hey Chris👋 I just reviewed your PR, here's my thoughts:
✅ What Looks Great
Comprehensive Implementation: The PR introduces a well-structured Sessions model, aligning with the project's architectural standards. This addition is crucial for managing user sessions effectively.
Integration: By implementing Celery tasks for periodic reminders, this PR ensures that the Sessions model is not just standalone but integrates seamlessly with the application's notification system.
Code Quality and Organization: The code additions and modifications are cleanly written, with a clear separation of concerns. This organization facilitates easier maintenance and future enhancements.
Documentation and Clarity: The PR provides adequate documentation, making it easier for other contributors to understand the changes and their implications within the broader application context.
💡 Suggestions for Improvement (if any)
- As documented in
backend/apps/notifications/tasks.py, create the Celery tasks for the session and assignment reminders. As defined, this can be done in your next PR!
🧪 Tested This Feature
- Are the model relationships properly defined?
- Are the database migrations working correctly?
- Is the priority field properly implemented in the Notification model?
- Does the Session model contain all necessary fields?
- Are the model methods and properties properly implemented?
- Is the documentation clear and comprehensive?
🚀 Final Thoughts
Overall, this PR exemplifies thoughtful design and integration, enhancing the application's functionality while maintaining high code quality standards.
TLDR
alliekameda
left a comment
There was a problem hiding this comment.
🔍 Peer Review: Great Job on This PR!
Hey Chris 👋 — just finished reviewing your PR "Sessions Model + Set up for Issue #50 #166". Here’s my feedback:
✅ What Looks Great
- Sessions, lessons, and notificatins all behave properly.
- The core feature has been implemented cleanly.
- UI/UX details look consistent, responsive, and user-friendly.
- Your well structured and modular code is easy to read.
- The use of JWT authentication in WebSockets is well-thought-out and securely integrated.
💡 Suggestions for Improvement (if any)
These are ideas for future improvements or things to address before merging. Nitpicks, really.
General
- Consider adding disconnect and reconnect logic on the client side to handle dropped WebSocket connections gracefully.
- For filtering features, you could add options like "unread messages only", "by chat participant", or "most recent first" to give users more control.
- Add some unit tests for the
ChatConsumerlogic to ensure edge cases like invalid tokens or unauthorized access are covered.
notifications/models.py
- Optional improvement: Replace
PRIORITY_CHOICESwith DjangoTextChoices:
class PriorityLevel(models.TextChoices):
LOW = 'low', 'Low'
MEDIUM = 'medium', 'Medium'
HIGH = 'high', 'High'
priority = models.CharField(
max_length=10,
choices=PriorityLevel.choices,
default=PriorityLevel.MEDIUM
)- Future-proof
info_category: Validate only ifnotification_type == INFO. Currently, invalidinfo_categorycan be set on non-INFO notifications.
sessions/models.py
- Add unique constraints (future-proofing): If there's only one session allowed per time slot or tutor/student pair, enforce that via
unique_together. - Validation: Add model-level
clean()method to ensure:end_time > start_timemeeting_linkmust be present ifis_virtual == True
lessons/models.py
- If
reminder_sentis meant to be used programmatically, think about adding aDateTimeFieldinstead for better auditing.
reminder_sent_at = models.DateTimeField(null=True, blank=True)notifications/tasks.py
- Double send bug: This line appears twice in the loop:
notification.mark_as_sent()
num_of_notifs += 1🧪 Tested This Feature
I ran the following test steps:
- ✅ Run migrations to create the new models and fields
- ✅ Access Django admin to verify the Notification model's new priority field
- ✅ Create a test Session through the admin interface
- ✅ Verify that relationships between models work correctly
🔄 Next Steps
- Merge. :) Genuinely I don't see anything urgent that needs to be changed.
🚀 Final Thoughts
Good work! ✧。٩(ˊᗜˋ )و✧*。


Prepare Models for Periodic Notification System Implementation
Overview
What does this PR do?
This PR establishes the foundational model changes needed for our upcoming Celery-based notification system. It modifies the existing Notification model to include priority levels, updates the Lesson model with reminder fields, and introduces a new Session model to track tutoring sessions.
Key Features & Changes
Model Enhancements: Added priority field to the Notification model
New Model Creation: Implemented Session model for tracking tutoring sessions
Model Updates: Added reminder tracking to the Assignment model
low,medium,high) toNotificationmodelSessionmodel with fields for tracking tutoring sessionsAssignmentmodel to support reminder functionalityWhy This Is Needed
What problem does this solve?
This PR lays the groundwork for our notification system by establishing the data structures necessary to track sessions, assignments, and notification priorities.
Related Issue
🔗 This PR addresses Issue #164 (which is a sub-issue of main Issue #50)
Implementation Details
How was this feature developed?
priorityfield to theNotificationmodel withLOW,MEDIUM, andHIGHchoicesSessionmodel with:Assignmentmodel with areminder_sentfieldHow to Test This PR
Step-by-step testing guide
1️⃣ Run migrations to create the new models and fields
2️⃣ Access Django admin to verify the Notification model's new priority field
3️⃣ Create a test Session through the admin interface
4️⃣ Verify that relationships between models work correctly
Files Added/Modified
📂 List of important files changed in this PR
notifications/models.pysessions/models.pylessons/models.pyAI Code Usage
🤖 Was AI-generated code used in this PR?
Checklist for Reviewers
Things to check before approving this PR:
Next Steps & Future Enhancements
What improvements can be made after merging?
Final Notes
This PR completes the first phase of our notification system by setting up all the necessary models. The next PR will focus on implementing the Celery tasks to make the reminder system fully functional. Prayge for me :<