-
Notifications
You must be signed in to change notification settings - Fork 55
feat(PM-1510): Send Email notification to PM/creator once copilot accepts invite #839
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
| }); | ||
| } | ||
|
|
||
| await t.commit(); |
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 t.commit() operation to ensure that any issues during the commit process are properly caught and managed. This will help prevent potential issues if the commit fails.
|
|
||
| req.log.debug(`updated all copilot applications`); | ||
|
|
||
| await _transaction.commit(); |
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 around the _transaction.commit() call to ensure that any issues during the commit process are properly managed. This could help prevent potential data inconsistencies or application crashes.
| updatedProps = _.pick(updatedProps, ['isPrimary', 'role']); | ||
| const fields = req.query.fields ? req.query.fields.split(',') : null; | ||
|
|
||
| let previousValue; |
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 await with models.ProjectMember.findOne since the transaction function is now asynchronous. This will ensure that the promise is resolved before proceeding.
| }) | ||
| .catch(err => next(err))); | ||
| .catch(async (err) => { | ||
| await _transaction.rollback(); |
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 the result of _transaction.rollback() for errors or exceptions to ensure that the rollback was successful.
| return false; | ||
| } | ||
|
|
||
| return item.email.toLowerCase() === creator.email.toLowerCase(); |
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 creator is being accessed as an object instead of an array. Ensure that creator is always an object and not an array to avoid runtime errors.
|
|
||
| const opportunity = await models.CopilotOpportunity.findOne({ | ||
| where: { | ||
| id: application.opportunityId, |
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 a null check for application before accessing application.opportunityId to prevent potential runtime errors if application is not found.
| where: { | ||
| id: application.opportunityId, | ||
| }, | ||
| include: [ |
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 database query to ensure that any issues with fetching the opportunity are properly managed.
| const creator = userDetails[0]; | ||
| const invitee = userDetails[1]; | ||
| const listOfSubjects = subjects; | ||
| if (creator && creator.email) { |
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 application is removed but is still referenced in the code. Ensure that the removal of this variable does not affect other parts of the code that might rely on it.
| return false; | ||
| } | ||
|
|
||
| return item.email.toLowerCase() === creator.email.toLowerCase(); |
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 opportunity is removed but is still referenced in the code. Ensure that the removal of this variable does not affect other parts of the code that might rely on it.
| const { subjects = [] } = await util.getRoleInfo(pmRole[0], req.log, req.id); | ||
|
|
||
| const creatorDetails = await util.getMemberDetailsByUserIds([opportunity.createdBy], req.log, req.id); | ||
| const inviteeDetails = await util.getMemberDetailsByUserIds([application.userId], 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.
The variable inviteeDetails is assigned using application.userId, but the previous code used invite.userId. Ensure that application.userId is the correct identifier for the invitee in this context.
| }, | ||
| }; | ||
|
|
||
| const completeAllCopilotRequests = async (req, projectId, _transaction, _member) => { |
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 _member parameter is added to the function signature but is not used within the function body. Consider removing it if it's not needed, or ensure it is utilized appropriately.
|
|
||
| await _transaction.commit(); | ||
|
|
||
| const memberDetails = await util.getMemberDetailsByUserIds([_member.userId], 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 checking if _member is defined before accessing userId to avoid potential runtime errors.
|
|
||
| req.log.debug(`Copilot request data: ${requestData}`); | ||
| const opportunity = copilotOpportunites.find(item => item.copilotRequestId === request.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.
Ensure opportunity is defined before accessing opportunity.id to prevent errors if find returns undefined.
src/routes/projectMembers/update.js
Outdated
| user_name: memberDetails ? memberDetails.handle : "", | ||
| }, | ||
| sendgrid_template_id: TEMPLATE_IDS.COPILOT_ALREADY_PART_OF_PROJECT, | ||
| recipients: [memberDetails.email], |
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 verifying that memberDetails.email is defined before using it as a recipient to avoid sending emails to undefined.
| .then(() => { | ||
| .then(async () => { | ||
| projectMember = projectMember.get({ plain: true }); | ||
| projectMember = _.omit(projectMember, ['deletedAt']); |
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 function completeAllCopilotRequests now has an additional parameter _member. Ensure that this parameter is necessary for the function's operation and that the function definition is updated accordingly to handle this new argument.
| transaction: _transaction, | ||
| }); | ||
|
|
||
| // If the invited member |
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 case when memberApplication is not found or the update fails. This will help ensure that the application status update process is robust and can handle unexpected scenarios gracefully.
| }); | ||
| } | ||
|
|
||
| req.log.debug(`updated all copilot applications`); |
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 updated all copilot applications might be misleading since the update operation for memberApplication is separate from allCopilotApplications. Consider revising the log message to accurately reflect the operations performed.
| if ((updatedProps.role === previousValue.role || action === 'complete-copilot-requests') && | ||
| (_.isUndefined(updatedProps.isPrimary) || | ||
| updatedProps.isPrimary === previousValue.isPrimary)) { | ||
| await completeAllCopilotRequests(req, projectId, _transaction, _member); |
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 function completeAllCopilotRequests now has an additional parameter _member. Ensure that this change is reflected in the function definition and that _member is being used appropriately within the function. If _member is not needed, consider removing it to maintain clarity and simplicity.
|
|
||
| req.log.debug(`updated all copilot applications`); | ||
|
|
||
| const memberDetails = await util.getMemberDetailsByUserIds([_member.userId], 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.
The removal of _transaction.commit() might lead to uncommitted changes in the database. Ensure that the transaction is being committed elsewhere or that this removal is intentional and safe.
| const allCopilotRequests = await models.CopilotRequest.findAll({ | ||
| where: { | ||
| projectId, | ||
| status: { |
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 the Op.in condition is necessary for the specific logic you are implementing. If the logic requires handling only specific statuses, ensure that the statuses included are correct and comprehensive for the intended functionality.
| req.log.debug(`updated all copilot applications`); | ||
|
|
||
| const memberDetails = await util.getMemberDetailsByUserIds([_member.userId], req.log, req.id); | ||
| const member = memberDetails[0]; |
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 memberDetails array to ensure it contains elements before accessing the first element. This will prevent potential runtime errors if the array is empty.
| opportunity_type: getCopilotTypeLabel(requestData.projectType), | ||
| opportunity_title: requestData.opportunityTitle, | ||
| start_date: moment.utc(requestData.startDate).format('DD-MM-YYYY'), | ||
| user_name: member ? member.handle : "", |
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 a null check for member before accessing member.handle to prevent potential runtime errors if member is undefined.
| user_name: member ? member.handle : "", | ||
| }, | ||
| sendgrid_template_id: TEMPLATE_IDS.COPILOT_ALREADY_PART_OF_PROJECT, | ||
| recipients: [member.email], |
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 a null check for member before accessing member.email to prevent potential runtime errors if member is undefined.
| version: 'v3', | ||
| }, req.log); | ||
|
|
||
| req.log.debug(`Sent email to ${member.email}`); |
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 a null check for member before accessing member.email to prevent potential runtime errors if member is undefined.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1510