fix: update rawSelect queries to use correct identifiers for tenants …#825
fix: update rawSelect queries to use correct identifiers for tenants …#825nevil-mathew merged 1 commit intodevelopfrom
Conversation
WalkthroughThe migrations adjust existence checks to use codes instead of ids, remove organization_features seeding from the default tenants/domains migration, and keep transactional behavior intact. A separate migration updates organization_features existence checks to use feature_code while preserving insert logic and error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Up as Migration: create-default-tenants-and-domains (up)
participant DB as Database
Up->>DB: SELECT tenant_code FROM tenants WHERE tenant_code=...
alt tenant not found
Up->>DB: INSERT INTO tenants (...)
end
Up->>DB: SELECT tenant_code FROM tenant_domains WHERE tenant_code=...
alt domain not found
Up->>DB: INSERT INTO tenant_domains (...)
end
note over Up,DB: Previous seeding of organization_features is removed
sequenceDiagram
autonumber
participant Up as Migration: add-organization-features (up)
participant DB as Database
Up->>DB: SELECT feature_code FROM organization_features WHERE organization_id=... AND tenant_code=... AND feature_code=...
alt not exists
Up->>DB: INSERT INTO organization_features (organization_id, tenant_code, feature_code, ...)
else exists
note over Up: Skip insert
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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
🧹 Nitpick comments (3)
src/database/migrations/20250916094307-add-organization-features.js (1)
63-63: Preserve 0 in display_order.
|| nullwill turn 0 into null. Use nullish coalescing.- display_order: f.display_order || null, + display_order: f.display_order ?? null,src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (2)
50-52: Confirm intended semantics for tenant_domains existence.Checking by
tenant_codeonly skips seeding 'localhost' if any domain exists for the tenant. If you intended to seed only when 'localhost' is missing, check that domain explicitly.- const existingDomain = await queryInterface.rawSelect( - 'tenant_domains', - { where: { tenant_code: DEFAULT_TENANT_CODE }, transaction: t }, - 'tenant_code' - ) + const existingDomain = await queryInterface.rawSelect( + 'tenant_domains', + { where: { tenant_code: DEFAULT_TENANT_CODE, domain: 'localhost' }, transaction: t }, + 'domain' + )
113-121: Down migrates organization_features despite Up no longer seeding here.This can delete rows inserted by the separate migration or cause cross-migration coupling. Consider aligning down with current up.
- // Delete seeded organization_features for this org & tenant - await queryInterface.bulkDelete( - 'organization_features', - { organization_code: DEFAULT_ORG_CODE, tenant_code: DEFAULT_TENANT_CODE }, - { transaction: t } - )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js(2 hunks)src/database/migrations/20250916094307-add-organization-features.js(1 hunks)
🧰 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/migrations/20250506091446-create-deafult-tenants-and-domains.jssrc/database/migrations/20250916094307-add-organization-features.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/20250916094307-add-organization-features.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/20250916094307-add-organization-features.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/20250916094307-add-organization-features.js
🧬 Code graph analysis (1)
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (1)
src/database/migrations/20250916094307-add-organization-features.js (4)
DEFAULT_TENANT_CODE(8-8)DEFAULT_TENANT_CODE(82-82)t(6-6)t(81-81)
🔇 Additional comments (3)
src/database/migrations/20250916094307-add-organization-features.js (2)
50-51: Good: existence check now uses feature_code.This aligns with code-based identifiers and avoids id dependency. Looks correct.
8-10: Repo consistently uses British spelling for env vars — confirm your env/deploy configs export DEFAULT_ORGANISATION_CODE (not DEFAULT_ORGANIZATION_CODE)Search found only DEFAULT_ORGANISATION_* across the codebase (e.g., src/envVariables.js, migrations, services); no DEFAULT_ORGANIZATION_* occurrences — verify your .env / CI/CD / deployment config defines DEFAULT_ORGANISATION_CODE to avoid silent no-ops.
src/database/migrations/20250506091446-create-deafult-tenants-and-domains.js (1)
18-19: Good: tenant existence now keyed by code.Using
'code'with rawSelect is consistent with identifier-based checks.
1 participant
…and features
Summary by CodeRabbit
Bug Fixes
Chores
Impact