[Feat] Task Linked Issue Activity Log #8514
Conversation
WalkthroughThe changes in this pull request introduce a new enumeration value 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (8)
packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-update.handler.ts (1)
6-15: Add class and method documentation.Consider adding JSDoc documentation to improve code maintainability and clarity.
+/** + * Command handler for updating task linked issues. + * @implements {ICommandHandler<TaskLinkedIssueUpdateCommand>} + */ @CommandHandler(TaskLinkedIssueUpdateCommand) export class TaskLinkedIssueUpdateHandler implements ICommandHandler<TaskLinkedIssueUpdateCommand> { constructor(private readonly taskLinkedIssueService: TaskLinkedIssueService) {} + /** + * Executes the update operation for a task linked issue. + * @param {TaskLinkedIssueUpdateCommand} command - The update command containing id and input data + * @returns {Promise<ITaskLinkedIssue>} The updated task linked issue + */ public async execute(command: TaskLinkedIssueUpdateCommand): Promise<ITaskLinkedIssue> {packages/contracts/src/task-linked-issue.model.ts (1)
Line range hint
4-11: Consider adding JSDoc comments to document the enum values.While the enum values are well-named, adding documentation would help developers understand the specific use case and implications of each relationship type.
Example improvement:
export enum TaskRelatedIssuesRelationEnum { + /** Indicates that the current task cannot proceed until the linked task is resolved */ IS_BLOCKED_BY = 1, + /** Indicates that the current task is preventing the linked task from proceeding */ BLOCKS = 2, // ... (add similar documentation for other values) }packages/core/src/tasks/linked-issue/task-linked-issue.module.ts (1)
8-8: Well-structured implementation of CQRS pattern!The addition of command handlers and their integration into the module's providers follows NestJS best practices for CQRS implementation. This architectural choice will improve separation of concerns and make the codebase more maintainable.
Consider adding event handlers in the future if you need to implement event sourcing or handle side effects after command execution.
Also applies to: 21-21
packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts (1)
3-8: Consider adding @example to the documentation.The documentation is well-structured and informative. Consider adding an @example section to demonstrate usage with a specific enum value and its corresponding output.
/** * Maps a task's related issue relation enum to a corresponding string description. * * @param {TaskRelatedIssuesRelationEnum} relation - The relation type from the enum `TaskRelatedIssuesRelationEnum`. * @returns {string} The corresponding string description for the given relation type. + * @example + * taskRelatedIssueRelationMap(TaskRelatedIssuesRelationEnum.BLOCKS) + * // Returns: "Blocks" */packages/core/src/activity-log/activity-log.service.ts (1)
Line range hint
140-182: Consider refactoring for improved maintainability.The
logActivitymethod could benefit from the following improvements:
- Use a parameter object to simplify the method signature
- Add type constraints to the generic type T
- Extract JSON fields construction logic
Consider refactoring like this:
interface ActivityLogParams<T extends Record<string, any>> { entity: BaseEntityEnum; actionType: ActionTypeEnum; actor: ActorTypeEnum; entityId: ID; entityName: string; data: T; organizationId: ID; tenantId: ID; originalValues?: Partial<T>; newValues?: Partial<T>; } private constructJsonFields<T>( actionType: ActionTypeEnum, originalValues?: Partial<T>, newValues?: Partial<T> ): Record<string, any> { if (actionType === ActionTypeEnum.Updated && originalValues && newValues) { const { updatedFields, previousValues, updatedValues } = activityLogUpdatedFieldsAndValues(originalValues, newValues); return { updatedFields, previousValues, updatedValues }; } return {}; } logActivity<T extends Record<string, any>>({ entity, actionType, actor, entityId, entityName, data, organizationId, tenantId, originalValues, newValues }: ActivityLogParams<T>) { const jsonFields = this.constructJsonFields(actionType, originalValues, newValues); this._eventBus.publish( new ActivityLogEvent({ entity, entityId, action: actionType, actorType: actor, description: generateActivityLogDescription(actionType, entity, entityName), data, organizationId, tenantId, ...jsonFields }) ); }packages/core/src/tasks/task.service.ts (3)
Line range hint
449-498: Consider extracting validation logic into helper functionsThe pagination method contains repeated validation patterns that could be extracted into helper functions for better maintainability.
Consider refactoring the validation logic like this:
+ private validatePaginationFilters(where: any) { + if (where.isDraft !== undefined && !isBoolean(where.isDraft)) { + where.isDraft = IsNull(); + } + if (where.organizationSprintId && !isUUID(where.organizationSprintId)) { + where.organizationSprintId = IsNull(); + } + return where; + } public async pagination(options: PaginationParams<Task>): Promise<IPagination<ITask>> { const likeOperator = isPostgres() ? 'ILIKE' : 'LIKE'; if (options?.where) { - // Current validation logic + options.where = this.validatePaginationFilters(options.where); }
Line range hint
872-884: Enhance error handling for JSON parsingThe current error handling could be more specific about the nature of the parsing error.
Consider this improved error handling:
try { viewFilters = isSqlite() ? (JSON.parse(queryParams as string) as IGetTasksByViewFilters) : (queryParams as IGetTasksByViewFilters) || {}; } catch (error) { - throw new HttpException('Invalid query parameters in task view', HttpStatus.BAD_REQUEST); + throw new HttpException( + `Invalid query parameters in task view: ${error instanceof Error ? error.message : 'Unknown parsing error'}`, + HttpStatus.BAD_REQUEST + ); }
Line range hint
1-1010: Consider architectural improvements and logging standardizationThe TaskService class has grown quite large and handles multiple concerns. Additionally, there are several console.log statements throughout the code.
Consider splitting this service into smaller, focused services:
- TaskQueryService (for complex queries)
- TaskMutationService (for updates and creation)
- TaskTeamService (for team-related operations)
Replace console.log statements with a proper logging service:
import { Logger } from '@nestjs/common'; @Injectable() export class TaskService { private readonly logger = new Logger(TaskService.name); // Replace console.log with logger this.logger.debug('query.getSql', query.getSql()); this.logger.error('Error while updating task', error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
packages/contracts/src/base-entity.model.ts(1 hunks)packages/contracts/src/task-linked-issue.model.ts(2 hunks)packages/core/src/activity-log/activity-log.service.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/handlers/index.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-create.handler.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-update.handler.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/index.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/task-linked-issue-create.command.ts(1 hunks)packages/core/src/tasks/linked-issue/commands/task-linked-issue-update.command.ts(1 hunks)packages/core/src/tasks/linked-issue/dto/task-linked-issue.dto.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.entity.ts(3 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.module.ts(2 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.service.ts(1 hunks)packages/core/src/tasks/task.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/tasks/linked-issue/commands/handlers/index.ts
- packages/core/src/tasks/linked-issue/commands/index.ts
🔇 Additional comments (18)
packages/core/src/tasks/linked-issue/commands/task-linked-issue-create.command.ts (1)
1-8: LGTM! Clean implementation following NestJS CQRS patterns.
The command implementation correctly follows the NestJS CQRS pattern with proper typing and immutability.
packages/core/src/tasks/linked-issue/commands/task-linked-issue-update.command.ts (1)
4-8: LGTM! Command implementation follows best practices.
The command class follows NestJS CQRS best practices with:
- Proper implementation of ICommand interface
- Immutable properties using readonly
- Strong typing with ID and ITaskLinkedIssueUpdateInput
packages/core/src/tasks/linked-issue/dto/task-linked-issue.dto.ts (2)
1-2: LGTM! Clean import structure following NestJS best practices.
The imports are well-organized, separating external packages from internal modules.
Also applies to: 4-4
6-8: Verify complete implementation of ITaskLinkedIssue interface.
The use of IntersectionType for DTO composition is a clean approach. However, ensure that all required properties from ITaskLinkedIssue are properly covered through the combination of TenantOrganizationBaseDTO and TaskLinkedIssue.
Let's verify the interface implementation:
✅ Verification successful
Let me gather more information about the TaskLinkedIssue class since the previous command didn't show its properties.
Let me gather one final piece of information about the TenantBaseDTO and TenantOrganizationBaseEntity to complete our verification.
Based on the gathered information, I can now provide the final verification response:
TaskLinkedIssueDTO correctly implements ITaskLinkedIssue interface
The implementation is verified to be complete through the inheritance chain:
- TaskLinkedIssueDTO extends IntersectionType(TenantOrganizationBaseDTO, TaskLinkedIssue)
- TaskLinkedIssue entity provides: action, taskFrom, taskFromId, taskTo, taskToId
- TenantOrganizationBaseDTO provides: organization, organizationId (from IBasePerTenantAndOrganizationEntityModel)
- TenantBaseDTO provides: tenant, tenantId (from IBasePerTenantEntityModel)
All required properties from ITaskLinkedIssue interface are properly covered through this composition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required properties from ITaskLinkedIssue are covered
# Test 1: Check ITaskLinkedIssue interface definition
echo "Checking ITaskLinkedIssue interface properties:"
rg -A 10 "interface ITaskLinkedIssue"
# Test 2: Check TaskLinkedIssue entity properties
echo "Checking TaskLinkedIssue entity properties:"
ast-grep --pattern 'class TaskLinkedIssue {
$$$
}'
# Test 3: Check TenantOrganizationBaseDTO properties
echo "Checking TenantOrganizationBaseDTO properties:"
ast-grep --pattern 'class TenantOrganizationBaseDTO {
$$$
}'
Length of output: 1909
Script:
#!/bin/bash
# Let's try different approaches to find TaskLinkedIssue implementation
# Search for TaskLinkedIssue class definition with ripgrep for better context
echo "Searching TaskLinkedIssue class with ripgrep:"
rg -A 10 "class TaskLinkedIssue"
# Try to find the file containing TaskLinkedIssue class
echo -e "\nSearching files containing TaskLinkedIssue:"
fd TaskLinkedIssue
# Search for TenantOrganizationBaseDTO with ripgrep
echo -e "\nSearching TenantOrganizationBaseDTO definition:"
rg -A 10 "class TenantOrganizationBaseDTO"
Length of output: 8989
Script:
#!/bin/bash
# Check TenantBaseDTO and TenantOrganizationBaseEntity implementations
# Check TenantBaseDTO
echo "Checking TenantBaseDTO properties:"
rg -A 10 "class TenantBaseDTO"
# Check TenantOrganizationBaseEntity
echo -e "\nChecking TenantOrganizationBaseEntity properties:"
rg -A 10 "class TenantOrganizationBaseEntity"
# Check IBasePerTenantAndOrganizationEntityModel
echo -e "\nChecking IBasePerTenantAndOrganizationEntityModel interface:"
rg -A 10 "interface IBasePerTenantAndOrganizationEntityModel"
Length of output: 4873
packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-create.handler.ts (3)
1-4: LGTM! Imports are well-organized.
The imports follow a clear organization pattern: framework dependencies, followed by contracts, and then local files.
8-8: Verify error handling in the TaskLinkedIssueService.
The service injection looks good, but we should ensure proper error handling in the service's create method.
Let's check the service implementation:
#!/bin/bash
# Description: Check error handling in the service's create method
# Expected: Find try-catch blocks or error handling patterns
ast-grep --pattern 'async create($$$) {
$$$
}'6-7: Verify handler registration in the module.
The handler implementation looks good, following NestJS CQRS patterns.
Let's verify if this handler is properly registered:
✅ Verification successful
Handler is properly registered in the module
The TaskLinkedIssueCreateHandler is correctly registered:
- Exported in the
CommandHandlersarray incommands/handlers/index.ts - The
CommandHandlersarray is spread into the providers array intask-linked-issue.module.ts - The module also imports the required
CqrsModule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the handler is registered in the module's providers array
# Expected: Find the module file and verify the handler registration
# Find the relevant module file
rg -l "TaskLinkedIssueCreateHandler" --type ts | grep "module.ts"
Length of output: 193
Script:
#!/bin/bash
# Let's try a broader search to find the module registration
# First, find all files that import or reference this handler
rg "TaskLinkedIssueCreateHandler" --type ts -l
# Then, let's look for any module files in the same directory or parent directories
fd ".*\.module\.ts$" packages/core/src/tasks
Length of output: 842
Script:
#!/bin/bash
# Let's check the content of both the index.ts and the task-linked-issue.module.ts
# First, check if the handler is exported in index.ts
cat packages/core/src/tasks/linked-issue/commands/handlers/index.ts
# Then check if it's registered in the module
cat packages/core/src/tasks/linked-issue/task-linked-issue.module.ts
Length of output: 1404
packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-update.handler.ts (1)
10-14: Verify input validation.
Ensure that the service layer properly validates the input parameters before processing the update.
packages/contracts/src/task-linked-issue.model.ts (2)
1-1: LGTM! Good practice using the standardized ID type.
The addition of the ID type import aligns with best practices for maintaining consistent type definitions across the codebase.
14-20: LGTM! Verify the impact of ID type changes across the codebase.
The changes to use the standardized ID type are good. The interface structure follows best practices by:
- Using proper inheritance hierarchy
- Making update interface partial
- Maintaining consistent type usage
Let's verify that all related files have been updated to use the new ID type:
Also applies to: 24-25
✅ Verification successful
Let's examine the actual usage of these types in the files to determine if they need updates.
No action needed - ID type changes are correctly implemented
The verification shows that:
- In the core entity (
task-linked-issue.entity.ts),taskFromIdandtaskToIdare already using theIDtype - The
stringtype usage found in other files is appropriate as they are:- API service methods where string is the expected type for URLs and HTTP calls
- UI component methods where string manipulation is needed
- Command handlers where string IDs are part of the public API
The interface changes in task-linked-issue.model.ts align well with the existing codebase structure and don't require additional changes in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of ITask['id'] or string types for task IDs
# that might need to be updated to use the new ID type
# Look for potential missed conversions in task-related files
rg -l "ITask\['id'\]|taskFromId:\s*string|taskToId:\s*string|id:\s*string" \
--type ts \
--glob '**/*task*.ts'
Length of output: 1395
Script:
#!/bin/bash
# Check the actual type usage in these files
rg "taskFromId|taskToId|id:\s*string" -A 2 -B 2 --type ts --glob '**/*task*.ts'
Length of output: 7148
packages/core/src/tasks/linked-issue/task-linked-issue.module.ts (1)
24-24: LGTM!
The class declaration follows the standard formatting.
packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts (2)
1-1: LGTM!
The import statement correctly imports the required enum from the contracts package.
10-18: Verify complete enum coverage.
Let's ensure that all possible values from TaskRelatedIssuesRelationEnum are mapped.
✅ Verification successful
All enum values are correctly mapped
After comparing the enum definition in packages/contracts/src/task-linked-issue.model.ts with the mapping in packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts, all values from TaskRelatedIssuesRelationEnum are present in the issueRelationMap:
IS_BLOCKED_BY(1) ↔ 'Is Blocked By'BLOCKS(2) ↔ 'Blocks'IS_CLONED_BY(3) ↔ 'Is cloned By'CLONES(4) ↔ 'Clones'IS_DUPLICATED_BY(5) ↔ 'Is Duplicated By'DUPLICATES(6) ↔ 'Duplicates'RELATES_TO(7) ↔ 'Relates To'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all enum values are mapped in the helper function
# Expected: All enum values from TaskRelatedIssuesRelationEnum should be present in the mapping
# Search for the enum definition
echo "Enum definition:"
rg -A 10 "export enum TaskRelatedIssuesRelationEnum"
# Search for the mapping usage
echo -e "\nEnum mapping:"
rg -A 10 "TaskRelatedIssuesRelationEnum\." "packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts"
Length of output: 1521
packages/core/src/tasks/linked-issue/task-linked-issue.entity.ts (2)
4-4: LGTM! Import consolidation improves code organization.
The consolidation of imports from @gauzy/contracts improves code readability while maintaining the same functionality.
32-32: LGTM! Type changes maintain type safety while being more generic.
The change from ITask['id'] to ID type for both taskFromId and taskToId properties is appropriate as it:
- Maintains type safety through
@IsUUID()validation - Preserves proper ORM relationship mapping via
@RelationIdand@MultiORMColumn - Aligns with the broader changes in the contracts package
Let's verify the consistency of ID type usage across related files:
Also applies to: 47-47
✅ Verification successful
Based on the search results, I can see that:
-
There are still several instances of
ITask['id']being used across the codebase, particularly in:- Desktop UI components
- Activity entity
- Task estimation entity
- Task update/calculate commands
-
However, looking at the core contracts and entities:
- The main
ITaskinterface inpackages/contracts/src/task.model.tsusesIDtype - The
TaskLinkedIssueentity correctly usesIDtype - Most new entities and DTOs are using
IDtype consistently
- The main
Let me verify if these remaining ITask['id'] usages are part of an ongoing migration or if they need attention:
Based on the git history and codebase analysis, I can now provide a final response:
The type change from ITask['id'] to ID is correct and part of an ongoing type standardization
The remaining ITask['id'] usages appear in files that were recently added or modified (as seen in the git history with commit bc0d6e3), and these are part of the natural progression of the codebase where:
- The core contracts package already uses the
IDtype consistently - The task-linked-issue entity correctly uses the new
IDtype - The remaining
ITask['id']instances are in peripheral components that will likely be updated in subsequent PRs
The change is safe and follows the broader type standardization effort across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of ID type in related task files
# Expected: All task-related entities and DTOs should use the ID type for identifiers
# Check for any remaining ITask['id'] references that might need updating
rg "ITask\['id'\]" --type ts
# Check the consistency of ID usage in task-related files
rg ":\s*(string|ITask\['id'\]|ID)\s*;" --type ts $(fd -e ts task)
Length of output: 30931
Script:
#!/bin/bash
# Check the git history of files still using ITask['id'] to see if they're part of migration
git log -p -- packages/core/src/time-tracking/activity/activity.entity.ts packages/core/src/tasks/estimation/task-estimation.entity.ts packages/core/src/tasks/commands/task-update.command.ts
# Also check if there are any recent commits mentioning type migration
git log --grep="type" --grep="ID" -i --since="6 months ago" -- packages/core/
Length of output: 8281
packages/contracts/src/base-entity.model.ts (2)
90-90: LGTM! The enum addition follows consistent patterns.
The new TaskLinkedIssue enum value is properly placed in alphabetical order and follows the established naming conventions.
90-90: Verify the enum usage in activity logging system.
Since this PR is titled "Task Linked Issue Activity Log", let's verify that this enum is properly integrated with the activity logging system and consistently used across related files.
✅ Verification successful
Based on the results, I can now generate the final response as we have comprehensive information about the TaskLinkedIssue integration with the activity logging system.
TaskLinkedIssue is properly integrated with activity logging system
The enum is correctly integrated and used in the activity logging system as evidenced by:
- Activity logging is implemented in
TaskLinkedIssueServicefor create, update, and delete operations - The service uses
BaseEntityEnum.TaskLinkedIssueconsistently for activity logging - Proper activity tracking is maintained with before/after states for updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of TaskLinkedIssue enum in activity logging and related files
# Test 1: Check for activity logging related files that use TaskLinkedIssue
echo "Checking activity logging integration..."
rg -l "TaskLinkedIssue.*activity"
# Test 2: Check all usages of the new enum value
echo "Checking all TaskLinkedIssue usages..."
rg "TaskLinkedIssue"
# Test 3: Check for task-linked issue related files
echo "Checking task-linked issue related files..."
fd "task-linked-issue"
Length of output: 17522
packages/core/src/activity-log/activity-log.service.ts (1)
125-125: Parameter rename improves clarity.
The rename from entityType to entity in the JSDoc comment better reflects the parameter's purpose and aligns with its usage in the method implementation.
Let's verify all existing usages of this method to ensure they've been updated:
✅ Verification successful
Let me generate another verification script to specifically look for logActivity method calls since the previous ast-grep pattern might have been too simple.
Let me try one more search pattern to find the method calls and its usages:
Let me try one final search pattern focusing on the activity log service file specifically:
Parameter rename is safe and correctly implemented.
The verification shows that:
- The
logActivitymethod inactivity-log.service.tshas been properly renamed fromentityTypetoentityin both the JSDoc and method signature. - All usages of
ActivityLogServiceare through dependency injection, and the method calls are made through the service instance, making the parameter rename internal to the service. - The
entityTypereferences found in the codebase are unrelated to this method parameter as they belong to different contexts (import records, integration entities).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to entityType parameter
rg "entityType:" -A 2 -B 2
# Search for all calls to logActivity to verify proper parameter usage
ast-grep --pattern 'logActivity($$$)'
Length of output: 7431
Script:
#!/bin/bash
# Search for logActivity method calls with more context
rg "logActivity\(" -A 3 -B 3
# Also search for the method definition to verify the parameter name
ast-grep --pattern 'logActivity($entity: $_,$$$) {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for logActivity method calls with a more flexible pattern
rg -B 3 -A 3 "\.logActivity\("
# Try a different ast-grep pattern for the method definition
ast-grep --pattern 'async logActivity($$$) {
$$$
}'
Length of output: 88
Script:
#!/bin/bash
# Check the content of the activity log service file
cat packages/core/src/activity-log/activity-log.service.ts
# Search for any imports of ActivityLogService
rg "import.*ActivityLogService" -A 1 -B 1
Length of output: 10865
packages/core/src/tasks/linked-issue/commands/task-linked-issue-create.command.ts
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/commands/task-linked-issue-update.command.ts
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-create.handler.ts
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/commands/handlers/task-linked-issue-update.handler.ts
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 25e8828. 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts (2)
3-8: Add @throws tag to JSDoc comment.The function can throw an Error for unsupported relation types. Consider adding this information to the documentation.
/** * Maps a task's related issue relation enum to a corresponding string description. * * @param {TaskRelatedIssuesRelationEnum} relation - The relation type from the enum `TaskRelatedIssuesRelationEnum`. * @returns {string} The corresponding string description for the given relation type. + * @throws {Error} When an unsupported relation type is provided. */
1-25: Consider generalizing the enum-to-string mapping pattern.This helper function implements a useful pattern for mapping enums to user-friendly strings. Consider creating a more generic utility that could be reused for other enum-to-string mappings in the application. This would promote code reuse and maintain consistency in how enums are displayed across the UI.
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts (3)
24-32: Improve constructor formatting for better readability.Consider removing the extra blank lines between constructor parameters to maintain consistent spacing.
constructor( @InjectRepository(TaskLinkedIssue) typeOrmTaskLinkedIssueRepository: TypeOrmTaskLinkedIssueRepository, - mikroOrmTaskLinkedIssueRepository: MikroOrmTaskLinkedIssueRepository, - private readonly activityLogService: ActivityLogService ) {
149-149: Remove empty comment line.The standalone comment `//" adds no value and should be removed for better code clarity.
- // return await super.delete(id, options);
42-154: Consider implementing centralized error handling.The service could benefit from a more robust error handling strategy:
- Create custom exceptions for domain-specific errors
- Implement error handling middleware for consistent error responses
- Add error logging for debugging purposes
This would improve error tracking and maintain consistent error responses across the application.
Example implementation:
// Custom exception export class TaskLinkedIssueException extends HttpException { constructor(message: string, status: HttpStatus = HttpStatus.BAD_REQUEST) { super(message, status); } } // Error handling middleware @Catch(TaskLinkedIssueException) export class TaskLinkedIssueExceptionFilter implements ExceptionFilter { catch(exception: TaskLinkedIssueException, host: ArgumentsHost) { const ctx = host.switchToHttp(); const response = ctx.getResponse<Response>(); const status = exception.getStatus(); response.status(status).json({ statusCode: status, message: exception.message, timestamp: new Date().toISOString(), }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.service.ts(1 hunks)
🔇 Additional comments (7)
packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts (2)
1-1: LGTM! Clean and focused import statement.
9-25: LGTM! Well-implemented helper function.
The implementation is type-safe, handles edge cases appropriately, and maintains consistent string formatting. Good job addressing the previous review comments about capitalization and error handling.
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (5)
1-35: LGTM! Well-structured controller setup.
The imports are well-organized, and the controller follows NestJS best practices with proper dependency injection and guard setup.
39-55: LGTM! Well-documented and properly implemented findAll endpoint.
The endpoint is well-documented with Swagger, includes proper validation, and follows REST API best practices for pagination.
62-77: LGTM! Well-implemented create endpoint using CQRS pattern.
The endpoint correctly implements the command pattern, includes proper validation, and uses consistent HTTP status codes.
87-91: HTTP status code inconsistency in update method.
This issue was previously identified in the past review comments and remains valid.
111-115: HTTP status code and response type inconsistency in delete method.
This issue was previously identified in the past review comments and remains valid.
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (1)
39-55: Enhance API documentation for paginationWhile the implementation is correct, the API documentation could be more descriptive about the pagination parameters and response structure.
Add pagination details to the API documentation:
@ApiOperation({ - summary: 'Find all' + summary: 'Find all task linked issues with pagination' }) @ApiResponse({ status: HttpStatus.OK, description: 'Found task linked issues', - type: TaskLinkedIssue + type: TaskLinkedIssue, + isArray: true })packages/core/src/tasks/linked-issue/task-linked-issue.service.ts (2)
24-32: Improve constructor formatting for better readability.The constructor parameters have inconsistent spacing. Consider maintaining consistent line spacing between parameters.
constructor( @InjectRepository(TaskLinkedIssue) typeOrmTaskLinkedIssueRepository: TypeOrmTaskLinkedIssueRepository, - mikroOrmTaskLinkedIssueRepository: MikroOrmTaskLinkedIssueRepository, - private readonly activityLogService: ActivityLogService ) {
125-134: Consider implementing transaction for delete operations.Both delete and softDelete methods perform two operations: activity logging and deletion. Consider wrapping these in a transaction to ensure data consistency.
Example implementation:
async delete(id: ID, options?: FindOneOptions<TaskLinkedIssue>): Promise<DeleteResult> { const queryRunner = this.connection.createQueryRunner(); await queryRunner.connect(); await queryRunner.startTransaction(); try { await this.deleteActivityLog(id); const result = await super.delete(id, options); await queryRunner.commitTransaction(); return result; } catch (error) { await queryRunner.rollbackTransaction(); throw new HttpException('Failed to permanently delete task linked issue.', HttpStatus.BAD_REQUEST); } finally { await queryRunner.release(); } }Also applies to: 143-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.service.ts(1 hunks)
🔇 Additional comments (3)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (3)
1-35: LGTM! Well-structured class setup
The imports are properly organized, and the class follows NestJS best practices with appropriate guards and dependency injection.
89-91: Previous comment about HTTP status codes is still valid
The inconsistency between @ApiResponse and @HttpCode decorators noted in the past review remains unaddressed.
111-124: Previous comments about delete method are still valid
Two issues noted in past reviews remain unaddressed:
- Mismatch between response status and HTTP code
- Redundant forward slash in the delete endpoint path
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (1)
39-55: Add error handling for findAll method.While the implementation is good, consider adding error handling for robustness.
@Get() @UseValidationPipe() async findAll(@Query() params: PaginationParams<TaskLinkedIssue>): Promise<IPagination<ITaskLinkedIssue>> { - return await this.taskLinkedIssueService.findAll(params); + try { + return await this.taskLinkedIssueService.findAll(params); + } catch (error) { + throw new HttpException( + 'Failed to retrieve task linked issues', + HttpStatus.INTERNAL_SERVER_ERROR + ); + } }packages/core/src/tasks/linked-issue/task-linked-issue.service.ts (2)
22-32: Consider enhancing repository abstractionThe service directly depends on both TypeORM and MikroORM repositories. Consider introducing a repository interface to better abstract the ORM implementation details and improve maintainability.
125-134: Enhance delete operation robustnessConsider adding these improvements:
- Validate any dependent entities before deletion
- Handle cleanup of related resources
- Use proper cascading delete configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.service.ts(1 hunks)
🔇 Additional comments (5)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (5)
1-35: LGTM! Well-structured controller setup.
The imports are well-organized, and the controller is properly configured with dependency injection and CQRS pattern implementation.
63-77: LGTM! Proper implementation of create endpoint.
The implementation follows NestJS best practices with proper validation and CQRS pattern usage.
87-109: LGTM! Proper implementation of update endpoint.
The implementation follows NestJS best practices with proper validation and CQRS pattern usage.
111-124: LGTM! Proper implementation of delete endpoint.
The implementation follows NestJS best practices with proper validation.
126-139: LGTM! Proper implementation of soft delete endpoint.
The implementation follows NestJS best practices with proper validation.
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (1)
59-61: Consider using a more specific return typeWhile the current implementation works, consider creating a specific pagination type for TaskLinkedIssue to improve type safety.
type TaskLinkedIssuePagination = IPagination<ITaskLinkedIssue>; async findAll(@Query() params: PaginationParams<TaskLinkedIssue>): Promise<TaskLinkedIssuePagination>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.entity.ts(4 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts(1 hunks)packages/core/src/tasks/linked-issue/task-linked-issue.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/tasks/linked-issue/task-linked-issue.entity.ts
- packages/core/src/tasks/linked-issue/task-linked-issue.helper.ts
🔇 Additional comments (2)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (1)
32-35: LGTM! Constructor properly implements CQRS pattern
The constructor correctly injects both the service and command bus, setting up the class for CQRS pattern implementation.
packages/core/src/tasks/linked-issue/task-linked-issue.service.ts (1)
86-86:
Critical: Using create instead of update
The update method is incorrectly using super.create which could lead to unexpected behavior. This should be using super.update instead.
Apply this fix:
- const updatedTaskLinkedIssue = await super.create({ ...input, tenantId, id });
+ const updatedTaskLinkedIssue = await super.update(id, { ...input, tenantId });Likely invalid or redundant comment.
| async delete(@Param('id', UUIDValidationPipe) id: ID): Promise<DeleteResult> { | ||
| return this.taskLinkedIssueService.delete(id); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance delete method implementation
The delete method should handle potential errors and consider the following improvements:
- Add error handling for cases where the task might be referenced elsewhere
- Consider returning void to match NO_CONTENT status code
-async delete(@Param('id', UUIDValidationPipe) id: ID): Promise<DeleteResult> {
- return this.taskLinkedIssueService.delete(id);
+async delete(@Param('id', UUIDValidationPipe) id: ID): Promise<void> {
+ try {
+ await this.taskLinkedIssueService.delete(id);
+ } catch (error) {
+ if (error.code === '23503') { // Foreign key violation
+ throw new HttpException(
+ 'Cannot delete task linked issue as it is referenced by other entities',
+ HttpStatus.CONFLICT
+ );
+ }
+ throw error;
+ }
+}📝 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 delete(@Param('id', UUIDValidationPipe) id: ID): Promise<DeleteResult> { | |
| return this.taskLinkedIssueService.delete(id); | |
| } | |
| async delete(@Param('id', UUIDValidationPipe) id: ID): Promise<void> { | |
| try { | |
| await this.taskLinkedIssueService.delete(id); | |
| } catch (error) { | |
| if (error.code === '23503') { // Foreign key violation | |
| throw new HttpException( | |
| 'Cannot delete task linked issue as it is referenced by other entities', | |
| HttpStatus.CONFLICT | |
| ); | |
| } | |
| throw error; | |
| } | |
| } |
| async softRemove(@Param('id', UUIDValidationPipe) id: ID): Promise<any> { | ||
| return this.taskLinkedIssueService.softDelete(id); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve softRemove method type safety and error handling
The method uses 'any' as return type and lacks error handling.
-async softRemove(@Param('id', UUIDValidationPipe) id: ID): Promise<any> {
- return this.taskLinkedIssueService.softDelete(id);
+async softRemove(@Param('id', UUIDValidationPipe) id: ID): Promise<void> {
+ try {
+ await this.taskLinkedIssueService.softDelete(id);
+ } catch (error) {
+ if (error.code === '23503') {
+ throw new HttpException(
+ 'Cannot soft delete task linked issue as it is referenced by other entities',
+ HttpStatus.CONFLICT
+ );
+ }
+ throw error;
+ }
+}📝 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 softRemove(@Param('id', UUIDValidationPipe) id: ID): Promise<any> { | |
| return this.taskLinkedIssueService.softDelete(id); | |
| async softRemove(@Param('id', UUIDValidationPipe) id: ID): Promise<void> { | |
| try { | |
| await this.taskLinkedIssueService.softDelete(id); | |
| } catch (error) { | |
| if (error.code === '23503') { | |
| throw new HttpException( | |
| 'Cannot soft delete task linked issue as it is referenced by other entities', | |
| HttpStatus.CONFLICT | |
| ); | |
| } | |
| throw error; | |
| } | |
| } |
| private async deleteActivityLog(id: ID) { | ||
| const tenantId = RequestContext.currentTenantId(); | ||
|
|
||
| try { | ||
| // Retrieve existing task linked issue | ||
| const existingTaskLinkedIssue = await this.findOneByIdString(id); | ||
| if (!existingTaskLinkedIssue) { | ||
| throw new NotFoundException('Task linked issue not found'); | ||
| } | ||
|
|
||
| // Generate deleted activity log | ||
| const { organizationId } = existingTaskLinkedIssue; | ||
| this.activityLogService.logActivity<TaskLinkedIssue>( | ||
| BaseEntityEnum.TaskLinkedIssue, | ||
| ActionTypeEnum.Deleted, | ||
| ActorTypeEnum.User, | ||
| id, | ||
| taskRelatedIssueRelationMap(existingTaskLinkedIssue.action), | ||
| existingTaskLinkedIssue, | ||
| organizationId, | ||
| tenantId | ||
| ); | ||
| } catch (error) { | ||
| console.error(`Failed to create activity log for deletion (ID: ${id}):`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in deleteActivityLog
The current implementation catches all errors and only logs them, which could mask critical issues. Consider differentiating between different types of errors and handling them appropriately.
Here's a suggested implementation:
private async deleteActivityLog(id: ID) {
const tenantId = RequestContext.currentTenantId();
try {
const existingTaskLinkedIssue = await this.findOneByIdString(id);
if (!existingTaskLinkedIssue) {
throw new NotFoundException('Task linked issue not found');
}
await this.activityLogService.logActivity<TaskLinkedIssue>(
BaseEntityEnum.TaskLinkedIssue,
ActionTypeEnum.Deleted,
ActorTypeEnum.User,
id,
taskRelatedIssueRelationMap(existingTaskLinkedIssue.action),
existingTaskLinkedIssue,
organizationId,
tenantId
);
} catch (error) {
- console.error(`Failed to create activity log for deletion (ID: ${id}):`, error);
+ console.error(`Failed to create activity log for deletion (ID: ${id}):`, error);
+ if (error instanceof NotFoundException) {
+ throw error; // Propagate not found errors
+ }
+ // Log but don't propagate activity logging errors to allow deletion to proceed
}
}Committable suggestion skipped: line range outside the PR's diff.
| async create(entity: ITaskLinkedIssueCreateInput): Promise<ITaskLinkedIssue> { | ||
| const tenantId = RequestContext.currentTenantId() || entity.tenantId; | ||
| const { organizationId } = entity; | ||
|
|
||
| try { | ||
| const taskLinkedIssue = await super.create({ ...entity, tenantId }); | ||
|
|
||
| // Generate the activity log | ||
| this.activityLogService.logActivity<TaskLinkedIssue>( | ||
| BaseEntityEnum.TaskLinkedIssue, | ||
| ActionTypeEnum.Created, | ||
| ActorTypeEnum.User, | ||
| taskLinkedIssue.id, | ||
| taskRelatedIssueRelationMap(taskLinkedIssue.action), | ||
| taskLinkedIssue, | ||
| organizationId, | ||
| tenantId | ||
| ); | ||
|
|
||
| // Return the created task linked issue | ||
| return taskLinkedIssue; | ||
| } catch (error) { | ||
| // Handle errors and return an appropriate error response | ||
| throw new HttpException(`Failed to create task linked issue : ${error.message}`, HttpStatus.BAD_REQUEST); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction handling for data consistency
The create operation and activity logging should be wrapped in a transaction to ensure atomicity. If activity logging fails, the creation should be rolled back.
Here's how to implement transaction handling:
async create(entity: ITaskLinkedIssueCreateInput): Promise<ITaskLinkedIssue> {
const tenantId = RequestContext.currentTenantId() || entity.tenantId;
const { organizationId } = entity;
try {
- const taskLinkedIssue = await super.create({ ...entity, tenantId });
-
- // Generate the activity log
- this.activityLogService.logActivity<TaskLinkedIssue>(
- BaseEntityEnum.TaskLinkedIssue,
- ActionTypeEnum.Created,
- ActorTypeEnum.User,
- taskLinkedIssue.id,
- taskRelatedIssueRelationMap(taskLinkedIssue.action),
- taskLinkedIssue,
- organizationId,
- tenantId
- );
+ return await this.repository.manager.transaction(async (transactionalEntityManager) => {
+ const taskLinkedIssue = await super.create({ ...entity, tenantId });
+
+ await this.activityLogService.logActivity(
+ BaseEntityEnum.TaskLinkedIssue,
+ ActionTypeEnum.Created,
+ ActorTypeEnum.User,
+ taskLinkedIssue.id,
+ taskRelatedIssueRelationMap(taskLinkedIssue.action),
+ taskLinkedIssue,
+ organizationId,
+ tenantId
+ );
+
+ return taskLinkedIssue;
+ });
} catch (error) {
throw new HttpException('Failed to create task linked issue.', HttpStatus.BAD_REQUEST);
}
}📝 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 create(entity: ITaskLinkedIssueCreateInput): Promise<ITaskLinkedIssue> { | |
| const tenantId = RequestContext.currentTenantId() || entity.tenantId; | |
| const { organizationId } = entity; | |
| try { | |
| const taskLinkedIssue = await super.create({ ...entity, tenantId }); | |
| // Generate the activity log | |
| this.activityLogService.logActivity<TaskLinkedIssue>( | |
| BaseEntityEnum.TaskLinkedIssue, | |
| ActionTypeEnum.Created, | |
| ActorTypeEnum.User, | |
| taskLinkedIssue.id, | |
| taskRelatedIssueRelationMap(taskLinkedIssue.action), | |
| taskLinkedIssue, | |
| organizationId, | |
| tenantId | |
| ); | |
| // Return the created task linked issue | |
| return taskLinkedIssue; | |
| } catch (error) { | |
| // Handle errors and return an appropriate error response | |
| throw new HttpException(`Failed to create task linked issue : ${error.message}`, HttpStatus.BAD_REQUEST); | |
| } | |
| } | |
| async create(entity: ITaskLinkedIssueCreateInput): Promise<ITaskLinkedIssue> { | |
| const tenantId = RequestContext.currentTenantId() || entity.tenantId; | |
| const { organizationId } = entity; | |
| try { | |
| return await this.repository.manager.transaction(async (transactionalEntityManager) => { | |
| const taskLinkedIssue = await super.create({ ...entity, tenantId }); | |
| await this.activityLogService.logActivity( | |
| BaseEntityEnum.TaskLinkedIssue, | |
| ActionTypeEnum.Created, | |
| ActorTypeEnum.User, | |
| taskLinkedIssue.id, | |
| taskRelatedIssueRelationMap(taskLinkedIssue.action), | |
| taskLinkedIssue, | |
| organizationId, | |
| tenantId | |
| ); | |
| return taskLinkedIssue; | |
| }); | |
| } catch (error) { | |
| throw new HttpException('Failed to create task linked issue.', HttpStatus.BAD_REQUEST); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (1)
60-62: Add error handling to findAll methodConsider adding try-catch block to handle potential database errors and provide meaningful error messages.
async findAll(@Query() params: PaginationParams<TaskLinkedIssue>): Promise<IPagination<ITaskLinkedIssue>> { - return this.taskLinkedIssueService.findAll(params); + try { + return await this.taskLinkedIssueService.findAll(params); + } catch (error) { + throw new HttpException( + 'Failed to retrieve task linked issues', + HttpStatus.INTERNAL_SERVER_ERROR + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts(1 hunks)
🔇 Additional comments (4)
packages/core/src/tasks/linked-issue/task-linked-issue.controller.ts (4)
84-84: Add error handling for command execution
A past review comment already suggested adding error handling for the command execution. That suggestion is still valid and should be implemented.
96-98: Fix inconsistent HTTP status codes in update method
A past review comment already identified the mismatch between @ApiResponse and @httpcode decorators. That suggestion is still valid and should be implemented.
126-138: Multiple improvements needed in delete method
Past review comments have already identified several issues that are still valid:
- Mismatch between response status and HTTP code
- Need for enhanced error handling
- Return type considerations
157-160: Multiple improvements needed in softRemove method
Past review comments have already identified several issues that are still valid:
- Need for improved type safety (avoid using 'any')
- Need for error handling
- Path format improvements
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
Improvements
Bug Fixes