-
Notifications
You must be signed in to change notification settings - Fork 56
[PROD RELEASE] - Q2 #823
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?
[PROD RELEASE] - Q2 #823
Conversation
kkartunov
commented
Jun 17, 2025
- QA bug fixes
- Copilot Portal
fix(PM-1077): permission issue to delete copilot invite
PM-589 add migration for fixing copilot request model
@@ -19,6 +21,8 @@ const updateMemberValidations = { | |||
status: Joi.any() | |||
.valid(_.values(INVITE_STATUS)) | |||
.required(), | |||
source: Joi.string() |
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 validating the source
field to ensure it only accepts specific values, similar to how status
is validated. This can help prevent invalid data from being accepted.
@@ -29,6 +33,7 @@ module.exports = [ | |||
permissions('projectMemberInvite.edit'), | |||
(req, res, next) => { | |||
const newStatus = req.body.status; | |||
const source = req.body.source; |
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 validating the source
field to ensure it contains expected values before using it further in the code. This can help prevent potential issues from unexpected input.
@@ -81,7 +86,7 @@ module.exports = [ | |||
.update({ | |||
status: newStatus, | |||
}) | |||
.then((updatedInvite) => { | |||
.then(async (updatedInvite) => { |
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 use of async
in the .then
function suggests that there might be an await
operation inside the function. Please ensure that any asynchronous operations within this function are properly handled with await
. If there are no asynchronous operations, consider removing async
for clarity.
@@ -94,7 +99,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.
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()
method can be confusing and is generally not recommended. Consider using await
within an async
function instead of chaining .then()
. This can improve readability and maintainability of the code.
.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.
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 initialization. If models.sequelize.transaction()
fails, it could lead to unhandled exceptions.
transaction: t, | ||
}); | ||
|
||
await application.update({ status: nextApplicationStatus }, { |
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 application
is not null before calling update
on it. If findOne
does not find a record, application
will be null, leading to a runtime error.
transaction: t | ||
}); | ||
|
||
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.
Ensure that opportunity
is not null before calling update
on it. If findOne
does not find a record, opportunity
will be null, leading to a runtime error.
transaction: t, | ||
}); | ||
|
||
const request = 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.
Ensure that request
is not null before calling update
on it. If findOne
does not find a record, request
will be null, leading to a runtime error.
// update the application if the invite | ||
// originated from copilot opportunity | ||
if (updatedInvite.applicationId) { | ||
const allPendingInvitesForApplication = await models.ProjectMemberInvite.getPendingInvitesForApplication(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 error handling for getPendingInvitesForApplication
. If it fails, it could lead to unhandled exceptions.
@@ -225,6 +225,23 @@ const projectServiceUtils = { | |||
return _.intersection(roles, ADMIN_ROLES.map(r => r.toLowerCase())).length > 0; | |||
}, | |||
|
|||
/** | |||
* Helper funtion to verify if user has project manager 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.
Typo in the comment: 'funtion' should be corrected to 'function'.
const isMachineToken = _.get(req, 'authUser.isMachine', false); | ||
const tokenScopes = _.get(req, 'authUser.scopes', []); | ||
if (isMachineToken) { | ||
if (_.indexOf(tokenScopes, M2M_SCOPES.CONNECT_PROJECT_ADMIN) >= 0) return 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 simplifying the condition by directly returning the result of the comparison: return _.indexOf(tokenScopes, M2M_SCOPES.CONNECT_PROJECT_ADMIN) >= 0;
.
return _.get(res, 'data.result.content', []); | ||
}); | ||
} catch (err) { | ||
logger.debug(err, "error on getting role info"); |
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 logger.error
instead of logger.debug
for logging errors to ensure they are appropriately flagged in the logs.
.map(r => r.id); | ||
}); | ||
} catch (err) { | ||
return Promise.reject(err); |
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 logging similar to the getRoleInfo
method to capture and debug errors effectively.
fix(PM-1273): Send canApplyAsCopilot to check if the user can apply for the opportunity
@@ -34,6 +34,10 @@ module.exports = function defineProjectMember(sequelize, DataTypes) { | |||
], | |||
}); | |||
|
|||
ProjectMember.associate = (models) => { |
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 or validation to ensure that the association with models.Project
is correctly established. This can help prevent runtime errors if models.Project
is undefined or if there are issues with the foreign key.
src/routes/copilotOpportunity/get.js
Outdated
}, | ||
], | ||
}) | ||
.then((copilotOpportunity) => { | ||
const plainOpportunity = copilotOpportunity.get({ plain: true }); | ||
const formattedOpportunity = Object.assign({}, plainOpportunity, | ||
req.log.info("authUser", req.authUser); |
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 log statement req.log.info("authUser", req.authUser);
may expose sensitive information about the authenticated user. Consider logging only non-sensitive information or ensuring that sensitive data is adequately protected.
const memberIds = plainOpportunity.project.members && plainOpportunity.project.members.map((member) => member.userId); | ||
let canApplyAsCopilot = false; | ||
if (req.authUser) { | ||
canApplyAsCopilot = !memberIds.includes(req.authUser.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 logic canApplyAsCopilot = !memberIds.includes(req.authUser.userId)
assumes memberIds
is always defined. Consider adding a check to ensure memberIds
is not undefined before calling includes
to avoid potential runtime errors.
if (req.authUser) { | ||
canApplyAsCopilot = !memberIds.includes(req.authUser.userId) | ||
} | ||
// This shouldn't be exposed to the clientside |
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 comment // This shouldn't be exposed to the clientside
suggests that plainOpportunity.project.members
should not be sent to the client. Ensure that the deletion of plainOpportunity.project.members
is correctly implemented and tested to prevent accidental exposure.
return next(); | ||
} | ||
req.log.info("token available", token); |
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.
Logging sensitive information such as tokens can lead to security vulnerabilities. Consider removing or masking the token in the log statement.
feat(PM-1379): cancel copilot opportunity
details: JSON.stringify({ message: 'You do not have permission to cancel copilot opportunity' }), | ||
status: 403, | ||
}); | ||
return Promise.reject(err); |
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.
Using Promise.reject(err)
here is unnecessary because the function is already async. You can simply throw err
to achieve the same effect.
return Promise.reject(err); | ||
} | ||
// default values | ||
const opportunityId = _.parseInt(req.params.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 validating opportunityId
after parsing to ensure it is a valid integer and not NaN. This will prevent potential issues if the input is not a valid number.
transaction, | ||
}); | ||
|
||
if (!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.
The error message here could be more descriptive by including the opportunity ID in the log message for easier debugging.
transaction, | ||
}); | ||
|
||
const applications = await models.CopilotApplication.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 checking if applications
is empty before proceeding with the update to avoid unnecessary operations.
transaction, | ||
}); | ||
|
||
res.status(200).send({ id: opportunity.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 sending a more detailed response, such as including a message indicating that the opportunity was successfully canceled, for better client-side handling.
|
||
// Cancel Copilot opportunity | ||
router.route('/v5/projects/copilots/opportunity/:id(\\d+)/cancel') | ||
.delete(require('./copilotOpportunity/delete')); |
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 indentation for the .delete(require('./copilotOpportunity/delete'));
line is inconsistent with the rest of the code. It should be indented to align with the .post(require('./copilotOpportunity/assign'));
line above it.