Open
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.
…pdates, ensuring compliance with Citus constraints. Updated mentorExtension.js, roleExtentions.js, and userExtension.js to maintain data integrity by preventing nullification of required fields.
User delete self delete
WalkthroughThis PR refactors database queries to prevent partition key updates in Citus, introduces a new cache wrapper module to resolve circular dependencies between modules, updates session query signatures to propagate tenant codes, and corrects multiple data flow issues and parameter misalignments in the admin service affecting user deletion, session mapping, and notifications. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 (6)
src/services/admin.js (2)
667-709:⚠️ Potential issue | 🟠 MajorMismatched property names passed to
sendSessionNotification.Lines 687-688 pass
orgsCodeandtenantsCodeas property names, butsendSessionNotification(line 82) destructuresorgCodesandtenantCodes. These mismatched names mean the notification handler receivesundefinedfor both, which may affect template lookup fallback behavior.🐛 Proposed fix
const notificationResult = await NotificationHelper.sendSessionNotification({ sessions: removedSessionsDetail, templateCode: process.env.MENTOR_SESSION_DELETION_EMAIL_CODE, orgCode, recipientField: 'attendees', addionalData: { nameOfTheSession: '{sessionName}' }, tenantCode, - orgsCode, - tenantsCode, + orgCodes, + tenantCodes, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/admin.js` around lines 667 - 709, The call in unenrollAndNotifySessionAttendees to NotificationHelper.sendSessionNotification passes wrong property names (orgsCode and tenantsCode) that do not match the expected destructured params (orgCodes and tenantCodes); update the payload to use orgCodes and tenantCodes (e.g., set orgCodes: orgsCode and tenantCodes: tenantsCode or rename the local variables) so the notification handler receives the correct values and template lookup works as intended; ensure you update the object passed to NotificationHelper.sendSessionNotification in unenrollAndNotifySessionAttendees accordingly.
168-196:⚠️ Potential issue | 🔴 CriticalCritical bug:
userInfois never assigned in the admin (isAdmin) branch, so admin-initiated deletions always returnUSER_NOT_FOUND.In the
isAdminpath (lines 178-182), the code setsgetUserDetails(an undeclared variable — will be an implicit global or throw in strict mode) and never assignsuserInfo. SinceuserInfostaysnull(line 173), the check on line 189 always triggers the failure response.Additionally, line 200 passes
{ token, userId, userTenantCode }tofetchUserDetails, but that function destructures{ token, userId, tenantCode }— sotenantCodewill beundefinedinside the function.🐛 Proposed fix for the admin path
if (isAdmin) { // Admin deleting any user - no tenant code restriction const userDetail = await menteeQueries.getMenteeExtensionById(userId, [], true) userTenantCode = userDetail?.tenant_code - getUserDetails = userDetail ? [userDetail] : [] + userInfo = userDetail || null } else { // Regular user deleting themselves - use tenant code from token (optimized path) const getUserDetails = await menteeQueries.getUsersByUserIds([userId], {}, tenantCode) userInfo = getUserDetails?.[0] }And fix the
fetchUserDetailscall:- const getUserDetailById = await userRequests.fetchUserDetails({ token, userId, userTenantCode }) + const getUserDetailById = await userRequests.fetchUserDetails({ token, userId, tenantCode: userTenantCode })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/admin.js` around lines 168 - 196, In userDelete, the admin branch never assigns userInfo and uses an undeclared getUserDetails; update the isAdmin branch to properly capture the fetched record (assign userInfo = userDetail or userInfo = userDetail ? userDetail : null) and avoid creating an implicit global (don’t use getUserDetails there), and ensure userTenantCode is set from userDetail?.tenant_code; also when calling fetchUserDetails pass the expected key name tenantCode (i.e. { token, userId, tenantCode: userTenantCode }) so the function that destructures { token, userId, tenantCode } receives the correct value.src/requests/user.js (2)
843-845:⚠️ Potential issue | 🟠 MajorBug:
erroris called as a function instead of being thrown.
throw error(error)tries to invoke the caughtErrorobject as a function, which will throw aTypeError: error is not a functionat runtime, masking the original error.🐛 Proposed fix
} catch (error) { - 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` around lines 843 - 845, The catch block currently does "throw error(error)" which attempts to call the caught Error as a function; change it to rethrow the original error by using "throw error" (i.e., remove the erroneous function call) so the original exception is preserved and not masked by a TypeError in the catch block that contains that line.
168-182:⚠️ Potential issue | 🔴 CriticalBug:
tenantCodequery parameter is appended twice toprofileUrl.When
tenantCodeis truthy,isTenantScoped(line 170) istrue, so line 178 appends?tenant_code=…. Then lines 180-182 also fire (sincetenantCodeis truthy) and append a second?tenant_code=…, producing a malformed URL like:.../userId?tenant_code=X?tenant_code=XRemove 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 168 - 182, The fetchUserDetails function is appending the tenant_code query parameter twice: the isTenantScoped branch already appends `?tenant_code=${encodeURIComponent(tenantCode)}` to profileUrl, so remove the duplicate `if (tenantCode) { profileUrl += \`?tenant_code=${tenantCode}\` }` block; keep the encoded append in the isTenantScoped branch (and, if you later need to append additional query params, ensure you use '&' instead of a second '?').src/database/queries/sessions.js (1)
1074-1134:⚠️ Potential issue | 🟡 MinorRemove the duplicate export and verify the semantic change to the query filter.
The function
getSessionsAssignedToMentoris defined twice (lines 1074-1100 and 1108-1134), with the second definition silently overwriting the first. Remove the dead code at lines 1074-1100.Additionally, the new version removes the
AND s.created_by = :mentorUserIdfilter from the WHERE clause. This broadens the query to return all sessions assigned to the mentor, not just those they created. Verify this semantic change is intentional — confirm whetherupdateSessionsWithAssignedMentor()inadmin.jsshould operate on all assigned sessions or only those created by the mentor.Remove dead code
-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 - WHERE s.mentor_id = :mentorUserId - AND s.tenant_code = :tenantCode - AND s.start_date > :currentTime - AND s.deleted_at IS NULL - AND s.created_by = :mentorUserId - ` - - const sessionsToDelete = await Sequelize.query(query, { - type: QueryTypes.SELECT, - replacements: { - mentorUserId, - currentTime: Math.floor(Date.now() / 1000), - tenantCode: tenantCode, - }, - }) - - return sessionsToDelete - } catch (error) { - throw error - } -} - // FIX: Added tenantCode parameter and included it in JOIN and WHERE clauses🤖 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, There are two definitions of getSessionsAssignedToMentor; remove the dead/duplicate export (the first occurrence) so the function is defined only once, and confirm the intended semantics: either keep the added tenant_code JOIN change (s.tenant_code = sa.tenant_code) and leave out the s.created_by = :mentorUserId filter, or if the original intent was to restrict to sessions created by the mentor, add back AND s.created_by = :mentorUserId to the remaining function; also search for updateSessionsWithAssignedMentor in admin.js and ensure it expects the broader “all assigned sessions” result or adjust that caller if the filter should be preserved.src/database/queries/userExtension.js (1)
243-285:⚠️ Potential issue | 🔴 CriticalFix
removeMenteeDetailsto excludeorganization_codeandtenant_codefrom nullificationThe method attempts to nullify these NOT NULL fields, which will throw a
notNull Violationerror. The parallelremoveMentorDetailsmethod was already corrected to exclude both fields—apply the same fix here.Proposed fix
if ( attribute.primaryKey || key === 'user_id' || key === 'organization_id' || // required field + key === 'organization_code' || // required NOT NULL field - cannot be nullified + key === 'tenant_code' || // required NOT NULL field - cannot be nullified key === 'created_at' || key === 'updated_at' || key === 'is_mentor' // has default value ) {🤖 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 243 - 285, The removeMenteeDetails function is currently attempting to nullify NOT NULL fields causing notNull violations; update its loop that builds fieldsToNullify (in the method removeMenteeDetails on MenteeExtension) to also skip keys 'organization_code' and 'tenant_code' (same pattern as the existing skips for 'organization_id' and 'user_id'), ensuring those two fields are not added to fieldsToNullify before calling MenteeExtension.update.
🤖 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: There are two definitions of
getSessionsAssignedToMentor; remove the dead/duplicate export (the first
occurrence) so the function is defined only once, and confirm the intended
semantics: either keep the added tenant_code JOIN change (s.tenant_code =
sa.tenant_code) and leave out the s.created_by = :mentorUserId filter, or if the
original intent was to restrict to sessions created by the mentor, add back AND
s.created_by = :mentorUserId to the remaining function; also search for
updateSessionsWithAssignedMentor in admin.js and ensure it expects the broader
“all assigned sessions” result or adjust that caller if the filter should be
preserved.
In `@src/database/queries/userExtension.js`:
- Around line 243-285: The removeMenteeDetails function is currently attempting
to nullify NOT NULL fields causing notNull violations; update its loop that
builds fieldsToNullify (in the method removeMenteeDetails on MenteeExtension) to
also skip keys 'organization_code' and 'tenant_code' (same pattern as the
existing skips for 'organization_id' and 'user_id'), ensuring those two fields
are not added to fieldsToNullify before calling MenteeExtension.update.
In `@src/requests/user.js`:
- Around line 843-845: The catch block currently does "throw error(error)" which
attempts to call the caught Error as a function; change it to rethrow the
original error by using "throw error" (i.e., remove the erroneous function call)
so the original exception is preserved and not masked by a TypeError in the
catch block that contains that line.
- Around line 168-182: The fetchUserDetails function is appending the
tenant_code query parameter twice: the isTenantScoped branch already appends
`?tenant_code=${encodeURIComponent(tenantCode)}` to profileUrl, so remove the
duplicate `if (tenantCode) { profileUrl += \`?tenant_code=${tenantCode}\` }`
block; keep the encoded append in the isTenantScoped branch (and, if you later
need to append additional query params, ensure you use '&' instead of a second
'?').
In `@src/services/admin.js`:
- Around line 667-709: The call in unenrollAndNotifySessionAttendees to
NotificationHelper.sendSessionNotification passes wrong property names (orgsCode
and tenantsCode) that do not match the expected destructured params (orgCodes
and tenantCodes); update the payload to use orgCodes and tenantCodes (e.g., set
orgCodes: orgsCode and tenantCodes: tenantsCode or rename the local variables)
so the notification handler receives the correct values and template lookup
works as intended; ensure you update the object passed to
NotificationHelper.sendSessionNotification in unenrollAndNotifySessionAttendees
accordingly.
- Around line 168-196: In userDelete, the admin branch never assigns userInfo
and uses an undeclared getUserDetails; update the isAdmin branch to properly
capture the fetched record (assign userInfo = userDetail or userInfo =
userDetail ? userDetail : null) and avoid creating an implicit global (don’t use
getUserDetails there), and ensure userTenantCode is set from
userDetail?.tenant_code; also when calling fetchUserDetails pass the expected
key name tenantCode (i.e. { token, userId, tenantCode: userTenantCode }) so the
function that destructures { token, userId, tenantCode } receives the correct
value.
Comment on lines
+80
to
+82
| if (updateData.tenant_code) { | ||
| delete updateData['tenant_code'] | ||
| } |
Collaborator
There was a problem hiding this comment.
This will be removed in the validator right if added in the req body?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
Key Changes
updateMentorExtensionandupdateRoleExtensionfunctions to removetenant_codefrom update payloads, preventing accidental updates to Citus partition keysremoveMentorDetailsto excludeorganization_codeandtenant_codefrom nullification to prevent NOT NULL constraint violationsgetSessionsAssignedToMentorto accepttenantCodeparameter and properly filter results by tenantorganizationCachewrapper module to break circular dependency between cacheHelper and user request handlersgetSessionsMappingcalls to pass three arguments instead of tworequest_session_mappingtabletenantCodeinstead oftenantCodesupdateMenteeExtensionto remove partition key columns (user_id,tenant_code) before update operationsLines Changed by Author