diff --git a/errors.js b/errors.js index 22fa568..39afc00 100644 --- a/errors.js +++ b/errors.js @@ -22,4 +22,21 @@ class UserDoesNotExistError extends Error { } } -module.exports = { MentorAlreadyExistsError, DataRequiresElevatedRoleError, UserDoesNotExistError }; \ No newline at end of file +class MentorDoesNotExistError extends Error { + constructor(message = 'Mentor does not exist') { + super(message); + this.name = 'MentorDoesNotExistError'; + this.status = 404; + } +} + +class ActionNotAllowedError extends Error { + constructor(message = 'Action not allowed') { + super(message); + this.name = 'ActionNotAllowedError'; + this.status = 403; + } +} + +module.exports = { MentorAlreadyExistsError, DataRequiresElevatedRoleError, + UserDoesNotExistError, MentorDoesNotExistError, ActionNotAllowedError }; \ No newline at end of file diff --git a/repositories/mentor_repository.js b/repositories/mentor_repository.js index 905dc46..8c051fd 100644 --- a/repositories/mentor_repository.js +++ b/repositories/mentor_repository.js @@ -17,13 +17,17 @@ async function getMentorByUserId(userId) { .first(); } -async function getMentorById(allowedStatuses, selectedFields, mentorId) { +async function getMentorById(allowedStatuses, selectedFields, mentorId, safeOutput = true) { const knex = getKnex(); const mentor = await knex("mentors") .join("users", "mentors.user_id", "users.id") .where("mentors.id", mentorId) .whereIn("mentors.status", allowedStatuses) .select(selectedFields).first(); + + if (mentor !== undefined && safeOutput) { + delete mentor["email"]; + } return mentor; } diff --git a/routes/mentors.js b/routes/mentors.js index e56c1fd..cc4747d 100644 --- a/routes/mentors.js +++ b/routes/mentors.js @@ -2,12 +2,11 @@ const express = require("express"); const router = express.Router(); const mentorService = require("../services/mentor_service"); -const { ROLE_NAMES, MENTOR_STATUSES, ALLOWED_MENTOR_UPDATES } = require("../utils"); +const { ROLE_NAMES, ALLOWED_MENTOR_UPDATES } = require("../utils"); const { verifyRoles, verifyMentorOwnership, verifyOwnership, OWNED_ENTITIES } = require("../middleware/authorization"); const { isValidIntegerId, respondWithError } = require("./helpers"); -const { MentorAlreadyExistsError, DataRequiresElevatedRoleError } = require("../errors"); -const { sendEmailOnMentorApplicationStatusChange } = require("../services/mailer_service"); -const { getUserById } = require("../services/user_service"); +const { MentorAlreadyExistsError, MentorDoesNotExistError, + DataRequiresElevatedRoleError, ActionNotAllowedError } = require("../errors"); const logger = require('../logger')(__filename); router.use(express.json()); @@ -76,26 +75,17 @@ router.patch("/mentors/:id", verifyRoles([ROLE_NAMES.member]), async (req, res) } try { const isOwner = await verifyMentorOwnership(id, req.userId); - const mentorInfo = await mentorService.getMentorById(req.userRoles, id, isOwner); - if (!mentorInfo) { - return respondWithError(res, 404, "Mentor not found"); - } const mentorId = await mentorService.updateMentorStatus(req.userRoles, id, status, isOwner); - - if (status === MENTOR_STATUSES.active || status === MENTOR_STATUSES.rejected) { - const userInfo = await getUserById(mentorInfo.user_id); - if (!userInfo) { - return respondWithError(res, 404, "User not found"); - } - await sendEmailOnMentorApplicationStatusChange(userInfo.email, mentorInfo.name, status); - }; res.json({ id: mentorId }); } catch (err) { logger.error(err); - if (err instanceof DataRequiresElevatedRoleError) { + if (err instanceof DataRequiresElevatedRoleError || err instanceof ActionNotAllowedError) { return respondWithError(res, 403, err.message); } + if (err instanceof MentorDoesNotExistError) { + return respondWithError(res, 404, err.message); + } respondWithError(res); } }); diff --git a/services/mentor_service.js b/services/mentor_service.js index 2ed7cc4..79635bc 100644 --- a/services/mentor_service.js +++ b/services/mentor_service.js @@ -2,7 +2,8 @@ const repo = require('../repositories/mentor_repository'); const tagsRepo = require('../repositories/tags_repository'); const rolesService = require('../services/roles_service'); const mailerService = require('../services/mailer_service'); -const { MentorAlreadyExistsError, DataRequiresElevatedRoleError } = require("../errors"); +const { MentorAlreadyExistsError, MentorDoesNotExistError, + DataRequiresElevatedRoleError, ActionNotAllowedError } = require("../errors"); const { ROLE_NAMES, MENTOR_STATUSES } = require("../utils"); const { assignRoleToUser, removeRoleFromUser } = require('../repositories/roles_repository'); @@ -12,6 +13,7 @@ const baseFields = [ 'mentors.about', 'mentors.status', 'users.name', + 'users.email as email', 'users.img as img_link', 'users.languages' ]; @@ -76,9 +78,9 @@ async function getMentors(userId, status, includeAdditionalFields) { return mentorsWithTags; } -async function getMentorById(userRoles, mentorId, isOwner) { +async function getMentorById(userRoles, mentorId, isOwner, safeOutput = true) { const allowedStatuses = await getEligibleMentorStatuses(userRoles, isOwner); - const mentor = await repo.getMentorById(allowedStatuses, allFields, mentorId); + const mentor = await repo.getMentorById(allowedStatuses, allFields, mentorId, safeOutput); mentor.tags = await tagsRepo.getAssignedTagNames(mentorId, 'mentor'); return mentor; } @@ -93,7 +95,15 @@ async function getEligibleMentorStatuses(userRoles, isOwner) { } async function updateMentorStatus(userRoles, mentorId, status, isOwner) { - const mentorData = await getMentorById(userRoles, mentorId, isOwner); + const mentorData = await getMentorById(userRoles, mentorId, isOwner, false); + if (!mentorData) { + throw new MentorDoesNotExistError(`Mentor with id ${mentorId} does not exist`); + } + + if (mentorData.status === MENTOR_STATUSES.rejected) { + throw new ActionNotAllowedError('Cannot change status of a rejected mentor'); + } + const isAdmin = userRoles.some(role => role.role_name === ROLE_NAMES.admin); const allowedStatusesForOwner = [ MENTOR_STATUSES.active, @@ -106,10 +116,12 @@ async function updateMentorStatus(userRoles, mentorId, status, isOwner) { if(mentorData.status === MENTOR_STATUSES.pending && status === MENTOR_STATUSES.active) { updatedMentorId = await repo.updateMentorById(mentorId, { status }); await assignRoleToUser(mentorData.user_id, 2); + await mailerService.sendEmailOnMentorApplicationStatusChange(mentorData.email, mentorData.name, status); } else if(status === MENTOR_STATUSES.rejected) { // if admin rejects mentor (change mentor status to rejected => remove mentor role) updatedMentorId = await repo.updateMentorById(mentorId, { status }); await removeRoleFromUser(mentorData.user_id, 2); + await mailerService.sendEmailOnMentorApplicationStatusChange(mentorData.email, mentorData.name, status); } else { // any other status changes allowed without side effect actions updatedMentorId = await repo.updateMentorById(mentorId, { status }); @@ -149,7 +161,7 @@ async function deleteMentorById(mentorId, userId) { return; } const deletedMentorId = await repo.deleteMentorById(mentorId); - removeRoleFromUser(userId, 2); + await removeRoleFromUser(userId, 2); // remove associated tags await tagsRepo.updateMentorTags(mentorId, []); return deletedMentorId; diff --git a/tests/routes/mentors.test.js b/tests/routes/mentors.test.js index eacd903..34452ca 100644 --- a/tests/routes/mentors.test.js +++ b/tests/routes/mentors.test.js @@ -1,6 +1,6 @@ const request = require('supertest'); const mentorService = require('../../services/mentor_service'); -const { MentorAlreadyExistsError, DataRequiresElevatedRoleError } = require('../../errors'); +const { MentorAlreadyExistsError, DataRequiresElevatedRoleError, MentorDoesNotExistError } = require('../../errors'); const defautlUserId = 42; const defaultUserRoles = [{ role_name: 'member', role_id: 1 }]; @@ -226,19 +226,16 @@ describe('PATCH /mentors/:id', () => { }); it('Update mentor status - mentor not found', async () => { - mentorService.getMentorById.mockResolvedValue(null); + mentorService.updateMentorStatus.mockRejectedValue(new MentorDoesNotExistError('Mentor with id 9999 does not exist')); const res = await request(app) .patch('/mentors/9999') // some non-existing mentorId .send({ status: 'active' }) .set('userId', defautlUserId); - expect(mentorService.getMentorById) - .toHaveBeenCalledWith(defaultUserRoles, '9999', false); - - expect(mentorService.updateMentorStatus).not.toHaveBeenCalled(); + expect(mentorService.updateMentorStatus).toHaveBeenCalledWith(defaultUserRoles, '9999', 'active', false); expect(res.statusCode).toBe(404); - expect(res.body.error).toBe('Mentor not found'); + expect(res.body.error).toContain('does not exist'); }); }); diff --git a/tests/services/mentor_service.test.js b/tests/services/mentor_service.test.js index dc23dc6..33798ba 100644 --- a/tests/services/mentor_service.test.js +++ b/tests/services/mentor_service.test.js @@ -5,7 +5,7 @@ const mailerService = require('../../services/mailer_service'); const repo = require('../../repositories/mentor_repository'); const rolesRepo = require('../../repositories/roles_repository'); const tagsRepo = require('../../repositories/tags_repository'); -const { MentorAlreadyExistsError, DataRequiresElevatedRoleError } = require('../../errors'); +const { MentorAlreadyExistsError, DataRequiresElevatedRoleError, ActionNotAllowedError } = require('../../errors'); jest.mock('../../repositories/mentor_repository'); jest.mock('../../repositories/roles_repository'); @@ -159,8 +159,9 @@ describe('Mentor Service', () => { const result = await mentorService.getMentorById(userRoles, mentorId, false); - expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId); + expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId, true); expect(result).toEqual(mentor); + expect(result['email']).toBeUndefined(); }); it('Returns mentor to owner even if not active', async () => { @@ -173,8 +174,9 @@ describe('Mentor Service', () => { const result = await mentorService.getMentorById(userRoles, mentorId, true); - expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId); + expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId, true); expect(result).toEqual(mentor); + expect(result['email']).toBeUndefined(); }); it('Returns mentor to admin even if not active', async () => { @@ -187,7 +189,7 @@ describe('Mentor Service', () => { const result = await mentorService.getMentorById(userRoles, mentorId, false); - expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId); + expect(repo.getMentorById).toHaveBeenCalledWith(allowedStatuses, mentorService.allFields, mentorId, true); expect(result).toEqual(mentor); }); }); @@ -213,6 +215,9 @@ describe('Mentor Service', () => { ); expect(repo.updateMentorById).toHaveBeenCalledWith(mentorId.toString(), { status: MENTOR_STATUSES.inactive }); + expect(rolesRepo.assignRoleToUser).not.toHaveBeenCalled(); + expect(rolesRepo.removeRoleFromUser).not.toHaveBeenCalled(); + expect(mailerService.sendEmailOnMentorApplicationStatusChange).not.toHaveBeenCalled(); expect(result).toBe(mentorId); }); @@ -238,6 +243,7 @@ describe('Mentor Service', () => { ); expect(rolesRepo.assignRoleToUser).toHaveBeenCalledWith(userId, 2); expect(rolesRepo.removeRoleFromUser).not.toHaveBeenCalled(); + expect(mailerService.sendEmailOnMentorApplicationStatusChange).toHaveBeenCalled(); expect(result).toBe(mentorId); }); @@ -261,6 +267,7 @@ describe('Mentor Service', () => { expect(repo.updateMentorById).not.toHaveBeenCalled(); expect(rolesRepo.assignRoleToUser).not.toHaveBeenCalled(); expect(rolesRepo.removeRoleFromUser).not.toHaveBeenCalled(); + expect(mailerService.sendEmailOnMentorApplicationStatusChange).not.toHaveBeenCalled(); }); it('User owning the profile cannot change status when current status is pending', async () => { @@ -284,7 +291,7 @@ describe('Mentor Service', () => { expect(rolesRepo.removeRoleFromUser).not.toHaveBeenCalled(); }); - it('User owning the profile cannot change status when current status is rejected', async () => { + it('Mentor status cannot be changed when current status is rejected - user', async () => { repo.getMentorById.mockResolvedValue({ id: mentorId, user_id: userId, @@ -298,7 +305,28 @@ describe('Mentor Service', () => { MENTOR_STATUSES.inactive, true // isOwner ) - ).rejects.toBeInstanceOf(DataRequiresElevatedRoleError); + ).rejects.toBeInstanceOf(ActionNotAllowedError); + + expect(repo.updateMentorById).not.toHaveBeenCalled(); + expect(rolesRepo.assignRoleToUser).not.toHaveBeenCalled(); + expect(rolesRepo.removeRoleFromUser).not.toHaveBeenCalled(); + }); + + it('Mentor status cannot be changed when current status is rejected - admin', async () => { + repo.getMentorById.mockResolvedValue({ + id: mentorId, + user_id: userId, + status: MENTOR_STATUSES.rejected, + }); + + await expect( + mentorService.updateMentorStatus( + [{ role_name: 'admin' }], + mentorId.toString(), + MENTOR_STATUSES.inactive, + false // isOwner + ) + ).rejects.toBeInstanceOf(ActionNotAllowedError); expect(repo.updateMentorById).not.toHaveBeenCalled(); expect(rolesRepo.assignRoleToUser).not.toHaveBeenCalled();