[Fix] Alter "Dashboard" Entity Based On Employee #8790
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request implements extensive updates across multiple modules. It restructures dashboard-related interfaces and type definitions, adds new utility types, renames and repositions user-related properties, and enhances error handling in command handlers. Additional seeding functions for default and random employee dashboards are introduced along with adjustments to API endpoint paths, DTOs, and database migrations for dashboard entities. Minor changes in notification seeding further streamline the data insertion process. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DC as DashboardController
participant DCH as DashboardCreateHandler
participant DS as DashboardService
participant DB as Database
U->>DC: POST /dashboard
DC->>DCH: execute(dashboardData)
DCH->>DS: create(dashboardData)
DS->>DB: Save dashboard
alt Dashboard creation success
DB-->>DS: Dashboard saved
DS-->>DCH: Return dashboard
DCH-->>DC: Return success response
else Dashboard creation failure
DS-->>DCH: Error occurred
DCH->>DCH: Log error details
DCH-->>DC: Throw HttpException (BAD_REQUEST)
end
sequenceDiagram
participant SDS as SeedDataService
participant DD as DashboardSeed
participant DB as Database
SDS->>DD: createDefaultEmployeeDashboards(dataSource, tenant, org, employees)
DD->>DB: insertDashboards(dashboardList)
DB-->>DD: Insertion result
DD-->>SDS: Return default dashboards
SDS->>DD: createRandomEmployeeDashboards(dataSource, tenants, orgEmpMap)
DD->>DB: insertDashboards(dashboardList)
DB-->>DD: Insertion result
DD-->>SDS: Return random dashboards
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (8)
packages/core/src/lib/database/migrations/1739950092274-AlterDashboardEntityTable.ts (2)
1-3: Consider avoiding console-based logging in migrations.Using
chalkandconsole.login a production setting may clutter logs. Consider removing these calls, or use a dedicated logging mechanism in your migration scripts for consistent and traceable logs.
675-704: Incorporate a log or validation step for MySQL user-employee mapping.Based on your prior learnings for MySQL migrations, consider adding a validation or logging routine for edge cases where dashboards lack corresponding employee records. A side logging table or migration summary can help track problematic rows.
packages/core/src/lib/dashboard/commands/handlers/dashboard.create.handler.ts (1)
24-25: Enhance error handling for better security and clarity.Consider these improvements to the error handling:
- Avoid logging the full stack trace as it might expose sensitive information
- Provide more specific error messages for different failure scenarios
- this.logger.error('Failed to create dashboard', error.stack); - throw new HttpException(`Error while creating dashboard: ${error.message}`, HttpStatus.BAD_REQUEST); + this.logger.error('Failed to create dashboard', error.message); + throw new HttpException( + 'Failed to create dashboard. Please check the input data and try again.', + HttpStatus.BAD_REQUEST + );packages/core/src/lib/dashboard/commands/handlers/dashboard.update.handler.ts (1)
24-25: Apply consistent error handling improvements.Similar to the create handler, enhance the error handling:
- Avoid logging the full stack trace
- Provide more specific error messages
- this.logger.error('Failed to update dashboard', error.stack); - throw new HttpException(`Error while updating dashboard: ${error.message}`, HttpStatus.BAD_REQUEST); + this.logger.error('Failed to update dashboard', error.message); + throw new HttpException( + 'Failed to update dashboard. Please check the input data and try again.', + HttpStatus.BAD_REQUEST + );packages/core/src/lib/dashboard/dashboard.entity.ts (2)
124-124: Fix typo in comment.Correct the spelling of "Dashbaord" to "Dashboard".
- * Dashbaord Widgets + * Dashboard Widgets🧰 Tools
🪛 GitHub Check: Cspell
[warning] 124-124:
Unknown word (Dashbaord)🪛 GitHub Actions: Check Spelling and Typos with cspell
[warning] 124-124: Unknown word (Dashbaord)
90-97: Consider adding database index for employeeId.Since
employeeIdwill likely be used in queries to fetch dashboards for specific employees, consider adding a database index to improve query performance.@ApiPropertyOptional({ type: () => String }) @IsOptional() @IsUUID() @IsEmployeeBelongsToOrganization() @RelationId((dashboard: Dashboard) => dashboard.employee) - @ColumnIndex() + @Index() @MultiORMColumn({ relationId: true }) employeeId?: ID;packages/core/src/lib/dashboard/dashboard.seed.ts (2)
101-102: Fix incorrect comment reference.The comment references "EmployeeNotification" instead of "Dashboard".
- // Get the repository for EmployeeNotification + // Get the repository for Dashboard
95-98: Enhance warning message with more context.The warning message could be more helpful by including the count of dashboards.
if (!dashboards.length) { - console.warn('No dashboards to insert. Please check the input data and try again.'); + console.warn('No dashboards to insert. Received an empty array of dashboards. Please check the input data and try again.'); return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/contracts/src/lib/dashboard.model.ts(2 hunks)packages/contracts/src/lib/employee.model.ts(1 hunks)packages/contracts/src/lib/user.model.ts(1 hunks)packages/core/src/lib/core/seeds/seed-data.service.ts(4 hunks)packages/core/src/lib/dashboard/commands/handlers/dashboard.create.handler.ts(1 hunks)packages/core/src/lib/dashboard/commands/handlers/dashboard.update.handler.ts(2 hunks)packages/core/src/lib/dashboard/dashboard.controller.ts(2 hunks)packages/core/src/lib/dashboard/dashboard.entity.ts(3 hunks)packages/core/src/lib/dashboard/dashboard.seed.ts(1 hunks)packages/core/src/lib/dashboard/dashboard.service.ts(3 hunks)packages/core/src/lib/dashboard/dashboard.subscriber.ts(0 hunks)packages/core/src/lib/dashboard/dto/create-dashboard.dto.ts(1 hunks)packages/core/src/lib/database/migrations/1739950092274-AlterDashboardEntityTable.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.seed.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/lib/dashboard/dashboard.subscriber.ts
🧰 Additional context used
🧠 Learnings (1)
packages/core/src/lib/database/migrations/1739950092274-AlterDashboardEntityTable.ts (2)
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.
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.
🪛 GitHub Check: Cspell
packages/core/src/lib/dashboard/dashboard.entity.ts
[warning] 124-124:
Unknown word (Dashbaord)
🪛 GitHub Actions: Check Spelling and Typos with cspell
packages/core/src/lib/dashboard/dashboard.entity.ts
[warning] 124-124: Unknown word (Dashbaord)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (23)
packages/core/src/lib/database/migrations/1739950092274-AlterDashboardEntityTable.ts (4)
13-30: Verify data migration from the dropped “creatorId” field.Dropping the existing
creatorIdcolumn without transferring its values to the newly introduced columns may cause data loss. If you need to retain or re-map the old data, please implement a transformation step before removingcreatorId.
106-391: Verify data continuity in the SQLite 'Up' process.The multi-step SQLite migration pattern re-creates the table multiple times. Ensure that all intermediate steps preserve data integrity (especially when dropping columns like
creatorIdand addingemployeeId,createdByUserId). Test with actual data to confirm no unintentional data loss occurs.
392-673: Review rollback steps for SQLite migrations.The down migration reverts certain columns but does not re-populate them with data previously stored in
employeeIdorcreatedByUserId. Verify if this partial rollback is acceptable or if additional data handling is required.
706-727: Confirm correct reversal of MySQL changes.When dropping
employeeIdandcreatedByUserIdduring rollback, data introduced by the up migration is lost. Confirm if this behavior is expected or if additional logic is needed to restore or preserve data in the oldcreatorIdcolumn.packages/core/src/lib/dashboard/dto/create-dashboard.dto.ts (1)
10-13: Confirm omission of the user-related fields in creation.The properties
createdByUser,createdByUserIdare now excluded from the DTO, so they won't be set when creating dashboards. Verify that you don't need to assign or capture the creating user at creation time.packages/contracts/src/lib/dashboard.model.ts (2)
1-4: Imports and new references look consistent.You’ve introduced imports for
IEmployeeEntityInputand updated user-creator fields. This aligns well with the new entity structure. No issues spotted on these lines.
9-23: Validate extension of `IEmployeeEntityInput` and `IHasUserCreator`.Dashboards now require an associated employee and user creator. If there are dashboards owned by users who are not employees, ensure they can still gracefully map these fields or safely handle missing references.
packages/core/src/lib/dashboard/dashboard.seed.ts (2)
14-36: LGTM! Well-structured default dashboard creation.The implementation is clean and includes all necessary fields for creating default dashboards.
47-81: LGTM! Efficient implementation of random dashboard creation.The nested iteration through tenants, organizations, and employees is well-structured and maintains proper relationships.
packages/core/src/lib/dashboard/dashboard.service.ts (4)
10-12: LGTM! Import paths are now more specific.The import paths have been updated to be more specific, improving code maintainability.
22-22: LGTM! Variable naming follows conventions.The private variable has been renamed from
activityLogServiceto_activityLogServiceto follow the naming convention for private members.
65-68: Enhanced error handling with logging.Error handling has been improved by adding console logging before throwing the HTTP exception. This will help with debugging issues in production.
92-95: Improved error handling for non-existent dashboard.The error handling for a non-existent dashboard now includes logging, which will help track down issues in production.
packages/core/src/lib/dashboard/dashboard.controller.ts (2)
30-30: LGTM! API endpoint paths follow REST best practices.The API endpoint paths have been updated to include leading slashes, following REST API best practices:
@Controller('/dashboard')@Get('/')@Get('/:id')@Post('/')@Put('/:id')@Delete('/:id')Also applies to: 54-54, 77-77, 102-102, 131-131, 153-153
43-43: LGTM! Enhanced API documentation.The API operation summaries have been improved with more descriptive text, making the API more user-friendly:
- "Retrieve a list of dashboards with pagination"
- "Retrieve a dashboard by its ID"
- "Create a new dashboard"
- "Update an existing dashboard"
- "Delete a dashboard by ID"
Also applies to: 66-66, 92-92, 115-115, 143-143
packages/core/src/lib/employee-notification/employee-notification.seed.ts (2)
25-25: LGTM! Simplified organization handling.Changed from array to single organization parameter, improving code clarity and reducing complexity.
140-141: LGTM! Added configurable batch size for performance tuning.Added optional
batchSizeparameter with a default value of 100, allowing for performance optimization of bulk inserts.packages/contracts/src/lib/user.model.ts (2)
33-33: LGTM! Improved type naming and field exclusion.The utility type has been renamed from
ExcludeCreatorFieldstoExcludeUserCreatorFieldsand now excludescreatedByUserandcreatedByUserIdfields, making it more specific and clearer in its purpose.
38-41: LGTM! Enhanced creator interface naming and fields.The interface has been renamed from
IHasCreatortoIHasUserCreatorwith updated field names, improving clarity and consistency in the codebase.packages/contracts/src/lib/employee.model.ts (1)
31-34: LGTM! Well-documented utility type.The
ExcludeEmployeeAuthorFieldstype is a well-designed utility that provides a flexible way to exclude employee-related fields from types. The implementation is clean and follows TypeScript best practices.packages/core/src/lib/core/seeds/seed-data.service.ts (3)
32-32: LGTM! Clean import statement.The import statement follows the existing pattern and imports the necessary functions for dashboard seeding.
724-732: LGTM! Well-structured default dashboard seeding.The implementation follows the established seeding pattern and correctly passes the required parameters for creating default employee dashboards.
1424-1432: LGTM! Well-structured random dashboard seeding.The implementation follows the established seeding pattern and correctly passes the required parameters for creating random employee dashboards.
| public async postgresUpQueryRunner(queryRunner: QueryRunner): Promise<any> { | ||
| await queryRunner.query(`ALTER TABLE "dashboard" DROP CONSTRAINT "FK_d343751cf98e2bfd85754a35a12"`); | ||
| await queryRunner.query(`DROP INDEX "public"."IDX_d343751cf98e2bfd85754a35a1"`); | ||
| await queryRunner.query(`ALTER TABLE "dashboard" DROP COLUMN "creatorId"`); | ||
| await queryRunner.query(`ALTER TABLE "dashboard" ADD "employeeId" uuid NOT NULL`); | ||
| await queryRunner.query(`ALTER TABLE "dashboard" ADD "createdByUserId" uuid NOT NULL`); | ||
| await queryRunner.query(`CREATE INDEX "IDX_b34e5ae765e0f8d674e0604621" ON "dashboard" ("employeeId") `); | ||
| await queryRunner.query(`CREATE INDEX "IDX_30613c8cd1a1df1b176dfb696b" ON "dashboard" ("createdByUserId") `); | ||
| await queryRunner.query( | ||
| `ALTER TABLE "dashboard" | ||
| ADD CONSTRAINT "FK_b34e5ae765e0f8d674e06046210" | ||
| FOREIGN KEY ("employeeId") REFERENCES "employee"("id") | ||
| ON DELETE CASCADE | ||
| ON UPDATE NO ACTION` | ||
| ); | ||
| await queryRunner.query( | ||
| `ALTER TABLE "dashboard" | ||
| ADD CONSTRAINT "FK_30613c8cd1a1df1b176dfb696ba" | ||
| FOREIGN KEY ("createdByUserId") REFERENCES "user"("id") | ||
| ON DELETE CASCADE | ||
| ON UPDATE NO ACTION` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add handling for missing employee records.
This migration enforces a NOT NULL foreign key relationship with the employee table but doesn’t handle scenarios where an existing dashboard might not have a matching employee. In similar past PRs, you mentioned logging missing user-employee mappings. Consider applying that solution here to avoid referential integrity breaks.
|
View your CI Pipeline Execution ↗ for commit 986558b.
☁️ Nx Cloud last updated this comment at |
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
New Features
Refactor
Chores