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
2 changes: 2 additions & 0 deletions src/constants/responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ export const NAME_CHANGED = "User nickname changed successfully";

export const ROLE_REMOVED = "Role Removed successfully";

export const ROLE_UPDATED = "Role updated successfully";

export const VERIFICATION_STRING =
"Please verify your discord account by clicking the link below 👇";

Expand Down
45 changes: 45 additions & 0 deletions src/controllers/editGuildRolesHandler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// This function updates group-role in discord.
import * as response from "../constants/responses";
import { IRequest } from "itty-router";
import { env } from "../typeDefinitions/default.types";
import JSONResponse from "../utils/JsonResponse";
import { verifyNodejsBackendAuthToken } from "../utils/verifyAuthToken";
import { updateRole } from "../typeDefinitions/discordMessage.types";
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Mismatch between interface and implementation.

The updateRole interface imported from discordMessage.types likely has fields roleid, rolename, and mentionable based on context, but only rolename is used in the implementation.


🏁 Script executed:

#!/bin/bash
# Check the updateRole interface definition
grep -A 10 "interface updateRole" src/typeDefinitions/discordMessage.types.ts || echo "Interface not found"

Length of output: 205


🏁 Script executed:

#!/bin/bash
# Locate the updateRole interface definition
grep -R "interface updateRole" -n -A5 .

# Display the handler to see how updateRole is used
echo
echo "----- editGuildRolesHandler.ts (first 30 lines) -----"
sed -n '1,30p' src/controllers/editGuildRolesHandler.ts

Length of output: 1893


🏁 Script executed:

#!/bin/bash
# Locate the editGuildRole implementation and display its signature
grep -R "export async function editGuildRole" -n src/utils/editGroupRole.ts || grep -R "function editGuildRole" -n src/utils/editGroupRole.ts
echo
echo "----- src/utils/editGroupRole.ts -----"
sed -n '1,50p' src/utils/editGroupRole.ts

Length of output: 1180


Align the updateRole interface with its usage

The updateRole type includes roleid and mentionable, but the handler only uses rolename (and a path‐derived roleId), and the helper always sets mentionable: true. We should either remove unused fields from the interface or wire them into the implementation:

• In src/controllers/editGuildRolesHandler.ts
– Drop roleid/mentionable from the type or pull body.mentionable if you intend to honor the client’s flag.
– Pass body.roleid instead of the path param if that’s the source of truth.

• In src/utils/editGroupRole.ts
– Update the data payload to use a mentionable parameter.
– Adjust the function signature to accept (rolename: string, roleid: string, env: env, reason?: string, mentionable?: boolean).

Example diff for using mentionable from the request:

--- a/src/controllers/editGuildRolesHandler.ts
+++ b/src/controllers/editGuildRolesHandler.ts
@@ -26,7 +26,8 @@ export async function editGuildRoleHandler(request: IRequest, env: env) {
     await verifyNodejsBackendAuthToken(authHeader, env);
     const body: updateRole = await request.json();
-    const result = await editGuildRole(body.rolename, roleId, env, reason);
+    const result = await editGuildRole(
+      body.rolename, body.roleid, env, reason, body.mentionable
+    );

     return new JSONResponse(result);
--- a/src/utils/editGroupRole.ts
+++ b/src/utils/editGroupRole.ts
@@ export async function editGuildRole(
-  rolename: string,
-  roleid: string,
-  env: env,
-  reason?: string
+) rolename: string,
+) roleid: string,
+) env: env,
+) reason?: string,
+) mentionable: boolean = true
 ) {
   const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;
@@
-  const data = {
-    name: rolename,
-    mentionable: true,
-  };
+  const data = { name: rolename, mentionable };

   try {
     const response = await fetch(editGuildRoleUrl, {

import { editGuildRole } from "../utils/editGroupRole";

Comment on lines +1 to +9
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Inconsistent naming between file and import path.

Similar to the test file, there's a naming inconsistency between the handler file name (editGuildRolesHandler.ts) and the imported utility function path (editGroupRole.ts). One uses "Guild" and the other uses "Group".

Standardize the naming convention across the project for consistency.

export async function editGuildRoleHandler(request: IRequest, env: env) {
const authHeader = request.headers.get("Authorization");
const reason = request.headers.get("X-Audit-Log-Reason");
const roleId = decodeURI(request.params?.roleId ?? "");
const { dev } = request.query;
const devFlag = dev === "true";

if (!authHeader) {
return new JSONResponse(response.BAD_SIGNATURE, { status: 401 });
}

if (!devFlag) {
return new JSONResponse(response.NOT_IMPLEMENTED, { status: 501 });
}
if (!roleId) {
return new JSONResponse(response.BAD_REQUEST, { status: 400 });
}
try {
await verifyNodejsBackendAuthToken(authHeader, env);
const body: updateRole = await request.json();
const result = await editGuildRole(body.rolename, roleId, env, reason);

if (result === response.ROLE_UPDATED) {
return new Response(null, { status: 204 });
} else {
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
Comment on lines +27 to +38
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Missing validation for request body.

The handler parses the request body as an updateRole object but doesn't validate that the required rolename field is present.

Add validation for the required fields in the request body:

   try {
     await verifyNodejsBackendAuthToken(authHeader, env);
     const body: updateRole = await request.json();
+    if (!body.rolename) {
+      return new JSONResponse(response.BAD_REQUEST, { status: 400 });
+    }
     const result = await editGuildRole(body.rolename, roleId, env, reason);
📝 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
try {
await verifyNodejsBackendAuthToken(authHeader, env);
const body: updateRole = await request.json();
const result = await editGuildRole(body.rolename, roleId, env, reason);
if (result === response.ROLE_UPDATED) {
return new Response(null, { status: 204 });
} else {
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
try {
await verifyNodejsBackendAuthToken(authHeader, env);
const body: updateRole = await request.json();
if (!body.rolename) {
return new JSONResponse(response.BAD_REQUEST, { status: 400 });
}
const result = await editGuildRole(body.rolename, roleId, env, reason);
if (result === response.ROLE_UPDATED) {
return new Response(null, { status: 204 });
} else {
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}

} catch (err) {
console.log("Error updating guild role: ", err);
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
}
Comment on lines +39 to +45
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Error handling is too generic.

The catch block logs the error but doesn't distinguish between different types of errors, such as unauthorized access, rate limiting, or server errors. This makes debugging more difficult.

Consider enhancing error logging to provide more context:

   } catch (err) {
-    console.log("Error updating guild role: ", err);
+    console.error("Error updating guild role: ", {
+      roleId,
+      error: err instanceof Error ? err.message : String(err),
+      stack: err instanceof Error ? err.stack : undefined
+    });
     return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
       status: 500,
     });
   }
📝 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
} catch (err) {
console.log("Error updating guild role: ", err);
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
}
} catch (err) {
console.error("Error updating guild role: ", {
roleId,
error: err instanceof Error ? err.message : String(err),
stack: err instanceof Error ? err.stack : undefined
});
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
}

Comment on lines +1 to +45
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 documentation for feature flag.

The handler implements a feature flag (dev=true) but doesn't document this in the file comments. Adding documentation would help other developers understand when and how the feature can be enabled.

Enhance the file comment to explain the feature flag:

-// This function updates group-role in discord.
+// This function updates group-role in discord.
+// Feature is behind a feature flag: set query parameter dev=true to enable it.
+// Example: PATCH /roles/:roleId?dev=true
📝 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
// This function updates group-role in discord.
import * as response from "../constants/responses";
import { IRequest } from "itty-router";
import { env } from "../typeDefinitions/default.types";
import JSONResponse from "../utils/JsonResponse";
import { verifyNodejsBackendAuthToken } from "../utils/verifyAuthToken";
import { updateRole } from "../typeDefinitions/discordMessage.types";
import { editGuildRole } from "../utils/editGroupRole";
export async function editGuildRoleHandler(request: IRequest, env: env) {
const authHeader = request.headers.get("Authorization");
const reason = request.headers.get("X-Audit-Log-Reason");
const roleId = decodeURI(request.params?.roleId ?? "");
const { dev } = request.query;
const devFlag = dev === "true";
if (!authHeader) {
return new JSONResponse(response.BAD_SIGNATURE, { status: 401 });
}
if (!devFlag) {
return new JSONResponse(response.NOT_IMPLEMENTED, { status: 501 });
}
if (!roleId) {
return new JSONResponse(response.BAD_REQUEST, { status: 400 });
}
try {
await verifyNodejsBackendAuthToken(authHeader, env);
const body: updateRole = await request.json();
const result = await editGuildRole(body.rolename, roleId, env, reason);
if (result === response.ROLE_UPDATED) {
return new Response(null, { status: 204 });
} else {
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
} catch (err) {
console.log("Error updating guild role: ", err);
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
}
// This function updates group-role in discord.
// Feature is behind a feature flag: set query parameter dev=true to enable it.
// Example: PATCH /roles/:roleId?dev=true
import * as response from "../constants/responses";
import { IRequest } from "itty-router";
import { env } from "../typeDefinitions/default.types";
import JSONResponse from "../utils/JsonResponse";
import { verifyNodejsBackendAuthToken } from "../utils/verifyAuthToken";
import { updateRole } from "../typeDefinitions/discordMessage.types";
import { editGuildRole } from "../utils/editGroupRole";
export async function editGuildRoleHandler(request: IRequest, env: env) {
const authHeader = request.headers.get("Authorization");
const reason = request.headers.get("X-Audit-Log-Reason");
const roleId = decodeURI(request.params?.roleId ?? "");
const { dev } = request.query;
const devFlag = dev === "true";
if (!authHeader) {
return new JSONResponse(response.BAD_SIGNATURE, { status: 401 });
}
if (!devFlag) {
return new JSONResponse(response.NOT_IMPLEMENTED, { status: 501 });
}
if (!roleId) {
return new JSONResponse(response.BAD_REQUEST, { status: 400 });
}
try {
await verifyNodejsBackendAuthToken(authHeader, env);
const body: updateRole = await request.json();
const result = await editGuildRole(body.rolename, roleId, env, reason);
if (result === response.ROLE_UPDATED) {
return new Response(null, { status: 204 });
} else {
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
} catch (err) {
console.log("Error updating guild role: ", err);
return new JSONResponse(response.INTERNAL_SERVER_ERROR, {
status: 500,
});
}
}

3 changes: 3 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { sendTaskUpdatesHandler } from "./controllers/taskUpdatesHandler";

import config, { loadEnv } from "./../config/config";
import { deleteGuildRoleHandler } from "./controllers/deleteGuildRoleHandler";
import { editGuildRoleHandler } from "./controllers/editGuildRolesHandler";

const router = Router();

Expand Down Expand Up @@ -65,6 +66,8 @@ router.post("/profile/blocked", sendProfileBlockedMessage);

router.post("/task/update", sendTaskUpdatesHandler);

router.patch("/roles/:roleId", editGuildRoleHandler);

router.get("/ankush", async (request, env, ctx: ExecutionContext) => {
ctx.waitUntil(send(env));

Expand Down
5 changes: 5 additions & 0 deletions src/typeDefinitions/discordMessage.types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ export interface createNewRole {
rolename: string;
mentionable: boolean;
}
export interface updateRole {
roleid: string;
rolename: string;
mentionable: boolean;
}
Comment on lines +93 to +97
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Interface definition looks good

The updateRole interface correctly defines the structure needed for role updates with appropriate types for each property.

One nitpick: Consider using camelCase for property names (roleId instead of roleid) to maintain consistency with TypeScript naming conventions.

export interface updateRole {
-  roleid: string;
+  roleId: string;
  rolename: string;
  mentionable: boolean;
}
📝 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
export interface updateRole {
roleid: string;
rolename: string;
mentionable: boolean;
}
export interface updateRole {
roleId: string;
rolename: string;
mentionable: boolean;
}

export interface memberGroupRole {
userid: string;
roleid: string;
Expand Down
37 changes: 37 additions & 0 deletions src/utils/editGroupRole.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { INTERNAL_SERVER_ERROR, ROLE_UPDATED } from "../constants/responses";
import { DISCORD_BASE_URL } from "../constants/urls";
import { env } from "../typeDefinitions/default.types";

import createDiscordHeaders from "./createDiscordHeaders";

export async function editGuildRole(
rolename: string,
roleid: string,
env: env,
reason?: string
) {
const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;

const headers: HeadersInit = createDiscordHeaders({
reason,
token: env.DISCORD_TOKEN,
});
const data = {
name: rolename,
mentionable: true,
};
try {
const response = await fetch(editGuildRoleUrl, {
method: "PATCH",
headers,
body: JSON.stringify(data),
});
if (response.ok) {
return ROLE_UPDATED;
} else {
return INTERNAL_SERVER_ERROR;
}
} catch (err) {
return INTERNAL_SERVER_ERROR;
}
}
Comment on lines +7 to +37
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Function implementation needs improvement

The function correctly implements the Discord API call to update a role, but has a few issues:

  1. The function parameter list doesn't match the updateRole interface - it doesn't accept the mentionable parameter but hardcodes it to true
  2. There's no detailed error logging, which will make debugging difficult

Consider these improvements:

export async function editGuildRole(
  rolename: string,
  roleid: string,
  env: env,
+  mentionable: boolean = true,
  reason?: string
) {
  const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;

  const headers: HeadersInit = createDiscordHeaders({
    reason,
    token: env.DISCORD_TOKEN,
  });
  const data = {
    name: rolename,
-    mentionable: true,
+    mentionable,
  };
  try {
    const response = await fetch(editGuildRoleUrl, {
      method: "PATCH",
      headers,
      body: JSON.stringify(data),
    });
    if (response.ok) {
      return ROLE_UPDATED;
    } else {
+     const errorData = await response.json().catch(() => null);
+     console.error("Failed to update role:", response.status, errorData);
      return INTERNAL_SERVER_ERROR;
    }
  } catch (err) {
+   console.error("Exception when updating role:", err);
    return INTERNAL_SERVER_ERROR;
  }
}

Let's verify if the mentionable property is used elsewhere:


🏁 Script executed:

#!/bin/bash
# Search for references to mentionable in the codebase to see if the property should be configurable
rg "mentionable" --type ts -A 3 -B 3

Length of output: 3413


Align function signature with updateRole interface and improve error logging

To ensure consistency with the updateRole type and make debugging easier:

• File needing changes:

  • src/utils/editGroupRole.ts

• What to update:

  1. Add a mentionable: boolean = true parameter.
  2. Use mentionable instead of hard-coding true in the request body.
  3. Log response errors and caught exceptions for better visibility.
 export async function editGuildRole(
   rolename: string,
   roleid: string,
   env: env,
+  mentionable: boolean = true,
   reason?: string
 ) {
   const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;

   const headers: HeadersInit = createDiscordHeaders({
     reason,
     token: env.DISCORD_TOKEN,
   });
   const data = {
     name: rolename,
-    mentionable: true,
+    mentionable,
   };
   try {
     const response = await fetch(editGuildRoleUrl, {
       method: "PATCH",
       headers,
       body: JSON.stringify(data),
     });
     if (response.ok) {
       return ROLE_UPDATED;
     } else {
+      const errorData = await response.json().catch(() => null);
+      console.error("Failed to update role:", response.status, errorData);
       return INTERNAL_SERVER_ERROR;
     }
   } catch (err) {
+    console.error("Exception when updating role:", err);
     return INTERNAL_SERVER_ERROR;
   }
 }
📝 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
export async function editGuildRole(
rolename: string,
roleid: string,
env: env,
reason?: string
) {
const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;
const headers: HeadersInit = createDiscordHeaders({
reason,
token: env.DISCORD_TOKEN,
});
const data = {
name: rolename,
mentionable: true,
};
try {
const response = await fetch(editGuildRoleUrl, {
method: "PATCH",
headers,
body: JSON.stringify(data),
});
if (response.ok) {
return ROLE_UPDATED;
} else {
return INTERNAL_SERVER_ERROR;
}
} catch (err) {
return INTERNAL_SERVER_ERROR;
}
}
export async function editGuildRole(
rolename: string,
roleid: string,
env: env,
mentionable: boolean = true,
reason?: string
) {
const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${env.DISCORD_GUILD_ID}/roles/${roleid}`;
const headers: HeadersInit = createDiscordHeaders({
reason,
token: env.DISCORD_TOKEN,
});
const data = {
name: rolename,
mentionable,
};
try {
const response = await fetch(editGuildRoleUrl, {
method: "PATCH",
headers,
body: JSON.stringify(data),
});
if (response.ok) {
return ROLE_UPDATED;
} else {
const errorData = await response.json().catch(() => null);
console.error("Failed to update role:", response.status, errorData);
return INTERNAL_SERVER_ERROR;
}
} catch (err) {
console.error("Exception when updating role:", err);
return INTERNAL_SERVER_ERROR;
}
}

129 changes: 129 additions & 0 deletions tests/unit/handlers/editGuildRoleHandler.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import { generateDummyRequestObject, guildEnv } from "../../fixtures/fixture";
import * as responseConstants from "../../../src/constants/responses";
import * as verifyTokenUtils from "../../../src/utils/verifyAuthToken";
import { editGuildRoleHandler } from "../../../src/controllers/editGuildRolesHandler";
import * as editGuildRoleUtils from "../../../src/utils/editGroupRole";

describe("editGuildRoleHandler", () => {
afterEach(() => {
jest.restoreAllMocks();
});
beforeEach(() => {
jest.clearAllMocks();
});
const roleId = "1A32BEX04";
it("should return BAD_SIGNATURE when authorization header is not provided", async () => {
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
method: "PATCH",
});
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(responseConstants.BAD_SIGNATURE);
});
it("should return NOT_IMPLEMENTED when dev is false", async () => {
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
query: {
dev: "false",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
});
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(responseConstants.NOT_IMPLEMENTED);
});
it("should return BAD_REQUEST when roleId is not valid", async () => {
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: "",
},
query: {
dev: "true",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
});
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(responseConstants.BAD_REQUEST);
});
it("should return INTERNAL_SERVER_ERROR when token is not verified", async () => {
const expectedResponse = responseConstants.INTERNAL_SERVER_ERROR;
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
query: {
dev: "true",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
});
jest
.spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
.mockRejectedValue(expectedResponse);
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(expectedResponse);
});
it("should return INTERNAL_SERVER_ERROR when update fails", async () => {
const expectedResponse = responseConstants.INTERNAL_SERVER_ERROR;
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
query: {
dev: "true",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
body: JSON.stringify({ rolename: "New Role Name" }),
});
jest
.spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
.mockResolvedValueOnce();
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(expectedResponse);
});
Comment on lines +79 to +99
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing implementation in the update failure test.

This test doesn't properly mock the editGuildRole function to return an error. Currently, it's likely testing JSON parsing failure rather than the role update failure scenario.

Fix the test to properly mock the role update failure:

-  const mockRequest = generateDummyRequestObject({
+  const mockRequest = generateDummyRequestObject({
     url: "/roles",
     params: {
       roleId: roleId,
     },
     query: {
       dev: "true",
     },
     method: "PATCH",
     headers: { Authorization: "Bearer testtoken" },
-    body: JSON.stringify({ rolename: "New Role Name" }),
+    json: () => Promise.resolve({ rolename: "New Role Name" }),
   });
   jest
     .spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
     .mockResolvedValueOnce();
+  jest
+    .spyOn(editGuildRoleUtils, "editGuildRole")
+    .mockResolvedValueOnce(responseConstants.INTERNAL_SERVER_ERROR);
   const response = await editGuildRoleHandler(mockRequest, guildEnv);
📝 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 INTERNAL_SERVER_ERROR when update fails", async () => {
const expectedResponse = responseConstants.INTERNAL_SERVER_ERROR;
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
query: {
dev: "true",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
body: JSON.stringify({ rolename: "New Role Name" }),
});
jest
.spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
.mockResolvedValueOnce();
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(expectedResponse);
});
it("should return INTERNAL_SERVER_ERROR when update fails", async () => {
const expectedResponse = responseConstants.INTERNAL_SERVER_ERROR;
const mockRequest = generateDummyRequestObject({
url: "/roles",
params: {
roleId: roleId,
},
query: {
dev: "true",
},
method: "PATCH",
headers: { Authorization: "Bearer testtoken" },
json: () => Promise.resolve({ rolename: "New Role Name" }),
});
jest
.spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
.mockResolvedValueOnce();
jest
.spyOn(editGuildRoleUtils, "editGuildRole")
.mockResolvedValueOnce(responseConstants.INTERNAL_SERVER_ERROR);
const response = await editGuildRoleHandler(mockRequest, guildEnv);
const jsonResponse = await response.json();
expect(jsonResponse).toEqual(expectedResponse);
});

it("should return ok response when role is updated", async () => {
const expectedResponse = new Response(null, {
status: 204,
});
const mockRequest = generateDummyRequestObject({
url: "/roles",
method: "PATCH",
params: {
roleId: roleId,
},
query: {
dev: "true",
},
headers: { Authorization: "Bearer testtoken" },
json: () => Promise.resolve({ rolename: "something" }),
});
const verifyTokenSpy = jest
.spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
.mockResolvedValueOnce();
const editGuildRoleSpy = jest
.spyOn(editGuildRoleUtils, "editGuildRole")
.mockResolvedValueOnce(responseConstants.ROLE_UPDATED);
const response = await editGuildRoleHandler(mockRequest, guildEnv);

expect(verifyTokenSpy).toHaveBeenCalledTimes(1);
expect(editGuildRoleSpy).toHaveBeenCalledTimes(1);
expect(response).toEqual(expectedResponse);
expect(response.status).toEqual(expectedResponse.status);
});
});
Comment on lines +1 to +129
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Missing test for malformed request body.

The test suite doesn't cover the scenario where the request body is malformed or doesn't contain the required fields.

Consider adding a test for handling malformed JSON:

it("should return INTERNAL_SERVER_ERROR when the request body is malformed", async () => {
  const mockRequest = generateDummyRequestObject({
    url: "/roles",
    params: {
      roleId: roleId,
    },
    query: {
      dev: "true",
    },
    method: "PATCH",
    headers: { Authorization: "Bearer testtoken" },
    json: () => Promise.reject(new Error("Invalid JSON")),
  });
  jest
    .spyOn(verifyTokenUtils, "verifyNodejsBackendAuthToken")
    .mockResolvedValueOnce();
  
  const response = await editGuildRoleHandler(mockRequest, guildEnv);
  const jsonResponse = await response.json();
  expect(jsonResponse).toEqual(responseConstants.INTERNAL_SERVER_ERROR);
  expect(response.status).toBe(500);
});

61 changes: 61 additions & 0 deletions tests/unit/utils/editGuildRole.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { DISCORD_BASE_URL } from "../../../src/constants/urls";
import { editGuildRole } from "../../../src/utils/editGroupRole";
import JSONResponse from "../../../src/utils/JsonResponse";
import { guildEnv } from "../../fixtures/fixture";
import * as response from "../../../src/constants/responses";
Comment on lines +1 to +5
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Inconsistent naming between file and import path.

There's a naming inconsistency between the test file name (editGuildRole.test.ts) and the imported function path (src/utils/editGroupRole.ts). This could lead to confusion - one uses "Guild" and the other uses "Group".

Consider standardizing the naming convention across the project for consistency.


describe("editGuildRole", () => {
const roleid = "1A32BEX04";
const rolename = "something";

const editGuildRoleUrl = `${DISCORD_BASE_URL}/guilds/${guildEnv.DISCORD_GUILD_ID}/roles/${roleid}`;
const mockRequestInit = {
method: "PATCH",
headers: {
"Content-Type": "application/json",
Authorization: `Bot ${guildEnv.DISCORD_TOKEN}`,
"X-Audit-Log-Reason": "This is reason for this action",
},
body: JSON.stringify({ name: rolename, mentionable: true }),
};
beforeEach(() => {
jest.clearAllMocks();
});
afterEach(() => {
jest.restoreAllMocks();
});
it("should pass the reason to discord as a X-Audit-Log-Reason header if provided", async () => {
jest
.spyOn(global, "fetch")
.mockImplementation((inp) => Promise.resolve(new JSONResponse(inp)));

await editGuildRole(
rolename,
roleid,
guildEnv,
"This is reason for this action"
);

expect(global.fetch).toHaveBeenCalledWith(
editGuildRoleUrl,
mockRequestInit
);
});

it("should return ROLE_UPDATED when response is ok", async () => {
const expectedResponse = new Response(null, {
status: 204,
});
jest.spyOn(global, "fetch").mockResolvedValue(expectedResponse);
const result = await editGuildRole(rolename, roleid, guildEnv);
expect(result).toEqual(response.ROLE_UPDATED);
expect(global.fetch).toHaveBeenCalledTimes(1);
});
it("should return INTERNAL_SERVER_ERROR when response is not ok", async () => {
const expectedErrorResponse = new Response(response.INTERNAL_SERVER_ERROR);
jest.spyOn(global, "fetch").mockRejectedValue(expectedErrorResponse);
const result = await editGuildRole(rolename, roleid, guildEnv);
expect(result).toEqual(response.INTERNAL_SERVER_ERROR);
expect(global.fetch).toHaveBeenCalledTimes(1);
});
Comment on lines +54 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)

Missing test for non-204 successful responses.

While the test covers the rejected Promise case, it doesn't test the scenario where the API responds with a status other than 204 but doesn't throw an error (the else branch in the utility function).

Consider adding a test for when the API responds with a non-204 status code:

it("should return INTERNAL_SERVER_ERROR when response status is not 204", async () => {
  const expectedErrorResponse = new Response(null, {
    status: 400,
  });
  jest.spyOn(global, "fetch").mockResolvedValue(expectedErrorResponse);
  const result = await editGuildRole(rolename, roleid, guildEnv);
  expect(result).toEqual(response.INTERNAL_SERVER_ERROR);
  expect(global.fetch).toHaveBeenCalledTimes(1);
});

});