-
Notifications
You must be signed in to change notification settings - Fork 55
feat(PM-1611): send email when opportunity is completed or canceled #849
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
Conversation
|
||
const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id); | ||
|
||
users.forEach(async (user) => { |
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.
Using forEach
with an async
function can lead to unexpected behavior because the asynchronous operations inside the loop won't be awaited. Consider using a for...of
loop or Promise.all
to handle asynchronous operations properly.
const requestData = copilotRequest.data; | ||
createEvent(emailEventType, { | ||
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity`, |
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 URL construction for opportunity_details_url
might need to include the specific opportunity ID or other identifiers to ensure the link directs to the correct opportunity details page.
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity`, | ||
opportunity_title: requestData.opportunityTitle, | ||
user_name: user ? user.handle : "", |
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.
Consider handling the case where user
might be null
or undefined
more explicitly to avoid potential errors when accessing user.handle
.
@@ -134,6 +169,9 @@ module.exports = [ | |||
}, req.log); | |||
|
|||
req.log.debug(`Email sent`); | |||
|
|||
// Send email to all applicants about opportunity completion | |||
await sendEmailToAllApplicants(req, opportunity, copilotRequest, application.id); |
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.
Consider handling potential errors from the sendEmailToAllApplicants
function. If the email sending fails, it might be useful to log the error or take corrective actions.
const userIds = applications.map(item => item.userId); | ||
const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id); | ||
|
||
users.forEach(async (user) => { |
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.
Consider using Promise.all
to handle asynchronous operations in the users.forEach
loop. This will ensure that all email sending operations are completed before proceeding, and it will also handle errors more effectively.
const requestData = copilotRequest.data; | ||
createEvent(emailEventType, { | ||
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity`, |
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 URL concatenation for opportunity_details_url
may lead to incorrect URLs if copilotPortalUrl
does not end with a slash. Consider using a URL library or ensuring the base URL ends with a slash to prevent potential issues.
@@ -93,6 +120,8 @@ module.exports = [ | |||
invite.toJSON()); | |||
} | |||
|
|||
await sendEmailToAllApplicants(req, copilotRequest, applications) |
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 seems like the function sendEmailToAllApplicants
is being called here, but there is no error handling around this asynchronous operation. Consider adding a try-catch block to handle any potential errors that may occur during the email sending process.
|
||
const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id); | ||
|
||
users.forEach(async (user) => { |
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.
Consider using a Promise.all
to handle asynchronous operations in the forEach
loop. This will ensure that all emails are sent before proceeding, and can help catch any errors in the email sending process.
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity`, | ||
opportunity_title: requestData.opportunityTitle, | ||
user_name: user ? user.handle : "", |
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 user_name
field is being set to an empty string if user
is falsy. Consider handling cases where user
might be undefined or null more explicitly to avoid sending incomplete data.
@@ -134,6 +169,9 @@ module.exports = [ | |||
}, req.log); | |||
|
|||
req.log.debug(`Email sent`); | |||
|
|||
// Send email to all applicants about opportunity completion | |||
await sendEmailToAllApplicants(opportunity, copilotRequest, application.id); |
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 sendEmailToAllApplicants
function call has been modified to remove the req
parameter. Ensure that this change is intentional and that the function sendEmailToAllApplicants
does not require the req
parameter for its execution. If req
is needed for logging or other purposes, consider revising the function signature or implementation.
const userIds = applications.map(item => item.userId); | ||
const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id); | ||
|
||
users.forEach(async (user) => { |
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.
Consider using Promise.all
to handle asynchronous operations within the forEach
loop. This ensures that all emails are sent before proceeding, and can help avoid potential issues with unhandled promises.
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity`, | ||
opportunity_title: requestData.opportunityTitle, | ||
user_name: user ? user.handle : "", |
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.
Consider using a default value or handling the case where user.handle
might be null
or undefined
to prevent potential issues with the email content.
@@ -93,6 +120,8 @@ module.exports = [ | |||
invite.toJSON()); | |||
} | |||
|
|||
await sendEmailToAllApplicants(copilotRequest, applications) |
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 sendEmailToAllApplicants
function call has been modified to remove the req
parameter. Ensure that this change does not affect the function's behavior or cause any issues, as the req
parameter might have been used within the function for accessing request-specific data.
src/routes/copilotOpportunity/get.js
Outdated
@@ -4,7 +4,7 @@ import util from '../../util'; | |||
module.exports = [ | |||
(req, res, next) => { | |||
const { id } = req.params; | |||
|
|||
req.log.info("Reached get endpoint"); |
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.
Consider using a more descriptive log message that includes the opportunity ID to provide better context for debugging, e.g., req.log.info(
Reached get endpoint for opportunity ID: ${id});
.
src/routes/index.js
Outdated
@@ -473,6 +473,7 @@ router.use((err, req, res, next) => { // eslint-disable-line no-unused-vars | |||
|
|||
// catch 404 and forward to error handler | |||
router.use((req, res, next) => { | |||
req.log.info("reached middleware") |
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.
Consider using a more descriptive log message to provide better context for debugging purposes. For example, specify which middleware was reached or the purpose of this log entry.
src/models/copilotRequest.js
Outdated
@@ -32,6 +32,7 @@ module.exports = function defineCopilotRequest(sequelize, DataTypes) { | |||
|
|||
CopliotRequest.associate = (models) => { | |||
CopliotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' }); | |||
CopliotRequest.belongsTo(models.Project, { as: 'project', foreignKey: 'projectId' }); |
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.
Typo in the model name: CopliotRequest
should be CopilotRequest
. This typo is present in the existing code as well, but it would be good to correct it to maintain consistency and avoid potential issues.
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.
@hentrymartin please remane and update for the typo. Also, check on all refrences using it.
Funny how we missed this one?
@@ -15,6 +17,10 @@ module.exports = [ | |||
return next(err); | |||
} | |||
|
|||
const page = parseInt(req.query.page, 10) || 1; |
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.
Consider handling the case where req.query.page
is not a valid integer. Currently, parseInt
will return NaN
for invalid inputs, which could lead to unexpected behavior when calculating offset
.
@@ -15,6 +17,10 @@ module.exports = [ | |||
return next(err); | |||
} | |||
|
|||
const page = parseInt(req.query.page, 10) || 1; | |||
const pageSize = parseInt(req.query.pageSize, 10) || DEFAULT_PAGE_SIZE; |
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.
Similar to the previous comment, ensure that req.query.pageSize
is a valid integer before using it. This will prevent potential issues with offset
calculation if pageSize
is NaN
.
@@ -29,19 +35,22 @@ module.exports = [ | |||
|
|||
const whereCondition = projectId ? { projectId } : {}; | |||
|
|||
return models.CopilotRequest.findAll({ | |||
return models.CopilotRequest.findAndCountAll({ |
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.
Consider checking if findAndCountAll
is necessary for your use case. If you only need the count for pagination headers, ensure that this change is intentional as it may impact performance.
as: 'copilotOpportunity', | ||
}, | ||
{ model: models.CopilotOpportunity, as: 'copilotOpportunity', required: false }, | ||
{ model: models.Project, as: 'project', required: false }, |
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.
Ensure that the inclusion of the Project
model is necessary for the functionality you are implementing. If not, it could lead to unnecessary data fetching.
@@ -227,6 +262,9 @@ module.exports = [ | |||
|
|||
await sendEmailToCopilot(); | |||
|
|||
// Send email to all applicants about opportunity completion | |||
await sendEmailToAllApplicants(opportunity, copilotRequest, application.id); |
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.
Consider handling potential errors that might occur during the execution of sendEmailToAllApplicants
. This will ensure that any issues in sending emails do not disrupt the overall flow of the application.
@@ -32,6 +32,33 @@ module.exports = [ | |||
return next(err); | |||
} | |||
|
|||
const sendEmailToAllApplicants = async (copilotRequest, allApplications) => { |
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 function signature of sendEmailToAllApplicants
has changed, but the logic for fetching allApplications
has been removed. Ensure that allApplications
is correctly populated before calling this function, as it is now expected to be passed as an argument.
@@ -238,6 +265,9 @@ module.exports = [ | |||
transaction: t, | |||
}); | |||
|
|||
// Send email to all applicants about opportunity completion | |||
await sendEmailToAllApplicants(copilotRequest, otherApplications); |
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.
Consider handling potential errors from the sendEmailToAllApplicants
function. If this function fails, it might be beneficial to log the error or take corrective action to ensure the application process continues smoothly.
src/models/copilotRequest.js
Outdated
@@ -32,6 +32,7 @@ module.exports = function defineCopilotRequest(sequelize, DataTypes) { | |||
|
|||
CopliotRequest.associate = (models) => { | |||
CopliotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' }); | |||
CopliotRequest.belongsTo(models.Project, { as: 'project', foreignKey: 'projectId' }); |
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.
@hentrymartin please remane and update for the typo. Also, check on all refrences using it.
Funny how we missed this one?
@@ -30,9 +30,10 @@ module.exports = function defineCopilotRequest(sequelize, DataTypes) { | |||
indexes: [], | |||
}); | |||
|
|||
CopliotRequest.associate = (models) => { | |||
CopliotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' }); | |||
CopilotRequest.associate = (models) => { |
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.
Typo correction needed: The model name CopliotRequest
should be corrected to CopilotRequest
.
}; | ||
|
||
return CopliotRequest; | ||
return CopilotRequest; |
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.
Typo correction needed: The model name CopliotRequest
should be corrected to CopilotRequest
.
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.
@hentrymartin please address all occurances of CopliotRequest
in the code base.
@kkartunov this is already fixed, somehow, ai buddy points to old version of code. |
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1611