-
Notifications
You must be signed in to change notification settings - Fork 55
PM 2321 - submit copilot request fails #858
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
CONNECT_NOTIFICATION_EVENT, | ||
COPILOT_OPPORTUNITY_STATUS, | ||
COPILOT_REQUEST_STATUS, | ||
TEMPLATE_IDS, USER_ROLE } from '../../constants'; |
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 formatting the import statement so that each constant is on a separate line for better readability, like the other constants in this block.
})) | ||
.then(async (opportunity) => { | ||
const roles = await util.getRolesByRoleName(USER_ROLE.TC_COPILOT, req.log, req.id); | ||
const { subjects = [] } = await util.getRoleInfo(roles[0], req.log, req.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 adding error handling for the await util.getRoleInfo(roles[0], req.log, req.id);
call to manage potential promise rejections.
await Promise.all(notificationPromises); | ||
|
||
// send email to notify via slack | ||
sendNotification('Copilots', config.copilotsSlackEmail); |
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 call to sendNotification('Copilots', config.copilotsSlackEmail);
is redundant here because it is already included in the notificationPromises
array and awaited with Promise.all(notificationPromises);
. Consider removing this line to avoid sending duplicate notifications.
})) | ||
.then(async (opportunity) => { | ||
// eslint-disable-next-line no-console | ||
console.time('getRolesByRoleName'); |
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 console.time
for performance measurement in production code is not recommended. Consider using a more robust logging or monitoring solution.
console.time('getRolesByRoleName'); | ||
const roles = await util.getRolesByRoleName(USER_ROLE.TC_COPILOT, req.log, req.id); | ||
// eslint-disable-next-line no-console | ||
console.timeEnd('getRolesByRoleName'); |
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 console.timeEnd
for performance measurement in production code is not recommended. Consider using a more robust logging or monitoring solution.
Adds await to emails being sent