-
Notifications
You must be signed in to change notification settings - Fork 55
[PROD] - Copilot Portal fixes and updates #838
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
base: master
Are you sure you want to change the base?
Conversation
fix(PM-1468): make notes mandatory
fix(PM-1368, PM-1394): group by active and then sort by createdAt
…unity-title PM-1498 - support for opportunity title
fix(PM-1494): Add extra info for copilot email
…equest PM-1499 edit copilot request
fix(PM-1510): added existing membership to the copilot application
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the case where copilotRequest
might be null
or undefined
after the findOne
query. This could prevent potential runtime errors if the expected data is not found.
where: { id: opportunity.copilotRequestId }, | ||
transaction: t, | ||
}); | ||
|
||
const application = await models.CopilotApplication.findOne({ |
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 that the application
object is properly validated before proceeding with operations that depend on its properties. This will help avoid errors if the findOne
query does not return the expected result.
}); | ||
|
||
req.log.debug(`All updations done`); | ||
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 using await transaction.commit()
to ensure the transaction is properly awaited and any potential errors are caught.
transaction, | ||
}); | ||
|
||
req.log.debug(`Updating other applications: ${JSON.stringify(copilotRequest)}`); |
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 debug log message here is misleading. It mentions updating 'other applications' but logs the copilotRequest
object. Consider updating the log message to accurately reflect the action being performed.
$ne: application.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 adding a transaction
option to the CopilotApplication.update
call to ensure it is part of the same transaction as the other updates.
req.log.debug(`Email sent`); | ||
}; | ||
|
||
const existingMember = activeMembers.find(item => item.userId === userId); |
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 existingMember
is used here, but it is essentially the same as existingUser
from the removed code. Ensure that the logic for finding the existing member is correct and consistent with the previous implementation.
where: { | ||
id: opportunityId, | ||
} | ||
}).then((opportunity) => { |
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 timeout or handling for the promise returned by models.CopilotOpportunity.findOne
to prevent potential hanging if the database query takes too long.
} | ||
}).then((opportunity) => { | ||
if (!opportunity) { | ||
const err = new Error('No opportunity found'); |
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 error message 'No opportunity found' could be more descriptive by including the opportunityId
to help with debugging.
}) | ||
.then(copilotApplications => { | ||
req.log.debug(`CopilotApplications ${JSON.stringify(copilotApplications)}`); | ||
return models.ProjectMember.getActiveProjectMembers(opportunity.projectId).then((members) => { |
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 promise returned by models.ProjectMember.getActiveProjectMembers
to ensure that any issues fetching project members are properly managed.
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); |
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 m
could be renamed to something more descriptive, such as member
, to improve code readability.
return enriched; | ||
}); | ||
|
||
req.log.debug(`Enriched Applications ${JSON.stringify(enrichedApplications)}`); |
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 res.json(enrichedApplications)
instead of res.status(200).send(enrichedApplications)
for consistency with the previous implementation and to automatically set the correct content-type header.
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.
The removal of _transaction.commit()
may lead to uncommitted changes in the database. Ensure that the transaction is being committed elsewhere or that this removal is intentional and safe.
projectMember = projectMember.get({ plain: true }); | ||
projectMember = _.omit(projectMember, ['deletedAt']); | ||
|
||
if (['observer', 'customer'].includes(previousValue.role) && ['copilot', 'manager'].includes(updatedProps.role)) { |
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 condition has been changed from checking updatedProps.role
to previousValue.role
. Ensure that this change aligns with the intended logic and that previousValue.role
is correctly defined and used in this context.
projectMember = projectMember.get({ plain: true }); | ||
projectMember = _.omit(projectMember, ['deletedAt']); | ||
|
||
if (['observer', 'customer'].includes(previousValue.role) && ['copilot', 'manager'].includes(updatedProps.role)) { | ||
await completeAllCopilotRequests(req, projectId, _transaction, projectMember); |
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 _member
has been replaced with projectMember
. Verify that projectMember
is the correct variable to use in this function call and that it contains the necessary data.
.circleci/config.yml
Outdated
@@ -149,7 +149,7 @@ workflows: | |||
context : org-global | |||
filters: | |||
branches: | |||
only: ['develop', 'migration-setup', 'pm-1378'] | |||
only: ['develop', 'migration-setup', 'PM-1314'] |
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 branch name 'PM-1314' should be verified to ensure it is correct and intended for production deployment. If this is a typo or incorrect branch, it may lead to unintended deployments.
PM-1314 Make grouping by status optional
@@ -21,6 +21,25 @@ module.exports = [ | |||
const pageSize = parseInt(req.query.pageSize, 10) || DEFAULT_PAGE_SIZE; | |||
const offset = (page - 1) * pageSize; | |||
const limit = pageSize; | |||
const noGroupingByStatus = req.query.noGrouping === 'true'; |
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 a more descriptive variable name than noGroupingByStatus
to improve code readability. For example, disableStatusGrouping
might better convey the purpose of the variable.
'ASC', | ||
]); | ||
} | ||
baseOrder.push([sortParams[0], sortParams[1]]); |
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 sortParams
is validated before being used in baseOrder.push([sortParams[0], sortParams[1]]);
to prevent potential runtime errors if sortParams
is not an array or does not contain the expected elements.
fix(PM-1398): Dont allow invite to be accepted when the opportunity is closed
@@ -42,11 +45,14 @@ module.exports = (req, data, existingTransaction) => { | |||
where: { | |||
projectId, | |||
type: data.type, | |||
status: { | |||
[Op.in]: [COPILOT_OPPORTUNITY_STATUS.ACTIVE], |
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 change from [Op.notIn]: [COPILOT_REQUEST_STATUS.CANCELED]
to [Op.in]: [COPILOT_OPPORTUNITY_STATUS.ACTIVE]
alters the logic to only consider active opportunities instead of excluding canceled ones. Ensure this change aligns with the intended functionality and does not inadvertently exclude other statuses that should be considered.
@@ -46,7 +50,7 @@ module.exports = [ | |||
|
|||
// get invite by id and project id | |||
return models.ProjectMemberInvite.getPendingOrRequestedProjectInviteById(projectId, inviteId) | |||
.then((invite) => { | |||
.then(async (invite) => { |
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 has been changed to an async
function, but there is no await
keyword used within the function body. If there are asynchronous operations intended to be awaited, ensure to use the await
keyword where necessary. Otherwise, consider if the async
keyword is needed.
@@ -81,6 +85,31 @@ module.exports = [ | |||
return next(err); | |||
} | |||
|
|||
// Check if the copilot opportunity is still active | |||
// When the invited user tries to accept the invite | |||
if (invite.applicationId) { |
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 check to ensure invite.applicationId
is valid before proceeding with the database query. This will prevent unnecessary database calls if applicationId
is null or undefined.
} | ||
}); | ||
|
||
const opportunity = await models.CopilotOpportunity.findOne({ |
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.
Add error handling for the case where application
is not found. This will prevent potential null reference errors when accessing application.opportunityId
.
}, | ||
}); | ||
|
||
req.log.debug(`Copilot opportunity status: ${opportunity.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.
Add error handling for the case where opportunity
is not found. This will prevent potential null reference errors when accessing opportunity.status
.
fix(PM-1398): cancel invites on canceling the copilot opportunity
@@ -1,10 +1,12 @@ | |||
import _ from 'lodash'; | |||
import { Op } from 'sequelize'; |
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 Op
import from 'sequelize' is added but not used in the current code. Consider removing it if it's not needed to avoid unnecessary imports.
|
||
import models from '../../models'; | ||
import util from '../../util'; | ||
import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS } from '../../constants'; | ||
import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, RESOURCES } from '../../constants'; |
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 constants EVENT
, INVITE_STATUS
, and RESOURCES
are imported but not used in the current code. Consider removing them if they are not needed to keep the imports clean and maintainable.
@@ -54,6 +56,14 @@ module.exports = [ | |||
})); | |||
}); | |||
|
|||
const allInvites = await models.ProjectMemberInvite.findAll({ |
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 ProjectMemberInvite
records are properly managed.
const allInvites = await models.ProjectMemberInvite.findAll({ | ||
where: { | ||
applicationId: { | ||
[Op.in]: applications.map(item => item.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 that applications
is not null or undefined before calling map
on it to prevent potential runtime errors.
// associated to the copilot opportunity | ||
// with cancel status | ||
for (const invite of allInvites) { | ||
await invite.update({ |
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 update
operation to ensure that any issues during the update process are caught and managed appropriately.
await invite.update({ | ||
status: INVITE_STATUS.CANCELED, | ||
}); | ||
await invite.reload(); |
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 reload
operation to ensure that any issues during the reload process are caught and managed appropriately.
status: INVITE_STATUS.CANCELED, | ||
}); | ||
await invite.reload(); | ||
util.sendResourceToKafkaBus( |
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 sendResourceToKafkaBus
function to ensure that any issues during the message sending process are caught and managed appropriately.
https://topcoder.atlassian.net/browse/PM-1291
https://topcoder.atlassian.net/browse/PM-1522