-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1977 - Request Offering Change: Student Application Tracker - Part 1 [Email Notification] #2114
#1977 - Request Offering Change: Student Application Tracker - Part 1 [Email Notification] #2114
Conversation
…1 [Email Notification]
.../services/application-offering-change-request/application-offering-change-request.service.ts
Outdated
Show resolved
Hide resolved
VALUES | ||
( | ||
13, | ||
'Offering Change Request Inprogress With Student', |
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.
It would be good to have the notification details also in the ticket.
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.
Notification details for offering change request In progress with student are present in the ticket. Not sure if Jason wants to change it to have a common template for all 3 scenarios - in progress with student, approved and declined by SABC.
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.
It would be good to have the notification details also in the ticket.
By details I meant that the 'Offering Change Request Inprogress With Student' would be part of the ticket AC also.
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.
I will check it with @JasonCTang
VALUES | ||
( | ||
13, | ||
'Offering Change Request Inprogress With Student', |
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.
Please fix the "Inprogress". Please ensure that the spell checker is also enabled for SQL files.
Was this message reviewed by the business? The idea is that these messages can potentially be shown to the user someday.
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.
I haven't reviewed with the business yet. I will check it with @JasonCTang
@@ -136,4 +136,8 @@ export enum NotificationMessageType { | |||
* MSFAA record gets cancelled. | |||
*/ | |||
MSFAACancellation = 12, | |||
/** | |||
* OfferingChangeRequest is in progress with student. |
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.
The feature is called "Application offering change request" in an attempt to differentiate from the other "offering request" that can be requested by the institution.
Can we have at least the comment changed to "An Application Offering Change Request is in progress with the student."?
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.
Nice work. Please take a look at the comments.
VALUES | ||
( | ||
13, | ||
'Offering Change Request Inprogress With Student', |
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.
shouldn't we also include application
, so it is `Application Offering Change Request In progress With Student',
@@ -30,6 +30,13 @@ export interface MSFAACancellationNotification { | |||
userId: number; | |||
} | |||
|
|||
export interface OfferingChangeRequestInProgressWithStudentNotification { |
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.
Maybe adding application
to the name will help differentiate it from the actual offering change
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.
Added some minor comments
VALUES | ||
( | ||
13, | ||
'Application Offering Change Request In Progress With Student', |
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.
I would say the message type could be "Institution requests a change"
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.
I have requested the message from @JasonCTang as well since it might be shown to the users someday.
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.
Confirmed with business area that the wording is good to go!
* @param auditUserId user that should be considered the one that is causing the changes. | ||
* @param entityManager optional entity manager to execute in transaction. | ||
*/ | ||
async saveOfferingChangeRequestInProgressWithStudent( |
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.
saveInstitutionRequestChangeNotification
?
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.
If there is a common template for all 3 scenarios, I will update the names everywhere.
/** | ||
* An Application Offering Change Request is in progress with the student. | ||
*/ | ||
OfferingChangeRequestInProgressWithStudent = 13, |
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.
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.
If there is a common template for all 3 scenarios, I will update the names everywhere.
Good Job. Added some minor comments. |
Discussed with Shashank and we will be sticking to this template for the initial email to the student when the institution first submits the request to change an offering for said student. A separate single template will be used to notify the student regarding the actions that the Ministry takes on the request later in the process. |
Kudos, SonarCloud Quality Gate passed!
|
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.
Thanks for doing the changes 👍
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.
LGTM, nice work @sh16011993
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.
LGTM 👍 Thanks for doing the changes @sh16011993 👍
As a part of this PR, added an email notification template to GC Notify for the scenario: offering change request In progress with student and added the code to save a notification message when the application offering change request is created successfully by the institution.
Screenshot attached below: