Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
75 changes: 75 additions & 0 deletions controllers/discordactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,80 @@
return res.boom.badImplementation(INTERNAL_SERVER_ERROR);
}
};
const editGroupRoles = async (req, res) => {
try {
const { groupId } = req.params;
const { dev } = req.query;
const { roleName, description } = req.body;

if (!dev === "true") {
Copy link

Choose a reason for hiding this comment

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

The feature flag check contains a logical error. The expression !dev === "true" will always evaluate to false because !dev produces a boolean value which cannot equal the string "true".

The correct condition should be dev !== "true" to properly check if the feature flag is not enabled.

// Current:
if (!dev === "true") { ... }

// Corrected:
if (dev !== "true") { ... }
Suggested change
if (!dev === "true") {
if (dev !== "true") {

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

return res.boom.notImplemented("This endpoint is only enabled via feature flag");
}

if (!roleName && !description) {
return res.boom.badRequest("At least one field (roleName or description) must be provided");
}

if (roleName && (roleName.length < 3 || roleName.length > 50)) {
return res.boom.badRequest("Role name must be between 3 and 50 characters");
}
Copy link
Member

Choose a reason for hiding this comment

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

  • Why are we performing these validations in the controller?
  • Consider moving them to the validator layer so all incoming data is validated consistently before reaching the controller.


if (description && description.length > 200) {
return res.boom.badRequest("Description must not exceed 200 characters");
}

const { roleExists, existingRoles } = await discordRolesModel.isGroupRoleExists({
groupId,
});
if (!roleExists) {
return res.boom.notFound("Group role not found");
}

const roleData = existingRoles.data();

const updateRequest = {};
if (roleName) {
updateRequest.roleName = roleName;
}
if (description) {
updateRequest.description = description;
}

let discordUpdateSuccess = true;
if (updateRequest.roleName) {
const discordUpdateResponse = await discordServices.updateDiscordGroupRole(
roleData.roleid,
updateRequest.roleName,
updateRequest.description
);
Comment on lines +107 to +111
Copy link

Choose a reason for hiding this comment

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

The dev parameter from the request query is not being passed to the updateDiscordGroupRole function, though the function signature includes it as a fourth parameter. This will result in the dev flag being undefined when making the Discord API call. Consider passing req.query.dev as the fourth argument to ensure the feature flag is properly propagated to the Discord service.

Suggested change
const discordUpdateResponse = await discordServices.updateDiscordGroupRole(
roleData.roleid,
updateRequest.roleName,
updateRequest.description
);
const discordUpdateResponse = await discordServices.updateDiscordGroupRole(
roleData.roleid,
updateRequest.roleName,
updateRequest.description,
req.query.dev
);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

if (!discordUpdateResponse.success) {
discordUpdateSuccess = false;
logger.error(`Failed to update role name in Discord for groupId: ${groupId}`);
}
}

Comment on lines +106 to +117
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle Discord API failures explicitly

discordUpdateSuccess is set to false only when the service returns { success:false }, but the preceding await can throw (network, 5xx).
Wrap the call in its own try…catch to avoid an unhandled exception that would skip Firestore rollback logic.

let firestoreUpdateSuccess = true;
if (updateRequest.description) {
try {
await discordRolesModel.updateGroupRole(groupId, {
description: updateRequest.description,
});
} catch (error) {
firestoreUpdateSuccess = false;
logger.error(`Failed to update description in Firestore for groupId: ${groupId}`);
}
}
Comment on lines +98 to +128
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Firestore update arguments swapped & rolename not persisted

discordRolesModel.updateGroupRole accepts (dataToUpdate, docId) (see call at L511-515 in the same file), but here the parameters are reversed and only description is saved.
This leaves Firestore out-of-sync when the role name changes.

-        await discordRolesModel.updateGroupRole(groupId, {
-          description: updateRequest.description,
-        });
+        await discordRolesModel.updateGroupRole(
+          {
+            ...(updateRequest.roleName && { rolename: updateRequest.roleName }),
+            ...(updateRequest.description && { description: updateRequest.description }),
+          },
+          groupId
+        );

Fixing the order and including rolename keeps the DB consistent with Discord.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (roleName) {
updateRequest.roleName = roleName;
}
if (description) {
updateRequest.description = description;
}
let discordUpdateSuccess = true;
if (updateRequest.roleName) {
const discordUpdateResponse = await discordServices.updateDiscordGroupRole(
roleData.roleid,
updateRequest.roleName,
updateRequest.description
);
if (!discordUpdateResponse.success) {
discordUpdateSuccess = false;
logger.error(`Failed to update role name in Discord for groupId: ${groupId}`);
}
}
let firestoreUpdateSuccess = true;
if (updateRequest.description) {
try {
await discordRolesModel.updateGroupRole(groupId, {
description: updateRequest.description,
});
} catch (error) {
firestoreUpdateSuccess = false;
logger.error(`Failed to update description in Firestore for groupId: ${groupId}`);
}
}
let firestoreUpdateSuccess = true;
if (updateRequest.description) {
try {
await discordRolesModel.updateGroupRole(
{
...(updateRequest.roleName && { rolename: updateRequest.roleName }),
...(updateRequest.description && { description: updateRequest.description }),
},
groupId
);
} catch (error) {
firestoreUpdateSuccess = false;
logger.error(`Failed to update description in Firestore for groupId: ${groupId}`);
}
}


if (!discordUpdateSuccess || !firestoreUpdateSuccess) {
return res.boom.badImplementation("Partial update failed. Check logs for details.");
}

return res.boom.success("updated the roleName and description successfully");
} catch (error) {
logger.error(`Error while editing group role: ${error}`);
return res.boom.badImplementation("Internal server error");
}
};

/**
* Controller function to handle the soft deletion of a group role.
Expand Down Expand Up @@ -367,7 +441,7 @@
const nickNameUpdatedUsers = [];
let counter = 0;
for (let i = 0; i < usersToBeEffected.length; i++) {
const { discordId, username, first_name: firstName } = usersToBeEffected[i];

Check warning on line 444 in controllers/discordactions.js

View workflow job for this annotation

GitHub Actions / build (22.10.0)

Variable Assigned to Object Injection Sink
try {
if (counter % 10 === 0 && counter !== 0) {
await new Promise((resolve) => setTimeout(resolve, 5500));
Expand All @@ -383,7 +457,7 @@
if (message) {
counter++;
totalNicknamesUpdated.count++;
nickNameUpdatedUsers.push(usersToBeEffected[i].id);

Check warning on line 460 in controllers/discordactions.js

View workflow job for this annotation

GitHub Actions / build (22.10.0)

Generic Object Injection Sink
}
}
} catch (error) {
Expand Down Expand Up @@ -559,6 +633,7 @@

module.exports = {
getGroupsRoleId,
editGroupRoles,
createGroupRole,
getPaginatedAllGroupRoles,
addGroupRoleToMember,
Expand Down
2 changes: 2 additions & 0 deletions routes/discordactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const authenticate = require("../middlewares/authenticate");
const {
createGroupRole,
editGroupRoles,
getGroupsRoleId,
addGroupRoleToMember,
deleteRole,
Expand Down Expand Up @@ -34,6 +35,7 @@
const router = express.Router();

router.post("/groups", authenticate, checkIsVerifiedDiscord, validateGroupRoleBody, createGroupRole);
router.patch("/groups/:groupId", authenticate, authorizeRoles([SUPERUSER]), editGroupRoles);

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
router.get("/groups", authenticate, checkIsVerifiedDiscord, validateLazyLoadingParams, getPaginatedAllGroupRoles);
router.delete("/groups/:groupId", authenticate, checkIsVerifiedDiscord, authorizeRoles([SUPERUSER]), deleteGroupRole);
router.post("/roles", authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember);
Expand Down
31 changes: 31 additions & 0 deletions services/discordService.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,36 @@ const addRoleToUser = async (userid, roleid) => {
return response;
};

const updateDiscordGroupRole = async (roleId, roleName, description, dev) => {
const authToken = generateAuthTokenForCloudflare();
try {
const response = await fetch(`${DISCORD_BASE_URL}/roles/${roleId}?dev=${dev}`, {
method: "PATCH",
body: JSON.stringify({
roleName,
description,
}),
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${authToken}`,
},
});
if (response.status === 204) {
return {
success: true,
message: "Role updated successfully",
};
}
return {
success: false,
message: response.message || "Failed to update role in Discord",
};
} catch (error) {
logger.error(`Error in updating group role in discord: ${error}`);
throw new Error(error);
}
};

const removeRoleFromUser = async (roleId, discordId, userData) => {
try {
const headers = generateCloudFlareHeaders(userData);
Expand Down Expand Up @@ -142,6 +172,7 @@ const deleteGroupRoleFromDiscord = async (roleId) => {

module.exports = {
getDiscordMembers,
updateDiscordGroupRole,
getDiscordRoles,
setInDiscordFalseScript,
addRoleToUser,
Expand Down
105 changes: 105 additions & 0 deletions test/unit/models/discordactions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ const cleanDb = require("../../utils/cleanDb");
const { userPhotoVerificationData } = require("../../fixtures/user/photo-verification");
const userData = require("../../fixtures/user/user")();
const userStatusModel = require("../../../models/userStatus");
const { editGroupRoles } = require("../../../controllers/discordactions");
const discordRolesModel = require("../../../models/discordactions");
const discordServices = require("../../../services/discordService");
Comment on lines +58 to +60
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider prefixing imported module with original name.

The file imports both discordService and discordServices which could lead to confusion.

  const { editGroupRoles } = require("../../../controllers/discordactions");
  const discordRolesModel = require("../../../models/discordactions");
- const discordServices = require("../../../services/discordService");
+ const discordServices = require("../../../services/discordService"); // Ensure this is consistent with line 16

Consider using the same variable name (either discordService or discordServices) for consistency.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { editGroupRoles } = require("../../../controllers/discordactions");
const discordRolesModel = require("../../../models/discordactions");
const discordServices = require("../../../services/discordService");
// test/unit/models/discordactions.test.js
const { editGroupRoles } = require("../../../controllers/discordactions");
const discordRolesModel = require("../../../models/discordactions");
const discordServices = require("../../../services/discordService"); // Ensure this is consistent with line 16

const { getStatusData } = require("../../fixtures/userStatus/userStatus");
const usersStatusData = getStatusData();
const dataAccessLayer = require("../../../services/dataAccessLayer");
Expand Down Expand Up @@ -97,6 +100,108 @@ describe("discordactions", function () {
});
});

describe("editGroupRoles", function () {
let req, res;
Comment on lines +103 to +104
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Declare variables separately per Biome style recommendation.

Change the combined variable declaration to separate declarations according to the Biome linter recommendation.

-  let req, res;
+  let req;
+  let res;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
describe("editGroupRoles", function () {
let req, res;
describe("editGroupRoles", function () {
let req;
let res;
🧰 Tools
🪛 Biome (1.9.4)

[error] 104-104: Declare variables separately

Unsafe fix: Break out into multiple declarations

(lint/style/useSingleVarDeclarator)


beforeEach(function () {
req = {
params: {
groupId: "group1",
},
query: {
dev: "true",
},
body: {},
userData: {
id: "user1",
},
};

res = {
boom: {
badRequest: sinon.stub(),
notFound: sinon.stub(),
badImplementation: sinon.stub(),
},
Comment on lines +121 to +125
Copy link

Choose a reason for hiding this comment

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

The test setup is missing a stub for res.boom.success and res.boom.notImplemented, which are used in the controller. The success method is called when the update is successful, and notImplemented is used in the feature flag check. Without these stubs, tests will fail when testing both the happy path and feature flag functionality. Consider adding:

res = {
  boom: {
    badRequest: sinon.stub(),
    notFound: sinon.stub(),
    badImplementation: sinon.stub(),
    success: sinon.stub(),
    notImplemented: sinon.stub()
  },
  // ...
};
Suggested change
boom: {
badRequest: sinon.stub(),
notFound: sinon.stub(),
badImplementation: sinon.stub(),
},
boom: {
badRequest: sinon.stub(),
notFound: sinon.stub(),
badImplementation: sinon.stub(),
success: sinon.stub(),
notImplemented: sinon.stub(),
},

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

status: sinon.stub().returnsThis(),
json: sinon.stub(),
};

sinon.restore();
});
Comment on lines +130 to +131
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider using more precise sinon cleanup method.

Using sinon.restore() restores all stubs, spies, and mocks. For better isolation between tests, use the more specific sinon.resetHistory() to only reset call history without removing stubs.

-      sinon.restore();
+      sinon.resetHistory();

Then use afterEach(sinon.restore) at the suite level to fully clean up after all tests in the suite have completed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sinon.restore();
});
sinon.resetHistory();
});


it("should return 400 if neither roleName nor description is provided", async function () {
await editGroupRoles(req, res);

expect(res.boom.badRequest.calledWith("At least one field (roleName or description) must be provided")).to.equal(
true
);
});
Comment on lines +133 to +139
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add test for combined field validation.

The test cases cover individual validation scenarios, but it would be valuable to test the case where both fields are provided but one is invalid.

Consider adding a test case like:

it("should return 400 if roleName is valid but description exceeds limit", async function () {
  req.body = {
    roleName: "valid-name",
    description: "a".repeat(201)
  };
  
  await editGroupRoles(req, res);
  
  expect(res.boom.badRequest.calledWith("Description must not exceed 200 characters")).to.equal(true);
});


it("should return 400 if roleName is less than 3 characters", async function () {
req.body.roleName = "ab";

await editGroupRoles(req, res);

expect(res.boom.badRequest.calledWith("Role name must be between 3 and 50 characters")).to.equal(true);
});

it("should return 400 if description exceeds 200 characters", async function () {
req.body.description = "a".repeat(201);

await editGroupRoles(req, res);

expect(res.boom.badRequest.calledWith("Description must not exceed 200 characters")).to.equal(true);
});

it("should return 404 if group role does not exist", async function () {
req.body.roleName = "new-role-name";
sinon.stub(discordRolesModel, "isGroupRoleExists").resolves({
roleExists: false,
});

await editGroupRoles(req, res);

expect(res.boom.notFound.calledWith("Group role not found")).to.equal(true);
});

it("should return 500 if Discord update fails", async function () {
req.body.roleName = "new-role-name";
sinon.stub(discordRolesModel, "isGroupRoleExists").resolves({
roleExists: true,
existingRoles: {
data: () => ({
roleid: "12345",
}),
},
});
sinon.stub(discordServices, "updateDiscordGroupRole").resolves({
success: false,
});

await editGroupRoles(req, res);

expect(res.boom.badImplementation.calledWith("Partial update failed. Check logs for details.")).to.equal(true);
});
Comment on lines +168 to +185
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Verify error handling consistency in test.

The test is checking for a 500 response on Discord update failure, but it would be good to ensure the controller doesn't attempt to update Firestore after a Discord failure.

Add an additional assertion to verify that updateGroupRole is not called when the Discord update fails:

  await editGroupRoles(req, res);

  expect(res.boom.badImplementation.calledWith("Partial update failed. Check logs for details.")).to.equal(true);
+ expect(discordRolesModel.updateGroupRole.called).to.equal(false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("should return 500 if Discord update fails", async function () {
req.body.roleName = "new-role-name";
sinon.stub(discordRolesModel, "isGroupRoleExists").resolves({
roleExists: true,
existingRoles: {
data: () => ({
roleid: "12345",
}),
},
});
sinon.stub(discordServices, "updateDiscordGroupRole").resolves({
success: false,
});
await editGroupRoles(req, res);
expect(res.boom.badImplementation.calledWith("Partial update failed. Check logs for details.")).to.equal(true);
});
it("should return 500 if Discord update fails", async function () {
req.body.roleName = "new-role-name";
sinon.stub(discordRolesModel, "isGroupRoleExists").resolves({
roleExists: true,
existingRoles: {
data: () => ({
roleid: "12345",
}),
},
});
sinon.stub(discordServices, "updateDiscordGroupRole").resolves({
success: false,
});
await editGroupRoles(req, res);
expect(res.boom.badImplementation.calledWith("Partial update failed. Check logs for details.")).to.equal(true);
expect(discordRolesModel.updateGroupRole.called).to.equal(false);
});


it("should return 500 if Firestore update fails", async function () {
req.body.description = "Updated description";
sinon.stub(discordRolesModel, "isGroupRoleExists").resolves({
roleExists: true,
existingRoles: {
data: () => ({
roleid: "12345",
}),
},
});
sinon.stub(discordRolesModel, "updateGroupRole").rejects(new Error("Firestore update failed"));

await editGroupRoles(req, res);

expect(res.boom.badImplementation.calledWith("Partial update failed. Check logs for details.")).to.equal(true);
});
});

describe("getAllGroupRoles", function () {
let getStub;

Expand Down
Loading