ISSUE #5339 address feedback

backend/src/v4/models/invitations.js

@@ -21,10 +21,11 @@ const { v5Path } = require("../../interop");
 
 const db = require("../handler/db");
 const User = require("./user");
-const Role = require("./role");
+
 const { changePermissions, findModelSettings } = require("./modelSetting");
 const { findProjectsById, setUserAsProjectAdminById } = require("./project");
 const { getSecurityRestrictions }  = require(`${v5Path}/models/teamspaceSettings`);
+const Roles  = require(`${v5Path}/models/roles`);
 const { SECURITY_SETTINGS: { SSO_RESTRICTED } }  = require(`${v5Path}/models/teamspaces.constants`);
 const systemLogger = require("../logger.js").systemLogger;
 const Mailer = require("../mailer/mailer");
@@ -97,10 +98,11 @@ invitations.create = async (email, teamspace, role, username, permissions = {})
 	const projectsPermissions = permissions.projects || [];
 
 	const projectIds = projectsPermissions.map(pr => pr.project);
+	const roleId = stringToUUID(role);
 
 	const [emailUser, teamspaceRole, projects] = await Promise.all([
 		User.findByEmail(email),
-		Role.findByRole(teamspace, role),
+		Roles.getRoleById(teamspace, roleId, { _id: 1 }),
 		findProjectsById(teamspace, projectIds)
 	]);
 
@@ -130,7 +132,7 @@ invitations.create = async (email, teamspace, role, username, permissions = {})
 	const coll = await getCollection();
 	coll.ensureIndex({ "teamSpaces.teamspace": 1 }, { "background": true });
 	const result = await coll.findOne({_id:email});
-	const teamspaceEntry = { teamspace, role: stringToUUID(role), permissions };
+	const teamspaceEntry = { teamspace, role: roleId, permissions };
 
 	if (result) {
 		const teamSpaces = result.teamSpaces.filter(entry => entry.teamspace !== teamspace);
@@ -150,7 +152,7 @@ invitations.create = async (email, teamspace, role, username, permissions = {})
 		await coll.insertOne(invitation);
 		await sendInvitationEmail(email, username, teamspace);
 
-		publish(events.INVITATION_ADDED, { teamspace, executor: username, email, role: stringToUUID(role), permissions});
+		publish(events.INVITATION_ADDED, { teamspace, executor: username, email, role: roleId, permissions});
 	}
 
 	return {email, role, permissions};

backend/src/v4/models/notification.js

@@ -19,15 +19,17 @@
 const { hasWriteAccessToModelHelper, hasReadAccessToModelHelper } = require("../middlewares/checkPermissions");
 const { getModelsData } = require("./modelSetting");
 const { listProjects } = require("./project");
-const { findByRole, findUsersWithRoles } = require("./role");
 const utils = require("../utils");
 const db = require("../handler/db");
 const _ = require("lodash");
 const User = require("./user");
 
 const {v5Path} = require("../../interop");
+const { stringToUUID } = require(`${v5Path}/utils/helper/uuids`);
 const { INTERNAL_DB } = require(`${v5Path}/handler/db.constants`);
 
