[Fix] Alter "ScreeningTask" Entity Table #8801
Conversation
|
Warning Rate limit exceeded@rahul-rocket has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request refactors the screening task interfaces and related DTOs and entities to improve modularity and clarity. Changes include splitting an interface into two parts, adjusting type omission logic, and renaming properties. Additionally, it adds robust error handling to task creation and update handlers, updates subscriber registration and routing definitions, introduces a new database migration for screening task table alterations, and updates service constructor parameter ordering and task creation logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/Client
participant Controller as ScreeningTasksController
participant Handler as ScreeningTaskCreateHandler
participant Service as ScreeningTasksService
participant EventBus as EventBus
participant Subscriber as ScreeningTaskSubscriber
participant DB as Database
Client->>Controller: POST /screening-tasks (create command)
Controller->>Handler: execute(create command)
Handler->>Service: create(data)
Service->>EventBus: publish(CreateSubscriptionEvent)
EventBus->>Subscriber: beforeEntityCreate(entity)
Subscriber->>Service: Set createdByUserId on entity
Service->>DB: Insert ScreeningTask
DB-->>Service: Return confirmation
Service-->>Handler: Return new ScreeningTask
Handler-->>Controller: Return new ScreeningTask
Controller-->>Client: HTTP response with new task
Possibly related PRs
Poem
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: 5
🔭 Outside diff range comments (2)
packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts (2)
48-153: 🛠️ Refactor suggestionRefactor create method to improve maintainability.
The create method is handling too many responsibilities. Consider extracting some operations into separate private methods.
+ private async createAssociatedTask(data: any, tenantId: string, organizationId: string, project: any): Promise<Task> { + const prefix = project?.name?.substring(0, 3) ?? null; + const maxNumber = await this.taskService.getMaxTaskNumberByProject({ + tenantId, + organizationId, + projectId: project?.id ?? null + }); + return this.taskService.create({ + ...data.task, + prefix, + number: maxNumber + 1, + isScreeningTask: true, + tenantId, + organizationId + }); + } + private async handleMentionsAndSubscriptions( + task: Task, + mentionEmployeeIds: ID[], + createdByUserId: string, + organizationId: string, + tenantId: string + ): Promise<void> { + const mentionPromises = mentionEmployeeIds.map((mentionedUserId: ID) => + this.mentionService.publishMention({ + entity: BaseEntityEnum.Task, + entityId: task.id, + entityName: task.title, + mentionedUserId, + mentionById: createdByUserId + }) + ); + + this.eventBus.publish( + new CreateSubscriptionEvent({ + entity: BaseEntityEnum.Task, + entityId: task.id, + userId: createdByUserId, + type: SubscriptionTypeEnum.CREATED_ENTITY, + organizationId, + tenantId + }) + ); + + await Promise.all(mentionPromises); + } async create(input: IScreeningTaskCreateInput): Promise<IScreeningTask> { try { const createdByUserId = RequestContext.currentUserId(); const tenantId = RequestContext.currentTenantId() ?? input.tenantId; const { organizationId, mentionEmployeeIds = [], ...data } = input; const project = data.task.projectId ? await this.organizationProjectService.findOneByIdString(data.task.projectId) : data.task.project || null; if (!project) { console.warn('No projectId or project provided. Proceeding without project information'); } const task = await this.createAssociatedTask(data, tenantId, organizationId, project); const screeningTask = await super.create({ ...data, status: ScreeningTaskStatusEnum.PENDING, taskId: task.id, organizationId, tenantId }); await Promise.all([ this.handleMentionsAndSubscriptions( task, mentionEmployeeIds, createdByUserId, organizationId, tenantId ), this.activityLogService.logActivity<Task>(/* ... */), this.activityLogService.logActivity<ScreeningTask>(/* ... */) ]); return screeningTask; } catch (error) { - throw new HttpException('Screening task creation failed', HttpStatus.BAD_REQUEST); + throw new HttpException( + `Screening task creation failed: ${error.message}`, + error instanceof HttpException ? error.getStatus() : HttpStatus.BAD_REQUEST + ); } }
163-210: 🛠️ Refactor suggestionImprove error handling in update method.
Similar to the create method, the error handling could be more specific.
} catch (error) { - throw new HttpException(`Failed to update screening task with ID ${id}`, HttpStatus.BAD_REQUEST); + throw new HttpException( + `Failed to update screening task with ID ${id}: ${error.message}`, + error instanceof HttpException ? error.getStatus() : HttpStatus.BAD_REQUEST + ); }
🧹 Nitpick comments (5)
packages/core/src/lib/tasks/screening-tasks/screening-task.subscriber.ts (1)
25-27: Remove redundant null check.The null check on
entityis unnecessary as TypeORM will never call the hook with a null entity.- if (entity) { entity.createdByUserId = RequestContext.currentUserId(); - }packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts (1)
29-39: Consider reordering constructor parameters by dependency importance.The
eventBusparameter has been moved to the end, but it might be better to group related dependencies together.constructor( readonly typeOrmScreeningTaskRepository: TypeOrmScreeningTaskRepository, readonly mikroOrmScreeningTaskRepository: MikroOrmScreeningTaskRepository, - private readonly eventBus: EventBus, private readonly taskService: TaskService, private readonly organizationProjectService: OrganizationProjectService, private readonly mentionService: MentionService, - private readonly activityLogService: ActivityLogService + private readonly activityLogService: ActivityLogService, + private readonly eventBus: EventBus )packages/contracts/src/lib/screening-task.model.ts (1)
26-26: Consider adding JSDoc comments for input interfaces.While the base interfaces are well-documented, the input interfaces could benefit from similar documentation.
Add JSDoc comments:
+/** + * Interface for creating a new screening task, excluding the status field. + */ export interface IScreeningTaskCreateInput extends OmitFields<IScreeningTask, 'status'>, IMentionEmployeeIds {} +/** + * Interface for updating an existing screening task. + */ export interface IScreeningTaskUpdateInput extends IScreeningTaskBase {}Also applies to: 28-28
packages/core/src/lib/database/migrations/1740385271257-AlterScreeningTaskEntityTable.ts (2)
1-4: Consider making chalk logging optional.Using
chalkto colorize console output in a production migration is fine if your environment expects it, but it can clutter logs in certain deployments. Consider making the log call optional or only logging in development mode.
86-168: SQLite up migration approach is standard but long.The temporary table approach is typical, due to SQLite's limited
ALTER TABLEcapabilities. For large datasets, notice that table re-creations and full data copies can be performance-heavy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/contracts/src/lib/screening-task.model.ts(2 hunks)packages/core/src/lib/core/entities/internal.ts(1 hunks)packages/core/src/lib/core/entities/subscribers/index.ts(2 hunks)packages/core/src/lib/database/migrations/1740385271257-AlterScreeningTaskEntityTable.ts(1 hunks)packages/core/src/lib/tasks/screening-tasks/commands/handlers/screening-task.create.handler.ts(2 hunks)packages/core/src/lib/tasks/screening-tasks/commands/handlers/screening-task.update.handler.ts(1 hunks)packages/core/src/lib/tasks/screening-tasks/dto/create-screening-task.dto.ts(1 hunks)packages/core/src/lib/tasks/screening-tasks/screening-task.entity.ts(3 hunks)packages/core/src/lib/tasks/screening-tasks/screening-task.subscriber.ts(1 hunks)packages/core/src/lib/tasks/screening-tasks/screening-tasks.controller.ts(5 hunks)packages/core/src/lib/tasks/screening-tasks/screening-tasks.module.ts(1 hunks)packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/core/src/lib/tasks/screening-tasks/screening-task.entity.ts (1)
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8800
File: packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts:81-81
Timestamp: 2025-02-24T05:25:58.665Z
Learning: The `creatorId` column in the `screening-task` entity will be renamed to `createdByUserId` in a future PR to maintain consistency with the task entity.
packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts (1)
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8800
File: packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts:81-81
Timestamp: 2025-02-24T05:25:58.665Z
Learning: The `creatorId` column in the `screening-task` entity will be renamed to `createdByUserId` in a future PR to maintain consistency with the task entity.
packages/core/src/lib/database/migrations/1740385271257-AlterScreeningTaskEntityTable.ts (1)
Learnt from: rahul-rocket
PR: ever-co/ever-gauzy#8800
File: packages/core/src/lib/database/migrations/1740146592586-AlterOrganizationProjectModuleEntityTable.ts:0-0
Timestamp: 2025-02-24T05:26:43.208Z
Learning: SQLite and MySQL migration implementations for the Task entity table alterations will be added in subsequent commits. This is an intentional decision to implement the changes incrementally.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (18)
packages/core/src/lib/core/entities/internal.ts (1)
203-203: LGTM! The subscriber export is well-placed.The new export for
screening-task.subscriberis correctly placed among other task-related subscribers, maintaining consistent organization.packages/core/src/lib/core/entities/subscribers/index.ts (1)
38-38: LGTM! The subscriber is properly integrated.The
ScreeningTaskSubscriberis correctly added to both the imports andcoreSubscribersarray, maintaining alphabetical ordering.Also applies to: 96-96
packages/core/src/lib/tasks/screening-tasks/dto/create-screening-task.dto.ts (1)
10-12: LGTM! The DTO inheritance structure is simplified.The flattened
IntersectionTypemakes the code more maintainable while preserving the same functionality.packages/core/src/lib/tasks/screening-tasks/screening-tasks.controller.ts (2)
30-30: LGTM! Route paths are now consistently formatted.The route paths have been standardized to include leading slashes, which improves consistency across the API.
Also applies to: 55-55, 78-78, 102-102, 129-129, 154-154
39-44: LGTM! Comprehensive JSDoc comments added.The addition of detailed JSDoc comments for all controller methods improves code documentation and maintainability.
Also applies to: 61-67, 86-91, 108-114, 138-143
packages/contracts/src/lib/screening-task.model.ts (2)
14-17: LGTM! Well-documented base interface.The
IScreeningTaskBaseinterface clearly defines core properties with descriptive comments.
19-22: LGTM! Good separation of concerns.The
IScreeningTaskAssociationsinterface effectively isolates relationship properties.packages/core/src/lib/tasks/screening-tasks/screening-task.entity.ts (3)
21-29: LGTM! Well-documented status property.The status property now has clear documentation explaining its purpose.
31-38: LGTM! New onHoldUntil property with validation.The new property is well-documented and includes appropriate validation decorators.
48-52: LGTM! Clear cascade configuration documentation.The cascade configuration is now well-documented with explanatory comments.
packages/core/src/lib/database/migrations/1740385271257-AlterScreeningTaskEntityTable.ts (8)
5-13: Migration class and naming convention look appropriate.The class name and property
namematch the typical TypeORM migration pattern, and the doc comments provide clarity on the migration purpose.
14-30: Solid multi-database handling in theupmethod.This switch-case pattern is a clear way to handle different database backends. Ensure that all new changes for each database dialect remain consistent as new features evolve.
32-52: Down method consistency check.The
downmethod mirrors theupmethod across the supported databases. This consistency helps maintain reliable rollback if needed.
54-70: Postgres up migration is well-structured.The steps cleanly drop the old constraint, rename the column, and recreate the foreign key. Make sure these constraints match your intended referential actions (e.g.,
NO ACTION) for your business logic.
71-84: Postgres down migration matches the up migration.Reverting the column renaming and constraints is straightforward. This symmetry ensures consistent rollbacks.
170-250: SQLite down migration retains full rollback.This contains a similar temporary table rebuild to revert the changes. The logic looks consistent.
252-269: MySQL up migration: column rename and index re-creation.The rename from
creatorIdtocreatedByUserIdaligns with other DB logic. Confirm that the defaultNO ACTIONon foreign key constraints meets your delete/update semantics.
271-289: MySQL down migration completes the cycle.Rolls back the column rename and constraints properly. No issues spotted here.
packages/core/src/lib/tasks/screening-tasks/screening-task.subscriber.ts
Show resolved
Hide resolved
packages/core/src/lib/tasks/screening-tasks/commands/handlers/screening-task.create.handler.ts
Show resolved
Hide resolved
packages/core/src/lib/tasks/screening-tasks/commands/handlers/screening-task.update.handler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/tasks/screening-tasks/screening-task.entity.ts
Outdated
Show resolved
Hide resolved
…ity.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…screening-task.update.handler.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
View your CI Pipeline Execution ↗ for commit 16727bf.
☁️ 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
Chores