Feat : Discord tag for new users #2535
Conversation
This reverts commit 6685c0c.
|
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 WalkthroughThis PR introduces automatic Discord role assignment for users when they register external accounts with their Discord IDs. It adds new configuration properties for the "New" role across environments, defines new log types for tracking role assignments, implements a utility function for assigning roles, and integrates it into the external accounts controller. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as External Accounts<br/>Controller
participant Utility as addDiscordRoleToUser<br/>Utility
participant Service as Discord Service
participant Logger
participant Config
Client->>Controller: POST external account with discordId
Controller->>Config: Get discordDeveloperRoleId
Controller->>Config: Get discordNewRoleId
Controller->>Utility: addDiscordRoleToUser(discordId, developerRoleId, "Developer")
Utility->>Service: addRoleToUser(discordId, developerRoleId)
Service-->>Utility: response
Utility->>Logger: Log ADD_ROLE_TO_USER_SUCCESS
Utility-->>Controller: success result
Controller->>Utility: addDiscordRoleToUser(discordId, newRoleId, "New")
Utility->>Service: addRoleToUser(discordId, newRoleId)
Service-->>Utility: response
Utility->>Logger: Log ADD_ROLE_TO_USER_SUCCESS
Utility-->>Controller: success result
Controller-->>Client: Account created with roles assigned
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @controllers/external-accounts.js:
- Around line 32-41: The current self-invoking async function fire-and-forgets
role assignment, so move/replace it with awaited logic: call
addDiscordRoleUtils.addDiscordRoleToUser for attributes.discordId with
developerRoleId and newRoleId and await their completion (use Promise.all to run
in parallel if desired), catch and surface any errors instead of only logging
(return an error response or propagate the exception so the HTTP response
reflects failure), and remove the un-awaited IIFE; ensure the await happens
before sending the success HTTP response so clients only receive success when
role assignment actually completed.
- Around line 27-42: Remove the inline IIFE role-assignment block from
addExternalAccountData (the block that reads attributes.discordId and calls
addDiscordRoleUtils.addDiscordRoleToUser for Developer and New and logs errors);
instead, add that same async role-assignment logic into linkExternalAccount
immediately after the unverified role is successfully removed (i.e., right after
the removal success path around where the unverified role removal completes).
Ensure you use the same discordId source available in linkExternalAccount
(extract from the linked external account or req.body as appropriate), preserve
the try/catch and logger.info/logger.error behavior, and await both
addDiscordRoleToUser calls so failures are caught and logged in the new
location.
In @utils/addDiscordRoleToUser.ts:
- Around line 21-27: The catch paths for discordServices.addRoleToUser currently
only call logger.error and skip emitting the ADD_ROLE_TO_USER_FAILED audit;
update the catch blocks around discordServices.addRoleToUser (both the first
try/catch and the second one at lines 37-41) to call
addLog(logType.ADD_ROLE_TO_USER_FAILED, { roleId, userid: discordId }, {
message: error.message || 'Unknown error' }) before rethrowing or throwing, and
include roleName/context in the log payload or message to match the non-throwing
failure branch.
- Around line 1-6: The module mixes CommonJS require with ES imports; replace
the require("./logger") with an ES import to keep module style consistent and
preserve typings—change to either `import logger from "./logger"` if logger has
a default export or `import * as logger from "./logger"` if it uses named
exports, and update any usages in this file (addDiscordRoleToUser) accordingly
to match the chosen import form.
- Around line 24-27: The log metadata uses the wrong key and the error
extraction is unsafe: in addDiscordRoleToUser update the metadata passed to
addLog (logType.ADD_ROLE_TO_USER_FAILED) to use userId (camelCase) or explicitly
discordId to match LogMeta, and when creating the thrown/logged message guard
the thrown value with an instanceof Error check and fall back to String(...) to
extract a stable message instead of accessing error.message directly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
config/default.jsconfig/production.jsconfig/staging.jsconstants/logs.tscontrollers/external-accounts.jsservices/discordService.jstest/config/test.jsutils/addDiscordRoleToUser.ts
🧰 Additional context used
🧬 Code graph analysis (3)
utils/addDiscordRoleToUser.ts (1)
constants/logs.ts (1)
logType(3-30)
controllers/external-accounts.js (1)
test/unit/middlewares/external-accounts-validator.test.js (3)
req(13-26)req(34-36)req(52-54)
services/discordService.js (16)
controllers/external-accounts.js (2)
config(9-9)logger(10-10)controllers/users.js (2)
config(35-35)logger(15-15)test/unit/models/discordactions.test.js (1)
config(4-4)controllers/auth.js (2)
config(2-2)logger(7-7)test/integration/tasks.test.js (1)
config(14-14)test/integration/auth.test.js (1)
config(8-8)controllers/discordactions.js (1)
config(3-3)test/integration/users.test.js (1)
config(18-18)test/integration/discordactions.test.js (1)
config(21-21)test/integration/extensionRequests.test.js (1)
config(11-11)test/integration/taskRequests.test.js (1)
config(24-24)utils/discord-actions.js (1)
config(2-2)test/integration/qrCodeAuth.test.js (1)
config(11-11)test/integration/events.test.js (1)
config(23-23)test/integration/usersFilter.test.js (1)
config(13-13)test/integration/external-accounts.test.js (1)
require(8-8)
⏰ 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 (6)
config/production.js (1)
7-10: Verify prod role id + bot permissions fordiscordNewRoleId.A wrong ID (or missing “Manage Roles” / role hierarchy issues) will make the new flow silently fail depending on caller handling.
config/default.js (1)
14-18: Config key added to defaults (good).Keeping
discordNewRoleIdindefault.jsmaintains the “all keys are discoverable” convention.test/config/test.js (1)
13-18: Test config updated for the new key (good).Please just confirm Discord calls are mocked in tests so the dummy role id isn’t used against real services.
constants/logs.ts (1)
16-17: No action needed.There are no key collisions between
ADD_ROLE_TO_USER_SUCCESS/ADD_ROLE_TO_USER_FAILEDand the keys inREQUEST_LOG_TYPE(which containsREQUEST_CREATED,REQUEST_APPROVED,REQUEST_REJECTED,REQUEST_BLOCKED,REQUEST_CANCELLED,REQUEST_UPDATED,PENDING_REQUEST_FOUND). The spread operator positioning poses no override risk.Likely an incorrect or invalid review comment.
services/discordService.js (1)
1-2: LGTM! Missing imports now added.These imports are necessary since the file already uses
config.get()on line 7 andlogger.error()throughout the file (lines 20, 36, 86, 107, 140). Adding them fixes the missing dependencies.config/staging.js (1)
8-8: The role ID format is valid, but you must verify this role exists in the staging Discord server.The ID
1458172929190924573follows the correct Discord snowflake format and is used properly in the code to assign the "New" role to users upon account verification. However, the application cannot validate its existence until runtime whenaddDiscordRoleToUsercalls the Discord service. Confirm with your staging server administrator that:
- This role ID exists in the staging server
- The bot has permissions to assign this role
- The role is named "New" in Discord (as expected by the code)
|
@MayankBansal12 |
Dhirenderchoudhary
changed the title
| expect(response.body).to.have.property("message"); | ||
| expect(response.body.message).to.equal( | ||
| "Your discord profile has been linked but role assignment failed. Please contact admin" | ||
| ); |
There was a problem hiding this comment.
expecation for unverified tag and negative expectation for new and dev roles are missing
…rnal accounts test
…pectations for Developer/New roles
4 participants
Date: 11 January 2026
Developer Name: DHIRENDER
Issue Ticket Number
#2531
Tech Doc Link
https://docs.google.com/document/d/1QPmzQMOfqdSzmfwB_RLpiE_1L0Hc3DdVz1b6QWq5LEA/edit?usp=sharing
Business Doc Link
https://docs.google.com/document/d/1wwEtHNn8dfmfBY0UDdJ-6iKnbJ7wLDkggLBRLqfP8U8/edit?usp=sharing
Description
We are updating the Discord verification process to make it more welcoming and visible for new community members.
Previously, verification only removed the “unverified” tag. With this update, the system will automatically assign two new roles “developers” and “new” after a user runs /verify command.
This change ensures that verified users are easily identifiable and can access the server without admin intervention.
This is a temporary change for sometime to enchance the user expirence and lowering admin load to manually give tags
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Verified locally using integration tests.
Screenshots
Screen.Recording.2026-01-14.at.3.mp4
Test Coverage
Screenshot 1
Additional Notes