Feature flag remove ooo #2548
Merged
Feature flag remove ooo #2548
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor Discord role removal error handling by making database deletion conditional on whether the user actually has the role, and update test expectations for tolerable-error scenarios to reflect successful linking outcomes instead of failures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dhirenderchoudhary
force-pushed
the
feature_flag_remove_OOO
branch
from
5b9287f to
016e298
Compare
- Add Developer and New roles after successful Discord verification - Remove Unverified role conditionally from database - Add comprehensive test coverage for role assignment flow - Fix error handling in addDiscordRoleToUser and removeDiscordRoleFromUser
Dhirenderchoudhary
force-pushed
the
feature_flag_remove_OOO
branch
from
016e298 to
b9c4856
Compare
AnujChhikara
previously approved these changes
Jan 15, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/removeDiscordRoleFromUser.ts (1)
60-62: Avoid returningundefinedwhen non-Error exceptions are thrown.If any dependency throws a non-Error,
error.messagewill be undefined. Consider coercing to string for predictable responses.♻️ Suggested change
-logger.error(`Error removing role ${roleId} for user ${discordId}: ${error.message}`); - -return { success: false, message: error.message }; +const errMessage = error instanceof Error ? error.message : String(error); +logger.error(`Error removing role ${roleId} for user ${discordId}: ${errMessage}`); + +return { success: false, message: errMessage };
🤖 Fix all issues with AI agents
In `@test/integration/external-accounts.test.js`:
- Around line 618-644: The test currently stubs removeDiscordRoleFromUser and
addDiscordRoleToUser but only asserts the HTTP response; update the test to also
assert that addDiscordRoleUtils.addDiscordRoleToUser was called (e.g.,
Sinon.assert.calledOnce or expect(addDiscordRoleStub.calledOnce) and optionally
assert the expected arguments) to lock in that role assignment proceeded despite
the tolerable removal error; apply the same assertion additions to the companion
test that also stubs these functions (the similar test further down).
In `@utils/removeDiscordRoleFromUser.ts`:
- Line 7: Replace the CommonJS require call that assigns logger (const logger =
require("./logger")) with an ES module import to preserve TypeScript typings and
consistency; change it to an import statement that matches the logger export
(e.g., use "import logger from './logger'" if logger is the default export, or
"import { logger } from './logger'" if it's a named export), then update any
references to the logger symbol in this module (removeDiscordRoleFromUser) if
necessary to match the chosen import form.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/integration/external-accounts.test.jsutils/removeDiscordRoleFromUser.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (22.10.0)
🔇 Additional comments (1)
utils/removeDiscordRoleFromUser.ts (1)
40-42: Guard against unexpectedgetGroupRolesForUsershapes.
userDiscordRoles.groupsis assumed to exist; if it can benull/undefined, this will throw. Please confirm the contract or guard it to avoid runtime crashes.✅ Safer guard (example)
-const userDiscordRoles = await discordActions.getGroupRolesForUser(discordId); -const userHasUnverifiedRole = userDiscordRoles.groups.some((group: { roleId: string }) => group.roleId === roleId); +const userDiscordRoles = await discordActions.getGroupRolesForUser(discordId); +const groups = userDiscordRoles?.groups ?? []; +const userHasUnverifiedRole = groups.some((group: { roleId: string }) => group.roleId === roleId);
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+618
to
644
| it("Should continue with role assignment when removeDiscordRole fails because role doesn't exist (tolerable error)", async function () { | ||
| await externalAccountsModel.addExternalAccountData(externalAccountData[2]); | ||
|
|
||
| const removeDiscordRoleStub = Sinon.stub(removeDiscordRoleUtils, "removeDiscordRoleFromUser").resolves({ | ||
| success: false, | ||
| message: "Role doesn't exist", | ||
| }); | ||
|
|
||
| const addDiscordRoleStub = Sinon.stub(addDiscordRoleUtils, "addDiscordRoleToUser").resolves({ | ||
| success: true, | ||
| message: "Role added successfully", | ||
| }); | ||
|
|
||
| const response = await chai | ||
| .request(app) | ||
| .patch(`/external-accounts/link/${externalAccountData[2].token}`) | ||
| .query({ action: EXTERNAL_ACCOUNTS_POST_ACTIONS.DISCORD_USERS_SYNC }) | ||
| .set("Cookie", `${cookieName}=${newUserJWT}`); | ||
|
|
||
| const unverifiedRoleRemovalResponse = await removeDiscordRoleStub(); | ||
|
|
||
| expect(response).to.have.status(500); | ||
| expect(response).to.have.status(200); | ||
| expect(response.body).to.be.an("object"); | ||
| expect(response.body).to.have.property("message"); | ||
| expect(response.body.message).to.equal( | ||
| `User details updated but ${unverifiedRoleRemovalResponse.message}. Please contact admin` | ||
| ); | ||
| expect(response.body.message).to.equal("Your discord profile has been linked successfully"); | ||
|
|
||
| removeDiscordRoleStub.restore(); | ||
| addDiscordRoleStub.restore(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert role-assignment calls in tolerable-error tests.
These tests intend to prove that role assignment continues even when removal fails, but they don’t assert addDiscordRoleToUser calls. Add assertions to lock in the behavior.
✅ Proposed test assertions
@@
expect(response).to.have.status(200);
expect(response.body).to.be.an("object");
expect(response.body).to.have.property("message");
expect(response.body.message).to.equal("Your discord profile has been linked successfully");
+
+ expect(addDiscordRoleStub.calledTwice).to.equal(true);
+ expect(addDiscordRoleStub.firstCall.args[0]).to.equal(externalAccountData[2].attributes.discordId);
+ expect(addDiscordRoleStub.firstCall.args[2]).to.equal("Developer");
+ expect(addDiscordRoleStub.secondCall.args[0]).to.equal(externalAccountData[2].attributes.discordId);
+ expect(addDiscordRoleStub.secondCall.args[2]).to.equal("New");
@@
expect(response).to.have.status(200);
expect(response.body).to.be.an("object");
expect(response.body).to.have.property("message");
expect(response.body.message).to.equal("Your discord profile has been linked successfully");
+
+ expect(addDiscordRoleStub.calledTwice).to.equal(true);
+ expect(addDiscordRoleStub.firstCall.args[0]).to.equal(externalAccountData[2].attributes.discordId);
+ expect(addDiscordRoleStub.firstCall.args[2]).to.equal("Developer");
+ expect(addDiscordRoleStub.secondCall.args[0]).to.equal(externalAccountData[2].attributes.discordId);
+ expect(addDiscordRoleStub.secondCall.args[2]).to.equal("New");Also applies to: 672-698
🤖 Prompt for AI Agents
In `@test/integration/external-accounts.test.js` around lines 618 - 644, The test
currently stubs removeDiscordRoleFromUser and addDiscordRoleToUser but only
asserts the HTTP response; update the test to also assert that
addDiscordRoleUtils.addDiscordRoleToUser was called (e.g.,
Sinon.assert.calledOnce or expect(addDiscordRoleStub.calledOnce) and optionally
assert the expected arguments) to lock in that role assignment proceeded despite
the tolerable removal error; apply the same assertion additions to the companion
test that also stubs these functions (the similar test further down).
| import discordServices from "../services/discordService"; | ||
| import { userData } from "../types/global"; | ||
|
|
||
| const logger = require("./logger"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer ES module import for logger to keep typings and consistency.
Using require in a TS module drops type safety. If ./logger has a default export, switch to an ES import (or adjust for named export).
♻️ Suggested change (adjust export name as needed)
-const logger = require("./logger");
+import logger from "./logger";📝 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 logger = require("./logger"); | |
| import logger from "./logger"; |
🤖 Prompt for AI Agents
In `@utils/removeDiscordRoleFromUser.ts` at line 7, Replace the CommonJS require
call that assigns logger (const logger = require("./logger")) with an ES module
import to preserve TypeScript typings and consistency; change it to an import
statement that matches the logger export (e.g., use "import logger from
'./logger'" if logger is the default export, or "import { logger } from
'./logger'" if it's a named export), then update any references to the logger
symbol in this module (removeDiscordRoleFromUser) if necessary to match the
chosen import form.
- Added logic to distinguish between tolerable and non-tolerable errors - Tolerable errors (role doesn't exist, database deletion failed) now continue with role assignment - Non-tolerable errors (discord deletion failed) still return 500 - Fixes failing integration tests for external accounts link endpoint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Date:
Developer Name:
Issue Ticket Number
Tech Doc Link
Business Doc Link
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes