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
64 changes: 63 additions & 1 deletion controllers/discordactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
const discordDeveloperRoleId = config.get("discordDeveloperRoleId");
const discordMavenRoleId = config.get("discordMavenRoleId");

const { setUserDiscordNickname, getDiscordMembers } = discordServices;
const { setUserDiscordNickname, getDiscordMembers, updateGroupRoleInDiscord } = discordServices;

/**
* Creates a role
Expand Down Expand Up @@ -118,6 +118,67 @@
}
};

/**
* Updates a group's name or description
* @param {Object} req - Express request object
* @param {Object} res - Express response object
*/
const updateGroupRoleDetails = async (req, res) => {
const { groupId } = req.params;
let { rolename, description } = req.body;
try {
const { roleExists, existingRoles } = await discordRolesModel.isGroupRoleExists({ groupId });
if (!roleExists) {
return res.boom.notFound("Group role not found");
}

const updateData = {};
if (rolename) {
if (!rolename.startsWith("group-")) {
rolename = `group-${rolename}`;
}

const { roleExists: nameExists, existingRoles: nameExistingRoles } =

Check failure on line 141 in controllers/discordactions.js

View workflow job for this annotation

GitHub Actions / build (22.10.0)

Replace `⏎········await·discordRolesModel.isGroupRoleExists({·rolename` with `·await·discordRolesModel.isGroupRoleExists({⏎········rolename,⏎·····`
await discordRolesModel.isGroupRoleExists({ rolename });
Comment on lines +141 to +142
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Fix formatting issues identified by ESLint.

The static analysis tools have identified formatting inconsistencies that should be addressed.

Apply this formatting fix:

-      const { roleExists: nameExists, existingRoles: nameExistingRoles } =
-        await discordRolesModel.isGroupRoleExists({ rolename });
+      const { roleExists: nameExists, existingRoles: nameExistingRoles } = 
+        await discordRolesModel.isGroupRoleExists({
+          rolename,
+        });
-      const discordUpdate = await updateGroupRoleInDiscord(
-        roleData.roleid,
-        rolename,
-        req.userData
-      );
+      const discordUpdate = await updateGroupRoleInDiscord(roleData.roleid, rolename, req.userData);

Also applies to: 163-167

🧰 Tools
🪛 ESLint

[error] 141-142: Replace ⏎········await·discordRolesModel.isGroupRoleExists({·rolename with ·await·discordRolesModel.isGroupRoleExists({⏎········rolename,⏎·····

(prettier/prettier)

🤖 Prompt for AI Agents
In controllers/discordactions.js at lines 141-142 and also lines 163-167, there
are formatting inconsistencies flagged by ESLint. Adjust the indentation,
spacing, and line breaks to conform to the project's ESLint rules, ensuring
consistent and clean code style. Review both code blocks and reformat them
accordingly to fix these issues.

if (nameExists) {
const existingId = nameExistingRoles.docs[0].id;
if (existingId !== groupId) {
return res.status(409).json({ message: "Role already exists!" });
}
}

updateData.rolename = rolename;
}
if (description !== undefined) {
updateData.description = description;
}

if (Object.keys(updateData).length === 0) {
return res.status(400).json({ message: "Nothing to update" });
}

const roleData = existingRoles.data();

if (updateData.rolename) {
const discordUpdate = await updateGroupRoleInDiscord(

Check failure on line 163 in controllers/discordactions.js

View workflow job for this annotation

GitHub Actions / build (22.10.0)

Replace `⏎········roleData.roleid,⏎········rolename,⏎········req.userData⏎······` with `roleData.roleid,·rolename,·req.userData`
roleData.roleid,
rolename,
req.userData
);
if (!discordUpdate.success) {
return res.boom.badImplementation(discordUpdate.message);
}
Comment on lines +168 to +170
Copy link

Choose a reason for hiding this comment

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

Missing error logging for Discord update failure category Error Handling

Tell me more
What is the issue?

The error handling for Discord update failure doesn't log the error, losing valuable debugging information.

Why this matters

When Discord updates fail, there's no error log created, making it impossible to track and debug these failures in production.

Suggested change ∙ Feature Preview
if (!discordUpdate.success) {
    logger.error(`Failed to update Discord role ${roleData.roleid} with name ${rolename}. Error: ${discordUpdate.message}`);
    return res.boom.badImplementation(discordUpdate.message);
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

}

await discordRolesModel.updateGroupRole(updateData, groupId);

return res.status(200).json({ message: "Group role updated successfully" });
} catch (error) {
logger.error(`Error while updating group role: ${error}`);
return res.boom.badImplementation("Internal server error");
Comment on lines +176 to +178
Copy link

Choose a reason for hiding this comment

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

Insufficient error context in logs category Error Handling

Tell me more
What is the issue?

The catch block in updateGroupRoleDetails function logs the error with minimal context and returns a generic error message.

Why this matters

Without specific error context in logs, debugging production issues becomes more difficult and time-consuming. The generic error message provides no actionable information to API consumers.

Suggested change ∙ Feature Preview
} catch (error) {
    logger.error(`Error while updating group role for groupId ${groupId}. Error details: ${error.message}. Stack: ${error.stack}`);
    return res.boom.badImplementation(`Failed to update group role: ${error.message}`);
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

}
};
Comment on lines +126 to +180
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 adding audit logging for consistency.

The implementation logic is solid with proper validation, conflict checking, and Discord-database synchronization. However, consider adding audit logging similar to the deleteGroupRole function for tracking group role modifications.

Add logging after successful update:

+    const groupUpdateLog = {
+      type: "group-role-update",
+      meta: {
+        userId: req.userData.id,
+      },
+      body: {
+        groupId: groupId,
+        updateData,
+        action: "update",
+      },
+    };
+    await addLog(groupUpdateLog.type, groupUpdateLog.meta, groupUpdateLog.body);
+
     return res.status(200).json({ message: "Group role updated successfully" });
📝 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 updateGroupRoleDetails = async (req, res) => {
const { groupId } = req.params;
let { rolename, description } = req.body;
try {
const { roleExists, existingRoles } = await discordRolesModel.isGroupRoleExists({ groupId });
if (!roleExists) {
return res.boom.notFound("Group role not found");
}
const updateData = {};
if (rolename) {
if (!rolename.startsWith("group-")) {
rolename = `group-${rolename}`;
}
const { roleExists: nameExists, existingRoles: nameExistingRoles } =
await discordRolesModel.isGroupRoleExists({ rolename });
if (nameExists) {
const existingId = nameExistingRoles.docs[0].id;
if (existingId !== groupId) {
return res.status(409).json({ message: "Role already exists!" });
}
}
updateData.rolename = rolename;
}
if (description !== undefined) {
updateData.description = description;
}
if (Object.keys(updateData).length === 0) {
return res.status(400).json({ message: "Nothing to update" });
}
const roleData = existingRoles.data();
if (updateData.rolename) {
const discordUpdate = await updateGroupRoleInDiscord(
roleData.roleid,
rolename,
req.userData
);
if (!discordUpdate.success) {
return res.boom.badImplementation(discordUpdate.message);
}
}
await discordRolesModel.updateGroupRole(updateData, groupId);
return res.status(200).json({ message: "Group role updated successfully" });
} catch (error) {
logger.error(`Error while updating group role: ${error}`);
return res.boom.badImplementation("Internal server error");
}
};
const updateGroupRoleDetails = async (req, res) => {
const { groupId } = req.params;
let { rolename, description } = req.body;
try {
const { roleExists, existingRoles } = await discordRolesModel.isGroupRoleExists({ groupId });
if (!roleExists) {
return res.boom.notFound("Group role not found");
}
const updateData = {};
if (rolename) {
if (!rolename.startsWith("group-")) {
rolename = `group-${rolename}`;
}
const { roleExists: nameExists, existingRoles: nameExistingRoles } =
await discordRolesModel.isGroupRoleExists({ rolename });
if (nameExists) {
const existingId = nameExistingRoles.docs[0].id;
if (existingId !== groupId) {
return res.status(409).json({ message: "Role already exists!" });
}
}
updateData.rolename = rolename;
}
if (description !== undefined) {
updateData.description = description;
}
if (Object.keys(updateData).length === 0) {
return res.status(400).json({ message: "Nothing to update" });
}
const roleData = existingRoles.data();
if (updateData.rolename) {
const discordUpdate = await updateGroupRoleInDiscord(
roleData.roleid,
rolename,
req.userData
);
if (!discordUpdate.success) {
return res.boom.badImplementation(discordUpdate.message);
}
}
await discordRolesModel.updateGroupRole(updateData, groupId);
// Audit log for the update
const groupUpdateLog = {
type: "group-role-update",
meta: {
userId: req.userData.id,
},
body: {
groupId: groupId,
updateData,
action: "update",
},
};
await addLog(groupUpdateLog.type, groupUpdateLog.meta, groupUpdateLog.body);
return res.status(200).json({ message: "Group role updated successfully" });
} catch (error) {
logger.error(`Error while updating group role: ${error}`);
return res.boom.badImplementation("Internal server error");
}
};
🧰 Tools
🪛 ESLint

[error] 141-142: Replace ⏎········await·discordRolesModel.isGroupRoleExists({·rolename with ·await·discordRolesModel.isGroupRoleExists({⏎········rolename,⏎·····

(prettier/prettier)


[error] 163-167: Replace ⏎········roleData.roleid,⏎········rolename,⏎········req.userData⏎······ with roleData.roleid,·rolename,·req.userData

(prettier/prettier)

🤖 Prompt for AI Agents
In controllers/discordactions.js around lines 126 to 180, after the successful
update of the group role (right after the call to
discordRolesModel.updateGroupRole), add audit logging to record the modification
event. This should include relevant details such as the user who made the
change, the groupId, and the updated fields to maintain consistency with the
existing audit logs like those in deleteGroupRole. Ensure the log entry clearly
indicates a successful update of the group role.


/**
* Fetches all group roles or provides paginated results when ?dev=true is passed.
*
Expand Down Expand Up @@ -367,7 +428,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 431 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 +444,7 @@
if (message) {
counter++;
totalNicknamesUpdated.count++;
nickNameUpdatedUsers.push(usersToBeEffected[i].id);

Check warning on line 447 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 @@ -572,5 +633,6 @@
setRoleToUsersWith31DaysPlusOnboarding,
getUserDiscordInvite,
generateInviteForUser,
updateGroupRoleDetails,
deleteGroupRole,
};
16 changes: 16 additions & 0 deletions middlewares/validators/discordactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ const validateGroupRoleBody = async (req, res, next) => {
res.boom.badRequest(error.details[0].message);
}
};

const validateGroupRoleUpdateBody = async (req, res, next) => {
const schema = Joi.object({
rolename: Joi.string().trim(),
description: Joi.string().trim(),
}).or("rolename", "description");

try {
await schema.validateAsync(req.body);
next();
} catch (error) {
logger.error(`Error validating updateGroupRole payload : ${error}`);
Copy link

Choose a reason for hiding this comment

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

Undefined Logger Reference category Logging

Tell me more
What is the issue?

The 'logger' object is being used but is not imported or defined in the file.

Why this matters

This will cause a ReferenceError when the code attempts to log errors, potentially crashing the application and preventing proper error tracking.

Suggested change ∙ Feature Preview

Import the logger at the top of the file:

const logger = require('../utils/logger');
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

res.boom.badRequest(error.details[0].message);
}
};
const validateMemberRoleBody = async (req, res, next) => {
const schema = Joi.object({
userid: Joi.string().trim().required(),
Expand Down Expand Up @@ -66,4 +81,5 @@ module.exports = {
validateMemberRoleBody,
validateLazyLoadingParams,
validateUpdateUsersNicknameStatusBody,
validateGroupRoleUpdateBody,
};
10 changes: 10 additions & 0 deletions routes/discordactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
syncDiscordGroupRolesInFirestore,
setRoleToUsersWith31DaysPlusOnboarding,
deleteGroupRole,
updateGroupRoleDetails,
getPaginatedAllGroupRoles,
} = require("../controllers/discordactions");
const {
validateGroupRoleBody,
validateMemberRoleBody,
validateUpdateUsersNicknameStatusBody,
validateLazyLoadingParams,
validateGroupRoleUpdateBody,
} = require("../middlewares/validators/discordactions");
const checkIsVerifiedDiscord = require("../middlewares/verifydiscord");
const checkCanGenerateDiscordLink = require("../middlewares/checkCanGenerateDiscordLink");
Expand All @@ -37,6 +39,14 @@
router.post("/groups", authenticate, checkIsVerifiedDiscord, validateGroupRoleBody, createGroupRole);
router.get("/groups", authenticate, checkIsVerifiedDiscord, validateLazyLoadingParams, getPaginatedAllGroupRoles);
router.delete("/groups/:groupId", authenticate, checkIsVerifiedDiscord, authorizeRoles([SUPERUSER]), deleteGroupRole);
router.patch(
"/groups/:groupId",
authenticate,

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.
checkIsVerifiedDiscord,
authorizeRoles([SUPERUSER]),
validateGroupRoleUpdateBody,
updateGroupRoleDetails
);
router.post("/roles", authenticate, checkIsVerifiedDiscord, validateMemberRoleBody, addGroupRoleToMember);
/**
* Short-circuit the GET method for this endpoint
Expand Down
21 changes: 21 additions & 0 deletions services/discordService.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,26 @@ const deleteGroupRoleFromDiscord = async (roleId) => {
}
};

const updateGroupRoleInDiscord = async (roleId, roleName, userData) => {
try {
const headers = generateCloudFlareHeaders(userData);
const response = await fetch(`${DISCORD_BASE_URL}/roles/${roleId}`, {
method: "PATCH",
headers,
body: JSON.stringify({ rolename: roleName }),
});

if (response.status === 200) {
return { success: true };
}
Comment on lines +152 to +154
Copy link

Choose a reason for hiding this comment

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

Incomplete Success Status Handling category Functionality

Tell me more
What is the issue?

The function only checks for status 200 but Discord API might return other valid success codes like 201 or 204.

Why this matters

Valid role updates might be incorrectly reported as failures if Discord returns a different success status code.

Suggested change ∙ Feature Preview

Use a range check for success status codes:

if (response.status >= 200 && response.status < 300) {
  return { success: true };
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

const data = await response.json();
return { success: false, message: data.message };
Comment on lines +155 to +156
Copy link

Choose a reason for hiding this comment

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

Unsafe Error Response Parsing category Error Handling

Tell me more
What is the issue?

The function assumes the error response will always contain a JSON body with a message property.

Why this matters

The function may throw an unexpected error if Discord returns a non-JSON error response or a different error format.

Suggested change ∙ Feature Preview

Add safety checks for error response parsing:

try {
  const data = await response.json();
  return { success: false, message: data.message || 'Unknown error occurred' };
} catch (parseError) {
  return { success: false, message: `Request failed with status ${response.status}` };
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

} catch (err) {
logger.error("Error updating role on Discord", err);
Copy link

Choose a reason for hiding this comment

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

Insufficient Error Context category Logging

Tell me more
What is the issue?

Error log missing contextual information about the roleId and roleName

Why this matters

Without the role details in the error log, debugging becomes more difficult as the specific role causing the error cannot be identified.

Suggested change ∙ Feature Preview
logger.error("Error updating role on Discord", { roleId, roleName, error: err });
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

return { success: false, message: "Internal server error" };
}
};

module.exports = {
getDiscordMembers,
getDiscordRoles,
Expand All @@ -148,4 +168,5 @@ module.exports = {
removeRoleFromUser,
setUserDiscordNickname,
deleteGroupRoleFromDiscord,
updateGroupRoleInDiscord,
};
Loading