Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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', 'pm-1168']
only: ['develop', 'migration-setup']
- deployProd:
context : org-global
filters:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module.exports = {
up: async (queryInterface, Sequelize) => {
await queryInterface.addColumn('project_member_invites', 'applicationId', {

Choose a reason for hiding this comment

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

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.

type: Sequelize.BIGINT,
allowNull: true,
references: {
model: 'copilot_applications',
key: 'id',
},
onUpdate: 'CASCADE',
onDelete: 'SET NULL',
});
},

down: async (queryInterface) => {
await queryInterface.removeColumn('project_member_invites', 'applicationId');
},
};
1 change: 1 addition & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const COPILOT_REQUEST_STATUS = {

export const COPILOT_APPLICATION_STATUS = {
PENDING: 'pending',
INVITED: 'invited',
ACCEPTED: 'accepted',
};

Expand Down
3 changes: 2 additions & 1 deletion src/models/projectMember.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({

Choose a reason for hiding this comment

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

Consider adding a default value for the transaction parameter t to ensure backward compatibility if the function is called without this argument.

where: {
deletedAt: { $eq: null },
projectId,
},
transaction: t,
raw: true,
});

Expand Down
10 changes: 10 additions & 0 deletions src/models/projectMemberInvite.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ module.exports = function defineProjectMemberInvite(sequelize, DataTypes) {
isEmail: true,
},
},
applicationId: {
type: DataTypes.BIGINT,
allowNull: true,

Choose a reason for hiding this comment

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

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.

references: {
model: 'copilot_applications',
key: 'id',
},
onUpdate: 'CASCADE',
onDelete: 'CASCADE',

Choose a reason for hiding this comment

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

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.

},
role: {
type: DataTypes.STRING,
allowNull: false,
Expand Down
99 changes: 85 additions & 14 deletions src/routes/copilotOpportunity/assign.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
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_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS } from '../../constants';
import { createEvent } from '../../services/busApi';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES } from '../../constants';

