Skip to content

feat(PM-1173): Notify all copilots on copilot opportunity #815

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

Merged
merged 23 commits into from
Jun 10, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
Prev Previous commit
Next Next commit
fix: send emails to PMs and creator
  • Loading branch information
hentrymartin committed Jun 10, 2025
commit cc5a28d95be2cea5df2dae1bcee89e14b2ab31bf
34 changes: 32 additions & 2 deletions src/routes/copilotOpportunityApply/create.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import _ from 'lodash';
import validate from 'express-validation';
import Joi from 'joi';
import config from 'config';

import models from '../../models';
import util from '../../util';
import { PERMISSION } from '../../permissions/constants';
import { COPILOT_OPPORTUNITY_STATUS } from '../../constants';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_OPPORTUNITY_STATUS } from '../../constants';

const applyCopilotRequestValidations = {
body: Joi.object().keys({
Expand Down Expand Up @@ -65,7 +66,36 @@ module.exports = [
}

return models.CopilotApplication.create(data)
.then((result) => {
.then(async (result) => {
const pmRole = await util.getRolesByRoleName('Project Manager', req.log, req.id);

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.getRolesByRoleName call to manage potential failures or exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use enum/constant for the role name. Hardcoding it isn't good practice.

const { subjects = [] } = await util.getRoleInfo(pmRole[0], req.log, req.id);

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 call to manage potential failures or exceptions.


const creator = await util.getMemberDetailsByUserIds([req.authUser.userId], req.log, req.id);

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.getMemberDetailsByUserIds call to manage potential failures or exceptions.

const listOfSubjects = subjects;
if (creator) {
const isCreatorPartofSubjects = subjects.find(item => item.email === creator[0].email);

Choose a reason for hiding this comment

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

The variable isCreatorPartofSubjects could be renamed to isCreatorPartOfSubjects to follow camelCase convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we compare lower case values here? Coul be some issues if we are not.

if (!isCreatorPartofSubjects) {
listOfSubjects.push({

Choose a reason for hiding this comment

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

It seems like creator is being accessed as an array with creator[0], but previously it was accessed as an object with creator.email. Ensure that creator is consistently treated as an array or an object throughout the code to avoid potential runtime errors.

email: creator.email,

Choose a reason for hiding this comment

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

There is a potential issue here: creator.email should be creator[0].email to match the structure used in the isCreatorPartofSubjects check.

handle: creator.handle,
});
}
}

const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL;
const copilotPortalUrl = config.get('copilotPortalUrl');
listOfSubjects.forEach((subject) => {

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 createEvent function call to manage potential failures or exceptions.

createEvent(emailEventType, {
data: {
user_name: subject.handle,
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}#applications`,
},
sendgrid_template_id: "d-d7c1f48628654798a05c8e09e52db14f",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please conver sendgrid remplate id to constant.

recipients: [subject.email],
version: 'v3',
}, req.log);
});

res.status(201).json(result);
return Promise.resolve();
})
Expand Down
2 changes: 0 additions & 2 deletions src/routes/copilotRequest/approveRequest.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,10 @@ module.exports = (req, data, existingTransaction) => {
}))
Copy link

Choose a reason for hiding this comment

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

The console.log(opportunity); statement has been removed, which is good for production code, but ensure that any necessary logging is still in place for debugging purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to use console here instead of the req.log?

.then(async (opportunity) => {
Copy link

Choose a reason for hiding this comment

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

Consider removing the console.log(opportunity); statement if it's not needed for debugging purposes. Leaving console logs in production code can lead to performance issues and cluttered logs.

const roles = await util.getRolesByRoleName('copilot', req.log, req.id);
Copy link

Choose a reason for hiding this comment

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

The function getRolesByRoleName now takes additional parameters req.log and req.id. Ensure that req is defined and available in this context to avoid potential runtime errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Conver role name to enum/constant. Do not use hardcoded names.

req.log.info("getting subjects for roles", roles[0]);
const { subjects = [] } = await util.getRoleInfo(roles[0], req.log, req.id);
Copy link

Choose a reason for hiding this comment

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

The console.log(roles, roleInfo, 'roles by copilot'); statement has been removed. Make sure that the necessary information is logged elsewhere if needed for debugging or auditing purposes.

Choose a reason for hiding this comment

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

The log message for getting subjects for roles has been removed. If this was intentional, ensure that the removal does not affect the debugging process. If not, consider reinstating it for better traceability.

const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL;
Copy link

Choose a reason for hiding this comment

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

The change of emailEventType from CONNECT_NOTIFICATION_EVENT.COPILOT_OPPORTUNITY_CREATED to CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL should be verified to ensure it aligns with the intended notification logic. This change might affect how the event is categorized or processed.

const copilotPortalUrl = config.get('copilotPortalUrl');
req.log.info("Sending emails to all copilots about new opportunity");
req.log.info(`${copilotPortalUrl}/opportunity/${opportunity.id}`, '`${copilotPortalUrl}/opportunity/${opportunity.id}`');
subjects.forEach(subject => {
Copy link

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 subjects might be an empty array to avoid unnecessary log entries or operations.

req.log.info("Each copilot members", subject);
Copy link

Choose a reason for hiding this comment

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

The log message 'Each copilot members' could be more descriptive. Consider including more context, such as the copilot's handle or email, to make the logs more informative.

Choose a reason for hiding this comment

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

The log message for the copilot portal URL has been removed. If this was intentional, ensure that the removal does not affect the ability to trace the URL being used. If not, consider reinstating it for better traceability.

createEvent(emailEventType, {

Choose a reason for hiding this comment

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

The log statement 'Each copilot members' was removed. If this was intentional, ensure that the logging is still sufficient for debugging purposes. If not, consider adding a relevant log statement to track each copilot being notified.

Expand Down