#3054 - Email notification for blocked disbursement to ministry only once #4245
Conversation
|
E2E Workflow Workers Coverage Report
|
Backend Unit Tests Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
| const notifications = [ | ||
| this.ministryBlockedDisbursementNotification, | ||
| this.studentBlockedDisbursementNotification, | ||
| ].map((notification) => | ||
| notification.notify(eCertDisbursement, entityManager, log), |
There was a problem hiding this comment.
❤️ What a great way to segregate different notification services.
| * Provides a basic structure for creating and | ||
| * sending notifications related to e-Cert disbursements. | ||
| */ | ||
| export abstract class ECertNotification { |
There was a problem hiding this comment.
Great job! I love the way how the way how the e-cert based notifications are handled here with extensibility to add more notifications.
| * Provides a basic structure for creating and | ||
| * sending notifications related to e-Cert disbursements. | ||
| */ | ||
| export abstract class ECertNotification { |
There was a problem hiding this comment.
One minor comment to mention. As this is a base class to create notifications, the student data is a core common part that can be built from a base class method. And that base class method can be called inside createNotification()
There was a problem hiding this comment.
The idea was that each notification could evolve differently and the data required for each should be handled in the inherited class. For instance, a notification for the Ministry could potentially contain further information about the blocked disbursement or something else. Assuming all notifications will have the student data as a core would not be accurate IMO.
There was a problem hiding this comment.
Excluding ministry emails for institutions, other ministry notifications have student first and last name as common. But not a blocker considering the present.
|
Amazing work @andrewsignori-aot. Just one comment. |
andrewsignori-aot
deleted the
feature/#3054-email-notification-disbursement
branch
ECertNotification) and segregated the logic to generate blocked notifications intoMinistryBlockedDisbursementNotificationandStudentBlockedDisbursementNotification.shouldCreateNotificationfor the Students was moved and no changes were done for this PR, unless critical no comments will be addressed.ECertNotificationcan be used for any other e-Cert notification as required, not only blocked disbursement.Please note that the student data required to generate the notification is now retrieved twice in the event that the disbursement is blocked and the Ministry should be notified for the first time. The benefit of having the separation between the logic for the different notifications seems greater than an occasional extra query to the student's table. Please note that the query was simplified from what it used to be.
Please also note that the below technical AC was not executed. The queries have different approaches, sometimes checking the existence, sometimes requiring a count, and sometimes requiring a count and a date last sent. I did not see a benefit for this PR. Please share an opinion/suggestion if needed.