const assignCopilotOpportunityValidations = {
body: Joi.object().keys({
Expand Down Expand Up @@ -64,7 +66,7 @@ module.exports = [

const projectId = opportunity.projectId;
const userId = application.userId;
const activeMembers = await models.ProjectMember.getActiveProjectMembers(projectId);
const activeMembers = await models.ProjectMember.getActiveProjectMembers(projectId, t);

const existingUser = activeMembers.find(item => item.userId === userId);
if (existingUser && existingUser.role === 'copilot') {
Expand All @@ -73,20 +75,89 @@ module.exports = [
throw err;
}

await models.CopilotRequest.update(
{ status: COPILOT_REQUEST_STATUS.FULFILLED },
{ where: { id: opportunity.copilotRequestId }, transaction: t },
);
const project = await models.Project.findOne({
where: {
id: projectId,
},
transaction: t,

Choose a reason for hiding this comment

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

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.

});

const existingInvite = await models.ProjectMemberInvite.findAll({
where: {
userId,
projectId,
role: PROJECT_MEMBER_ROLE.COPILOT,
status: INVITE_STATUS.PENDING,
},
transaction: t,

Choose a reason for hiding this comment

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

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.

});

if (existingInvite && existingInvite.length) {

Choose a reason for hiding this comment

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

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.

const err = new Error(`User already has an pending invite to the project`);

Choose a reason for hiding this comment

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

The error message contains a grammatical error. It should be 'a pending invite' instead of 'an pending invite'.

err.status = 400;
throw err;
}

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

const invite = await models.ProjectMemberInvite.create({
status: INVITE_STATUS.PENDING,
role: PROJECT_MEMBER_ROLE.COPILOT,
userId,
projectId,
applicationId: application.id,
email: applicationUser[0].email,
createdBy: req.authUser.userId,
createdAt: new Date(),
updatedBy: req.authUser.userId,
updatedAt: new Date(),
}, {
transaction: t,
})

await opportunity.update(
{ status: COPILOT_OPPORTUNITY_STATUS.COMPLETED },
{ transaction: t },
);
util.sendResourceToKafkaBus(
req,
EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED,
RESOURCES.PROJECT_MEMBER_INVITE,
invite.toJSON());

await models.CopilotApplication.update(
{ status: COPILOT_APPLICATION_STATUS.ACCEPTED },
{ where: { id: applicationId }, transaction: t },
);
const authUserDetails = await util.getMemberDetailsByUserIds([req.authUser.userId], req.log, req.id);

const emailEventType = CONNECT_NOTIFICATION_EVENT.PROJECT_MEMBER_EMAIL_INVITE_CREATED;
await createEvent(emailEventType, {
data: {
workManagerUrl: config.get('workManagerUrl'),
accountsAppURL: config.get('accountsAppUrl'),
subject: config.get('inviteEmailSubject'),
projects: [{
name: project.name,
projectId,
sections: [
{
EMAIL_INVITES: true,
title: config.get('inviteEmailSectionTitle'),
projectName: project.name,
projectId,
initiator: authUserDetails[0],

Choose a reason for hiding this comment

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

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.

isSSO: util.isSSO(project),
},
],
}],
},
recipients: [applicationUser[0].email],
version: 'v3',
from: {
name: config.get('EMAIL_INVITE_FROM_NAME'),
email: config.get('EMAIL_INVITE_FROM_EMAIL'),
},
categories: [`${process.env.NODE_ENV}:${emailEventType}`.toLowerCase()],
}, req.log);

await application.update({

Choose a reason for hiding this comment

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

Consider checking if the application object exists before attempting to update it to avoid potential runtime errors if application is undefined.

status: COPILOT_APPLICATION_STATUS.INVITED,

Choose a reason for hiding this comment

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

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.

}, {
transaction: t,
});

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 the transaction t might not be available or valid. This could prevent potential issues during the database update operation.


res.status(200).send({ id: applicationId });
}).catch(err => next(err));
Expand Down
56 changes: 50 additions & 6 deletions src/routes/projectMemberInvites/update.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Choose a reason for hiding this comment

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

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.

import { PERMISSION } from '../../permissions/constants';

/**
Expand Down Expand Up @@ -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) => {

Choose a reason for hiding this comment

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

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.

req.context = req.context || {};
req.context.currentProjectMembers = members;
let userId = updatedInvite.userId;
Expand All @@ -117,10 +117,54 @@ module.exports = [
createdBy: req.authUser.userId,
updatedBy: req.authUser.userId,
};
return util
.addUserToProject(req, member)
.then(() => res.json(util.postProcessInvites('$.email', updatedInvite, req)))
.catch(err => next(err));
const t = await models.sequelize.transaction();

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 transaction creation to ensure that any issues during transaction initialization are caught and handled appropriately.

try {
await util.addUserToProject(req, member, t);
if (invite.applicationId) {

Choose a reason for hiding this comment

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

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.

const application = await models.CopilotApplication.findOne({

Choose a reason for hiding this comment

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

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.

where: {
id: invite.applicationId,
},
transaction: t,
});

await application.update({ status: COPILOT_APPLICATION_STATUS.ACCEPTED }, {
transaction: t
});

const opportunity = await models.CopilotOpportunity.findOne({

Choose a reason for hiding this comment

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

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.

where: {
id: application.opportunityId,
},
transaction: t,
});

await opportunity.update({
status: COPILOT_OPPORTUNITY_STATUS.COMPLETED
}, {
transaction: t,
});

const request = await models.CopilotRequest.findOne({

Choose a reason for hiding this comment

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

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.

where: {
id: opportunity.copilotRequestId,
},
transaction: t,
});

await request.update({
status: COPILOT_REQUEST_STATUS.FULFILLED
}, {
transaction: t,
});
}

await t.commit();
return res.json(util.postProcessInvites('$.email', updatedInvite, req));
} catch (e) {
await t.rollback();
return next(e);
}
});
}
return res.json(util.postProcessInvites('$.email', updatedInvite, req));
Expand Down