fix[validation]: organization deactivation validation and error messages(BUG-3834) #831
Conversation
WalkthroughAdds three new English locale strings for organization deactivation errors, introduces a new validator for deactivateOrg that checks route param and tenant header, and updates the admin service deactivateOrg flow to pre-check org existence/status and use updated error keys while keeping downstream deactivation steps unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant V as Validator (v1/admin)
participant S as AdminService.deactivateOrg
participant O as OrgRepo
participant U as UserService
participant SS as SessionService
participant E as EventBus
C->>V: Request: deactivateOrg(id, tenant-id)
V-->>C: 400 if params/headers invalid
V->>S: Valid request
rect rgba(230,240,255,0.6)
note over S,O: Pre-checks (new)
S->>O: findByCodeAndTenant(id, tenant-id)
O-->>S: org | null
alt org not found
S-->>C: Error: ORG_NOT_FOUND
else org inactive
S-->>C: Error: ORG_ALREADY_INACTIVE
else proceed
S->>O: update status -> inactive
O-->>S: ok | fail
alt update failed
S-->>C: Error: ORG_DEACTIVATION_FAILED
else success
S->>U: Deactivate users in org
U-->>S: ok
S->>SS: End active sessions
SS-->>S: ok
S->>E: Broadcast deactivation event
E-->>S: ack
S-->>C: Success
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validators/v1/admin.js (1)
8-10: Fix runtime ReferenceError: import body from express-validatorThe login validator chains use body(...) but it is not imported. This will throw at runtime.
Apply this diff:
const filterRequestBody = require('../common') const { admin } = require('@constants/blacklistConfig') +const { body } = require('express-validator')
🧹 Nitpick comments (3)
src/validators/v1/admin.js (2)
101-107: Align phone pattern used in conditional with the one used aboveYou validate phone as 7–15 digits earlier but use 6–15 digits here for the conditional. Make them consistent:
- .if(body('identifier').custom((value) => /^\d{6,15}$/.test(value))) // Apply only for phone identifiers + .if(body('identifier').custom((value) => /^\d{7,15}$/.test(value))) // Apply only for phone identifiers
151-157: Unify tenant-id header validation across validatorsaddOrgAdmin checks only isString, while deactivateOrg additionally pattern-matches. Consider extracting a shared helper to validate tenant-id once to avoid drift.
Example:
- helpers/validators.js: export validateTenantId(req) that applies isString + /^[a-z0-9_-]+$/.
- Use in all validators that touch tenant-id.
src/locales/en.json (1)
88-90: Standardize spelling: use “Organization” consistentlyElsewhere keys predominantly use “Organization”. Recommend updating new strings for consistency.
Apply this diff:
- "ORG_DEACTIVATION_FAILED": "Failed to deactivate organisation.", - "ORG_NOT_FOUND": "Organisation not found.", - "ORG_ALREADY_INACTIVE": "Organisation is already inactive.", + "ORG_DEACTIVATION_FAILED": "Failed to deactivate organization.", + "ORG_NOT_FOUND": "Organization not found.", + "ORG_ALREADY_INACTIVE": "Organization is already inactive.",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/locales/en.json(1 hunks)src/services/admin.js(2 hunks)src/validators/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/admin.js
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/validators/v1/admin.jssrc/services/admin.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/validators/v1/admin.jssrc/services/admin.js
🔇 Additional comments (2)
src/validators/v1/admin.js (2)
167-179: Broaden id and tenant-id validation
- id: allow numbers or lowercase slug with hyphens/underscores (
^(?:\d+|[a-z0-9_-]+)$) and update the message to “id must be a number or lowercase alphanumeric string with hyphens/underscores.”- tenant-id: enforce
.isString(), allow hyphens/underscores (/^[a-z0-9_-]+$/), and update its message.- Verify if numeric org IDs are ever passed to
deactivateOrg; if they aren’t, adjust the message to reference org codes only.
167-179: deactivateOrg update result check is correct TheorganizationQueries.updatecall indeactivateOrgdoesn’t use{returning: true, raw: true}, so it returns a numeric count andif (orgRowsAffected === 0)correctly handles failures.
Summary by CodeRabbit
New Features
Bug Fixes