[Feature] Organization Project Modules management (Add method to retrieve project modules by projectId in Organiza…) #8528
Conversation
…tionProjectModuleService
WalkthroughThis pull request introduces a new method Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
@rahul-rocket , |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/ui-core/core/src/lib/services/organization/organization-projects-module.service.ts (1)
48-55: Consider adding error handling and caching.The implementation looks good but could be enhanced with:
- Explicit error handling for failed requests
- Request caching to improve performance when repeatedly fetching modules for the same project
Consider applying this enhancement:
findModulesByProject(projectId: ID): Observable<IOrganizationProjectModule[]> { - return this.http.get<IOrganizationProjectModule[]>(`${this.API_URL}/project/${projectId}`); + return this.http.get<IOrganizationProjectModule[]>( + `${this.API_URL}/project/${projectId}` + ).pipe( + catchError((error: HttpErrorResponse) => { + console.error('Error fetching project modules:', error); + return throwError(() => new Error('Failed to fetch project modules')); + }), + shareReplay(1) // Cache the last result + );Don't forget to add these imports:
import { catchError, shareReplay } from 'rxjs/operators'; import { throwError } from 'rxjs'; import { HttpErrorResponse } from '@angular/common/http';packages/core/src/organization-project-module/organization-project-module.service.ts (1)
362-362: Remove or implement the example join comment.The comment indicates this is an example join that needs adjustment. Either implement the necessary join or remove this line if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/organization-project-module/organization-project-module.controller.ts(1 hunks)packages/core/src/organization-project-module/organization-project-module.service.ts(1 hunks)packages/ui-core/core/src/lib/services/organization/organization-projects-module.service.ts(2 hunks)
🔇 Additional comments (3)
packages/ui-core/core/src/lib/services/organization/organization-projects-module.service.ts (2)
48-52: Documentation and method signature look good!
The JSDoc comments are clear and comprehensive, and the method signature correctly reflects the requirements from the PR objectives.
12-12: Verify the impact of API endpoint change.
The API endpoint has been changed from singular to plural form. While this follows REST conventions better, we need to ensure all consumers are updated accordingly.
✅ Verification successful
Let me analyze the results further by checking the actual usage context.
No impact from API endpoint change - both forms are correctly aligned
The verification shows that the plural form organization-project-modules in the API_URL is actually correct and aligned with the backend routing. Key findings:
- The backend router in
organization-project-module.module.tsalready uses the plural form:/organization-project-modules - The singular form appearances are only in type/class names and file paths, not in API endpoints
- No other services or components are using the old singular form for API calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old singular endpoint
rg -l "organization-project-module" --type ts
# Search for API calls to ensure they're using the new plural endpoint
rg -l "organization-project-modules" --type ts
Length of output: 2119
Script:
#!/bin/bash
# Check the actual usage context of both singular and plural forms
rg "organization-project-module[s]?" -B 2 -A 2 --type ts
Length of output: 17595
packages/core/src/organization-project-module/organization-project-module.controller.ts (1)
140-155: LGTM! Well-documented API endpoint.
The API documentation is comprehensive with:
- Clear JSDoc description
- Proper Swagger annotations
- Appropriate HTTP status codes and responses
| @Permissions(PermissionsEnum.ALL_ORG_VIEW, PermissionsEnum.PROJECT_MODULE_READ) | ||
| @Get('project/:projectId') | ||
| async findModulesByProject( | ||
| @Param('projectId', UUIDValidationPipe) projectId: ID, | ||
| @Query() params: OrganizationProjectModuleFindInputDTO | ||
| ): Promise<IOrganizationProjectModule[]> { | ||
| return await this.organizationProjectModuleService.findModulesByProject(projectId, params); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider pagination for large datasets - confirmed issue
The review comment is valid. Looking at the codebase:
- All other list endpoints in this controller use pagination (
IPagination<IOrganizationProjectModule>) - The
findModulesByProjectendpoint is the only one returning an unpaginated array - The service implementation shows that it uses
getMany()without any limit, which could cause performance issues with large datasets
Key points:
- The endpoint should be consistent with other endpoints in the controller that use
PaginationParams - The service method should use the pagination query execution pattern like other methods
- The API response type should be updated to return
IPagination<IOrganizationProjectModule>
🔗 Analysis chain
Consider pagination for large datasets.
The implementation looks good, but returning an array without pagination could lead to performance issues if the project has many modules.
Consider using the existing IPagination interface like other endpoints in this controller:
- async findModulesByProject(
- @Param('projectId', UUIDValidationPipe) projectId: ID,
- @Query() params: OrganizationProjectModuleFindInputDTO
- ): Promise<IOrganizationProjectModule[]> {
+ async findModulesByProject(
+ @Param('projectId', UUIDValidationPipe) projectId: ID,
+ @Query() params: PaginationParams<OrganizationProjectModule>
+ ): Promise<IPagination<IOrganizationProjectModule>> {Let's verify if other endpoints in the codebase use pagination for similar list operations:
Let me search for more context about the specific endpoint and its service implementation.
Let me check the service implementation and other endpoints in this controller to understand the pagination pattern better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for pagination usage in similar endpoints
rg -A 2 "IPagination<IOrganizationProjectModule>" packages/core/src/
Length of output: 2797
Script:
#!/bin/bash
# Search for findModulesByProject implementation in service
ast-grep --pattern 'findModulesByProject($$$) {
$$$
}'
# Also check if there are any other similar project-specific endpoints
rg -B 2 "project/:projectId" packages/core/src/
Length of output: 483
Script:
#!/bin/bash
# Get the full controller file content to see all endpoints
cat packages/core/src/organization-project-module/organization-project-module.controller.ts
# Get the service implementation
cat packages/core/src/organization-project-module/organization-project-module.service.ts
Length of output: 24224
packages/core/src/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
| async findModulesByProject( | ||
| projectId: ID, | ||
| options: IOrganizationProjectModuleFindInput | ||
| ): Promise<IOrganizationProjectModule[]> { |
There was a problem hiding this comment.
@coderabbitai,
The return type was intentionally set to an array for this method since it will be used for selecting a parent module, and the expected result set is generally limited. Returning an array keeps the response lightweight for this purpose.
However, if maintaining a consistent IPagination return type is essential, I can adjust the implementation accordingly. Let me know if you’d prefer the change.
There was a problem hiding this comment.
@samuelmbabhazi, thank you for the clarification. Keeping the response lightweight by returning an array makes sense in this context. It's acceptable to keep the return type as IOrganizationProjectModule[].
✏️ Learnings added
Learnt from: samuelmbabhazi
PR: ever-co/ever-gauzy#8528
File: packages/core/src/organization-project-module/organization-project-module.service.ts:350-353
Timestamp: 2024-11-06T06:06:04.919Z
Learning: In `OrganizationProjectModuleService`, the `findModulesByProject` method returns an array (`IOrganizationProjectModule[]`) instead of `IPagination<T>` intentionally to keep the response lightweight, as it's used for selecting a parent module and the expected result set is generally limited.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Thanks @coderabbitai
There was a problem hiding this comment.
@samuelmbabhazi, you're welcome! Happy to help.
packages/core/src/organization-project-module/organization-project-module.service.ts
Outdated
Show resolved
Hide resolved
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e321178. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
Sent with 💌 from NxCloud. |
| async findModulesByProject( | ||
| projectId: ID, | ||
| options: IOrganizationProjectModuleFindInput | ||
| ): Promise<IOrganizationProjectModule[]> { |
There was a problem hiding this comment.
@coderabbitai,
The return type was intentionally set to an array for this method since it will be used for selecting a parent module, and the expected result set is generally limited. Returning an array keeps the response lightweight for this purpose.
However, if maintaining a consistent IPagination return type is essential, I can adjust the implementation accordingly. Let me know if you’d prefer the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/organization-project-module/organization-project-module.service.ts (1)
361-364: Remove unnecessary joins unless required by the options parameter.The method performs joins with
members,teams, andsprintstables but doesn't appear to use these relations in the query results. Consider removing these joins unless they're needed based on theoptionsparameter.-// Joins and where clauses -query.innerJoin(`${query.alias}.members`, 'member'); -query.leftJoin(`${query.alias}.teams`, 'project_team'); -query.leftJoin(`${query.alias}."organizationSprints"`, 'sprint'); +// Add joins only if required by options +if (options?.relations?.includes('members')) { + query.leftJoin(`${query.alias}.members`, 'member'); +} +if (options?.relations?.includes('teams')) { + query.leftJoin(`${query.alias}.teams`, 'project_team'); +} +if (options?.relations?.includes('organizationSprints')) { + query.leftJoin(`${query.alias}."organizationSprints"`, 'sprint'); +}
packages/core/src/organization-project-module/organization-project-module.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/core/src/organization-project-module/organization-project-module.service.ts (1)
344-353: Enhance method documentation by including options parameter.The JSDoc comments should document all parameters, including the
optionsparameter which is currently undocumented./** * @description Retrieve all modules for a given project * @param projectId - The ID of the project for which to retrieve modules + * @param options - Options for filtering and customizing the query * @returns A promise that resolves to the list of project modules * @memberof OrganizationProjectModuleService */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/organization-project-module/organization-project-module.service.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/core/src/organization-project-module/organization-project-module.service.ts (1)
Learnt from: samuelmbabhazi
PR: ever-co/ever-gauzy#8528
File: packages/core/src/organization-project-module/organization-project-module.service.ts:350-353
Timestamp: 2024-11-06T06:06:05.086Z
Learning: In `OrganizationProjectModuleService`, the `findModulesByProject` method returns an array (`IOrganizationProjectModule[]`) instead of `IPagination<T>` intentionally to keep the response lightweight, as it's used for selecting a parent module and the expected result set is generally limited.
🔇 Additional comments (2)
packages/core/src/organization-project-module/organization-project-module.service.ts (2)
354-360: LGTM! Input validation and context setup are well implemented.
The code properly validates the projectId and correctly extracts tenant and organization IDs from the context.
379-387: Refer to the previous comment about error handling security.
The error handling still includes sensitive information in the error message. Please address the existing review comment that suggests removing the projectId from the error message.
| // Create query builder | ||
| const query = this.typeOrmProjectModuleRepository.createQueryBuilder(this.tableName); | ||
|
|
||
| // Joins and where clauses | ||
| query.innerJoin(`${query.alias}.members`, 'member'); | ||
| query.leftJoin(`${query.alias}.teams`, 'project_team'); | ||
| query.leftJoin(`${query.alias}."organizationSprints"`, 'sprint'); | ||
|
|
||
| query.andWhere( | ||
| new Brackets((qb: WhereExpressionBuilder) => { | ||
| qb.andWhere(p(`"${query.alias}"."projectId" = :projectId`), { projectId }) | ||
| .andWhere(p(`"${query.alias}"."tenantId" = :tenantId`), { tenantId }) | ||
| .andWhere(p(`"${query.alias}"."organizationId" = :organizationId`), { organizationId }); | ||
| }) | ||
| ); | ||
|
|
||
| console.log('Query to retrieve modules by project:', query.getSql()); // Log query for debugging | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize query construction and remove debug logging.
- The joins to 'members', 'teams', and 'sprints' appear unnecessary as they're not used in the where clause.
- Debug logging should not be in production code.
// Create query builder
const query = this.typeOrmProjectModuleRepository.createQueryBuilder(this.tableName);
-// Joins and where clauses
-query.innerJoin(`${query.alias}.members`, 'member');
-query.leftJoin(`${query.alias}.teams`, 'project_team');
-query.leftJoin(`${query.alias}."organizationSprints"`, 'sprint');
query.andWhere(
new Brackets((qb: WhereExpressionBuilder) => {
qb.andWhere(p(`"${query.alias}"."projectId" = :projectId`), { projectId })
.andWhere(p(`"${query.alias}"."tenantId" = :tenantId`), { tenantId })
.andWhere(p(`"${query.alias}"."organizationId" = :organizationId`), { organizationId });
})
);
-console.log('Query to retrieve modules by project:', query.getSql()); // Log query for debugging📝 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.
| // Create query builder | |
| const query = this.typeOrmProjectModuleRepository.createQueryBuilder(this.tableName); | |
| // Joins and where clauses | |
| query.innerJoin(`${query.alias}.members`, 'member'); | |
| query.leftJoin(`${query.alias}.teams`, 'project_team'); | |
| query.leftJoin(`${query.alias}."organizationSprints"`, 'sprint'); | |
| query.andWhere( | |
| new Brackets((qb: WhereExpressionBuilder) => { | |
| qb.andWhere(p(`"${query.alias}"."projectId" = :projectId`), { projectId }) | |
| .andWhere(p(`"${query.alias}"."tenantId" = :tenantId`), { tenantId }) | |
| .andWhere(p(`"${query.alias}"."organizationId" = :organizationId`), { organizationId }); | |
| }) | |
| ); | |
| console.log('Query to retrieve modules by project:', query.getSql()); // Log query for debugging | |
| // Create query builder | |
| const query = this.typeOrmProjectModuleRepository.createQueryBuilder(this.tableName); | |
| query.andWhere( | |
| new Brackets((qb: WhereExpressionBuilder) => { | |
| qb.andWhere(p(`"${query.alias}"."projectId" = :projectId`), { projectId }) | |
| .andWhere(p(`"${query.alias}"."tenantId" = :tenantId`), { tenantId }) | |
| .andWhere(p(`"${query.alias}"."organizationId" = :organizationId`), { organizationId }); | |
| }) | |
| ); |
…tionProjectModuleService
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
Bug Fixes