Refactor cache handling in session and user retrieval methods across …#1612
Merged
nevil-mathew merged 12 commits intoMar 30, 2026
Merged
Conversation
…nant_phase2 Feature/tenant phase2 temp branch -1
…ficiency. Updated cache set and delete methods in cacheHelper for session and user data. Simplified cache invalidation logic in EntityHelper and NotificationTemplateHelper by capturing old values before updates. Removed redundant cache invalidation after entity operations to streamline processing.
…services to improve efficiency. Updated cache deletion logic in AdminService and streamlined user cache clearing in EntityHelper. Enhanced organization extension updates in OrganizationService by including tenantCode in the upsert method.
| // Step 3: Fetch missing users from DB immediately using getUsersByUserIds | ||
| const dbFetchedUsers = [] | ||
| let userMentorEntries = [] | ||
| if (missingUserIds.length > 0) { |
Collaborator
There was a problem hiding this comment.
The getMenteeKafka method has no equivalent call to this._sendToKafkaBackground() to getMentorKafka. What is the use of getMenteeKafka?
Collaborator
Author
There was a problem hiding this comment.
This needs to be done later as discussion
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChanged cache key usage to rely on tenantCode + ID (removed organizationCode) for single-ID session/mentor/mentee cache operations, updated related cache deletes and logs, iterated entity-type model invalidation across all models and deferred user-cache clearing post-delete, removed entity-list caching, and passed tenantCode into organisationExtensionQueries.upsert. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/generics/cacheHelper.js (2)
1094-1094:⚠️ Potential issue | 🟡 MinorMissing
organizationCodeparameter in_sendToKafkaBackgroundcall.The
mentor._sendToKafkaBackgroundmethod signature at line 975 expects three parameters:(userMappingData, tenantCode, organizationCode), but this call only passes two. This will causeorganizationCodeto beundefinedin the Kafka batch message (line 1008).🐛 Proposed fix
- this._sendToKafkaBackground(userMappingData, tenantCode) + this._sendToKafkaBackground(userMappingData, tenantCode, organizationCode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` at line 1094, The call to this._sendToKafkaBackground(userMappingData, tenantCode) is missing the third parameter organizationCode expected by _sendToKafkaBackground; update the call to pass the appropriate organizationCode value from the current scope (e.g., organizationCode or this.organizationCode) so the method receives (userMappingData, tenantCode, organizationCode) and the Kafka batch message will include the correct organizationCode.
1101-1101:⚠️ Potential issue | 🟡 MinorIncorrect method name in error log.
The error message references
getMenteeKafkabut this code is inside thegetMentorKafkamethod.📝 Proposed fix
- console.error(`❌ [getMenteeKafka] Error processing user IDs:`, error) + console.error(`❌ [getMentorKafka] Error processing user IDs:`, error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` at line 1101, The error log in the getMentorKafka function incorrectly references getMenteeKafka; update the console.error call inside getMentorKafka to use the correct method name (e.g., change the message string from "[getMenteeKafka] Error processing user IDs:" to "[getMentorKafka] Error processing user IDs:") and scan for any other misplaced identifiers in getMentorKafka to ensure log consistency with the function name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/entity.js`:
- Around line 398-404: The call to getAllEntitiesWithEntityTypeDetails is
passing [tenantCode] (an array) which breaks the Sequelize WHERE clause and
tenant isolation; change the call so tenantCode is passed as a scalar string
(remove the surrounding array) or simply rely on the existing filter.
Specifically, update the invocation of getAllEntitiesWithEntityTypeDetails to
pass tenantCode (not [tenantCode]) and ensure filter.tenant_code remains the
scalar tenant_code value so the function (and the query using tenant_code in
entityQueries.getAllEntitiesWithEntityTypeDetails) receives a string, not an
array.
---
Outside diff comments:
In `@src/generics/cacheHelper.js`:
- Line 1094: The call to this._sendToKafkaBackground(userMappingData,
tenantCode) is missing the third parameter organizationCode expected by
_sendToKafkaBackground; update the call to pass the appropriate organizationCode
value from the current scope (e.g., organizationCode or this.organizationCode)
so the method receives (userMappingData, tenantCode, organizationCode) and the
Kafka batch message will include the correct organizationCode.
- Line 1101: The error log in the getMentorKafka function incorrectly references
getMenteeKafka; update the console.error call inside getMentorKafka to use the
correct method name (e.g., change the message string from "[getMenteeKafka]
Error processing user IDs:" to "[getMentorKafka] Error processing user IDs:")
and scan for any other misplaced identifiers in getMentorKafka to ensure log
consistency with the function name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27f91ab6-bde5-4001-abdb-f204a0a62d7c
📒 Files selected for processing (6)
src/generics/cacheHelper.jssrc/services/admin.jssrc/services/entity-type.jssrc/services/entity.jssrc/services/notification.jssrc/services/organization.js
…dom/mentoring into tenant_fork_phase2
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/generics/cacheHelper.js (1)
1112-1120:⚠️ Potential issue | 🟡 MinorIncorrect log prefix in error messages.
The error logs reference
[getMenteeKafka]but this code is insidementor.getMentorKafka. This appears to be a copy-paste error that will cause confusion during debugging.Proposed fix
} catch (error) { - console.error(`❌ [getMenteeKafka] Error processing user IDs:`, error) + console.error(`❌ [getMentorKafka] Error processing user IDs:`, error) // Fallback to regular cache method for all user IDs const fallbackResults = [] for (const userId of Array.isArray(userIds) ? userIds : [userIds]) { try { const user = await this.get(tenantCode, userId) if (user) fallbackResults.push(user) } catch (fallbackError) { - console.error(`❌ [getMenteeKafka] Fallback failed for user ${userId}:`, fallbackError.message) + console.error(`❌ [getMentorKafka] Fallback failed for user ${userId}:`, fallbackError.message) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generics/cacheHelper.js` around lines 1112 - 1120, The console error messages inside the mentor.getMentorKafka flow use the wrong log prefix "[getMenteeKafka]"; update those log messages to use "[getMentorKafka]" so logs reflect the correct function context—specifically change the two console.error calls that print the main error and the fallbackError.message within the getMentorKafka implementation to use the correct prefix.src/services/notification.js (1)
103-117:⚠️ Potential issue | 🔴 CriticalCritical:
isDefaultOrgis undefined and will cause a ReferenceError.The
isDefaultOrgvariable is used on line 105 but is never defined in this method. Based on the AI summary, the variable definition was removed but its usage was not. This will cause a runtime crash when any notification template update is attempted.Proposed fix - Add isDefaultOrg computation or simplify logic
Option 1: Add the missing
isDefaultOrgcomputation:const existingTemplate = existingTemplates?.[0] const oldCode = existingTemplate?.code || filter.code + + // Determine if this is the default organization + const defaults = await getDefaults() + const isDefaultOrg = tokenInformation.organization_code === defaults.orgCode const result = await notificationTemplateQueries.updateTemplate(filter, bodyData, tenantCode)Option 2: If default org check is no longer needed, simplify to always use single-org deletion:
try { if (oldCode) { - if (isDefaultOrg) { - await cacheHelper.notificationTemplates.deleteNotificationsAcrossAllOrgs(tenantCode, oldCode) - } else { - await cacheHelper.notificationTemplates.delete( - tenantCode, - tokenInformation.organization_code, - oldCode - ) - } + await cacheHelper.notificationTemplates.delete( + tenantCode, + tokenInformation.organization_code, + oldCode + ) } } catch (cacheError) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/notification.js` around lines 103 - 117, The code uses an undefined isDefaultOrg causing a ReferenceError; update the try block in the notification update flow to compute or remove that check: either compute isDefaultOrg (e.g., derive from tokenInformation or tenantCode) before the if (oldCode) block and use it to choose between cacheHelper.notificationTemplates.deleteNotificationsAcrossAllOrgs(tenantCode, oldCode) and cacheHelper.notificationTemplates.delete(tenantCode, tokenInformation.organization_code, oldCode), or remove the isDefaultOrg conditional and always call the single-org delete path (cacheHelper.notificationTemplates.delete(...)) if default-org semantics are no longer required; ensure you reference tenantCode, tokenInformation.organization_code and oldCode consistently.
🤖 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/generics/cacheHelper.js`:
- Around line 1112-1120: The console error messages inside the
mentor.getMentorKafka flow use the wrong log prefix "[getMenteeKafka]"; update
those log messages to use "[getMentorKafka]" so logs reflect the correct
function context—specifically change the two console.error calls that print the
main error and the fallbackError.message within the getMentorKafka
implementation to use the correct prefix.
In `@src/services/notification.js`:
- Around line 103-117: The code uses an undefined isDefaultOrg causing a
ReferenceError; update the try block in the notification update flow to compute
or remove that check: either compute isDefaultOrg (e.g., derive from
tokenInformation or tenantCode) before the if (oldCode) block and use it to
choose between
cacheHelper.notificationTemplates.deleteNotificationsAcrossAllOrgs(tenantCode,
oldCode) and cacheHelper.notificationTemplates.delete(tenantCode,
tokenInformation.organization_code, oldCode), or remove the isDefaultOrg
conditional and always call the single-org delete path
(cacheHelper.notificationTemplates.delete(...)) if default-org semantics are no
longer required; ensure you reference tenantCode,
tokenInformation.organization_code and oldCode consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8d49d9a-dbf9-433c-816d-1fdc9c15357a
📒 Files selected for processing (2)
src/generics/cacheHelper.jssrc/services/notification.js
Merged
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.
…services to improve efficiency. Updated cache deletion logic in AdminService and streamlined user cache clearing in EntityHelper. Enhanced organization extension updates in OrganizationService by including tenantCode in the upsert method.
Release Notes
Lines Changed by Author