Conversation
… codes, and introduce organization caching functionality. Updated session retrieval logic to ensure tenant code consistency and added caching methods for organization details.
…hancing request filtering for user sessions.
…into USER_DELETE_SELF_DELETE
…ries with tenant code. Updated AdminService to correctly handle parameters and removed erroneous JOINs, ensuring proper query execution and notification functionality.
WalkthroughMultiple database queries and services are updated to enforce explicit field clearing in user extension removal, add tenant code filtering to session retrieval, restructure user info handling in admin deletion flows, fix notification payload field naming, and remove remote dependency calls from default org ID retrieval. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/requests/user.js (2)
844-844:⚠️ Potential issue | 🟠 MajorPre-existing bug:
throw error(error)—erroris not a function.
erroris the caughtErrorobject, not a callable. This line throws aTypeError: error is not a functioninstead of the original error, masking the real failure ingetUserDetailedList.🐛 Proposed fix
- throw error(error) + throw error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/requests/user.js` at line 844, The catch in getUserDetailedList mistakenly calls error as a function (throw error(error)), which causes a TypeError; change it to rethrow the original Error object (throw error) or throw a new Error with the original as cause so the original stack/message is preserved; update the catch block in src/requests/user.js around getUserDetailedList to remove the callable invocation and rethrow the Error object or wrap it appropriately.
170-182:⚠️ Potential issue | 🔴 CriticalBug:
tenant_codequery parameter appended twice, producing a malformed URL.
isTenantScopedisBoolean(tenantCode), so whentenantCodeis truthy both line 178 and lines 180-182 execute, yielding a URL like…/internal/<id>?tenant_code=X?tenant_code=X. The second?breaks the query string.Remove the duplicate block:
🐛 Proposed fix
if (isTenantScoped) profileUrl += `?tenant_code=${encodeURIComponent(tenantCode)}` - if (tenantCode) { - profileUrl += `?tenant_code=${tenantCode}` - } - const isInternalTokenRequired = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/requests/user.js` around lines 170 - 182, The code appends tenant_code twice because isTenantScoped (Boolean(tenantCode)) already adds `?tenant_code=${encodeURIComponent(tenantCode)}`; remove the redundant `if (tenantCode) { profileUrl += \`?tenant_code=${tenantCode}\` }` block so tenant_code is only appended once (keep the existing isTenantScoped branch which uses encodeURIComponent and applies to internal vs public endpoints); ensure no other code later re-appends tenant_code.src/services/admin.js (1)
173-196:⚠️ Potential issue | 🔴 CriticalCritical bug:
userInfois never set in the admin (isAdmin) path, causing admin deletions to always fail with USER_NOT_FOUND.In the
isAdminbranch (Line 178-182):
userInforemainsnull(initialized at Line 173, never reassigned in this branch)- Line 182 assigns to bare
getUserDetailswithout a declaration keyword, creating an implicit globalThe check at Line 189 (
!userInfo) will always be true whenisAdminis true, returning a failure response.Proposed fix
let userInfo = null let userTenantCode = tenantCode if (isAdmin) { const userDetail = await menteeQueries.getMenteeExtensionById(userId, [], true) userTenantCode = userDetail?.tenant_code - getUserDetails = userDetail ? [userDetail] : [] + userInfo = userDetail || null } else { const getUserDetails = await menteeQueries.getUsersByUserIds([userId], {}, tenantCode) userInfo = getUserDetails?.[0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/admin.js` around lines 173 - 196, In the isAdmin branch fix the logic so userInfo gets set and avoid the implicit global getUserDetails: when calling menteeQueries.getMenteeExtensionById(userId, [], true) assign its result to a local variable (e.g., userDetail) and then set userInfo = userDetail (and update userTenantCode = userDetail?.tenant_code); remove the bare assignment to getUserDetails and ensure any getUserDetails used elsewhere is declared with const/let or not used at all so the subsequent check (!userInfo) and is_mentor access work correctly.src/database/queries/sessions.js (1)
1074-1134:⚠️ Potential issue | 🟠 MajorRemove duplicate function definition and add missing filter to prevent processing overlap.
Two issues:
Duplicate export: The first definition at line 1074 is dead code since
exports.getSessionsAssignedToMentoris reassigned at line 1108. Remove lines 1074–1100 entirely.Missing business logic filter: The new version removes
AND s.created_by = :mentorUserIdfrom the old query, but this removal is incorrect. The function comment at line 1300 inadmin.jsexplicitly states: "Update sessions where mentor was assigned (not created by mentor)". Without filtering out self-created sessions, this function now returns all sessions where the user is a mentor (both self-created and assigned by others). This causes overlap:
- Step 5 (
removeAndReturnMentorSessions, line 1279) updates sessions wherementor_id = userId AND created_by = userId- Step 6 (
updateSessionsWithAssignedMentor, line 1301) would then re-update those same sessions unnecessarilyAdd the missing filter:
AND s.created_by != :mentorUserIdto the WHERE clause to exclude self-created sessions and match the function's documented purpose.Suggested fix
// FIX: Added tenantCode parameter and included it in JOIN and WHERE clauses // ROOT CAUSE: Citus distributed database requires JOINs to include the distribution column (tenant_code). // Original query failed with: "complex joins are only supported when all distributed tables are // co-located and joined on their distribution columns" // SOLUTION: Added tenant_code to both the JOIN condition (line 1108) and WHERE clause (line 1112) // to ensure Citus can properly route the query across distributed shards. exports.getSessionsAssignedToMentor = async (mentorUserId, tenantCode) => { try { const query = ` SELECT s.*, sa.mentee_id FROM ${Session.tableName} s LEFT JOIN session_attendees sa ON s.id = sa.session_id AND s.tenant_code = sa.tenant_code WHERE s.mentor_id = :mentorUserId AND s.start_date > :currentTime AND s.deleted_at IS NULL AND s.tenant_code = :tenantCode + AND s.created_by != :mentorUserId `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/sessions.js` around lines 1074 - 1134, Remove the duplicate dead function definition of exports.getSessionsAssignedToMentor (the earlier block that starts the first declaration) so only the later implementation remains, and in that remaining function (the exports.getSessionsAssignedToMentor that builds query using Session.tableName and LEFT JOIN session_attendees) add the missing business filter AND s.created_by != :mentorUserId to the WHERE clause to exclude sessions the mentor created themself (ensure the replacements object still includes mentorUserId and currentTime and tenantCode); keep the tenant_code join condition (s.tenant_code = sa.tenant_code) intact so Citus routing stays correct.
🧹 Nitpick comments (1)
src/helpers/organizationCache.js (1)
27-41: The current top-levelrequireis already safe and well-documented; lazy-requiring is an optional enhancement.The extensive comment in the file (lines 3–25) correctly explains why the top-level
requireon line 27 is safe: thecacheHelperfunctions are only invoked at runtime inside the asyncset()andget()functions, not at module load time. By the time these functions execute, all modules are fully loaded and the circular dependency is already broken.While moving the
requireinside each function (lazy-requiring) would be marginally more robust, it is not necessary here. The current approach is correct and already justified in the code's documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/organizationCache.js` around lines 27 - 41, The top-level require of cacheHelper is safe given the documented reasoning, so keep the existing top-level const cacheHelper = require('@generics/cacheHelper') and do not move the require into exports.set or exports.get; ensure the existing comment block explaining that cacheHelper functions are only invoked at runtime (not module load) remains accurate and present above the require to document the circular-dependency rationale for exports.set and exports.get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/database/queries/sessions.js`:
- Around line 1074-1134: Remove the duplicate dead function definition of
exports.getSessionsAssignedToMentor (the earlier block that starts the first
declaration) so only the later implementation remains, and in that remaining
function (the exports.getSessionsAssignedToMentor that builds query using
Session.tableName and LEFT JOIN session_attendees) add the missing business
filter AND s.created_by != :mentorUserId to the WHERE clause to exclude sessions
the mentor created themself (ensure the replacements object still includes
mentorUserId and currentTime and tenantCode); keep the tenant_code join
condition (s.tenant_code = sa.tenant_code) intact so Citus routing stays
correct.
In `@src/requests/user.js`:
- Line 844: The catch in getUserDetailedList mistakenly calls error as a
function (throw error(error)), which causes a TypeError; change it to rethrow
the original Error object (throw error) or throw a new Error with the original
as cause so the original stack/message is preserved; update the catch block in
src/requests/user.js around getUserDetailedList to remove the callable
invocation and rethrow the Error object or wrap it appropriately.
- Around line 170-182: The code appends tenant_code twice because isTenantScoped
(Boolean(tenantCode)) already adds
`?tenant_code=${encodeURIComponent(tenantCode)}`; remove the redundant `if
(tenantCode) { profileUrl += \`?tenant_code=${tenantCode}\` }` block so
tenant_code is only appended once (keep the existing isTenantScoped branch which
uses encodeURIComponent and applies to internal vs public endpoints); ensure no
other code later re-appends tenant_code.
In `@src/services/admin.js`:
- Around line 173-196: In the isAdmin branch fix the logic so userInfo gets set
and avoid the implicit global getUserDetails: when calling
menteeQueries.getMenteeExtensionById(userId, [], true) assign its result to a
local variable (e.g., userDetail) and then set userInfo = userDetail (and update
userTenantCode = userDetail?.tenant_code); remove the bare assignment to
getUserDetails and ensure any getUserDetails used elsewhere is declared with
const/let or not used at all so the subsequent check (!userInfo) and is_mentor
access work correctly.
---
Nitpick comments:
In `@src/helpers/organizationCache.js`:
- Around line 27-41: The top-level require of cacheHelper is safe given the
documented reasoning, so keep the existing top-level const cacheHelper =
require('@generics/cacheHelper') and do not move the require into exports.set or
exports.get; ensure the existing comment block explaining that cacheHelper
functions are only invoked at runtime (not module load) remains accurate and
present above the require to document the circular-dependency rationale for
exports.set and exports.get.
… fields clearing approach. Update session queries to ensure tenant code is consistently applied, and remove unnecessary organization cache helper to streamline dependencies.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/admin.js (1)
173-196:⚠️ Potential issue | 🔴 CriticalCritical: admin-deletion path leaves
userInfo = null, causing every admin-initiated deletion to fail withUSER_NOT_FOUND.The PR correctly introduced
let userInfo = null(line 173) and readsuserInfo = getUserDetails?.[0]in theelsebranch (lines 185–186). However, theisAdminbranch (lines 178–182) was not updated: it still assignsgetUserDetails = userDetail ? [userDetail] : [](an undeclared-variable assignment) and never setsuserInfo. Theif (!userInfo)guard at line 189 therefore always fires for admin calls.🐛 Proposed fix
if (isAdmin) { const userDetail = await menteeQueries.getMenteeExtensionById(userId, [], true) userTenantCode = userDetail?.tenant_code - getUserDetails = userDetail ? [userDetail] : [] + userInfo = userDetail } else { const getUserDetails = await menteeQueries.getUsersByUserIds([userId], {}, tenantCode) userInfo = getUserDetails?.[0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/admin.js` around lines 173 - 196, The admin deletion branch fails to set userInfo and wrongly assigns to an undeclared getUserDetails; update the isAdmin branch (where menteeQueries.getMenteeExtensionById is called) to populate userInfo and userTenantCode consistently: assign userInfo = userDetail (or userDetail transformed to match the regular branch shape) and set userTenantCode = userDetail?.tenant_code, and remove or properly declare getUserDetails so the subsequent if (!userInfo) check and is_mentor access work for admin deletions.src/database/queries/userExtension.js (1)
241-278:⚠️ Potential issue | 🟡 MinorRemove dead code
removeMenteeDetailsfromuserExtension.js.This function is unreachable in the codebase. The only external reference is commented-out code in
admin.js(line 621), and the active user-deletion path inadmin.js(line 433) callsdeleteMenteeExtensioninstead. SincedeleteMenteeExtension(hard delete) has replacedremoveMenteeDetails(soft delete) as the removal strategy, the latter should be removed to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/userExtension.js` around lines 241 - 278, Remove the dead/unreachable soft-delete function removeMenteeDetails from userExtension.js: it's not used anywhere (only in commented code) and has been superseded by deleteMenteeExtension. Delete the entire removeMenteeDetails function declaration and any export entries that reference it (search for removeMenteeDetails in userExtension.js and the module.exports/object that exports extension functions) and run tests/lint to ensure no remaining references; keep deleteMenteeExtension intact.
🧹 Nitpick comments (3)
src/database/queries/mentorExtension.js (1)
156-182:fieldsToClearis byte-for-byte identical to the object inuserExtension.jsremoveMenteeDetails.Both share the same underlying table via
UserExtension.scope('mentors'), so the duplication is a pure DRY violation. The same factory-function refactor suggested inuserExtension.jsapplies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/mentorExtension.js` around lines 156 - 182, The fieldsToClear object in mentorExtension.js is identical to the one in userExtension.js (used by removeMenteeDetails) and should be deduplicated: extract a factory function (e.g., createFieldsToClear or getDefaultClearFields) into a shared module and have both mentorExtension.js and userExtension.js import and call that factory instead of redefining fieldsToClear; update usages where UserExtension.scope('mentors') logic relies on the object to call the factory so behavior remains unchanged.src/database/queries/userExtension.js (1)
241-267: IdenticalfieldsToClearduplicated acrossuserExtension.jsandmentorExtension.js.Both
removeMenteeDetailsandremoveMentorDetailsdefine the same 25-field object verbatim. SinceMentorExtensionisUserExtension.scope('mentors')(same underlying model), they share identical columns. Extract to a shared factory function to avoid drift:♻️ Suggested refactor – shared factory in a common module
+// In `@constants/common.js` or a shared db-utils module: +const getUserExtensionClearFields = () => ({ + designation: [], + area_of_expertise: [], + education_qualification: null, + rating: {}, + meta: {}, + stats: {}, + tags: [], + configs: {}, + visible_to_organizations: [], + external_session_visibility: null, + custom_entity_text: {}, + experience: null, + external_mentee_visibility: null, + mentee_visibility: null, + external_mentor_visibility: null, + mentor_visibility: null, + name: common.USER_NOT_FOUND, + email: null, + phone: null, + settings: {}, + image: null, + gender: null, + status: null, + username: null, + deleted_at: new Date(), +}) // In removeMenteeDetails / removeMentorDetails: -const fieldsToClear = { designation: [], ... } +const fieldsToClear = getUserExtensionClearFields()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/queries/userExtension.js` around lines 241 - 267, Extract the duplicated fieldsToClear object used in removeMenteeDetails and removeMentorDetails into a single shared factory function (e.g., getDefaultFieldsToClear or createFieldsToClear) located in a common module, export it, and replace the inline object in both userExtension and mentorExtension with calls to that factory; ensure the factory returns the same shape (including deleted_at: new Date()) and update imports/usages in removeMenteeDetails and removeMentorDetails accordingly to eliminate the verbatim duplication.src/services/admin.js (1)
1425-1433:SELECT *in raw query — consider projecting only needed columns.The simplified query is a clean improvement and the
tenant_codefilter correctly scopes results. However,SELECT *fromRequestSessionpulls every column when downstream code only needsid,requestor_id,requestee_id, andtitle(based on usage inrejectSessionRequestsDueToMentorDeletion). This is a minor performance concern on wide tables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/admin.js` around lines 1425 - 1433, The raw SQL in getPendingSessionRequestsForMentor uses SELECT * which returns unnecessary columns; change the projection to explicitly select id, requestor_id, requestee_id, and title (the fields used later by rejectSessionRequestsDueToMentorDeletion) instead of all columns so the query only returns needed data and reduces payload/IO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/database/queries/userExtension.js`:
- Around line 241-278: Remove the dead/unreachable soft-delete function
removeMenteeDetails from userExtension.js: it's not used anywhere (only in
commented code) and has been superseded by deleteMenteeExtension. Delete the
entire removeMenteeDetails function declaration and any export entries that
reference it (search for removeMenteeDetails in userExtension.js and the
module.exports/object that exports extension functions) and run tests/lint to
ensure no remaining references; keep deleteMenteeExtension intact.
In `@src/services/admin.js`:
- Around line 173-196: The admin deletion branch fails to set userInfo and
wrongly assigns to an undeclared getUserDetails; update the isAdmin branch
(where menteeQueries.getMenteeExtensionById is called) to populate userInfo and
userTenantCode consistently: assign userInfo = userDetail (or userDetail
transformed to match the regular branch shape) and set userTenantCode =
userDetail?.tenant_code, and remove or properly declare getUserDetails so the
subsequent if (!userInfo) check and is_mentor access work for admin deletions.
---
Nitpick comments:
In `@src/database/queries/mentorExtension.js`:
- Around line 156-182: The fieldsToClear object in mentorExtension.js is
identical to the one in userExtension.js (used by removeMenteeDetails) and
should be deduplicated: extract a factory function (e.g., createFieldsToClear or
getDefaultClearFields) into a shared module and have both mentorExtension.js and
userExtension.js import and call that factory instead of redefining
fieldsToClear; update usages where UserExtension.scope('mentors') logic relies
on the object to call the factory so behavior remains unchanged.
In `@src/database/queries/userExtension.js`:
- Around line 241-267: Extract the duplicated fieldsToClear object used in
removeMenteeDetails and removeMentorDetails into a single shared factory
function (e.g., getDefaultFieldsToClear or createFieldsToClear) located in a
common module, export it, and replace the inline object in both userExtension
and mentorExtension with calls to that factory; ensure the factory returns the
same shape (including deleted_at: new Date()) and update imports/usages in
removeMenteeDetails and removeMentorDetails accordingly to eliminate the
verbatim duplication.
In `@src/services/admin.js`:
- Around line 1425-1433: The raw SQL in getPendingSessionRequestsForMentor uses
SELECT * which returns unnecessary columns; change the projection to explicitly
select id, requestor_id, requestee_id, and title (the fields used later by
rejectSessionRequestsDueToMentorDeletion) instead of all columns so the query
only returns needed data and reduces payload/IO.
2 participants
Release Notes
fieldsToClearobject mappings, replacing dynamic attribute-driven nullificationgetSessionsAssignedToMentor()to accept and enforcetenantCodeparameter for consistent tenant filteringuserInfoobject pattern throughout deletion flowstenantCodestotenantCodeacross mentor/mentee deletion, private session cancellations, and session manager notificationsgetDefaultOrgId.js, now relying solely on environment variablesgetPendingSessionRequestsForMentorwith direct filtering instead of request_session_mapping joinsChanges by Author