Refactor codebase to use organization codes instead of organization IDs #776
Conversation
…o feat/verify-entity-type-flow
WalkthroughThe changes refactor the codebase to shift from using organization IDs to organization codes for identifying organizations across controllers, services, database models, queries, and migrations. This includes updating method signatures, query filters, associations, and validation logic. Database migrations add and enforce new organization code fields and constraints, while validation rules are updated for stricter parameter requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Query
participant DB
Client->>Controller: Request (with decodedToken.organization_code)
Controller->>Service: Method call (organization_code, tenant_code, ...)
Service->>Query: Query (organization_code, tenant_code, ...)
Query->>DB: SQL (filter by organization_code, tenant_code)
DB-->>Query: Result
Query-->>Service: Data
Service-->>Controller: Response data
Controller-->>Client: API Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
coderabbitai
bot
changed the title
![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
🔭 Outside diff range comments (3)
src/database/queries/entityType.js (2)
36-36: Incorrect error handling - should throw instead of return.The error is being returned instead of thrown, which breaks the error handling pattern used throughout the class.
Apply this diff:
-return error +throw error
156-157: Incorrect error handling pattern in findAllEntityTypesAndEntities.Similar to other methods, errors should be thrown instead of returned.
Apply this diff:
-return error +throw errorsrc/services/entityType.js (1)
83-89: Fix incorrect error message for UniqueConstraintError.The error message "ENTITY_TYPE_ALREADY_DELETED" doesn't make sense for a UniqueConstraintError during an update operation. This should indicate a duplicate value issue.
} catch (error) { if (error instanceof UniqueConstraintError) { return responses.failureResponse({ - message: 'ENTITY_TYPE_ALREADY_DELETED', + message: 'ENTITY_TYPE_ALREADY_EXISTS', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) }
♻️ Duplicate comments (1)
src/services/user.js (1)
466-466: Consistent pattern with removeDefaultOrgEntityTypes.Similar to the
readmethod, this call toremoveDefaultOrgEntityTypesuses organization ID while the rest of the method uses organization codes, indicating the utility function may require organization ID.
🧹 Nitpick comments (7)
src/database/migrations/20250717071710-add-organization-code-to-entities-and-entity-types.js (3)
75-75: Remove console.log statements from production migration.Console logging in database migrations can clutter production logs and may not be captured properly in all deployment environments.
Consider removing these console.log statements or replacing them with proper migration logging if your framework supports it.
Also applies to: 117-117, 120-120
98-99: Clarify the strategy for removing organization_id column.The removal of
organization_idfromentity_typestable is commented out, which suggests a phased migration approach. This should be documented or tracked.Would you like me to create a follow-up issue to track the completion of this migration by uncommenting and executing the organization_id removal in a future release?
184-184: Inconsistent error message format in rollback.The error message uses
err.messagewhile the up migration uses the fullerrobject.Apply this diff for consistency:
-console.error('❌ Migration rollback failed:', err.message) +console.error('❌ Migration rollback failed:', err)src/database/queries/entityType.js (1)
161-174: Inconsistent module export pattern.The file uses a class-based export pattern but then adds a standalone function export at line 161. This mixing of patterns can be confusing.
Consider moving the
hardDeletefunction inside the class as a static method for consistency:-exports.hardDelete = async (id) => { +static async hardDelete(id) { try { await EntityType.destroy({ where: { id, }, force: true, }) return { success: true } } catch (error) { console.error(error) - return error + throw error } }src/services/account.js (1)
637-637: Remove console.log statements.Multiple console.log statements are present throughout the file which should be replaced with proper logging mechanism.
Replace console.log with a proper logger:
-console.log(error) +logger.error('Error in [method_name]:', error)Also applies to: 823-823, 1100-1100, 1520-1520, 1672-1672, 1819-1819, 1910-1910, 2009-2009
src/services/entityType.js (2)
20-25: Add parameter validation for organization identifiers.The method correctly incorporates both
organizationCodeandorganizationIdparameters, but consider adding validation to ensure these parameters are provided and valid before setting them onbodyData.static async create(bodyData, id, organizationCode, organizationId, tenantCode) { + if (!organizationCode || !organizationId || !tenantCode) { + throw new Error('Missing required organization or tenant identifiers') + } bodyData.created_by = id bodyData.updated_by = id bodyData.organization_code = organizationCode bodyData.organization_id = organizationId bodyData.tenant_code = tenantCode
56-56: Improve readability by avoiding comma operator.The comma operator usage makes the code less readable. Consider using separate statements for clarity.
- ;(bodyData.updated_by = loggedInUserId), (bodyData.organization_code = organizationCode) + bodyData.updated_by = loggedInUserId + bodyData.organization_code = organizationCode
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/controllers/v1/entity-type.js(4 hunks)src/controllers/v1/entity.js(4 hunks)src/controllers/v1/user.js(2 hunks)src/database/migrations/20250717071710-add-organization-code-to-entities-and-entity-types.js(1 hunks)src/database/migrations/20250723183753-add-entity-types-constraints.js(1 hunks)src/database/models/entities.js(1 hunks)src/database/models/entityType.js(2 hunks)src/database/queries/entities.js(4 hunks)src/database/queries/entityType.js(4 hunks)src/services/account.js(4 hunks)src/services/entities.js(7 hunks)src/services/entityType.js(5 hunks)src/services/tenant.js(3 hunks)src/services/user.js(6 hunks)src/validators/v1/entity-type.js(1 hunks)src/validators/v1/entity.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/controllers/**
⚙️ CodeRabbit Configuration File
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/user.jssrc/controllers/v1/entity-type.jssrc/controllers/v1/entity.js
src/validators/**
⚙️ CodeRabbit Configuration File
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/entity.jssrc/validators/v1/entity-type.js
src/services/**
⚙️ CodeRabbit Configuration File
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/tenant.jssrc/services/user.jssrc/services/account.jssrc/services/entityType.jssrc/services/entities.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/entities.jssrc/database/queries/entityType.js
🧬 Code Graph Analysis (5)
src/database/models/entityType.js (2)
src/database/queries/entities.js (1)
EntityType(3-3)src/database/queries/entityType.js (1)
EntityType(1-1)
src/services/tenant.js (6)
src/services/user.js (5)
defaultOrganizationCode(61-61)defaultOrganizationCode(349-349)defaultOrganizationCode(454-454)defaultOrganizationCode(533-533)entityTypeQueries(15-15)src/services/account.js (3)
defaultOrganizationCode(351-351)defaultOrganizationCode(1478-1478)entityTypeQueries(24-24)src/services/org-admin.js (2)
defaultOrgId(477-480)entityTypeQueries(19-19)src/services/entityType.js (2)
entityTypeQueries(4-4)entityType(27-27)src/services/public.js (1)
entityTypeQueries(8-8)src/controllers/v1/entity-type.js (1)
entityTypeService(2-2)
src/database/queries/entities.js (3)
src/database/models/entityType.js (1)
EntityType(3-42)src/database/queries/entityType.js (2)
EntityType(1-1)Entity(2-2)src/database/models/entities.js (1)
Entity(3-29)
src/services/account.js (3)
src/services/user.js (18)
userQueries(11-11)defaultOrganizationCode(61-61)defaultOrganizationCode(349-349)defaultOrganizationCode(454-454)defaultOrganizationCode(533-533)validationData(71-71)validationData(353-360)validationData(458-465)entityTypeQueries(15-15)user(48-51)user(319-319)user(440-440)user(490-490)user(525-525)prunedEntities(72-72)prunedEntities(361-361)prunedEntities(466-466)prunedEntities(544-544)src/generics/utils.js (2)
bcryptJs(8-8)removeDefaultOrgEntityTypes(835-842)src/database/queries/users.js (1)
user(224-269)
src/services/entities.js (5)
src/database/queries/entities.js (1)
require(2-2)src/services/entityType.js (11)
require(5-5)require(7-7)require(8-8)entityTypeQueries(4-4)responses(9-9)updateCount(58-67)filter(127-134)entities(98-104)entities(136-136)deleteCount(161-161)attributes(96-96)src/helpers/responses.js (1)
error(28-28)src/controllers/v1/entity.js (2)
updatedEntity(44-50)updatedEntity(85-89)src/database/queries/entityType.js (1)
entities(141-145)
🔇 Additional comments (39)
src/controllers/v1/user.js (1)
27-27: LGTM! Clean refactoring from organization_id to organization_code.The changes consistently update token property access to use
organization_codeinstead oforganization_idwhile maintaining the same authentication pattern and service method signatures.Also applies to: 121-121
src/validators/v1/entity-type.js (1)
38-38: Verify impact of restrictingmodel_namesto only'User'The validator in
src/validators/v1/entity-type.jsnow only permitsmodel_names=['User'](create at line 38; update at line 75). Our code search did not uncover any references to the previously allowed values (Session,MentorExtension,UserExtension) in entity-type logic or migrations.• src/validators/v1/entity-type.js – line 38 (create)
• src/validators/v1/entity-type.js – line 75 (update)Please confirm:
- No existing
entity_typesrecords in the database use any othermodel_names.- No external services or integrations rely on the old values.
src/database/models/entities.js (2)
21-24: LGTM! Proper addition of organization_code field.The new
organization_codefield is correctly defined as a required string, aligning with the PR's objective to shift from organization IDs to organization codes.
31-40: Good association improvements.The uncommented association method properly defines the relationship with explicit
targetKey: 'id'and includes a scope to filter out deleted records, which is a best practice for maintaining data integrity.src/validators/v1/entity.js (2)
78-78: Good fix for validation chain order.Moving
.trim()after.optional()prevents potential errors when the field is undefined, which is the correct validation chain order.Also applies to: 86-86
92-110: Excellent addition of entity_type_id validation and custom logic.The new validation for
entity_type_idincludes proper trimming, emptiness, and numeric checks. The custom validator ensuring eitheridOR (value+entity_type_id) is provided adds robust input validation that prevents invalid parameter combinations.src/database/models/entityType.js (1)
43-51: Good association implementation.The
hasManyassociation with Entity is properly scoped to exclude deleted records, which maintains data integrity and follows best practices.src/services/tenant.js (5)
145-145: Good improvement: Dynamic organization code extraction.Extracting the organization code from the creation response ensures consistency between the created organization and subsequent operations, replacing the previous reliance on static environment variables.
152-154: Consistent usage of dynamic organization code.The update to use
defaultOrganizationCodein thefindAllEntityTypescall is consistent with the broader refactor from static environment variables to dynamic organization codes.
161-161: Correct implementation of organization code parameter.The usage of
defaultOrganizationCodein thereadUserEntityTypescall follows the established pattern of using dynamic organization codes instead of static environment variables.
189-189: Proper organization code parameter usage.The
entityTypeService.createcall correctly uses the dynamicdefaultOrganizationCodeparameter, maintaining consistency with the broader refactor.
221-227: Correct multi-tenant parameter usage.The
EntityHelper.createcall properly includes bothdefaultOrganizationCodeandtenantCodeparameters, aligning with the multi-tenant architecture and the organization code refactor.src/controllers/v1/entity-type.js (3)
15-21: Correct organization code parameter usage.The
createmethod correctly usesreq.decodedToken.organization_codeand maintains both organization code and ID parameters for backward compatibility during the transition period.
37-43: Clean organization code implementation.The
updatemethod cleanly usesreq.decodedToken.organization_codewithout the legacy organization_id parameter, indicating a complete migration to organization codes for this operation.
85-91: Clean and simplified delete method.The
deletemethod correctly usesreq.decodedToken.organization_codewith a simplified parameter set, demonstrating a complete migration to organization codes for delete operations.src/controllers/v1/entity.js (4)
22-28: Correct implementation of organizational identifiers.The
createmethod properly usesreq.decodedToken.tenant_codeandreq.decodedToken.organization_code, replacing user-centric identifiers with organizational ones as part of the broader refactor.
44-51: Consistent organizational identifier usage.The
updatemethod correctly implements the organizational identifier pattern withreq.decodedToken.organization_codeandreq.decodedToken.tenant_codeparameters.
65-73: Simplified read method with tenant scoping.The
readmethod uses onlyreq.decodedToken.tenant_codefor scoping. Ensure that tenant-level scoping provides sufficient access control for entity read operations.
85-89: Comprehensive organizational scoping for delete operations.The
deletemethod correctly implements bothreq.decodedToken.organization_codeandreq.decodedToken.tenant_codeparameters, providing proper multi-level scoping for delete operations.src/services/user.js (7)
38-38: Correct method signature update.The method signature properly changes from
orgIdtoorgCodeparameter, aligning with the organization code refactor.
65-66: Proper query filter update.The query filter correctly uses
organization_codefield with theorgCodeparameter, replacing the previousorganization_idusage.
349-356: Consistent organization code implementation.The changes correctly implement organization code usage with proper variable extraction from
user.organizations[0].codeand updated query filters usingorganization_code.
454-461: Correct organization code pattern implementation.The method properly implements the organization code pattern with consistent variable naming and query filter usage.
522-522: Updated method signature for organization code.The method signature correctly changes to use
organizationCodeparameter, maintaining consistency with the broader refactor.
538-538: Consistent query filter implementation.The query filter properly uses
organization_codewith the organizational identifier pattern.
361-361: Ignore mixed ID/code concern forremoveDefaultOrgEntityTypes
The utility’s signature in src/generics/utils.js expects an organization ID (as seen on line 835), while the Sequelize query in src/services/user.js uses the organization code to filter byorganization_code. Passinguser.organizations[0].idintoremoveDefaultOrgEntityTypesis therefore correct and consistent with its implementation. No changes required.Likely an incorrect or invalid review comment.
src/database/queries/entities.js (4)
3-3: Required import for new functionality.The
EntityTypeimport is correctly added to support the newfindEntityWithOrgCheckmethod that includes EntityType associations.
26-32: Correct implementation of organizational scoping.The
updateOneEntitymethod correctly transitions from user-based filtering to organizational identifiers (organizationCodeandtenantCode), providing proper multi-tenant scoping for entity updates.
41-47: Consistent organizational scoping for delete operations.The
deleteOneEntitymethod follows the same correct pattern as the update method, using organizational identifiers for proper access control and multi-tenant scoping.
94-112: Ignore naming inconsistency warning for findEntityWithOrgCheckThe
findEntityWithOrgCheck(id, tenantCode, organizationID)method is correctly usingorganizationIDto filter on theorganization_idcolumn in theEntityTypemodel. The codebase still relies onorganization_id(numeric ID) in this context, and numerous other queries and the model definition itself useorganization_id. No change is required.Likely an incorrect or invalid review comment.
src/database/migrations/20250717071710-add-organization-code-to-entities-and-entity-types.js (1)
106-114: Verify the unique constraint covers all necessary columns.The new unique index on entities includes
value,entity_type_id,organization_code, andtenant_code. This is a significant change from the previous constraint on justvalue.Ensure that:
- This constraint aligns with your business logic (allowing duplicate entity values across different organizations/tenants)
- Application code is updated to handle potential unique constraint violations with the new composite key
- Performance impact of the new index has been considered
src/database/migrations/20250723183753-add-entity-types-constraints.js (1)
43-43: Verify CASCADE delete behavior is intended.The foreign key constraint uses
CASCADEon delete, which means deleting an entity_type will automatically delete all associated entities. This could lead to unintended data loss.Confirm that:
- This cascading delete behavior is intentional and documented
- Application code properly handles or prevents entity_type deletions when entities exist
- Soft deletes (deleted_at) are preferred over hard deletes in your system
src/database/queries/entityType.js (1)
39-66: Excellent query optimization using Sequelize associations.The refactoring to use Sequelize's include mechanism with proper LEFT JOIN and tenant isolation is a significant improvement over the previous implementation. The error handling has also been enhanced with specific error messages.
src/services/entities.js (1)
184-216: Good use of pagination in list method.The list method correctly uses pagination parameters and handles empty results appropriately.
src/services/account.js (2)
351-360: Good implementation of organization code validation and filtering.The error handling for missing organization codes and the logic for collecting organization codes from user data is well implemented. The use of the default organization code as a fallback is appropriate.
Also applies to: 786-799
357-364: Consistent and correct implementation of entity type filtering.The entity type queries consistently use organization codes with the
Op.inoperator, properly including both user-specific and default organization codes. This ensures users have access to both their organization-specific and system-wide entity types.Also applies to: 789-796, 1481-1488
src/services/entityType.js (3)
94-122: LGTM! Well-structured organization code migration.The method correctly handles the transition by:
- Querying both default and specific organization codes
- Using
organizationIdfor the pruning logic (maintaining backward compatibility)- Properly handling empty result sets
The implementation maintains functionality while supporting the new organization code structure.
124-150: Excellent multi-tenant isolation and error handling improvements.The updates demonstrate good practices:
- Added
tenant_codefiltering for proper tenant isolation- Clear comments explaining the deduplication logic
- Enhanced error logging for better debuggability
- Proper use of Sequelize
Op.infor organization code filteringThe method correctly handles the migration while maintaining security through tenant isolation.
159-177: LGTM! Clean and straightforward organization code integration.The delete method properly incorporates the
organizationCodeparameter and passes it to the query layer. The error handling remains appropriate for the not found scenario.
|
@coderabbitai help |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Can you help me to generate the changelog that I can share with QA for this PR ? |
|
Tip For best results, initiate chat on the files or code changes. @nevil-mathew: I'll generate a comprehensive changelog for QA testing based on the changes in this PR. Let me organize this by the types of changes that impact testing: Changelog for PR #776: Refactor Organization IDs to Organization Codes🔄 API Parameter ChangesImpact: All API endpoints now use Controllers Updated:
🗄️ Database Schema ChangesImpact: New columns added, constraints updated New Migrations:
Model Changes:
🔧 Validation ChangesImpact: Request validation updated
🏢 Business Logic ChangesImpact: Organization handling and entity inheritance
📝 User-Facing Changes
🧪 QA Testing Focus AreasHigh Priority Testing:
API Testing:
Edge Cases:
Regression Testing:
This refactoring maintains the same business functionality while changing the underlying organization identification system from numeric IDs to string codes. 🧠 Learnings used |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Database
Validation