Refactor default tenant/domain migration with safety improvements#821
Refactor default tenant/domain migration with safety improvements#821nevil-mathew merged 2 commits intodevelopfrom
Conversation
|
Warning Rate limit exceeded@nevil-mathew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 34 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 (2)
WalkthroughRemoves an earlier default-tenant seeding migration, adds a new consolidated migration that seeds tenant, domain, organization, and organization_features idempotently, introduces a separate migration to seed organization_features from features, refactors the org-code normalization migration (FK handling and signatures), and updates a seeder to include organization_code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Migration (20250506091446)
participant DB as DB (queryInterface)
rect rgba(180,220,255,0.25)
note over Dev,DB: Up: Seed default tenant/domain/org/org_features (idempotent)
Dev->>DB: BEGIN TRANSACTION
Dev->>DB: SELECT tenant by DEFAULT_TENANT_CODE
alt Tenant missing
Dev->>DB: INSERT tenants (...)
end
Dev->>DB: SELECT tenant_domain localhost
alt Domain missing
Dev->>DB: INSERT tenant_domains (...)
end
Dev->>DB: SELECT organization by DEFAULT_ORG_CODE
alt Org missing
Dev->>DB: INSERT organizations (...)
end
Dev->>DB: SELECT features
loop For each feature
Dev->>DB: SELECT org_feature (org_code, feature_code, tenant_code)
alt Missing link
Dev->>DB: INSERT organization_features (...)
end
end
Dev-->>DB: COMMIT
end
rect rgba(255,210,180,0.25)
note over Dev,DB: Down: Remove seeded rows
Dev->>DB: BEGIN TRANSACTION
Dev->>DB: DELETE organization_features (org_code, tenant_code)
Dev->>DB: DELETE organizations (code)
Dev->>DB: DELETE tenant_domains (tenant_code)
Dev->>DB: DELETE tenants (code)
Dev-->>DB: COMMIT
end
sequenceDiagram
autonumber
actor Dev as Migration (20250916094307)
participant DB as DB (queryInterface)
rect rgba(200,255,200,0.25)
note over Dev,DB: Up: Seed organization_features from features
Dev->>DB: BEGIN TRANSACTION
Dev->>DB: SELECT organization by DEFAULT_ORG_CODE
alt Org not found
Dev-->>DB: COMMIT
note right of Dev: Exit early
else Org found
Dev->>DB: SELECT all features
alt No features
Dev-->>DB: COMMIT
else
loop For each feature
Dev->>DB: SELECT org_feature (org_code, feature_code, tenant_code)
alt Missing
Dev->>DB: Prepare row for bulk insert
end
end
Dev->>DB: BULK INSERT organization_features (new rows)
Dev-->>DB: COMMIT
end
end
end
rect rgba(255,230,200,0.25)
note over Dev,DB: Down: Clean up links
Dev->>DB: BEGIN TRANSACTION
Dev->>DB: DELETE organization_features (DEFAULT or 'default' org/tenant)
Dev-->>DB: COMMIT
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/database/seeders/20230802144103-add-entity-and-entity-types.js (3)
171-183: Bug: entity label is being overwritten with tenant code.This line mutates each entity and sets
label = DEFAULT_TENANT_CODE, corrupting seed data (e.g., "English" becomes "default"). Also avoids cloning, so you mutate source templates in-place.Apply this diff to fix by cloning and preserving label:
- entitiesArray[eachType.value].forEach((eachEntity) => { - eachEntity.entity_type_id = eachType.id - eachEntity.type = 'SYSTEM' - eachEntity.status = 'ACTIVE' - eachEntity.tenant_code = process.env.DEFAULT_TENANT_CODE - eachEntity.organization_code = process.env.DEFAULT_ORGANISATION_CODE - eachEntity.created_at = new Date() - eachEntity.updated_at = new Date() - eachEntity.created_by = 0 - ;(eachEntity.label = process.env.DEFAULT_TENANT_CODE), entitiesFinalArray.push(eachEntity) - }) + entitiesArray[eachType.value].forEach((eachEntity) => { + const row = { + ...eachEntity, // keep original label/value + entity_type_id: eachType.id, + type: 'SYSTEM', + status: 'ACTIVE', + tenant_code: DEFAULT_TENANT_CODE, + organization_code: DEFAULT_ORG_CODE, + created_at: new Date(), + updated_at: new Date(), + created_by: 0, + updated_by: 0, + } + entitiesFinalArray.push(row) + })
2-7: Validate and bind env vars once; fail fast if missing.Seeding will insert NULLs if envs aren’t set. Capture and validate them up-front; reuse variables instead of
process.envinline to prevent drift.up: async (queryInterface, Sequelize) => { const defaultOrgId = queryInterface.sequelize.options.defaultOrgId if (!defaultOrgId) { throw new Error('Default org ID is undefined. Please make sure it is set in sequelize options.') } + const DEFAULT_TENANT_CODE = process.env.DEFAULT_TENANT_CODE + const DEFAULT_ORG_CODE = process.env.DEFAULT_ORGANISATION_CODE + if (!DEFAULT_TENANT_CODE || !DEFAULT_ORG_CODE) { + throw new Error('DEFAULT_TENANT_CODE and DEFAULT_ORGANISATION_CODE must be set for this seeder.') + }Also replace inline usages:
- organization_code: process.env.DEFAULT_ORGANISATION_CODE, - tenant_code: process.env.DEFAULT_TENANT_CODE, + organization_code: DEFAULT_ORG_CODE, + tenant_code: DEFAULT_TENANT_CODE,and
- eachEntity.tenant_code = process.env.DEFAULT_TENANT_CODE - eachEntity.organization_code = process.env.DEFAULT_ORGANISATION_CODE + eachEntity.tenant_code = DEFAULT_TENANT_CODE + eachEntity.organization_code = DEFAULT_ORG_CODE
165-167: Scope the entity_types lookup to the default org/tenant.
SELECT * FROM entity_typesrisks binding to rows from other orgs and duplicating entities. Filter by org/tenant or by the ids you just inserted.- const entityTypes = await queryInterface.sequelize.query('SELECT * FROM entity_types', { - type: queryInterface.sequelize.QueryTypes.SELECT, - }) + const entityTypes = await queryInterface.sequelize.query( + 'SELECT id, value FROM entity_types WHERE organization_id = :orgId AND tenant_code = :tenant', + { + type: queryInterface.sequelize.QueryTypes.SELECT, + replacements: { orgId: defaultOrgId, tenant: DEFAULT_TENANT_CODE }, + } + )
🧹 Nitpick comments (7)
src/database/seeders/20230802144103-add-entity-and-entity-types.js (1)
134-164: Wrap seeding in a transaction for atomicity.Two bulk inserts + read/compose without a transaction can leave partial data on failure.
If desired, I can provide a patch to wrap the inserts and SELECTs in a single transaction.
src/database/migrations/20250729064710-org-code-fix.js (1)
136-141: Assert post-update normalization succeeded.You fetch remaining offenders but ignore the result. Fail fast if any rows still contain spaces/uppercase.
- const fetchOrgs = await queryInterface.sequelize.query(ORG_FETCH_QUERY, { + const fetchOrgs = await queryInterface.sequelize.query(ORG_FETCH_QUERY, { type: QueryTypes.SELECT, raw: true, transaction, }) + if (fetchOrgs.length > 0) { + throw new Error(`Organization code normalization incomplete for ${fetchOrgs.length} row(s).`) + }src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (3)
78-99: Remove unused variable and stale comment.
sinsertedOrgCodeis never read; the comment hints at an id fetch that doesn’t happen.- let insertedOrgCode = DEFAULT_ORG_CODE - - if (!existingOrgId) { + if (!existingOrgId) { await queryInterface.bulkInsert( 'organizations', [ { name: DEFAULT_ORG_NAME, code: DEFAULT_ORG_CODE, description: 'Default Organisation', status: 'ACTIVE', tenant_code: DEFAULT_TENANT_CODE, created_at: now, updated_at: now, }, ], { transaction: t } ) - - // get the id just inserted (DB may have returned id via serial; rawSelect by code ensures we get it) }
1-1: Filename typo: “deafult”.Consider renaming to “create-default-tenants-and-domains.js” for clarity (ack: renaming migrations affects order; do only if safe).
100-141: Avoid duplicate feature seeding across two migrations.This migration and 20250916094307 both seed
organization_features. It’s idempotent but redundant.Pick one authoritative migration (prefer the consolidated one) or guard the latter behind an existence check on this file’s timestamp/applied status.
src/database/migrations/20250916094307-add-organization-features.js (2)
38-66: Set-based insert will be faster and simpler.Looping + many
rawSelectcalls is N+1. Prefer a single insert-select withON CONFLICT DO NOTHING(if a unique constraint exists).Example (Postgres):
INSERT INTO organization_features (organization_code, feature_code, enabled, feature_name, icon, tenant_code, created_at, updated_at, display_order) SELECT :org, f.code, TRUE, f.label, f.icon, :tenant, NOW(), NOW(), f.display_order FROM features f LEFT JOIN organization_features of ON of.feature_code = f.code AND of.organization_code = :org AND of.tenant_code = :tenant WHERE of.id IS NULL;Happy to translate this to a
queryInterface.sequelize.querycall.Also applies to: 68-70
1-99: Redundant with create-default-tenants-and-domains seeding.Coordinate with 20250506091446 to avoid duplication; keep only one features seeding path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/database/migrations/20250502091425-create-deafult-tenants-and-domains.js(0 hunks)src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js(1 hunks)src/database/migrations/20250729064710-org-code-fix.js(1 hunks)src/database/migrations/20250916094307-add-organization-features.js(1 hunks)src/database/seeders/20230802144103-add-entity-and-entity-types.js(2 hunks)
💤 Files with no reviewable changes (1)
- src/database/migrations/20250502091425-create-deafult-tenants-and-domains.js
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#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
PR: ELEVATE-Project/user#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/seeders/20230802144103-add-entity-and-entity-types.jssrc/database/migrations/20250729064710-org-code-fix.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#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/database/migrations/20250729064710-org-code-fix.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#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/database/migrations/20250729064710-org-code-fix.js
📚 Learning: 2025-09-12T10:35:28.285Z
Learnt from: praveenKDass
PR: ELEVATE-Project/user#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/database/migrations/20250729064710-org-code-fix.js
🧬 Code graph analysis (4)
src/database/migrations/20250916094307-add-organization-features.js (2)
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (9)
now(5-5)t(6-6)t(152-152)DEFAULT_TENANT_CODE(9-9)DEFAULT_TENANT_CODE(153-153)DEFAULT_ORG_CODE(10-10)DEFAULT_ORG_CODE(154-154)features(101-103)exists(111-122)src/database/models/index.js (1)
process(6-6)
src/database/seeders/20230802144103-add-entity-and-entity-types.js (1)
src/database/models/index.js (1)
process(6-6)
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (1)
src/database/migrations/20250916094307-add-organization-features.js (9)
now(5-5)t(6-6)t(81-81)DEFAULT_TENANT_CODE(8-8)DEFAULT_TENANT_CODE(82-82)DEFAULT_ORG_CODE(9-9)DEFAULT_ORG_CODE(83-83)features(26-29)exists(40-51)
src/database/migrations/20250729064710-org-code-fix.js (1)
src/scripts/deleted-org-data-clean-up/clean.js (1)
require(1-1)
🔇 Additional comments (2)
src/database/seeders/20230802144103-add-entity-and-entity-types.js (1)
146-148: Ensure org_id and org_code refer to the same organization.Mixing
organization_id(from sequelize options) withorganization_code(from env) can mismatch if configs drift.Confirm that
queryInterface.sequelize.options.defaultOrgIdbelongs toDEFAULT_ORGANISATION_CODE. If unsure, gate the seeder with a sanity check comparing(id, code)fromorganizations.Also applies to: 178-178
src/database/migrations/20250729064710-org-code-fix.js (1)
29-57: Resolved — composite FKs to organizations(code, tenant_code) are valid.Verified a UNIQUE/PK on (code, tenant_code) and that child tables include organization_code + tenant_code, so the composite FKs are supported. Evidence: src/database/migrations/20250506091445-update-user-org-tables.js (adds unique_org_code_per_tenant and adjusts PK to include tenant_code), src/database/migrations/20250623073422-organizations-normalize-registration-code.js (index + fk using (organization_code, tenant_code) -> organizations(code, tenant_code)), src/database/migrations/20250626180047-add-pending-realtions.js (adds composite FKs for organization_user_invites, organization_features, etc.), and src/database/migrations/20250603104503-update-table-organization_user_invites.js (organization_code + tenant_code present in constraints).
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js
Show resolved
Hide resolved
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js
Show resolved
Hide resolved
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js
Outdated
Show resolved
Hide resolved
src/database/migrations/20250916094307-add-organization-features.js
Outdated
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores