PM- 3351 Send notifications to resources on manual phase change#70
PM- 3351 Send notifications to resources on manual phase change#70himaniraghav3 wants to merge 18 commits intodevelopfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Build payload for phase change email notification | ||
| * @param {String} challenge Id |
There was a problem hiding this comment.
[💡 maintainability]
The JSDoc comment for buildPhaseChangeEmailData has incorrect parameter names. The parameters should match the destructured object properties: challengeId, challengeName, phaseName, operation, and at.
| */ | ||
| async function sendPhaseChangeNotification(type, recipients, data) { | ||
| try { | ||
| const settings = constants.PhaseChangeNotificationSettings?.[type]; |
There was a problem hiding this comment.
[correctness]
The optional chaining operator ?. is used with constants.PhaseChangeNotificationSettings?.[type]. Ensure that constants.PhaseChangeNotificationSettings is always defined to avoid potential runtime errors.
| ); | ||
| return; | ||
| } | ||
| const safeRecipients = Array.isArray(recipients) ? recipients.filter(Boolean) : []; |
There was a problem hiding this comment.
[💡 maintainability]
The safeRecipients array is created by filtering out falsy values. Consider logging or handling cases where recipients are filtered out to ensure no unintended omissions.
| ], | ||
| }); | ||
| } catch (e) { | ||
| logger.debug(`Failed to post notification ${type}: ${e.message}`); |
There was a problem hiding this comment.
[correctness]
The catch block logs the error message but does not rethrow the error or handle it in a way that would alert the caller of the failure. Consider rethrowing the error or handling it to ensure the caller is aware of the failure.
| at, | ||
| }); | ||
|
|
||
| await helper.sendPhaseChangeNotification(notificationType, recipients, payload); |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for helper.sendPhaseChangeNotification. Currently, if this function fails, it will not be logged or handled, which could lead to silent failures in sending notifications.
src/common/helper.js
Outdated
| async function sendSelfServiceNotification(type, recipients, data) { | ||
| try { | ||
| await postBusEvent(constants.Topics.Notifications, { | ||
| await postBusEvent(constants.Topics.ChallengePhaseUpdated, { |
There was a problem hiding this comment.
[❗❗ correctness]
The change from constants.Topics.Notifications to constants.Topics.ChallengePhaseUpdated alters the topic of the event being posted. Ensure that this change aligns with the intended behavior and that all consumers of this event are updated accordingly to handle the new topic. This could impact systems relying on the previous topic.
src/common/helper.js
Outdated
| async function sendSelfServiceNotification(type, recipients, data) { | ||
| try { | ||
| await postBusEvent(constants.Topics.Notifications, { | ||
| await postBusEvent('external.action.email', { |
There was a problem hiding this comment.
[❗❗ correctness]
The topic for the bus event has been changed from constants.Topics.ChallengePhaseUpdated to 'external.action.email'. Ensure that this change aligns with the intended functionality and that all dependent systems are updated accordingly. If this is not intentional, it could lead to incorrect event handling.
| await postBusEvent('external.action.email', | ||
| { | ||
| from: config.EMAIL_FROM, | ||
| replyTo: config.EMAIL_FROM, |
There was a problem hiding this comment.
[design]
The replyTo field is set to config.EMAIL_FROM. Ensure that this is the intended behavior, as it might be more appropriate to have a separate configuration for the reply-to address to avoid potential issues with email replies.
| from: config.EMAIL_FROM, | ||
| replyTo: config.EMAIL_FROM, | ||
| recipients: safeRecipients, | ||
| data: data, |
There was a problem hiding this comment.
[❗❗ security]
The data object is being passed directly without any transformation or validation. Consider validating or sanitizing the data object to prevent potential security issues, such as injection attacks.
| return; | ||
| } | ||
|
|
||
| await postBusEvent('external.action.email', |
There was a problem hiding this comment.
[maintainability]
The removal of detailed logging statements might hinder debugging and monitoring capabilities. Consider retaining some level of logging to track the flow and data involved in the notification process.
| * @param {String} challenge name | ||
| * @param {String} challenge phase name | ||
| * @param {String} operation to be performed on the phase - open | close | reopen | ||
| * @param {String|Date} at - The date/time when the phase opened/closed |
There was a problem hiding this comment.
[correctness]
The parameter at is documented as String|Date, but the function buildPhaseChangeEmailData does not validate or convert the at parameter to ensure it is a Date object. Consider adding validation or conversion logic to handle cases where at is a string to avoid potential issues with date formatting or operations.
| /** | ||
| * Send phase change notification | ||
| * @param {String} type the notification type | ||
| * @param {Array} recipients the array of recipients emails |
There was a problem hiding this comment.
[correctness]
The updated documentation for recipients specifies it as an array of emails, but the function sendPhaseChangeNotification does not validate the email format. Consider adding validation to ensure all elements in the recipients array are valid email addresses to prevent potential errors in email delivery.
1 participant
Sends notification to resources on a challenge when phase is changed manually
https://topcoder.atlassian.net/browse/PM-3351