Feature/org id code migration#1644
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…into feature/org_id_code_migration
|
@coderabbitai full review and check if everything is okay and approved from your side. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details
gh pr view 1644 --repo ELEVATE-Project/mentoring --json title,body,state,additions,deletions,changedFiles,files,reviews,comments,reviewRequests,labels,assignees
echo "---FILES---"
gh pr diff 1644 --repo ELEVATE-Project/mentoring
echo "---REVIEW COMMENTS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/comments --paginate | jq '[.[] | {id, path, line, body, user: .user.login}]'
echo "---REVIEWS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/reviews | jq '[.[] | {id, state, body, user: .user.login}]'Length of output: 63657 🏁 Script executed: #!/bin/bash
echo "=== processEntityTypesToAddValueLabels signature ==="
rg -n "processEntityTypesToAddValueLabels" --type=js -C 5 | head -80
echo ""
echo "=== organisationExtensionQueries.findAll ==="
ast-grep --pattern 'static async findAll($$$) { $$$ }' | head -60
echo ""
echo "=== findAll in organisationExtension queries ==="
fd -i "organisationExtension" --type f | xargs grep -n "findAll" 2>/dev/null | head -30
echo ""
echo "=== updateRelatedOrgs function signature in services/org-admin ==="
rg -n "updateRelatedOrgs" --type=js -C 3 | head -60
echo ""
echo "=== getEntityTypesWithMentorOrg callers ==="
rg -n "getEntityTypesWithMentorOrg" --type=js -C 2
echo ""
echo "=== PUSH and POP constants ==="
rg -n "PUSH\|POP" src/constants/common.jsLength of output: 12145
🔴 Critical — SQL Injection Risks1. Migration batch update —
|
|
Warning Review limit reached
More reviews will be available in 21 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReplaces numeric organization IDs with organization code strings across cache, helpers, DB schema/migrations, queries, services, Kafka consumer, and a cleanup script. ChangesOrganization Code Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/database/queries/userExtension.js (1)
120-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw interpolation in array SQL literals is unsafe for org codes.
Line 120 and Line 150 embed org-code values directly into SQL. This can fail on quoted values and introduces injection risk.
Also applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/queries/userExtension.js` around lines 120 - 121, The code currently injects org codes directly into SQL via the raw interpolation used in the ARRAY[${newRelatedOrgsSql}] expression (affecting the array_cat/COALESCE update of visible_to_organizations and the similar occurrence around line 150), which is unsafe; instead, change the SQL to use a parameterized array (e.g. array_cat(COALESCE("visible_to_organizations", ARRAY[]::varchar[]), $X::varchar[])) and pass the org codes as a bound array parameter (e.g. relatedOrgs) from the query builder/execution call; update both places that reference newRelatedOrgsSql to use the parameter placeholder and add the corresponding parameter in the query parameters list so values are safely escaped and not raw-interpolated.
🧹 Nitpick comments (2)
src/services/mentors.js (1)
492-496: ⚖️ Poor tradeoffMentor creation sets minimal
visible_to_organizationscompared to role change.When creating a mentor extension directly,
visible_to_organizationsis set to only[orgCode]. However, when a mentee is changed to mentor viachangeRoleToMentorinorg-admin.js(lines 235-241), thevisible_to_organizationsincludes bothrelated_orgscodes and the current org code.This inconsistency means mentors created through different flows will have different visibility scopes. If this is intentional (e.g., direct creation should have minimal visibility, role change inherits existing visibility), consider adding a comment to clarify.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/mentors.js` around lines 492 - 496, The mentor creation path in src/services/mentors.js currently sets visible_to_organizations to only [orgCode], which differs from the behavior in changeRoleToMentor (org-admin.js) that sets visible_to_organizations to include related_orgs plus the current org; update the mentor creation logic (the code that builds data and assigns visible_to_organizations) to compute the same visibility set as changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal visibility is intentional, add a clear inline comment near the assignment of visible_to_organizations explaining the deliberate difference and why it should not inherit related_orgs—refer to visible_to_organizations, orgCode, and changeRoleToMentor to locate the relevant code paths.src/services/org-admin.js (1)
686-694: 💤 Low valueParameter names are misleading after migration.
The function receives organization codes from the controller, but the parameter names still say
deltaOrganizationIdsandorgId. While.toString()and.map(String)are harmless on strings, the naming creates confusion about what data type is expected.♻️ Suggested rename for clarity
-static async updateRelatedOrgs(deltaOrganizationIds, orgId, action, tenantCode) { +static async updateRelatedOrgs(deltaOrgCodes, orgCode, action, tenantCode) { try { - orgId = orgId.toString() - deltaOrganizationIds = deltaOrganizationIds.map(String) + orgCode = String(orgCode) + deltaOrgCodes = deltaOrgCodes.map(String) if (action?.toLowerCase() === common.PUSH.toLowerCase()) { - await menteeQueries.addVisibleToOrg(orgId, deltaOrganizationIds, tenantCode) + await menteeQueries.addVisibleToOrg(orgCode, deltaOrgCodes, tenantCode) } else if (action?.toLowerCase() === common.POP.toLowerCase()) { - await menteeQueries.removeVisibleToOrg(orgId, deltaOrganizationIds, tenantCode) + await menteeQueries.removeVisibleToOrg(orgCode, deltaOrgCodes, tenantCode) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/org-admin.js` around lines 686 - 694, The parameter names in updateRelatedOrgs are misleading: rename orgId to orgCode and deltaOrganizationIds to deltaOrganizationCodes (function: updateRelatedOrgs) and update all internal uses (replace orgId.toString() with orgCode = String(orgCode) or just use orgCode if already a string, and deltaOrganizationIds = deltaOrganizationIds.map(String) → deltaOrganizationCodes = deltaOrganizationCodes.map(String)); also update calls to menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to use the new parameter names to keep semantics clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/v1/org-admin.js`:
- Around line 139-145: The conversion result for organization_id may be empty
causing orgCode to be undefined; in the controller code around
convertOrgIdsToOrgCodes and before calling orgAdminService.updateRelatedOrgs,
validate that the returned [orgCode] is present and valid (e.g., not
undefined/null/empty) and if missing respond with an appropriate error (HTTP 400
or throw a ValidationError) including a clear message referencing
req.body.organization_id; ensure deltaOrgCodes is still computed but
short-circuit the updateRelatedOrgs call when orgCode is invalid to avoid
downstream failures.
In `@src/database/migrations/20260525000001-backfill-org-codes-from-ids.js`:
- Around line 101-108: The migration is doing per-row DB updates in the sessions
loop (sessionRows, toCodeArray, pgArr, queryInterface.sequelize.query) which
causes many round-trips; change it to a set-based update: build a single
in-memory list of (id, visible_to_organization_codes) updates using toCodeArray
and pgArr, then execute one SQL statement to apply them (for example using a
single UPDATE with a CASE ... WHEN id = ... THEN ... END or by bulk inserting
the tuples into a temp table and running an UPDATE ... FROM temp_table) so
sessionArrUpdated is updated accordingly; apply the same refactor to the other
per-row loop referenced in the comment (the block around the second loop).
- Around line 52-53: The current pgArr helper builds Postgres array literals via
arr.join(',') which breaks on org codes containing commas/quotes/braces; replace
use of pgArr by passing JS arrays as parameterized query parameters (e.g. use
$n::text[] and supply the JS array to node-postgres) instead of
string-concatenating, or implement a safe escaping routine that produces a valid
Postgres array literal (escaping quotes/backslashes and wrapping elements in
double quotes) and use that in place of pgArr; update every place that calls
pgArr (the pgArr function and the query sites that use it in this file) to use
parameterized array binding or the safe-escaping function.
- Around line 68-74: The migration is interpolating org codes into SQL via the
values string (sessionCodeUpdates → values) which can break on quotes and allow
injection; instead build a parameterized VALUES list and pass the parameters to
queryInterface.sequelize.query (or use binds) so no raw codes are concatenated.
Concretely, produce a VALUES clause of placeholders like ($1,$2),($3,$4),...
corresponding to each sessionCodeUpdates entry, collect ids and codes into a
single replacements/bind array, and call queryInterface.sequelize.query(sql, {
replacements: params }) (or { bind: params }) so the UPDATE of sessions SET
mentor_organization_code = v.code FROM (VALUES ...) AS v(id, code) uses
parameterized values rather than interpolated u.code.
In `@src/database/queries/mentorExtension.js`:
- Around line 360-363: The SQL literal is vulnerable because newRelatedOrgs
values are interpolated directly into sequelize.literal for array_cat/COALESCE;
instead, stop direct string interpolation of newRelatedOrgs and parameterize or
escape each org value before building the array expression. Locate the
array_cat(...) usage that references "visible_to_organizations" and
newRelatedOrgs and either (a) build the array via Sequelize helpers (e.g.,
sequelize.fn/ sequelize.cast) so values are passed as parameters, or (b) escape
each value with sequelize.escape (or use query bind/replacements) when
constructing the ARRAY[...] expression so quotes and special chars cannot break
the SQL or allow injection.
In `@src/scripts/dropOrgNumericBackupColumns.js`:
- Around line 106-107: The ALTER TABLE drop loop currently exits on the first
error which prevents the final materialized view rebuilds; modify the block that
runs await db.sequelize.query(`ALTER TABLE "${table}" DROP COLUMN IF EXISTS
"${column}" CASCADE`) / console.log(...) so it catches and records errors (do
not call process.exit or return from inside the loop), continue processing
remaining drops, and after the loop run the materialized view rebuild logic
regardless, then if any drops failed return a non-zero exit or throw a
summarized error; update any early-exit calls near the drop loop to instead
defer failure propagation until after the rebuild step.
---
Outside diff comments:
In `@src/database/queries/userExtension.js`:
- Around line 120-121: The code currently injects org codes directly into SQL
via the raw interpolation used in the ARRAY[${newRelatedOrgsSql}] expression
(affecting the array_cat/COALESCE update of visible_to_organizations and the
similar occurrence around line 150), which is unsafe; instead, change the SQL to
use a parameterized array (e.g. array_cat(COALESCE("visible_to_organizations",
ARRAY[]::varchar[]), $X::varchar[])) and pass the org codes as a bound array
parameter (e.g. relatedOrgs) from the query builder/execution call; update both
places that reference newRelatedOrgsSql to use the parameter placeholder and add
the corresponding parameter in the query parameters list so values are safely
escaped and not raw-interpolated.
---
Nitpick comments:
In `@src/services/mentors.js`:
- Around line 492-496: The mentor creation path in src/services/mentors.js
currently sets visible_to_organizations to only [orgCode], which differs from
the behavior in changeRoleToMentor (org-admin.js) that sets
visible_to_organizations to include related_orgs plus the current org; update
the mentor creation logic (the code that builds data and assigns
visible_to_organizations) to compute the same visibility set as
changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal
visibility is intentional, add a clear inline comment near the assignment of
visible_to_organizations explaining the deliberate difference and why it should
not inherit related_orgs—refer to visible_to_organizations, orgCode, and
changeRoleToMentor to locate the relevant code paths.
In `@src/services/org-admin.js`:
- Around line 686-694: The parameter names in updateRelatedOrgs are misleading:
rename orgId to orgCode and deltaOrganizationIds to deltaOrganizationCodes
(function: updateRelatedOrgs) and update all internal uses (replace
orgId.toString() with orgCode = String(orgCode) or just use orgCode if already a
string, and deltaOrganizationIds = deltaOrganizationIds.map(String) →
deltaOrganizationCodes = deltaOrganizationCodes.map(String)); also update calls
to menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and
deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to
use the new parameter names to keep semantics clear.
🪄 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: 342279b4-2f90-44da-b1e9-ed03d2de5b48
📒 Files selected for processing (18)
src/constants/common.jssrc/controllers/v1/org-admin.jssrc/database/migrations/20260525000001-backfill-org-codes-from-ids.jssrc/database/migrations/20260525000002-rename-visible-to-org-codes.jssrc/database/models/sessions.jssrc/database/queries/mentorExtension.jssrc/database/queries/sessions.jssrc/database/queries/userExtension.jssrc/generics/cacheHelper.jssrc/generics/kafka/consumers/organization.jssrc/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.jssrc/helpers/orgUtils.jssrc/helpers/saasUserAccessibility.jssrc/scripts/dropOrgNumericBackupColumns.jssrc/services/mentees.jssrc/services/mentors.jssrc/services/org-admin.jssrc/services/sessions.js
|
@coderabbitai full review and see if everything is okay. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== 1. Migration backfill VALUES interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape"
echo ""
echo "=== 2. pgArr helper ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "pgArr" | head -10
echo ""
echo "=== 3. mentorExtension.js - array_cat unsafe interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[" | head -20
echo ""
echo "=== 4. userExtension.js - ARRAY interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "ARRAY\[" | head -20
echo ""
echo "=== 5. controller org-admin undefined guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -A5 "organization_id\|convertOrgIds\|failureResponse\|400" | head -40
echo ""
echo "=== 6. information_schema table_schema filter ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public"
echo ""
echo "=== 7. scripts/dropOrgNumericBackupColumns.js - main() guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | tail -20
echo ""
echo "=== 8. JSDoc in org-admin service updateRelatedOrgs ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -B2 -A10 "updateRelatedOrgs" | head -40Length of output: 3224 🏁 Script executed: #!/bin/bash
# Get the PR head SHA first
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
HEAD_REF=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.ref')
echo "HEAD REF: $HEAD_REF"
echo ""
echo "=== 1. Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape\|table_schema\|information_schema"
echo ""
echo "=== 2. mentorExtension.js - array_cat unsafe interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\["
echo ""
echo "=== 3. userExtension.js - ARRAY interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\["
echo ""
echo "=== 4. controller org-admin - undefined guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A8 "updateRelatedOrgs\|convertOrgIds\|organization_id\|failureResponse\|400" | head -50
echo ""
echo "=== 5. dropOrgNumericBackupColumns.js tail ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -15
echo ""
echo "=== 6. org-admin service JSDoc for updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A15 "updateRelatedOrgs" | head -50Length of output: 7293
✅ Fixed
🔴 Still Open — Blocking Approval
// Still unescaped:
const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code}')`).join(', ')Fix: escape single quotes in const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code.replace(/'/g, "''")}')`).join(', ')
// Still broken for codes with commas/spaces:
const pgArr = (arr) => `{${arr.join(',')}}`Fix: const pgArr = (arr) => `{${arr.map((v) => `"${v.replace(/"/g, '\\"')}"`).join(',')}}`
`ARRAY[${newRelatedOrgs.map((v) => `'${v}'`).join(',')}]::varchar[]`
`COALESCE(...) || ARRAY['${organizationId}']::varchar[]`Org codes interpolated as string literals — a
🟡 Still Open — Non-blocking
I cannot approve the PR yet because issues |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai made changes to the codebase. Just verify now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== `#1` & `#2`: Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "pgArr\|u\.code\|VALUES\|replace\|escape\|table_schema\|information_schema" | head -30
echo ""
echo "=== `#1` & `#2`: Full pgArr and VALUES section ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '45,90p'
echo ""
echo "=== `#3`: mentorExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[\|varchar\|replace\|escape" | head -20
echo ""
echo "=== `#4`: userExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\[\|varchar\|replace\|escape" | head -20
echo ""
echo "=== `#6`: information_schema table_schema filter in migration ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public" | head -10
echo ""
echo "=== `#7`: JSDoc in org-admin service updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -B2 -A12 "updateRelatedOrgs" | head -50
echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js tail - require.main guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -20Length of output: 10331
✅ All Critical Issues Fixed
🟡 Remaining Non-Blocking Items (can be addressed in follow-up)
Since all blocking SQL injection / data corruption risks are resolved, I'm approving the PR. The three remaining items are minor and can be cleaned up in a follow-up. [approve] 🐇 ✨ |
…erized bind queries in addVisibleToOrg Switches both UPDATE statements in addVisibleToOrg from Model.update() + sequelize.literal (manual .replace() escaping) to Sequelize.query() with bind parameters ($1/$2/$3). The pg driver sends bind values as typed protocol-level parameters — no SQL text manipulation, no escaping needed. Also removes the now-dead options param, otherOptions, optionsWhere, and whereClause1 variables that were unused after the query rewrite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai Full review again the recent change, approve if okay from your side. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Recent commits on PR branch ==="
gh api "repos/ELEVATE-Project/mentoring/pulls/1644/commits" | jq -r '.[] | "\(.sha[0:10]) \(.commit.author.date) \(.commit.message | split("\n")[0])"'
echo ""
echo "=== `#1` & `#2`: Migration - pgArr and VALUES escaping ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '45,95p'
echo ""
echo "=== `#3`: mentorExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[\|replace\|varchar" | head -20
echo ""
echo "=== `#4`: userExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\[\|replace\|varchar" | head -20
echo ""
echo "=== `#6`: information_schema table_schema filter ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public" | head -10
echo ""
echo "=== `#7`: JSDoc in org-admin service ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A15 "updateRelatedOrgs" | head -50
echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js tail ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -20
echo ""
echo "=== Controller org-admin - updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A15 "updateRelatedOrgs\|convertOrgIds\|organization_id\|orgCode" | head -60
echo ""
echo "=== orgUtils.js - convertOrgIdsToOrgCodes ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/helpers/orgUtils.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "normaliz\|undefined\|filter\|map\|orgIds" | head -30
echo ""
echo "=== dropOrgNumericBackupColumns full main() area ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "main\(\|require\.main\|module\.exports" | head -10Length of output: 13781
✅ All Critical Issues — Confirmed Fixed
The latest refactor on 🟡 Remaining Non-Blocking (can be addressed in a follow-up)
All blocking issues are resolved. Approving. 🐇 ✨ [approve] |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/database/queries/userExtension.js (1)
99-100: Ensure an index backs the neworganization_code+tenant_codepredicates.Both UPDATEs filter
user_extensionsonorganization_codeandtenant_code(Lines 99-100 and 116-117). A composite index on(tenant_code, organization_code)will keep these targeted instead of full scans as the table grows. As per coding guidelines ("Review database queries for performance ... ensure indexes can be used").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/queries/userExtension.js` around lines 99 - 100, Add a composite B-tree index to back the WHERE predicates on user_extensions for organization_code and tenant_code so the UPDATEs filter without full scans; create an index such as idx_user_extensions_tenant_organization on (tenant_code, organization_code) via a DB migration (and drop it in rollback) so the UPDATE statements filtering "organization_code" and "tenant_code" can use an index; ensure the migration references the table name user_extensions and the same column names used in the queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/database/queries/userExtension.js`:
- Around line 99-100: Add a composite B-tree index to back the WHERE predicates
on user_extensions for organization_code and tenant_code so the UPDATEs filter
without full scans; create an index such as
idx_user_extensions_tenant_organization on (tenant_code, organization_code) via
a DB migration (and drop it in rollback) so the UPDATE statements filtering
"organization_code" and "tenant_code" can use an index; ensure the migration
references the table name user_extensions and the same column names used in the
queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75ce20a6-e951-47d1-8b53-5941a63c4c3a
📒 Files selected for processing (4)
src/controllers/v1/org-admin.jssrc/database/migrations/20260525000001-backfill-org-codes-from-ids.jssrc/database/queries/mentorExtension.jssrc/database/queries/userExtension.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/controllers/v1/org-admin.js
- src/database/migrations/20260525000001-backfill-org-codes-from-ids.js
… rebuild - Guard visible_to_organizations.includes() with || [] in checkIfMentorIsAccessible (ASSOCIATED and ALL cases) to avoid TypeError when the column is null on old rows - Rebuild materialized views at the end of PR2 rename migration (up and down): visible_to_organizations changed type from integer[] to varchar[] so existing views must be replaced; uses triggerViewBuildForAllTenants() for zero-downtime swap Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai full review the PR and see if any changes are needed to be done , if not approve the changes from your side. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Recent commits ==="
gh api "repos/ELEVATE-Project/mentoring/pulls/1644/commits" | jq -r '.[] | "\(.sha[0:10]) \(.commit.author.date) \(.commit.message | split("\n")[0])"'
echo ""
echo "=== `#6`: information_schema table_schema filter in migration ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public\|table_name\|column_name" | head -20
echo ""
echo "=== Full columnExists / information_schema sections ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '1,40p'
echo ""
echo "=== `#7`: JSDoc in org-admin service updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -B2 -A20 "updateRelatedOrgs" | head -60
echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js - main() guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -25
echo ""
echo "=== dropOrgNumericBackupColumns.js - check main call ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "main\(\|require\.main\|module\.exports\|module ==" | head -10
echo ""
echo "=== Cache helper - orgIdCode namespace ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/generics/cacheHelper.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "orgIdCode\|defaultTtl\|TTL\|86400" | head -20
echo ""
echo "=== convertOrgIdsToOrgCodes - full implementation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/helpers/orgUtils.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d
echo ""
echo "=== sessions.js queries - mentor_organization_code index check ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/sessions.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "mentor_organization_code\|mentor_organization_id\|visible_to_organizations" | head -30
echo ""
echo "=== check if any index migration exists for new columns ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/?ref=${HEAD_SHA}" | jq -r '.[].name' | grep -i "index\|20260525"
echo ""
echo "=== PR files changed ==="
gh api "repos/ELEVATE-Project/mentoring/pulls/1644/files" | jq -r '.[].filename'Length of output: 13867
✅ Full Review StatusCritical Issues — All Fixed ✅
Non-Blocking Items — Still Open (follow-up PR recommended)
-- Current (could match tables in other schemas):
WHERE table_name = 'sessions' AND column_name = 'mentor_organization_code'
-- Recommended:
WHERE table_schema = 'public' AND table_name = 'sessions' AND column_name = 'mentor_organization_code'
* `@param` {Array} relatedOrgs - Array of related organization passed // ← old
* `@param` {Number} orgId - Specific orgId which was updated // ← oldShould reflect the new org-code based signature.
main() // ← executes immediately on require()
.then(() => process.exit(0))
.catch(...)Recommend wrapping: if (require.main === module) {
main().then(() => process.exit(0)).catch((err) => { console.error(err); process.exit(1) })
}📊 Performance Analysis1. Eliminated DB Round-Trips in
|
| Factor | Impact |
|---|---|
| String comparison cost | Marginally higher than integer, negligible in practice |
| Storage per row | Slightly larger (avg org code ~10 chars vs 4-byte int), negligible |
| GIN index compatibility | String arrays index just as well as integer arrays — no degradation |
| Join elimination | ✅ No join to organisation_extension needed to resolve display codes |
Net effect: Neutral-to-slightly-positive. The join elimination outweighs the tiny string comparison overhead.
4. One Missing Optimization — Index on mentor_organization_code
The PR adds sessions.mentor_organization_code as a new column and queries it in filterSessionsBasedOnSaasPolicy (via SQL WHERE mentor_organization_code = ...), but no index migration was added for this column. For production tables with many sessions, sequential scans on this column could be slow.
Recommendation (follow-up): Add a migration:
CREATE INDEX CONCURRENTLY idx_sessions_mentor_org_code
ON sessions (mentor_organization_code)
WHERE mentor_organization_code IS NOT NULL;Summary Table
| Change | DB Queries Saved | Latency Reduction |
|---|---|---|
getSessionTenantCode direct code read |
1 per session-tenant call | ~5–20ms per call |
convertOrgIdsToOrgCodes cache hits |
N–1 per request (N org IDs) | ~10–50× vs repeated DB joins |
| Deduplication in batch fetches | Up to N-1 queries | Proportional to duplicate rate |
| Kafka cache invalidation | N/A — correctness | Prevents stale-code bugs |
All critical and blocking issues are resolved. The three remaining items (#6, #7, #9) are cosmetic/defensive and suitable for a follow-up. The index on mentor_organization_code is worth adding soon if query volume warrants it.
Approving the PR. 🐇 ✨
[approve]
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/database/migrations/20260525000002-rename-visible-to-org-codes.js (1)
21-49: ⚡ Quick winConsider wrapping the column renames in a transaction.
PostgreSQL DDL is transactional, but each
renameColumnhere runs in its own implicit transaction. If, say, the firstuser_extensionsrename succeeds and the second fails, the schema is left half-migrated and re-running the migration throws on the already-renamed columns. Wrapping the renames in a single transaction makes the swap atomic and keeps retries clean. Keep the view rebuild outside the transaction (it's long-running and best-effort with the explicit failure check above).♻️ Proposed refactor for up() (mirror in down())
- // sessions: park old column as backup, promote new column to primary name - await queryInterface.renameColumn('sessions', 'visible_to_organizations', 'visible_to_organizations_numeric') - await queryInterface.renameColumn('sessions', 'visible_to_organization_codes', 'visible_to_organizations') - console.log( - 'sessions: visible_to_organizations (numeric) → visible_to_organizations_numeric (backup); visible_to_organization_codes → visible_to_organizations' - ) - - // user_extensions: same swap - await queryInterface.renameColumn( - 'user_extensions', - 'visible_to_organizations', - 'visible_to_organizations_numeric' - ) - await queryInterface.renameColumn( - 'user_extensions', - 'visible_to_organization_codes', - 'visible_to_organizations' - ) - console.log( - 'user_extensions: visible_to_organizations (numeric) → visible_to_organizations_numeric (backup); visible_to_organization_codes → visible_to_organizations' - ) + await queryInterface.sequelize.transaction(async (transaction) => { + // sessions: park old column as backup, promote new column to primary name + await queryInterface.renameColumn('sessions', 'visible_to_organizations', 'visible_to_organizations_numeric', { transaction }) + await queryInterface.renameColumn('sessions', 'visible_to_organization_codes', 'visible_to_organizations', { transaction }) + // user_extensions: same swap + await queryInterface.renameColumn('user_extensions', 'visible_to_organizations', 'visible_to_organizations_numeric', { transaction }) + await queryInterface.renameColumn('user_extensions', 'visible_to_organization_codes', 'visible_to_organizations', { transaction }) + }) + console.log('Renamed visible_to_organizations columns for sessions and user_extensions')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/migrations/20260525000002-rename-visible-to-org-codes.js` around lines 21 - 49, Wrap the series of renameColumn calls inside the migration's up (and mirror the same change in down) within a single transaction so the swaps are atomic: begin a transaction via queryInterface.sequelize.transaction and perform all queryInterface.renameColumn calls for 'sessions' and 'user_extensions' inside that transaction, committing on success and rolling back on error; leave materializedViewsService.triggerViewBuildForAllTenants() and the surrounding console logs outside the transaction (run it after commit) so the long-running view rebuild remains best-effort.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/database/migrations/20260525000002-rename-visible-to-org-codes.js`:
- Around line 46-48: The migration's up() and down() call
materializedViewsService.triggerViewBuildForAllTenants() but ignore its {
success: false, ... } return shape so migrations can silently succeed even when
rebuilds fail; update both up() and down() in
20260525000002-rename-visible-to-org-codes.js to capture the result of
materializedViewsService.triggerViewBuildForAllTenants(), check result.success,
and if false throw an Error (or rethrow result.error) so the migration fails and
stops when view rebuilds do not succeed.
---
Nitpick comments:
In `@src/database/migrations/20260525000002-rename-visible-to-org-codes.js`:
- Around line 21-49: Wrap the series of renameColumn calls inside the
migration's up (and mirror the same change in down) within a single transaction
so the swaps are atomic: begin a transaction via
queryInterface.sequelize.transaction and perform all queryInterface.renameColumn
calls for 'sessions' and 'user_extensions' inside that transaction, committing
on success and rolling back on error; leave
materializedViewsService.triggerViewBuildForAllTenants() and the surrounding
console logs outside the transaction (run it after commit) so the long-running
view rebuild remains best-effort.
🪄 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: 20629be0-94be-4477-8cec-68fab37b15fe
📒 Files selected for processing (2)
src/database/migrations/20260525000002-rename-visible-to-org-codes.jssrc/services/mentors.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/mentors.js
…ode indexes - Wrap all four renameColumn calls in migration 00002 in a single transaction so a partial failure rolls back the entire column swap atomically, keeping the schema consistent and the migration safely re-runnable - Check triggerViewBuildForAllTenants() return value and throw on failure so the migration surfaces view rebuild errors instead of silently succeeding - Add migration 00003: composite indexes idx_user_extensions_tenant_org_code (tenant_code, organization_code) and idx_sessions_tenant_mentor_org_code (tenant_code, mentor_organization_code) to back the addVisibleToOrg UPDATE predicates and session queries on the new column without full table scans Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai do a final review and provide your approval or disapproval. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "Current HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Recent commits ==="
gh api "repos/ELEVATE-Project/mentoring/pulls/1644/commits" | jq -r '.[] | "\(.sha[0:10]) \(.commit.author.date) \(.commit.message | split("\n")[0])"'
echo ""
echo "=== `#1` `#2`: Migration - pgArr and VALUES escaping ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '45,100p'
echo ""
echo "=== `#3`: mentorExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[\|replace\|varchar" | head -20
echo ""
echo "=== `#4`: userExtension.js - ARRAY literals / bind params ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\[\|\\\$[0-9]\|bind\|varchar\|replace" | head -20
echo ""
echo "=== `#5`: Controller org-admin - orgCode guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A5 "orgCode\|failureResponse\|ORGANIZATION_NOT_FOUND" | head -30
echo ""
echo "=== `#6`: information_schema table_schema ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public" | head -10
echo ""
echo "=== `#7`: JSDoc in org-admin service ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A20 "updateRelatedOrgs" | head -50
echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js - main() guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "main\(\|require\.main\|module ==" | head -10
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -15
echo ""
echo "=== PR review state ==="
gh api repos/ELEVATE-Project/mentoring/pulls/1644/reviews | jq -r '.[] | "\(.id) \(.state) \(.user.login) \(.submitted_at)"'Length of output: 12469
✅ Final Review — All Blocking Issues Confirmed Fixed
The latest commit also adds org-code indexes, which directly addresses the missing index concern I flagged in the performance analysis — a nice improvement for query performance on 🟡 Non-Blocking Remaining (follow-up PR)
These are all cosmetic/defensive and do not affect correctness or production safety. Verdict: ✅ Approved. The migration is architecturally sound, all SQL injection/data-corruption risks are resolved, and the latest commit adds further defensive improvements. The three remaining items can be cleaned up in a follow-up. 🐇 ✨ [approve] |
1 participant
Release Notes
orgIdCodefor mapping organization IDs to codes with Kafka-based cache invalidationconvertOrgIdsToOrgCodes()helper for efficient organization code resolution with cachingdropOrgNumericBackupColumns.js) to clean up backup columns after migration completionContribution Summary