Staging to Master #760
Staging to Master #760
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughPropagates tenant and domain context across controllers, services, queries, models; adds multi-tenant models, many schema migrations and seeds, new controllers/endpoints (tenant, feature, organization-feature, public), Kafka event helpers and scripts, health/config updates, env keys, and a CodeRabbit review config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API
participant Controller
participant Service
participant DB
Note over Controller: extract domain/tenant from Origin/headers
Client->>API: HTTP request (Origin / tenant headers)
API->>Controller: route -> handler(req)
Controller->>Controller: extract tenant_code / domain / organization_code
Controller->>Service: invoke(params..., tenant_code, organization_code, domain)
Service->>DB: queries filtered by tenant_code / organization_code
DB-->>Service: rows
Service-->>Controller: result
Controller-->>API: response (tenant-scoped)
sequenceDiagram
autonumber
participant App
participant Broadcaster
participant Kafka
participant Microservice
App->>Broadcaster: broadcastEvent(group, body)
Broadcaster->>Kafka: push*EventsToKafka(topic, payload)
Broadcaster->>Microservice: POST to configured endpoints (parallel)
Kafka-->>Broadcaster: ack
Microservice-->>Broadcaster: 200 / health responses
Broadcaster-->>App: aggregated result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
…o org-code-fix
…flow Refactor codebase to use organization codes instead of organization IDs
added Script to hard - delete organization and related data
Refactor AdminHelper.create to add validation, role checks, and event emission
Switch user acceptance logic from organization_id to tenant_code identifier
Tenant code filter added to organization/read internal api
…redundancy in organization feature access checks.
…mline role handling
…nization features to empty set when no role mappings are found
feat: add database connection pool and timeout configurations
organization miss fix
add(role): migration to create tenant_admin role and assign permissions
feat: implement feature-role mapping
fix: improve error handling in database initialization and seeding
feat(admin): add assignRole method for validating user and role IDs
release: v3.3.24 – develop → staging
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (35)
src/distributionColumns.sql (1)
1-1: Add terminating semicolon to the first statement.The
create extension citusstatement is missing its semicolon. While most other SELECT statements in the file have been fixed, this one was overlooked and will cause the script to abort when executed via psql or migrations.Apply this diff:
-create extension citus +create extension citus;src/locales/en.json (8)
88-90: Standardize spelling: use "Organization" consistently.These lines still use the British spelling "organisation" instead of the American spelling "organization". This was flagged in a prior review.
Apply this diff to align with locale conventions:
- "ORG_DEACTIVATION_FAILED": "Failed to deactivate organisation.", - "ORG_NOT_FOUND": "Organisation not found.", - "ORG_ALREADY_INACTIVE": "Organisation is already inactive.", + "ORG_DEACTIVATION_FAILED": "Failed to deactivate organization.", + "ORG_NOT_FOUND": "Organization not found.", + "ORG_ALREADY_INACTIVE": "Organization is already inactive.",
147-147: Fix spacing in error message.The word "Atleast" should be "At least" (two words). This was flagged in a prior review.
Apply this diff:
- "TENANT_DOMAINS_ATLEAST_ONE_MANDATORY": "Cannot delete all the domains from tenant. Atleast one domain is mandatory!", + "TENANT_DOMAINS_ATLEAST_ONE_MANDATORY": "Cannot delete all the domains from tenant. At least one domain is mandatory!",
160-160: Subject–verb agreement."Organization and Tenant does not match" uses incorrect agreement; the plural subject requires "do" not "does". Also lowercase "tenant" for consistency. This was flagged in a prior review.
Apply this diff:
- "INVALID_ORG_TENANT_MAPPING": "Organization and Tenant does not match.", + "INVALID_ORG_TENANT_MAPPING": "Organization and tenant do not match.",
174-174: Fix grammar in error message."Feature already exist" should be "Feature already exists" (subject-verb agreement). This was flagged in a prior review.
Apply this diff:
- "FEATURE_ALREADY_EXISTS": "Feature already exist", + "FEATURE_ALREADY_EXISTS": "Feature already exists.",
177-177: Fix pluralization mismatch.Key "FEATURES_FETCHED_SUCCESSFULLY" is plural, but message says singular "Feature fetched"; should be "Features fetched successfully." This was flagged in a prior review.
Apply this diff:
- "FEATURES_FETCHED_SUCCESSFULLY": "Feature fetched successfully.", + "FEATURES_FETCHED_SUCCESSFULLY": "Features fetched successfully.",
178-178: Improve error message clarity."Feature delete failed" is grammatically awkward. Consider "Failed to delete feature" or "Feature deletion failed" for consistency with other error messages. This was flagged in a prior review.
Apply this diff:
- "FEATURE_NOT_DELETED": "Feature delete failed", + "FEATURE_NOT_DELETED": "Failed to delete feature.",
184-184: Plural-friendly phrasing for field errors."{{fields}} is Invalid" is not grammatically correct for both singular and plural contexts; use plural-appropriate grammar. This was flagged in a prior review.
Apply this diff:
- "UNIQUE_CONSTRAINT_ERROR": "{{fields}} is Invalid.", + "UNIQUE_CONSTRAINT_ERROR": "{{fields}} are invalid.",
193-193: Header list/grammar cleanup.The sentence structure is awkward with the comma before "and". This was flagged in a prior review.
Apply this diff:
- "ADD_ORG_HEADER": "Please provide all required organization headers: {{orgCodeHeader}}, and {{tenantCodeHeader}} for admin override.", + "ADD_ORG_HEADER": "Please provide the following organization headers for admin override: {{orgCodeHeader}} and {{tenantCodeHeader}}.",src/middlewares/authenticator.js (3)
301-305: Good fix: always set req.decodedToken for admin flow.This resolves earlier downstream breakage when override headers were used. Based on learnings.
163-176: Crash risk: org may be undefined; accessing org.roles throws. Add guard and explicit error.If decodedToken.data.organizations is empty/undefined, org is undefined → runtime error and missing org/tenant context.
- org = decodedToken.data.organizations?.[0] + org = decodedToken.data.organizations?.[0] + + if (!org) { + throw responses.failureResponse({ + message: 'USER_HAS_NO_ORGANIZATION', + statusCode: httpStatusCode.unauthorized, + responseCode: 'UNAUTHORIZED', + }) + } @@ - decodedToken.data = { + decodedToken.data = { id: decodedToken.data.id, name: decodedToken.data.name, session_id: decodedToken.data.session_id, tenant_code: decodedToken.data.tenant_code, organization_id, organization_code, - roles: org.roles, + roles: org?.roles || [], }
321-325: Avoid reusing org; use token’s roles (post-override) and guard nulls.Re-reading org.roles can crash if org was missing and does not reflect admin/tenant-admin overrides.
- const roles = org.roles + const roles = Array.isArray(decodedToken.data?.roles) ? decodedToken.data.roles : [] @@ - decodedToken.data.roles = roles //TODO: Update this to get roles from the user org roles table + decodedToken.data.roles = roles // TODO: later fetch from user-org roles tableRun to locate other risky uses of
org.roles:#!/bin/bash rg -n -C2 -g '!**/node_modules/**' -P '\borg\.roles\b'src/controllers/v1/organization.js (1)
210-254: Deduplicate admin/tenant resolution logic in add/removeRegistrationCode.The blocks are identical; extract a helper (e.g., _validateAdminAndResolveTenant(req)) and reuse. This was flagged earlier.
Also applies to: 255-299
src/controllers/v1/notification.js (1)
57-63: Validate type before single-template read (prevents ambiguous reads).Add a fast-fail check for req.query.type before calling notificationService.read; otherwise callers can get ambiguous results. This mirrors the earlier review suggestion.
} else { + if (!req.query.type || !req.query.type.trim()) { + throw responses.failureResponse({ + message: 'TYPE_QUERY_PARAM_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } const templatesData = await notificationService.read( req.params.id, req.query.code, req.query.type, req.decodedToken.organization_code, req.decodedToken.tenant_code )src/controllers/v1/organization-feature.js (6)
30-32: Return standardized failure response; don’t return raw errorsUse responses.failureResponse(...) with proper status and codes for consistency across controllers.
- } catch (error) { - return error - } + } catch (error) { + console.error('OrganizationFeature.create error:', error) + return responses.failureResponse({ + message: error.message || 'FEATURE_CREATION_FAILED', + statusCode: error.statusCode || httpStatusCode.internal_server_error, + responseCode: error.responseCode || 'SERVER_ERROR', + }) + }
48-52: Guard missing id for PATCH/DELETE before delegatingPrevent accidental ops without id and align error shape.
- // Handle delete vs update operations - return req.method === common.DELETE_METHOD + // Handle delete vs update operations + if ((req.method === common.DELETE_METHOD || req.method === 'PATCH') && !req.params?.id) { + return responses.failureResponse({ + message: 'RESOURCE_ID_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + return req.method === common.DELETE_METHOD ? await organizationFeatureService.delete(req.params.id, req.decodedToken) : await organizationFeatureService.update(req.params.id, req.body, req.decodedToken)
53-54: Return standardized failure response; don’t return raw errorsMirror create/read error handling for consistency.
- } catch (error) { - return error - } + } catch (error) { + console.error('OrganizationFeature.update error:', error) + return responses.failureResponse({ + message: error.message || 'FEATURE_UPDATE_FAILED', + statusCode: error.statusCode || httpStatusCode.internal_server_error, + responseCode: error.responseCode || 'SERVER_ERROR', + }) + }
76-82: Return a structured failure response instead of throwing itThrowing a response object adds an unnecessary hop; return it directly.
- if (!req.decodedToken.organization_code || !req.decodedToken.tenant_code) { - throw responses.failureResponse({ + if (!req.decodedToken.organization_code || !req.decodedToken.tenant_code) { + return responses.failureResponse({ message: 'ORGANIZATION_CODE_OR_TENANT_CODE_NOT_FOUND', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) }
95-97: Return standardized failure response; don’t return raw errorsAlign with other methods for consistent API responses.
- } catch (error) { - return error - } + } catch (error) { + console.error('OrganizationFeature.read error:', error) + return responses.failureResponse({ + message: error.message || 'FEATURE_READ_FAILED', + statusCode: error.statusCode || httpStatusCode.internal_server_error, + responseCode: error.responseCode || 'SERVER_ERROR', + }) + }
66-74: Fix role check: pass array, not stringutilsHelper.validateRoleAccess expects an array of roles.
- if (utilsHelper.validateRoleAccess(req.decodedToken.roles, common.ADMIN_ROLE)) { + if (utilsHelper.validateRoleAccess(req.decodedToken.roles, [common.ADMIN_ROLE])) {src/controllers/v1/org-admin.js (2)
223-229: Use const; avoid reassignable bindingentityTypeDetails isn’t reassigned; prefer const for clarity.
- let entityTypeDetails = await orgAdminService.inheritEntityType( + const entityTypeDetails = await orgAdminService.inheritEntityType(
252-267: Restore RBAC and guard upload_type to prevent crash
- Security: RBAC is commented out; restore consistent admin guard.
- Robustness: req?.body?.upload_type.toUpperCase() throws if upload_type is undefined.
Apply:
- /* if (!utilsHelper.validateRoleAccess(req.decodedToken.roles, common.ORG_ADMIN_ROLE)) { - throw responses.failureResponse({ - message: 'USER_IS_NOT_A_ADMIN', - statusCode: httpStatusCode.bad_request, - responseCode: 'CLIENT_ERROR', - }) - } - */ + if ( + !utilsHelper.validateRoleAccess(req.decodedToken.roles, [ + common.ORG_ADMIN_ROLE, + common.TENANT_ADMIN_ROLE, + ]) + ) { + return responses.failureResponse({ + message: 'USER_IS_NOT_A_ADMIN', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } const userUploadRes = await orgAdminService.bulkCreate( req.body.file_path, req.decodedToken, req?.body?.editable_fields, - req?.body?.upload_type.toUpperCase() + req?.body?.upload_type?.toUpperCase() )src/envVariables.js (3)
397-401: Align DEFAULT_TENANT_CODE casingCode uses
'default'(lowercase), but if the sample uses'DEFAULT'(uppercase), casing inconsistency will break tenant lookups.Standardize on lowercase
'default'across code, samples, and seed data to avoid case-sensitivity issues in lookups.
140-140: Breaking change: DEFAULT_ORGANISATION_CODE requires repo-wide updatesChanging the default from
'default_code'to'default'breaks existing scripts, migrations, and seed data that reference the old value.Update all references:
- Scripts:
src/scripts/insertDefaultOrg.js,src/scripts/delete-transactional-data.js- Migrations and seeders that hardcode
'default_code'- Services comparing against
DEFAULT_ORGANISATION_CODEVerify impact:
#!/bin/bash # Find all hardcoded references to old default value rg -n "default_code" --type js --type sql # Find usages of the constant rg -n "DEFAULT_ORGANISATION_CODE" --type js
383-386: Sync OTP_EXP_TIME default with .env.sampleLine 385 defaults to 300 seconds, but
src/.env.samplestill shows 86400, causing confusion.Update the sample file:
# src/.env.sample -OTP_EXP_TIME = 86400 +OTP_EXP_TIME = 300src/controllers/v1/admin.js (4)
34-35: Enforce tenant isolation — pass tenant context to deleteUserThe service call lacks tenant context, allowing cross-tenant deletes if an admin knows another tenant's user ID.
Based on past review comments, pass tenant context:
- const user = await adminService.deleteUser(req.params.id, req.decodedToken.id) + const user = await adminService.deleteUser( + req.params.id, + req.decodedToken.id, + req.decodedToken?.tenant_code || req.headers?.[common.TENANT_CODE_HEADER] + )Then update src/services/admin.js to validate the tenant in the query.
155-158: Validate tenant-id header against token to prevent cross-tenant writesLine 156 reads
tenant-idwithout verifying it matchesreq.decodedToken.tenant_code, allowing forged headers to enable cross-tenant operations.Apply validation before the service call:
+ const tenantId = req.headers?.[common.TENANT_CODE_HEADER] + if (!tenantId) { + throw responses.failureResponse({ + message: 'TENANT_ID_HEADER_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + if (req.decodedToken?.tenant_code && tenantId !== req.decodedToken.tenant_code) { + throw responses.failureResponse({ + message: 'TENANT_MISMATCH', + statusCode: httpStatusCode.forbidden, + responseCode: 'CLIENT_ERROR', + }) + } const orgAdminCreation = await adminService.addOrgAdmin( req.body?.user_id, req.body.organization_id, req.decodedToken.id, req.body?.identifier, - req.headers?.[common.TENANT_CODE_HEADER], + tenantId, req.body?.phone_code )
198-202: Enforce tenant-id validation before deactivationMissing validation of tenant-id header against
req.decodedToken.tenant_codeallows header spoofing.Validate before calling the service:
+ const tenantId = req.headers?.[common.TENANT_CODE_HEADER] + if (!tenantId) { + throw responses.failureResponse({ + message: 'TENANT_ID_HEADER_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + if (req.decodedToken?.tenant_code && tenantId !== req.decodedToken.tenant_code) { + throw responses.failureResponse({ + message: 'TENANT_MISMATCH', + statusCode: httpStatusCode.forbidden, + responseCode: 'CLIENT_ERROR', + }) + } const result = await adminService.deactivateOrg( req.params.id, - req.headers?.[common.TENANT_CODE_HEADER], + tenantId, req.decodedToken.id )
242-271: Critical: Validate and restrict raw SQL to SELECT-only queriesAccepting arbitrary SQL from
req.body.query(line 262) is a critical security risk even for admins. Without validation, this allows DDL/DML operations, multi-statement injection, and comment-based bypasses.Enforce strict validation:
const { query } = req.body + // Validate query safety + if (!query || typeof query !== 'string' || query.trim() === '') { + throw responses.failureResponse({ + message: 'QUERY_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + const trimmed = query.trim() + // Reject multiple statements, comments, or non-SELECT + if ( + /;/.test(trimmed) || + /--/.test(trimmed) || + /\/\*/.test(trimmed) || + !/^\s*select\b/i.test(trimmed) || + /\b(insert|update|delete|drop|create|alter|truncate|merge)\b/i.test(trimmed) + ) { + throw responses.failureResponse({ + message: 'ONLY_SELECT_QUERIES_ALLOWED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } const pageNo = req.pageNo const pageSize = req.pageSize - return await adminService.executeRawQuery(query, req.decodedToken.id, pageNo, pageSize) + return await adminService.executeRawQuery(trimmed, req.decodedToken.id, pageNo, pageSize)Additionally verify the service uses parameterized queries and safe pagination.
src/database/models/Feature.js (1)
35-58: Fix timestamp mapping for paranoid modeLines 35-46 manually define
created_at/updated_at/deleted_atbut model options lackunderscored: trueand explicit timestamp mapping. Sequelize will look forcreatedAt/updatedAt/deletedAt(camelCase) by default, causing column mismatches.Remove manual timestamp fields and configure Sequelize:
- created_at: { - type: DataTypes.DATE, - allowNull: false, - }, - updated_at: { - type: DataTypes.DATE, - allowNull: false, - }, - deleted_at: { - type: DataTypes.DATE, - allowNull: true, - }, display_order: { type: DataTypes.INTEGER, allowNull: false, }, }, { sequelize, modelName: 'Feature', tableName: 'features', freezeTableName: true, paranoid: true, + underscored: true, + timestamps: true, }Or set global Sequelize config in
src/configs/postgres.jswithdefine: { underscored: true, timestamps: true }.src/database/queries/notificationTemplate.js (3)
30-41: Fix Op.or misuse — use Op.in for single-column array matchingLines 37-40 incorrectly use
{ [Op.or]: [orgCode, defaultOrgCode] }fororganization_code. Sequelize'sOp.orexpects an array of condition objects, not values. UseOp.infor matching against multiple values.organization_code: orgCode && orgCode != defaultOrgCode ? { - [Op.or]: [orgCode, defaultOrgCode], + [Op.in]: [orgCode, defaultOrgCode], } : defaultOrgCode,
69-75: Same Op.or bug in email template pathLines 71-73 repeat the same
Op.ormisuse.organization_code: orgCode && orgCode != defaultOrgCode ? { - [Op.or]: [orgCode, defaultOrgCode], + [Op.in]: [orgCode, defaultOrgCode], } : defaultOrgCode,
196-207: Add tenant/org scoping to hardDeleteLine 196 deletes by
idonly, without tenant or organization validation. This risks cross-tenant deletes.-exports.hardDelete = async (id) => { +exports.hardDelete = async (id, tenantCode = null, organizationCode = null) => { try { + const whereClause = { id } + if (tenantCode) whereClause.tenant_code = tenantCode + if (organizationCode) whereClause.organization_code = organizationCode + return await NotificationTemplate.destroy({ - where: { - id: id, - }, + where: whereClause, force: true, }) } catch (error) { throw error } }Update callers to pass tenant/org context.
src/constants/common.js (2)
74-75: Breaking change: redisUserPrefix invalidates existing cacheChanging from
'user_'to'user_profile_cache_'will invalidate all cached user data. This breaks existing deployments without a migration strategy.Options:
- Revert the prefix change until a cache migration is ready
- Add migration script to rename/copy existing
user_*keys touser_profile_cache_*before deployment- Flush old keys with a deployment step (document carefully)
Verify this is intentional and coordinate rollout to minimize cache-miss impact on production.
106-109: Critical: Fix typo and align error code to PostgresThree issues:
- Typo:
ENGLISH_LANGUGE_CODEshould beENGLISH_LANGUAGE_CODE- Wrong DB error code:
'ER_DUP_ENTRY'is MySQL; this repo uses Postgres ('23505')- Unsafe env access: Lines 107-109 call
.toLowerCase()on potentially undefined env vars, causing startup crash- ENGLISH_LANGUGE_CODE: 'en', + ENGLISH_LANGUGE_CODE: 'en', // deprecated, kept for compatibility + ENGLISH_LANGUAGE_CODE: 'en', - ORG_CODE_HEADER: process.env.ORG_CODE_HEADER_NAME.toLowerCase(), - ORG_ID_HEADER: process.env.ORG_ID_HEADER_NAME.toLowerCase(), - TENANT_CODE_HEADER: process.env.TENANT_CODE_HEADER_NAME.toLowerCase(), + ORG_CODE_HEADER: (process.env.ORG_CODE_HEADER_NAME || 'organizationcode').toLowerCase(), + ORG_ID_HEADER: (process.env.ORG_ID_HEADER_NAME || 'organizationid').toLowerCase(), + TENANT_CODE_HEADER: (process.env.TENANT_CODE_HEADER_NAME || 'tenantcode').toLowerCase(), DELETE_METHOD: 'DELETE', SEQUELIZE_FOREIGN_KEY_CONSTRAINT_ERROR: 'SequelizeForeignKeyConstraintError', BULK_INVITATION_VALIDITY: '604800000', TYPE_UPLOAD: 'UPLOAD', TYPE_INVITE: 'INVITE', INVITED_STATUS: 'INVITED', SIGNEDUP_STATUS: 'SIGNEDUP', SEQUELIZE_UNIQUE_CONSTRAINT_ERROR: 'SequelizeUniqueConstraintError', - SEQUELIZE_UNIQUE_CONSTRAINT_ERROR_CODE: 'ER_DUP_ENTRY', + SEQUELIZE_UNIQUE_CONSTRAINT_ERROR_CODE: '23505', // Postgres unique violationVerify error handling in services checks
error.original.codeorerror.parent.code(noterror.code) for Sequelize errors.
🧹 Nitpick comments (26)
src/helpers/userInvite.js (1)
282-339: Consider extracting repeated meta field mapping logic.The pattern for mapping external entities to IDs is repeated for each meta field (block, state, school, cluster, district, professional_role, professional_subroles). This creates significant code duplication and increases maintenance burden.
Consider extracting this logic into a helper function:
+const mapExternalEntityToId = (value, entityType, externalEntityNameIdMap) => { + if (!value) return null + const key = `${value.replaceAll(/\s+/g, '').toLowerCase()}${entityType.replaceAll(/\s+/g, '').toLowerCase()}` + return externalEntityNameIdMap?.[key]?._id || null +} + +const mapExternalEntitiesToIds = (values, entityType, externalEntityNameIdMap) => { + if (!values) return [] + return values.split(',').map(val => { + const key = `${val?.replaceAll(/\s+/g, '').toLowerCase()}${entityType.replaceAll(/\s+/g, '').toLowerCase()}` + return externalEntityNameIdMap[key]?._id + }).filter(Boolean) +} row.meta = { - block: row?.block - ? externalEntityNameIdMap?.[ - `${row.block?.replaceAll(/\s+/g, '').toLowerCase()}${'block'.replaceAll(/\s+/g, '').toLowerCase()}` - ]?._id || null - : '', + block: mapExternalEntityToId(row?.block, 'block', externalEntityNameIdMap) || '', - state: row?.state - ? externalEntityNameIdMap?.[ - `${row.state?.replaceAll(/\s+/g, '').toLowerCase()}${'state'.replaceAll(/\s+/g, '').toLowerCase()}` - ]?._id || null - : '', + state: mapExternalEntityToId(row?.state, 'state', externalEntityNameIdMap) || '', // ... apply to other fields similarly - professional_subroles: row?.professional_subroles - ? row.professional_subroles - .split(',') - .map( - (prof_subRole) => - externalEntityNameIdMap[ - `${prof_subRole?.replaceAll(/\s+/g, '').toLowerCase()}${'professional_subroles'.replaceAll(/\s+/g, '').toLowerCase()}` - ]?._id - ) || [] - : [], + professional_subroles: mapExternalEntitiesToIds(row?.professional_subroles, 'professional_subroles', externalEntityNameIdMap), }src/locales/en.json (6)
145-146: Add missing period for consistency.These messages lack terminal periods, inconsistent with similar success messages throughout the file.
Apply this diff:
- "TENANT_DOMAINS_ALREADY_PRESENT": "All the domains are already added for the tenant", + "TENANT_DOMAINS_ALREADY_PRESENT": "All domains are already added for the tenant.", - "TENANT_DOMAINS_ADDED_SUCCESSFULLY": "Tenant domains added successfully", + "TENANT_DOMAINS_ADDED_SUCCESSFULLY": "Tenant domains added successfully.",
148-148: Fix mid-sentence capitalization."Matching" should be lowercase when appearing mid-sentence.
Apply this diff:
- "NO_MATCHING_TENANT_DOMAINS_TO_REMOVE": "No Matching domains found to remove.", + "NO_MATCHING_TENANT_DOMAINS_TO_REMOVE": "No matching domains found to remove.",
159-159: Fix capitalization of common nouns mid-sentence."Organization" and "Feature" should not be capitalized when appearing as common nouns mid-sentence. Also add missing periods for consistency.
Apply this diff:
- "ORGANIZATION_NOT_ACCESSIBLE": "You cannot access this Organization details.", + "ORGANIZATION_NOT_ACCESSIBLE": "You cannot access this organization's details.", - "ORG_FEATURE_FETCHED": "Organization Feature fetched successfully.", + "ORG_FEATURE_FETCHED": "Organization feature fetched successfully.", - "ORG_FEATURE_NOT_FOUND": "Organization Feature not found", + "ORG_FEATURE_NOT_FOUND": "Organization feature not found.", - "ORG_FEATURE_DELETED_SUCCESSFULLY": "Organization Feature deleted successfully", + "ORG_FEATURE_DELETED_SUCCESSFULLY": "Organization feature deleted successfully.", - "ORG_FEATURE_UPDATED_SUCCESSFULLY": "Organization Feature updated successfully", + "ORG_FEATURE_UPDATED_SUCCESSFULLY": "Organization feature updated successfully.", - "FAILED_TO_UPDATE_ORG_FEATURE": "Organization Feature update failed.", + "FAILED_TO_UPDATE_ORG_FEATURE": "Organization feature update failed.",Also applies to: 167-171
161-165: Add missing terminal periods for consistency.Multiple messages lack terminal periods, inconsistent with locale conventions used throughout the file.
Apply this diff:
- "USERNAME_TAKEN": "Username is already taken", + "USERNAME_TAKEN": "Username is already taken.", - "USERNAME_AVAILABLE": "Username is available", + "USERNAME_AVAILABLE": "Username is available.", - "ORGANIZATION_CODE_OR_TENANT_CODE_NOT_FOUND": "Organization code or tenant code not found", + "ORGANIZATION_CODE_OR_TENANT_CODE_NOT_FOUND": "Organization code or tenant code not found.", - "ORGANIZATION_FEATURE_EXISTS": "Organization feature already exists", + "ORGANIZATION_FEATURE_EXISTS": "Organization feature already exists.", - "ORG_FEATURE_CREATED_SUCCESSFULLY": "Organization feature created successfully", + "ORG_FEATURE_CREATED_SUCCESSFULLY": "Organization feature created successfully.", - "FEATURE_CREATED_SUCCESSFULLY": "Feature created successfully", + "FEATURE_CREATED_SUCCESSFULLY": "Feature created successfully.", - "FEATURE_NOT_FOUND": "Feature not found", + "FEATURE_NOT_FOUND": "Feature not found.",Also applies to: 173-173, 175-175
166-166: Standardize capitalization for common nouns and adjectives.Avoid capitalizing common nouns and adjectives mid-sentence. Titles like "Admin" and "ID" should follow convention.
Apply this diff:
- "ACCOUNT_NOT_FOUND": "Invalid Login ID", + "ACCOUNT_NOT_FOUND": "Invalid login credentials.", - "INVALID_INVITATION": "Invitation is Invalid or expired. Please contact your Admin.", + "INVALID_INVITATION": "Invitation is invalid or expired. Please contact your administrator.", - "USER_DATA_FETCHED": "User Data fetched successfully", + "USER_DATA_FETCHED": "User data fetched successfully.",Also applies to: 179-180
181-183: Improve consistency and clarity in error messages.These error messages have inconsistent capitalization, formatting, and awkward phrasing ("Updation" is not standard; use "update" instead). Also standardize variable formatting.
Apply this diff:
- "ORG_UNIQUE_CONSTRAIN_ERROR": "Organization Creation / Updation Failed. code / registration_code not unique.", + "ORG_UNIQUE_CONSTRAIN_ERROR": "Organization creation or update failed. Code or registration code is not unique.", - "REG_CODE_ERROR": "registration_code is not valid or unique.", + "REG_CODE_ERROR": "Registration code is not valid or unique.", - "INVALID_REG_CODE_ERROR": "registration_codes {{errorMessage}}. Invalid Code(s) : {{errorValues}}", + "INVALID_REG_CODE_ERROR": "Registration codes {{errorMessage}}. Invalid code(s): {{errorValues}}.",src/middlewares/authenticator.js (3)
48-54: Return 404 (or 400) instead of 406 for “not found” tenant-domain.406 Not Acceptable is semantically off here. Use 404 NOT_FOUND (preferred) or 400 BAD_REQUEST and a distinct responseCode.
Proposed:
-const notFoundResponse = (message) => - responses.failureResponse({ - message, - statusCode: httpStatusCode.not_acceptable, - responseCode: 'CLIENT_ERROR', - }) +const notFoundResponse = (message) => + responses.failureResponse({ + message, + statusCode: httpStatusCode.not_found, // if unavailable, use httpStatusCode.bad_request + responseCode: 'NOT_FOUND', + })
122-133: Ensure req.body exists before writing tenant_code (avoid TypeError on GET/no body).Some routes may not have a parsed body. Guard before assignment.
- req.body.tenant_code = tenantDomain.tenant_code + req.body = req.body || {} + req.body.tenant_code = tenantDomain.tenant_code
192-194: TTL renewal should parse env as number; ignore invalid/zero values.Passing a string may misbehave depending on redisSet; also skip non-positive.
- if (process.env.ALLOWED_IDLE_TIME != null) { - await utilsHelper.redisSet(sessionId, redisData, process.env.ALLOWED_IDLE_TIME) - } + const allowedIdle = Number(process.env.ALLOWED_IDLE_TIME) + if (!Number.isNaN(allowedIdle) && allowedIdle > 0) { + await utilsHelper.redisSet(sessionId, redisData, allowedIdle) + }src/database/migrations/20251022160602-add-tenant-admin-role.js (2)
29-40: Avoid magic numbers/enums drift for user_type and visibility.Hard-coding user_type = 1 and visibility = 'PUBLIC' risks mismatch with enum values across envs.
- Use shared constants or lookups (e.g., USER_TYPES.TENANT_ADMIN) or fetch from a reference table.
- Validate 'PUBLIC' matches the column enum/case.
Confirm allowed values for user_roles.user_type and user_roles.visibility.
95-103: Summary logs miscount assigned permissions.You log adminPermissions.length, which is unique permission types, not rows inserted across tenants (and ignores dedup). Log the actual insert size.
- console.log(`- Assigned permissions: ${adminPermissions.length}`) + console.log(`- Assigned permissions: ${typeof permissionInserts !== 'undefined' ? permissionInserts.length : 0}`)src/database/queries/users.js (3)
25-31: Avoid raw console.error in data layer; prefer structured log.Use your logger (with redaction) and include context (operation, tenant_code) instead of full error objects. Keep passing options — good.
- } catch (error) { - console.error(error) - throw error - } + } catch (error) { + logger.error({ op: 'User.create', err: error.message }, 'create failed') + throw error + }
166-220: Transform after toJSON; project user attributes to avoid PII.
- Convert instances to POJOs before DTO.
- Default-exclude sensitive fields unless caller overrides.
- let users = await database.User.findAll({ + let users = await database.User.findAll({ where: filter, ...options, @@ - if (!raw) { - users = UserTransformDTO.transform(users) - } + if (!raw) { + users = users.map(u => (u.toJSON ? u.toJSON() : u)) + users = UserTransformDTO.transform(users) + }Optionally enforce safe defaults:
- let users = await database.User.findAll({ + let users = await database.User.findAll({ where: filter, - ...options, + attributes: options.attributes || { exclude: ['password', 'reset_token', 'salt'] }, + ...options,As per coding guidelines.
552-584: Usereturningto fetch updated records in a single query.Currently, the function returns the users fetched before the update, not the actual updated state. Use
returning: truewith conditional destructuring to avoid this stale data issue, matching the established pattern throughout the codebase (permissions.js, user-role.js, feature.js, etc.).- const [rowsAffected] = await database.User.update(updateData, { - where: { id: { [Op.in]: userIds }, tenant_code: tenantCode }, - }) - return [rowsAffected, returnUpdatedUsers ? users : []] + const [rowsAffected, updated] = await database.User.update(updateData, { + where: { id: { [Op.in]: userIds }, tenant_code: tenantCode }, + returning: !!returnUpdatedUsers, + }) + return [rowsAffected, returnUpdatedUsers ? updated : []]Optionally wrap find + update in a transaction to prevent race conditions, following the pattern in
changeOrganization(same file).src/database/migrations/20251002164809-create-feature-role-mapping-table.js (2)
6-48: Add supporting indexes and defaults for write/read paths.
- Reads will commonly filter by tenant_code, organization_code, role_title, feature_code. Add indexes (or confirm a later migration adds them), e.g., (tenant_code), and a unique on (feature_code, role_title, organization_code, tenant_code) with deleted_at IS NULL.
- Consider default values for created_at/updated_at (Sequelize.fn('NOW')) to avoid mandatory explicit timestamps in every insert.
As per coding guidelines for queries/migrations.
50-55: Composite PK with auto-increment id — verify intent and ORM implications.Using (id, tenant_code) as the primary key is unusual. Ensure callers always include tenant_code in where clauses for updates/deletes; otherwise, ORM operations may misbehave. If cross-tenant id collisions aren’t required, a simpler single-column PK on id plus a separate unique (feature_code, role_title, organization_code, tenant_code) is often easier.
src/database/queries/featureRoleMapping.js (1)
78-87: Consider renaming delete() and accepting options.delete is valid but easily confused with the JS operator. Prefer destroy/remove and accept options (e.g., transaction, limit).
- static async delete(filter) { + static async delete(filter, options = {}) { try { const result = await FeatureRoleMapping.destroy({ - where: filter, + where: filter, + ...options, }) return result } catch (error) { throw error } }src/controllers/v1/organization.js (1)
188-209: Confirm authorization on related-org mutations.addRelatedOrg/removeRelatedOrg have no in-controller role checks. If not guarded at the router/middleware level, add admin/tenant-admin validation similar to update().
src/database/migrations/20251002164938-add-new-feature.js (1)
76-86: Down should also be transactional.Ensure deletes are in a transaction to mirror Up.
- async down(queryInterface) { - // Remove organization_feature records only for the SCP feature - await queryInterface.bulkDelete('organization_features', { - feature_code: 'scp', - }) - - // Remove SCP feature - await queryInterface.bulkDelete('features', { - code: 'scp', - }) - }, + async down(queryInterface) { + await queryInterface.sequelize.transaction(async (t) => { + await queryInterface.bulkDelete( + 'organization_features', + { feature_code: 'scp' }, + { transaction: t } + ) + await queryInterface.bulkDelete('features', { code: 'scp' }, { transaction: t }) + }) + },src/database/migrations/20251003155747-add-feature-role-mapping-constraints.js (1)
5-18: Wrap DDL in a transaction and pass transaction handlesAvoid partial state if one statement fails. Wrap up/down in a transaction and pass { transaction: t } to every addConstraint/query.
Illustrative change:
- async up(queryInterface, Sequelize) { - // Add foreign key constraint for feature_code - await queryInterface.addConstraint('feature_role_mapping', { + async up(queryInterface, Sequelize) { + await queryInterface.sequelize.transaction(async (t) => { + // Add foreign key constraint for feature_code + await queryInterface.addConstraint('feature_role_mapping', { fields: ['feature_code'], type: 'foreign key', name: 'fk_feature_role_mapping_feature_code', references: { table: 'features', field: 'code' }, onUpdate: 'CASCADE', onDelete: 'CASCADE', - }) + transaction: t, + }) // ... - await queryInterface.sequelize.query(`ALTER TABLE ...`, ) + await queryInterface.sequelize.query(`ALTER TABLE ...`, { transaction: t }) // ... - }) + }) }Also applies to: 19-31, 33-41, 53-59, 60-67, 70-79
src/database/models/featureRoleMapping.js (2)
37-45: Consider underscored timestamps to match created_at/updated_atThe migration inserts created_at/updated_at. Add underscored: true (and optionally timestamps: true) to ensure Sequelize maps correctly.
{ modelName: 'FeatureRoleMapping', tableName: 'feature_role_mapping', freezeTableName: true, - paranoid: false, + paranoid: false, + underscored: true, + timestamps: true,
73-82: Association with Organization lacks tenant scopingSequelize cannot model composite FKs; joining only on organization_code can cross-tenant accidentally. Ensure every include that uses 'organization' also filters by tenant_code in the ON/WHERE clause, or model a scoped association via manual join conditions.
src/database/migrations/20251002165109-map-roles-to-features.js (3)
69-82: Avoid N× duplicate queries for tenant roles — cache by tenantRoles are per tenant; querying inside the per-organization loop is O(orgs). Cache once per tenant_code.
Illustrative change:
- for (const org of uniqueOrgs) { + const rolesByTenant = new Map() + for (const org of uniqueOrgs) { const orgKey = `${org.organization_code}|${org.tenant_code}` const enabledFeatures = orgFeaturesMap.get(orgKey) || new Set() - // Fetch roles available for this tenant - const tenantRoles = await queryInterface.sequelize.query( - 'SELECT title FROM user_roles WHERE tenant_code = ? AND deleted_at IS NULL', - { replacements: [org.tenant_code], type: Sequelize.QueryTypes.SELECT } - ) - const tenantRoleTitles = tenantRoles.map((role) => role.title) + // Fetch roles available for this tenant (cached) + let tenantRoleTitles = rolesByTenant.get(org.tenant_code) + if (!tenantRoleTitles) { + const tenantRoles = await queryInterface.sequelize.query( + 'SELECT title FROM user_roles WHERE tenant_code = ? AND deleted_at IS NULL', + { replacements: [org.tenant_code], type: Sequelize.QueryTypes.SELECT } + ) + tenantRoleTitles = tenantRoles.map((r) => r.title) + rolesByTenant.set(org.tenant_code, tenantRoleTitles) + }Also applies to: 84-118
120-128: Wrap data load + bulkInsert in a transaction; consider batch insertsEnsure atomicity and reduce lock churn. If featureRoleMappingData is large, insert in batches to avoid memory/statement limits.
Transaction sketch:
- if (featureRoleMappingData.length > 0) { - await queryInterface.bulkInsert('feature_role_mapping', featureRoleMappingData) - } + if (featureRoleMappingData.length > 0) { + await queryInterface.sequelize.transaction(async (t) => { + await queryInterface.bulkInsert('feature_role_mapping', featureRoleMappingData, { transaction: t }) + }) + }Optional batching:
const chunk = 5000 for (let i = 0; i < featureRoleMappingData.length; i += chunk) { await queryInterface.bulkInsert('feature_role_mapping', featureRoleMappingData.slice(i, i + chunk), { transaction: t }) }Also applies to: 129-133
121-127: Cap skippedMappings to avoid unbounded memory/loggingKeep at most N samples for diagnostics.
- const skippedMappings = [] + const skippedMappings = [] + const SKIP_CAP = 1000 ... - if (!enabledFeatures.has(featureCode)) { - skippedMappings.push({ /* ... */ }) + if (!enabledFeatures.has(featureCode)) { + if (skippedMappings.length < SKIP_CAP) skippedMappings.push({ /* ... */ }) continue }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/configs/postgres.jsis excluded by!src/configs/**
📒 Files selected for processing (23)
src/constants/common.js(4 hunks)src/controllers/v1/admin.js(5 hunks)src/controllers/v1/notification.js(2 hunks)src/controllers/v1/org-admin.js(9 hunks)src/controllers/v1/organization-feature.js(1 hunks)src/controllers/v1/organization.js(4 hunks)src/database/migrations/20251002164809-create-feature-role-mapping-table.js(1 hunks)src/database/migrations/20251002164938-add-new-feature.js(1 hunks)src/database/migrations/20251002165109-map-roles-to-features.js(1 hunks)src/database/migrations/20251003155747-add-feature-role-mapping-constraints.js(1 hunks)src/database/migrations/20251022160602-add-tenant-admin-role.js(1 hunks)src/database/models/Feature.js(1 hunks)src/database/models/featureRoleMapping.js(1 hunks)src/database/queries/featureRoleMapping.js(1 hunks)src/database/queries/notificationTemplate.js(6 hunks)src/database/queries/organization-feature.js(1 hunks)src/database/queries/users.js(6 hunks)src/distributionColumns.sql(1 hunks)src/envVariables.js(9 hunks)src/helpers/userInvite.js(10 hunks)src/locales/en.json(5 hunks)src/middlewares/authenticator.js(6 hunks)src/package.json(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/notification.jssrc/controllers/v1/organization.jssrc/controllers/v1/admin.jssrc/controllers/v1/organization-feature.jssrc/controllers/v1/org-admin.js
src/database/queries/**
⚙️ CodeRabbit configuration file
Review database queries for performance. Check for N+1 problems and ensure indexes can be used.
Files:
src/database/queries/organization-feature.jssrc/database/queries/featureRoleMapping.jssrc/database/queries/notificationTemplate.jssrc/database/queries/users.js
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (2)
📚 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/middlewares/authenticator.jssrc/controllers/v1/organization.jssrc/controllers/v1/org-admin.js
📚 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/controllers/v1/admin.jssrc/controllers/v1/organization-feature.js
🧬 Code graph analysis (14)
src/database/models/featureRoleMapping.js (2)
src/database/queries/featureRoleMapping.js (1)
FeatureRoleMapping(8-8)src/database/queries/organization-feature.js (1)
FeatureRoleMapping(3-3)
src/database/queries/organization-feature.js (2)
src/services/organization-feature.js (7)
require(14-14)require(16-16)require(18-18)filter(265-268)filter(395-399)organizationFeature(402-402)roleTitles(298-298)src/database/models/featureRoleMapping.js (1)
FeatureRoleMapping(2-52)
src/database/queries/featureRoleMapping.js (3)
src/database/models/featureRoleMapping.js (1)
FeatureRoleMapping(2-52)src/database/queries/organization-feature.js (2)
FeatureRoleMapping(3-3)require(4-4)src/services/organization-feature.js (5)
require(14-14)require(16-16)require(18-18)filter(265-268)filter(395-399)
src/middlewares/authenticator.js (3)
src/services/user.js (2)
common(10-10)roles(329-329)src/services/account.js (11)
tenantDomainQueries(33-33)common(16-16)tenantDomain(69-69)tenantDomain(659-659)tenantDomain(1000-1000)tenantDomain(1133-1133)tenantDomain(1330-1330)role(190-191)role(1610-1615)role(1793-1793)roles(1871-1873)src/controllers/v1/public.js (3)
domain(7-7)domain(25-25)tenantCode(8-8)
src/controllers/v1/organization.js (3)
src/middlewares/authenticator.js (2)
common(11-11)tenantCode(220-220)src/controllers/v1/admin.js (3)
common(10-10)req(262-262)result(198-202)src/services/organization.js (4)
common(2-2)result(405-405)result(827-827)tenantCode(281-281)
src/controllers/v1/admin.js (3)
src/middlewares/authenticator.js (5)
user(312-312)utilsHelper(17-17)common(11-11)responses(16-16)httpStatusCode(10-10)src/services/admin.js (18)
user(58-58)user(114-114)user(317-320)user(455-455)user(566-566)adminService(32-32)common(15-15)responses(41-41)httpStatusCode(16-16)params(111-111)result(66-66)result(527-527)result(732-736)result(1012-1012)result(1024-1024)result(1036-1036)query(434-439)query(585-590)src/generics/materializedViews.js (2)
adminService(422-427)query(402-402)
src/database/migrations/20251002165109-map-roles-to-features.js (1)
src/services/organization-feature.js (2)
featureRoleMappingData(122-129)featureRoleMappingData(223-230)
src/controllers/v1/organization-feature.js (3)
src/middlewares/authenticator.js (10)
require(15-15)require(18-18)require(19-19)common(11-11)responses(16-16)httpStatusCode(10-10)utilsHelper(17-17)isAdmin(207-207)orgCode(219-219)tenantCode(220-220)src/services/organization-feature.js (10)
require(14-14)require(16-16)require(18-18)common(15-15)responses(12-12)httpStatusCode(8-8)utilsHelper(13-13)isAdmin(40-40)orgCode(42-42)tenantCode(43-43)src/helpers/responses.js (1)
error(28-28)
src/database/queries/notificationTemplate.js (4)
src/middlewares/authenticator.js (3)
orgCode(219-219)tenantCode(220-220)filter(38-38)src/services/notification.js (3)
defaultOrgCode(107-107)filter(55-58)filter(96-101)src/utils/notification.js (2)
templateData(14-18)templateData(58-62)src/database/models/notificationTemplates.js (1)
NotificationTemplate(3-57)
src/database/migrations/20251002164938-add-new-feature.js (1)
src/database/models/index.js (1)
process(6-6)
src/helpers/userInvite.js (3)
src/services/account.js (28)
entityTypeQueries(24-24)require(26-26)require(27-27)require(37-37)require(41-41)emailEncryption(29-29)userSessionsService(31-31)modelName(349-349)modelName(791-791)modelName(1487-1487)utils(25-25)userQueries(17-17)validationData(356-364)validationData(796-803)validationData(1489-1496)_(11-11)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)eventBody(604-623)eventBody(1754-1769)src/services/user.js (20)
entityTypeQueries(15-15)require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)emailEncryption(21-21)utils(12-12)userQueries(11-11)validationData(71-71)validationData(353-360)validationData(458-465)_(18-18)user(48-51)user(319-319)user(440-440)user(490-490)user(525-525)modifiedKeys(129-133)eventBody(191-201)src/utils/usernameGenerator.js (1)
module(32-32)
src/database/queries/users.js (4)
src/services/admin.js (16)
UserTransformDTO(45-45)require(11-11)require(21-21)require(29-29)require(36-36)require(38-38)require(40-40)require(47-47)user(58-58)user(114-114)user(317-320)user(455-455)user(566-566)userIds(828-828)userIds(893-893)rowsAffected(910-913)src/services/account.js (23)
UserTransformDTO(38-38)require(26-26)require(27-27)require(37-37)require(41-41)users(1577-1577)users(1617-1624)users(1888-1898)users(2062-2062)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)userIds(1554-1554)emailIds(1876-1876)emailIds(2051-2051)roleIds(1875-1875)src/services/org-admin.js (10)
require(22-22)require(23-23)require(24-24)require(31-31)filter(540-544)user(371-374)roleIds(605-605)rowsAffected(342-349)rowsAffected(445-457)rowsAffected(465-477)src/services/user.js (6)
filter(63-70)filter(234-237)filter(269-269)filter(300-300)filter(427-427)filter(536-542)
src/constants/common.js (1)
src/database/models/index.js (1)
process(6-6)
src/database/models/Feature.js (1)
src/database/queries/feature.js (1)
Feature(1-1)
🪛 Biome (2.1.2)
src/constants/common.js
[error] 44-44: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
| TENANT_ADMIN_ROLE: 'tenant_admin', | ||
| ORG_ADMIN_ROLE: 'org_admin', | ||
| TENANT_ADMIN_ROLE: 'tenant_admin', |
There was a problem hiding this comment.
Remove duplicate TENANT_ADMIN_ROLE key
Lines 44 and 46 define TENANT_ADMIN_ROLE twice. JavaScript silently keeps only the last definition, making line 44 dead code.
ADMIN_ROLE: 'admin',
- TENANT_ADMIN_ROLE: 'tenant_admin',
ORG_ADMIN_ROLE: 'org_admin',
TENANT_ADMIN_ROLE: 'tenant_admin',Based on learnings.
📝 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.
| TENANT_ADMIN_ROLE: 'tenant_admin', | |
| ORG_ADMIN_ROLE: 'org_admin', | |
| TENANT_ADMIN_ROLE: 'tenant_admin', | |
| ADMIN_ROLE: 'admin', | |
| ORG_ADMIN_ROLE: 'org_admin', | |
| TENANT_ADMIN_ROLE: 'tenant_admin', |
🧰 Tools
🪛 Biome (2.1.2)
[error] 44-44: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
In src/constants/common.js around lines 44 to 46, there is a duplicate
TENANT_ADMIN_ROLE key (defined on lines 44 and 46); remove the duplicate entry
(delete the second TENANT_ADMIN_ROLE line) so each role key is unique, and if
that duplicate was meant to be a different role name, replace it with the
correct identifier instead, preserving proper comma placement and formatting.
| const result = await orgService.read( | ||
| req.query.organisation_id ? req.query.organisation_id : '', | ||
| req.query.organisation_code ? req.query.organisation_code : '' | ||
| req.query.organisation_code ? req.query.organisation_code : '', | ||
| req.query.tenant_code ? req.query.tenant_code : '' | ||
| ) | ||
| return result | ||
| } catch (error) { | ||
| return error |
There was a problem hiding this comment.
Do not trust tenant/org from query; source from token to prevent cross-tenant reads.
read() accepts organisation_code and tenant_code from req.query, enabling callers to probe other tenants. Use token-derived values instead.
Based on learnings
- const result = await orgService.read(
- req.query.organisation_id ? req.query.organisation_id : '',
- req.query.organisation_code ? req.query.organisation_code : '',
- req.query.tenant_code ? req.query.tenant_code : ''
- )
+ const result = await orgService.read(
+ req.query.organisation_id ? req.query.organisation_id : '',
+ req.decodedToken.organization_code,
+ req.decodedToken.tenant_code
+ )🤖 Prompt for AI Agents
In src/controllers/v1/organization.js around lines 154 to 161, the controller
currently passes organisation_id/organisation_code/tenant_code from req.query
into orgService.read which allows cross-tenant probing; instead derive
organisation and tenant identity from the authenticated token (e.g. req.user or
req.auth payload) and pass those values to orgService.read. Replace any use of
req.query.organisation_id/organisation_code/tenant_code with token-derived
fields (e.g. req.user.organisationId, req.user.organisationCode,
req.user.tenantCode), validate they exist and return a 401/403 if not, and
remove or ignore query-supplied tenant/org values so callers cannot request data
for other tenants. Ensure error handling still returns/throws the service error
as before.
| async up(queryInterface, Sequelize) { | ||
| // Check if SCP feature already exists | ||
| const scpFeature = await queryInterface.sequelize.query("SELECT code FROM features WHERE code = 'scp'", { | ||
| type: Sequelize.QueryTypes.SELECT, | ||
| }) | ||
|
|
||
| // Insert SCP feature if it doesn't exist | ||
| if (scpFeature.length === 0) { | ||
| await queryInterface.bulkInsert('features', [ | ||
| { | ||
| code: 'scp', | ||
| label: 'Self Creation Portal', | ||
| display_order: 9, | ||
| description: 'SCP capability', | ||
| created_at: new Date(), | ||
| updated_at: new Date(), | ||
| }, | ||
| ]) | ||
| } | ||
|
|
||
| // Get all organizations | ||
| const tenants = await queryInterface.sequelize.query('SELECT code FROM tenants WHERE deleted_at IS NULL', { | ||
| type: Sequelize.QueryTypes.SELECT, | ||
| }) | ||
|
|
||
| // Get default orgs for each organization | ||
| const organizationFeatureData = [] | ||
| for (const tenant of tenants) { | ||
| const orgExist = await queryInterface.sequelize.query( | ||
| 'SELECT code, tenant_code FROM organizations WHERE deleted_at IS NULL AND tenant_code = :tenantCode AND code = :orgCode', | ||
| { | ||
| replacements: { | ||
| tenantCode: tenant.code, | ||
| orgCode: process.env.DEFAULT_ORGANISATION_CODE || 'default_code', | ||
| }, | ||
| type: Sequelize.QueryTypes.SELECT, | ||
| } | ||
| ) | ||
|
|
||
| if (orgExist.length > 0) { | ||
| const orgFeatureExist = await queryInterface.sequelize.query( | ||
| 'SELECT organization_code FROM organization_features WHERE tenant_code = :tenantCode AND organization_code = :orgCode AND feature_code = :featureCode', | ||
| { | ||
| replacements: { | ||
| tenantCode: tenant.code, | ||
| orgCode: process.env.DEFAULT_ORGANISATION_CODE || 'default_code', | ||
| featureCode: 'scp', | ||
| }, | ||
| type: Sequelize.QueryTypes.SELECT, | ||
| } | ||
| ) | ||
| if (orgFeatureExist.length === 0) { | ||
| organizationFeatureData.push({ | ||
| organization_code: process.env.DEFAULT_ORGANISATION_CODE || 'default_code', | ||
| tenant_code: tenant.code, | ||
| feature_code: 'scp', | ||
| feature_name: 'SCP', | ||
| enabled: true, | ||
| display_order: 9, | ||
| created_at: new Date(), | ||
| updated_at: new Date(), | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (organizationFeatureData.length > 0) { | ||
| await queryInterface.bulkInsert('organization_features', organizationFeatureData) | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wrap the migration in a transaction for atomicity.
Up performs several reads/inserts; wrap them in a single transaction and pass the transaction to each query. Also hoist defaultOrgCode to a const.
- async up(queryInterface, Sequelize) {
- // Check if SCP feature already exists
+ async up(queryInterface, Sequelize) {
+ const defaultOrgCode = process.env.DEFAULT_ORGANISATION_CODE || 'default_code'
+ await queryInterface.sequelize.transaction(async (t) => {
+ // Check if SCP feature already exists
- const scpFeature = await queryInterface.sequelize.query("SELECT code FROM features WHERE code = 'scp'", {
- type: Sequelize.QueryTypes.SELECT,
- })
+ const scpFeature = await queryInterface.sequelize.query(
+ "SELECT code FROM features WHERE code = 'scp'",
+ { type: Sequelize.QueryTypes.SELECT, transaction: t }
+ )
@@
- await queryInterface.bulkInsert('features', [
+ await queryInterface.bulkInsert('features', [
{
code: 'scp',
label: 'Self Creation Portal',
display_order: 9,
description: 'SCP capability',
created_at: new Date(),
updated_at: new Date(),
},
- ])
+ ], { transaction: t })
@@
- const tenants = await queryInterface.sequelize.query('SELECT code FROM tenants WHERE deleted_at IS NULL', {
- type: Sequelize.QueryTypes.SELECT,
- })
+ const tenants = await queryInterface.sequelize.query(
+ 'SELECT code FROM tenants WHERE deleted_at IS NULL',
+ { type: Sequelize.QueryTypes.SELECT, transaction: t }
+ )
@@
- const orgExist = await queryInterface.sequelize.query(
+ const orgExist = await queryInterface.sequelize.query(
'SELECT code, tenant_code FROM organizations WHERE deleted_at IS NULL AND tenant_code = :tenantCode AND code = :orgCode',
{
replacements: {
tenantCode: tenant.code,
- orgCode: process.env.DEFAULT_ORGANISATION_CODE || 'default_code',
+ orgCode: defaultOrgCode,
},
- type: Sequelize.QueryTypes.SELECT,
+ type: Sequelize.QueryTypes.SELECT,
+ transaction: t,
}
)
@@
- const orgFeatureExist = await queryInterface.sequelize.query(
+ const orgFeatureExist = await queryInterface.sequelize.query(
'SELECT organization_code FROM organization_features WHERE tenant_code = :tenantCode AND organization_code = :orgCode AND feature_code = :featureCode',
{
replacements: {
tenantCode: tenant.code,
- orgCode: process.env.DEFAULT_ORGANISATION_CODE || 'default_code',
+ orgCode: defaultOrgCode,
featureCode: 'scp',
},
- type: Sequelize.QueryTypes.SELECT,
+ type: Sequelize.QueryTypes.SELECT,
+ transaction: t,
}
)
@@
- organization_code: process.env.DEFAULT_ORGANISATION_CODE || 'default_code',
+ organization_code: defaultOrgCode,
tenant_code: tenant.code,
feature_code: 'scp',
feature_name: 'SCP',
enabled: true,
display_order: 9,
created_at: new Date(),
updated_at: new Date(),
})
}
}
- }
+ }
@@
- if (organizationFeatureData.length > 0) {
- await queryInterface.bulkInsert('organization_features', organizationFeatureData)
- }
+ if (organizationFeatureData.length > 0) {
+ await queryInterface.bulkInsert('organization_features', organizationFeatureData, { transaction: t })
+ }
+ })
},🤖 Prompt for AI Agents
In src/database/migrations/20251002164938-add-new-feature.js around lines 5 to
74, the migration performs multiple reads and inserts without a transaction and
repeats the default org code; wrap the entire up logic in a single transaction
(using queryInterface.sequelize.transaction) so all selects/inserts are atomic,
hoist process.env.DEFAULT_ORGANISATION_CODE || 'default_code' to a const at the
top of the up function, and ensure every queryInterface.sequelize.query and
queryInterface.bulkInsert call receives the transaction option (e.g., {
transaction }) so they all run within that transaction; commit/rollback is
handled by awaiting the transaction callback.
| // Add composite foreign key for organization_code (organization_code, tenant_code) -> organizations (code, tenant_code) | ||
| await queryInterface.sequelize.query(` | ||
| ALTER TABLE feature_role_mapping | ||
| ADD CONSTRAINT fk_feature_role_mapping_organization_code | ||
| FOREIGN KEY (organization_code, tenant_code) | ||
| REFERENCES organizations (code, tenant_code) | ||
| ON UPDATE CASCADE | ||
| ON DELETE CASCADE; | ||
| `) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify referenced unique/PK constraints for composite FKs
PostgreSQL requires the referenced column set to be UNIQUE or PRIMARY KEY, in the same order.
- fk_feature_role_mapping_organization_code → organizations (code, tenant_code)
- fk_org_feature_role_mapping_organization_code → organization_features (feature_code, tenant_code, organization_code)
Please confirm those unique/PK constraints exist exactly in that order, or the ALTER TABLE will fail at deploy time.
Also applies to: 60-67
🏁 Script executed:
#!/bin/bash
# Scan migrations to confirm composite unique/PK definitions
rg -nP "(create-table|addConstraint|addIndex).*organizations|organization_features" -C3 src/database/migrationsLength of output: 36947
Fix column order in second FK to match organization_features PK
The second foreign key references organization_features (feature_code, tenant_code, organization_code), but the table's primary key is defined as (tenant_code, organization_code, feature_code) (from migration 20250505092012-create-org-features-table.js). PostgreSQL requires the referenced column order to match the PK/UNIQUE constraint exactly.
At lines 60-67, reorder the referenced columns to:
REFERENCES organization_features (tenant_code, organization_code, feature_code)And update the corresponding FK columns to match:
FOREIGN KEY (tenant_code, organization_code, feature_code)The first FK (lines 32-41) is correct—it properly references organizations (code, tenant_code), which matches the UNIQUE constraint.
🤖 Prompt for AI Agents
In
src/database/migrations/20251003155747-add-feature-role-mapping-constraints.js
around lines 32-41 (and the second FK at ~60-67), the second foreign key
currently references organization_features with columns in the wrong order;
update the FK definition so the child columns are (tenant_code,
organization_code, feature_code) and the REFERENCES clause matches the primary
key order as REFERENCES organization_features (tenant_code, organization_code,
feature_code), keeping the same ON UPDATE/ON DELETE behavior.
| const [orgsByTenant] = await queryInterface.sequelize.query( | ||
| `SELECT DISTINCT tenant_code, organization_id | ||
| FROM user_roles | ||
| WHERE deleted_at IS NULL | ||
| AND tenant_code IN ( | ||
| SELECT code FROM tenants WHERE deleted_at IS NULL | ||
| ) | ||
| ORDER BY tenant_code, organization_id`, | ||
| { transaction } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against null organization_id in seed source.
If any user_roles rows have organization_id NULL, Step 2 will create invalid roles. Filter them out.
Apply:
- WHERE deleted_at IS NULL
+ WHERE deleted_at IS NULL
+ AND organization_id IS NOT NULLPlease confirm organization_id is NOT NULL at schema level; if yes, this guard is still harmless and future‑proof.
---
🏁 Script executed:
```shell
#!/bin/bash
# Retrieve the complete migration file
cat src/database/migrations/20251022160602-add-tenant-admin-role.js
Length of output: 4561
🏁 Script executed:
#!/bin/bash
# Find model definitions to understand schema constraints and paranoid settings
fd -e js -path '*/models/*' -type f | head -20Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for sequelize config to determine DB engine
fd -e js -e json -path '*config*' -type f | grep -E '(sequelize|database)" | head -20Length of output: 68
🏁 Script executed:
#!/bin/bash
# Search for user_roles and role_permission_mapping model definitions
rg -n "user_roles|role_permission_mapping" --type=js -A 5 -B 1 | head -80Length of output: 5630
🏁 Script executed:
#!/bin/bash
# Check for schema migrations or migrations that define organization_id nullability
rg -n "organization_id" --type=js -C 2 src/database/migrations/ | head -60Length of output: 5764
🏁 Script executed:
#!/bin/bash
# Verify if tenants, roles, user_roles tables use paranoid (soft deletes)
rg -n "paranoid.*true|timestamps.*true" --type=js -C 1 | head -40Length of output: 1844
🏁 Script executed:
#!/bin/bash
# Find and display role_permission_mapping model definition
rg -n "role_permission_mapping" --type=js -A 20 src/database/models/ 2>/dev/null | head -80Length of output: 519
🏁 Script executed:
#!/bin/bash
# Check if role_permission_mapping migration exists and examine structure
rg -n "createTable.*role_permission_mapping|addColumn.*role_permission_mapping" --type=js -A 15 src/database/migrations/ | head -100Length of output: 1445
🏁 Script executed:
#!/bin/bash
# Determine DB engine from sequelize config
cat src/configs/postgres.js | head -50Length of output: 1407
🏁 Script executed:
#!/bin/bash
# Search for role_permission_mapping references to understand if tenant-scoped
rg -n "role_permission_mapping" --type=js -B 2 -A 2 | grep -E "(tenant_code|deleted_at)" | head -30Length of output: 110
🏁 Script executed:
#!/bin/bash
# Verify: does the down migration actually use hard DELETE or if tables are paranoid, should be soft delete?
rg -n "DELETE FROM|UPDATE.*deleted_at" --type=js src/database/migrations/ | grep -A 2 -B 2 "tenant_admin" | head -20Length of output: 775
Fix organization_id nullability guard and PostgreSQL compatibility issues.
Step 1 retrieves organization_id values without NULL filtering; a null value would create roles with null organization_id, violating data integrity. Step 2 and Step 4 use ignoreDuplicates, which is MySQL-specific; PostgreSQL requires ON CONFLICT syntax. The down migration uses hard DELETE on user_roles, which conflicts with its paranoid (soft-delete) configuration—should use UPDATE with deleted_at for consistency.
Apply:
- WHERE deleted_at IS NULL
+ WHERE deleted_at IS NULL
+ AND organization_id IS NOT NULLAdditionally:
- Replace
ignoreDuplicates: truewith PostgreSQL-compatible ON CONFLICT DO NOTHING (or use raw queries with UPSERT). - Use soft-delete semantics in down migration:
UPDATE user_roles SET deleted_at = NOW() WHERE title = 'tenant_admin'instead of hard DELETE.
📝 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.
| const [orgsByTenant] = await queryInterface.sequelize.query( | |
| `SELECT DISTINCT tenant_code, organization_id | |
| FROM user_roles | |
| WHERE deleted_at IS NULL | |
| AND tenant_code IN ( | |
| SELECT code FROM tenants WHERE deleted_at IS NULL | |
| ) | |
| ORDER BY tenant_code, organization_id`, | |
| { transaction } | |
| ) | |
| const [orgsByTenant] = await queryInterface.sequelize.query( | |
| `SELECT DISTINCT tenant_code, organization_id | |
| FROM user_roles | |
| WHERE deleted_at IS NULL | |
| AND organization_id IS NOT NULL | |
| AND tenant_code IN ( | |
| SELECT code FROM tenants WHERE deleted_at IS NULL | |
| ) | |
| ORDER BY tenant_code, organization_id`, | |
| { transaction } | |
| ) |
🤖 Prompt for AI Agents
In src/database/migrations/20251022160602-add-tenant-admin-role.js around lines
9–18, fix three issues: (1) add a WHERE clause to the SELECT to exclude rows
with organization_id IS NULL so you don’t create roles with null
organization_id; (2) replace Sequelize's MySQL-only ignoreDuplicates usage in
Step 2/4 with a PostgreSQL-safe upsert (use INSERT ... ON CONFLICT (...) DO
NOTHING or perform a raw query) and target the appropriate unique constraint
(e.g. tenant_code, organization_id, title) as the conflict key; (3) change the
down migration to use soft-delete semantics instead of hard DELETE — run an
UPDATE user_roles SET deleted_at = NOW() WHERE title = 'tenant_admin' (and any
other matching filters like tenant_code/organization_id) so it respects paranoid
soft deletes.
| exports.searchUsersWithOrganization = async ({ | ||
| roleIds, | ||
| organization_id, | ||
| page, | ||
| limit, | ||
| search, | ||
| userIds, | ||
| emailIds, | ||
| excluded_user_ids, | ||
| tenantCode, | ||
| }) => { | ||
| try { | ||
| const offset = (page - 1) * limit | ||
|
|
||
| // Base filter for user | ||
| const userWhere = {} | ||
|
|
||
| // Filter by userIds / exclude | ||
| if (userIds && Array.isArray(userIds)) { | ||
| userWhere.id = { [Op.in]: userIds } | ||
| if (excluded_user_ids && excluded_user_ids.length > 0) { | ||
| userWhere.id = { | ||
| [Op.and]: [{ [Op.in]: userIds }, { [Op.notIn]: excluded_user_ids }], | ||
| } | ||
| } | ||
| } else if (excluded_user_ids && excluded_user_ids.length > 0) { | ||
| userWhere.id = { [Op.notIn]: excluded_user_ids } | ||
| } | ||
|
|
||
| // Filter by search text or email | ||
| if (emailIds && emailIds.length > 0) { | ||
| userWhere.email = { [Op.in]: emailIds } | ||
| } else if (search) { | ||
| userWhere.name = { [Op.iLike]: `%${search}%` } | ||
| } | ||
|
|
||
| const users = await database.User.findAndCountAll({ | ||
| where: userWhere, | ||
| limit: limit, | ||
| offset: offset, | ||
| order: [['name', 'ASC']], | ||
| include: [ | ||
| { | ||
| model: database.UserOrganization, | ||
| as: 'user_organizations', | ||
| required: true, | ||
| where: { | ||
| tenant_code: tenantCode, | ||
| ...(organization_id && { organization_id }), | ||
| }, | ||
| include: [ | ||
| { | ||
| model: database.Organization, | ||
| as: 'organization', | ||
| required: false, | ||
| where: { | ||
| status: 'ACTIVE', | ||
| tenant_code: tenantCode, | ||
| }, | ||
| attributes: ['id', 'name', 'code'], | ||
| }, | ||
| { | ||
| model: database.UserOrganizationRole, | ||
| as: 'roles', | ||
| required: roleIds && roleIds.length > 0, | ||
| where: { | ||
| tenant_code: tenantCode, | ||
| ...(roleIds && roleIds.length > 0 && { role_id: { [Op.in]: roleIds } }), | ||
| }, | ||
| attributes: ['role_id'], | ||
| include: [ | ||
| { | ||
| model: database.UserRole, | ||
| as: 'role', | ||
| required: false, | ||
| where: { tenant_code: tenantCode }, | ||
| attributes: ['id', 'title', 'label'], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| raw: false, | ||
| distinct: true, // Needed for correct count when using include | ||
| }) | ||
|
|
||
| return { | ||
| count: users.count, | ||
| data: users.rows, | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
Restrict attributes and return transformed POJOs; keep count accurate.
- Project safe fields; avoid leaking password/PII.
- Convert to POJOs and apply DTO transform for consistency with other reads.
- Optionally coerce limit/offset to int.
- const users = await database.User.findAndCountAll({
- where: userWhere,
- limit: limit,
- offset: offset,
+ const users = await database.User.findAndCountAll({
+ where: userWhere,
+ attributes: ['id','name','about','image'], // add fields as needed, exclude sensitive
+ limit: parseInt(limit, 10),
+ offset: parseInt(offset, 10),
@@
- raw: false,
- distinct: true, // Needed for correct count when using include
+ raw: false,
+ distinct: true,
})
- return { count: users.count, data: users.rows }
+ const rows = users.rows.map(u => (u.toJSON ? u.toJSON() : u))
+ return { count: users.count, data: UserTransformDTO.transform(rows) }As per coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/database/queries/users.js around lines 394 to 485, the query returns full
Sequelize instances (risking PII exposure) and doesn't coerce pagination inputs;
update the findAndCountAll call to request only safe user attributes (explicitly
list id, name, email, createdAt, etc. — exclude password and sensitive fields),
ensure limit and page/offset are coerced to integers before use, keep distinct:
true for correct counting, then convert the returned rows to plain POJOs (e.g.,
map rows to row.get({ plain: true })) and apply the existing DTO/transform
function used by other reads before returning. Ensure the returned shape remains
{ count, data } with transformed objects.
| if (userData?.email) { | ||
| notificationUtils.sendEmailNotification({ | ||
| emailId: userData.email, | ||
| templateCode: process.env.BULK_CREATE_TEMPLATE_CODE, | ||
| variables: { | ||
| name: user.name, | ||
| orgName: userData.org_name, | ||
| appName: tenantDetails.name, | ||
| roles: roleToString || '', | ||
| portalURL: tenantDomains.domain, | ||
| username: inviteeData.username, | ||
| }, | ||
| tenantCode: tenantDetails.code, | ||
| organization_code: user.organization_code, | ||
| }) | ||
| } | ||
|
|
||
| // Send SMS notification with OTP if phone is provided | ||
| if (userData?.phone) { | ||
| notificationUtils.sendSMSNotification({ | ||
| phoneNumber: userData.phone, | ||
| templateCode: process.env.BULK_CREATE_TEMPLATE_CODE, | ||
| variables: { | ||
| name: user.name, | ||
| orgName: userData.org_name, | ||
| appName: tenantDetails.name, | ||
| roles: roleToString || '', | ||
| portalURL: tenantDomains.domain, | ||
| username: inviteeData.username, | ||
| }, | ||
| tenantCode: tenantDetails.code, | ||
| organization_code: user.organization_code, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Incorrect variable reference in notification.
At lines 1184-1217, the notification uses userData.email and userData.phone, but userData is a locally scoped object defined at line 1177 that only contains name, email, roles, and org_name. The userData.phone reference at line 1202 will be undefined since phone is not part of this object.
Apply this fix to use the correct variable:
// Send SMS notification with OTP if phone is provided
-if (userData?.phone) {
+if (raw_phone) {
notificationUtils.sendSMSNotification({
- phoneNumber: userData.phone,
+ phoneNumber: raw_phone,
templateCode: process.env.BULK_CREATE_TEMPLATE_CODE,
variables: {
name: user.name,
orgName: userData.org_name,
appName: tenantDetails.name,
roles: roleToString || '',
portalURL: tenantDomains.domain,
username: inviteeData.username,
},
tenantCode: tenantDetails.code,
organization_code: user.organization_code,
})
}🤖 Prompt for AI Agents
In src/helpers/userInvite.js around lines 1184 to 1217, the SMS notification is
checking userData.phone which doesn't exist on the locally scoped userData
(defined at ~1177); update the SMS branch to use the correct phone field
(inviteeData.phone), keep the existing email check as-is, and ensure the
null/optional check uses inviteeData?.phone so the SMS block only runs when a
phone is present.
| let token | ||
| if (process.env.IS_AUTH_TOKEN_BEARER === 'true') { | ||
| const [authType, extractedToken] = authHeader.split(' ') | ||
| if (authType.toLowerCase() !== 'bearer') throw unAuthorizedResponse | ||
| token = extractedToken.trim() | ||
| } else token = authHeader.trim() | ||
|
|
There was a problem hiding this comment.
Guard Bearer parsing to avoid .trim() on undefined (malformed header).
If header is “Bearer” with no token or has extra spaces, extractedToken is undefined → crash.
- if (process.env.IS_AUTH_TOKEN_BEARER === 'true') {
- const [authType, extractedToken] = authHeader.split(' ')
+ if (process.env.IS_AUTH_TOKEN_BEARER === 'true') {
+ const [authType, extractedToken] = authHeader.trim().split(/\s+/)
if (authType.toLowerCase() !== 'bearer') throw unAuthorizedResponse
- token = extractedToken.trim()
+ if (!extractedToken) throw unAuthorizedResponse
+ token = extractedToken.trim()
} else token = authHeader.trim()🤖 Prompt for AI Agents
In src/middlewares/authenticator.js around lines 151 to 157, the bearer parsing
assumes extractedToken exists and calls .trim(), which will throw if the header
is malformed (e.g., "Bearer" with no token or extra spaces); update the logic to
guard against missing parts by validating authHeader.split(' ') yields at least
two elements (or check extractedToken !== undefined) before calling .trim(), and
if the token is missing or empty throw unAuthorizedResponse; ensure you also
normalize authType and handle extra spaces safely so malformed headers do not
crash.
| "elevate-encryption": "^1.0.1", | ||
| "elevate-logger": "^3.1.0", | ||
| "elevate-node-cache": "^1.0.6", | ||
| "elevate-services-health-check": "^0.0.6", |
There was a problem hiding this comment.
🧩 Analysis chain
Health-check package still missing required env var definitions (unresolved from prior review).
The past review flagged that elevate-services-health-check references these environment variables that are missing from src/envVariables.js and .env.sample:
SERVICE_NAMEINTERFACE_SERVICE_HOSTDEV_DATABASE_URL(note:.env.sampleusesDATABASE_URL)KAFKA_HEALTH_CHECK_TOPIC
Ensure these env vars are properly defined in both src/envVariables.js and .env.sample before this is merged, or the health-check will fail at startup.
🏁 Script executed:
#!/bin/bash
# Verify env vars referenced by health-check are defined
echo "=== Env vars referenced in health-check/health.config.js ==="
rg -n "process\.env\.(SERVICE_NAME|INTERFACE_SERVICE_HOST|DEV_DATABASE_URL|KAFKA_HEALTH_CHECK_TOPIC)" src/health-checks/health.config.js 2>/dev/null || echo "health.config.js not found or no matches"
echo -e "\n=== Check if these are exported in envVariables.js ==="
rg -n "SERVICE_NAME|INTERFACE_SERVICE_HOST|DEV_DATABASE_URL|KAFKA_HEALTH_CHECK_TOPIC" src/envVariables.js || echo "Not found in envVariables.js"
echo -e "\n=== Check if these are documented in .env.sample ==="
rg -n "SERVICE_NAME|INTERFACE_SERVICE_HOST|DEV_DATABASE_URL|KAFKA_HEALTH_CHECK_TOPIC" src/.env.sample || echo "Not found in .env.sample"Length of output: 674
I need to examine the structure of envVariables.js more carefully to understand how variables are exported, and check for alternative naming patterns.
I need to inspect the actual structure and content of envVariables.js to verify if these variables are defined or exported.
Now let me inspect the actual files in the codebase to verify the exact state of variable definitions:
Based on the script execution results, the review comment has been verified as accurate. The shell script output definitively shows:
-
health.config.js references these environment variables (lines 9, 15, 24, 29, 45):
SERVICE_NAME✓ Referenced (3 times)INTERFACE_SERVICE_HOST✓ Referenced (2 times)DEV_DATABASE_URL✓ Referenced (1 time)KAFKA_HEALTH_CHECK_TOPIC✓ Referenced (1 time)
-
envVariables.js status:
- Only
KAFKA_HEALTH_CHECK_TOPICis defined (line 455-456) ✓ - Missing:
SERVICE_NAME,INTERFACE_SERVICE_HOST,DEV_DATABASE_URL✗
- Only
-
.env.sample status:
- Only
KAFKA_HEALTH_CHECK_TOPICis defined (line 183) ✓ - Missing:
SERVICE_NAME,INTERFACE_SERVICE_HOST,DEV_DATABASE_URL✗
- Only
Health-check package still missing required env var definitions (unresolved from prior review).
The elevate-services-health-check package references environment variables in src/health-checks/health.config.js that are not properly defined in either src/envVariables.js or .env.sample:
SERVICE_NAME— missing from both filesINTERFACE_SERVICE_HOST— missing from both filesDEV_DATABASE_URL— missing from both files
Only KAFKA_HEALTH_CHECK_TOPIC is currently defined. Add the three missing variables to both files before merging, or the health check will fail at startup.
🤖 Prompt for AI Agents
In src/package.json around lines 48-48, the health-check package references env
vars that are missing from src/envVariables.js and .env.sample; add
SERVICE_NAME, INTERFACE_SERVICE_HOST, and DEV_DATABASE_URL to both
src/envVariables.js and .env.sample using the exact names referenced in
src/health-checks/health.config.js (uppercase keys), export them in
envVariables.js in the same pattern used for KAFKA_HEALTH_CHECK_TOPIC (so the
health-check can import them), and provide sensible placeholder/default values
or comments in .env.sample so developers know what to set before deployment.
refactor(organization): optimize feature access logic for role mappings
release: v3.3.24 – develop → staging
feat(admin): decrypt user email and phone before user event
release: v3.3.24 – develop → staging
Summary by CodeRabbit
New Features
Enhancements
Chores