[Feat] Organization Project Modules management (FIx module creation && update) #8575
Conversation
…fix/#8532-adding-manager
WalkthroughThis pull request introduces significant modifications across several files, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (6)
packages/core/src/tasks/task.service.ts (1)
71-84: Consider refactoring therelationsobject to reduce duplicationThe
relationsobject is being explicitly defined here. Since a similarrelationsobject is also defined inautomation-task.sync.handler.ts, consider extracting this into a shared constant or utility function to adhere to the DRY principle and improve maintainability.packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1)
180-193: Consider refactoring therelationsobject to reduce duplicationThe
relationsobject defined here is identical to the one intask.service.ts. To adhere to the DRY principle, consider extracting this common definition into a shared constant or utility function to avoid code duplication and enhance maintainability.packages/core/src/organization-project/organization-project.service.ts (1)
Line range hint
478-484: Inconsistent use of 'role' and 'roleId' propertiesWhen updating existing members, you use the
roleproperty, but when adding new members, you useroleId. This inconsistency may lead to issues in how roles are assigned to members.Ensure consistent property usage by modifying the update operation:
// In the update of existing members await this.typeOrmOrganizationProjectEmployeeRepository.update(member.id, { - role: newRole, + roleId: newRole?.id, isManager });And when adding new members, ensure the same property is used:
new OrganizationProjectEmployee({ organizationProjectId, employeeId: employee.id, tenantId, organizationId, isManager: managerIds.includes(employee.id), - roleId: managerIds.includes(employee.id) ? managerRole.id : null + roleId: managerIds.includes(employee.id) ? managerRole?.id : null })Also applies to: 492-498
packages/core/src/organization-project-module/organization-project-module.service.ts (1)
150-161: Remove unnecessary console logsDebugging statements like
console.logshould be removed from production code to maintain code cleanliness and prevent unintended information disclosure.Apply this diff to remove the console logs:
- console.log(Array.isArray(memberIds), Array.isArray(managerIds));packages/core/src/organization-project-module/dto/create-organization-project-module.dto.ts (1)
12-12: Clarify the usage of IntersectionType in DTO definitionThe nested
IntersectionTypeusage may lead to complexity in the DTO structure. Consider simplifying the DTO definition for better readability and maintainability.Refactor the DTO definition as needed to make it more straightforward, possibly by flattening or reorganizing the types.
packages/core/src/resource-link/resource-link.service.ts (1)
83-83: Adjust exception type for resource not foundIn the
updatemethod, changing the exception fromNotFoundExceptiontoBadRequestExceptionmay not accurately represent the error condition. ANotFoundExceptionis more appropriate when the resource does not exist.Consider reverting to
NotFoundExceptionfor clearer semantics:- throw new BadRequestException('Resource Link not found'); + throw new NotFoundException('Resource Link not found');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/contracts/src/employee.model.ts(1 hunks)packages/core/src/organization-project-module/dto/create-organization-project-module.dto.ts(2 hunks)packages/core/src/organization-project-module/organization-project-module.service.ts(7 hunks)packages/core/src/organization-project/organization-project.service.ts(1 hunks)packages/core/src/organization-sprint/organization-sprint.service.ts(1 hunks)packages/core/src/resource-link/resource-link.service.ts(1 hunks)packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts(2 hunks)packages/core/src/tasks/task.service.ts(2 hunks)packages/ui-core/shared/src/lib/project/project-module/add-project-module-dialog/add-project-module-dialog.component.ts(1 hunks)packages/ui-core/shared/src/lib/project/project-module/module-table/project-module-table.component.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts/src/employee.model.ts (1)
Learnt from: GloireMutaliko21
PR: ever-co/ever-gauzy#8573
File: packages/contracts/src/employee.model.ts:293-296
Timestamp: 2024-12-05T09:19:04.024Z
Learning: In this codebase, it's acceptable for `IMemberEntityBased` to not extend `IBasePerTenantAndOrganizationEntityModel` because the child interfaces already include these properties.
🔇 Additional comments (6)
packages/core/src/tasks/task.service.ts (1)
11-12: Import statements are appropriate
The import of Between and FindOptionsRelations from 'typeorm' is necessary for the subsequent usage in the code.
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1)
3-3: Import statement is appropriate
The import of FindOptionsRelations is necessary for defining the relations object in the code.
packages/contracts/src/employee.model.ts (1)
293-296: LGTM! Interface decoupling improves separation of concerns.
The decoupling of IMemberEntityBased from IBasePerTenantAndOrganizationEntityModel is a good architectural decision as it promotes better separation of concerns and reduces inheritance complexity. This aligns with the established pattern where child interfaces already include tenant and organization properties.
packages/core/src/organization-sprint/organization-sprint.service.ts (1)
154-154: LGTM! Explicit relations improve code clarity and type safety.
The explicit specification of relations (project, members, modules) is better than dynamic mapping as it:
- Provides better clarity about which related entities are being loaded
- Ensures type safety
- Makes the code more maintainable by making dependencies visible
packages/ui-core/shared/src/lib/project/project-module/add-project-module-dialog/add-project-module-dialog.component.ts (1)
233-235: LGTM! Improved manager validation ensures data integrity.
The explicit setting of filtered managerIds to the form control is a good improvement as it:
- Validates that managers exist in the employees list
- Prevents invalid manager assignments
- Maintains data integrity in the form
packages/core/src/organization-project/organization-project.service.ts (1)
158-160: Ensure all necessary relations are included when fetching the project
In the update method, you fetch the organizationProject with specified relations. Verify that all required relations are included to avoid incomplete data issues during the update.
Run the following script to check for any missing relations:
✅ Verification successful
The current relations configuration is appropriate for the update operation
Based on the entity definition and update method implementation, the current relations being loaded (image, members, organizationContact, tags, teams) are sufficient for the update operation. These relations match the entity's structure and are actively used in the update process, particularly for member management and team associations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for relations used in the project entity that might be missing.
# Search for usages of 'organizationProject' properties that require relations.
rg -A 5 'organizationProject\.' packages/core/src/organization-project/organization-project.service.ts | grep -E '\.(members|tasks|teams|tags|image|organizationContact)\b'
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to analyze the relations:
# 1. First, let's see the entity definition to understand all possible relations
echo "=== OrganizationProject Entity Definition ==="
ast-grep --pattern 'class OrganizationProject {
$$$
}'
# 2. Then, let's check the update method implementation to see how these relations are used
echo -e "\n=== Update Method Implementation ==="
ast-grep --pattern 'async update($$$) {
$$$
}'
# 3. Finally, let's see all usages of organizationProject in the service
echo -e "\n=== organizationProject Usages ==="
rg "organizationProject\." -B 2 -A 2 packages/core/src/organization-project/organization-project.service.ts
Length of output: 494
Script:
#!/bin/bash
# Let's try a more specific approach to find the entity file and analyze its relations
# 1. Find the entity file
echo "=== Finding OrganizationProject Entity File ==="
fd "organization-project.entity.ts"
# 2. If found, let's examine its content
echo -e "\n=== Entity Relations ==="
rg "@(ManyToOne|OneToMany|ManyToMany|OneToOne)" -B 1 -A 1 $(fd "organization-project.entity.ts")
# 3. Let's also check the update method specifically
echo -e "\n=== Update Method Content ==="
rg "async update" -B 2 -A 10 packages/core/src/organization-project/organization-project.service.ts
Length of output: 2651
Script:
#!/bin/bash
# Let's examine the entity file content to see all relations and their usage
echo "=== Entity File Content ==="
cat packages/core/src/organization-project/organization-project.entity.ts
echo -e "\n=== Update Method Usage of Relations ==="
# Look for any property access after the findOneByIdString call
rg -A 15 "const organizationProject = await super.findOneByIdString" packages/core/src/organization-project/organization-project.service.ts
Length of output: 13838
| managers: { | ||
| title: this.getTranslation('ORGANIZATIONS_PAGE.EDIT.TEAMS_PAGE.MANAGERS'), | ||
| type: 'custom', | ||
| isFilterable: false, | ||
| renderComponent: EmployeeWithLinksComponent, | ||
| componentInitFunction: (instance: EmployeeWithLinksComponent, cell: Cell) => { | ||
| componentInitFunction: (instance: EmployeesMergedTeamsComponent, cell: Cell) => { | ||
| instance.rowData = cell.getRow().getData(); | ||
| instance.value = cell.getRawValue(); |
There was a problem hiding this comment.
Inconsistent component types in 'managers' column configuration
The renderComponent is set to EmployeeWithLinksComponent, but the componentInitFunction uses EmployeesMergedTeamsComponent. This mismatch could lead to runtime errors or unexpected behavior.
Apply this diff to align the component types:
managers: {
title: this.getTranslation('ORGANIZATIONS_PAGE.EDIT.TEAMS_PAGE.MANAGERS'),
type: 'custom',
isFilterable: false,
- renderComponent: EmployeeWithLinksComponent,
+ renderComponent: EmployeesMergedTeamsComponent,
componentInitFunction: (instance: EmployeesMergedTeamsComponent, cell: Cell) => {
instance.rowData = cell.getRow().getData();
instance.value = cell.getRawValue();
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| managers: { | |
| title: this.getTranslation('ORGANIZATIONS_PAGE.EDIT.TEAMS_PAGE.MANAGERS'), | |
| type: 'custom', | |
| isFilterable: false, | |
| renderComponent: EmployeeWithLinksComponent, | |
| componentInitFunction: (instance: EmployeeWithLinksComponent, cell: Cell) => { | |
| componentInitFunction: (instance: EmployeesMergedTeamsComponent, cell: Cell) => { | |
| instance.rowData = cell.getRow().getData(); | |
| instance.value = cell.getRawValue(); | |
| managers: { | |
| title: this.getTranslation('ORGANIZATIONS_PAGE.EDIT.TEAMS_PAGE.MANAGERS'), | |
| type: 'custom', | |
| isFilterable: false, | |
| renderComponent: EmployeesMergedTeamsComponent, | |
| componentInitFunction: (instance: EmployeesMergedTeamsComponent, cell: Cell) => { | |
| instance.rowData = cell.getRow().getData(); | |
| instance.value = cell.getRawValue(); |
| getNonManagerEmployees(projectModule: IOrganizationProjectModule): IEmployee[][] { | ||
| return [ | ||
| projectModule.members | ||
| .filter((member: IOrganizationProjectModuleEmployee) => !member.isManager) | ||
| .map((member: IOrganizationProjectModuleEmployee) => member.employee) | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify return type in 'getNonManagerEmployees' method
The getNonManagerEmployees method returns an array containing a single array of employees (IEmployee[][]), which introduces unnecessary nesting. Consider returning a flat array of employees (IEmployee[]) to simplify data handling.
Apply this diff to simplify the return value:
- getNonManagerEmployees(projectModule: IOrganizationProjectModule): IEmployee[][] {
- return [
- projectModule.members
- .filter((member: IOrganizationProjectModuleEmployee) => !member.isManager)
- .map((member: IOrganizationProjectModuleEmployee) => member.employee)
- ];
+ getNonManagerEmployees(projectModule: IOrganizationProjectModule): IEmployee[] {
+ return projectModule.members
+ .filter((member: IOrganizationProjectModuleEmployee) => !member.isManager)
+ .map((member: IOrganizationProjectModuleEmployee) => member.employee);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getNonManagerEmployees(projectModule: IOrganizationProjectModule): IEmployee[][] { | |
| return [ | |
| projectModule.members | |
| .filter((member: IOrganizationProjectModuleEmployee) => !member.isManager) | |
| .map((member: IOrganizationProjectModuleEmployee) => member.employee) | |
| ]; | |
| } | |
| getNonManagerEmployees(projectModule: IOrganizationProjectModule): IEmployee[] { | |
| return projectModule.members | |
| .filter((member: IOrganizationProjectModuleEmployee) => !member.isManager) | |
| .map((member: IOrganizationProjectModuleEmployee) => member.employee); | |
| } |
| async updateOrganizationProjectModuleMembers( | ||
| organizationProjectModuleId: ID, | ||
| organizationId: ID, | ||
| employees: IEmployee[], | ||
| managerIds: ID[], | ||
| memberIds: ID[] | ||
| ): Promise<void> { | ||
| const tenantId = RequestContext.currentTenantId(); | ||
| const membersToUpdate = new Set([...managerIds, ...memberIds].filter(Boolean)); | ||
|
|
||
| // Find the manager role | ||
| const managerRole = await this._roleService.findOneByWhereOptions({ | ||
| name: RolesEnum.MANAGER | ||
| }); | ||
|
|
||
| // Fetch existing project members with their roles | ||
| const projectMembers = await this.typeOrmOrganizationProjectModuleEmployeeRepository.find({ | ||
| where: { tenantId, organizationId, organizationProjectModuleId } | ||
| }); | ||
|
|
||
| // Create a map of existing members for quick lookup | ||
| const existingMemberMap = new Map(projectMembers.map((member) => [member.employeeId, member])); | ||
|
|
||
| // Separate members into removed, updated, and new members | ||
| const removedMembers = projectMembers.filter((member) => !membersToUpdate.has(member.employeeId)); | ||
| const updatedMembers = projectMembers.filter((member) => membersToUpdate.has(member.employeeId)); | ||
| const newMembers = employees.filter((employee) => !existingMemberMap.has(employee.id)); | ||
|
|
||
| // 1. Remove members who are no longer assigned to the project | ||
| if (removedMembers.length) { | ||
| await this.deleteMemberByIds(removedMembers.map((member) => member.id)); | ||
| } | ||
|
|
||
| // 2. Update roles for existing members where necessary | ||
| await Promise.all( | ||
| updatedMembers.map(async (member) => { | ||
| const isManager = managerIds.includes(member.employeeId); | ||
| const newRole = isManager ? managerRole : null; | ||
|
|
||
| // Only update if the role has changed | ||
| if (newRole?.id !== member.roleId) { | ||
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.update(member.id, { | ||
| role: newRole, | ||
| isManager | ||
| }); | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| // 3. Add new members to the project | ||
| if (newMembers.length) { | ||
| const newProjectMembers = newMembers.map( | ||
| (employee) => | ||
| new OrganizationProjectModuleEmployee({ | ||
| organizationProjectModuleId, | ||
| employeeId: employee.id, | ||
| tenantId, | ||
| organizationId, | ||
| isManager: managerIds.includes(employee.id), | ||
| roleId: managerIds.includes(employee.id) ? managerRole.id : null | ||
| }) | ||
| ); | ||
|
|
||
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.save(newProjectMembers); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent use of 'role' and 'roleId' properties
Similar to the OrganizationProjectService, there is inconsistency in using role and roleId when updating and adding module members. This may lead to incorrect role assignments.
Modify the update and creation of members to use consistent property names:
// Update existing members
await this.typeOrmOrganizationProjectModuleEmployeeRepository.update(member.id, {
- role: newRole,
+ roleId: newRole?.id,
isManager
});
// Add new members
new OrganizationProjectModuleEmployee({
organizationProjectModuleId,
employeeId: employee.id,
tenantId,
organizationId,
isManager: managerIds.includes(employee.id),
- roleId: managerIds.includes(employee.id) ? managerRole.id : null
+ roleId: managerIds.includes(employee.id) ? managerRole?.id : null
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async updateOrganizationProjectModuleMembers( | |
| organizationProjectModuleId: ID, | |
| organizationId: ID, | |
| employees: IEmployee[], | |
| managerIds: ID[], | |
| memberIds: ID[] | |
| ): Promise<void> { | |
| const tenantId = RequestContext.currentTenantId(); | |
| const membersToUpdate = new Set([...managerIds, ...memberIds].filter(Boolean)); | |
| // Find the manager role | |
| const managerRole = await this._roleService.findOneByWhereOptions({ | |
| name: RolesEnum.MANAGER | |
| }); | |
| // Fetch existing project members with their roles | |
| const projectMembers = await this.typeOrmOrganizationProjectModuleEmployeeRepository.find({ | |
| where: { tenantId, organizationId, organizationProjectModuleId } | |
| }); | |
| // Create a map of existing members for quick lookup | |
| const existingMemberMap = new Map(projectMembers.map((member) => [member.employeeId, member])); | |
| // Separate members into removed, updated, and new members | |
| const removedMembers = projectMembers.filter((member) => !membersToUpdate.has(member.employeeId)); | |
| const updatedMembers = projectMembers.filter((member) => membersToUpdate.has(member.employeeId)); | |
| const newMembers = employees.filter((employee) => !existingMemberMap.has(employee.id)); | |
| // 1. Remove members who are no longer assigned to the project | |
| if (removedMembers.length) { | |
| await this.deleteMemberByIds(removedMembers.map((member) => member.id)); | |
| } | |
| // 2. Update roles for existing members where necessary | |
| await Promise.all( | |
| updatedMembers.map(async (member) => { | |
| const isManager = managerIds.includes(member.employeeId); | |
| const newRole = isManager ? managerRole : null; | |
| // Only update if the role has changed | |
| if (newRole?.id !== member.roleId) { | |
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.update(member.id, { | |
| role: newRole, | |
| isManager | |
| }); | |
| } | |
| }) | |
| ); | |
| // 3. Add new members to the project | |
| if (newMembers.length) { | |
| const newProjectMembers = newMembers.map( | |
| (employee) => | |
| new OrganizationProjectModuleEmployee({ | |
| organizationProjectModuleId, | |
| employeeId: employee.id, | |
| tenantId, | |
| organizationId, | |
| isManager: managerIds.includes(employee.id), | |
| roleId: managerIds.includes(employee.id) ? managerRole.id : null | |
| }) | |
| ); | |
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.save(newProjectMembers); | |
| } | |
| } | |
| async updateOrganizationProjectModuleMembers( | |
| organizationProjectModuleId: ID, | |
| organizationId: ID, | |
| employees: IEmployee[], | |
| managerIds: ID[], | |
| memberIds: ID[] | |
| ): Promise<void> { | |
| const tenantId = RequestContext.currentTenantId(); | |
| const membersToUpdate = new Set([...managerIds, ...memberIds].filter(Boolean)); | |
| // Find the manager role | |
| const managerRole = await this._roleService.findOneByWhereOptions({ | |
| name: RolesEnum.MANAGER | |
| }); | |
| // Fetch existing project members with their roles | |
| const projectMembers = await this.typeOrmOrganizationProjectModuleEmployeeRepository.find({ | |
| where: { tenantId, organizationId, organizationProjectModuleId } | |
| }); | |
| // Create a map of existing members for quick lookup | |
| const existingMemberMap = new Map(projectMembers.map((member) => [member.employeeId, member])); | |
| // Separate members into removed, updated, and new members | |
| const removedMembers = projectMembers.filter((member) => !membersToUpdate.has(member.employeeId)); | |
| const updatedMembers = projectMembers.filter((member) => membersToUpdate.has(member.employeeId)); | |
| const newMembers = employees.filter((employee) => !existingMemberMap.has(employee.id)); | |
| // 1. Remove members who are no longer assigned to the project | |
| if (removedMembers.length) { | |
| await this.deleteMemberByIds(removedMembers.map((member) => member.id)); | |
| } | |
| // 2. Update roles for existing members where necessary | |
| await Promise.all( | |
| updatedMembers.map(async (member) => { | |
| const isManager = managerIds.includes(member.employeeId); | |
| const newRole = isManager ? managerRole : null; | |
| // Only update if the role has changed | |
| if (newRole?.id !== member.roleId) { | |
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.update(member.id, { | |
| roleId: newRole?.id, | |
| isManager | |
| }); | |
| } | |
| }) | |
| ); | |
| // 3. Add new members to the project | |
| if (newMembers.length) { | |
| const newProjectMembers = newMembers.map( | |
| (employee) => | |
| new OrganizationProjectModuleEmployee({ | |
| organizationProjectModuleId, | |
| employeeId: employee.id, | |
| tenantId, | |
| organizationId, | |
| isManager: managerIds.includes(employee.id), | |
| roleId: managerIds.includes(employee.id) ? managerRole?.id : null | |
| }) | |
| ); | |
| await this.typeOrmOrganizationProjectModuleEmployeeRepository.save(newProjectMembers); | |
| } | |
| } |
| const projectModule = await super.create({ | ||
| ...input, | ||
| members, | ||
| creatorId | ||
| }); | ||
| console.log(input.tasks,projectModule ) |
There was a problem hiding this comment.
@samuelmbabhazi Please remove console.log
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
Release Notes
New Features
Bug Fixes
Documentation
Refactor