Feature/Extension notification system - Backend #60
Conversation
samindiii
left a comment
samindiii
left a comment
There was a problem hiding this comment.
Great work Sahiru! The code is very easy to read (thanks to the comments) and meets all the functional requirements of the task :) I found one issue but its quite minor so should be an easy fix, let me know what you think!
app/mailers/notifications_mailer.rb
Outdated
| end | ||
|
|
||
| # Set default from address using class methods | ||
| default from: -> { "#{self.class.doubtfire_product_name} <#{@granted_by&.email}>" } |
There was a problem hiding this comment.
Isn't @granted_by an instance variable, which means it's only set inside specific methods? I think when Rails loads the notification mailer class, this block is evaluated in the context of the notification class and not in the context of an individual email being sent.
I think @granted_by will be nil for each email and it might work if you just move the logic to each method.
Maybe you can add a test to check that the from: address is set correctly in the emails.
There was a problem hiding this comment.
Thank you for the review, will be making these changes !!
|
Everything looks great Sahiru! The backend notification system is well thought out and clearly tested. I only have one minor suggestion :) |
app/api/staff_grant_extension_api.rb
Outdated
| params[:student_ids].count, | ||
| results[:failed], | ||
| true # is_staff_grant = true | ||
| ).deliver_later |
There was a problem hiding this comment.
To help with debugging failed emails, it might help to wrap this in a begin...rescue block and logging any errors in case deliver_later fails :)
There was a problem hiding this comment.
Thank you for the review, A good suggestion, I will make these changes!!
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.
SahiruWithanage
force-pushed
the
feature/extension-notification-system
branch
from
dfb4e36 to
c454932
Compare
|
|
A comment line change made in the staff grant extension feature branch that hasn't been updated here. Changing to keep the consistency.
|
@samindiii @JoeMacl Changes suggested have been made and tested. Thank you :) |
|
LGTM! |
4 participants
Description
Important Note:
This pull request depends on PR #56 (Staff Grant Extension).
The first few commits are from PR #56.
Please review only the changes related to the Notification System feature.
This pull request implements the backend notification system for the Staff Grant Extension feature in OnTrack.
When a tutor grants extensions to students through the Staff Grant Extension endpoint:
The Staff Grant Extension API also returns additional notification-related information in its response, allowing the frontend to display in-platform notifications to students if preferred.
This enhancement improves communication between staff and students and prepares the system for frontend notification display.
The functionality was tested through a new test file:
notifications_mailer_test.rb.Related project: Staff Grant Extension - Backend Notification System
Type of change
How Has This Been Tested?
The following tests have been written and run to verify correct behavior:
notifications_mailer_test.rbNote: This is backend-only work. Frontend integration for in-app notification display is tracked separately.
Checklist
If involving code
If modified config files
Folders and Files Added/Modified
Added:
app/views/notifications_mailer/extension_granted.html.erbapp/views/notifications_mailer/extension_granted.text.erbtest/mailers/notifications_mailer_test.rbModified:
app/api/staff_grant_extension_api.rbapp/mailers/notifications_mailer.rbconfig/environments/test.rbAdditional Notes