Issue #000 fix: creating usres in bulk - TypeError: Cannot read prope… #868
Conversation
…rties of undefined (reading 'outputFilePath')
WalkthroughAdded defensive null-checking in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TenantSvc as Tenant Lookup (DB/Service)
participant AccountSvc as Account Service
participant Auth as Token Generator / JWT
participant Redis as Redis
Client->>AccountSvc: POST /account.create(bodyData, deviceInfo, domain, onlyCreate?, req)
alt domain provided
AccountSvc->>TenantSvc: resolve tenant by domain
TenantSvc-->>AccountSvc: tenantDetail / not found
else no domain
AccountSvc->>TenantSvc: resolve tenant by tenant_code from req.headers
TenantSvc-->>AccountSvc: tenantDetail / not found
end
AccountSvc->>AccountSvc: validate input, select roles (req.body.roles || defaults)
AccountSvc->>AccountSvc: create user record
alt onlyCreate == false
AccountSvc->>Auth: generate access & refresh tokens (tokenDetail includes user orgs & tenant)
Auth-->>AccountSvc: access_token, refresh_token
AccountSvc->>Redis: persist session & refresh token
Redis-->>AccountSvc: ack
AccountSvc-->>Client: return user + access_token + refresh_token
else onlyCreate == true
AccountSvc-->>Client: return created user (no tokens)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/generics/utils.js (2)
1022-1059: Note: Discrepancy between PR title and code change.The PR title mentions "reading 'outputFilePath'" but the actual code change addresses accessing the
data_typeproperty on a potentially undefinedfindEntity. While the fix correctly prevents undefined property access errors, please verify that this change addresses the specific error mentioned in the PR title, or update the PR description to accurately reflect the fix.
1037-1055: Address fallback behavior when entity type is missing.When
findEntityis undefined (no matching entity inprunedEntities), the code falls through to the else block (lines 1049–1055). This processes metadata without entity type information and returns incomplete results like{name: undefined, id: undefined, externalId: undefined}if the corresponding data isn't found infeederData. The returned data is merged without validation into event bodies and change tracking structures across multiple call sites (account.js, user.js, userInvite.js), which could cause downstream issues. Either add validation to reject incomplete metadata or document why partial results are acceptable.
🧹 Nitpick comments (1)
src/generics/utils.js (1)
1041-1041: Defensive null-check correctly prevents the runtime error.The added check for
findEntityexistence before accessing itsdata_typeproperty prevents the "Cannot read properties of undefined" error when no matching entity is found in theprunedEntitiesarray.Minor note: The optional chaining
findEntity?.data_typeis redundant after the explicitfindEntity &&check, though it's harmless. Consider simplifying to:🔎 Suggested simplification
-if (findEntity && (findEntity?.data_type == 'ARRAY' || findEntity?.data_type == 'ARRAY[STRING]')) { +if (findEntity && (findEntity.data_type == 'ARRAY' || findEntity.data_type == 'ARRAY[STRING]')) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/api-doc/bulkUser.mdis excluded by!src/api-doc/**,!**/*.md
📒 Files selected for processing (1)
src/generics/utils.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/generics/utils.js (1)
src/helpers/userInvite.js (1)
findEntity(579-579)
…crypting phone and passing correct meta in account search
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/account.js (1)
322-334: Type safety issue withreq.body.roles.If
req.body.rolesis truthy but not a string (e.g., already an array), calling.split(',')will throw a TypeError. Consider adding type validation.Proposed fix
role = await roleQueries.findAll( { title: { - [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','), + [Op.in]: req.body.roles + ? (Array.isArray(req.body.roles) ? req.body.roles : req.body.roles.split(',')) + : process.env.DEFAULT_ROLE.split(','), }, tenant_code: tenantDetail.code, },
🤖 Fix all issues with AI agents
In `@src/controllers/v1/tenant.js`:
- Around line 162-168: The JSDoc for the "Create account user" endpoint has an
incorrect `@name` value; change the `@name` annotation from "bulkUserCreate" to
"accountCreate" in the JSDoc block above the accountCreate handler so the docs
match the actual method/intent (look for the comment block containing "Create
account user" and update its `@name`).
In `@src/services/account.js`:
- Around line 1946-1969: The code reads user.user_organizations[0] without
checking existence which can throw if a user has no organizations; update the
loop in the section using user_organizations to first verify
user.user_organizations && user.user_organizations.length > 0, and if absent
either skip processing that user (continue) or fall back to
defaultOrganizationCode for userOrg and handle the missing id when calling
removeDefaultOrgEntityTypes (e.g., pass null/undefined or guard that call).
Ensure the branches adjust the values passed to findUserEntityTypesAndEntities
and removeDefaultOrgEntityTypes and still call utils.processDbResponse only when
safe.
- Around line 384-385: Remove the two debug console.log statements that print
validationData and userModel; either delete them entirely or replace them with
structured logging using the project's logger (e.g., logger.debug or
processLogger.debug) and ensure sensitive fields from validationData/userModel
are omitted or redacted before logging; update occurrences of
console.log('validationData', validationData) and console.log('userModel',
userModel) in src/services/account.js accordingly.
- Line 42: Remove the unused import "use" from i18next in this module: delete
the line that destructures use from the require call (the symbol "use" in the
const { use } = require('i18next') statement) so the file no longer imports an
unused binding; if the require call is otherwise unnecessary, replace or
simplify it accordingly (e.g., remove the entire require if nothing else from
i18next is used).
- Around line 70-84: tenantDomain can be undefined if neither domain nor
req.headers[common.TENANT_CODE_HEADER] are present, causing a TypeError when
accessing tenantDomain.tenant_code in the tenantQueries.findOne call; update the
logic in the block using tenantDomain and tenantId (the code around
tenantDomain, tenantId, TENANT_CODE_HEADER and the notFoundResponse call) to
explicitly handle the missing-case by returning a notFoundResponse (or similar
error) when both domain and the tenant header are absent, or ensure tenantDomain
is initialized (e.g., set tenantDomain = { tenant_code: tenantId }) before
calling tenantQueries.findOne so tenantQueries.findOne always gets a valid
tenant_code.
🧹 Nitpick comments (3)
src/services/account.js (3)
115-116: Remove empty else block.The empty
elseblock adds no value and should be removed for code cleanliness.Proposed fix
if (!domainDetails) { return responses.failureResponse({ message: 'INVALID_ORG_registration_code', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) } - } else { }
506-512: Remove commented-out code.Commented-out code should be removed. If this logic is needed in the future, it can be retrieved from version control.
Proposed fix
- /* let tenantDetails = await organizationQueries.findOne( - { id: user.organization_id }, - { attributes: ['related_orgs'] } - ) - - const tenant_id = - tenantDetails && tenantDetails.parent_id !== null ? tenantDetails.parent_id : user.organization_id */
1730-1730: Good defensive error handling.Swallowing Redis deletion errors with
.catch((err) => {})is appropriate here since this is a fire-and-forget cache invalidation that shouldn't fail the main operation. However, consider logging the error for observability.- utilsHelper.redisDel(redisUserKey).catch((err) => {}) + utilsHelper.redisDel(redisUserKey).catch((err) => { + console.error('Failed to delete Redis cache:', err) + })
| /** | ||
| * Create account user | ||
| * @method POST | ||
| * @name bulkUserCreate | ||
| * @param {Object} req -request data. | ||
| * @returns {JSON} - success or error message | ||
| */ |
There was a problem hiding this comment.
Fix JSDoc @name annotation.
The @name should be accountCreate, not bulkUserCreate.
Proposed fix
/**
* Create account user
* `@method` POST
- * `@name` bulkUserCreate
+ * `@name` accountCreate
* `@param` {Object} req -request data.
* `@returns` {JSON} - success or error message
*/🤖 Prompt for AI Agents
In `@src/controllers/v1/tenant.js` around lines 162 - 168, The JSDoc for the
"Create account user" endpoint has an incorrect `@name` value; change the `@name`
annotation from "bulkUserCreate" to "accountCreate" in the JSDoc block above the
accountCreate handler so the docs match the actual method/intent (look for the
comment block containing "Create account user" and update its `@name`).
| const notificationUtils = require('@utils/notification') | ||
| const userHelper = require('@helpers/userHelper') | ||
| const { broadcastEvent } = require('@helpers/eventBroadcasterMain') | ||
| const { use } = require('i18next') |
There was a problem hiding this comment.
Remove unused import.
The use function is imported from i18next but never used in this file.
Proposed fix
-const { use } = require('i18next')📝 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 { use } = require('i18next') |
🤖 Prompt for AI Agents
In `@src/services/account.js` at line 42, Remove the unused import "use" from
i18next in this module: delete the line that destructures use from the require
call (the symbol "use" in the const { use } = require('i18next') statement) so
the file no longer imports an unused binding; if the require call is otherwise
unnecessary, replace or simplify it accordingly (e.g., remove the entire require
if nothing else from i18next is used).
| let tenantDomain | ||
|
|
||
| const tenantDomain = await tenantDomainQueries.findOne({ domain }) | ||
|
|
||
| if (!tenantDomain) { | ||
| return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN') | ||
| if (domain) { | ||
| tenantDomain = await tenantDomainQueries.findOne({ domain }) | ||
|
|
||
| if (!tenantDomain) { | ||
| return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN') | ||
| } | ||
| } else if (req.headers?.[common.TENANT_CODE_HEADER]) { | ||
| tenantId = req.headers?.[common.TENANT_CODE_HEADER] | ||
| tenantDomain = { tenant_code: tenantId } | ||
| } | ||
|
|
||
| const tenantDetail = await tenantQueries.findOne({ | ||
| code: tenantDomain.tenant_code, |
There was a problem hiding this comment.
Potential TypeError when both domain and tenant header are missing.
If domain is falsy and req.headers?.[common.TENANT_CODE_HEADER] is also undefined, tenantDomain remains undefined. Accessing tenantDomain.tenant_code on line 84 will throw a TypeError.
Proposed fix
if (domain) {
tenantDomain = await tenantDomainQueries.findOne({ domain })
if (!tenantDomain) {
return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN')
}
} else if (req.headers?.[common.TENANT_CODE_HEADER]) {
tenantId = req.headers?.[common.TENANT_CODE_HEADER]
tenantDomain = { tenant_code: tenantId }
+ } else {
+ return notFoundResponse('TENANT_DOMAIN_OR_CODE_REQUIRED')
}🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 70 - 84, tenantDomain can be undefined
if neither domain nor req.headers[common.TENANT_CODE_HEADER] are present,
causing a TypeError when accessing tenantDomain.tenant_code in the
tenantQueries.findOne call; update the logic in the block using tenantDomain and
tenantId (the code around tenantDomain, tenantId, TENANT_CODE_HEADER and the
notFoundResponse call) to explicitly handle the missing-case by returning a
notFoundResponse (or similar error) when both domain and the tenant header are
absent, or ensure tenantDomain is initialized (e.g., set tenantDomain = {
tenant_code: tenantId }) before calling tenantQueries.findOne so
tenantQueries.findOne always gets a valid tenant_code.
| console.log('validationData', validationData) | ||
| console.log('userModel', userModel) |
There was a problem hiding this comment.
Remove debug console.log statements.
These debug statements should be removed before merging to production or converted to structured logging with appropriate log levels.
Proposed fix
- console.log('validationData', validationData)
- console.log('userModel', userModel)📝 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.
| console.log('validationData', validationData) | |
| console.log('userModel', userModel) |
🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 384 - 385, Remove the two debug
console.log statements that print validationData and userModel; either delete
them entirely or replace them with structured logging using the project's logger
(e.g., logger.debug or processLogger.debug) and ensure sensitive fields from
validationData/userModel are omitted or redacted before logging; update
occurrences of console.log('validationData', validationData) and
console.log('userModel', userModel) in src/services/account.js accordingly.
| const defaultOrganizationCode = process.env.DEFAULT_ORGANISATION_CODE | ||
|
|
||
| for (let i = 0; i < users.data.length; i++) { | ||
| let user = users.data[i] | ||
|
|
||
| // Convert Sequelize instance to plain object | ||
| if (user.toJSON) { | ||
| user = user.toJSON() | ||
| users.data[i] = user | ||
| } | ||
|
|
||
| let userOrg = user.user_organizations[0].organization_code | ||
| let validationData = await entityTypeQueries.findUserEntityTypesAndEntities({ | ||
| status: 'ACTIVE', | ||
| organization_code: { | ||
| [Op.in]: [userOrg, defaultOrganizationCode], | ||
| }, | ||
| tenant_code: params.query.tenant_code, | ||
| model_names: { [Op.contains]: [await userQueries.getModelName()] }, | ||
| }) | ||
| const prunedEntities = removeDefaultOrgEntityTypes(validationData, user.user_organizations[0].id) | ||
| const processedUser = await utils.processDbResponse(user, prunedEntities) | ||
| Object.assign(user, processedUser) | ||
| } |
There was a problem hiding this comment.
Missing bounds check on user_organizations array.
Accessing user.user_organizations[0] without checking if the array exists and has elements could cause a runtime error if a user has no organizations.
Proposed fix
for (let i = 0; i < users.data.length; i++) {
let user = users.data[i]
// Convert Sequelize instance to plain object
if (user.toJSON) {
user = user.toJSON()
users.data[i] = user
}
+ if (!user.user_organizations?.length) {
+ continue
+ }
+
let userOrg = user.user_organizations[0].organization_code🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 1946 - 1969, The code reads
user.user_organizations[0] without checking existence which can throw if a user
has no organizations; update the loop in the section using user_organizations to
first verify user.user_organizations && user.user_organizations.length > 0, and
if absent either skip processing that user (continue) or fall back to
defaultOrganizationCode for userOrg and handle the missing id when calling
removeDefaultOrgEntityTypes (e.g., pass null/undefined or guard that call).
Ensure the branches adjust the values passed to findUserEntityTypesAndEntities
and removeDefaultOrgEntityTypes and still call utils.processDbResponse only when
safe.
1 participant
…rties of undefined (reading 'outputFilePath')
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.