fix(PM-1168): invite user when assigning as copilot #809
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| RESOURCES.PROJECT_MEMBER_INVITE, | ||
| invite.toJSON()); | ||
|
|
||
| const initiator = await util.getMemberDetailsByUserIds([req.authUser.userId], req.log, req.id); |
There was a problem hiding this comment.
Check if util.getMemberDetailsByUserIds can return an empty result or error, and handle such cases to avoid potential runtime errors.
| email: existingUser.email, | ||
| }) | ||
|
|
||
| console.log("aftr create", invite) |
There was a problem hiding this comment.
Remove the console.log statement before merging the code. Debugging statements should not be present in production code.
| ], | ||
| }], | ||
| }, | ||
| recipients: [existingUser.email], |
There was a problem hiding this comment.
The change from invite.email to existingUser.email suggests that the email recipient is now being set to an existing user's email. Ensure that existingUser is properly defined and initialized before this line to avoid potential runtime errors.
| }, | ||
| }); | ||
|
|
||
| req.log.info("before create", existingUser) |
There was a problem hiding this comment.
Consider removing or modifying the log statement req.log.info("before create", existingUser) if it is not necessary for production. Logging sensitive information or excessive logging can lead to performance issues and security concerns.
| email: existingUser.email, | ||
| }) | ||
|
|
||
| req.log.info("aftr create", invite) |
There was a problem hiding this comment.
Consider correcting the typo in the log message from "aftr create" to "after create" for better readability and consistency.
| }, | ||
| }); | ||
|
|
||
| const applicationUser = await util.getMemberDetailsByUserIds([userId], req.log, req.id)[0]; |
There was a problem hiding this comment.
It seems like util.getMemberDetailsByUserIds([userId], req.log, req.id) returns a promise, but the code attempts to access the first element directly without awaiting the promise. Consider using await to ensure applicationUser is correctly assigned.
| email: applicationUser.email, | ||
| }) | ||
|
|
||
| req.log.info("aftr create", invite) |
There was a problem hiding this comment.
Typo in log message: 'aftr' should be corrected to 'after'.
| ], | ||
| }], | ||
| }, | ||
| recipients: [applicationUser.email], |
There was a problem hiding this comment.
It seems like the recipient email has been changed from existingUser.email to applicationUser.email. Ensure that applicationUser is correctly defined and initialized before this line, and that it contains the expected email address for the invitation process.
|
|
||
| const applicationUser = await util.getMemberDetailsByUserIds([userId], req.log, req.id); | ||
|
|
||
| req.log.info("before create", applicationUser, userId) |
There was a problem hiding this comment.
Consider verifying if userId is necessary in the log statement. If applicationUser already contains relevant information about the user, logging userId might be redundant.
|
|
||
| const applicationUser = await util.getMemberDetailsByUserIds([userId], req.log, req.id); | ||
|
|
||
| req.log.info("before create", applicationUser, userId, applicationUser[0].email) |
There was a problem hiding this comment.
Ensure that applicationUser[0] is not undefined before accessing .email to prevent potential runtime errors. Consider adding a check to verify that applicationUser[0] exists.
| @@ -0,0 +1,18 @@ | |||
| module.exports = { | |||
| up: async (queryInterface, Sequelize) => { | |||
| await queryInterface.addColumn('project_member_invites', 'applicationId', { | |||
There was a problem hiding this comment.
Consider specifying an index on the applicationId column for improved query performance, especially if this column will be frequently used in WHERE clauses or joins.
| }, | ||
| }); | ||
|
|
||
| if (existingInvite) { |
There was a problem hiding this comment.
The findAll method returns an array, so existingInvite will always be truthy even if it's empty. Consider using findOne if you expect a single result, or check if existingInvite.length > 0.
| }); | ||
|
|
||
| if (existingInvite) { | ||
| const err = new Error(`User already has an pending invite to the project`); |
There was a problem hiding this comment.
The error message contains a grammatical error. It should be 'a pending invite' instead of 'an pending invite'.
| title: config.get('inviteEmailSectionTitle'), | ||
| projectName: project.name, | ||
| projectId, | ||
| initiator: authUserDetails[0], |
There was a problem hiding this comment.
The removal of the code block that updates the status of CopilotRequest, opportunity, and CopilotApplication might lead to inconsistencies in the database. Ensure that these status updates are handled elsewhere if they are still necessary for the application's logic.
| @@ -119,7 +119,9 @@ module.exports = [ | |||
| }; | |||
| return util | |||
| .addUserToProject(req, member) | |||
| .then(() => res.json(util.postProcessInvites('$.email', updatedInvite, req))) | |||
| .then(async () => { | |||
There was a problem hiding this comment.
The use of async in the then block seems unnecessary here since there is no await expression inside the block. Consider removing async to simplify the code.
| @@ -44,11 +44,12 @@ module.exports = function defineProjectMember(sequelize, DataTypes) { | |||
| }) | |||
| .then(res => _.without(_.map(res, 'projectId'), null)); | |||
|
|
|||
| ProjectMember.getActiveProjectMembers = projectId => ProjectMember.findAll({ | |||
| ProjectMember.getActiveProjectMembers = (projectId, t) => ProjectMember.findAll({ | |||
There was a problem hiding this comment.
Consider adding a default value for the transaction parameter t to ensure backward compatibility if the function is called without this argument.
| @@ -13,6 +13,16 @@ module.exports = function defineProjectMemberInvite(sequelize, DataTypes) { | |||
| isEmail: true, | |||
| }, | |||
| }, | |||
| applicationId: { | |||
| type: DataTypes.BIGINT, | |||
| allowNull: true, | |||
There was a problem hiding this comment.
Consider adding a validation to ensure applicationId is a positive integer if it is not null. This can help prevent invalid data from being stored in the database.
| key: 'id', | ||
| }, | ||
| onUpdate: 'CASCADE', | ||
| onDelete: 'CASCADE', |
There was a problem hiding this comment.
Ensure that the onDelete: 'CASCADE' behavior aligns with the intended data integrity rules. This setting will automatically delete the invite if the associated copilot application is deleted, which may not always be desirable.
| where: { | ||
| id: projectId, | ||
| }, | ||
| transaction: t, |
There was a problem hiding this comment.
Ensure that the transaction t is properly initialized and managed. If t is not defined or handled correctly, it could lead to issues with database operations.
| role: PROJECT_MEMBER_ROLE.COPILOT, | ||
| status: INVITE_STATUS.PENDING, | ||
| }, | ||
| transaction: t, |
There was a problem hiding this comment.
The addition of transaction: t seems to be intended for ensuring that the invite creation is part of a transaction. Ensure that t is a valid transaction object and is correctly managed (e.g., committed or rolled back) in the surrounding code to prevent potential issues with database consistency.
| @@ -94,7 +94,7 @@ module.exports = [ | |||
| if (updatedInvite.status === INVITE_STATUS.ACCEPTED || | |||
| updatedInvite.status === INVITE_STATUS.REQUEST_APPROVED) { | |||
| return models.ProjectMember.getActiveProjectMembers(projectId) | |||
| .then((members) => { | |||
| .then(async (members) => { | |||
There was a problem hiding this comment.
Using async in a .then() callback can be confusing and is generally not recommended. Consider refactoring the code to use async/await throughout for better readability and consistency.
| .addUserToProject(req, member) | ||
| .then(() => res.json(util.postProcessInvites('$.email', updatedInvite, req))) | ||
| .catch(err => next(err)); | ||
| const t = await models.sequelize.transaction(); |
There was a problem hiding this comment.
Consider adding error handling for the transaction creation to ensure that any issues during transaction initialization are caught and handled appropriately.
| const t = await models.sequelize.transaction(); | ||
| try { | ||
| await util.addUserToProject(req, member, t); | ||
| if (invite.applicationId) { |
There was a problem hiding this comment.
The invite.applicationId check assumes that invite is always defined. Consider adding a check to ensure invite is not null or undefined before accessing its properties to prevent potential runtime errors.
| try { | ||
| await util.addUserToProject(req, member, t); | ||
| if (invite.applicationId) { | ||
| const application = await models.CopilotApplication.findOne({ |
There was a problem hiding this comment.
The findOne operation on CopilotApplication does not handle the case where no application is found. Consider adding a check to handle the scenario where application is null, which could prevent potential errors when attempting to update a non-existent record.
| transaction: t | ||
| }); | ||
|
|
||
| const opportunity = await models.CopilotOpportunity.findOne({ |
There was a problem hiding this comment.
Similar to the previous comment, ensure that the findOne operation on CopilotOpportunity handles the case where no opportunity is found. This will prevent errors when trying to update a null object.
| transaction: t, | ||
| }); | ||
|
|
||
| const request = await models.CopilotRequest.findOne({ |
There was a problem hiding this comment.
Ensure that the findOne operation on CopilotRequest handles the case where no request is found. This will prevent errors when trying to update a null object.
| transaction: t, | ||
| }); | ||
|
|
||
| console.log(existingInvite, 'existingInvite asdas') |
There was a problem hiding this comment.
Remove the console.log statement used for debugging purposes. Leaving it in production code can lead to unnecessary console output and potential performance issues.
|
|
||
| console.log(existingInvite, 'existingInvite asdas') | ||
|
|
||
| if (existingInvite && existingInvite.length) { |
There was a problem hiding this comment.
The condition existingInvite && existingInvite.length assumes existingInvite is an array. Ensure that existingInvite is always an array to avoid runtime errors. If existingInvite can be null or another type, consider using Array.isArray(existingInvite) && existingInvite.length for a more robust check.
| transaction: t, | ||
| }); | ||
|
|
||
| req.log.info(existingInvite, 'existingInvite asdas') |
There was a problem hiding this comment.
Consider removing or updating the log message 'existingInvite asdas' to be more descriptive and relevant to the context.
| @@ -4,7 +4,7 @@ import Joi from 'joi'; | |||
| import { middleware as tcMiddleware } from 'tc-core-library-js'; | |||
| import models from '../../models'; | |||
| import util from '../../util'; | |||
| import { INVITE_STATUS, EVENT, RESOURCES } from '../../constants'; | |||
| import { INVITE_STATUS, EVENT, RESOURCES, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS } from '../../constants'; | |||
There was a problem hiding this comment.
It seems like new constants COPILOT_OPPORTUNITY_STATUS and COPILOT_REQUEST_STATUS have been added to the import statement. Ensure these constants are actually used in the code below. If they are not used, consider removing them to keep the imports clean and maintainable.
| categories: [`${process.env.NODE_ENV}:${emailEventType}`.toLowerCase()], | ||
| }, req.log); | ||
|
|
||
| await application.update({ |
There was a problem hiding this comment.
Consider checking if the application object exists before attempting to update it to avoid potential runtime errors if application is undefined.
| }, req.log); | ||
|
|
||
| await application.update({ | ||
| status: COPILOT_APPLICATION_STATUS.INVITED, |
There was a problem hiding this comment.
Ensure that COPILOT_APPLICATION_STATUS.INVITED is a valid status and is defined in the context of the application. If not, this could lead to unexpected behavior.
| status: COPILOT_APPLICATION_STATUS.INVITED, | ||
| }, { | ||
| transaction: t, | ||
| }); |
There was a problem hiding this comment.
Consider handling the case where the transaction t might not be available or valid. This could prevent potential issues during the database update operation.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1168