+const { getRoleById, getUsersByRoles } = require(`${v5Path}/models/roles`);
+
 const types = {
 	ISSUE_ASSIGNED : "ISSUE_ASSIGNED",
 	ISSUE_CLOSED: "ISSUE_CLOSED",
@@ -275,8 +277,8 @@ module.exports = {
 	 * @returns {Promise< Array<username:string,notification:Notification> >} It contains the newly created notifications and usernames
 	 */
 	upsertIssueAssignedNotifications : async function(username, teamSpace, modelId, issue) {
-		const assignedRole = issue.assigned_roles[0];
-		const rs = await findByRole(teamSpace,assignedRole);
+		const assignedRole = stringToUUID(issue.assigned_roles[0]);
+		const rs = await getRoleById(teamSpace, assignedRole);
 		if (!rs || !rs.users) {
 			return [];
 		}
@@ -344,9 +346,9 @@ module.exports = {
 			return Promise.resolve([]);
 		}
 
-		const assignedRole = issue.assigned_roles[0];
+		const assignedRole = stringToUUID(issue.assigned_roles[0]);
 
-		return findByRole(teamSpace,assignedRole)
+		return getRoleById(teamSpace,assignedRole)
 			.then(rs => {
 				if (!rs || !rs.users) {
 					return [];
@@ -373,7 +375,7 @@ module.exports = {
 		const assignedRoles = getHistoricAssignedRoles(issue);
 		const issueType = types.ISSUE_CLOSED;
 
-		const matchedUsers = await findUsersWithRoles(teamSpace, [...assignedRoles]);
+		const matchedUsers = await getUsersByRoles(teamSpace, [...assignedRoles].map(stringToUUID));
 
 		// Leave out the current user , closing the issue.
 		const users = matchedUsers.filter(m => m !== username);
@@ -393,7 +395,7 @@ module.exports = {
 
 	upsertIssueClosedNotifications: async function (username, teamSpace, modelId, issue) {
 		const assignedRoles = getHistoricAssignedRoles(issue);
-		const matchedUsers = await findUsersWithRoles(teamSpace, [...assignedRoles]);
+		const matchedUsers = await getUsersByRoles(teamSpace, [...assignedRoles].map(stringToUUID));
 
 		const users = [];
 		const getUserPromises = [];

backend/src/v4/models/user.js

@@ -48,6 +48,7 @@ const PermissionTemplates = require("./permissionTemplates");
 const { get } = require("lodash");
 const { fileExists } = require("./fileRef");
 const {v5Path} = require("../../interop");
+const { deleteIfUndefined } = require(`${v5Path}/utils/helper/objects.js`);
 const { UUIDToString } = require(`${v5Path}/utils/helper/uuids.js`);
 const { types: { strings } } = require(`${v5Path}/utils/helper/yup.js`);
 const { sanitiseRegex } = require(`${v5Path}/utils/helper/strings.js`);
@@ -869,10 +870,6 @@ User.addTeamMember = async function(teamspace, userToAdd, role, permissions, exe
 		throw (responseCodes.USER_NOT_FOUND);
 	}
 
-	if (!role) {
-		throw (responseCodes.USER_NOT_ASSIGNED_ROLE);
-	}
-
 	if (isMemberOfTeamspace(userEntry, teamspace)) {
 		throw (responseCodes.USER_ALREADY_ASSIGNED);
 	}
@@ -881,7 +878,10 @@ User.addTeamMember = async function(teamspace, userToAdd, role, permissions, exe
 	publish(events.USER_ADDED, { teamspace, executor, user: userEntry.user});
 
 	const promises = [];
-	promises.push(addUserToRole(teamspace, role, userEntry.user));
+
+	if(role) {
+		promises.push(addUserToRole(teamspace, role, userEntry.user));
+	}
 
 	const teamspaceSettings = await TeamspaceSettings.getTeamspaceSettings(teamspace);
 
@@ -891,7 +891,7 @@ User.addTeamMember = async function(teamspace, userToAdd, role, permissions, exe
 
 	await Promise.all(promises);
 
-	return  { role, permissions, ... User.getBasicDetails(userEntry) };
+	return  deleteIfUndefined({ role, permissions, ... User.getBasicDetails(userEntry) });
 };
 
 User.getBasicDetails = function(userObj) {

backend/src/v4/routes/job.js

@@ -18,7 +18,7 @@
 "use strict";
 
 const { v5Path } = require("../../interop");
-const { routeDeprecated } = require(`${v5Path}/middleware/common`);
+const { httpVerbs, routeDeprecated } = require(`${v5Path}/middleware/common`);
 
 (function () {
 	const express = require("express");
@@ -57,7 +57,7 @@ const { routeDeprecated } = require(`${v5Path}/middleware/common`);
 	 * 	color:"#ffff00"
 	 * }
 	 */
-	router.post("/jobs", routeDeprecated("POST: /v5/teamspaces/{teamspace}/roles"));
+	router.post("/jobs", routeDeprecated(httpVerbs.POST, "/v5/teamspaces/{teamspace}/roles"));
 
 	/**
 	 * @api {put} /:teamspace/jobs/:jobId Update job
@@ -82,7 +82,7 @@ const { routeDeprecated } = require(`${v5Path}/middleware/common`);
 	 * HTTP/1.1 200 OK
 	 * {}
 	 */
-	router.put("/jobs/:jobId", routeDeprecated("PATCH: /v5/teamspaces/{teamspace}/roles/{role}"));
+	router.put("/jobs/:jobId", routeDeprecated(httpVerbs.PATCH, "/v5/teamspaces/{teamspace}/roles/{role}"));
 
 	/**
 	 * @api {get} /:teamspace/jobs List all jobs
@@ -115,7 +115,7 @@ const { routeDeprecated } = require(`${v5Path}/middleware/common`);
 	 * 	}
 	 * ]
 	 */
-	router.get("/jobs", routeDeprecated("GET: /v5/teamspaces/{teamspace}/roles"));
+	router.get("/jobs", routeDeprecated(httpVerbs.GET, "/v5/teamspaces/{teamspace}/roles"));
 
 	/**
 	 * @api {post} /:teamspace/jobs/:jobId/:user Assign a job
@@ -135,7 +135,7 @@ const { routeDeprecated } = require(`${v5Path}/middleware/common`);
 	 * HTTP/1.1 200 OK
 	 * {}
 	 */
-	router.post("/jobs/:jobId/:user", routeDeprecated("PATCH: /v5/teamspaces/{teamspace}/roles/{role}"));
+	router.post("/jobs/:jobId/:user", routeDeprecated(httpVerbs.PATCH, "/v5/teamspaces/{teamspace}/roles/{role}"));
 
 	/**
 	 * @api {delete} /:teamspace/jobs/:jobId Delete a job
@@ -154,7 +154,7 @@ const { routeDeprecated } = require(`${v5Path}/middleware/common`);
 	 * HTTP/1.1 200 OK
 	 * {}
 	 */
-	router.delete("/jobs/:jobId", routeDeprecated("DELETE: /v5/teamspaces/{teamspace}/roles/{role}"));
+	router.delete("/jobs/:jobId", routeDeprecated(httpVerbs.DELETE, "/v5/teamspaces/{teamspace}/roles/{role}"));
 
 	module.exports = router;
 }());

backend/src/v5/middleware/common.js

@@ -31,9 +31,17 @@ Common.validateMany = (validators) => {
 	return validateAll;
 };
 
-Common.routeDeprecated = (newEndpoint) => (req, res) => {
-	const errorMessage = `This route is deprecated${newEndpoint ? `, please use ${newEndpoint} instead.` : '.'}`;
-	respond(req, res, createResponseCode(templates.resourceNotAvailable, errorMessage));
+Common.httpVerbs = {
+	GET: 'GET',
+	POST: 'POST',
+	PATCH: 'PATCH',
+	PUT: 'PUT',
+	DELETE: 'DELETE',
+};
+
+Common.routeDeprecated = (httpVerb, newEndpoint) => (req, res) => {
+	const errorMessage = `This route is deprecated${newEndpoint ? `, please use ${httpVerb}: ${newEndpoint} instead.` : '.'}`;
+	respond(req, res, createResponseCode(templates.endpointDecomissioned, errorMessage));
 };
 
 module.exports = Common;

backend/src/v5/models/roles.js

@@ -21,6 +21,7 @@ const COL_NAME = 'roles';
 const db = require('../handler/db');
 const { generateUUID } = require('../utils/helper/uuids');
 const { templates } = require('../utils/responseCodes');
+const { uniqueElements } = require('../utils/helper/arrays');
 
 const Roles = {};
 
@@ -53,8 +54,9 @@ Roles.getRolesToUsers = (teamspace) => findMany(teamspace, {}, { _id: 1, users:
 
 Roles.getRoles = (teamspace, projection) => findMany(teamspace, {}, projection);
 
-Roles.getRoleById = (teamspace, roleId, projection) => findOne(teamspace, { _id: roleId }, projection);
-Roles.getRoleByName = (teamspace, roleName, projection) => findOne(teamspace, { name: roleName }, projection);
+Roles.getRoleById = (teamspace, _id, projection) => findOne(teamspace, { _id }, projection);
+
+Roles.getRoleByName = (teamspace, name, projection) => findOne(teamspace, { name }, projection);
 
 Roles.createRoles = async (teamspace, roles) => {
 	await db.insertMany(teamspace, COL_NAME, roles);
@@ -82,6 +84,11 @@ Roles.getRolesByUsers = async (teamspace, users) => {
 	return roles.map((j) => j._id);
 };
 
+Roles.getUsersByRoles = async (teamspace, roleIds) => {
+	const roles = await findMany(teamspace, { _id: { $in: roleIds } }, { users: 1 });
+	return uniqueElements(roles.reduce((users, roleItem) => users.concat(roleItem.users), []));
+};
+
 Roles.updateRole = (teamspace, role, updatedRole) => updateOne(teamspace, { _id: role }, { $set: updatedRole });
 
 Roles.deleteRole = (teamspace, roleId) => db.deleteOne(teamspace, COL_NAME, { _id: roleId });

backend/src/v5/routes/teamspaces/roles.js

@@ -65,10 +65,9 @@ const updateRole = async (req, res) => {
 
 const deleteRole = async (req, res) => {
 	const { teamspace, role } = req.params;
-	const updatedRole = req.body;
 
 	try {
-		await Roles.deleteRole(teamspace, role, updatedRole);
+		await Roles.deleteRole(teamspace, role);
 		respond(req, res, templates.ok);
 	} catch (err) {
 		// istanbul ignore next
@@ -83,7 +82,7 @@ const establishRoutes = () => {
 	* /teamspaces/{teamspace}/roles:
 	*   get:
 	*     description: Get the list of roles within this teamspace
-	*     tags: [Teamspaces]
+	*     tags: [Roles]
 	*     parameters:
 	*       - name: teamspace
 	*         description: name of teamspace
@@ -134,7 +133,7 @@ const establishRoutes = () => {
 	* /teamspaces/{teamspace}/roles:
 	*   post:
 	*     description: Creates a new role
-	*     tags: [Teamspaces]
+	*     tags: [Roles]
 	*     parameters:
 	*       - name: teamspace
 	*         description: name of teamspace
@@ -188,7 +187,7 @@ const establishRoutes = () => {
 	* /teamspaces/{teamspace}/roles/{role}:
 	*   patch:
 	*     description: Updates a role
-	*     tags: [Teamspaces]
+	*     tags: [Roles]
 	*     parameters:
 	*       - name: teamspace
 	*         description: name of teamspace
@@ -236,7 +235,7 @@ const establishRoutes = () => {
 	* /teamspaces/{teamspace}/roles/{role}:
 	*   delete:
 	*     description: Deletes a role
-	*     tags: [Teamspaces]
+	*     tags: [Roles]
 	*     parameters:
 	*       - name: teamspace
 	*         description: name of teamspace

backend/src/v5/utils/responseCodes.js

@@ -97,8 +97,8 @@ ResponseCodes.templates = {
 	// Invalid Arguments
 	invalidArguments: { message: 'The arguments provided are not valid.', status: 400 },
 
-	// Resource not available
-	resourceNotAvailable: { message: 'Resource not available.', status: 410 },
+	// Endpoint Decomissioned
+	endpointDecomissioned: { message: 'Endpoint no longer available.', status: 410 },
 
 	// Queue related
 	queueConnectionError: { message: 'There was a problem connecting to the queue. Please contact support.', status: 500 },

backend/tests/v5/unit/middleware/common.test.js

@@ -69,23 +69,24 @@ const testValidateMany = () => {
 	});
 
 	describe('Route Deprecated', () => {
-		test('with no newEndpoint suggestion should respond with resourceNotAvailable', async () => {
+		test('with no newEndpoint suggestion should respond with endpointDecomissioned', async () => {
 			Responder.respond.mockResolvedValueOnce(undefined);
 
 			await Common.routeDeprecated()({}, {});
 			expect(Responder.respond).toHaveBeenCalledTimes(1);
-			expect(Responder.respond.mock.calls[0][2].code).toEqual(templates.resourceNotAvailable.code);
+			expect(Responder.respond.mock.calls[0][2].code).toEqual(templates.endpointDecomissioned.code);
 			expect(Responder.respond.mock.calls[0][2].message).toEqual('This route is deprecated.');
 		});
 
-		test('with newEndpoint suggestion should respond with resourceNotAvailable and custom message', async () => {
+		test('with newEndpoint suggestion should respond with endpointDecomissioned and custom message', async () => {
 			Responder.respond.mockResolvedValueOnce(undefined);
+			const httpVerb = generateRandomString();
 			const newEndpoint = generateRandomString();
 
-			await Common.routeDeprecated(newEndpoint)({}, {});
+			await Common.routeDeprecated(httpVerb, newEndpoint)({}, {});
 			expect(Responder.respond).toHaveBeenCalledTimes(1);
-			expect(Responder.respond.mock.calls[0][2].code).toEqual(templates.resourceNotAvailable.code);
-			expect(Responder.respond.mock.calls[0][2].message).toEqual(`This route is deprecated, please use ${newEndpoint} instead.`);
+			expect(Responder.respond.mock.calls[0][2].code).toEqual(templates.endpointDecomissioned.code);
+			expect(Responder.respond.mock.calls[0][2].message).toEqual(`This route is deprecated, please use ${httpVerb}: ${newEndpoint} instead.`);
 		});
 	});
 };