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 15 commits
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
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ workflows:
context : org-global
filters:
branches:
only: ['develop', 'migration-setup']
only: ['develop', 'migration-setup', 'pm-1173']
- deployProd:
context : org-global
filters:
Expand Down
1 change: 1 addition & 0 deletions config/default.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
"inviteEmailSubject": "You are invited to Topcoder",
"inviteEmailSectionTitle": "Project Invitation",
"workManagerUrl": "https://challenges.topcoder-dev.com",
"copilotPortalUrl": "https://copilots.topcoder-dev.com",
"accountsAppUrl": "https://accounts.topcoder-dev.com",
"MAX_REVISION_NUMBER": 100,
"UNIQUE_GMAIL_VALIDATION": false,
Expand Down
1 change: 1 addition & 0 deletions config/development.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"pubsubExchangeName": "dev.projects",
"attachmentsS3Bucket": "topcoder-dev-media",
"workManagerUrl": "https://challenges.topcoder-dev.com",
"copilotPortalUrl": "https://copilots.topcoder-dev.com",
"fileServiceEndpoint": "https://api.topcoder-dev.com/v5/files",
"memberServiceEndpoint": "https://api.topcoder-dev.com/v5/members",
"identityServiceEndpoint": "https://api.topcoder-dev.com/v3/",
Expand Down
1 change: 1 addition & 0 deletions config/production.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"authDomain": "topcoder.com",
"workManagerUrl": "https://challenges.topcoder.com",
"copilotPortalUrl": "https://copilots.topcoder.com",
"sfdcBillingAccountNameField": "Billing_Account_name__c",
"sfdcBillingAccountMarkupField": "Mark_up__c",
"sfdcBillingAccountActiveField": "Active__c"
Expand Down
3 changes: 3 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,9 @@ export const CONNECT_NOTIFICATION_EVENT = {
TOPIC_UPDATED: 'connect.notification.project.topic.updated',
POST_CREATED: 'connect.notification.project.post.created',
POST_UPDATED: 'connect.notification.project.post.edited',

// External action email
EXTERNAL_ACTION_EMAIL: '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 from COPILOT_OPPORTUNITY_CREATED to EXTERNAL_ACTION_EMAIL seems to alter the meaning of the event. Ensure that this change aligns with the intended functionality described in the pull request. If the intention is to notify all copilots, verify that the event name accurately reflects this purpose.

};

export const REGEX = {
Expand Down
2 changes: 1 addition & 1 deletion src/routes/copilotRequest/approveRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ module.exports = [
updatedBy: req.authUser.userId,
});

return approveRequest(data)
return approveRequest(req, data)
Copy link

Choose a reason for hiding this comment

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

The approveRequest function signature has changed to include req as a parameter. Ensure that the approveRequest function is updated to handle this new parameter correctly. If req is not needed in the function, consider removing it to maintain the original function signature.

