Add versioned cache helper; switch services from Redis to cacheClient #820
Conversation
…d update user-role service to use the corrected constant
…type read in user login process
…t and eviction methods
…e, update, and delete operations
…nization in constants
WalkthroughAdds a namespace- and version-aware cache helper, replaces direct Redis usage with cacheClient across many services, introduces per-tenant/org cache keys and invalidation, renames a misspelled language constant, extends several service signatures to accept tenant_code/organization_code, and deletes the legacy redis-communication module. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller (User.read)
participant S as User Service
participant Cache as cacheHelper
participant DB as Database
C->>S: read(id, headers, language, tenantCode, organizationCode)
S->>Cache: versionedKey(ns=profile, orgId=organizationCode, id)
S->>Cache: get(key)
alt Cache hit
Cache-->>S: cached profile
S->>S: enrich roles/permissions, image URL
S-->>C: PROFILE_FETCHED_SUCCESSFULLY
else Cache miss
S->>DB: fetch user + org + roles
alt Not found / role error
S-->>C: USER_NOT_FOUND / ROLE_NOT_FOUND
else Found
S->>Cache: getOrSet(ns=entity_types, key="${orgCode}:${model}", fetchFn)
DB-->>S: entity types (via fetchFn)
S->>S: prune entities, compute permissions
S->>Cache: setScoped(ns=profile, id) Note over S,Cache: conditional (MENTOR/MENTEE)
S-->>C: PROFILE_FETCHED_SUCCESSFULLY
end
end
sequenceDiagram
autonumber
participant U as Client (login)
participant A as Account Service
participant Cache as cacheHelper
participant DB as Database
participant Auth as Token Generator
U->>A: login(identifier, password, tenantCode, orgCode)
A->>Cache: getOrSet(ns=tenant, key=tenant lookup)
alt tenant cached
Cache-->>A: tenantDetail
else
A->>DB: tenant lookup (fetchFn)
DB-->>A: tenantDetail
A->>Cache: set(ns=tenant, tenantDetail)
end
A->>DB: find user by identifier
alt invalid
A-->>U: IDENTIFIER_OR_PASSWORD_INVALID
else valid
A->>DB: create session
A->>Cache: getOrSet(ns=entity_types, key="${org}:${model}")
A->>Auth: generate tokens
A->>Cache: del(namespaced profile key) Note over A,Cache: purge profile cache
A-->>U: tokens + transformed user
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/entityType.js (2)
103-109: Wrong error mapping on update.UniqueConstraintError should map to ALREADY_EXISTS, not ALREADY_DELETED.
- if (error instanceof UniqueConstraintError) { - return responses.failureResponse({ - message: 'ENTITY_TYPE_ALREADY_DELETED', + if (error instanceof UniqueConstraintError) { + return responses.failureResponse({ + message: 'ENTITY_TYPE_ALREADY_EXISTS', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) }
198-216: Add tenant scoping to deleteOneEntityType (prevent cross-tenant deletes)Confirmed: deleteOneEntityType in src/database/queries/entityType.js (lines ~109–116) only filters by id and organization_code; src/services/entityType.js (lines ~198–200) accepts tenantCode but does not forward it.
- Update service call (src/services/entityType.js):
- const deleteCount = await entityTypeQueries.deleteOneEntityType(id, organizationCode) + const deleteCount = await entityTypeQueries.deleteOneEntityType(id, organizationCode, tenantCode)- Update DAO (src/database/queries/entityType.js): change signature to (id, organizationCode, tenantCode) and include tenant_code in the where clause:
where: { id: id, organization_code: organizationCode, tenant_code: tenantCode }
🧹 Nitpick comments (11)
src/services/user-role.js (1)
196-202: roles.rows check is incorrect.roles.rows is an array; compare length.
- if (roles.rows == 0 || roles.count == 0) { + if (roles.rows.length === 0 || roles.count === 0) {src/helpers/userHelper.js (2)
16-16: Comment contradicts implementation.md5Hash is used; update the comment or the hash.
- email: `${utils.md5Hash(userId)}@deletedUser`, // Changed to SHA-256 + email: `${utils.md5Hash(userId)}@deletedUser`, // Pseudonymized using MD5
130-169: Session cleanup still uses helpers↔services dependency; extract shared teardown.To avoid circular deps and keep Redis key handling consistent, move session teardown into helpers/sessionCleanup.js and reuse here and in services.
We used your prior preference from learnings for session cleanup extraction.
src/services/entities.js (1)
10-34: Invalidate profile namespace at root when default org changes (align with entityType.js).entityType.js also bumps root profile namespace for default org; mirror here for consistency.
if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { await cacheClient.invalidateNamespaceVersion({ tenantCode, ns: common.CACHE_CONFIG.namespaces.entity_types.name, }) + await cacheClient.invalidateNamespaceVersion({ + tenantCode, + ns: common.CACHE_CONFIG.namespaces.profile.name, + }) }src/constants/common.js (1)
116-128: CACHE_CONFIG addition is solid; consider deprecating legacy Redis constants.Deprecate redisUserPrefix/redisUserCacheTTL to avoid dual sources of truth.
src/services/entityType.js (2)
72-74: Avoid comma operator for assignments.Use standard statements for clarity.
- ;(bodyData.updated_by = loggedInUserId), (bodyData.organization_code = organizationCode) + bodyData.updated_by = loggedInUserId + bodyData.organization_code = organizationCode
114-141: getOrSet: redundant key/id; pass only id (scoped by ns/org/version).Keeps the API usage consistent.
- const prunedEntities = await cacheClient.getOrSet({ - key: cacheId, + const prunedEntities = await cacheClient.getOrSet({ tenantCode, orgId: organizationCode, ns, id: cacheId, fetchFn, })src/services/user.js (1)
353-359: Consider extracting language handling logic.The role label translation logic could be extracted into a helper function for better reusability and testability.
-if (language && language !== common.ENGLISH_LANGUAGE_CODE) { - utils.setRoleLabelsByLanguage(roles, language) -} else { - roles.forEach((r) => { - delete r.translations - }) -} +utils.processRoleLanguage(roles, language, common.ENGLISH_LANGUAGE_CODE)Add this helper to utils:
processRoleLanguage(roles, language, defaultLanguageCode) { if (language && language !== defaultLanguageCode) { this.setRoleLabelsByLanguage(roles, language) } else { roles.forEach((r) => delete r.translations) } }src/services/account.js (1)
720-726: Regex patterns need escaping and validation.The regex patterns have potential issues:
- The email regex doesn't escape dots properly
- Consider using established email validation libraries for better coverage of edge cases
-const EMAIL_RE = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/ +const EMAIL_RE = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,63}$/Consider using a more comprehensive email validation:
const emailValidator = require('email-validator'); const isEmailFormat = (s) => emailValidator.validate(s);src/generics/cacheHelper.js (2)
94-99: Consider connection pooling for Redis client.The
getRedisClientfunction creates a new client reference each time. Consider implementing connection pooling or reusing the client instance.+let _redisClient = null; + function getRedisClient() { try { - if (RedisCache && typeof RedisCache.native === 'function') return RedisCache.native() + if (!_redisClient && RedisCache && typeof RedisCache.native === 'function') { + _redisClient = RedisCache.native() + } + return _redisClient } catch (err) { console.log(err, 'error in getting native redis client') } }
214-231: Consider adding retry logic for Redis operations.The set operation could benefit from retry logic for transient Redis failures, especially for critical data.
async function set(key, value, ttlSeconds, { useInternal = false } = {}) { if (!ENABLE_CACHE) return false let wroteRedis = false + let retries = 3 + while (retries > 0 && !wroteRedis) { try { if (ttlSeconds) await RedisCache.setKey(key, value, ttlSeconds) else await RedisCache.setKey(key, value) wroteRedis = true } catch (e) { - console.error('redis set error', e) + retries-- + if (retries === 0) { + console.error('redis set error after retries', e) + } else { + await new Promise(r => setTimeout(r, 100 * (4 - retries))) + } } + } // Only write to InternalCache if opted in if (useInternal && InternalCache && InternalCache.setKey) { try { InternalCache.setKey(key, value) } catch (e) {} } return wroteRedis }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/configs/redis.jsis excluded by!src/configs/**
📒 Files selected for processing (11)
src/constants/common.js(2 hunks)src/controllers/v1/user.js(1 hunks)src/generics/cacheHelper.js(1 hunks)src/generics/redis-communication.js(0 hunks)src/helpers/userHelper.js(2 hunks)src/package.json(1 hunks)src/services/account.js(9 hunks)src/services/entities.js(4 hunks)src/services/entityType.js(6 hunks)src/services/user-role.js(1 hunks)src/services/user.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/generics/redis-communication.js
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/user-role.jssrc/services/entities.jssrc/services/entityType.jssrc/services/user.jssrc/services/account.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/user.js
🧠 Learnings (3)
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: In ELEVATE-Project/user, avoid potential circular dependencies between src/helpers/userHelper.js and src/services/user-sessions by extracting shared session cleanup logic (finding active sessions, deleting Redis keys, ending sessions) into a dedicated utility (e.g., helpers/sessionCleanup.js) that both layers can consume.
Applied to files:
src/helpers/userHelper.jssrc/services/user.js
📚 Learning: 2025-08-08T15:12:44.423Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:117-156
Timestamp: 2025-08-08T15:12:44.423Z
Learning: In ELEVATE-Project/user, prefer extracting shared session teardown (find active sessions, delete Redis keys, mark ended_at) into helpers/sessionCleanup.js so both helpers and services use it, preventing upward (helper→service) dependencies and avoiding circular imports.
Applied to files:
src/helpers/userHelper.jssrc/services/user.js
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: When session management is shared across helpers and services in ELEVATE-Project/user, prefer extracting the primitives (query active sessions, delete Redis keys, mark ended) into a lower-level generics utility to avoid helper↔service circular dependencies and keep Redis key handling consistent.
Applied to files:
src/helpers/userHelper.js
🧬 Code graph analysis (5)
src/services/entities.js (3)
src/services/user.js (7)
common(10-10)require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)cacheClient(28-28)src/services/entityType.js (5)
common(4-4)require(6-6)require(7-7)require(8-8)cacheClient(10-10)src/services/account.js (6)
common(16-16)require(26-26)require(27-27)require(37-37)require(41-41)cacheClient(42-42)
src/helpers/userHelper.js (1)
src/services/account.js (7)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)ns(2044-2044)common(16-16)
src/services/entityType.js (3)
src/services/user.js (18)
common(10-10)entityTypeQueries(15-15)cacheClient(28-28)ns(146-146)ns(310-310)ns(587-587)attributes(252-252)attributes(278-278)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)filter(65-72)filter(242-245)filter(277-277)filter(308-308)filter(445-445)filter(554-560)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/database/queries/entityType.js (3)
entities(141-145)result(53-59)result(147-153)
src/services/user.js (6)
src/services/entityType.js (5)
cacheClient(10-10)ns(117-117)ns(162-162)common(4-4)prunedEntities(132-139)src/services/account.js (27)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)ns(2044-2044)common(16-16)cacheKey(2045-2050)tenantCode(706-706)organizationCode(194-194)utils(25-25)options(1634-1640)options(2116-2116)user(136-145)user(758-758)user(900-900)user(950-950)user(1103-1103)user(1256-1265)user(1437-1445)user(1785-1785)user(2008-2011)user(2123-2123)roles(1923-1925)prunedEntities(366-366)prunedEntities(843-857)prunedEntities(1550-1550)src/generics/utils.js (7)
require(12-12)require(16-16)require(21-21)common(20-20)removeDefaultOrgEntityTypes(835-842)value(892-892)value(914-914)src/controllers/v1/user.js (2)
userDetails(48-54)userDetails(85-85)src/database/queries/users.js (1)
user(225-270)src/generics/cacheHelper.js (1)
raw(118-118)
src/services/account.js (4)
src/helpers/userHelper.js (2)
cacheClient(2-2)utils(3-3)src/services/user.js (24)
cacheClient(28-28)require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)common(10-10)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
🔇 Additional comments (22)
src/services/user-role.js (1)
203-210: Good fix: use ENGLISH_LANGUAGE_CODE.src/services/entities.js (1)
53-55: LGTM: post-mutation cache invalidation hooks.Also applies to: 113-115, 195-197
src/constants/common.js (1)
104-104: Rename looks good.src/services/entityType.js (1)
13-41: Invalidate both entity_types and profile; good.Matches the caching model.
src/controllers/v1/user.js (1)
52-54: Confirmed — only one call site and organization_code is passedOnly invocation found: src/controllers/v1/user.js (lines 44–56); userService.read is called with req.decodedToken.organization_code.
src/package.json (1)
46-46: Confirm elevate-node-cache v2 compatibility and update lockfile.
- Verify v2 preserves the APIs this repo uses (RedisCache.getKey/setKey/deleteKey, RedisCache.deleteKey, RedisCache.native() returning a client with incr, and InternalCache.getKey/setKey/delKey) — check src/package.json (dependency), src/generics/cacheHelper.js and src/generics/utils.js.
- No lockfile found at repo root (yarn.lock / package-lock.json / pnpm-lock.yaml); after confirming compatibility run the project install for your package manager and commit the generated lockfile.
src/services/user.js (6)
28-28: LGTM!The addition of the cacheClient import is properly placed with other dependencies.
365-379: Cache key composition strategy looks solid.The per-organization cache key using
${userOrg}:${modelName}provides good isolation. The lazy loading pattern withgetOrSetis well implemented.
146-153: Cache invalidation on update is correct.The cache invalidation after user update properly uses the namespaced key with the profile namespace.
396-404: Confirm selective caching for mentor/mentee only.Code caches profiles only when utils.validateRoleAccess(roles, [common.MENTOR_ROLE, common.MENTEE_ROLE]) (src/services/user.js ~396–404). The repo also uses ADMIN_ROLE, ORG_ADMIN_ROLE, SESSION_MANAGER_ROLE and USER_ROLE elsewhere — confirm this restriction is intentional. If not intentional, include the other roles in the cache or add an inline comment explaining why only mentor/mentee are cached.
308-327: Confirm profile cache invalidation across all user write paths.
- Cache read logic is fine. cacheClient.del calls found: src/services/user.js:151, src/services/user.js:594, src/helpers/userHelper.js:103, src/services/account.js:2051.
- Namespace invalidation for profile exists in src/services/entities.js and src/services/entityType.js.
- Potential gaps: update/create calls at src/services/admin.js:749, src/services/org-admin.js:612, src/services/userInvite.js:349, src/services/account.js:1526/1748/1866 use utils.redisDel or lack an obvious cacheClient.del — verify these clear the profile cache or bump the profile namespace version.
- Action: ensure every create/update/delete path either calls cacheClient.del(versionedKey) for the profile key or uses cacheClient.invalidate* to bump the profile namespace version.
306-306: Signature change verified — no action required.The only direct caller (src/controllers/v1/user.js — line 48) passes (id, headers, language, tenant_code, organization_code), which matches the new signature in src/services/user.js (line 306).
src/services/account.js (5)
42-42: LGTM!The cacheClient import is properly added.
685-690: Good helper function extraction.The
makeNotFoundResponsehelper reduces code duplication and improves consistency.
693-714: Excellent caching strategy for tenant lookups.The cache implementation for tenant domain and tenant detail lookups is well-designed, reducing database hits for frequently accessed configuration data.
843-856: Cache configuration for entity types is well implemented.The caching strategy for entity types with per-organization keys provides good isolation and performance benefits.
799-806: Verified — admin role pruning is intentional and consistent. Normal account login intentionally removes common.ADMIN_ROLE from user.user_organizations; admin-specific flows validate presence of ADMIN_ROLE (src/services/admin.js), org-admin updates strip ADMIN_ROLE when assigning roles (src/services/org-admin.js), and validators disallow assigning ADMIN_ROLE via normal account endpoints (src/validators/v1/account.js).src/generics/cacheHelper.js (5)
1-15: Well-structured cache configuration initialization.The configuration loading with fallback to constants is robust and handles JSON parsing errors gracefully.
137-172: Atomic version bumping implementation is solid.The version bump logic with atomic INCR operation and multiple fallback strategies ensures reliability. The in-process cache update maintains consistency.
324-349: Pattern-based deletion with SCAN is well implemented.The implementation correctly uses SCAN to avoid blocking operations and handles batch deletion efficiently with UNLINK support.
257-275: getOrSet implementation follows best practices.The lazy loading pattern with proper namespace checking and TTL resolution is well designed. The versioned key generation ensures cache invalidation safety.
83-87: Good sharding strategy for key distribution.The MD5-based sharding provides even distribution across shards, which helps with Redis cluster scalability.
There was a problem hiding this comment.
Actionable comments posted: 4
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)
799-806: Wrong constant: admin-role removal likely ineffective.Elsewhere you use common.ORG_ADMIN_ROLE; here you compare against common.ADMIN_ROLE. This may fail to strip admin roles.
- if (org.roles) { - org.roles = org.roles.filter((r) => r.role?.title?.toLowerCase() !== common.ADMIN_ROLE) - } + if (org.roles) { + const adminTitle = (common.ORG_ADMIN_ROLE || 'org_admin').toLowerCase() + org.roles = org.roles.filter((r) => r.role?.title?.toLowerCase() !== adminTitle) + }Optionally handle both ADMIN_ROLE and ORG_ADMIN_ROLE if both exist.
♻️ Duplicate comments (1)
src/services/user.js (1)
587-595: Resolved: orgId uses organizationCode here.This addresses the earlier “orgCode undefined” feedback on this path. Cache invalidation looks correct.
🧹 Nitpick comments (7)
src/services/user.js (2)
146-154: Profile cache invalidation added — OK.Using namespacedKey + del for the profile namespace is consistent with the new helper. Minor: parameter naming in this class alternates between orgCode and organizationCode; consider standardizing for readability.
318-327: Avoid mutating cached object on read.If cacheClient.get returns the stored object reference, rewriting image risks polluting cache values. Clone before mutation.
- const userDetails = await cacheClient.get(fullKey) - if (userDetails) { - if (userDetails.image) userDetails.image = await utils.getDownloadableUrl(userDetails.image) + const cached = await cacheClient.get(fullKey) + if (cached) { + const userDetails = _.cloneDeep(cached) + if (userDetails.image) userDetails.image = await utils.getDownloadableUrl(userDetails.image)src/services/account.js (5)
719-727: Email/phone/username regexes are strict; consider minimal tweaks.
- Email: allow “+” tag (e.g., name+tag@domain.com).
- Username: current is fine; phone is E.164, fine.
- const EMAIL_RE = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/ + const EMAIL_RE = /^[\w.+-]+@([\w-]+\.)+[\w-]{2,}$/ // looser TLD length, allow '+'
745-752: Phone login path should validate phone_code presence.Query requires phone_code; if missing, it silently fails. Return a clear validation error.
} else if (isPhoneFormat(identifier)) { + if (!bodyData.phone_code) { + return responses.failureResponse({ + message: 'PHONE_CODE_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } query[Op.or].push({ phone: emailEncryption.encrypt(identifier), phone_code: bodyData.phone_code, })
769-778: Parse env before numeric comparison.ALLOWED_ACTIVE_SESSIONS is a string; parse to int to avoid edge coercions.
- if (process.env.ALLOWED_ACTIVE_SESSIONS != null) { + const maxSessions = process.env.ALLOWED_ACTIVE_SESSIONS != null + ? parseInt(process.env.ALLOWED_ACTIVE_SESSIONS, 10) + : null + if (maxSessions != null) { const activeSessionCount = await userSessionsService.activeUserSessionCounts(user.id) - if (activeSessionCount >= process.env.ALLOWED_ACTIVE_SESSIONS) { + if (activeSessionCount >= maxSessions) {
839-841: Only push default org code if defined.- orgCodes.push(process.env.DEFAULT_ORGANISATION_CODE) + if (process.env.DEFAULT_ORGANISATION_CODE) { + orgCodes.push(process.env.DEFAULT_ORGANISATION_CODE) + }
2053-2055: Use a single utils alias for Redis helpers.You mix utilsHelper and utils for Redis ops across the file. Stick to one (utilsHelper) for consistency.
- await utils.redisDel(user?.username) + await utilsHelper.redisDel(user?.username)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/account.js(9 hunks)src/services/user.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/user.jssrc/services/account.js
🧠 Learnings (5)
📓 Common learnings
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: When session management is shared across helpers and services in ELEVATE-Project/user, prefer extracting the primitives (query active sessions, delete Redis keys, mark ended) into a lower-level generics utility to avoid helper↔service circular dependencies and keep Redis key handling consistent.
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: In ELEVATE-Project/user, avoid potential circular dependencies between src/helpers/userHelper.js and src/services/user-sessions by extracting shared session cleanup logic (finding active sessions, deleting Redis keys, ending sessions) into a dedicated utility (e.g., helpers/sessionCleanup.js) that both layers can consume.
Applied to files:
src/services/user.js
📚 Learning: 2025-08-08T15:12:44.423Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:117-156
Timestamp: 2025-08-08T15:12:44.423Z
Learning: In ELEVATE-Project/user, prefer extracting shared session teardown (find active sessions, delete Redis keys, mark ended_at) into helpers/sessionCleanup.js so both helpers and services use it, preventing upward (helper→service) dependencies and avoiding circular imports.
Applied to files:
src/services/user.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/services/user.jssrc/services/account.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 the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/user.jssrc/services/account.js
🧬 Code graph analysis (2)
src/services/user.js (6)
src/services/account.js (27)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)ns(2044-2044)common(16-16)cacheKey(2045-2050)tenantCode(706-706)organizationCode(194-194)utils(25-25)options(1634-1640)options(2116-2116)user(136-145)user(758-758)user(900-900)user(950-950)user(1103-1103)user(1256-1265)user(1437-1445)user(1785-1785)user(2008-2011)user(2123-2123)roles(1923-1925)prunedEntities(366-366)prunedEntities(843-857)prunedEntities(1550-1550)src/services/entityType.js (5)
cacheClient(10-10)ns(117-117)ns(162-162)common(4-4)prunedEntities(132-139)src/generics/utils.js (7)
require(12-12)require(16-16)require(21-21)common(20-20)removeDefaultOrgEntityTypes(835-842)value(892-892)value(914-914)src/controllers/v1/user.js (2)
userDetails(48-54)userDetails(85-85)src/database/queries/users.js (1)
user(225-270)src/generics/cacheHelper.js (1)
raw(118-118)
src/services/account.js (4)
src/services/user.js (19)
cacheClient(28-28)common(10-10)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/services/entityType.js (5)
cacheClient(10-10)prunedEntities(132-139)entityTypeQueries(5-5)ns(117-117)ns(162-162)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
🔇 Additional comments (5)
src/services/user.js (1)
28-29: Good: adopting cacheClient.Import looks correct and aligns with the cache migration.
src/services/account.js (4)
42-42: Good: centralized cache helper import.
692-700: Tenant-domain cache lookup — nice.Using getOrSet for domain → tenant resolution reduces DB hits.
706-714: Tenant detail cache lookup — LGTM.
843-857: Entity-types cache usage — LGTM.Good use of per-org+model cache key and pruning.
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 (1)
src/services/account.js (1)
2013-2016: Runtime error: ERROR_MESSAGES is undefined.Replace with the literal used elsewhere or import the constant.
-return responses.failureResponse({ - message: ERROR_MESSAGES.USER_DOESNOT_EXISTS, +return responses.failureResponse({ + message: 'USER_DOESNOT_EXISTS',
🧹 Nitpick comments (6)
src/services/account.js (6)
719-726: Identifier validation: enforce phone_code when identifier is phone; consider centralizing regex.Missing
phone_codewill yield no match; return a clear 400. Also these regexes are duplicated across the codebase—prefer a shared validator.Apply:
} else if (isPhoneFormat(identifier)) { + if (!bodyData.phone_code) { + return responses.failureResponse({ + message: 'PHONE_CODE_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } query[Op.or].push({ phone: emailEncryption.encrypt(identifier), phone_code: bodyData.phone_code, })
768-773: Coerce ALLOWED_ACTIVE_SESSIONS to a number.Avoid implicit string-to-number coercion and edge cases.
-if (process.env.ALLOWED_ACTIVE_SESSIONS != null) { - const activeSessionCount = await userSessionsService.activeUserSessionCounts(user.id) - if (activeSessionCount >= process.env.ALLOWED_ACTIVE_SESSIONS) { +const maxSessions = Number(process.env.ALLOWED_ACTIVE_SESSIONS) +if (Number.isFinite(maxSessions)) { + const activeSessionCount = await userSessionsService.activeUserSessionCounts(user.id) + if (activeSessionCount >= maxSessions) {
839-841: Don’t push undefined default org code into IN clause.If
DEFAULT_ORGANISATION_CODEis unset, the query may includeundefined.-const orgCodes = user.organizations?.map((o) => o.code).filter(Boolean) || [] -orgCodes.push(process.env.DEFAULT_ORGANISATION_CODE) +const orgCodes = [...(user.organizations?.map((o) => o.code).filter(Boolean) || [])] +if (process.env.DEFAULT_ORGANISATION_CODE) { + orgCodes.push(process.env.DEFAULT_ORGANISATION_CODE) +}
2044-2051: Profile cache invalidation looks correct; consider multi-org invalidation.If users belong to multiple orgs, also clear profile cache for each org membership (loop org codes or use a scoped delete if available).
-const cacheKey = cacheClient.namespacedKey({ - tenantCode, - orgId: organizationCode, - ns, - id: userId, -}) -await cacheClient.del(cacheKey) +for (const orgCode of new Set([organizationCode, ...(user.organizations?.map(o => o.code) || [])])) { + const cacheKey = cacheClient.namespacedKey({ tenantCode, orgId: orgCode, ns, id: userId }) + await cacheClient.del(cacheKey) +}
2053-2055: Use consistent redis helper.Other methods use
utilsHelper.redisDel; keep it consistent.-await utils.redisDel(user?.username) +await utilsHelper.redisDel(user?.username)
756-759: Guard toJSON or normalize findUserWithOrganization return typefindUserWithOrganization(filter, options = {}, raw = false) returns the raw DB result when the 3rd arg is truthy (it skips the internal toJSON/transform). Calling .toJSON() on a plain object will throw — make the callsite robust or make the helper consistently return a plain object.
- Replace at src/services/account.js (~lines 740–770):
-const userInstance = await userQueries.findUserWithOrganization(query, {}, true) -let user = userInstance ? userInstance.toJSON() : null +const userInstance = await userQueries.findUserWithOrganization(query, {}, true) +const user = userInstance && typeof userInstance.toJSON === 'function' ? userInstance.toJSON() : userInstance
- Also fix inconsistent 3rd-arg usage (e.g. src/services/account.js:1088 passes tenantDomain.tenant_code as the 3rd arg) — pass tenant_code in the filter/options or change the helper signature to accept tenantCode separately.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/account.js(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/account.js
🧠 Learnings (2)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/account.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 the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/account.js
🧬 Code graph analysis (1)
src/services/account.js (5)
src/services/entities.js (1)
cacheClient(7-7)src/services/entityType.js (5)
cacheClient(10-10)prunedEntities(132-139)entityTypeQueries(5-5)ns(117-117)ns(162-162)src/services/user.js (24)
cacheClient(28-28)require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)common(10-10)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
🔇 Additional comments (4)
src/services/account.js (4)
799-806: Confirm admin role title constant.You filter by
common.ADMIN_ROLE, elsewhere the codebase usesORG_ADMIN_ROLE. Verify the actual DB role title to ensure roles are pruned as intended.
843-857: Good: per-tenant, per-org cached entity type pruning.Use of
cacheClient.getOrSetwith namespace + scoped id is solid and consistent with user.js.
2057-2065: Scope session lookup by tenant (defensive).If session table isn’t globally unique by user_id, add
tenant_codeto the query.const userSessionData = await userSessionsService.findUserSession( { user_id: userId, ended_at: null, + tenant_code: tenantCode, },
692-714: LGTM: Tenant and tenant-domain cached lookups.Correct namespace selection and ids; aligns with the new cacheClient usage.
…idate cache on role updates and deletions
…paces in admin service
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/services/account.js (4)
430-437: Hardcoded organization_id=1 for credentials.This incorrectly links credentials to org 1. Use the created userOrg id.
- organization_id: 1, + organization_id: userOrg.id,
2019-2023: ReferenceError: ERROR_MESSAGES is undefined.Use the locale key string directly.
- message: ERROR_MESSAGES.USER_DOESNOT_EXISTS, + message: 'USER_DOESNOT_EXISTS',
1023-1030: Wrong record updated when issuing new access token.
updateUserSessionfilters by user id; should filter by session id.- let check = await userSessionsService.updateUserSession( - { - id: decodedToken.data.id, - }, - { token: accessToken } - ) + await userSessionsService.updateUserSession( + { id: decodedToken.data.session_id }, + { token: accessToken } + )
1536-1537: Resets: deleting the wrong OTP key.OTP was stored under
user.username; deletingencryptedIdentifierwon’t clear it.- await utilsHelper.redisDel(encryptedIdentifier) + await utilsHelper.redisDel(user.username)src/database/queries/organization.js (1)
152-175: Removing multiple related orgs won’t work with a single array_remove call.
array_remove(column, removedOrgIds)removes only a single value; arrays won’t be fully processed.- exports.removeRelatedOrg = async (removedOrgIds, ids, tenantCode, options = {}) => { + exports.removeRelatedOrg = async (removedOrgIds, ids, tenantCode, options = {}) => { try { - const result = await Organization.update( - { - related_orgs: sequelize.fn('array_remove', sequelize.col('related_orgs'), removedOrgIds), - }, + const expr = (Array.isArray(removedOrgIds) ? removedOrgIds : [removedOrgIds]) + .reduce((acc, id) => sequelize.fn('array_remove', acc, id), sequelize.col('related_orgs')) + const result = await Organization.update( + { related_orgs: expr }, { where: { id: ids, tenant_code: tenantCode, // enforce same tenant }, ...options, individualHooks: true, } )
🧹 Nitpick comments (11)
src/services/account.js (3)
98-104: Message key typo: INVALID_ORG_registration_code does not exist in locales.Locales define
INVALID_ORG_REGISTRATION_CODE. Current key won’t translate.- message: 'INVALID_ORG_registration_code', + message: 'INVALID_ORG_REGISTRATION_CODE',Also applies to: 1224-1230
351-366: Validation may prefer default entity types due to null orgId.
removeDefaultOrgEntityTypes(validationData, userOrgId)passesnull, so default types may override org-specific ones.Option A (preferred): fetch org by code to get id before pruning.
const org = await organizationQueries.findOne( { code: organizationCode, tenant_code: tenantDetail.code }, { attributes: ['id'] } ) const prunedEntities = removeDefaultOrgEntityTypes(validationData, org?.id || null)Option B: add a helper that prefers by organization_code.
561-566: Non-awaited cache invalidation.If you intend fire-and-forget, at least
.catch()errors; otherwiseawaitto ensure consistency.- cacheClient.invalidateOrgNamespaceVersion({ + await cacheClient.invalidateOrgNamespaceVersion({ tenantCode: tenantDetail.code, orgId: user.organization_id, ns: common.CACHE_CONFIG.namespaces.organization.name, -}) +}).catch(() => {})src/services/tenant.js (1)
640-645: Message mismatch for removeDomain success.Method removes domains but returns “TENANT_DOMAINS_ADDED_SUCCESSFULLY”.
- message: 'TENANT_DOMAINS_ADDED_SUCCESSFULLY', + message: 'TENANT_DOMAINS_REMOVED_SUCCESSFULLY',src/services/admin.js (1)
604-609: Await cache invalidations and handle errors.Prevents silent failures and races post admin mapping.
- cacheClient.invalidateOrgNamespaceVersion({ + await cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationId, ns: common.CACHE_CONFIG.namespaces.organization.name, -}) +}).catch(() => {}) ... - await cacheClient.del(cacheKey) + await cacheClient.del(cacheKey).catch(() => {})Also applies to: 610-617
src/constants/common.js (1)
116-128: CACHE_CONFIG addition aligns with the new cache layer.Config shape is coherent and namespace TTLs are sensible.
Consider freezing the object to prevent accidental mutation at runtime:
module.exports = { ... - CACHE_CONFIG: { + CACHE_CONFIG: Object.freeze({ enableCache: true, enableTracking: true, shards: 32, versionDefault: 0, namespaces: { profile: { name: 'profile', enabled: true, defaultTtl: 3600, useInternal: false }, entity_types: { name: 'entity_types', enabled: true, defaultTtl: 86400, useInternal: false }, tenant: { name: 'tenant', enabled: true, defaultTtl: 21600, useInternal: false }, branding: { name: 'branding', enabled: true, defaultTtl: 43200, useInternal: false }, organization: { name: 'organization', enabled: true, defaultTtl: 21600, useInternal: false }, }, - }, + }),src/services/user-role.js (3)
82-82: Method signature changed; update JSDoc.Add userOrganizationCode to the update() JSDoc to keep docs in sync.
/** * Update a role by ID. @@ - * @param {number} userOrganizationId - ID of the organization. + * @param {number} userOrganizationId - ID of the organization. + * @param {string} userOrganizationCode - Code of the organization (for cache invalidation). * @param {string} tenantCode - Tenant code of the requestor. * @returns {Promise<Object>} - Updated role response. */
94-112: Run cache invalidations in parallel to cut latency.Two awaits serialize invalidations; use Promise.allSettled.
- await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.organization.name, - }) - .catch((error) => { - console.error(error) - }) - await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.profile.name, - }) - .catch((error) => { - console.error(error) - }) + const { organization, profile } = common.CACHE_CONFIG.namespaces + const tasks = [organization.name, profile.name].map((ns) => + cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: userOrganizationCode, ns }) + ) + const results = await Promise.allSettled(tasks) + results.forEach((r) => r.status === 'rejected' && console.error(r.reason))
148-156: Method signature changed; update JSDoc.Add userOrganizationCode to delete() JSDoc.
/** * Delete a role by ID. @@ - * @param {number} userOrganizationId - ID of the organization. + * @param {number} userOrganizationId - ID of the organization. + * @param {string} userOrganizationCode - Code of the organization (for cache invalidation). * @param {string} tenantCode - Tenant code of the requestor. * @returns {Promise<Object>} - Deletion result response. */src/services/organization.js (2)
181-181: Update() signature changed; align JSDoc.Add tenantCode to the JSDoc for update().
/** * Update Organization @@ - * @returns {JSON} - Update Organization data. + * @param {string} tenantCode + * @returns {JSON} - Update Organization data. */
632-632: Use console.error for errors.Aligns with other catch blocks and log levels.
- console.log(error) + console.error(error)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/constants/common.js(2 hunks)src/controllers/v1/organization.js(3 hunks)src/controllers/v1/user-role.js(2 hunks)src/database/queries/organization.js(3 hunks)src/generics/cacheHelper.js(1 hunks)src/locales/en.json(1 hunks)src/services/account.js(10 hunks)src/services/admin.js(3 hunks)src/services/organization.js(6 hunks)src/services/public.js(3 hunks)src/services/tenant.js(4 hunks)src/services/user-role.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/generics/cacheHelper.js
🧰 Additional context used
📓 Path-based instructions (3)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/tenant.jssrc/services/organization.jssrc/services/account.jssrc/services/public.jssrc/services/user-role.jssrc/services/admin.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/organization.jssrc/controllers/v1/user-role.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.js
🧠 Learnings (4)
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/controllers/v1/organization.jssrc/controllers/v1/user-role.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 the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/services/organization.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/user-role.jssrc/services/account.jssrc/services/user-role.jssrc/services/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 the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/account.js
🧬 Code graph analysis (8)
src/services/tenant.js (3)
src/services/public.js (1)
cacheClient(15-15)src/services/account.js (5)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)src/services/organization.js (3)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)
src/controllers/v1/organization.js (1)
src/services/organization.js (2)
updatedOrg(567-579)updatedOrg(679-683)
src/services/organization.js (2)
src/database/queries/organization.js (5)
require(2-2)require(3-3)organization(70-74)organization(241-241)t(7-7)src/controllers/v1/organization.js (1)
updatedOrg(95-100)
src/services/account.js (6)
src/services/public.js (11)
cacheClient(15-15)tenantDetail(37-42)tenantDetail(94-99)common(14-14)tenantDomain(32-32)tenantDomain(84-89)tenantQueries(2-2)prunedEntities(165-165)modelName(151-151)entityTypeQueries(8-8)utils(10-10)src/services/organization.js (10)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)tenantCode(272-272)utils(5-5)cacheKey(135-135)cacheKey(243-243)src/services/entityType.js (5)
cacheClient(10-10)prunedEntities(132-139)entityTypeQueries(5-5)ns(117-117)ns(162-162)src/services/user.js (19)
cacheClient(28-28)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)common(10-10)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
src/database/queries/organization.js (3)
src/controllers/v1/organization.js (10)
tenantCode(221-221)tenantCode(266-266)result(124-124)result(143-143)result(159-163)result(180-185)result(194-198)result(206-210)result(251-255)result(296-300)src/services/organization.js (3)
tenantCode(272-272)result(396-396)result(885-885)src/database/models/organization.js (1)
Organization(3-67)
src/services/public.js (1)
src/services/account.js (13)
cacheClient(42-42)tenantDetail(74-77)tenantDetail(713-719)tenantDetail(1064-1066)tenantDetail(1196-1199)tenantDetail(1394-1396)common(16-16)tenantQueries(34-34)tenantDomain(69-69)tenantDomain(699-705)tenantDomain(1058-1058)tenantDomain(1191-1191)tenantDomain(1388-1388)
src/services/user-role.js (4)
src/services/account.js (3)
cacheClient(42-42)tenantCode(712-712)common(16-16)src/services/organization.js (7)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)tenantCode(272-272)common(2-2)common(597-597)common(698-698)src/services/user.js (3)
cacheClient(28-28)userOrganizationCode(474-474)common(10-10)src/controllers/v1/user-role.js (1)
updateRole(60-66)
src/services/admin.js (3)
src/services/account.js (14)
cacheClient(42-42)common(16-16)cacheKey(2051-2056)user(136-145)user(764-764)user(906-906)user(956-956)user(1109-1109)user(1262-1271)user(1443-1451)user(1791-1791)user(2014-2017)user(2129-2129)ns(2050-2050)src/services/organization.js (9)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)cacheKey(135-135)cacheKey(243-243)i(307-307)src/services/user.js (10)
cacheClient(28-28)common(10-10)cacheKey(147-152)cacheKey(588-593)user(50-53)user(335-335)user(458-458)ns(146-146)ns(310-310)ns(587-587)
🪛 Biome (2.1.2)
src/services/user-role.js
[error] 173-173: 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)
🔇 Additional comments (26)
src/services/account.js (2)
1109-1110: Duplicate: third arg to findUserWithOrganization is raw boolean, not tenant code.Passing
tenantDomain.tenant_codeas the 3rd arg inadvertently setsraw=trueand changes return shape. Include tenant in the filter (you already do) and drop the 3rd arg.- const user = await userQueries.findUserWithOrganization(query, {}, tenantDomain.tenant_code) + const user = await userQueries.findUserWithOrganization(query, {})
2050-2057: Good: profile cache key deletion uses new namespacing.NamespacedKey params look correct; matches usage elsewhere.
If profile cache is also stored under any alt key shapes, consider deleting those too.
src/locales/en.json (1)
140-141: LGTM: added RELATED_ORGANIZATIONS_NOT_FOUND.Entry looks consistent with existing keys.
src/services/tenant.js (4)
33-34: LGTM: cache helper introduced for tenant invalidations.
442-442: LGTM: invalidate tenant version after update. Silent catch is okay here to avoid blocking writes.
524-525: LGTM: invalidate after addDomain.
638-639: LGTM: invalidate after removeDomain.src/controllers/v1/organization.js (3)
95-101: LGTM: passing tenant_code into orgService.update.Keeps tenant scoping consistent with cache invalidations downstream.
194-199: LGTM: addRelatedOrg now tenant-scoped.Matches updated query/service behavior.
206-211: LGTM: removeRelatedOrg now tenant-scoped.src/database/queries/organization.js (1)
114-134: LGTM: appendRelatedOrg now enforces tenant and avoids duplicates/nulls.Also applies to: 139-146
src/services/admin.js (1)
40-41: LGTM: cache helper imported for admin workflows.src/controllers/v1/user-role.js (2)
60-66: Resolved — roleService.update signature includes organization_code
Service method is defined at src/services/user-role.js:82 as static async update(id, bodyData, userOrganizationId, userOrganizationCode, tenantCode); the controller call matches — no change required.
84-89: roleService.delete signature verified — no action requiredService signature is static async delete(id, userOrganizationId, userOrganizationCode, tenantCode) and the controller call passes the matching values (id, organization_id, organization_code, tenant_code).
src/services/public.js (3)
15-15: Import of cache helper is correct.
37-42: Tenant cache read: LGTM.Keying by tenantCode within the tenant namespace is consistent with CACHE_CONFIG.
49-59: Org cache read: LGTM.Tenant-scoped org lookup with orgCode is consistent with other services.
src/services/user-role.js (3)
17-17: Cache helper import: OK.
245-246: Constant rename usage is correct.
117-124: Confirm updateRole return shape.exports.updateRole uses UserRole.update(..., returning: true) in src/database/queries/user-role.js — Sequelize returns [count, rows] only for dialects that support RETURNING (Postgres). If your DB/dialect doesn't return rows, updateRole[0] will be undefined even when updateCount > 0. Guard the access or fetch the updated record (e.g., findOne after update) in src/services/user-role.js before reading updateRole[0].
src/services/organization.js (5)
22-22: Cache helper import: OK.
522-543: Good normalization and early return in addRelatedOrg().
545-549: Tenant-scoped existence check: OK.
590-617: Parallel cache invalidation with diagnostics: LGTM.
691-717: Parallel invalidations + diagnostics: LGTM.src/constants/common.js (1)
104-104: Rename verified — no stale references remain.
No occurrences of ENGLISH_LANGUGE_CODE; current references use ENGLISH_LANGUAGE_CODE (seen in src/constants/common.js, src/services/user.js, src/services/user-role.js).
…cache invalidation for user organization deletions
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/services/account.js (1)
1536-1536: OTP cleanup uses wrong key; delete by username to match where it was stored.
For forget-password, OTP is stored under user.username, not encryptedIdentifier.Apply:
- await utilsHelper.redisDel(encryptedIdentifier) + await utilsHelper.redisDel(user.username)src/database/queries/userOrganization.js (3)
15-18: Prevent accidental deletion when oldOrgCode === newOrgCodeIf oldOrgCode equals newOrgCode, this flow upserts the same record and then deletes it, also wiping roles. Short-circuit early.
Apply this diff:
exports.changeUserOrganization = async ({ userId, tenantCode, oldOrgCode, newOrgCode }) => { - const transaction = await sequelize.transaction() + if (oldOrgCode === newOrgCode) { + return { + success: true, + message: `User ${userId} already in organization ${newOrgCode}; no changes applied`, + } + } + const transaction = await sequelize.transaction()
31-37: Do not set created_at in upsertUpserting an existing row will overwrite created_at, breaking auditability.
Apply this diff:
{ user_id: userId, tenant_code: tenantCode, organization_code: newOrgCode, - created_at: new Date(), updated_at: new Date(), },
134-140: Expose rowsAffected from update (detect no-op updates)Destructure the affected count from UserOrganization.update and return a success flag based on rowsAffected; rg shows mixed return shapes across query modules—align this file to the destructuring pattern.
- await UserOrganization.update(updates, { + const [rowsAffected] = await UserOrganization.update(updates, { where: filter, transaction, }) await transaction.commit() - return { success: true } + return { success: rowsAffected > 0, rowsAffected }
🧹 Nitpick comments (15)
src/services/user-role.js (6)
66-80: Update JSDoc: missing userOrganizationCode param.The method signature now includes userOrganizationCode; document it.
/** * Update a role by ID. @@ - * @param {number} userOrganizationId - ID of the organization. - * @param {string} tenantCode - Tenant code of the requestor. + * @param {number} userOrganizationId - ID of the organization. + * @param {string} userOrganizationCode - Code of the organization (used for cache invalidation). + * @param {string} tenantCode - Tenant code of the requestor. * @returns {Promise<Object>} - Updated role response. */
86-86: Use strict equality for the update count check.Prevents truthy/falsy surprises if the driver returns a string/BigInt.
- if (updateCount == 0) { + if (updateCount === 0) {
114-115: HTTP 201 for update is incorrect; use 200 OK.Update isn’t resource creation.
- statusCode: httpStatusCode.created, + statusCode: httpStatusCode.ok,
94-112: Parallelize cache invalidations; avoid await + .catch mix.Cuts latency and simplifies error handling.
- await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.organization.name, - }) - .catch((error) => { - console.error(error) - }) - await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.profile.name, - }) - .catch((error) => { - console.error(error) - }) + await Promise.all([ + cacheClient.invalidateOrgNamespaceVersion({ + tenantCode, + orgId: userOrganizationCode, + ns: common.CACHE_CONFIG.namespaces.organization.name, + }), + cacheClient.invalidateOrgNamespaceVersion({ + tenantCode, + orgId: userOrganizationCode, + ns: common.CACHE_CONFIG.namespaces.profile.name, + }), + ]).catch((error) => { + console.error('user-role.update: cache invalidation failed', { + tenantCode, + org: userOrganizationCode, + error, + }) + })
139-146: Update JSDoc: delete() now takes userOrganizationCode./** * Delete a role by ID. @@ - * @param {number} userOrganizationId - ID of the organization. - * @param {string} tenantCode - Tenant code of the requestor. + * @param {number} userOrganizationId - ID of the organization. + * @param {string} userOrganizationCode - Code of the organization (used for cache invalidation). + * @param {string} tenantCode - Tenant code of the requestor. * @returns {Promise<Object>} - Deletion result response. */
161-179: Parallelize delete() cache invalidations; add context to logs.- await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.organization.name, - }) - .catch((error) => { - console.error(error) - }) - await cacheClient - .invalidateOrgNamespaceVersion({ - tenantCode, - orgId: userOrganizationCode, - ns: common.CACHE_CONFIG.namespaces.profile.name, - }) - .catch((error) => { - console.error(error) - }) + await Promise.all([ + cacheClient.invalidateOrgNamespaceVersion({ + tenantCode, + orgId: userOrganizationCode, + ns: common.CACHE_CONFIG.namespaces.organization.name, + }), + cacheClient.invalidateOrgNamespaceVersion({ + tenantCode, + orgId: userOrganizationCode, + ns: common.CACHE_CONFIG.namespaces.profile.name, + }), + ]).catch((error) => { + console.error('user-role.delete: cache invalidation failed', { + tenantCode, + org: userOrganizationCode, + error, + }) + })src/services/account.js (2)
805-813: Admin-role pruning constant may be wrong.
Elsewhere you use ORG_ADMIN_ROLE; here you compare against ADMIN_ROLE. Unify to avoid failing to prune.Apply:
- if (org.roles) { - org.roles = org.roles.filter((r) => r.role?.title?.toLowerCase() !== common.ADMIN_ROLE) - } + if (org.roles) { + org.roles = org.roles.filter( + (r) => r.role?.title && r.role.title.toLowerCase() !== common.ORG_ADMIN_ROLE.toLowerCase() + ) + }
849-863: Harden cache keying for prunedEntities; avoid undefined org code.
Compute a primaryOrgCode and reuse for orgId and id.Apply:
- const prunedEntities = await cacheClient.getOrSet({ - tenantCode, - orgId: tokenPayload.data.organization_codes[0], - ns: common.CACHE_CONFIG.namespaces.entity_types.name, - id: `${tokenPayload.data.organization_codes[0]}:${modelName}`, + const primaryOrgCode = tokenPayload.data.organization_codes?.[0] || process.env.DEFAULT_ORGANISATION_CODE + const prunedEntities = await cacheClient.getOrSet({ + tenantCode, + orgId: primaryOrgCode, + ns: common.CACHE_CONFIG.namespaces.entity_types.name, + id: `${primaryOrgCode}:${modelName}`,src/database/queries/userOrganization.js (3)
149-155: Return affected rows from destroyHelps callers know if anything was deleted.
Apply this diff:
- await UserOrganization.destroy({ + const rowsAffected = await UserOrganization.destroy({ where: filter, ...options, }) - - return { success: true } + return { success: rowsAffected > 0, rowsAffected }
16-16: Prefer managed transactions to reduce error pathsUse sequelize.transaction(async (t) => { ... }) to avoid manual commit/rollback branching.
Also applies to: 80-82, 88-91
10-11: Use centralized logger; avoid console.error for consistency/PII controlSwitch to the project logger with structured context; sanitize inputs.
Also applies to: 102-103, 142-143, 156-156
src/helpers/userHelper.js (4)
16-16: Hashing comment contradicts implementation.Comment says “Changed to SHA-256” but code uses
md5Hash. Fix the comment or switch to SHA‑256.- email: `${utils.md5Hash(userId)}@deletedUser`, // Changed to SHA-256 + // Either adjust the comment or actually switch the hash: + email: `${utils.sha256Hash(String(userId))}@deletedUser`, // Uses SHA-256
115-116: Improve error context for cache invalidation.Add identifiers to aid debugging.
- } catch (err) { - console.error('Failed to delete user cache', err) - } + } catch (err) { + console.error('Failed to delete user cache', { userId, tenantCode: user.tenant_code, orgCount: orgCodes?.length }, err) + }
100-101: Avoid side effects outside the DB transaction.
removeUserSessions(userSessionIds)runs outside the enclosing transaction, risking partial teardown (e.g., cache/Redis cleared but DB rollback).
- Option A: refactor session teardown into a lower-level helper (per prior learning) that accepts
{ transaction }and run it withinexecuteInTransaction.- Option B: move session teardown after
commit()and make it idempotent with retries/backoff.
73-79: Keep transaction in options; filter out falsy org codes.findAll signature is findAll(filter = {}, options = {}), so passing transaction inside options is correct — ignore the suggested third-arg change.
Suggested change:
const orgCodes = [...new Set(orgRows.map(r => r.organization_code).filter(Boolean))];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/database/queries/userOrganization.js(1 hunks)src/helpers/userHelper.js(3 hunks)src/services/account.js(10 hunks)src/services/admin.js(3 hunks)src/services/organization.js(6 hunks)src/services/user-role.js(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/user-role.jssrc/services/organization.jssrc/services/account.jssrc/services/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/userOrganization.js
🧠 Learnings (8)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/user-role.jssrc/services/organization.jssrc/helpers/userHelper.jssrc/services/account.jssrc/database/queries/userOrganization.jssrc/services/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 the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/user-role.jssrc/services/organization.jssrc/helpers/userHelper.jssrc/services/account.jssrc/services/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 the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/services/organization.jssrc/helpers/userHelper.jssrc/services/account.jssrc/database/queries/userOrganization.js
📚 Learning: 2025-09-12T10:14:22.455Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/organization.js:831-867
Timestamp: 2025-09-12T10:14:22.455Z
Learning: In the ELEVATE-Project/user codebase, the related_orgs field is stored as an array in the database by default, so additional array validation checks are unnecessary when processing related organization data.
Applied to files:
src/services/organization.js
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: In ELEVATE-Project/user, avoid potential circular dependencies between src/helpers/userHelper.js and src/services/user-sessions by extracting shared session cleanup logic (finding active sessions, deleting Redis keys, ending sessions) into a dedicated utility (e.g., helpers/sessionCleanup.js) that both layers can consume.
Applied to files:
src/helpers/userHelper.js
📚 Learning: 2025-08-08T15:12:44.423Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:117-156
Timestamp: 2025-08-08T15:12:44.423Z
Learning: In ELEVATE-Project/user, prefer extracting shared session teardown (find active sessions, delete Redis keys, mark ended_at) into helpers/sessionCleanup.js so both helpers and services use it, preventing upward (helper→service) dependencies and avoiding circular imports.
Applied to files:
src/helpers/userHelper.js
📚 Learning: 2025-08-08T13:06:32.911Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#790
File: src/helpers/userHelper.js:10-10
Timestamp: 2025-08-08T13:06:32.911Z
Learning: When session management is shared across helpers and services in ELEVATE-Project/user, prefer extracting the primitives (query active sessions, delete Redis keys, mark ended) into a lower-level generics utility to avoid helper↔service circular dependencies and keep Redis key handling consistent.
Applied to files:
src/helpers/userHelper.js
📚 Learning: 2025-09-02T14:07:27.882Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#811
File: src/validators/v1/admin.js:38-39
Timestamp: 2025-09-02T14:07:27.882Z
Learning: In src/validators/v1/admin.js, the username field validation intentionally accepts either a regular username pattern (a-z0-9_- with 3-40 chars) OR an email format pattern. This dual validation approach is by design, not an oversight.
Applied to files:
src/services/account.js
🧬 Code graph analysis (6)
src/services/user-role.js (3)
src/services/organization.js (6)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)src/services/user.js (3)
cacheClient(28-28)userOrganizationCode(474-474)common(10-10)src/controllers/v1/user-role.js (1)
updateRole(60-66)
src/services/organization.js (2)
src/database/queries/organization.js (3)
organization(70-74)organization(241-241)t(7-7)src/controllers/v1/organization.js (1)
updatedOrg(95-100)
src/helpers/userHelper.js (3)
src/services/account.js (18)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)user(136-145)user(764-764)user(906-906)user(956-956)user(1109-1109)user(1262-1271)user(1443-1451)user(1791-1791)user(2014-2017)user(2129-2129)orgCodes(845-845)ns(2050-2050)common(16-16)src/services/admin.js (11)
cacheClient(40-40)require(11-11)require(27-27)require(34-34)require(36-36)require(37-37)user(55-55)user(201-204)user(339-339)user(450-450)common(15-15)src/database/queries/userOrganization.js (1)
require(3-3)
src/services/account.js (4)
src/services/organization.js (9)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)tenantCode(272-272)cacheKey(135-135)cacheKey(243-243)src/services/public.js (10)
cacheClient(15-15)tenantDetail(37-42)tenantDetail(94-99)common(14-14)tenantDomain(32-32)tenantDomain(84-89)tenantQueries(2-2)prunedEntities(165-165)modelName(151-151)entityTypeQueries(8-8)src/services/user.js (18)
cacheClient(28-28)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)common(10-10)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/cacheHelper.js (1)
raw(118-118)
src/database/queries/userOrganization.js (3)
src/services/account.js (2)
options(1640-1646)options(2122-2122)src/services/organization.js (2)
options(275-277)options(318-320)src/services/user.js (2)
options(330-332)options(450-454)
src/services/admin.js (3)
src/services/account.js (14)
cacheClient(42-42)cacheKey(2051-2056)user(136-145)user(764-764)user(906-906)user(956-956)user(1109-1109)user(1262-1271)user(1443-1451)user(1791-1791)user(2014-2017)user(2129-2129)ns(2050-2050)organizationCode(194-194)src/services/organization.js (8)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)cacheKey(135-135)cacheKey(243-243)results(609-609)results(710-710)i(307-307)src/services/user.js (9)
cacheClient(28-28)cacheKey(147-152)cacheKey(588-593)user(50-53)user(335-335)user(458-458)ns(146-146)ns(310-310)ns(587-587)
🔇 Additional comments (35)
src/services/admin.js (4)
40-40: LGTM!The cacheClient module is properly imported and follows the existing pattern for requiring generics.
604-609: LGTM! Good cache invalidation pattern.The code properly invalidates the organization namespace cache after updating org admin roles.
610-616: LGTM! Proper cache key construction.The namespaced cache key is correctly constructed using the tenant code, organization ID, and user ID for the profile namespace.
703-718: Critical:orgIdis undefined in error logging at Line 715.The error handler references
orgIdwhich is not defined in scope. UseorganizationCodeinstead for proper error logging.await Promise.allSettled( namespaces.map((ns) => cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns }) ) ).then((results) => results.forEach((r, i) => { if (r.status === 'rejected') { - console.error(`invalidate failed for ns=${namespaces[i]} org=${orgId}`, r.reason) + console.error(`invalidate failed for ns=${namespaces[i]} org=${organizationCode}`, r.reason) } }) )src/services/organization.js (8)
22-22: LGTM!The cacheClient module is properly imported following the existing pattern.
181-181: LGTM! Method signature properly extended for tenant-aware operations.The update method now accepts tenantCode parameter, enabling proper cache invalidation.
196-213: Good cache invalidation implementation with proper error handling.The code correctly invalidates both organization and profile namespace caches after updates, with individual error handling that doesn't block the main flow. Each invalidation is properly isolated with its own catch block.
522-559: Robust tenant-scoped validation for related organizations.The code properly:
- Validates the parent org exists within the tenant
- Normalizes input and removes self-references
- Verifies all related orgs exist within the same tenant
- Returns meaningful errors for missing organizations
590-616: Excellent parallel cache invalidation with detailed error logging.The implementation efficiently invalidates caches for all affected organizations using Promise.allSettled, ensuring that failures don't block other invalidations. The error logging includes all necessary context (org, tenant, namespace) for debugging.
637-672: Well-structured removeRelatedOrg with proper tenant validation.The implementation correctly:
- Validates the organization exists in the tenant
- Checks that related_orgs is not empty before removal
- Verifies all orgs to remove exist in the same tenant
- Returns descriptive error messages
692-716: LGTM! Consistent cache invalidation pattern with removeRelatedOrg.The cache invalidation follows the same robust pattern as addRelatedOrg, with proper error handling and detailed logging.
675-675: Consider normalizing types before _.difference to avoid mismatches.The
relatedOrgsparameter might contain strings whileorgDetailsBeforeUpdate.related_orgscontains numbers, potentially causing incorrect difference calculations.+const normalizedRelatedOrgs = relatedOrgs.map(Number).filter(n => !isNaN(n)) -const newRelated = _.difference(orgDetailsBeforeUpdate.related_orgs, relatedOrgs) +const newRelated = _.difference(orgDetailsBeforeUpdate.related_orgs, normalizedRelatedOrgs)src/services/user-role.js (4)
17-17: LGTM: switched to cacheClient.Import looks correct and consistent with the PR’s direction.
161-166: Thanks for fixing the duplicate orgId key noted earlier.No duplicate object keys now; invalidation payload looks correct.
244-244: ENGLISH_LANGUAGE_CODE exists and is exported — no change required.Defined at src/constants/common.js:104; referenced in src/services/user.js:353 and src/services/user-role.js:244 — no runtime undefined risk.
85-85: Resolved — updateRole uses returning: true; updateRole[0] is populated on Postgressrc/database/queries/user-role.js calls UserRole.update(..., { returning: true }), so the destructure [updateCount, updateRole] is valid; add raw: true if you need plain objects or fetch the record post-update to support dialects that don't honor RETURNING.
src/services/account.js (13)
42-42: cacheClient import — LGTM.
Consistent with PR objective to migrate off direct Redis usage.
645-688: Login JSDoc — LGTM.
Clear steps; matches intent.
691-697: Shared not-found helper — LGTM.
Keeps responses consistent.
698-706: Tenant-domain cache lookup — LGTM.
Appropriate use of cacheClient.getOrSet.
711-720: Tenant detail cache lookup — LGTM.
Scopes by tenant code correctly.
725-733: Username regex rejects hyphens; inconsistent with other flows.
Allow hyphens to match generateOtp/resetPassword and validator behavior.Apply:
- const USERNAME_RE = /^[a-zA-Z0-9_]{3,30}$/ + const USERNAME_RE = /^[a-zA-Z0-9_-]{3,30}$/
762-765: Avoid passing raw=true to findUserWithOrganization here.
You already guard toJSON; prefer consistent non-raw objects (prevents surprises downstream).Apply:
- const userInstance = await userQueries.findUserWithOrganization(query, {}, true) + const userInstance = await userQueries.findUserWithOrganization(query, {})
814-827: Token payload build — LGTM.
Includes org ids/codes and session id as expected.
845-847: Guard against empty organizations.
If a user has no orgs, push default may still leave organization_codes[0] undefined later.Do we guarantee at least one org post-transform?
871-881: Session store update — LGTM.
Matches existing flow.
2050-2057: Password-change: profile cache invalidation — LGTM.
Correct namespace usage and key composition with userId.
1099-1110: Third arg to findUserWithOrganization israw; tenant_code is being misused.
Passing tenant_code as arg3 flips to raw mode and can alter behavior inconsistently.Apply:
- const user = await userQueries.findUserWithOrganization(query, {}, tenantDomain.tenant_code) + const userInstance = await userQueries.findUserWithOrganization(query, {}) + const user = userInstance ? (userInstance.toJSON ? userInstance.toJSON() : userInstance) : null
751-758: Phone login: require phone_code or omit from query.
Pushing { phone_code: undefined } will likely miss matches.Apply (option A: require):
- } else if (isPhoneFormat(identifier)) { - query[Op.or].push({ - phone: emailEncryption.encrypt(identifier), - phone_code: bodyData.phone_code, - }) + } else if (isPhoneFormat(identifier)) { + if (!bodyData.phone_code) { + return responses.failureResponse({ + message: 'PHONE_CODE_REQUIRED', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + query[Op.or].push({ + phone: emailEncryption.encrypt(identifier), + phone_code: bodyData.phone_code, + })(Option B: conditionally include phone_code on the query if present.)
src/database/queries/userOrganization.js (2)
107-129: Ensure DB indexes for tenant joins & make Organization include optional (src/database/queries/userOrganization.js:107-129)Confirm/add composite indexes and avoid unintended inner-joins:
- Ensure composite indexes exist: user_organizations — UNIQUE(tenant_code, user_id, organization_code) plus indexes (tenant_code, user_id) and (tenant_code, organization_code); user_organization_roles — (tenant_code, user_id, organization_code, role_id); organizations — (tenant_code, code).
- If the Organization include is optional, set required: false on the include to avoid an inner join.
- Repo search returned no index declarations — verify migrations/models and the DB schema. Example search: rg -nP --hidden -S "addIndex|indexes:|unique:\s*true|uniqueKeys".
109-124: Do not mutate caller options; guard organizationAttributes; preserve flat raw shape (do NOT add nest: true)Callers rely on flattened keys (example: src/services/organization.js reads userOrgs.map(o => o['organization.id']). Keep raw: true. Apply non-mutating guard and only add include.where when tenant_code exists.
- if (options?.organizationAttributes?.length > 0) { - options.include = [ - { - model: Organization, - attributes: options.organizationAttributes, - as: 'organization', - where: { tenant_code: filter.tenant_code }, - }, - ] - delete options.organizationAttributes - } - return await UserOrganization.findAll({ - where: filter, - ...options, - raw: true, - }) + const { organizationAttributes, ...restOptions } = options || {} + const include = + Array.isArray(organizationAttributes) && organizationAttributes.length > 0 + ? [ + { + model: Organization, + attributes: organizationAttributes, + as: 'organization', + ...(filter.tenant_code ? { where: { tenant_code: filter.tenant_code } } : {}), + }, + ] + : undefined + return await UserOrganization.findAll({ + where: filter, + ...(include ? { include } : {}), + ...restOptions, + raw: true, + })src/helpers/userHelper.js (4)
2-2: LGTM: moved to cacheClient.Import of cache helper aligns with the PR’s cache strategy.
102-116: Cache invalidation: add tenant-only fallback and delete in parallel.If a user has no orgs, the current loop skips invalidation entirely. Add a tenant-only key path; also parallelize deletes.
- const ns = common.CACHE_CONFIG.namespaces.profile.name - for (const orgId of orgCodes) { - const fullKey = await cacheClient.versionedKey({ - tenantCode: user.tenant_code, - orgId, - ns, - id: userId, - }) - await cacheClient.del(fullKey) - } + const ns = common.CACHE_CONFIG.namespaces.profile.name + const targets = (orgCodes && orgCodes.length) ? orgCodes : [undefined] // tenant-only fallback + await Promise.all( + targets.map(async (orgId) => { + const fullKey = await cacheClient.versionedKey({ + tenantCode: user.tenant_code, + orgId, // undefined => tenant-scoped key + ns, + id: userId, + }) + return cacheClient.del(fullKey) + }) + )
89-101: Incorrect — findUserSession returns an array; mapping is valid.JSDoc and implementation in src/services/user-sessions.js (lines 227–229) show findUserSession returns Promise<Object[]> via userSessionsQueries.findAll, so using userSessionData.map(...) is correct.
Likely an incorrect or invalid review comment.
164-170: Migrate Redis deletes to cacheClient and confirm DB type for ended_at
- Multiple direct Redis deletes remain — replace with cacheClient.versionedKey(...)/cacheClient.del(key) or update the central wrapper (src/generics/utils.js::redisDel) so callers no longer hit Redis directly. Locations found: src/helpers/userHelper.js:165, src/services/user-sessions.js:196, src/services/userInvite.js:392, src/helpers/userInvite.js:955, src/services/admin.js:594, src/services/org-admin.js:657, src/services/account.js:545, 546, 1536, 1769, 2060, src/generics/utils.js:116.
- No matches for sessions namespace in common.CACHE_CONFIG.namespaces — confirm the exact namespace key (e.g., common.CACHE_CONFIG.namespaces.sessions.name) before constructing versioned keys.
- ended_at is currently set as epoch seconds — set to a DB TIMESTAMP (e.g., new Date()) or the exact type your DB expects when updating rows.
… cache management
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/services/public.js (1)
37-42: Minor: make fetchFn consistent and drop no-op options arg.Use the same single-arg pattern used elsewhere and avoid passing an unused
{}.- fetchFn: () => tenantQueries.findOne({ code }, {}), + fetchFn: () => tenantQueries.findOne({ code }),src/generics/cacheHelper.js (4)
175-191: Clarify base key for global entries; avoid “tenant:null”.buildVersionedKey/namespacedKey use tenantKey even when tenantCode is null, producing “tenant:null:…”. Prefer an explicit global prefix to improve readability and eviction targeting.
+function globalKey(parts = []) { + return ['global', ...parts].join(':') +} @@ - const base = orgId ? orgKey(tenantCode, orgId, []) : tenantKey(tenantCode, []) + const base = tenantCode != null + ? (orgId ? orgKey(tenantCode, orgId, []) : tenantKey(tenantCode, [])) + : globalKey(orgId ? ['org', orgId] : []) @@ - const base = tenantKey(tenantCode, []) + const base = tenantCode != null ? tenantKey(tenantCode, []) : globalKey([])Update namespacedKey similarly to keep prefixes consistent.
324-349: Chunk UNLINK/DEL calls to avoid large-arity issues.Spreading hundreds/thousands of keys into a single call can hit engine arg limits or cause big event-loop stalls. Chunk deletions.
- if (keys.length) { - // try unlink as best-effort - try { - if (unlink && typeof redis.unlink === 'function') await redis.unlink(...keys) - else await redis.del(...keys) - } catch (e) { - // fallback to individual deletes - for (const k of keys) { - try { - if (unlink && typeof redis.unlink === 'function') await redis.unlink(k) - else await redis.del(k) - } catch (__) {} - } - } - } + if (keys.length) { + const CHUNK = 512 + for (let i = 0; i < keys.length; i += CHUNK) { + const slice = keys.slice(i, i + CHUNK) + try { + if (unlink && typeof redis.unlink === 'function') await redis.unlink(...slice) + else await redis.del(...slice) + } catch (e) { + for (const k of slice) { + try { + if (unlink && typeof redis.unlink === 'function') await redis.unlink(k) + else await redis.del(k) + } catch (_) {} + } + } + } + }
17-25: Nits: unused config items.SHARD_RETENTION_DAYS and shardOf() are defined but unused. Remove or wire them up to avoid dead code.
-const SHARD_RETENTION_DAYS = toInt(CACHE_CONFIG.shardRetentionDays, 7) ... -function shardOf(key) { - const h = md5(key) - const asInt = parseInt(h.slice(0, 8), 16) - return (asInt >>> 0) % SHARDS -}
233-245: Consider deleting even when cache is disabled.get/set guard on ENABLE_CACHE, del doesn’t. This asymmetry is fine if intentional (cleanup regardless of toggle); otherwise, add the same guard for consistency.
async function del(key, { useInternal = false } = {}) { + if (!ENABLE_CACHE) return try { await RedisCache.deleteKey(key)Confirm intended behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/generics/cacheHelper.js(1 hunks)src/services/public.js(3 hunks)src/services/tenant.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/tenant.js
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/public.js
🧬 Code graph analysis (1)
src/services/public.js (3)
src/services/tenant.js (4)
cacheClient(33-33)require(32-32)common(10-10)tenantQueries(11-11)src/services/account.js (17)
cacheClient(42-42)require(26-26)require(27-27)require(37-37)require(41-41)tenantDetail(74-77)tenantDetail(713-719)tenantDetail(1064-1066)tenantDetail(1196-1199)tenantDetail(1394-1396)common(16-16)tenantQueries(34-34)tenantDomain(69-69)tenantDomain(699-705)tenantDomain(1058-1058)tenantDomain(1191-1191)tenantDomain(1388-1388)src/generics/cacheHelper.js (2)
require(3-3)common(5-5)
🔇 Additional comments (4)
src/services/public.js (4)
15-15: LGTM: switched to cacheClient.Import path matches other services (e.g., tenant.js). No issues.
49-59: LGTM: org-scoped caching with tenant scoping.Key is properly tenant- and org-scoped; query filter includes tenant_code.
62-62: LGTM: 200 OK is correct for a read.Returning httpStatusCode.ok for GET aligns with semantics.
94-99: Bug: undefined variable “code” used for cache key.Will throw ReferenceError and create wrong cache scope. Use tenantDomain.tenant_code for tenantCode and id.
- const tenantDetail = await cacheClient.getOrSet({ - tenantCode: code, // ensures key is tenant-scoped - ns: common.CACHE_CONFIG.namespaces.tenant.name, - id: code, // unique per tenant - fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }, {}), - }) + const tenantDetail = await cacheClient.getOrSet({ + tenantCode: tenantDomain.tenant_code, // ensures key is tenant-scoped + ns: common.CACHE_CONFIG.namespaces.tenant.name, + id: tenantDomain.tenant_code, // unique per tenant + fetchFn: () => tenantQueries.findOne({ code: tenantDomain.tenant_code }), + })Run to ensure no other occurrences remain:
#!/bin/bash rg -nC2 -e $'tenantCode:\\s*code\\b' -e $'id:\\s*code\\b' src/services/public.js
…ity types and profiles
…er organization ID
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/account.js (1)
1436-1444: Reset password: avoid raw=true to keep instance shape stableRaw results risk shape mismatches before transform; return a model instance.
- let user = await userQueries.findUserWithOrganization( + let user = await userQueries.findUserWithOrganization( query, { attributes: { exclude: projection, }, }, - true + /* raw */ false ) + // if needed: + user = user ? (user.toJSON ? user.toJSON() : user) : nullsrc/services/entityType.js (1)
100-108: Wrong error mapping for UniqueConstraintError.“Already deleted” is misleading for a uniqueness violation.
- if (error instanceof UniqueConstraintError) { - return responses.failureResponse({ - message: 'ENTITY_TYPE_ALREADY_DELETED', + if (error instanceof UniqueConstraintError) { + return responses.failureResponse({ + message: 'ENTITY_TYPE_ALREADY_EXISTS', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) }
🧹 Nitpick comments (8)
src/services/account.js (2)
798-805: Admin-role pruning: confirm correct constant(s)Code compares to common.ADMIN_ROLE (lowercased title). Elsewhere the code uses common.ORG_ADMIN_ROLE. Safer to support both, case-insensitively.
- if (org.roles) { - org.roles = org.roles.filter((r) => r.role?.title?.toLowerCase() !== common.ADMIN_ROLE) - } + if (org.roles) { + const adminTitles = [common.ORG_ADMIN_ROLE, common.ADMIN_ROLE] + .filter(Boolean) + .map((t) => t.toLowerCase()) + org.roles = org.roles.filter((r) => !adminTitles.includes(r.role?.title?.toLowerCase())) + }
838-847: Guard cache keying when user has no orgs; fall back to default orgIf user.organizations is empty, orgId/id become undefined and cache keys degrade; prefer default org code.
- const prunedEntities = await cacheClient.getOrSet({ - tenantCode, - orgId: tokenPayload.data.organization_codes[0], - ns: common.CACHE_CONFIG.namespaces.entity_types.name, - id: `${tokenPayload.data.organization_codes[0]}:${modelName}`, + const primaryOrgCode = + user.organizations?.[0]?.code || process.env.DEFAULT_ORGANISATION_CODE + const preferredOrgId = user.organizations?.[0]?.id ?? null + const prunedEntities = await cacheClient.getOrSet({ + tenantCode, + orgId: primaryOrgCode, + ns: common.CACHE_CONFIG.namespaces.entity_types.name, + id: `${primaryOrgCode}:${modelName}`, fetchFn: async () => { const raw = await entityTypeQueries.findUserEntityTypesAndEntities({ status: 'ACTIVE', organization_code: { [Op.in]: orgCodes }, tenant_code: tenantDetail.code, model_names: { [Op.contains]: [modelName] }, }) - return removeDefaultOrgEntityTypes(raw, user.organizations[0].id) + return removeDefaultOrgEntityTypes(raw, preferredOrgId) }, })src/services/entities.js (2)
10-36: Parallelize and harden cache invalidation (lower latency, safer no-ops).
- Add param guard to avoid broad evictions on missing inputs.
- Run invalidations/evictions in parallel.
- Include context in error logs.
static async _invalidateEntityCaches({ tenantCode, organizationCode }) { - try { - await cacheClient.invalidateOrgNamespaceVersion({ - tenantCode, - orgId: organizationCode, - ns: common.CACHE_CONFIG.namespaces.entity_types.name, - }) - - await cacheClient.invalidateOrgNamespaceVersion({ - tenantCode, - orgId: organizationCode, - ns: common.CACHE_CONFIG.namespaces.profile.name, - }) - - if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { - await cacheClient.evictTenantByPattern(tenantCode, { - patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.entity_types.name}:*`, - }) - await cacheClient.evictTenantByPattern(tenantCode, { - patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.profile.name}:*`, - }) - } - } catch (err) { - console.error('Entity cache invalidation failed', err) - // Do not throw. Cache failures should not block DB ops. - } + if (!tenantCode || !organizationCode) { + console.warn('Skip entity cache invalidation: missing identifiers', { tenantCode, organizationCode }) + return + } + const { entity_types, profile } = common.CACHE_CONFIG.namespaces + try { + await Promise.all([ + cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns: entity_types.name }), + cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns: profile.name }), + ]) + if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { + await Promise.all([ + cacheClient.evictTenantByPattern(tenantCode, { patternSuffix: `org:*:${entity_types.name}:*` }), + cacheClient.evictTenantByPattern(tenantCode, { patternSuffix: `org:*:${profile.name}:*` }), + ]) + } + } catch (err) { + console.error('Entity cache invalidation failed', { tenantCode, organizationCode, err }) + // Do not throw. Cache failures should not block DB ops. + } }
55-57: Don’t await cache invalidation on the hot path.Fire-and-forget to avoid adding response latency; failures are already swallowed inside the method.
- await this._invalidateEntityCaches({ tenantCode, organizationCode }) + void this._invalidateEntityCaches({ tenantCode, organizationCode })(repeat for Lines 116 and 198)
Also applies to: 115-117, 197-199
src/services/entityType.js (4)
13-39: Parallelize and harden cache invalidation (same as entities).Guard inputs, parallelize invalidations/evictions, and include context in logs.
static async _invalidateEntityTypeCaches({ tenantCode, organizationCode }) { - try { - await cacheClient.invalidateOrgNamespaceVersion({ - tenantCode, - orgId: organizationCode, - ns: common.CACHE_CONFIG.namespaces.entity_types.name, - }) - - await cacheClient.invalidateOrgNamespaceVersion({ - tenantCode, - orgId: organizationCode, - ns: common.CACHE_CONFIG.namespaces.profile.name, - }) - - if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { - await cacheClient.evictTenantByPattern(tenantCode, { - patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.entity_types.name}:*`, - }) - await cacheClient.evictTenantByPattern(tenantCode, { - patternSuffix: `org:*:${common.CACHE_CONFIG.namespaces.profile.name}:*`, - }) - } - } catch (err) { - console.error('Entity type cache invalidation failed', err) - // Do not throw. Caching failure should not block main operation. - } + if (!tenantCode || !organizationCode) { + console.warn('Skip entity-type cache invalidation: missing identifiers', { tenantCode, organizationCode }) + return + } + const { entity_types, profile } = common.CACHE_CONFIG.namespaces + try { + await Promise.all([ + cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns: entity_types.name }), + cacheClient.invalidateOrgNamespaceVersion({ tenantCode, orgId: organizationCode, ns: profile.name }), + ]) + if (process.env.DEFAULT_ORGANISATION_CODE === organizationCode) { + await Promise.all([ + cacheClient.evictTenantByPattern(tenantCode, { patternSuffix: `org:*:${entity_types.name}:*` }), + cacheClient.evictTenantByPattern(tenantCode, { patternSuffix: `org:*:${profile.name}:*` }), + ]) + } + } catch (err) { + console.error('Entity type cache invalidation failed', { tenantCode, organizationCode, err }) + // Do not throw. Caching failure should not block main operation. + } }
51-51: Avoid awaiting cache invalidation after create.Reduce latency; keep the operation non-blocking.
- await this._invalidateEntityTypeCaches({ tenantCode, organizationCode }) + void this._invalidateEntityTypeCaches({ tenantCode, organizationCode })
71-71: Replace comma-expression hack with clear assignments.Improves readability; avoids accidental precedence issues.
- ;(bodyData.updated_by = loggedInUserId), (bodyData.organization_code = organizationCode) + bodyData.updated_by = loggedInUserId + bodyData.organization_code = organizationCode
196-209: Apply fire-and-forget cache invalidation; DAO signature confirmed.deleteOneEntityType(id, organizationCode) is defined at src/database/queries/entityType.js:109 — tenantCode is not required by the DAO (tenantCode is still used here for cache invalidation in the service). Replace awaiting the cache invalidation to avoid adding latency:
- await this._invalidateEntityTypeCaches({ tenantCode, organizationCode }) + void this._invalidateEntityTypeCaches({ tenantCode, organizationCode })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/services/account.js(10 hunks)src/services/entities.js(4 hunks)src/services/entityType.js(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/entities.jssrc/services/account.jssrc/services/entityType.js
🧠 Learnings (4)
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/account.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 the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/account.js
📚 Learning: 2025-09-02T14:07:27.882Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#811
File: src/validators/v1/admin.js:38-39
Timestamp: 2025-09-02T14:07:27.882Z
Learning: In src/validators/v1/admin.js, the username field validation intentionally accepts either a regular username pattern (a-z0-9_- with 3-40 chars) OR an email format pattern. This dual validation approach is by design, not an oversight.
Applied to files:
src/services/account.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 the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/services/account.js
🧬 Code graph analysis (3)
src/services/entities.js (2)
src/services/entityType.js (5)
common(4-4)require(6-6)require(7-7)require(8-8)cacheClient(10-10)src/services/organization.js (6)
common(2-2)common(597-597)common(698-698)cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)
src/services/account.js (6)
src/services/entityType.js (5)
cacheClient(10-10)prunedEntities(130-137)entityTypeQueries(5-5)ns(115-115)ns(160-160)src/services/public.js (9)
cacheClient(15-15)tenantDetail(37-42)tenantDetail(94-99)common(14-14)tenantQueries(2-2)prunedEntities(165-165)modelName(151-151)entityTypeQueries(8-8)utils(10-10)src/services/organization.js (10)
cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)common(2-2)common(597-597)common(698-698)tenantCode(272-272)utils(5-5)cacheKey(135-135)cacheKey(243-243)src/services/user.js (19)
cacheClient(28-28)user(50-53)user(335-335)user(458-458)user(508-508)user(543-543)common(10-10)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)modelName(363-363)entityTypeQueries(15-15)utils(12-12)ns(146-146)ns(310-310)ns(587-587)cacheKey(147-152)cacheKey(588-593)src/generics/utils.js (2)
common(20-20)removeDefaultOrgEntityTypes(835-842)src/generics/cacheHelper.js (1)
raw(118-118)
src/services/entityType.js (6)
src/services/account.js (20)
require(26-26)require(27-27)require(37-37)require(41-41)common(16-16)entityTypeQueries(24-24)responses(30-30)cacheClient(42-42)organizationCode(194-194)tenantCode(705-705)ns(2043-2043)prunedEntities(366-366)prunedEntities(842-856)prunedEntities(1549-1549)result(568-568)result(866-866)result(1572-1572)result(1677-1677)result(1803-1803)result(2083-2083)src/services/entities.js (9)
require(4-4)common(6-6)responses(5-5)cacheClient(7-7)attributes(228-228)entities(159-159)entities(229-229)filter(144-144)filter(223-223)src/services/organization.js (10)
common(2-2)common(597-597)common(698-698)responses(20-20)cacheClient(22-22)cacheClient(598-598)cacheClient(699-699)tenantCode(272-272)filter(430-430)filter(498-501)src/services/user.js (24)
require(17-17)require(19-19)require(20-20)require(24-24)require(26-26)common(10-10)entityTypeQueries(15-15)responses(22-22)cacheClient(28-28)ns(146-146)ns(310-310)ns(587-587)attributes(252-252)attributes(278-278)prunedEntities(74-74)prunedEntities(365-379)prunedEntities(484-484)prunedEntities(562-562)filter(65-72)filter(242-245)filter(277-277)filter(308-308)filter(445-445)filter(554-560)src/generics/utils.js (6)
require(12-12)require(16-16)require(21-21)common(20-20)removeDefaultOrgEntityTypes(835-842)result(402-402)src/database/queries/entityType.js (3)
entities(141-145)result(53-59)result(147-153)
🔇 Additional comments (16)
src/services/account.js (14)
42-42: Introduce cacheClient — good additionImport looks correct and aligns with migration to versioned cache helper.
645-688: Docs update acknowledgedComment‑only change; no functional impact.
691-697: Helper for not-found responses — LGTMKeeps responses consistent.
704-713: Ensure tenant is ACTIVE in cache fetchIf inactive tenants must be blocked from login, filter in fetchFn.
- fetchFn: async () => tenantQueries.findOne({ code: tenantCode }), + fetchFn: async () => + tenantQueries.findOne({ code: tenantCode, status: common.ACTIVE_STATUS }),
736-743: Base query construction — LGTMCovers password existence, active status, and tenant scoping.
807-820: Token payload construction — LGTMIDs/codes coerced to strings; consistent with other services.
864-866: Preserving original identifier — LGTMUseful for UI feedback/debug; no persistence impact.
868-873: Session store update — LGTMMatches existing pattern of writing access/refresh to Redis and DB.
1102-1103: Third param to findUserWithOrganization is raw flag; tenant_code passed by mistakePassing tenantDomain.tenant_code as 3rd arg flips raw mode and may change shape. Keep tenant_code in the filter (already present) and drop the boolean.
- const user = await userQueries.findUserWithOrganization(query, {}, tenantDomain.tenant_code) + const user = await userQueries.findUserWithOrganization(query, {})
2043-2051: Profile cache key invalidation — LGTM; confirm namespace semanticsKeying by tenantCode + organizationCode + profile ns + userId matches cacheHelper contract. Minor: right below, Line 2053 calls utils.redisDel; elsewhere this module uses utilsHelper.redisDel — verify the correct helper.
718-726: Username regex rejects hyphens; other flows accept themAlign with generateOtp/resetPassword and validator learnings; allow “-”.
- const USERNAME_RE = /^[a-zA-Z0-9_]{3,30}$/ + const USERNAME_RE = /^[a-zA-Z0-9_-]{3,30}$/
744-751: Phone login should validate phone_code or omit it from the queryIncluding undefined phone_code will likely break matching; enforce presence or build conditionally.
- } else if (isPhoneFormat(identifier)) { - query[Op.or].push({ - phone: emailEncryption.encrypt(identifier), - phone_code: bodyData.phone_code, - }) + } else if (isPhoneFormat(identifier)) { + if (!bodyData.phone_code) { + return responses.failureResponse({ + message: 'INVALID_IDENTIFIER_FORMAT', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + query[Op.or].push({ + phone: emailEncryption.encrypt(identifier), + phone_code: bodyData.phone_code, + })
755-758: Avoid raw=true; it changes shape and risks downstream breakagefindUserWithOrganization(..., {}, true) returns a raw object; remove the boolean to get a model instance consistently.
- const userInstance = await userQueries.findUserWithOrganization(query, {}, true) - let user = userInstance ? (userInstance.toJSON ? userInstance.toJSON() : userInstance) : null + const userInstance = await userQueries.findUserWithOrganization(query, {}) + const user = userInstance ? userInstance.toJSON() : null
561-565: Cache invalidation likely no‑op: use organization code, not numeric IDNamespace orgId across the codebase is the organization code (string). Passing user.organization_id (number) won’t bump the right version; stale org caches will persist.
- cacheClient.invalidateOrgNamespaceVersion({ - tenantCode: tenantDetail.code, - orgId: user.organization_id, - ns: common.CACHE_CONFIG.namespaces.organization.name, - }) + cacheClient.invalidateOrgNamespaceVersion({ + tenantCode: tenantDetail.code, + orgId: organization.code, + ns: common.CACHE_CONFIG.namespaces.organization.name, + })src/services/entityType.js (1)
158-158: Ensure callers pass organizationId; default '' degrades pruning.This was raised earlier; please verify all call sites provide the 4th arg.
#!/bin/bash # Find and inspect call sites to confirm the 4th argument is provided. rg -nP -C2 '\breadUserEntityTypes\s*\(' --type=js --type=tssrc/services/entities.js (1)
24-31: Eviction pattern is correct — no change required.cacheHelper builds org keys as tenant::org:::... (version token is between ns and id); evictTenantByPattern prepends tenant:: to the supplied suffix, so patternSuffix "org:::" becomes "tenant::org:::" and will match org-level and versioned namespaced keys.
| const ns = common.CACHE_CONFIG.namespaces.entity_types.name | ||
| const cacheId = `all` // stable id under namespace; versioning handles invalidation | ||
| const fetchFn = async () => { | ||
| const attributes = ['value', 'label', 'id', 'organization_code'] | ||
| const entities = await entityTypeQueries.findAllEntityTypes( | ||
| [organizationCode, process.env.DEFAULT_ORGANISATION_CODE], | ||
| attributes, | ||
| { | ||
| tenant_code: tenantCode, | ||
| } | ||
| ) | ||
| const pruned = removeDefaultOrgEntityTypes(entities, organizationId) | ||
| return pruned |
There was a problem hiding this comment.
Pruning will fail without organization_id in attributes.
removeDefaultOrgEntityTypes compares organization_id; it’ll never prefer org-specific rows if the id is missing.
- const attributes = ['value', 'label', 'id', 'organization_code']
+ const attributes = ['value', 'label', 'id', 'organization_code', 'organization_id']📝 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 ns = common.CACHE_CONFIG.namespaces.entity_types.name | |
| const cacheId = `all` // stable id under namespace; versioning handles invalidation | |
| const fetchFn = async () => { | |
| const attributes = ['value', 'label', 'id', 'organization_code'] | |
| const entities = await entityTypeQueries.findAllEntityTypes( | |
| [organizationCode, process.env.DEFAULT_ORGANISATION_CODE], | |
| attributes, | |
| { | |
| tenant_code: tenantCode, | |
| } | |
| ) | |
| const pruned = removeDefaultOrgEntityTypes(entities, organizationId) | |
| return pruned | |
| const ns = common.CACHE_CONFIG.namespaces.entity_types.name | |
| const cacheId = `all` // stable id under namespace; versioning handles invalidation | |
| const fetchFn = async () => { | |
| const attributes = ['value', 'label', 'id', 'organization_code', 'organization_id'] | |
| const entities = await entityTypeQueries.findAllEntityTypes( | |
| [organizationCode, process.env.DEFAULT_ORGANISATION_CODE], | |
| attributes, | |
| { | |
| tenant_code: tenantCode, | |
| } | |
| ) | |
| const pruned = removeDefaultOrgEntityTypes(entities, organizationId) | |
| return pruned |
🤖 Prompt for AI Agents
In src/services/entityType.js around lines 115 to 127, the attributes array
passed to findAllEntityTypes omits organization_id, so
removeDefaultOrgEntityTypes cannot detect org-specific rows and pruning fails;
add 'organization_id' to the attributes list (e.g., const attributes =
['value','label','id','organization_code','organization_id']) so the fetched
entities include the organization identifier required by the pruning function,
then keep the rest of the flow unchanged.
| const prunedEntities = await cacheClient.getOrSet({ | ||
| key: cacheId, | ||
| tenantCode, | ||
| orgId: organizationCode, | ||
| ns, | ||
| id: cacheId, | ||
| fetchFn, | ||
| }) |
There was a problem hiding this comment.
getOrSet API misuse: drop unsupported “key”.
Existing usage elsewhere passes “id” only; passing both risks runtime issues or duplicate keys.
- const prunedEntities = await cacheClient.getOrSet({
- key: cacheId,
+ const prunedEntities = await cacheClient.getOrSet({
tenantCode,
orgId: organizationCode,
ns,
id: cacheId,
fetchFn,
})📝 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 prunedEntities = await cacheClient.getOrSet({ | |
| key: cacheId, | |
| tenantCode, | |
| orgId: organizationCode, | |
| ns, | |
| id: cacheId, | |
| fetchFn, | |
| }) | |
| const prunedEntities = await cacheClient.getOrSet({ | |
| tenantCode, | |
| orgId: organizationCode, | |
| ns, | |
| id: cacheId, | |
| fetchFn, | |
| }) |
🤖 Prompt for AI Agents
In src/services/entityType.js around lines 130 to 137 the call to
cacheClient.getOrSet is passing both key and id which misuses the API (only id
is expected and other usages pass id alone); remove the unsupported key property
and call getOrSet with id: cacheId (and keep tenantCode, orgId:
organizationCode, ns, fetchFn) so the cache key generation remains consistent
and avoids duplicate/runtime issues.
| // Read user entity types by value (cached) | ||
| static async readUserEntityTypes(body, organizationCode, tenantCode, organizationId = '') { | ||
| try { | ||
| // Include tenant_code in filter for consistency with schema | ||
| const filter = { | ||
| value: body.value, | ||
| status: 'ACTIVE', | ||
| tenant_code: tenantCode, // Ensure tenant isolation | ||
| organization_code: { | ||
| [Op.in]: [process.env.DEFAULT_ORGANISATION_CODE, organizationCode], | ||
| }, | ||
| const ns = common.CACHE_CONFIG.namespaces.entity_types.name | ||
| const cacheId = `user:value:${body.value}` | ||
| const fetchFn = async () => { | ||
| const filter = { | ||
| value: body.value, | ||
| status: 'ACTIVE', | ||
| tenant_code: tenantCode, | ||
| organization_code: { | ||
| [Op.in]: [process.env.DEFAULT_ORGANISATION_CODE, organizationCode], | ||
| }, | ||
| } | ||
| const entities = await entityTypeQueries.findUserEntityTypesAndEntities(filter) | ||
| const pruned = removeDefaultOrgEntityTypes(entities, organizationId) | ||
| return { entity_types: pruned } | ||
| } |
There was a problem hiding this comment.
Validate input and sanitize cache key; also drop “key” here.
Guard missing value, encode key, and align getOrSet params.
static async readUserEntityTypes(body, organizationCode, tenantCode, organizationId = '') {
try {
+ if (!body || typeof body.value !== 'string' || body.value.length === 0) {
+ return responses.failureResponse({
+ message: 'INVALID_REQUEST',
+ statusCode: httpStatusCode.bad_request,
+ responseCode: 'CLIENT_ERROR',
+ })
+ }
const ns = common.CACHE_CONFIG.namespaces.entity_types.name
- const cacheId = `user:value:${body.value}`
+ const cacheId = `user:value:${encodeURIComponent(body.value)}`
const fetchFn = async () => {
const filter = {
value: body.value,
status: 'ACTIVE',
tenant_code: tenantCode,
organization_code: {
[Op.in]: [process.env.DEFAULT_ORGANISATION_CODE, organizationCode],
},
}
const entities = await entityTypeQueries.findUserEntityTypesAndEntities(filter)
const pruned = removeDefaultOrgEntityTypes(entities, organizationId)
return { entity_types: pruned }
}
- const result = await cacheClient.getOrSet({
- key: cacheId,
+ const result = await cacheClient.getOrSet({
tenantCode,
orgId: organizationCode,
ns,
id: cacheId,
fetchFn,
})Also applies to: 176-183
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Chores