-
Notifications
You must be signed in to change notification settings - Fork 56
fix(PM-1494): Add extra info for copilot email #830
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
], | ||
}); | ||
req.log.debug(opportunityWithProjectInfo, "debug log opportunityWithProjectInfo"); | ||
const data = opportunityWithProjectInfo.copilotRequest.data; |
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 variable name instead of reusing data
to avoid confusion with the data
parameter passed to the function.
@@ -71,6 +96,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(opportunity.type), | |||
opportunity_title: opportunityWithProjectInfo.project.name, |
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 opportunityWithProjectInfo.project
is defined before accessing name
to prevent potential runtime errors if project
is undefined.
@@ -71,6 +96,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(opportunity.type), | |||
opportunity_title: opportunityWithProjectInfo.project.name, | |||
start_date: moment.utc(data.startDate).format(), |
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 data.startDate
is a valid date before formatting it with moment.utc()
to avoid unexpected results or errors.
}, | ||
], | ||
}); | ||
req.log.debug(opportunityWithProjectInfo); |
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 debug log statement on line 85 has been split into two separate log statements. Consider combining them into a single log statement to maintain clarity and avoid redundancy.
], | ||
}); | ||
req.log.debug(opportunityWithProjectInfo); | ||
req.log.debug("debug log opportunityWithProjectInfo") |
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 log message on line 86 ('debug log opportunityWithProjectInfo') is not associated with any specific data. Consider providing more context or data with this log message to make it more informative.
}, | ||
], | ||
}); | ||
req.log.info(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.
Consider removing the req.log.info(opportunity);
line if it is not necessary to log the opportunity
object separately from opportunityWithProjectInfo
. Logging both might lead to redundant information in the logs.
@@ -60,6 +61,27 @@ module.exports = (req, data, existingTransaction) => { | |||
.create(data, { transaction }); | |||
})) | |||
.then(async (opportunity) => { | |||
const copilotRequestWithProjectInfo = await models.CopilotRequest.findOne({ |
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 variable name copilotRequestWithProjectInfo
suggests that it contains both copilot request and project information. Ensure that the variable accurately reflects the data it holds, especially since the include
statement only specifies the Project
model.
@@ -60,6 +61,27 @@ module.exports = (req, data, existingTransaction) => { | |||
.create(data, { transaction }); | |||
})) | |||
.then(async (opportunity) => { | |||
const copilotRequestWithProjectInfo = await models.CopilotRequest.findOne({ | |||
where: { id: opportunity.copilotRequestId }, |
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 where
clause uses opportunity.copilotRequestId
. Verify that opportunity
has a property copilotRequestId
and that it is correctly set before this point in the code.
}, | ||
], | ||
}); | ||
req.log.info(copilotRequestWithProjectInfo); |
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 variable copilotRequestWithProjectInfo
is used here, but it is not defined in the provided diff. Ensure that this variable is correctly defined and initialized before this line.
}); | ||
req.log.info(copilotRequestWithProjectInfo); | ||
req.log.debug("debug log copilotRequestWithProjectInfo") | ||
const data = copilotRequestWithProjectInfo.data; |
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 variable copilotRequestWithProjectInfo
is used to access .data
, but it is not clear from the diff if this variable is correctly structured to have a .data
property. Verify that copilotRequestWithProjectInfo
has the expected structure.
@@ -71,6 +93,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(opportunity.type), | |||
opportunity_title: copilotRequestWithProjectInfo.project.name, |
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 variable copilotRequestWithProjectInfo
should be checked to ensure it is defined and has the expected structure before accessing project.name
. This will prevent potential runtime errors if the structure of copilotRequestWithProjectInfo
is not as expected.
@@ -71,6 +79,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(opportunity.type), | |||
opportunity_title: data.opportunityTitle, |
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 variable data.opportunityTitle
is being used here. Ensure that data.opportunityTitle
is always defined and has the correct value before this line is executed. Consider adding validation earlier in the code to prevent potential runtime errors.
where: { id: opportunity.copilotRequestId }, | ||
}); | ||
req.log.info(copilotRequestWithProjectInfo); | ||
req.log.info(opportunity.copilotRequestId); |
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 you're logging opportunity.copilotRequestId
here. Ensure that this log is necessary for debugging or monitoring purposes, as excessive logging can clutter log files and make it harder to find important information.
req.log.info(copilotRequestWithProjectInfo); | ||
req.log.info(opportunity.copilotRequestId); | ||
req.log.debug("debug log copilotRequestWithProjectInfo") | ||
const data = copilotRequestWithProjectInfo.data; |
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 variable data
is being redeclared here, which can lead to confusion or errors. Consider using a different variable name to avoid shadowing the data
parameter of the function.
@@ -16,7 +17,7 @@ const resolveTransaction = (transaction, callback) => { | |||
}; | |||
|
|||
module.exports = (req, data, existingTransaction) => { | |||
const { projectId, copilotRequestId } = data; | |||
const { projectId, copilotRequestId, opportunityTitle, type } = data; |
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 destructuring assignment now includes opportunityTitle
and type
, but it's unclear how these new variables are used in the function. Ensure that they are necessary for the logic and are utilized appropriately within the function body.
@@ -71,6 +72,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(type), |
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 variable type
is used here, but it is not clear from the diff if type
is defined or correctly assigned a value. Ensure that type
is defined and holds the expected value before using it in getCopilotTypeLabel(type)
.
@@ -71,6 +72,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(type), | |||
opportunity_title: opportunityTitle, |
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 variable opportunityTitle
is used here, but it is not clear from the diff if opportunityTitle
is defined or correctly assigned a value. Ensure that opportunityTitle
is defined and holds the expected value before using it.
@@ -98,6 +98,7 @@ module.exports = [ | |||
createdBy: req.authUser.userId, | |||
updatedBy: req.authUser.userId, | |||
type: copilotRequest.data.projectType, | |||
opportunityTitle: copilotRequest.data.opportunityTitle, |
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 copilotRequest.data.opportunityTitle
is always defined and valid before using it here. Consider adding validation or a default value to prevent potential errors if opportunityTitle
is missing or undefined.
@@ -71,6 +72,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(type), | |||
opportunity_title: opportunityTitle, | |||
start_date: moment.utc(startDate).format(), |
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 variable startDate
is used here, but it seems like data.startDate
was used previously. Ensure that startDate
is defined and accessible in this context, or if data.startDate
was intended, consider reverting to the original.
@@ -98,6 +98,8 @@ module.exports = [ | |||
createdBy: req.authUser.userId, | |||
updatedBy: req.authUser.userId, | |||
type: copilotRequest.data.projectType, | |||
opportunityTitle: copilotRequest.data.opportunityTitle, | |||
startDate: copilotRequest.data.startDate, |
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 copilotRequest.data.startDate
is properly validated and sanitized before being used here to prevent potential security issues or data inconsistencies.
@@ -1,11 +1,13 @@ | |||
import _ from 'lodash'; | |||
import config from 'config'; | |||
import moment from 'moment'; | |||
import { Op } from 'sequelize'; |
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 import statement for Op
from 'sequelize' has been moved but not modified. Consider reverting the change if it was unintentional, as it does not affect functionality.
@@ -71,6 +73,9 @@ module.exports = (req, data, existingTransaction) => { | |||
user_name: subject.handle, | |||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | |||
work_manager_url: config.get('workManagerUrl'), | |||
opportunity_type: getCopilotTypeLabel(type), | |||
opportunity_title: opportunityTitle, | |||
start_date: moment.utc(startDate).format("YYYY-MM-DD HH:mm:ss [UTC]"), |
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 date format has been changed from DD-MM-YYYY h:mm:ss a
to YYYY-MM-DD HH:mm:ss [UTC]
. Ensure that this new format is compatible with all systems and services that consume this data, as it may affect parsing or display logic.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1494