feat(PM-1168): Added API to assign an opportunity with an applicant #805
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
migrations/umzug/migrations/20250511123109-copilot_application_status.js
Show resolved
Hide resolved
|
|
||
| const existingUser = activeMembers.find(item => item.userId === userId); | ||
|
|
||
| if (existingUser) { |
There was a problem hiding this comment.
We should check if existing user is a copilot in the project not just that they are member of a project.
| details: JSON.stringify({ message: 'You do not have permission to assign a copilot opportunity' }), | ||
| status: 403, | ||
| }); | ||
| return next(err); |
There was a problem hiding this comment.
Is next a promise? Returning a callback function won't revert the transaction, what do you think?
| } | ||
| }); | ||
|
|
||
| res.status(200).send({id: applicationId}); |
There was a problem hiding this comment.
I am missing the requirement "Mark the related to opportunity copilot request as FULFILLED".
Where is this implemented?
| where: { | ||
| opportunityId: opportunityId, | ||
| opportunityId, |
There was a problem hiding this comment.
The property opportunityId is using shorthand syntax. Ensure that this is intentional and consistent with the rest of the codebase.
|
|
||
| return models.sequelize.transaction(async (t) => { | ||
| const opportunity = await models.CopilotOpportunity.findOne({ | ||
| where: { id: copilotOpportunityId }, |
There was a problem hiding this comment.
Consider adding error handling for the database transaction itself to ensure that any issues during the transaction are properly caught and handled.
| } | ||
|
|
||
| const application = await models.CopilotApplication.findOne({ | ||
| where: { id: applicationId }, |
There was a problem hiding this comment.
The transaction: t option is correctly added to the findOne method, but ensure that all database operations within the transaction are consistently using this option to maintain atomicity.
| } | ||
|
|
||
| await models.CopilotRequest.update( | ||
| { status: COPILOT_REQUEST_STATUS.FULFILLED }, |
There was a problem hiding this comment.
Ensure that the transaction: t option is used consistently across all update operations to maintain atomicity within the transaction.
| { where: { id: applicationId }, transaction: t }, | ||
| ); | ||
|
|
||
| res.status(200).send({ id: applicationId }); |
There was a problem hiding this comment.
Consider adding a final catch block after the transaction to handle any errors that might occur during the transaction and ensure a proper rollback if necessary.
| @@ -63,16 +63,16 @@ module.exports = [ | |||
| res.status(200).json(existingApplication); | |||
| return Promise.resolve(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Remove unnecessary whitespace change to keep the code clean and consistent.
| util.handleError('Error creating copilot application', err, req, next); | ||
| return next(err); | ||
| }); | ||
| util.handleError('Error creating copilot application', err, req, next); |
There was a problem hiding this comment.
Ensure consistent indentation for the .catch block to improve readability.
| @@ -295,13 +295,12 @@ module.exports = [ | |||
| // whom we are inviting, because Member Service has a loose search logic and may return | |||
| // users with handles whom we didn't search for | |||
| .then((foundUsers) => { | |||
| if(invite.handles) { | |||
| if (invite.handles) { | |||
There was a problem hiding this comment.
There should be a space between if and the opening parenthesis for consistency with the rest of the code style.
| return [] | ||
| } | ||
|
|
||
| return []; |
There was a problem hiding this comment.
Ensure consistent use of semicolons. The semicolon at the end of the return []; statement is correct, but make sure this style is consistent throughout the codebase.
| @@ -443,7 +442,7 @@ module.exports = [ | |||
| } | |||
| }); | |||
| }).catch((err) => { | |||
| console.log(err) | |||
| console.log(err); | |||
There was a problem hiding this comment.
Using console.log for error handling is not recommended in production code. Consider using a proper logging mechanism or error handling strategy.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1168