-
Notifications
You must be signed in to change notification settings - Fork 120
Add notification table and model for in-system notifications #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 9.x
Are you sure you want to change the base?
Add notification table and model for in-system notifications #69
Conversation
Enable staff to grant extensions to multiple students without formal requests. Reuse existing student extension logic through a new service for consistency. Supports flexible academic support and streamlines staff workflows. Relates to the OnTrack Staff Grant Extension design documentation.
…rpolated strings This aligns the test file with the string formatting convention used in the rest of the codebase. Single quotes are preferred when string interpolation is not needed, improving consistency. Reviewed as part of peer feedback.
Linked extension_comments_api (student-requested extensions) to use the shared ExtensionService, previously set up for staff-granted extensions. This refactor ensures both student and staff extension flows use the same logic, improving consistency and reducing duplication.
Implemented backend logic to send emails to tutor and student when extensions are granted. Also enable it so the front end can use the returned information from the api to display notifications.
A comment line change made in the staff grant extension feature branch that hasn't been updated here. Changing to keep the consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Samindi !
I've reviewed the pr and it looks solid! The code is concise and follows the project's patterns well. I like how you've structured the database with a clean notification model connected to users and your API endpoints for fetching and deleting notifications are well-implemented.
A few minor suggestions that might be worth considering: adding some basic validations to the Notification model (e.g., presence/length for message), adding tests specifically for the notification API endpoints to ensure complete coverage and implementing pagination for the GET endpoint as notifications could accumulate over time (for an example someone might not click this at all for a long time and decides to click it only to get flooded with notifications).
I ran the Rails tests and no issues came up related to this implementation, which is a good sign but worth considering adding some test cases specific for this feature.
Overall, this provides a good foundation for the in-system notifications that accomplishes the goal of showing extension notifications to students. Great work!!
JoeMacl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested everything and it’s working great! Code is well structured and easy to follow.
As Sahiru mentioned, it’d be great to add some basic validations to the notification model like presence: true and maybe a character limit for the message. Also, adding pagination to the GET /notifications endpoint would help handle cases where students accumulate a lot of notifications over time.
Overall, great job! :)
|
@samindiii LGTM. Please open an upstream pull request against 9.x branch in /doubtfire-lms/doubtfire-api. |
- Add notifcation factory - Add unit tests for notifications - Only send first 20 notifications to front end - Have a max characters of 500
|
Fixed up the code based on Sahiru and Joe's suggestions. Added a new test for the notifications API |
Description
This PR contains backend changes from PR:60 by Sahiru. The first five commits are Sahiru's and have already been reviewed in the relevant PR. In his PR, he created the backend for the Staff Grant Extension Feature and additionally created the backend notification system for emails.
This PR contains the backend notification system for in-system notifications. Since we currently do not have a way to persist notifications to display on the onTrack homepage, I created a notifications table and associated model. This table will store notifications for each user along with an associated message. This functionality can be extended in the future to store a boolean read value etc. The model has a relation with the user object so that we can access notifications for a specific user.
In terms of the Staff Grant Extension feature, a notification is created whenever an extension is granted successfully. This means we have a way of persisting the notification so that it doesn't just exist in memory but can be stored until it is read and deleted by the user. Additionally added tests to check that a notification is created when an extension is granted successfully.
Type of change
How Has This Been Tested?
To test this:
In your terminal, change directory to doubtfire-api - if you are in doubtfire-deploy, simply run the following
cd doubtfire-apiRun the following to apply pending migrations and update your database schema:
rails db:migrateRun all test files using the following command:
rails testRun the staff grant extension test using the following command:
rails test test/api/staff_grant_extension_test.rbAdditionally, if you want to test the notifications system integration with the front end, pull the front end code from PR:353 This means you can now view your notifications on the ontrack homepage.
Notification.create!(user_id: 13, message: "Test notification for student 13")Checklist:
If you have any questions, please contact @macite or @jakerenzella.