Skip to content

fix(PM-1510): added existing membership to the copilot application #840

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
Jul 30, 2025

Conversation

hentrymartin
Copy link
Collaborator

TBA

@@ -9,11 +9,23 @@ const permissions = tcMiddleware.permissions;

module.exports = [
permissions('copilotApplications.view'),
(req, res, next) => {
async (req, res, next) => {

Choose a reason for hiding this comment

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

The function has been changed to an async function, but there is no error handling for the asynchronous operations. Consider using a try-catch block to handle potential errors from the await calls.

}
});

if (!opportunity) {

Choose a reason for hiding this comment

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

The error message 'No opportunity found' could be more descriptive to help with debugging. Consider including the opportunityId in the error message to provide more context.

@@ -41,7 +53,15 @@ module.exports = [
],
order: [[sortParams[0], sortParams[1]]],
})
.then(copilotApplications => res.json(copilotApplications))
.then(copilotApplications => {
return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {

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 opportunity.projectId might be undefined or null to prevent potential errors when calling getActiveProjectMembers.

return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
return res.json(copilotApplications.map(application => {
return Object.assign({}, application, {
existingMembership: members.find(m => m.userId === application.userId),

Choose a reason for hiding this comment

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

The find method will return undefined if no matching member is found. Ensure that the rest of the code handles this case appropriately, especially if existingMembership is expected to be an object.

@@ -45,11 +45,17 @@ module.exports = [
throw err;
}

const copilotRequest = await models.CopilotRequest.findOne({

Choose a reason for hiding this comment

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

Consider checking if copilotRequest is null or undefined after fetching it from the database. This will help prevent potential runtime errors if the request is not found.

const application = await models.CopilotApplication.findOne({
where: { id: applicationId, opportunityId: copilotOpportunityId },
transaction: t,
});


Choose a reason for hiding this comment

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

Remove the extra blank line here to maintain consistent formatting and readability.

if (existingMember) {
if (['copilot', 'manager'].includes(existingMember.role)) {

updateCopilotOpportunity();

Choose a reason for hiding this comment

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

The function updateCopilotOpportunity() is called without await. If this function performs asynchronous operations, consider using await to ensure it completes before proceeding.

projectMember.get({ plain: true }),
existingMember);

updateCopilotOpportunity();

Choose a reason for hiding this comment

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

Similar to line 109, updateCopilotOpportunity() is called without await. If this function is asynchronous, use await to ensure proper execution order.

};

const existingMember = activeMembers.find(item => item.userId === userId);
if (existingMember) {

Choose a reason for hiding this comment

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

The logic for checking existingMember and its role could be optimized. Consider combining the role check with the initial find operation to reduce complexity and improve readability.

updateCopilotOpportunity();

} else {
await models.ProjectMember.update({

Choose a reason for hiding this comment

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

The await keyword is used here, but the surrounding code does not handle potential errors that might arise from this asynchronous operation. Consider adding error handling to manage any exceptions that may occur.


const existingMember = activeMembers.find(item => item.userId === userId);
if (existingMember) {
req.log.debug(`User already part of project: ${JSON.stringify(existingMember)}`);

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 JSON.stringify(existingMember) might throw an error if existingMember contains circular references. You could use a try-catch block or a safe stringification method.

if (existingMember) {
req.log.debug(`User already part of project: ${JSON.stringify(existingMember)}`);
if (['copilot', 'manager'].includes(existingMember.role)) {
req.log.debug(`User is a copilot or manager`);

Choose a reason for hiding this comment

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

The debug log message User is a copilot or manager could be more descriptive by including the user's role. Consider logging the actual role for better traceability.

req.log.debug(`User is a copilot or manager`);
updateCopilotOpportunity();
} else {
req.log.debug(`User has read/write role`);

Choose a reason for hiding this comment

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

The debug log message User has read/write role might be misleading if the user has other roles. Consider specifying the exact role or roles the user has for clarity.

},
});

req.log.debug(`Updated project member: ${JSON.stringify(projectMember.get({plain: true}))}`);

Choose a reason for hiding this comment

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

Consider handling potential errors that might occur during the JSON.stringify operation. If projectMember.get({plain: true}) contains circular references or other non-serializable data, JSON.stringify could throw an error.

RESOURCES.PROJECT_MEMBER,
projectMember.get({ plain: true }),
existingMember);
req.log.debug(`Member updated in kafka`);

Choose a reason for hiding this comment

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

The log message Member updated in kafka could be more descriptive. Consider including additional context, such as the member ID or other relevant details, to make the log more informative.

.then(copilotApplications => {
return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
const applications = copilotApplications.get({plain: true});
req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);

Choose a reason for hiding this comment

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

Consider handling potential errors or exceptions that might occur during the JSON.stringify operation, especially if the members object contains circular references or non-serializable data.

return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
const applications = copilotApplications.get({plain: true});
req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(applications)}`);

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 applications can be safely serialized with JSON.stringify without causing runtime errors due to circular references or non-serializable data.

req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(applications)}`);
return res.json(applications.map(application => {
req.log.debug(`Existing member to application ${JSON.stringify(members.find(m => m.userId === application.userId))}`);

Choose a reason for hiding this comment

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

The debug log statement may produce a large output if members is a large array. Consider truncating the log or summarizing the information to avoid excessive logging, which could impact performance or readability.

@@ -31,19 +31,42 @@ module.exports = [
canAccessAllApplications ? {} : { createdBy: userId },
);

Choose a reason for hiding this comment

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

The opportunityId variable is used here, but it is not clear from the diff where it is defined or assigned. Ensure that opportunityId is properly defined and assigned a value before this line.

})
.then(copilotApplications => {
return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
const applications = copilotApplications.get({plain: true});

Choose a reason for hiding this comment

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

The copilotApplications.get({plain: true}) method is used, but copilotApplications is an array returned by findAll. This might cause an error since get is not a method on arrays. Consider iterating over copilotApplications and calling get on each element if needed.

order: [[sortParams[0], sortParams[1]]],
})
.then(copilotApplications => {
req.log.debug(`CopilotApplications ${JSON.stringify(copilotApplications)}`);

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 log message to clarify what copilotApplications represents in this context.

req.log.debug(`CopilotApplications ${JSON.stringify(copilotApplications)}`);
return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);

Choose a reason for hiding this comment

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

The log message for Applications is redundant with the previous log message for CopilotApplications. Consider removing one of them to avoid unnecessary duplication.

req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
return res.json(copilotApplications.map(application => {
req.log.debug(`Existing member to application ${JSON.stringify(members.find(m => m.userId === application.userId))}`);

Choose a reason for hiding this comment

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

The log message Existing member to application could be more descriptive to clearly indicate what is being logged. Consider rephrasing for better clarity.

return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => {
req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {

Choose a reason for hiding this comment

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

Consider using const for enrichedApplications only if it is not reassigned later in the code. If it is reassigned, let would be more appropriate.

req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {
req.log.debug(`Existing member to application ${JSON.stringify(members.find(m => m.userId === application.userId))}`);

Choose a reason for hiding this comment

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

The debug log message could be more descriptive. Consider specifying which application is being processed by including relevant details from the application object.

});
});

req.log.debug(`Enriched Applications ${JSON.stringify(enrichedApplications)}`);

Choose a reason for hiding this comment

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

The debug log message could be more informative by including the number of enriched applications. Consider adding enrichedApplications.length to the log message.

req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {
req.log.debug(`Existing member to application ${JSON.stringify()}`);

Choose a reason for hiding this comment

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

The JSON.stringify() function is called without any arguments, which will result in an empty string being logged. Consider passing the object you intend to stringify.

req.log.debug(`Fetched existing active members ${JSON.stringify(members)}`);
req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {
const m = members.find(m => m.userId === application.userId);

Choose a reason for hiding this comment

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

The debug log statement was removed but the comment remains. Consider removing the comment as it is not necessary to keep commented-out code.

});

req.log.debug(`Enriched Applications ${JSON.stringify(enrichedApplications)}`);
res.status(200).send(enrichedApplications);

Choose a reason for hiding this comment

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

The response method has been changed from res.json to res.status(200).send. Ensure that this change is intentional and that send is the appropriate method for sending the enriched applications. If res.json was previously used to automatically set the content-type to application/json, verify that send achieves the same result.

req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {
const m = members.find(m => m.userId === application.userId);
req.log.debug(`Existing member to application ${JSON.stringify(Object.assign({}, application, {

Choose a reason for hiding this comment

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

Consider using the spread operator instead of Object.assign for better readability and consistency with modern JavaScript practices. Example: { ...application, existingMembership: m }.

req.log.debug(`Applications ${JSON.stringify(copilotApplications)}`);
const enrichedApplications = copilotApplications.map(application => {
const m = members.find(m => m.userId === application.userId);
req.log.debug(`Existing member to application ${JSON.stringify(Object.assign({}, application, m ? {

Choose a reason for hiding this comment

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

Consider using the spread operator for merging objects instead of Object.assign for better readability and performance. For example: {...application, ...(m ? { existingMembership: m } : {})}.

const enrichedApplications = copilotApplications.map(application => {
const m = members.find(m => m.userId === application.userId);

// Using spread operator fails in lint check

Choose a reason for hiding this comment

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

Consider removing the commented-out code if it is no longer needed to keep the codebase clean.

// While Object.assign fails silently during run time
// So using this method
const enriched = {
id: application.id,

Choose a reason for hiding this comment

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

Instead of manually listing all properties, consider using a utility function or library to clone objects if the goal is to avoid using Object.assign or the spread operator. This can help reduce potential errors and improve maintainability.

transaction: t,
});

req.log.debug(`Updating other applications: ${JSON.stringify(copilotRequest)}`);

Choose a reason for hiding this comment

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

The log message here seems to be incorrect. It mentions copilotRequest but the operation is updating other applications. Consider updating the log message to reflect the correct context, such as Updating other applications status to CANCELED.

}
});

req.log.debug(`All updations done`);

Choose a reason for hiding this comment

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

Consider removing or rephrasing the debug log message to be more specific about what updates were completed. This will help in understanding the context of the log when reviewing logs later.

req.log.debug(`Member updated in kafka`);
updateCopilotOpportunity();
}
t.commit();

Choose a reason for hiding this comment

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

Consider handling potential errors when committing the transaction with t.commit(). If the commit fails, it might leave the application in an inconsistent state. You could use a try-catch block to manage this scenario and ensure proper error handling.

err.status = 400;
throw err;
const updateCopilotOpportunity = async () => {
const transaction = await models.sequelize.transaction();

Choose a reason for hiding this comment

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

The transaction is initialized here but not used in a try-catch block. Consider wrapping the transaction logic in a try-catch block to ensure that the transaction is properly committed or rolled back in case of an error.

});

req.log.debug(`All updations done`);
transaction.commit();

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.commit() operation to ensure that any issues during the commit process are properly managed. This could involve wrapping the commit in a try-catch block and handling any potential exceptions.

projectMember.get({ plain: true }),
existingMember);
req.log.debug(`Member updated in kafka`);
updateCopilotOpportunity();

Choose a reason for hiding this comment

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

The removal of t.commit(); suggests that the transaction might not be committed anymore. Ensure that the transaction is being properly handled to avoid potential data inconsistencies.

@@ -1,11 +1,13 @@
import _ from 'lodash';
import validate from 'express-validation';
import Joi from 'joi';
import config from 'config';

Choose a reason for hiding this comment

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

The config import is added but not used in the current diff. Ensure that it is necessary, or remove it if it is not used.


import models from '../../models';
import util from '../../util';
import { PERMISSION } from '../../permissions/constants';
import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES } from '../../constants';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES, TEMPLATE_IDS } from '../../constants';
import { getCopilotTypeLabel } from '../../utils/copilot';

Choose a reason for hiding this comment

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

The getCopilotTypeLabel import is added but not used in the current diff. Ensure that it is necessary, or remove it if it is not used.

throw err;
const updateCopilotOpportunity = async () => {
const transaction = await models.sequelize.transaction();
const memberDetails = await util.getMemberDetailsByUserIds([application.userId], req.log, req.id);

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 getMemberDetailsByUserIds function call to manage cases where the function might fail or return an unexpected result.

const updateCopilotOpportunity = async () => {
const transaction = await models.sequelize.transaction();
const memberDetails = await util.getMemberDetailsByUserIds([application.userId], req.log, req.id);
const member = memberDetails[0];

Choose a reason for hiding this comment

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

Ensure that memberDetails contains at least one element before accessing memberDetails[0] to prevent potential runtime errors.

const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL;
const copilotPortalUrl = config.get('copilotPortalUrl');
const requestData = copilotRequest.data;
createEvent(emailEventType, {

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 createEvent function to ensure that any issues with sending the email do not affect the transaction commit.

user_name: member ? member.handle : "",
},
sendgrid_template_id: TEMPLATE_IDS.COPILOT_ALREADY_PART_OF_PROJECT,
recipients: [member.email],

Choose a reason for hiding this comment

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

Ensure that member.email is validated or sanitized before being used as a recipient to prevent potential issues with invalid email addresses.

req.log.debug(`User already part of project: ${JSON.stringify(existingMember)}`);
if (['copilot', 'manager'].includes(existingMember.role)) {
req.log.debug(`User is a copilot or manager`);
await updateCopilotOpportunity();

Choose a reason for hiding this comment

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

The function updateCopilotOpportunity is now awaited, which implies it returns a promise. Ensure that the function is properly handling asynchronous operations and that any necessary error handling is in place for the promise resolution or rejection.

projectMember.get({ plain: true }),
existingMember);
req.log.debug(`Member updated in kafka`);
await updateCopilotOpportunity();

Choose a reason for hiding this comment

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

The function updateCopilotOpportunity() is now awaited. Ensure that this function returns a promise, otherwise the await keyword will not have the intended effect. If it does not return a promise, consider refactoring the function to do so.

status: COPILOT_APPLICATION_STATUS.CANCELED,
}, {
where: {
opportunityId: opportunity.id,

Choose a reason for hiding this comment

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

The removal of projectId from the where clause may affect the specificity of the query. Ensure that this change is intentional and that the query will still correctly identify the intended records without this condition.

import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES } from '../../constants';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES, TEMPLATE_IDS } from '../../constants';
import { getCopilotTypeLabel } from '../../utils/copilot';
import { createEvent } from '../../services/busApi';

Choose a reason for hiding this comment

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

The import statement for createEvent from ../../services/busApi is added, but it is not used in the current code. Consider removing it if it is not needed, or ensure it is utilized in the code changes.

@@ -263,7 +263,7 @@ module.exports = [
projectMember = projectMember.get({ plain: true });
projectMember = _.omit(projectMember, ['deletedAt']);

if (['observer', 'customer'].includes(updatedProps.role)) {
if (['observer', 'customer'].includes(previousValue.role)) {

Choose a reason for hiding this comment

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

The condition has been changed from checking updatedProps.role to previousValue.role. Ensure that this change aligns with the intended logic of the application. If the goal is to check the role before the update rather than after, this change is correct. Otherwise, it might lead to unintended behavior if the role is meant to be checked after the update.

import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, PROJECT_MEMBER_ROLE, RESOURCES, TEMPLATE_IDS } from '../../constants';
import { getCopilotTypeLabel } from '../../utils/copilot';
import { createEvent } from '../../services/busApi';
import moment from 'moment';

Choose a reason for hiding this comment

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

Consider using a more lightweight date library or native JavaScript Date methods if only basic date manipulation is required. The 'moment' library can significantly increase the bundle size.

@hentrymartin hentrymartin changed the title [WIP] fix(PM-1510): added existing membership to the copilot application fix(PM-1510): added existing membership to the copilot application Jul 30, 2025
if (['observer', 'customer'].includes(updatedProps.role)) {
await completeAllCopilotRequests(req, projectId, _transaction, _member);
if (['observer', 'customer'].includes(previousValue.role)) {
await completeAllCopilotRequests(req, projectId, _transaction, projectMember);

Choose a reason for hiding this comment

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

The variable _member was replaced with projectMember. Ensure that projectMember contains all necessary properties and data expected by the completeAllCopilotRequests function. Verify that this change aligns with the intended functionality and does not introduce any unintended side effects.

@@ -163,8 +163,6 @@ const completeAllCopilotRequests = async (req, projectId, _transaction, _member)

req.log.debug(`Sent email to ${member.email}`);
});

await _transaction.commit();
};

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() may lead to uncommitted transactions if this function is intended to finalize database changes. Ensure that the transaction is committed elsewhere if this is intentional, or consider re-adding the commit statement to prevent potential data inconsistencies.

@@ -263,8 +261,8 @@ module.exports = [
projectMember = projectMember.get({ plain: true });
projectMember = _.omit(projectMember, ['deletedAt']);

if (['observer', 'customer'].includes(updatedProps.role)) {
await completeAllCopilotRequests(req, projectId, _transaction, _member);
if (['observer', 'customer'].includes(previousValue.role) && ['copilot', 'manager'].includes(updatedProps.role)) {

Choose a reason for hiding this comment

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

Consider checking if updatedProps.role is defined before using it in the condition to prevent potential runtime errors if updatedProps is undefined or does not have a role property.

@hentrymartin hentrymartin merged commit 6122446 into develop Jul 30, 2025
2 checks passed
@hentrymartin hentrymartin deleted the pm-1510 branch July 30, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant