-
Notifications
You must be signed in to change notification settings - Fork 55
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
Changes from all commits
683b3d6
f0a889f
4823330
26dc288
59c6077
2ba33ac
04b4e37
f2a8c89
76a22c1
cf120a8
471d5e1
1559e31
44ac842
de8c14f
40076d3
d40373d
8b2fa3e
e7a328b
ab268ca
e82e6b2
dad92bc
ada10e8
c2df9ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
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, 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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
import { createEvent } from '../../services/busApi'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The import statement for |
||
import moment from 'moment'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
const assignCopilotOpportunityValidations = { | ||
body: Joi.object().keys({ | ||
|
@@ -45,11 +49,17 @@ module.exports = [ | |
throw err; | ||
} | ||
|
||
const copilotRequest = await models.CopilotRequest.findOne({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider checking if |
||
where: { id: opportunity.copilotRequestId }, | ||
transaction: t, | ||
}); | ||
|
||
const application = await models.CopilotApplication.findOne({ | ||
where: { id: applicationId, opportunityId: copilotOpportunityId }, | ||
transaction: t, | ||
}); | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (!application) { | ||
const err = new Error('No such application available'); | ||
err.status = 400; | ||
|
@@ -65,12 +75,101 @@ module.exports = [ | |
const projectId = opportunity.projectId; | ||
const userId = application.userId; | ||
const activeMembers = await models.ProjectMember.getActiveProjectMembers(projectId, t); | ||
|
||
const existingUser = activeMembers.find(item => item.userId === userId); | ||
if (existingUser && existingUser.role === 'copilot') { | ||
const err = new Error(`User is already a copilot of this project`); | ||
err.status = 400; | ||
throw err; | ||
const updateCopilotOpportunity = async () => { | ||
const transaction = await models.sequelize.transaction(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The transaction is initialized here but not used in a |
||
const memberDetails = await util.getMemberDetailsByUserIds([application.userId], req.log, req.id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding error handling for the |
||
const member = memberDetails[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that |
||
req.log.debug(`Updating opportunity: ${JSON.stringify(opportunity)}`); | ||
await opportunity.update({ | ||
status: COPILOT_OPPORTUNITY_STATUS.COMPLETED, | ||
}, { | ||
transaction, | ||
}); | ||
req.log.debug(`Updating application: ${JSON.stringify(application)}`); | ||
await application.update({ | ||
status: COPILOT_APPLICATION_STATUS.ACCEPTED, | ||
}, { | ||
transaction, | ||
}); | ||
|
||
req.log.debug(`Updating request: ${JSON.stringify(copilotRequest)}`); | ||
await copilotRequest.update({ | ||
status: COPILOT_REQUEST_STATUS.FULFILLED, | ||
}, { | ||
transaction, | ||
}); | ||
|
||
req.log.debug(`Updating other applications: ${JSON.stringify(copilotRequest)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log message here seems to be incorrect. It mentions |
||
await models.CopilotApplication.update({ | ||
status: COPILOT_APPLICATION_STATUS.CANCELED, | ||
}, { | ||
where: { | ||
opportunityId: opportunity.id, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removal of |
||
id: { | ||
$ne: application.id, | ||
}, | ||
} | ||
}); | ||
|
||
req.log.debug(`All updations done`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
transaction.commit(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding error handling for the |
||
|
||
req.log.debug(`Sending email notification`); | ||
const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL; | ||
const copilotPortalUrl = config.get('copilotPortalUrl'); | ||
const requestData = copilotRequest.data; | ||
createEvent(emailEventType, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding error handling around the |
||
data: { | ||
opportunity_details_url: `${copilotPortalUrl}/opportunity/${opportunity.id}`, | ||
work_manager_url: config.get('workManagerUrl'), | ||
opportunity_type: getCopilotTypeLabel(requestData.projectType), | ||
opportunity_title: requestData.opportunityTitle, | ||
start_date: moment.utc(requestData.startDate).format('DD-MM-YYYY'), | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that |
||
version: 'v3', | ||
}, req.log); | ||
|
||
req.log.debug(`Email sent`); | ||
}; | ||
|
||
const existingMember = activeMembers.find(item => item.userId === userId); | ||
if (existingMember) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for checking |
||
req.log.debug(`User already part of project: ${JSON.stringify(existingMember)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider handling the case where |
||
if (['copilot', 'manager'].includes(existingMember.role)) { | ||
req.log.debug(`User is a copilot or manager`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The debug log message |
||
await updateCopilotOpportunity(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function |
||
} else { | ||
req.log.debug(`User has read/write role`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The debug log message |
||
await models.ProjectMember.update({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
role: 'copilot', | ||
}, { | ||
where: { | ||
id: existingMember.id, | ||
}, | ||
}); | ||
|
||
const projectMember = await models.ProjectMember.findOne({ | ||
where: { | ||
id: existingMember.id, | ||
}, | ||
}); | ||
|
||
req.log.debug(`Updated project member: ${JSON.stringify(projectMember.get({plain: true}))}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider handling potential errors that might occur during the |
||
|
||
util.sendResourceToKafkaBus( | ||
req, | ||
EVENT.ROUTING_KEY.PROJECT_MEMBER_UPDATED, | ||
RESOURCES.PROJECT_MEMBER, | ||
projectMember.get({ plain: true }), | ||
existingMember); | ||
req.log.debug(`Member updated in kafka`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log message |
||
await updateCopilotOpportunity(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function |
||
} | ||
res.status(200).send({ id: applicationId }); | ||
return; | ||
} | ||
|
||
const existingInvite = await models.ProjectMemberInvite.findAll({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,19 +31,68 @@ module.exports = [ | |
canAccessAllApplications ? {} : { createdBy: userId }, | ||
); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
return models.CopilotApplication.findAll({ | ||
where: whereCondition, | ||
include: [ | ||
{ | ||
model: models.CopilotOpportunity, | ||
as: 'copilotOpportunity', | ||
}, | ||
], | ||
order: [[sortParams[0], sortParams[1]]], | ||
return models.CopilotOpportunity.findOne({ | ||
where: { | ||
id: opportunityId, | ||
} | ||
}).then((opportunity) => { | ||
if (!opportunity) { | ||
const err = new Error('No opportunity found'); | ||
err.status = 404; | ||
throw err; | ||
} | ||
return models.CopilotApplication.findAll({ | ||
where: whereCondition, | ||
include: [ | ||
{ | ||
model: models.CopilotOpportunity, | ||
as: 'copilotOpportunity', | ||
}, | ||
], | ||
order: [[sortParams[0], sortParams[1]]], | ||
}) | ||
.then(copilotApplications => { | ||
req.log.debug(`CopilotApplications ${JSON.stringify(copilotApplications)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a more descriptive log message to clarify what |
||
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)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log message for |
||
const enrichedApplications = copilotApplications.map(application => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
const m = members.find(m => m.userId === application.userId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
// Using spread operator fails in lint check | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
opportunityId: application.opportunityId, | ||
notes: application.notes, | ||
status: application.status, | ||
userId: application.userId, | ||
deletedAt: application.deletedAt, | ||
createdAt: application.createdAt, | ||
updatedAt: application.updatedAt, | ||
deletedBy: application.deletedBy, | ||
createdBy: application.createdBy, | ||
updatedBy: application.updatedBy, | ||
copilotOpportunity: application.copilotOpportunity, | ||
}; | ||
|
||
if (m) { | ||
enriched.existingMembership = m; | ||
} | ||
|
||
req.log.debug(`Existing member to application ${JSON.stringify(enriched)}`); | ||
|
||
return enriched; | ||
}); | ||
|
||
req.log.debug(`Enriched Applications ${JSON.stringify(enrichedApplications)}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
res.status(200).send(enrichedApplications); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The response method has been changed from |
||
}); | ||
}) | ||
}) | ||
.then(copilotApplications => res.json(copilotApplications)) | ||
.catch((err) => { | ||
util.handleError('Error fetching copilot applications', err, req, next); | ||
}); | ||
.catch((err) => { | ||
util.handleError('Error fetching copilot applications', err, req, next); | ||
}); | ||
}, | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,8 +163,6 @@ const completeAllCopilotRequests = async (req, projectId, _transaction, _member) | |
|
||
req.log.debug(`Sent email to ${member.email}`); | ||
}); | ||
|
||
await _transaction.commit(); | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removal of |
||
module.exports = [ | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider checking if |
||
await completeAllCopilotRequests(req, projectId, _transaction, projectMember); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable |
||
} | ||
}) | ||
.then(() => ( | ||
|
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
config
import is added but not used in the current diff. Ensure that it is necessary, or remove it if it is not used.