[Fix] Foreign Key Constraint Violation in project_module_task During Project Module Creation #8776
Conversation
WalkthroughThis pull request enhances task management for project modules. A new optional Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Nitpick comments (5)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (3)
44-45: Optional naming consistency
These private service fields start with an underscore. If there's no code-base standard for underscores, consider removing them for clarity.
147-148: Consider optimizing task updates
This loop-based update approach is straightforward, but if the number of tasks grows significantly, multiple calls to_taskService.updatemight affect performance. A batched approach could minimize database round trips.Also applies to: 150-150, 152-153, 155-155, 160-162, 166-168, 172-172
642-656: Ensure unique module references
Pushing the projectModule onto each task’smodulesarray might cause duplicates if the method is called repeatedly. Consider using a set or checking for existence first.- task.modules.push(projectModule); + if (!task.modules.find(module => module.id === projectModule.id)) { + task.modules.push(projectModule); + }packages/core/src/lib/organization-project-module/organization-project-module.entity.ts (2)
193-193: Consider adding index definitions for the join table.The join table
project_module_taskmight benefit from indexes on foreign key columns to improve query performance and maintain referential integrity.-@JoinTable({ name: 'project_module_task' }) +@JoinTable({ + name: 'project_module_task', + // Add indexes for better performance + indices: [ + { columns: ['organizationProjectModuleId'] }, + { columns: ['taskId'] } + ] +})
184-186: Enhance API documentation for tasks property.The current API documentation could be more descriptive. Consider adding more details about the relationship and its implications.
-@ApiPropertyOptional({ type: () => Array, isArray: true, description: 'List of task IDs' }) +@ApiPropertyOptional({ + type: () => Array, + isArray: true, + description: 'List of tasks associated with this project module. Changes to this list will affect the task-module relationships.' +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/lib/organization-project-module/organization-project-module.entity.ts(1 hunks)packages/core/src/lib/organization-project-module/organization-project-module.module.ts(2 hunks)packages/core/src/lib/organization-project-module/organization-project-module.service.ts(8 hunks)packages/ui-core/shared/src/lib/project-module/project-module-table/project-module-table.module.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 (15)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (12)
2-2: Approved usage of new imports.
All newly introduced imports fromtypeorm,IEmployee, andITaskappear correct and properly utilized.Also applies to: 16-17
33-33: New TaskService import
Looks correct and consistent with the usage in_taskService.
62-62: Destructure tasks
Includingtasks = []in the destructuring ensures a default empty array, which is helpful.
65-65: Check logic inaddCurrentEmployeeToManagers
Calling this method for employees with the 'EMPLOYEE' role and automatically promoting them to managers might be unintentional. Please verify the intended user flow.
74-75: Task assignment and logging
The calls togetExistingTasks(...),buildModuleMembers(...),assignTasksToModule(...), and finallylogModuleActivity(...)are logically structured and appear correct.Also applies to: 83-83, 85-85
114-114: Default tasks array
Destructuringtasks = []in the update method is consistent with the create method.
118-118: Fetching tasks in relational query
Addingtasks: trueto the relations ensures the existing tasks are retrieved for update. Good approach.
125-125: Clear inline documentation
These comments clarify the logic for updating members and managers. The approach is straightforward and helps readability.Also applies to: 127-127, 129-129
179-179: Activity log for updates
The logging call effectively captures changes for tracking.Also applies to: 181-181
599-610: Potential missing organization checks
While this retrieves tasks by ID, consider validating that these tasks belong to the same organization to avoid potential cross-tenant data leaks.
612-640: Manager role assignment
This method concisely creates module members with manager roles where needed. The logic looks good and consistent.
658-685: Comprehensive activity logging
Logging old and new states provides clarity in activity logs. Implementation looks thorough.packages/ui-core/shared/src/lib/project-module/project-module-table/project-module-table.module.ts (1)
3-3: Improved module imports
The addition of Nebular modules (NbButtonModule,NbIconModule,NbSpinnerModule) and translation (TranslateModule.forChild()) extends the functionality of the table module. Good job integrating these.Also applies to: 7-7, 12-20
packages/core/src/lib/organization-project-module/organization-project-module.module.ts (1)
15-15: Integration with TaskModule
IncludingTaskModulein the imports ensures the new task management features can be leveraged within project modules.Also applies to: 25-25
packages/core/src/lib/organization-project-module/organization-project-module.entity.ts (1)
184-194: Verify cascade behavior for task relationships.The cascade configuration could potentially lead to unintended data loss. When a project module is deleted, all associated task relationships will be deleted due to
onDelete: 'CASCADE'. Ensure this is the intended behavior, as it might be safer to useSET NULLto preserve task data.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for existing task relationships that might be affected by cascade delete # Search for references to project_module_task table in migrations or schema rg -A 5 "project_module_task" # Search for any existing delete cascade configurations ast-grep --pattern 'onDelete: "CASCADE"'Length of output: 18618
Cascade behavior verification for task relationships
The migration files and entity definition confirm that the current cascade configuration is applied solely to the join table records. This means that deleting a project module (or a task) will automatically remove only the corresponding entries in the "project_module_task" table—leaving the standalone Task records intact. This behavior is standard for many‑to‑many relationships, but please double‑check that it aligns with your intended data integrity requirements.
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 128cc85.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (1)
62-84:⚠️ Potential issueFix inverted role check in addCurrentEmployeeToManagers.
Based on previous review comments, the role check in
addCurrentEmployeeToManagersis inverted. The method checks for EMPLOYEE role before adding as manager, which seems counterintuitive.The role check should be for MANAGER role instead:
-const currentRole = await this._roleService.findOneByIdString(currentRoleId, { - where: { name: RolesEnum.EMPLOYEE } -}); +const managerRole = await this._roleService.findOneByIdString(currentRoleId, { + where: { name: RolesEnum.MANAGER } +});
🧹 Nitpick comments (2)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (2)
33-33: Consider using a more specific import path.The import path
'../tasks'is using a barrel import. While this works, it's better to use the specific file path for better maintainability and to avoid potential circular dependencies.-import { TaskService } from '../tasks'; +import { TaskService } from '../tasks/task.service';
664-685: Consider adding error handling for activity logging.The
logModuleActivitymethod should handle potential failures in the activity logging service.private logModuleActivity( action: ActionTypeEnum, updatedModule: IOrganizationProjectModule, existingModule?: IOrganizationProjectModule, changes?: Partial<IOrganizationProjectModuleUpdateInput> ): void { const tenantId = RequestContext.currentTenantId(); const organizationId = updatedModule.organizationId; + try { this.activityLogService.logActivity<OrganizationProjectModule>( BaseEntityEnum.OrganizationProjectModule, action, ActorTypeEnum.User, updatedModule.id, updatedModule.name, updatedModule, organizationId, tenantId, existingModule, changes ); + } catch (error) { + console.error(`Failed to log module activity: ${error.message}`); + // Consider whether to throw or just log the error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Outdated
Show resolved
Hide resolved
...es/ui-core/shared/src/lib/project-module/project-module-table/project-module-table.module.ts
Show resolved
Hide resolved
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (2)
582-597:⚠️ Potential issueFix inverted role check logic.
The method checks for EMPLOYEE role before adding as a manager, which seems incorrect. It should check for MANAGER role instead.
- const currentRole = await this._roleService.findOneByIdString(currentRoleId, { - where: { name: RolesEnum.EMPLOYEE } - }); - if (currentRole && !managerIds.includes(employeeId)) { + const managerRole = await this._roleService.findOneByIdString(currentRoleId, { + where: { name: RolesEnum.MANAGER } + }); + if (managerRole && !managerIds.includes(employeeId)) {
146-170: 🛠️ Refactor suggestionImprove error handling for task updates.
The task update logic should handle potential failures more gracefully and be wrapped in a transaction.
- // Update tasks logic - if (Array.isArray(tasks)) { - const existingTasks = await this.getExistingTasks(tasks); - - // Determine tasks to add - const existingTaskIds = new Set(existingTasks.map((task) => task.id)); - const newTasks = tasks.filter((task) => !existingTaskIds.has(task.id)); - - // Determine tasks to remove - const tasksToRemove = existingProjectModule.tasks.filter( - (task) => !tasks.some((updatedTask) => updatedTask.id === task.id) - ); - - // Add new tasks - for (const task of newTasks) { - task.modules = [...(task.modules || []), existingProjectModule]; - await this._taskService.update(task.id, task); - } - - // Remove tasks - for (const task of tasksToRemove) { - task.modules = task.modules?.filter((module) => module.id !== existingProjectModule.id) || []; - await this._taskService.update(task.id, task); - } - } + // Update tasks logic + if (Array.isArray(tasks)) { + const queryRunner = this.dataSource.createQueryRunner(); + await queryRunner.connect(); + await queryRunner.startTransaction(); + + try { + const existingTasks = await this.getExistingTasks(tasks); + + // Determine tasks to add + const existingTaskIds = new Set(existingTasks.map((task) => task.id)); + const newTasks = tasks.filter((task) => !existingTaskIds.has(task.id)); + + // Determine tasks to remove + const tasksToRemove = existingProjectModule.tasks.filter( + (task) => !tasks.some((updatedTask) => updatedTask.id === task.id) + ); + + // Add new tasks + for (const task of newTasks) { + try { + task.modules = [...(task.modules || []), existingProjectModule]; + await this._taskService.update(task.id, task); + } catch (error) { + throw new Error(`Failed to add task ${task.id}: ${error.message}`); + } + } + + // Remove tasks + for (const task of tasksToRemove) { + try { + task.modules = task.modules?.filter((module) => module.id !== existingProjectModule.id) || []; + await this._taskService.update(task.id, task); + } catch (error) { + throw new Error(`Failed to remove task ${task.id}: ${error.message}`); + } + } + + await queryRunner.commitTransaction(); + } catch (error) { + await queryRunner.rollbackTransaction(); + throw new Error(`Failed to update tasks: ${error.message}`); + } finally { + await queryRunner.release(); + } + }
🧹 Nitpick comments (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (1)
33-33: Use absolute import path for TaskService.To prevent potential circular dependencies, use the absolute import path.
-import { TaskService } from '../tasks/task.service'; +import { TaskService } from '@gauzy/core';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (2)
597-612:⚠️ Potential issueFix inverted role check in addCurrentEmployeeToManagers.
The method checks for EMPLOYEE role before adding as manager, which seems incorrect. This is a duplicate of a previous review comment that hasn't been addressed.
- try { - const currentRole = await this._roleService.findOneByIdString(currentRoleId, { - where: { name: RolesEnum.EMPLOYEE } - }); - if (currentRole && !managerIds.includes(employeeId)) { - managerIds.push(employeeId); - } - } catch { - // Role is not "EMPLOYEE" or no action needed. - } + try { + const managerRole = await this._roleService.findOneByIdString(currentRoleId, { + where: { name: RolesEnum.MANAGER } + }); + if (managerRole && !managerIds.includes(employeeId)) { + managerIds.push(employeeId); + } + } catch { + // Role is not "MANAGER" or no action needed. + }
662-671:⚠️ Potential issueEnhance error handling in assignTasksToModule.
The method should handle potential failures during task updates more gracefully. This is a duplicate of a previous review comment that hasn't been addressed.
- private async assignTasksToModule(tasks: ITask[], projectModule: IOrganizationProjectModule): Promise<void> { - const taskUpdates = tasks.map((task) => { - if (!task.modules) { - task.modules = []; - } - task.modules.push(projectModule); - return this._taskService.update(task.id, { ...task }); - }); - await Promise.all(taskUpdates); + private async assignTasksToModule(tasks: ITask[], projectModule: IOrganizationProjectModule): Promise<void> { + try { + const taskUpdates = tasks.map(async (task) => { + try { + if (!task.modules) { + task.modules = []; + } + task.modules.push(projectModule); + await this._taskService.update(task.id, { ...task }); + } catch (error) { + throw new Error(`Failed to update task ${task.id}: ${error.message}`); + } + }); + await Promise.all(taskUpdates); + } catch (error) { + throw new Error(`Failed to assign tasks to module: ${error.message}`); + } }
🧹 Nitpick comments (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts (1)
42-42: Use full path for TaskService import to prevent circular dependencies.Consider using the full path for the TaskService import to prevent potential circular dependencies.
-import { TaskService } from '../tasks/task.service'; +import { TaskService } from '@gauzy/core/src/tasks/task.service';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/organization-project-module/organization-project-module.service.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
packages/core/src/lib/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
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
Improvements
Bug Fixes