From a3190cf3e779271c7a2780585cdba99fec3d7876 Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Tue, 5 Mar 2019 15:44:02 +0800 Subject: [PATCH 1/3] winning submission from challenge 30085188 - Topcoder Connect - Handle invitation errors --- postman.json | 27 ++++- src/routes/projectMemberInvites/create.js | 95 +++++++-------- .../projectMemberInvites/create.spec.js | 109 ++++++++++++++---- swagger.yaml | 12 +- 4 files changed, 170 insertions(+), 73 deletions(-) diff --git a/postman.json b/postman.json index 96995389..8c74a585 100644 --- a/postman.json +++ b/postman.json @@ -1041,6 +1041,31 @@ }, "response": [] }, + { + "name": "Invite with userIds and emails - both success and failed", + "request": { + "url": "{{api-url}}/v4/projects/1/members/invite", + "method": "POST", + "header": [ + { + "key": "Authorization", + "value": "Bearer {{jwt-token-manager-40051334}}", + "description": "" + }, + { + "key": "Content-Type", + "value": "application/json", + "description": "" + } + ], + "body": { + "mode": "raw", + "raw": "{\n\t\"param\": {\n\t\t\"userIds\": [40051331, 40051334],\n\t\t\"emails\": [\"divyalife526@gmail.com\"],\n\t\t\"role\": \"manager\"\n\t}\n}" + }, + "description": "" + }, + "response": [] + }, { "name": "Update invite status with userId", "request": { @@ -5497,4 +5522,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index 90817f2c..862c1f7c 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -35,12 +35,12 @@ const addMemberValidations = { * @param {Object} invite invite to process * @param {Array} invites existent invites from DB * @param {Object} data template for new invites to be put in DB + * @param {Array} failed failed invites error message * * @returns {Promise} list of promises */ -const buildCreateInvitePromises = (req, invite, invites, data) => { +const buildCreateInvitePromises = (req, invite, invites, data, failed) => { const invitePromises = []; - if (invite.userIds) { // remove invites for users that are invited already _.remove(invite.userIds, u => _.some(invites, i => i.userId === u)); @@ -91,15 +91,15 @@ const buildCreateInvitePromises = (req, invite, invites, data) => { invitePromises.push(models.ProjectMemberInvite.create(dataNew)); }); - - return Promise.resolve(invitePromises); + return invitePromises; }).catch((error) => { req.log.error(error); - return Promise.reject(invitePromises); + _.forEach(invite.emails, email => failed.push(_.assign({}, { email, message: error.statusText }))); + return invitePromises; }); } - return Promise.resolve(invitePromises); + return invitePromises; }; const sendInviteEmail = (req, projectId, invite) => { @@ -157,6 +157,7 @@ module.exports = [ validate(addMemberValidations), permissions('projectMemberInvite.create'), (req, res, next) => { + let failed = []; const invite = req.body.param; if (!invite.userIds && !invite.emails) { @@ -192,16 +193,16 @@ module.exports = [ if (invite.emails) { // email invites can only be used for CUSTOMER role if (invite.role !== PROJECT_MEMBER_ROLE.CUSTOMER) { // eslint-disable-line no-lonely-if - const err = new Error(`Emails can only be used for ${PROJECT_MEMBER_ROLE.CUSTOMER}`); - err.status = 400; - return next(err); + const message = `Emails can only be used for ${PROJECT_MEMBER_ROLE.CUSTOMER}`; + failed = _.concat(failed, _.map(invite.emails, email => _.assign({}, { email, message }))); + delete invite.emails; } } - if (promises.length === 0) { promises.push(Promise.resolve()); } return Promise.all(promises).then((rolesList) => { + const updatedInvite = invite; if (!!invite.userIds && _.includes(PROJECT_MEMBER_MANAGER_ROLES, invite.role)) { req.log.debug('Chekcing if userId is allowed as manager'); const forbidUserList = []; @@ -209,23 +210,23 @@ module.exports = [ const [userId, roles] = data; req.log.debug(roles); - if (!util.hasIntersection(MANAGER_ROLES, roles)) { + if (roles && !util.hasIntersection(MANAGER_ROLES, roles)) { forbidUserList.push(userId); } }); if (forbidUserList.length > 0) { - const err = new Error(`${forbidUserList.join()} cannot be added with a Manager role to the project`); - err.status = 403; - return next(err); + const message = 'cannot be added with a Manager role to the project'; + failed = _.concat(failed, _.map(forbidUserList, id => _.assign({}, { id, message }))); + updatedInvite.userIds = _.filter(invite.userIds, userId => !_.includes(forbidUserList, userId)); } } return models.ProjectMemberInvite.getPendingInvitesForProject(projectId) .then((invites) => { const data = { projectId, - role: invite.role, + role: updatedInvite.role, // invite directly if user is admin or copilot manager - status: (invite.role !== PROJECT_MEMBER_ROLE.COPILOT || + status: (updatedInvite.role !== PROJECT_MEMBER_ROLE.COPILOT || util.hasRoles(req, [USER_ROLE.CONNECT_ADMIN, USER_ROLE.COPILOT_MANAGER])) ? INVITE_STATUS.PENDING : INVITE_STATUS.REQUESTED, @@ -233,39 +234,39 @@ module.exports = [ updatedBy: req.authUser.userId, }; - return buildCreateInvitePromises(req, invite, invites, data) - .then((invitePromises) => { - if (invitePromises.length === 0) { - return []; - } - - req.log.debug('Creating invites'); - return models.sequelize.Promise.all(invitePromises) - .then((values) => { - values.forEach((v) => { - req.app.emit(EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED, { - req, - userId: v.userId, - email: v.email, - status: v.status, - role: v.role, - }); - req.app.services.pubsub.publish( - EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED, - v, - { correlationId: req.id }, - ); - // send email invite (async) - if (v.email && !v.userId && v.status === INVITE_STATUS.PENDING) { - sendInviteEmail(req, projectId, v); - } - }); - return values; - }); // models.sequelize.Promise.all - }); // buildCreateInvitePromises + req.log.debug('Creating invites'); + return models.sequelize.Promise.all(buildCreateInvitePromises(req, updatedInvite, invites, data, failed)) + .then((values) => { + values.forEach((v) => { + req.app.emit(EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED, { + req, + userId: v.userId, + email: v.email, + status: v.status, + role: v.role, + }); + req.app.services.pubsub.publish( + EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED, + v, + { correlationId: req.id }, + ); + // send email invite (async) + if (v.email && !v.userId && v.status === INVITE_STATUS.PENDING) { + sendInviteEmail(req, projectId, v); + } + }); + return values; + }); // models.sequelize.Promise.all }); // models.ProjectMemberInvite.getPendingInvitesForProject }) - .then(values => res.status(201).json(util.wrapResponse(req.id, values, null, 201))) + .then((values) => { + const success = _.assign({}, { success: values }); + if (failed.length) { + res.status(403).json(util.wrapResponse(req.id, _.assign({}, success, { failed }), null, 403)); + } else { + res.status(201).json(util.wrapResponse(req.id, success, null, 201)); + } + }) .catch(err => next(err)); }, ]; diff --git a/src/routes/projectMemberInvites/create.spec.js b/src/routes/projectMemberInvites/create.spec.js index e648a06f..74f6be9c 100644 --- a/src/routes/projectMemberInvites/create.spec.js +++ b/src/routes/projectMemberInvites/create.spec.js @@ -42,6 +42,15 @@ describe('Project Member Invite create', () => { createdBy: 1, updatedBy: 1, }); + + models.ProjectMember.create({ + userId: 40051334, + projectId: project1.id, + role: 'manager', + isPrimary: true, + createdBy: 1, + updatedBy: 1, + }); }).then(() => models.Project.create({ type: 'generic', @@ -87,6 +96,7 @@ describe('Project Member Invite create', () => { sinon.stub(server.services.pubsub, 'init', () => {}); sinon.stub(server.services.pubsub, 'publish', () => {}); // by default mock lookupUserEmails return nothing so all the cases are not broken + sandbox.stub(util, 'getUserRoles', () => Promise.resolve([])); sandbox.stub(util, 'lookupUserEmails', () => Promise.resolve([])); sandbox.stub(util, 'getMemberDetailsByUserIds', () => Promise.resolve([{ userId: 40051333, @@ -246,9 +256,11 @@ describe('Project Member Invite create', () => { result: { success: true, status: 200, - content: [{ - roleName: USER_ROLE.COPILOT, - }], + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, }, }, }), @@ -271,7 +283,7 @@ describe('Project Member Invite create', () => { if (err) { done(err); } else { - const resJson = res.body.result.content[0]; + const resJson = res.body.result.content.success[0]; should.exist(resJson); resJson.role.should.equal('customer'); resJson.projectId.should.equal(project2.id); @@ -292,9 +304,11 @@ describe('Project Member Invite create', () => { result: { success: true, status: 200, - content: [{ - roleName: USER_ROLE.COPILOT, - }], + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, }, }, }), @@ -322,7 +336,7 @@ describe('Project Member Invite create', () => { if (err) { done(err); } else { - const resJson = res.body.result.content[0]; + const resJson = res.body.result.content.success[0]; should.exist(resJson); resJson.role.should.equal('customer'); resJson.projectId.should.equal(project2.id); @@ -344,9 +358,11 @@ describe('Project Member Invite create', () => { result: { success: true, status: 200, - content: [{ - roleName: USER_ROLE.COPILOT, - }], + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, }, }, }), @@ -369,7 +385,7 @@ describe('Project Member Invite create', () => { if (err) { done(err); } else { - const resJson = res.body.result.content[0]; + const resJson = res.body.result.content.success[0]; should.exist(resJson); resJson.role.should.equal('customer'); resJson.projectId.should.equal(project2.id); @@ -390,9 +406,11 @@ describe('Project Member Invite create', () => { result: { success: true, status: 200, - content: [{ - roleName: USER_ROLE.COPILOT, - }], + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, }, }, }), @@ -415,7 +433,7 @@ describe('Project Member Invite create', () => { if (err) { done(err); } else { - const resJson = res.body.result.content; + const resJson = res.body.result.content.success; should.exist(resJson); resJson.length.should.equal(0); server.services.pubsub.publish.neverCalledWith('project.member.invite.created').should.be.true; @@ -484,16 +502,18 @@ describe('Project Member Invite create', () => { it('should return 201 if try to create manager with MANAGER_ROLES', (done) => { const mockHttpClient = _.merge(testUtil.mockHttpClient, { get: () => Promise.resolve({ - status: 200, + status: 403, data: { id: 'requesterId', version: 'v3', result: { success: true, - status: 200, - content: [{ - roleName: USER_ROLE.MANAGER, - }], + status: 403, + content: { + failed: [{ + message: 'cannot be added with a Manager role to the project', + }], + }, }, }, }), @@ -511,16 +531,57 @@ describe('Project Member Invite create', () => { }, }) .expect('Content-Type', /json/) + .expect(403) + .end((err, res) => { + const failed = res.body.result.content.failed[0]; + should.exist(failed); + failed.message.should.equal('cannot be added with a Manager role to the project'); + done(); + }); + }); + + it('should return 201 if try to create customer with COPILOT', (done) => { + const mockHttpClient = _.merge(testUtil.mockHttpClient, { + get: () => Promise.resolve({ + status: 200, + data: { + id: 'requesterId', + version: 'v3', + result: { + success: true, + status: 200, + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, + }, + }, + }), + }); + sandbox.stub(util, 'getHttpClient', () => mockHttpClient); + request(server) + .post(`/v4/projects/${project1.id}/members/invite`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ + param: { + userIds: [40051331], + role: 'copilot', + }, + }) + .expect('Content-Type', /json/) .expect(201) .end((err, res) => { if (err) { done(err); } else { - const resJson = res.body.result.content[0]; + const resJson = res.body.result.content.success[0]; should.exist(resJson); - resJson.role.should.equal('manager'); + resJson.role.should.equal('copilot'); resJson.projectId.should.equal(project1.id); - resJson.userId.should.equal(40152855); + resJson.userId.should.equal(40051331); server.services.pubsub.publish.calledWith('project.member.invite.created').should.be.true; done(); } diff --git a/swagger.yaml b/swagger.yaml index eb8d063f..fcd02b60 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -4071,6 +4071,16 @@ definitions: description: READ-ONLY. User that last updated this task readOnly: true + ProjectMemberInviteSuccessAndFailure: + type: object + properties: + success: + $ref: "#/definitions/ProjectMemberInvite" + failed: + type: array + items: + type: object + AddProjectMemberInvitesRequest: title: Add project member invites request object type: object @@ -4133,4 +4143,4 @@ definitions: metadata: $ref: "#/definitions/ResponseMetadata" content: - $ref: "#/definitions/ProjectMemberInvite" \ No newline at end of file + $ref: "#/definitions/ProjectMemberInviteSuccessAndFailure" From 7a24574c940aa01fd49ca855d47991382307ef8a Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Tue, 5 Mar 2019 16:06:04 +0800 Subject: [PATCH 2/3] updated so individual failed invitataions have 'userId' field instead of 'id' to make it clear --- src/routes/projectMemberInvites/create.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index 862c1f7c..a4b0860e 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -216,7 +216,7 @@ module.exports = [ }); if (forbidUserList.length > 0) { const message = 'cannot be added with a Manager role to the project'; - failed = _.concat(failed, _.map(forbidUserList, id => _.assign({}, { id, message }))); + failed = _.concat(failed, _.map(forbidUserList, id => _.assign({}, { userId: id, message }))); updatedInvite.userIds = _.filter(invite.userIds, userId => !_.includes(forbidUserList, userId)); } } From 5106789fd1b11959f6a136046670b8a06bb075d5 Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Tue, 5 Mar 2019 16:16:24 +0800 Subject: [PATCH 3/3] remove unnecessary variable to avoid confusion --- src/routes/projectMemberInvites/create.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index a4b0860e..4839ba45 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -202,7 +202,6 @@ module.exports = [ promises.push(Promise.resolve()); } return Promise.all(promises).then((rolesList) => { - const updatedInvite = invite; if (!!invite.userIds && _.includes(PROJECT_MEMBER_MANAGER_ROLES, invite.role)) { req.log.debug('Chekcing if userId is allowed as manager'); const forbidUserList = []; @@ -217,16 +216,16 @@ module.exports = [ if (forbidUserList.length > 0) { const message = 'cannot be added with a Manager role to the project'; failed = _.concat(failed, _.map(forbidUserList, id => _.assign({}, { userId: id, message }))); - updatedInvite.userIds = _.filter(invite.userIds, userId => !_.includes(forbidUserList, userId)); + invite.userIds = _.filter(invite.userIds, userId => !_.includes(forbidUserList, userId)); } } return models.ProjectMemberInvite.getPendingInvitesForProject(projectId) .then((invites) => { const data = { projectId, - role: updatedInvite.role, + role: invite.role, // invite directly if user is admin or copilot manager - status: (updatedInvite.role !== PROJECT_MEMBER_ROLE.COPILOT || + status: (invite.role !== PROJECT_MEMBER_ROLE.COPILOT || util.hasRoles(req, [USER_ROLE.CONNECT_ADMIN, USER_ROLE.COPILOT_MANAGER])) ? INVITE_STATUS.PENDING : INVITE_STATUS.REQUESTED, @@ -235,7 +234,7 @@ module.exports = [ }; req.log.debug('Creating invites'); - return models.sequelize.Promise.all(buildCreateInvitePromises(req, updatedInvite, invites, data, failed)) + return models.sequelize.Promise.all(buildCreateInvitePromises(req, invite, invites, data, failed)) .then((values) => { values.forEach((v) => { req.app.emit(EVENT.ROUTING_KEY.PROJECT_MEMBER_INVITE_CREATED, {