Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ workflows:
context : org-global
filters:
branches:
only: ['develop', 'migration-setup', 'pm-1613']
only: ['develop', 'migration-setup', 'pm-1611']
- deployProd:
context : org-global
filters:
Expand Down
2 changes: 2 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,8 @@ export const TEMPLATE_IDS = {
INFORM_PM_COPILOT_APPLICATION_ACCEPTED: 'd-b35d073e302b4279a1bd208fcfe96f58',
COPILOT_ALREADY_PART_OF_PROJECT: 'd-003d41cdc9de4bbc9e14538e8f2e0585',
COPILOT_APPLICATION_ACCEPTED: 'd-eef5e7568c644940b250e76d026ced5b',
COPILOT_OPPORTUNITY_COMPLETED: 'd-dc448919d11b4e7d8b4ba351c4b67b8b',
COPILOT_OPPORTUNITY_CANCELED: 'd-2a67ba71e82f4d70891fe6989c3522a3'
}
export const REGEX = {
URL: /^(http(s?):\/\/)?(www\.)?[a-zA-Z0-9\.\-\_]+(\.[a-zA-Z]{2,15})+(\:[0-9]{2,5})?(\/[a-zA-Z0-9\_\-\s\.\/\?\%\#\&\=;]*)?$/, // eslint-disable-line
Expand Down
1 change: 1 addition & 0 deletions src/models/copilotRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module.exports = function defineCopilotRequest(sequelize, DataTypes) {

CopliotRequest.associate = (models) => {
CopliotRequest.hasMany(models.CopilotOpportunity, { as: 'copilotOpportunity', foreignKey: 'copilotRequestId' });
CopliotRequest.belongsTo(models.Project, { as: 'project', foreignKey: 'projectId' });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the model name: CopliotRequest should be CopilotRequest. This typo is present in the existing code as well, but it would be good to correct it to maintain consistency and avoid potential issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hentrymartin please remane and update for the typo. Also, check on all refrences using it.
Funny how we missed this one?

};

return CopliotRequest;
Expand Down
30 changes: 30 additions & 0 deletions src/routes/copilotOpportunity/assign.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ module.exports = [
return next(err);
}

const sendEmailToAllApplicants = async (copilotRequest, allApplications) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature of sendEmailToAllApplicants has changed, but the logic for fetching allApplications has been removed. Ensure that allApplications is correctly populated before calling this function, as it is now expected to be passed as an argument.


const userIds = allApplications.map(item => item.userId);

const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id);

users.forEach(async (user) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a Promise.all to handle asynchronous operations in the forEach loop. This will ensure that all emails are sent before proceeding, and can help catch any errors in the email sending process.

req.log.debug(`Sending email notification to copilots who are not accepted`);
const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL;
const copilotPortalUrl = config.get('copilotPortalUrl');
const requestData = copilotRequest.data;
createEvent(emailEventType, {
data: {
opportunity_details_url: `${copilotPortalUrl}/opportunity`,
opportunity_title: requestData.opportunityTitle,
user_name: user ? user.handle : "",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user_name field is being set to an empty string if user is falsy. Consider handling cases where user might be undefined or null more explicitly to avoid sending incomplete data.

},
sendgrid_template_id: TEMPLATE_IDS.COPILOT_OPPORTUNITY_COMPLETED,
recipients: [user.email],
version: 'v3',
}, req.log);

req.log.debug(`Email sent to copilots who are not accepted`);
});

};

return models.sequelize.transaction(async (t) => {
const opportunity = await models.CopilotOpportunity.findOne({
where: { id: copilotOpportunityId },
Expand Down Expand Up @@ -238,6 +265,9 @@ module.exports = [
transaction: t,
});

// Send email to all applicants about opportunity completion
await sendEmailToAllApplicants(copilotRequest, otherApplications);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider handling potential errors from the sendEmailToAllApplicants function. If this function fails, it might be beneficial to log the error or take corrective action to ensure the application process continues smoothly.


for (const otherApplication of otherApplications) {
await otherApplication.update({
status: COPILOT_APPLICATION_STATUS.CANCELED,
Expand Down
31 changes: 30 additions & 1 deletion src/routes/copilotOpportunity/delete.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import _ from 'lodash';
import { Op } from 'sequelize';
import config from 'config';

import models from '../../models';
import util from '../../util';
import { COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, RESOURCES } from '../../constants';
import { CONNECT_NOTIFICATION_EVENT, COPILOT_APPLICATION_STATUS, COPILOT_OPPORTUNITY_STATUS, COPILOT_REQUEST_STATUS, EVENT, INVITE_STATUS, RESOURCES, TEMPLATE_IDS } from '../../constants';
import { createEvent } from '../../services/busApi';
import { PERMISSION } from '../../permissions/constants';


Expand All @@ -20,6 +22,31 @@ module.exports = [
// default values
const opportunityId = _.parseInt(req.params.id);

const sendEmailToAllApplicants = async (copilotRequest, applications) => {
const userIds = applications.map(item => item.userId);
const users = await util.getMemberDetailsByUserIds(userIds, req.log, req.id);

users.forEach(async (user) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using Promise.all to handle asynchronous operations within the forEach loop. This ensures that all emails are sent before proceeding, and can help avoid potential issues with unhandled promises.

req.log.debug(`Sending email notification to copilots who applied`);
const emailEventType = CONNECT_NOTIFICATION_EVENT.EXTERNAL_ACTION_EMAIL;
const copilotPortalUrl = config.get('copilotPortalUrl');
const requestData = copilotRequest.data;
createEvent(emailEventType, {
data: {
opportunity_details_url: `${copilotPortalUrl}/opportunity`,
opportunity_title: requestData.opportunityTitle,
user_name: user ? user.handle : "",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a default value or handling the case where user.handle might be null or undefined to prevent potential issues with the email content.

},
sendgrid_template_id: TEMPLATE_IDS.COPILOT_OPPORTUNITY_CANCELED,
recipients: [user.email],
version: 'v3',
}, req.log);

req.log.debug(`Email sent to copilots who applied`);
});

};

return models.sequelize.transaction(async (transaction) => {
req.log.debug('Canceling Copilot opportunity transaction', opportunityId);
const opportunity = await models.CopilotOpportunity.findOne({
Expand Down Expand Up @@ -93,6 +120,8 @@ module.exports = [
invite.toJSON());
}

await sendEmailToAllApplicants(copilotRequest, applications)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sendEmailToAllApplicants function call has been modified to remove the req parameter. Ensure that this change does not affect the function's behavior or cause any issues, as the req parameter might have been used within the function for accessing request-specific data.


res.status(200).send({ id: opportunity.id });
})

Expand Down
2 changes: 1 addition & 1 deletion src/routes/copilotOpportunity/get.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import util from '../../util';
module.exports = [
(req, res, next) => {
const { id } = req.params;

req.log.info("Reached get endpoint");

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 log message that includes the opportunity ID to provide better context for debugging, e.g., req.log.info(Reached get endpoint for opportunity ID: ${id});.

if (!id || isNaN(id)) {
return util.handleError('Invalid opportunity ID', null, req, next, 400);
}
Expand Down
29 changes: 19 additions & 10 deletions src/routes/copilotRequest/list.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import _ from 'lodash';
import { Op, Sequelize } from 'sequelize';

import models from '../../models';
import util from '../../util';
import { PERMISSION } from '../../permissions/constants';
import { DEFAULT_PAGE_SIZE } from '../../constants';

module.exports = [
(req, res, next) => {
Expand All @@ -15,6 +17,10 @@ module.exports = [
return next(err);
}

const page = parseInt(req.query.page, 10) || 1;

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 req.query.page is not a valid integer. Currently, parseInt will return NaN for invalid inputs, which could lead to unexpected behavior when calculating offset.

const pageSize = parseInt(req.query.pageSize, 10) || DEFAULT_PAGE_SIZE;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, ensure that req.query.pageSize is a valid integer before using it. This will prevent potential issues with offset calculation if pageSize is NaN.

const offset = (page - 1) * pageSize;

const projectId = _.parseInt(req.params.projectId);

let sort = req.query.sort ? decodeURIComponent(req.query.sort) : 'createdAt desc';
Expand All @@ -29,19 +35,22 @@ module.exports = [

const whereCondition = projectId ? { projectId } : {};

return models.CopilotRequest.findAll({
return models.CopilotRequest.findAndCountAll({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider checking if findAndCountAll is necessary for your use case. If you only need the count for pagination headers, ensure that this change is intentional as it may impact performance.

where: whereCondition,
include: [
{
model: models.CopilotOpportunity,
as: 'copilotOpportunity',
},
{ model: models.CopilotOpportunity, as: 'copilotOpportunity', required: false },
{ model: models.Project, as: 'project', required: false },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that the inclusion of the Project model is necessary for the functionality you are implementing. If not, it could lead to unnecessary data fetching.

],
order: [[sortParams[0], sortParams[1]]],
})
.then(copilotRequests => res.json(copilotRequests))
.catch((err) => {
util.handleError('Error fetching copilot requests', err, req, next);
});
limit: pageSize,
offset,
distinct: true,
subQuery: false,
}).then(({rows: copilotRequests, count}) => util.setPaginationHeaders(req, res, {
count: count,
rows: copilotRequests,
page,
pageSize,
}));
},
];
1 change: 1 addition & 0 deletions src/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ router.use((err, req, res, next) => { // eslint-disable-line no-unused-vars

// catch 404 and forward to error handler
router.use((req, res, next) => {
req.log.info("reached middleware")

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 log message to provide better context for debugging purposes. For example, specify which middleware was reached or the purpose of this log entry.

const err = new Error('Not Found');
err.status = 404;
next(err);
Expand Down