.then(_newCopilotOpportunity => res.status(201).json(_newCopilotOpportunity))
.catch((err) => {
if (err.message) {
Expand Down
32 changes: 30 additions & 2 deletions src/routes/copilotRequest/approveRequest.service.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import _ from 'lodash';
import config from 'config';
Copy link

Choose a reason for hiding this comment

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

Consider adding a check to ensure that the 'config' module is correctly configured and contains the necessary properties before using it. This can help prevent runtime errors if the configuration is missing or incorrect.


import models from '../../models';
import { COPILOT_REQUEST_STATUS } from '../../constants';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_REQUEST_STATUS } from '../../constants';
import util from '../../util';
import { createEvent } from '../../services/busApi';
Copy link

Choose a reason for hiding this comment

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

Ensure that the 'createEvent' function from 'busApi' is used correctly later in the code. If not already done, verify that it handles errors and edge cases appropriately.


const resolveTransaction = (transaction, callback) => {
if (transaction) {
Expand All @@ -11,7 +14,7 @@ const resolveTransaction = (transaction, callback) => {
return models.sequelize.transaction(callback);
};

module.exports = (data, existingTransaction) => {
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 function signature has been changed to include req as a parameter. Ensure that req is used appropriately within the function. If req is not used, consider removing it to avoid confusion.

const { projectId, copilotRequestId } = data;

return resolveTransaction(existingTransaction, transaction =>
Expand Down Expand Up @@ -52,6 +55,31 @@ module.exports = (data, existingTransaction) => {
return models.CopilotOpportunity
.create(data, { transaction });
}))
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);

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}`');
Copy link

Choose a reason for hiding this comment

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

The log statement seems to have a redundant string repetition. The second argument appears to be a duplicate of the first. Consider removing the duplicate string to avoid unnecessary repetition.

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.

data: {
user_name: subject.handle,
Copy link

Choose a reason for hiding this comment

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

The key handle has been changed to user_name. Ensure that this change is compatible with the rest of the system and that any downstream dependencies are updated accordingly.

Choose a reason for hiding this comment

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

Consider using camelCase for consistency with JavaScript naming conventions. Change user_name to userName.

opportunityDetailsUrl: `${copilotPortalUrl}/opportunity/${opportunity.id}`,
},
sendgrid_template_id: "d-3efdc91da580479d810c7acd50a4c17f",
Copy link

Choose a reason for hiding this comment

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

The addition of sendgrid_template_id should be checked to confirm that the template ID d-3efdc91da580479d810c7acd50a4c17f is correct and that it exists in SendGrid. Also, verify that it matches the intended email format and content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Move the template id to constant.

recipients: [subject.email],
version: '433b1688-c543-4656-a295-efcbea57444d',
Copy link

Choose a reason for hiding this comment

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

The version field has been changed from 'v3' to '433b1688-c543-4656-a295-efcbea57444d'. Ensure that this version identifier is correct and that it corresponds to the expected versioning system or API.

}, req.log);
});

req.log.info("Finished sending emails to copilots");

return opportunity;
})
.catch((err) => {
transaction.rollback();
return Promise.reject(err);
Expand Down
2 changes: 1 addition & 1 deletion src/routes/copilotRequest/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ module.exports = [
updatedBy: req.authUser.userId,
type: copilotRequest.data.projectType,
});
return approveRequest(approveData, transaction).then(() => copilotRequest);
return approveRequest(req, approveData, transaction).then(() => copilotRequest);
Copy link

Choose a reason for hiding this comment

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

The approveRequest function signature has changed to include req as an additional parameter. Ensure that the approveRequest function is updated accordingly to handle this new parameter, and verify that all calls to approveRequest throughout the codebase are updated to match this new signature.

}).then(copilotRequest => res.status(201).json(copilotRequest))
.catch((err) => {
try {
Expand Down
48 changes: 48 additions & 0 deletions src/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,54 @@ const projectServiceUtils = {
}
},

getRoleInfo: Promise.coroutine(function* (roleId, logger, requestId) { // eslint-disable-line func-names
Copy link

Choose a reason for hiding this comment

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

Consider using async/await syntax instead of Promise.coroutine for better readability and modern JavaScript practices.

try {
const token = yield this.getM2MToken();
const httpClient = this.getHttpClient({ id: requestId, log: logger });
httpClient.defaults.timeout = 6000;
Copy link

Choose a reason for hiding this comment

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

Setting a timeout for the HTTP client is a good practice to prevent hanging requests. However, ensure that 6000 milliseconds is an appropriate timeout for all expected network conditions and use cases. Consider making this configurable if different timeouts might be needed in different environments.

logger.debug(`${config.identityServiceEndpoint}roles/${roleId}`, "fetching role info");
return httpClient.get(`${config.identityServiceEndpoint}roles/${roleId}`, {
params: {
fields: `subjects`,
},
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${token}`,
},
}).then((res) => {
logger.debug(`Role info by ${roleId}: ${JSON.stringify(res.data.result.content)}`);
return _.get(res, 'data.result.content', []);
Copy link

Choose a reason for hiding this comment

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

The use of _.get is good for safely accessing nested properties, but ensure that lodash is imported as _ at the top of the file if not already done.

});
} catch (err) {
logger.debug(err, "error on getting role info");
Copy link

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 error message to provide better context for debugging. For example, include information about the operation that failed or the parameters involved.

return Promise.reject(err);
Copy link

Choose a reason for hiding this comment

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

Instead of returning Promise.reject(err), consider using throw err; to maintain consistency with async/await error handling patterns.

}
}),

getRolesByRoleName: Promise.coroutine(function* (roleName, logger, requestId) { // eslint-disable-line func-names
Copy link

Choose a reason for hiding this comment

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

Consider using async/await instead of Promise.coroutine for better readability and modern syntax.

try {
const token = yield this.getM2MToken();
const httpClient = this.getHttpClient({ id: requestId, log: logger });
httpClient.defaults.timeout = 6000;
Copy link

Choose a reason for hiding this comment

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

Setting a timeout for the HTTP client is a good practice to prevent hanging requests. However, consider making the timeout value configurable through an environment variable or configuration file instead of hardcoding it. This would allow for easier adjustments in different environments or scenarios.

return httpClient.get(`${config.identityServiceEndpoint}roles`, {
params: {
filter: `roleName=${roleName}`,
},
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${token}`,
},
}).then((res) => {
logger.debug(`Roles by ${roleName}: ${JSON.stringify(res.data.result.content)}`);
return _.get(res, 'data.result.content', [])
.filter(item => item.roleName === roleName)
Copy link

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 than item to improve code readability. For example, you could use role or roleItem to indicate that this represents a role object.

.map(r => r.id);
});
} catch (err) {
return Promise.reject(err);
Copy link

Choose a reason for hiding this comment

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

Instead of catching the error and returning a rejected promise, consider allowing the error to propagate naturally. This will make the function easier to handle with async/await syntax.

}
}),

/**
* Retrieve member details from userIds
*/
Expand Down