feat(PM-1379): cancel copilot opportunity #825
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const opportunityId = _.parseInt(req.params.id); | ||
|
|
||
| return models.sequelize.transaction(async (transaction) => { | ||
| req.log.debug('Canceling Copilot opportunity transaction', data); |
There was a problem hiding this comment.
The variable data is referenced here but not defined anywhere in the code. This will cause a ReferenceError. Consider defining data or removing its usage.
| transaction, | ||
| }); | ||
|
|
||
| applications.update({ |
There was a problem hiding this comment.
The applications.update method is being called on an array of applications, which will not work as expected. You should iterate over each application and call the update method individually.
| transaction, | ||
| }); | ||
|
|
||
| copilotRequest.update({ |
There was a problem hiding this comment.
The copilotRequest.update method should be awaited to ensure the update operation completes before proceeding.
| transaction, | ||
| }); | ||
|
|
||
| opportunity.update({ |
There was a problem hiding this comment.
The opportunity.update method should be awaited to ensure the update operation completes before proceeding.
| @@ -419,6 +419,10 @@ router.route('/v5/projects/copilots/opportunity/:id(\\d+)/applications') | |||
| router.route('/v5/projects/copilots/opportunity/:id(\\d+)/assign') | |||
| .post(require('./copilotOpportunity/assign')); | |||
|
|
|||
| // Cancel Copilot opportunity | |||
| router.route('/v5/projects/copilots/opportunity/:id(\\d+)/cancel') | |||
| .delete(require('./copilotOpportunity/delete')); | |||
There was a problem hiding this comment.
The file being required here is named delete, which might be misleading since the route is for canceling an opportunity. Consider renaming the file to something more descriptive, like cancel, to better reflect its purpose.
| const opportunityId = _.parseInt(req.params.id); | ||
|
|
||
| return models.sequelize.transaction(async (transaction) => { | ||
| req.log.debug('Canceling Copilot opportunity transaction', opportunityId); |
There was a problem hiding this comment.
The log message has been updated to include opportunityId, which is a good improvement for debugging. However, ensure that opportunityId is always a valid integer before logging to avoid potential issues with malformed input.
| transaction, | ||
| }); | ||
|
|
||
| const applications = await models.CopilotApplication.findAll({ |
There was a problem hiding this comment.
The model name CopilotApplications was changed to CopilotApplication. Ensure that this change is consistent across the entire codebase to prevent any potential issues with model references.
| transaction, | ||
| }); | ||
|
|
||
| applications.forEach(async (application) => { |
There was a problem hiding this comment.
The use of forEach with async/await may not work as expected because forEach does not wait for promises to resolve. Consider using a for...of loop or Promise.all to ensure that all updates are completed before proceeding.
| }); | ||
|
|
||
| const promises = []; | ||
| applications.forEach(async (application) => { |
There was a problem hiding this comment.
Using forEach with an async function does not wait for the promises to resolve. Consider using a for...of loop or Promise.all to ensure all updates are completed before proceeding.
What's in this PR?
Ticket link - https://topcoder.atlassian.net/browse/PM-1378