Fix mentor status update

errors.js

@@ -22,4 +22,21 @@ class UserDoesNotExistError extends Error {
   }
 }
 
-module.exports = { MentorAlreadyExistsError, DataRequiresElevatedRoleError, UserDoesNotExistError };
+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 };

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;
 }

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);
   }
 });

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;

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');
   });
 });
 

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();