Citus issue fix - remove the relations #867
Conversation
… for tenant validation
|
Warning Rate limit exceeded@priyanka-TL has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughModifies foreign-key behavior and migration error handling for feature_role_mapping, removes a Feature → FeatureRoleMapping association, and updates a service call parameter in the account service. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/account.js (1)
1049-1098: FixfindUserWithOrganizationcall: incorrect parameter type and missing tenant filteringThe third argument should be a boolean (raw flag), not
tenantDetail.code. The function signature isfindUserWithOrganization(filter, options = {}, raw = false)whererawcontrols output transformation, not tenant filtering. Tenant filtering is handled byfilter.tenant_code, which must be included in the filter object.Change line 1049 from:
const user = await userQueries.findUserWithOrganization(query, {}, tenantDetail.code)To include
tenant_codein the filter and use the correct boolean parameter:const user = await userQueries.findUserWithOrganization({...query, tenant_code: tenantDetail.code}, {})Without
tenant_codein the filter, the function's tenant isolation checks fail (accessing undefinedfilter.tenant_codein WHERE clauses).
🧹 Nitpick comments (3)
src/database/models/TenantDomain.js (1)
48-54: Tenant–TenantDomain association wiring looks correct; consider explicit cascade optionsThe belongsTo setup (foreignKey
tenant_code, targetKeycode, aliastenant) matches the new include usage and should enabledomainWithTenant.tenantcleanly. You may want to add explicitonDelete/onUpdatesemantics (e.g.,RESTRICTorCASCADE) to avoid accidental orphaning or unexpected tenant reassignment when tenants change.src/services/account.js (1)
658-699: Combined domain+tenant lookup in login looks good; note behavior on query failuresUsing
tenantDomainQueries.findOneWithTenant({ domain })and then validatingdomainWithTenant.tenantwith an ACTIVE status is a solid improvement and avoids an extra round-trip. Be aware that, since the query helper returns errors instead of throwing, any underlying DB/ORM error here will be surfaced to the user asTENANT_NOT_FOUND_PING_ADMINrather than a 5xx; that matches the existing pattern in this service, but if you want clearer operational signals you might consider standardizing on throwing from query helpers later.src/database/queries/tenantDomain.js (1)
2-63: NewfindOneWithTenanthelper correctly models domain→tenant join; consider future options passthroughImporting both
TenantDomainandTenantand using an include with aliastenantaligns with the model association and thedomainWithTenant.tenantaccess inaccount.login. The LEFT JOIN plusresult.get({ plain: true })is appropriate for returning a simple object while still allowing “domain found but tenant missing” handling. Right now this helper only respectsattributesandtenantAttributesfromoptions; if you later need to pass things like transactions,paranoid, orlogging, you may want to selectively spread additional safe options into the query.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/database/models/TenantDomain.js(1 hunks)src/database/queries/tenantDomain.js(2 hunks)src/services/account.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/account.js
src/database/queries/**
⚙️ CodeRabbit configuration file
Review database queries for performance. Check for N+1 problems and ensure indexes can be used.
Files:
src/database/queries/tenantDomain.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
📚 Learning: 2025-09-29T08:17:56.193Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 827
File: src/validators/v1/user.js:77-81
Timestamp: 2025-09-29T08:17:56.193Z
Learning: In src/validators/v1/user.js, the tenant_code field validation for the profileById function is intentionally required (not optional) for the /profileById endpoint, ensuring tenant_code must always be provided when fetching user profiles by ID.
Applied to files:
src/services/account.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/services/account.js
📚 Learning: 2025-07-31T08:43:35.971Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
Applied to files:
src/services/account.js
🧬 Code graph analysis (2)
src/services/account.js (2)
src/services/public.js (3)
tenantDetail(35-35)tenantDetail(80-83)common(14-14)src/services/admin.js (2)
tenantDetail(250-253)common(15-15)
src/database/queries/tenantDomain.js (1)
src/database/queries/tenants.js (1)
Tenant(2-2)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/services/account.js (2)
69-79: Tenant/domain join usage looks good; consider minor consistency tweaksThe new
findOneWithTenantusage and ACTIVE-status validation are correct and align the create flow with tenant lifecycle. Also good to seeportalURLnow driven from the tenant-domain record.Two small consistency nits you may want to address:
- Line 210: you use
domainWithTenant.tenant_codehere but elsewhere in this method you consistently usetenantDetail.code. Since these should always match, usingtenantDetail.codeeverywhere would reduce cognitive load and avoid future confusion iffindOneWithTenantever changes its shape.- Lines 570/586:
portalURLis now the rawdomainWithTenant.domain. If templates previously expected a fully qualified URL (e.g. including scheme or path), double‑check that this change doesn’t alter email/SMS link behavior. If they do, consider normalizing the value (e.g. adding a scheme) at a single helper instead of per‑callsite.Also applies to: 210-210, 570-571, 586-587
656-665: Login tenant validation is correct; consider reusing it across OTP/reset flowsThe switch to
findOneWithTenant({ domain })and enforcingtenantDetail.status === ACTIVEbefore login is a solid improvement and matches the PR’s goal of tenant validation. The commentedisUsernamehelper is also fine since the fallback branch already treats “not email/phone” as username.To avoid inconsistent behavior across flows:
generateOtpandresetPasswordstill only verify that a tenant exists, not that it’s ACTIVE. You might want to reuse the samedomainWithTenant+ ACTIVE-status check there so suspended tenants can’t request OTPs or reset passwords while being blocked at login/create.Also applies to: 672-672
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/account.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/account.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
📚 Learning: 2025-09-29T08:17:56.193Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 827
File: src/validators/v1/user.js:77-81
Timestamp: 2025-09-29T08:17:56.193Z
Learning: In src/validators/v1/user.js, the tenant_code field validation for the profileById function is intentionally required (not optional) for the /profileById endpoint, ensuring tenant_code must always be provided when fetching user profiles by ID.
Applied to files:
src/services/account.js
📚 Learning: 2025-07-31T08:43:35.971Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
Applied to files:
src/services/account.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/services/account.js
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/account.js (1)
1047-1055: FixfindUserWithOrganizationthird parameter ingenerateOtp—should be raw flag, not tenant codeHere you’re calling:
const user = await userQueries.findUserWithOrganization(query, {}, tenantDetail.code)but elsewhere in this file the same helper is used as:
const userInstance = await userQueries.findUserWithOrganization(query, {}, true) let user = userInstance ? userInstance.toJSON() : nulland in other call sites without a third argument at all. The third parameter is a boolean “raw / return instance” flag, not a tenant code; passing
tenantDetail.codehere is a type misuse and was already flagged in a previous review on this file.A consistent, safe pattern here would be:
- const user = await userQueries.findUserWithOrganization(query, {}, tenantDetail.code) + const userInstance = await userQueries.findUserWithOrganization(query, {}, true) + const user = userInstance ? userInstance.toJSON() : nullThe tenant scoping is already enforced via
query.tenant_code = tenantDetail.code, so no extra tenant parameter is needed.
🧹 Nitpick comments (3)
src/database/queries/tenantDomain.js (1)
2-62:findOneWithTenantquery shape looks good; consider passing through common Sequelize optionsThe new helper correctly uses a single
findOnewith anincludeonTenant(alias'tenant'), avoiding N+1 issues and matching the TenantDomain→Tenant association. Returningresult.get({ plain: true })also keeps the service layer simple.One gap vs the existing
findOnehelper is thatoptionsare not forwarded (aside fromattributes). Callers can’t currently passtransaction,paranoid,logging, etc., which may matter in flows that need transactional consistency.You can keep control of
includeandrawwhile still allowing the usual options by destructuring:-exports.findOneWithTenant = async (filter, options = {}) => { +exports.findOneWithTenant = async (filter, options = {}) => { try { + const { attributes, tenantAttributes, ...restOptions } = options const result = await TenantDomain.findOne({ where: filter, - attributes: options.attributes || undefined, + attributes: attributes || undefined, include: [ { model: Tenant, as: 'tenant', required: false, // LEFT JOIN to allow separate validation - attributes: options.tenantAttributes || [ + attributes: tenantAttributes || [ 'code', 'name', 'status', 'description', 'logo', 'meta', 'theming', ], }, ], - raw: false, // Need nested object structure + raw: false, // Need nested object structure + ...restOptions, // e.g., transaction, paranoid })This keeps the public API flexible without letting callers override
include/raw.src/services/account.js (2)
69-76: Domain→tenant validation increatelooks solid; tiny consistency tweak for tenant code usageUsing
tenantDomainQueries.findOneWithTenant({ domain })and then enforcingtenantDetail.status === ACTIVEbefore proceeding is a good tightening of the create flow. It also cleanly separates “unknown domain” vs “inactive tenant” error messages.
filterCondition.tenant_code = domainWithTenant.tenant_codeis correct, sincedomainWithTenantcomes from the same query; for readability you might standardize ontenantDetail.codeas the canonical source throughout this method:- const tenantDetail = domainWithTenant.tenant + const tenantDetail = domainWithTenant.tenant @@ - filterCondition.tenant_code = domainWithTenant.tenant_code + filterCondition.tenant_code = tenantDetail.codeFunctionally everything looks correct as-is; this is just for consistency.
Also applies to: 210-213
656-665: Login flow’s ACTIVE tenant check is correct; consider aligning generate/reset flowsThe login flow now:
- Resolves
domainWithTenantviafindOneWithTenant({ domain }).- Enforces
tenantDetail.status === common.ACTIVE_STATUSbefore looking up the user.This matches the intent of blocking logins for inactive tenants and is implemented correctly.
For consistency of tenant validation across account flows, consider applying the same
status: common.ACTIVE_STATUS/ active check ingenerateOtpandresetPasswordas well (those currently only check that a tenant exists, not that it’s ACTIVE). That would ensure deactivated tenants can’t continue to request OTPs or reset passwords unless that behavior is explicitly desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/database/queries/tenantDomain.js(2 hunks)src/services/account.js(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/account.js
src/database/queries/**
⚙️ CodeRabbit configuration file
Review database queries for performance. Check for N+1 problems and ensure indexes can be used.
Files:
src/database/queries/tenantDomain.js
🧠 Learnings (6)
📓 Common learnings
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
Repo: ELEVATE-Project/user PR: 816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In ELEVATE-Project/user admin.js, there's an inconsistency in organizationQueries.update usage: addOrgAdmin calls it without options (returns number), while deactivateOrg calls it with { returning: true, raw: true } (returns object). The validation logic 'if (orgRowsAffected === 0)' only works for the first case - the second case needs 'if (orgRowsAffected.rowsAffected === 0)' to properly validate updates.
Applied to files:
src/services/account.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
Repo: ELEVATE-Project/user PR: 816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when organizationQueries.update is called with { returning: true, raw: true }, it returns an object { rowsAffected, updatedRows }, not just a number. The validation logic in admin.js deactivateOrg method checking 'if (orgRowsAffected === 0)' is incorrect and should be 'if (orgRowsAffected.rowsAffected === 0)' to properly validate successful updates.
Applied to files:
src/services/account.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
Repo: ELEVATE-Project/user PR: 816
File: src/services/admin.js:704-704
Timestamp: 2025-09-12T10:35:28.285Z
Learning: In the ELEVATE-Project/user codebase, when using organizationQueries.update with { returning: true, raw: true } options, the method consistently returns a structured result with updatedRows property when successful. The initial check for orgRowsAffected === 0 already handles failed updates, so additional null checking for updatedRows is redundant when the code execution reaches the event emission logic.
Applied to files:
src/services/account.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/services/account.js
📚 Learning: 2025-09-29T08:17:56.193Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 827
File: src/validators/v1/user.js:77-81
Timestamp: 2025-09-29T08:17:56.193Z
Learning: In src/validators/v1/user.js, the tenant_code field validation for the profileById function is intentionally required (not optional) for the /profileById endpoint, ensuring tenant_code must always be provided when fetching user profiles by ID.
Applied to files:
src/services/account.js
🧬 Code graph analysis (2)
src/services/account.js (2)
src/controllers/v1/admin.js (2)
user(34-34)user(77-77)src/database/queries/users.js (1)
user(228-273)
src/database/queries/tenantDomain.js (2)
src/database/models/TenantDomain.js (1)
TenantDomain(3-46)src/database/queries/tenants.js (1)
Tenant(2-2)
🔇 Additional comments (1)
src/services/account.js (1)
562-575: UsingdomainWithTenant.domainforportalURLkeeps notifications aligned with the tenant domainSwitching the registration email/SMS
portalURLvariable todomainWithTenant.domainreuses the same canonical source you validated at the start ofcreate, which is preferable to relying on request input. No issues here.Also applies to: 578-591
…modify foreign key constraints in migrations
priyanka-TL
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/database/migrations/20251003155747-add-feature-role-mapping-constraints.js(1 hunks)src/database/migrations/20251211000000-modify-feature-role-mapping-fk-constraints.js(1 hunks)src/database/models/Feature.js(0 hunks)src/services/account.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/database/models/Feature.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/account.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
📚 Learning: 2025-07-31T08:43:35.971Z
Learnt from: nevil-mathew
Repo: ELEVATE-Project/user PR: 776
File: src/database/models/entityType.js:38-38
Timestamp: 2025-07-31T08:43:35.971Z
Learning: The migration for converting tenant_code to a primary key in the EntityType model was already handled in a previous PR, not in the current refactoring PR that focuses on organization codes instead of organization IDs.
Applied to files:
src/database/migrations/20251003155747-add-feature-role-mapping-constraints.jssrc/database/migrations/20251211000000-modify-feature-role-mapping-fk-constraints.js
🔇 Additional comments (2)
src/database/migrations/20251003155747-add-feature-role-mapping-constraints.js (1)
6-44: LGTM: Up migration with proper error handling.The try/catch pattern with re-throw ensures Sequelize properly tracks migration failure state. The mixed FK behaviors (NO ACTION for feature_code, CASCADE for composite keys) appear intentional for this Citus-related fix.
src/database/migrations/20251211000000-modify-feature-role-mapping-fk-constraints.js (1)
1-78: Error handling pattern is appropriate for migrations.The try/catch with re-throw ensures proper migration failure tracking. The defensive
.catch(() => {})combined withDROP CONSTRAINT IF EXISTSis redundant but harmless—provides extra safety across different PostgreSQL configurations.
… for tenant validation
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.