[Fix] Alter Resource Link Entity Based On Employee #8772
Conversation
WalkthroughThis pull request updates the resource link modules by shifting from a user-centric model to an employee-centric model. In the contracts package, interface definitions are modified to extend employee-related inputs and remove user-specific properties. In the core package, the DTO now directly extends the ResourceLink entity, the entity itself renames properties (e.g., from creator to employee), and service methods are updated to use EmployeeService with improved error handling and logging. The controller methods are also enhanced with detailed JSDoc comments for API endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant RC as ResourceLinkController
participant RS as ResourceLinkService
participant ES as EmployeeService
participant ALS as ActivityLogService
C->>RC: Create resource link (DTO)
RC->>RS: Forward DTO to create method
RS->>ES: Retrieve employee by employeeId
ES-->>RS: Employee Found / Not Found
alt Employee Found
RS->>DB: Insert new resource link (with employeeId)
RS->>ALS: Log creation activity
RS-->>RC: Return created resource link
else Employee Not Found
RS-->>RC: Throw NotFoundException
end
RC-->>C: Return response
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![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 (1)
packages/core/src/lib/database/migrations/1739272947765-AlterResourceLinkRenameCreatedByField.ts (1)
84-156: SQLite approach is comprehensive but quite large.
Renaming columns in SQLite typically requires creating a temporary table and performing multiple index drops and recreations, which can be verbose. This implementation is valid for older SQLite versions.If you can guarantee a newer SQLite version (≥3.25.0), you could use the built-in
ALTER TABLE ... RENAME COLUMN ...command to simplify the process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/contracts/src/lib/resource-link.model.ts(1 hunks)packages/core/src/lib/database/migrations/1739272947765-AlterResourceLinkRenameCreatedByField.ts(1 hunks)packages/core/src/lib/resource-link/dto/create-resource-link.dto.ts(1 hunks)packages/core/src/lib/resource-link/resource-link.entity.ts(1 hunks)packages/core/src/lib/resource-link/resource-link.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (11)
packages/core/src/lib/database/migrations/1739272947765-AlterResourceLinkRenameCreatedByField.ts (6)
5-30: Migration class structure looks solid.
This class includes a clear name, implementsMigrationInterface, and provides anupmethod to handle database-specific logic. The switch statement effectively delegates the migration to helper methods that handle each supported database.
32-52: Down migration structure is well-defined.
Thedownmethod properly complements theupmethod by restoring the previous column name and constraints for each database engine, ensuring reversibility of the migration.
54-82: PostgreSQL rename logic is correctly implemented.
Dropping and recreating constraints and indexes around the rename ensures that relationships remain intact. This approach is consistent with best practices for column renaming in Postgres.
158-230: SQLite down migration correctly reverts changes.
The code meticulously reconstructs the original table structure and constraints, ensuring data integrity during rollback.
232-247: MySQL up migration is well-structured.
Dropping foreign keys and indexes, then performing the column rename and re-creating constraints follows MySQL’s best approach for renaming columns referenced by foreign keys.
249-262: MySQL down migration restores the original schema.
Consistently mirrors the changes from the up migration, returning to the previous column name and constraints without data loss.packages/core/src/lib/resource-link/dto/create-resource-link.dto.ts (1)
10-10: Omitting the new fields aligns with the renamed properties.
Changing the omitted fields from['creator', 'creatorId']to['createdBy', 'createdById']correctly mirrors the updated naming convention in the entity and contracts, ensuring consistent DTO validation.packages/contracts/src/lib/resource-link.model.ts (2)
8-9: Renaming properties matches the updated naming convention.
ReplacingcreatorandcreatorIdwithcreatedByandcreatedByIdkeeps the code consistent with the rest of the application.
13-13: Create interface aligns with the new property names.
AdjustingIResourceLinkCreateInputto omit'createdBy', 'createdById'instead of the old fields avoids confusion and maintains consistency with the refactored schema.packages/core/src/lib/resource-link/resource-link.entity.ts (1)
63-63: LGTM! Property renaming follows consistent naming convention.The renaming from
creatortocreatedByandcreatorIdtocreatedByIdmaintains functionality while improving naming consistency.Let's verify that all references to these properties have been updated across the codebase:
Also applies to: 65-65, 68-68
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to old property names echo "Checking for remaining references to 'creator' and 'creatorId'..." rg -i "creator(?!.*by)" packages/core/src/lib/resource-link/Length of output: 441
I'll run an updated search command with PCRE2 enabled to check if any references to the old property names remain:
Once this command completes, please verify that no output is produced—indicating that all mentions of the old names have been updated. This ensures that the renaming (from
creatortocreatedByandcreatorIdtocreatedById) is applied consistently across the codebase.LGTM! The renaming is consistently implemented and the PCRE2-enabled search should confirm that no obsolete references remain.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Checking for remaining references to 'creator' and 'creatorId' using PCRE2 regex..." rg --pcre2 -i "creator(?!.*by)" packages/core/src/lib/resource-link/Length of output: 381
LGTM! Renamed properties are consistently updated.
The PCRE2 regex search did not return any obsolete references, confirming that the renaming from
creator/creatorIdtocreatedBy/createdByIdis fully implemented in the targeted code.packages/core/src/lib/resource-link/resource-link.service.ts (1)
53-53: LGTM! Property name updated consistently.The property name change from
creatorIdtocreatedByIdin the create method aligns with the entity changes.
|
View your CI Pipeline Execution ↗ for commit dcc60a4.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/lib/resource-link/resource-link.controller.ts (5)
57-58: Remove redundant implementation comment.The comment on line 57 is redundant as the method name and JSDoc already clearly indicate the purpose of this method.
- // Call the service to retrieve the paginated list of resource links return await this.resourceLinkService.findAll(params);
82-83: Remove redundant parameter comments.The inline comments for parameters are redundant as they are already documented in the JSDoc.
- @Param('id', UUIDValidationPipe) id: ID, // Validate and retrieve the ID parameter - @Query() params: OptionParams<ResourceLink> // Retrieve optional query parameters + @Param('id', UUIDValidationPipe) id: ID, + @Query() params: OptionParams<ResourceLink>
74-74: Remove or uncomment the type specification.The commented-out type specification should either be used or removed to maintain clean code.
- // type: ResourceLink // Uncomment and specify the type if needed for API documentation
110-111: Remove redundant implementation comment.The comment on line 110 is redundant as the method name and JSDoc already clearly indicate the purpose of this method.
- // Execute the command to create the resource link return await this.commandBus.execute(new ResourceLinkCreateCommand(entity));
166-167: Remove redundant implementation comment.The comment on line 166 is redundant as the method name and JSDoc already clearly indicate the purpose of this method.
- // Execute the delete operation and return the result return await this.resourceLinkService.delete(id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/contracts/src/lib/resource-link.model.ts(1 hunks)packages/core/src/lib/resource-link/dto/create-resource-link.dto.ts(2 hunks)packages/core/src/lib/resource-link/resource-link.controller.ts(3 hunks)packages/core/src/lib/resource-link/resource-link.entity.ts(2 hunks)packages/core/src/lib/resource-link/resource-link.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/lib/resource-link/resource-link.service.ts
- packages/contracts/src/lib/resource-link.model.ts
- packages/core/src/lib/resource-link/resource-link.entity.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (2)
packages/core/src/lib/resource-link/dto/create-resource-link.dto.ts (1)
9-11:❓ Verification inconclusive
Verify the impact of including all properties from ResourceLink.
The DTO now includes all properties from
ResourceLinkthroughIntersectionType. Please ensure that this change aligns with the intended API contract and doesn't expose any sensitive or internal properties that should be omitted during creation.Run the following script to analyze the properties of
ResourceLink:
🏁 Script executed:
#!/bin/bash # Description: Analyze the properties of ResourceLink entity. # Search for the ResourceLink entity definition ast-grep --pattern $'class ResourceLink { $$$ }'Length of output: 51
Please run the above script to confirm where and how the ResourceLink class is defined. Once we know the complete definition and its properties, we can ensure that no sensitive or unintended properties are being exposed in the CreateResourceLinkDTO.
Action Required: Verify Exposure of ResourceLink Properties
The updated CreateResourceLinkDTO now includes all properties from ResourceLink via IntersectionType. However, our initial automated check using ast-grep did not return any output, so we couldn’t confirm the details of ResourceLink directly.
- Please run the provided rg search to locate the ResourceLink definition.
- Once located, manually verify that the properties merged into the DTO do not include sensitive or internal fields that should be excluded during resource creation.
🏁 Script executed:
#!/bin/bash # Broader search for the definition of ResourceLink using rg to capture potential file variations. rg "class ResourceLink" -n .Length of output: 1372
Review Impact Verification – Manual Check Recommended
The
CreateResourceLinkDTOnow inherits all properties fromResourceLinkvia theIntersectionType. Our search confirms that theResourceLinkentity is defined inpackages/core/src/lib/resource-link/resource-link.entity.ts(line 13 onward). However, we couldn’t automatically verify the individual properties.
- Action: Manually inspect the
ResourceLinkdefinition inpackages/core/src/lib/resource-link/resource-link.entity.tsto confirm that none of its properties are sensitive or meant for internal use.- Context: Ensure that merging properties from both
TenantOrganizationBaseDTOandResourceLinkaligns with the intended API contract. Exclude or transform any internal fields as needed.packages/core/src/lib/resource-link/resource-link.controller.ts (1)
114-144: LGTM!The JSDoc comments and implementation look good. The documentation clearly describes the purpose, parameters, and return type of the method.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (3)
8-30: Add error handling and logging for migration failures.While the database type switching logic is correct, consider adding try-catch blocks with proper error logging to track migration failures.
public async up(queryRunner: QueryRunner): Promise<void> { console.log(chalk.yellow(this.name + ' start running!')); + try { switch (queryRunner.connection.options.type) { case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: await this.sqliteUpQueryRunner(queryRunner); break; case DatabaseTypeEnum.postgres: await this.postgresUpQueryRunner(queryRunner); break; case DatabaseTypeEnum.mysql: await this.mysqlUpQueryRunner(queryRunner); break; default: throw Error(`Unsupported database: ${queryRunner.connection.options.type}`); } + } catch (error) { + console.error(chalk.red(`Error during migration up: ${error.message}`)); + throw error; + } }
32-52: Add error handling for down migration.Apply the same error handling pattern to the down migration method.
public async down(queryRunner: QueryRunner): Promise<void> { + try { switch (queryRunner.connection.options.type) { case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: await this.sqliteDownQueryRunner(queryRunner); break; case DatabaseTypeEnum.postgres: await this.postgresDownQueryRunner(queryRunner); break; case DatabaseTypeEnum.mysql: await this.mysqlDownQueryRunner(queryRunner); break; default: throw Error(`Unsupported database: ${queryRunner.connection.options.type}`); } + } catch (error) { + console.error(chalk.red(`Error during migration down: ${error.message}`)); + throw error; + } }
124-126: Remove duplicate logging message.The logging message for step 4 appears twice.
- // Step 4: Copy data from "employeeId" to "creatorId" via employee table mapping - console.log('Step 4: Copying data from "employeeId" to "creatorId" via employee table mapping...'); await queryRunner.query(`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts(1 hunks)packages/core/src/lib/resource-link/resource-link.module.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (3)
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8484
File: packages/core/src/database/migrations/1729861943822-AlterConstraintsForResourceLinkTable.ts:60-87
Timestamp: 2024-11-12T14:06:02.202Z
Learning: In the Gauzy project's TypeORM migrations (e.g., `packages/core/src/database/migrations`), explicit transaction handling is not required because TypeORM handles transactions implicitly.
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8769
File: packages/core/src/lib/database/migrations/1739364133493-AlterCommentEntityTable.ts:658-659
Timestamp: 2025-02-13T16:48:54.177Z
Learning: TypeORM-generated migrations may use different data types for UUID fields (uuid in PostgreSQL, varchar(255) in MySQL) as this is handled by TypeORM's migration generator and is the standard practice across the packages.
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8771
File: packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts:600-642
Timestamp: 2025-02-17T07:36:46.838Z
Learning: MySQL migrations involving user-employee mappings should include logging and error handling for cases where employee records don't exist or multiple records exist for a user. Use a logging table to track problematic records and generate migration summaries.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
🔇 Additional comments (3)
packages/core/src/lib/resource-link/resource-link.module.ts (2)
6-6: LGTM! Module import changes align with the PR objective.The replacement of
UserModulewithEmployeeModulecorrectly implements the transition from user-based to employee-based model.Also applies to: 18-18
23-23:✅ Verification successful
Verify the impact of removing exports.
The removal of previously exported services (
ResourceLinkServiceandTypeOrmResourceLinkRepository) is a breaking change that could affect dependent modules.Let's verify the impact:
🏁 Script executed:
#!/bin/bash # Description: Search for imports of previously exported services to identify affected modules echo "Searching for ResourceLinkService imports..." rg "import.*ResourceLinkService.*from.*resource-link" --type ts echo "Searching for TypeOrmResourceLinkRepository imports..." rg "import.*TypeOrmResourceLinkRepository.*from.*resource-link" --type tsLength of output: 1228
Impact of Removing Service Exports Confirmed Safe
Verification shows that downstream consumers import both services using direct, relative paths rather than relying on module re-exports. Specifically:
- ResourceLinkService: Imported in handler files (
resource-link.update.handler.ts,resource-link.create.handler.ts) and the controller (resource-link.controller.ts), all referencing the service directly fromresource-link.service.- TypeOrmResourceLinkRepository: Imported directly in both
resource-link.module.tsandresource-link.service.tsfrom its repository file.Based on these findings, the removal of these exports (resulting in
exports: []in the module) does not pose a breaking change under current usage. If any consumers were to rely on module re-exports, updating the documentation to reflect the direct import pattern is recommended.packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (1)
1-7: LGTM! Clean imports and class structure.The migration class is well-structured with appropriate imports.
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (1)
59-102: 🛠️ Refactor suggestionAdd logging for problematic records in PostgreSQL up migration.
Based on previous learnings, we should add logging for cases where employee records don't exist or multiple records exist for a user.
Apply this diff to add logging:
public async postgresUpQueryRunner(queryRunner: QueryRunner): Promise<any> { + // Create a temporary table to log migration issues + await queryRunner.query(` + CREATE TABLE IF NOT EXISTS "resource_link_migration_log" ( + "id" SERIAL PRIMARY KEY, + "resource_link_id" uuid, + "old_creator_id" uuid, + "issue" varchar, + "created_at" TIMESTAMP DEFAULT CURRENT_TIMESTAMP + ) + `); // ... existing steps 1-3 ... + // Log cases where no employee exists + await queryRunner.query(` + INSERT INTO "resource_link_migration_log" ("resource_link_id", "old_creator_id", "issue") + SELECT rl."id", rl."creatorId", 'No employee record found' + FROM "resource_link" rl + LEFT JOIN "employee" e ON e."userId" = rl."creatorId" + WHERE rl."creatorId" IS NOT NULL AND e."id" IS NULL + `); + + // Log cases where multiple employees exist + await queryRunner.query(` + INSERT INTO "resource_link_migration_log" ("resource_link_id", "old_creator_id", "issue") + SELECT rl."id", rl."creatorId", 'Multiple employee records found' + FROM "resource_link" rl + JOIN ( + SELECT "userId" + FROM "employee" + GROUP BY "userId" + HAVING COUNT(*) > 1 + ) multi ON multi."userId" = rl."creatorId" + WHERE rl."creatorId" IS NOT NULL + `); // ... existing data migration query ... + // Generate migration summary + const logResults = await queryRunner.query(` + SELECT issue, COUNT(*) as count + FROM "resource_link_migration_log" + GROUP BY issue + `); + console.log('Migration issues summary:', logResults);
🧹 Nitpick comments (1)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (1)
13-30: Add error handling and logging for database type switching.The database type switching logic could benefit from enhanced error handling and logging.
Apply this diff to improve error handling:
public async up(queryRunner: QueryRunner): Promise<void> { console.log(chalk.yellow(this.name + ' start running!')); + const dbType = queryRunner.connection.options.type; + console.log(`Database type: ${dbType}`); try { - switch (queryRunner.connection.options.type) { + switch (dbType) { case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: await this.sqliteUpQueryRunner(queryRunner); break; case DatabaseTypeEnum.postgres: await this.postgresUpQueryRunner(queryRunner); break; case DatabaseTypeEnum.mysql: await this.mysqlUpQueryRunner(queryRunner); break; default: - throw Error(`Unsupported database: ${queryRunner.connection.options.type}`); + throw Error(`Unsupported database: ${dbType}`); } + console.log(chalk.green(`${this.name} completed successfully!`)); + } catch (error) { + console.error(chalk.red(`Error in ${this.name}:`), error); + throw error; } }Also applies to: 37-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (3)
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8769
File: packages/core/src/lib/database/migrations/1739364133493-AlterCommentEntityTable.ts:658-659
Timestamp: 2025-02-13T16:48:54.177Z
Learning: TypeORM-generated migrations may use different data types for UUID fields (uuid in PostgreSQL, varchar(255) in MySQL) as this is handled by TypeORM's migration generator and is the standard practice across the packages.
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8484
File: packages/core/src/database/migrations/1729861943822-AlterConstraintsForResourceLinkTable.ts:60-87
Timestamp: 2024-11-12T14:06:02.202Z
Learning: In the Gauzy project's TypeORM migrations (e.g., `packages/core/src/database/migrations`), explicit transaction handling is not required because TypeORM handles transactions implicitly.
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8771
File: packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts:600-642
Timestamp: 2025-02-17T07:36:46.838Z
Learning: MySQL migrations involving user-employee mappings should include logging and error handling for cases where employee records don't exist or multiple records exist for a user. Use a logging table to track problematic records and generate migration summaries.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
🔇 Additional comments (2)
packages/core/src/lib/database/migrations/1739782998215-AlterResourceLinkEntityTable.ts (2)
159-518: SQLite migration implementation looks good!The SQLite migration implementation is thorough and follows best practices:
- Uses temporary tables for schema changes
- Preserves data during migration
- Handles index recreation correctly
- Maintains referential integrity
525-612:✅ Verification successful
Verify UUID field type handling in MySQL.
Based on previous learnings, TypeORM-generated migrations may use different data types for UUID fields (uuid in PostgreSQL, varchar(255) in MySQL).
Run this script to check UUID field type handling across databases:
🏁 Script executed:
#!/bin/bash # Check UUID field type handling in migrations echo "Searching for UUID field type definitions..." rg -A 2 "uuid|varchar.*255" --type tsLength of output: 87902
UUID Field Type Handling in MySQL Confirmed
After verifying the UUID field type usage across migrations, it appears that the MySQL-specific migration in this file uses
varchar(255)for UUID fields, which is consistent with our approach in other MySQL migrations. The code correctly distinguishes between PostgreSQL (using nativeuuid) and MySQL (usingvarchar(255)) data types. No changes are required regarding the UUID field type handling in these migration methods.
rahul-rocket
deleted the
fix/resource-link-rename-creator-to-created-by
branch
rahul-rocket
changed the title
creator to createdBy
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
Refactor
UserModulewithEmployeeModule.ResourceLinkentity.Documentation