PROD RELEASE - Copilot Portal updates #852
Conversation
feat(PM-1611): send email when opportunity is completed or canceled
fix(PM-1650): Added sorting to copilot requests API
fix(PM-1611): Url in cancel and completion email
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| CopliotRequest.associate = (models) => { | ||
| CopliotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' }); | ||
| CopilotRequest.associate = (models) => { |
There was a problem hiding this comment.
Typo in the model name: CopliotRequest should be corrected to CopilotRequest.
| CopilotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' }); | ||
| CopilotRequest.belongsTo(models.Project, { as: 'project', foreignKey: 'projectId' }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Typo in the return statement: CopliotRequest should be corrected to CopilotRequest.
|
|
||
| const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id); | ||
|
|
||
| users.forEach(async (user) => { |
There was a problem hiding this comment.
Using forEach with an async function does not wait for the asynchronous operations to complete. Consider using a for...of loop or Promise.all to ensure all emails are sent before proceeding.
| const requestData = copilotRequest.data; | ||
| createEvent(emailEventType, { | ||
| data: { | ||
| opportunity_details_url: copilotPortalUrl, |
There was a problem hiding this comment.
The opportunity_details_url is set to copilotPortalUrl, which might not be specific to the opportunity. Ensure this URL is correct and points to the intended resource.
| opportunity_details_url: copilotPortalUrl, | ||
| work_manager_url: config.get('workManagerUrl'), | ||
| opportunity_title: requestData.opportunityTitle, | ||
| user_name: user ? user.handle : "", |
There was a problem hiding this comment.
Consider handling the case where user might be null or undefined more explicitly to avoid potential errors.
| }); | ||
|
|
||
| // Send email to all applicants about opportunity completion | ||
| await sendEmailToAllApplicants(copilotRequest, otherApplications); |
There was a problem hiding this comment.
Consider handling potential errors from sendEmailToAllApplicants using a try-catch block to ensure that any issues with sending emails do not affect the subsequent operations.
| @@ -1,9 +1,11 @@ | |||
| import _ from 'lodash'; | |||
| import { Op } from 'sequelize'; | |||
| import config from 'config'; | |||
There was a problem hiding this comment.
Consider checking if the config module is actually used in this file. If not, it might be unnecessary to import it.
| import util from '../../util'; | ||
| import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, RESOURCES } from '../../constants'; | ||
| import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, RESOURCES, TEMPLATE_IDS } from '../../constants'; | ||
| import { createEvent } from '../../services/busApi'; |
There was a problem hiding this comment.
Ensure that the createEvent function from busApi is used in this file. If not, the import might be redundant.
| 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.
Consider using Promise.all to handle the asynchronous operations in the forEach loop. The current implementation may not wait for all emails to be sent before proceeding, which could lead to unexpected behavior.
| opportunity_details_url: copilotPortalUrl, | ||
| work_manager_url: config.get('workManagerUrl'), | ||
| opportunity_title: requestData.opportunityTitle, | ||
| user_name: user ? user.handle : "", |
There was a problem hiding this comment.
The ternary operation user ? user.handle : "" could be simplified by using optional chaining and nullish coalescing: user?.handle ?? "".
| invite.toJSON()); | ||
| } | ||
|
|
||
| await sendEmailToAllApplicants(copilotRequest, applications) |
There was a problem hiding this comment.
It seems like the function sendEmailToAllApplicants is being called without checking if applications is not empty or null. Consider adding a condition to ensure applications contains valid data before proceeding with the email sending operation to avoid potential errors.
| } | ||
|
|
||
| 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.
Consider defining DEFAULT_PAGE_SIZE explicitly in this file or importing it from a constants file to ensure clarity and maintainability.
| const sortParams = sort.split(' '); | ||
| let sortParams = sort.split(' '); | ||
| let order = [[sortParams[0], sortParams[1]]]; | ||
| const relationBasedSortParams = ['projectName']; |
There was a problem hiding this comment.
The variable relationBasedSortParams is defined but only used for checking inclusion. Consider adding a comment or documentation to clarify its purpose and usage.
| let sortParams = sort.split(' '); | ||
| let order = [[sortParams[0], sortParams[1]]]; | ||
| const relationBasedSortParams = ['projectName']; | ||
| const jsonBasedSortParams = ['opportunityTitle', 'projectType']; |
There was a problem hiding this comment.
The variable jsonBasedSortParams is defined but only used for checking inclusion. Consider adding a comment or documentation to clarify its purpose and usage.
|
|
||
| if (jsonBasedSortParams.includes(sortParams[0])) { | ||
| order = [ | ||
| [models.sequelize.literal(`("CopilotRequest"."data"->>'${sortParams[0]}')`), sortParams[1]], |
There was a problem hiding this comment.
Using sequelize.literal for JSON-based sorting can be error-prone if the JSON structure changes. Ensure that the JSON keys are consistently structured or add validation to handle potential errors.
| offset, | ||
| distinct: true, | ||
| subQuery: false, | ||
| }).then(({rows: copilotRequests, count}) => util.setPaginationHeaders(req, res, { |
There was a problem hiding this comment.
Ensure that util.setPaginationHeaders correctly handles pagination headers and edge cases, such as when count is zero or page exceeds the total number of pages.
2 participants
https://topcoder.atlassian.net/browse/PM-1582