Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ workflows:
only:
- develop
- security
- PM-3327
- PM-3351

- "build-qa":
context: org-global
Expand Down
9 changes: 9 additions & 0 deletions app-constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ const PhaseFact = {
UNRECOGNIZED: -1
}

const PhaseChangeNotificationSettings = {
PHASE_CHANGE: {
sendgridTemplateId: config.PHASE_CHANGE_SENDGRID_TEMPLATE_ID,
cc: [],
},
};


const auditFields = [
'createdAt', 'createdBy', 'updatedAt', 'updatedBy'
]
Expand All @@ -168,4 +176,5 @@ module.exports = {
SelfServiceNotificationSettings,
PhaseFact,
auditFields,
PhaseChangeNotificationSettings,
};
2 changes: 2 additions & 0 deletions config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,6 @@ module.exports = {
RESOURCES_DB_SCHEMA: process.env.RESOURCES_DB_SCHEMA || "resources",
REVIEW_DB_SCHEMA: process.env.REVIEW_DB_SCHEMA || "reviews",
CHALLENGE_SERVICE_PRISMA_TIMEOUT: process.env.CHALLENGE_SERVICE_PRISMA_TIMEOUT ? parseInt(process.env.CHALLENGE_SERVICE_PRISMA_TIMEOUT, 10) : 10000,
CHALLENGE_URL: process.env.CHALLENGE_URL || 'https://www.topcoder-dev.com/challenges' ,
PHASE_CHANGE_SENDGRID_TEMPLATE_ID: process.env.PHASE_CHANGE_SENDGRID_TEMPLATE_ID || "",
};
68 changes: 68 additions & 0 deletions src/common/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,72 @@ async function sendSelfServiceNotification(type, recipients, data) {
}
}

/**
* Build payload for phase change email notification
* @param {String} challenge Id

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 maintainability]
The JSDoc comment for buildPhaseChangeEmailData has incorrect parameter names. The parameters should match the destructured object properties: challengeId, challengeName, phaseName, operation, and at.

* @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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

*/
function buildPhaseChangeEmailData({ challengeId, challengeName, phaseName, operation, at }) {
const isOpen = operation === 'open' || operation === 'reopen';
const isClose = operation === 'close';

return {
challengeURL: `${config.CHALLENGE_URL}/${challengeId}`,
challengeName,
phaseOpen: isOpen ? phaseName : null,
phaseOpenDate: isOpen ? at : null,
phaseClose: isClose ? phaseName : null,
phaseCloseDate: isClose ? at : null,
};
}


/**
* Send phase change notification
* @param {String} type the notification type
* @param {Array} recipients the array of recipients emails

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

* @param {Object} data the data
*/
async function sendPhaseChangeNotification(type, recipients, data) {
try {
const settings = constants.PhaseChangeNotificationSettings?.[type];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The optional chaining operator ?. is used with constants.PhaseChangeNotificationSettings?.[type]. Ensure that constants.PhaseChangeNotificationSettings is always defined to avoid potential runtime errors.


if (!settings) {
logger.debug(`sendPhaseChangeNotification: unknown type ${type}`);
return;
}

if (!settings.sendgridTemplateId) {
logger.debug(
`sendPhaseChangeNotification: sendgridTemplateId not configured for type ${type}`
);
return;
}
const safeRecipients = Array.isArray(recipients) ? recipients.filter(Boolean) : [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.


if (!safeRecipients.length) {
logger.debug(`sendPhaseChangeNotification: no recipients for type ${type}`);
return;
}

await postBusEvent('external.action.email',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

{
from: config.EMAIL_FROM,
replyTo: config.EMAIL_FROM,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

recipients: safeRecipients,
data: data,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

sendgrid_template_id: settings.sendgridTemplateId,
version: 'v3',
},
);
} catch (e) {
logger.debug(`Failed to post notification ${type}: ${e.message}`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

}
}

/**
* Submit a request to zendesk
* @param {Object} request the request
Expand Down Expand Up @@ -1756,6 +1822,8 @@ module.exports = {
setToInternalCache,
flushInternalCache,
removeNullProperties,
buildPhaseChangeEmailData,
sendPhaseChangeNotification
};

logger.buildService(module.exports);
65 changes: 65 additions & 0 deletions src/services/ChallengePhaseService.js
Original file line number Diff line number Diff line change
Expand Up @@ -813,9 +813,74 @@ async function partiallyUpdateChallengePhase(currentUser, challengeId, id, data)
_.assignIn({ id: result.id }, data)
);
await postChallengeUpdatedNotification(challengeId);

// send notification logic
try {
const shouldNotifyClose = Boolean(isClosingPhase);
const shouldNotifyOpen = Boolean(isOpeningPhase); // includes reopen

if (shouldNotifyClose || shouldNotifyOpen) {
// Single template - single type
const notificationType = "PHASE_CHANGE";

const operation = shouldNotifyClose
? "close"
: (isReopeningPhase ? "reopen" : "open");

const at = shouldNotifyClose
? (result.actualEndDate || new Date().toISOString())
: (result.actualStartDate || new Date().toISOString());

// fetch challenge name
const challenge = await prisma.challenge.findUnique({
where: { id: challengeId },
select: { name: true },
});

const challengeName = challenge?.name;

// build recipients
const resources = await helper.getChallengeResources(challengeId);

const recipients = Array.from(
new Set(
(resources || [])
.map(r => r?.email || r?.memberEmail)
.filter(Boolean)
.map(e => String(e).trim().toLowerCase())
)
);

if (!recipients.length) {
logger.debug(
`phase change notification skipped: no recipients for challenge ${challengeId}`
);
return _.omit(result, constants.auditFields);
}

// build payload that matches the SendGrid HTML template
const phaseName = result.name || data.name || challengePhase.name;

const payload = helper.buildPhaseChangeEmailData({
challengeId,
challengeName,
phaseName,
operation,
at,
});

await helper.sendPhaseChangeNotification(notificationType, recipients, payload);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

}
} catch (e) {
logger.debug(
`phase change notification failed for challenge ${challengeId}, phase ${id}: ${e.message}`
);
}

return _.omit(result, constants.auditFields);
}


partiallyUpdateChallengePhase.schema = {
currentUser: Joi.any(),
challengeId: Joi.id(),
Expand Down
Loading