Tenant code changes bbb #828
Conversation
WalkthroughPropagates tenant_code and organization_code through admin and organization controller→service calls, enriches multiple emitted event payloads with those fields, adds two internal tenant endpoints to internalAccessUrls, and introduces tenantHelper.getDomain(tenantCode). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin
participant API as Admin Controller
participant SVC as Admin Service
participant EB as Event Broadcaster
Admin->>API: Deactivate User (token with tenant_code, organization_code)
API->>SVC: deactivateUser(body, adminId, tenant_code, organization_code)
SVC->>EB: emit deactivateUpcomingSession { user_ids, tenant_code, organization_code }
EB-->>SVC: ack
SVC-->>API: result
API-->>Admin: response
sequenceDiagram
autonumber
actor Ops as Org Ops
participant C as Org Controller
participant S as Organization Service
participant EB as Event Broadcaster
Ops->>C: add/remove related orgs (token includes tenant_code)
C->>S: addRelatedOrg/removeRelatedOrg(id, related_orgs, tenant_code)
S->>EB: emit related_orgs_delta { action: PUSH/POP, tenant_code }
EB-->>S: ack
S-->>C: result
C-->>Ops: response
sequenceDiagram
autonumber
actor Sys as Internal Service
participant C as Tenant Controller (internal)
participant T as tenantHelper
participant DB as DB
Sys->>C: GET /user/v1/tenant/domain?tenant_code=...
C->>T: getDomain(tenantCode)
T->>DB: fetch tenant and primary verified domain
DB-->>T: tenant + domain
T-->>C: success(domain, tenant_code, verified)
C-->>Sys: 200 OK (domain)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/services/admin.js (1)
724-771: Critical: Deactivation not scoped to tenant/org — risk of cross-tenant updatessrc/controllers/v1/admin.js calls adminService.deactivateUser with tenant_code and organization_code, but src/services/admin.js (deactivateUser, lines ~724–771) builds a raw filter and updates users without tenant_code/organization_code checks. Replace the implementation to use userQueries.deactivateUserInOrg (or add tenant_code + organization_code constraints), return the updated user IDs, remove user sessions before broadcasting, and fail if no rows affected.
Locations: src/services/admin.js:724-771; caller: src/controllers/v1/admin.js:191
src/controllers/v1/admin.js (2)
108-115: Don’t trust tenant context from headers; use token’s tenant_codeUsing req.headers['tenant-id'] is spoofable. Use req.decodedToken.tenant_code for tenant isolation consistency and security (per codebase practice).
Apply:
- const orgAdminCreation = await adminService.addOrgAdmin( + const orgAdminCreation = await adminService.addOrgAdmin( req.body?.user_id, req.body.organization_id, req.decodedToken.id, req.body?.identifier, - req.headers?.['tenant-id'], + req.decodedToken.tenant_code, req.body?.phone_code )
155-159: Same: avoid header-sourced tenant; use token’s tenant_codeAlign with the change you made for deactivateUser and prevent cross-tenant spoofing.
Apply:
- const result = await adminService.deactivateOrg( - req.params.id, - req.headers?.['tenant-id'], - req.decodedToken.id - ) + const result = await adminService.deactivateOrg( + req.params.id, + req.decodedToken.tenant_code, + req.decodedToken.id + )
🧹 Nitpick comments (6)
src/services/userInvite.js (1)
368-375: Add tenant_code to updateOrganization event for parityDownstream consumers likely need tenant scoping just like roleChange. Include tenant_code alongside organization_code.
eventBroadcaster('updateOrganization', { requestBody: { user_id: existingUser.id, organization_id: user.organization_id, roles: currentRoles, + tenant_code: user.tenant_code, organization_code: user.organization_code }, })src/services/tenant.js (1)
788-837: LGTM: getDomain endpoint with clear not-found handlingSolid validations and response shape. Optional: if multiple verified domains exist, consider ordering (e.g., primary flag or earliest created) to make selection deterministic.
src/controllers/v1/admin.js (4)
166-172: Fix JSDoc: source of user identifier is body, not paramsDoc currently says req.params.id, but the code reads req.body.id/email.
Apply:
/** * Deactivate User * @method * @name deactivateUser - * @param {String} req.params.id - user Id. + * @param {Object} req.body - Deactivation request. + * @param {String} [req.body.id] - User ID. + * @param {String} [req.body.email] - User email. * @returns {JSON} - deactivated user response */
26-32: Use 403 Forbidden (not 400) for “not an admin”400 implies client payload issues; lack of role is authorization. Prefer 403 to match semantics (others here use 401 for unauthenticated).
Apply:
- statusCode: httpStatusCode.bad_request, + statusCode: httpStatusCode.forbidden,Repeat for both admin checks.
Also applies to: 175-181
201-206: Standardize role check helperElsewhere you use utilsHelper.validateRoleAccess; here you manually check role.title. Prefer one approach for consistency.
Example:
- if (!req.decodedToken.roles.some((role) => role.title === common.ADMIN_ROLE)) { + if (!utilsHelper.validateRoleAccess(req.decodedToken.roles, common.ADMIN_ROLE)) {Apply similarly in triggerPeriodicViewRefresh.
Also applies to: 216-221
191-193: Minor: destructure token fields for claritySmall readability win and avoids repeated lookups.
- const result = await adminService.deactivateUser(req.body, req.decodedToken.id, req.decodedToken.tenant_code, req.decodedToken.organization_code) + const { id: adminId, tenant_code: tenantCode, organization_code: organizationCode } = req.decodedToken + const result = await adminService.deactivateUser(req.body, adminId, tenantCode, organizationCode)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/constants/common.js(1 hunks)src/controllers/v1/admin.js(1 hunks)src/controllers/v1/organization.js(2 hunks)src/helpers/userInvite.js(3 hunks)src/services/admin.js(4 hunks)src/services/org-admin.js(1 hunks)src/services/organization.js(4 hunks)src/services/tenant.js(1 hunks)src/services/userInvite.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/org-admin.jssrc/services/organization.jssrc/services/tenant.jssrc/services/admin.jssrc/services/userInvite.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/organization.jssrc/controllers/v1/admin.js
🧠 Learnings (5)
📓 Common learnings
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/org-admin.jssrc/services/admin.jssrc/controllers/v1/admin.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/services/organization.jssrc/controllers/v1/organization.jssrc/services/userInvite.js
📚 Learning: 2025-07-31T08:43:35.971Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
Applied to files:
src/services/organization.js
📚 Learning: 2025-09-12T10:40:34.860Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/organization.js:831-0
Timestamp: 2025-09-12T10:40:34.860Z
Learning: In the ELEVATE-Project/user codebase, the organizationDTO.eventBodyDTO is designed to handle empty changes arrays properly, so organization update events should always be emitted even when changedValues is empty. The DTO handles this scenario correctly and there may be other reasons to emit events beyond just field changes (like metadata updates).
Applied to files:
src/services/userInvite.js
🧬 Code graph analysis (7)
src/services/org-admin.js (1)
src/services/tenant.js (1)
tokenInformation(773-777)
src/services/organization.js (2)
src/controllers/v1/organization.js (2)
tenantCode(216-216)tenantCode(261-261)src/controllers/v1/tenant.js (1)
tenantCode(171-171)
src/services/tenant.js (3)
src/controllers/v1/tenant.js (3)
tenantCode(171-171)responses(13-13)httpStatusCode(12-12)src/helpers/userInvite.js (2)
tenantQueries(34-34)tenantDomainQueries(33-33)src/services/public.js (4)
tenantQueries(2-2)responses(6-6)httpStatusCode(7-7)tenantDomainQueries(1-1)
src/services/admin.js (4)
src/services/org-admin.js (2)
organization(51-54)organization(120-123)src/services/account.js (1)
organization(549-549)src/database/queries/organization.js (2)
organization(70-74)organization(235-235)src/helpers/userInvite.js (1)
orgCode(1057-1065)
src/services/userInvite.js (4)
src/services/admin.js (4)
user(54-54)user(200-203)user(338-338)user(449-449)src/services/org-admin.js (1)
user(369-372)src/services/user.js (1)
user(48-51)src/services/account.js (10)
user(136-145)user(703-703)user(848-848)user(898-898)user(1051-1051)user(1204-1213)user(1385-1393)user(1733-1733)user(1956-1959)user(2067-2067)
src/helpers/userInvite.js (2)
src/services/org-admin.js (4)
require(22-22)require(23-23)require(24-24)user(369-372)src/services/user.js (10)
require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)user(48-51)user(319-319)user(440-440)user(490-490)user(525-525)
src/controllers/v1/admin.js (1)
src/controllers/v1/org-admin.js (3)
result(113-113)result(140-140)result(164-164)
🔇 Additional comments (11)
src/services/userInvite.js (1)
380-386: LGTM: roleChange payload now carries tenant and organization contextAdding tenant_code and organization_code improves event scoping.
Please confirm the mentoring/event consumer expects these fields to be present (no breaking schema assumptions).
src/services/org-admin.js (1)
495-501: LGTM: Added tenant and organization context to deactivateUpcomingSessionIncluding tenant_code and organization_code ensures correct scoping of cleanup in downstream services.
src/controllers/v1/organization.js (1)
189-193: LGTM: Controllers now pass tenant_code to related-org operationsSignatures align with service updates and propagate tenant scope.
Also applies to: 201-205
src/services/organization.js (2)
515-557: LGTM: addRelatedOrg includes tenant_code in event payloadEvent payload now carries tenant context; signatures align with controllers.
569-631: LGTM: removeRelatedOrg includes tenant_code in event payloadConsistent with addRelatedOrg. Minor nit (spacing around colon) is non-blocking.
src/services/admin.js (2)
605-612: LGTM: updateOrganization event enriched with organization_codeImproves downstream correlation of org context.
696-699: LGTM: deactivateUpcomingSession event includes tenant and org contextCorrect scoping for cleanup flows.
src/helpers/userInvite.js (3)
36-36: No action: added import appears unusedSkipping per guidelines (don’t flag unused imports without static analysis signals). Please remove if confirmed unused.
931-938: LGTM: updateOrganization event now includes organization_codeImproves context for downstream services.
943-951: LGTM: roleChange event carries tenant and organization contextConsistent with other flows in this PR.
src/controllers/v1/admin.js (1)
191-191: Approve — service signature and caller matchController at src/controllers/v1/admin.js:191 calls adminService.deactivateUser(req.body, req.decodedToken.id, req.decodedToken.tenant_code, req.decodedToken.organization_code); service is defined as static async deactivateUser(bodyData, loggedInUserId, tenantCode, orgCode) in src/services/admin.js (line 724). No changes required.
| '/user/v1/tenant/read/', | ||
| '/user/v1/tenant/domain/', | ||
| '/user/v1/tenant/readInternal/', |
There was a problem hiding this comment.
🧩 Analysis chain
Verify whitelist matching for new internalAccessUrls entries
If the whitelist checks exact matches (not prefix), trailing slashes and missing route params could block access. Ensure middleware matches these paths correctly and the routes exist.
🏁 Script executed:
#!/bin/bash
# 1) Find where internalAccessUrls is used to verify matching logic (exact vs prefix).
rg -n -C3 'internalAccessUrls' --type js --type ts
# 2) Locate tenant controller routes to confirm endpoints exist and match patterns.
rg -n -C3 '/tenant/(read|domain)' --type js --type ts
# 3) List all router definitions mentioning tenant to confirm exact paths.
rg -n -C3 'router\.(get|post|patch|put|delete)\(.*tenant' --type js --type tsLength of output: 4508
Normalize whitelist matching for internalAccessUrls
internalAccessUrls uses req.path.includes(path), so patterns with trailing slashes (e.g. /user/v1/tenant/read/) won’t match routes defined without them. Remove trailing slashes from these entries or trim/normalize req.path (or switch to startsWith) so the intended endpoints are correctly whitelisted.
🤖 Prompt for AI Agents
In src/constants/common.js around lines 33 to 35, the whitelist entries have
trailing slashes which fail to match when code uses req.path.includes(path);
remove the trailing slashes from '/user/v1/tenant/read/',
'/user/v1/tenant/domain/', and '/user/v1/tenant/readInternal/' so they become
'/user/v1/tenant/read', '/user/v1/tenant/domain', and
'/user/v1/tenant/readInternal'; alternatively, if you prefer matching logic
changes instead, normalize req.path by trimming trailing slashes or replace
includes() with startsWith() so route comparisons are consistent.
…o tenant_code_changes_bbb
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/helpers/userInvite.js (1)
36-36: Remove unusedorganizationimport.Line 36: the
organizationconstant from@constants/blacklistConfigisn’t referenced elsewhere and can be removed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/helpers/userInvite.js(3 hunks)src/services/admin.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
🧬 Code graph analysis (2)
src/services/admin.js (6)
src/services/org-admin.js (3)
organization(51-54)organization(120-123)bodyData(435-435)src/services/account.js (3)
organization(549-549)userIds(1554-1554)organizationCode(194-194)src/database/queries/organization.js (2)
organization(70-74)organization(235-235)src/controllers/v1/organization.js (2)
tenantCode(216-216)tenantCode(261-261)src/services/organization.js (1)
tenantCode(254-254)src/helpers/userInvite.js (1)
orgCode(1085-1093)
src/helpers/userInvite.js (2)
src/services/org-admin.js (4)
require(22-22)require(23-23)require(24-24)user(369-372)src/services/user.js (10)
require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)user(48-51)user(319-319)user(440-440)user(490-490)user(525-525)
🔇 Additional comments (6)
src/helpers/userInvite.js (2)
959-967: LGTM! Event payload enriched with organization_code.The
updateOrganizationevent payload now includesorganization_code, which properly provides organizational context for downstream consumers. The value is correctly sourced fromuser.organization_code.
969-982: LGTM! Role change event enriched with tenant and organization codes.The
roleChangeevent payload now includestenant_codeandorganization_code, providing complete tenant and organizational context. These values are correctly sourced fromuser.tenant_codeanduser.organization_code, which are guaranteed to be present per the learned context.Based on learnings
src/services/admin.js (4)
604-613: LGTM! Organization code added to event payload.The
updateOrganizationevent broadcast now includesorganization_code: organization.code, enriching the payload with organizational context for downstream consumers.
717-719: LGTM! Tenant and organization codes added to deactivation event.The
deactivateUpcomingSessionevent now includestenant_codeandorganization_codein the payload, providing complete context for session cleanup operations.
784-790: LGTM! Event payload uses new parameters correctly.The
deactivateUpcomingSessionevent broadcast correctly uses the newly addedtenantCodeandorgCodeparameters to enrich the payload with tenant and organizational context.
744-744: All deactivateUser call sites updated to include tenantCode and orgCode. Verified that the sole invocation in controllers/v1/admin.js passes all four parameters; no other usages found.
| '/user/v1/tenant/read/', | ||
| '/user/v1/tenant/domain/', |
There was a problem hiding this comment.
We cannot have public facing API (tenant/read) to be used an internal API, please add a new endpoint if needed.
| } | ||
|
|
||
| const result = await adminService.deactivateUser(req.body, req.decodedToken.id) | ||
| const result = await adminService.deactivateUser(req.body, req.decodedToken.id, req.decodedToken.tenant_code, req.decodedToken.organization_code) |
There was a problem hiding this comment.
Why do we need these changes here?
| req.body.related_orgs ? req.body.related_orgs : [], | ||
| req?.decodedToken?.tenant_code |
There was a problem hiding this comment.
We cannot have req?.decodedToken?.tenant_code as of now, as the admin tenant and orgs tenant_code might not match.
| req.body.related_orgs ? req.body.related_orgs : [], | ||
| req?.decodedToken?.tenant_code |
There was a problem hiding this comment.
Same as above comment in addRelatedOrg
| @@ -33,6 +33,7 @@ const notificationUtils = require('@utils/notification') | |||
| const tenantDomainQueries = require('@database/queries/tenantDomain') | |||
There was a problem hiding this comment.
Why do we need changes in userInvite?
| delta_organization_ids: deltaOrgs, | ||
| organization_id: id, | ||
| action: 'PUSH', | ||
| tenant_code: tenantCode |
There was a problem hiding this comment.
Same as above, the admin tenant might not match with related orgs tenant.
| } | ||
| } | ||
| static async removeRelatedOrg(id, relatedOrgs = []) { | ||
| static async removeRelatedOrg(id, relatedOrgs = [], tenantCode) { |
There was a problem hiding this comment.
Same issue as addRelatedOrg
| /** | ||
| * Get primary domain for tenant | ||
| * @method | ||
| * @name getDomain | ||
| * @param {string} tenantCode - code of the tenant | ||
| * @returns {JSON} - tenant domain information | ||
| */ |
There was a problem hiding this comment.
A proper doc string would be helpful, including information about the internal access nature of this endpoint.
| * @param {string} tenantCode - code of the tenant | ||
| * @returns {JSON} - tenant domain information | ||
| */ | ||
| static async getDomain(tenantCode) { |
There was a problem hiding this comment.
Where is the controlled for this ednpoint ?
| tenant_code: user.tenant_code, | ||
| organization_code: user.organization_code |
There was a problem hiding this comment.
User invites is no longer used as of now, and might not work as intended.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
src/controllers/v1/organization.js (2)
191-192: Verify tenant_code sourcing for cross-tenant admin scenarios.A previous review flagged that admin and organization tenant_code might not match in cross-tenant scenarios. If an admin from one tenant can manage organizations in another tenant, passing
req.decodedToken.tenant_code(the admin's tenant) instead of the target organization's tenant_code could cause incorrect tenant scoping in downstream event broadcasts.Verify whether cross-tenant admin operations are supported and if so, confirm the correct tenant_code to pass:
#!/bin/bash # Check if there's logic to handle cross-tenant admin operations rg -n -C5 'addRelatedOrg|removeRelatedOrg' --type js # Look for how tenant_code is resolved in organization service ast-grep --pattern $'static async addRelatedOrg($$$) { $$$ }'
203-204: Verify tenant_code sourcing for cross-tenant admin scenarios.Same concern as in
addRelatedOrgabove - verify thatreq.decodedToken.tenant_codeis the correct tenant_code to pass for cross-tenant admin operations.src/services/tenant.js (2)
788-794: Enhance documentation to clarify internal access nature.The documentation should explicitly mention that this endpoint is intended for internal service-to-service communication, as indicated by its inclusion in
internalAccessUrls. This will help future maintainers understand the security and access control context./** * Get primary domain for tenant + * Note: This is an internal-only endpoint for service-to-service communication. * @method * @name getDomain * @param {string} tenantCode - code of the tenant * @returns {JSON} - tenant domain information */
795-837: Verify controller endpoint exists for getDomain method.As previously noted, confirm that a corresponding controller and route exist for this service method. The method appears to be intended for internal use (based on
/user/v1/tenant/domain/ininternalAccessUrls), but the controller implementation is not present in the files under review.#!/bin/bash # Search for tenant controller methods that might call getDomain rg -n -C5 'getDomain|tenant/domain' --type js # Look for route definitions for tenant domain endpoint rg -n -C3 'router\.(get|post).*tenant.*domain' --type jssrc/constants/common.js (1)
33-34: Do not expose public-facing APIs as internal access endpoints.As previously flagged, public-facing endpoints like
/user/v1/tenant/read/should not be added tointernalAccessUrls. If internal-only access is required, create a separate endpoint (e.g.,/user/v1/tenant/readInternal/) to maintain clear separation between public and internal APIs.Additionally, the trailing slashes may cause matching issues if the middleware uses
req.path.includes(path)- consider removing them for consistency.src/services/admin.js (2)
716-719: Verify tenant_code and organization_code match the organization being deactivated.Line 718 passes
tenantCodeandorganizationCodeto the event, but these come from the method parameters (lines 653). The method fetches the org at line 655 usingorganizationCodeandtenantCode. However, confirm that the calling controller passes the correct tenant_code for the organization being deactivated, not the admin's tenant_code, as noted in previous comments.Based on past review comment: "the admin tenant might not match with related orgs tenant."
Run the following script to verify how the controller invokes this method:
#!/bin/bash # Check how deactivateOrg is called from controllers to confirm tenant_code source rg -nP 'adminService\.deactivateOrg\(' --type=js -A3
744-744: Verify tenantCode and orgCode parameters match the users being deactivated.The updated signature now accepts
tenantCodeandorgCode(line 744), which are used in the event broadcast at lines 787-788. These parameters appear to come from the admin's decoded token (as seen in controller changes). Confirm that these codes are appropriate for the users being deactivated, especially in multi-tenant scenarios where an admin might deactivate users from different organizations or tenants.Based on past review comment: "the admins org and tenant code might not match with the user he's deactivating."
Run the following script to verify the controller invocation:
#!/bin/bash # Verify how the controller calls deactivateUser and where tenantCode/orgCode originate rg -nP 'adminService\.deactivateUser\(' --type=js -A3src/controllers/v1/admin.js (1)
191-191: Verify that admin's tenant_code and organization_code are appropriate for deactivating users from potentially different organizations.Line 191 now passes
req.decodedToken.tenant_codeandreq.decodedToken.organization_codeto the service. This assumes the admin's tenant and organization context should apply to the deactivation event. However, if the admin is deactivating users from a different organization (e.g., a super-admin scenario), these codes may not match the users' actual tenant/organization.Confirm this is the intended behavior, or if the service should instead derive the tenant_code and organization_code from the users being deactivated.
Based on past review comment: "Why do we need these changes here?" and related concerns about tenant/org mismatch.
Run the following script to check how user tenant/org codes are determined during deactivation:
#!/bin/bash # Check if deactivateUser derives tenant/org from target users or uses admin's context ast-grep --pattern $'static async deactivateUser($$$) { $$$ }'src/services/organization.js (2)
515-515: Verify callers pass the organization's tenant_code, not the admin's tenant_code.The method signature now includes
tenantCodeas a third parameter (line 515), which is passed to the event broadcast at line 556. Confirm that the controller invokes this method with the tenant_code of the organization being updated (available fromorgDetailsBeforeUpdateat line 518 asorgDetailsBeforeUpdate.tenant_code), rather than the admin's tenant_code from the token.Based on past review comment: "the admin tenant might not match with related orgs tenant."
Run the following script to verify the controller invocation:
#!/bin/bash # Check how addRelatedOrg is called and which tenant_code is passed rg -nP 'orgService\.addRelatedOrg\(' --type=js -A3
569-569: Verify callers pass the organization's tenant_code, not the admin's tenant_code.Similar to
addRelatedOrg, the method signature now includestenantCodeas a third parameter (line 569), which is passed to the event broadcast at line 629. Confirm that the controller passes the organization's tenant_code, not the admin's.Based on past review comment: "Same issue as addRelatedOrg."
Run the following script to verify the controller invocation:
#!/bin/bash # Check how removeRelatedOrg is called and which tenant_code is passed rg -nP 'orgService\.removeRelatedOrg\(' --type=js -A3
🧹 Nitpick comments (3)
src/helpers/userInvite.js (1)
36-36: Unused import:organizationis not referenced.The import of
organizationfrom@constants/blacklistConfigat line 36 appears unused in this file. Consider removing it to keep imports clean.Apply this diff if the import is indeed unused:
-const { organization } = require('@constants/blacklistConfig')src/services/organization.js (2)
556-556: Consider using organization's tenant_code from database instead of parameter.Line 556 uses the
tenantCodeparameter in the event payload. SinceorgDetailsBeforeUpdateis fetched at line 518 and contains the organization'stenant_code, consider usingorgDetailsBeforeUpdate.tenant_codeinstead to ensure consistency and avoid potential mismatch if the caller passes an incorrect value.Apply this diff to use the organization's own tenant_code:
eventBroadcaster('updateRelatedOrgs', { requestBody: { delta_organization_ids: deltaOrgs, organization_id: id, action: 'PUSH', - tenant_code: tenantCode + tenant_code: orgDetailsBeforeUpdate.tenant_code }, })
629-629: Consider using organization's tenant_code from database instead of parameter.Line 629 uses the
tenantCodeparameter in the event payload. SinceorgDetailsBeforeUpdateis fetched at line 572 and contains the organization'stenant_code, consider usingorgDetailsBeforeUpdate.tenant_codeinstead to ensure consistency.Apply this diff to use the organization's own tenant_code:
eventBroadcaster('updateRelatedOrgs', { requestBody: { delta_organization_ids: relatedOrgs, organization_id: id, action: 'POP', - tenant_code:tenantCode + tenant_code: orgDetailsBeforeUpdate.tenant_code }, })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/constants/common.js(1 hunks)src/controllers/v1/admin.js(1 hunks)src/controllers/v1/organization.js(2 hunks)src/helpers/userInvite.js(3 hunks)src/services/admin.js(4 hunks)src/services/org-admin.js(1 hunks)src/services/organization.js(4 hunks)src/services/tenant.js(1 hunks)src/services/userInvite.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/tenant.jssrc/services/admin.jssrc/services/org-admin.jssrc/services/organization.jssrc/services/userInvite.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.jssrc/controllers/v1/organization.js
🧠 Learnings (2)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/admin.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/controllers/v1/organization.js
🔇 Additional comments (6)
src/services/org-admin.js (1)
498-499: LGTM! Event payload enrichment looks correct.The addition of
tenant_codeandorganization_codeto thedeactivateUpcomingSessionevent payload correctly propagates tenant and organization context to downstream consumers. The values are sourced fromtokenInformation, which is validated at the controller layer.src/services/userInvite.js (2)
373-373: Consider whether changes are needed if module is deprecated.The addition of
organization_codeto the event payload looks correct. However, a previous comment indicated that "User invites is no longer used as of now, and might not work as intended." If this module is deprecated, these changes may be unnecessary maintenance burden.Please confirm whether this module is still in active use or if these changes can be deferred.
384-385: Consider whether changes are needed if module is deprecated.The addition of
tenant_codeandorganization_codeto the roleChange event payload looks correct. However, same concern as above - verify whether this deprecated module requires these changes.src/services/admin.js (1)
610-610: LGTM: organization_code correctly sourced from fetched organization.The addition of
organization_code: organization.codeto the event payload uses the organization object fetched from the database at line 509, ensuring consistency.src/helpers/userInvite.js (2)
969-982: Ensureuserincludestenant_codeandorganization_codewhen emittingroleChange
Confirm that whereveruseris fetched or constructed, it always has these two fields populated before this block; add validation or defaults to prevent them being undefined.
958-967: Verifyuser.organization_codeis always defined whenisOrgUpdateis true
TheupdateOrganizationevent payload (lines 961–967) usesuser.organization_code, which comes from thedata.userobject passed intocreateUserInvites. Confirm that the authenticated user object always includes a validorganization_code(or add a guard/fallback) before settingisOrgUpdateto true.
Summary by CodeRabbit
New Features
Enhancements