[Fix] Activity Log Event use generic method #8449
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on enhancing activity logging across various services in the application. Key updates include the introduction of two new functions, 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: 4
🧹 Outside diff range and nitpick comments (6)
packages/core/src/tasks/commands/handlers/task-create.handler.ts (1)
81-81: Address TODO comment regarding actor type.The TODO comment indicates that the actor type needs to be reviewed to account for GitHub Integration. Consider implementing a mechanism to determine the correct actor type (User or System) based on the context of the task creation.
Would you like assistance in implementing a solution to handle both User and System actor types?
packages/core/src/tasks/views/view.service.ts (2)
47-55: Add Error Handling for Activity Log CreationIf
activityLogCreateActionfails, it could cause unhandled errors. Consider adding error handling to prevent exceptions from interrupting the operation.Suggested addition:
try { // Generate the activity log activityLogCreateAction( this._eventBus, BaseEntityEnum.TaskView, view.name, ActorTypeEnum.User, organizationId, tenantId, view ); } catch (error) { // Handle or log the error as needed console.error('Failed to create activity log:', error); }
91-102: Add Error Handling for Activity Log UpdateSimilarly, wrap
activityLogUpdateActionin a try-catch block to handle potential errors during the logging process.Suggested addition:
try { // Generate the activity log const { organizationId } = updatedTaskView; activityLogUpdateAction( this._eventBus, BaseEntityEnum.TaskView, updatedTaskView.name, ActorTypeEnum.User, organizationId, tenantId, existingView, input, updatedTaskView ); } catch (error) { // Handle or log the error as needed console.error('Failed to update activity log:', error); }packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1)
148-157: Consider Using Consistent Sources fororganizationIdandtenantIdIn the
createTaskmethod, you're destructuringorganizationIdandtenantIdfromcreatedTask. To ensure consistency and avoid potential issues if these properties are undefined oncreatedTask, consider usingoptions.organizationIdandoptions.tenantId, which are already available.packages/core/src/organization-sprint/organization-sprint.service.ts (1)
Line range hint
151-193: Ensure the organization sprint updates when member IDs are emptyIn the
updatemethod, the current logic only updates the organization sprint ifmemberIdsormanagerIdsare provided. If both arrays are empty, the sprint's other properties frominputwill not be updated, and the activity log will not be generated. This could lead to updates being ignored when only sprint details (likename,status, etc.) are changed without modifying members.Consider restructuring the code to ensure that the sprint updates occur regardless of whether
memberIdsormanagerIdsare provided. Here's a suggested refactor:try { // Search for existing Organization Sprint const organizationSprint = await super.findOneByIdString(id, { where: { organizationId, tenantId, projectId }, relations: { members: true, modules: true } }); - // Retrieve members and managers IDs if (isNotEmpty(memberIds) || isNotEmpty(managerIds)) { // Combine memberIds and managerIds into a single array const employeeIds = [...memberIds, ...managerIds].filter(Boolean); // Retrieve a collection of employees based on specified criteria. const sprintMembers = await this._employeeService.findActiveEmployeesByEmployeeIds( employeeIds, organizationId, tenantId ); // Update nested entity (Organization Sprint Members) await this.updateOrganizationSprintMembers(id, organizationId, sprintMembers, managerIds, memberIds); } + // Update the organization sprint with the provided input + const { id: organizationSprintId } = organizationSprint; + const updatedSprint = await super.create({ + ...input, + organizationId, + tenantId, + id: organizationSprintId + }); + // Generate the activity log + activityLogUpdateAction( + this._eventBus, + BaseEntityEnum.OrganizationSprint, + updatedSprint.name, + ActorTypeEnum.User, + organizationId, + tenantId, + organizationSprint, + input, + updatedSprint + ); // Return the updated sprint return updatedSprint; } catch (error) { // Handle errors and return an appropriate error response throw new HttpException(`Failed to update organization sprint: ${error.message}`, HttpStatus.BAD_REQUEST); }This refactor ensures that the sprint is updated and the activity log is generated even when there are no changes to the members.
packages/core/src/tasks/task.service.ts (1)
102-114: Address the TODO: Handle 'System' actor type for GitHub integrationAs noted in the TODO comment on line 108, since there is GitHub integration, consider implementing logic to set the actor type to
ActorTypeEnum.Systemwhen actions are performed by the system via GitHub. This ensures accurate logging of the actor responsible for the action.Would you like assistance in implementing this functionality or opening a new GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/core/src/activity-log/activity-log.helper.ts (2 hunks)
- packages/core/src/organization-project-module/organization-project-module.service.ts (3 hunks)
- packages/core/src/organization-sprint/organization-sprint.service.ts (4 hunks)
- packages/core/src/resource-link/resource-link.service.ts (3 hunks)
- packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (5 hunks)
- packages/core/src/tasks/commands/handlers/task-create.handler.ts (2 hunks)
- packages/core/src/tasks/task.service.ts (2 hunks)
- packages/core/src/tasks/views/view.service.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (21)
packages/core/src/tasks/commands/handlers/task-create.handler.ts (2)
3-4: LGTM: Import changes are appropriate.The new imports for
BaseEntityEnum,ActorTypeEnum,ITask, andactivityLogCreateActionare correctly added and align with the changes in theexecutemethod. The import paths are consistent with the project structure.
76-84: LGTM: Activity log generation simplified.The new implementation using
activityLogCreateActionsimplifies the activity log creation process. It consolidates the logging functionality into a single method call, improving code clarity and maintainability.packages/core/src/organization-project-module/organization-project-module.service.ts (4)
20-20: LGTM: Import changes for activity logging.The import of
activityLogCreateActionandactivityLogUpdateActionfunctions suggests a good refactoring of the activity logging process. This change likely improves code organization and reusability.
52-60: LGTM: Refactored activity logging in create method.The implementation of activity logging has been improved by using the
activityLogCreateActionhelper function. This change enhances code readability and maintainability by centralizing the logging logic.
101-112: LGTM: Refactored activity logging in update method.The activity logging in the
updatemethod has been improved using theactivityLogUpdateActionhelper function. This change is consistent with the modifications in thecreatemethod and enhances the overall code quality.
Line range hint
1-424: Overall LGTM: Improved activity logging implementation.The changes in this file consistently enhance the activity logging process across the
OrganizationProjectModuleService. By introducing and utilizing theactivityLogCreateActionandactivityLogUpdateActionhelper functions, the code becomes more maintainable and consistent. This refactoring likely improves the overall quality of the codebase and aligns with best practices for logging and event tracking.packages/core/src/resource-link/resource-link.service.ts (3)
14-14: Importing activity log helper functionsThe import statement correctly adds
activityLogCreateActionandactivityLogUpdateActionfrom'../activity-log/activity-log.helper', which are used for logging activities in thecreateandupdatemethods.
56-64: Proper usage ofactivityLogCreateActionincreatemethodThe
activityLogCreateActionfunction is appropriately called to log the creation of a new resource link. All necessary parameters are provided, ensuring consistent activity logging.
93-104: Correct implementation ofactivityLogUpdateActioninupdatemethodThe
activityLogUpdateActionfunction is correctly used to log updates to resource links. The destructuring oforganizationIdandtenantIdfromupdatedResourceLink, as well as the parameters passed to the function, are appropriate.packages/core/src/tasks/views/view.service.ts (4)
2-2: Appropriate Import of Exception ClassesThe addition of
BadRequestException,HttpException,HttpStatus, andInjectablefrom@nestjs/commonis necessary for error handling and dependency injection in the service class.
15-15: Importing Activity Log Helper FunctionsThe import of
activityLogCreateActionandactivityLogUpdateActionfrom../../activity-log/activity-log.helperis appropriate and required for logging activities in thecreateandupdatemethods.
47-55: Ensure Correct Parameters inactivityLogCreateActionThe usage of
activityLogCreateActionappears correct. However, please verify that the parameters passed align with the function's expected signature to ensure accurate logging.If needed, refer to the
activityLogCreateActiondefinition to confirm the parameter order and types.
91-102: Ensure Correct Parameters inactivityLogUpdateActionPlease verify that the parameters passed to
activityLogUpdateActionmatch the function's expected signature to ensure accurate logging of the update action.Confirm that:
existingView,input, andupdatedTaskVieware correctly placed.- All necessary parameters are included and correctly ordered.
packages/core/src/activity-log/activity-log.helper.ts (2)
78-105: Well-Implemented Activity Log CreationThe
activityLogCreateActionfunction is well-structured and correctly emits theActivityLogEventwith all necessary details. This enhances the clarity and consistency of activity logging.
120-156: Effective Activity Log for UpdatesThe
activityLogUpdateActionfunction appropriately handles updated entities by logging the changed fields and their values. The use ofactivityLogUpdatedFieldsAndValuesensures that all modifications are accurately recorded.packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (2)
35-37: EventBus Dependency Successfully InjectedThe
EventBushas been correctly injected into the constructor, enabling the dispatch of events within the handler.
188-199: VerifyorganizationIdandtenantIdValues inupdateTaskIn the
updateTaskmethod, you're extractingorganizationIdandtenantIdfromupdatedTask. Ensure that these properties are reliably present after the update operation. If there's a risk they might be undefined, consider using the values fromexistingTaskor ensure they are correctly set in theentitybefore merging.packages/core/src/organization-sprint/organization-sprint.service.ts (3)
19-19: Importing activity log helpersThe import statement correctly adds
activityLogCreateActionandactivityLogUpdateActionfrom'../activity-log/activity-log.helper', which are necessary for activity logging.
183-193: Verify parameters inactivityLogUpdateActionIn the
updatemethod, theactivityLogUpdateActionfunction is called to log updates to the organization sprint. Please confirm that the parameters—especiallyorganizationSprint,input, andupdatedSprint—are correctly ordered and appropriate for this function.You can use the following script to check the function definition and verify parameter usage:
#!/bin/bash # Description: Verify the parameter order of activityLogUpdateAction # Search for the function definition of activityLogUpdateAction rg --type typescript -A 10 -B 2 "function activityLogUpdateAction"
116-124: Ensure correct parameters inactivityLogCreateActionIn the
createmethod, theactivityLogCreateActionfunction is used to generate an activity log. Please verify that the parameters passed to this function match its expected signature and order.To confirm the correctness of the parameters, you can run the following script:
✅ Verification successful
Parameters in
activityLogCreateActionare correctly ordered and match the expected signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the parameter order of activityLogCreateAction # Search for the function definition of activityLogCreateAction rg --type typescript -A 5 -B 2 "function activityLogCreateAction"Length of output: 103
Script:
#!/bin/bash # Description: Verify the parameter order of activityLogCreateAction # Search for the function definition of activityLogCreateAction in .ts and .tsx files rg --files | rg '\.ts$|\.tsx$' | xargs rg "function activityLogCreateAction"Length of output: 499
Script:
#!/bin/bash # Description: Extract the complete function definition of activityLogCreateAction # Extract lines starting from the function definition, capturing the full parameter list rg "export function activityLogCreateAction" packages/core/src/activity-log/activity-log.helper.ts -A 10Length of output: 436
packages/core/src/tasks/task.service.ts (1)
31-31: Import statement for 'activityLogUpdateAction'The import of
activityLogUpdateActionfrom'../activity-log/activity-log.helper'is appropriate and necessary for the added activity logging functionality.
| export function activityLogUpdatedFieldsAndValues<T>(originalValues: T, updated: Partial<T>) { | ||
| const updatedFields: string[] = []; | ||
| const previousValues: IActivityLogUpdatedValues[] = []; | ||
| const updatedValues: IActivityLogUpdatedValues[] = []; | ||
|
|
||
| for (const key of Object.keys(updated)) { | ||
| if (original[key] !== updated[key]) { | ||
| if (originalValues[key] !== updated[key]) { | ||
| // Add updated field | ||
| updatedFields.push(key); | ||
|
|
||
| // Add old and new values | ||
| previousValues.push({ [key]: original[key] }); | ||
| previousValues.push({ [key]: originalValues[key] }); |
There was a problem hiding this comment.
Ensure Deep Comparison for Accurate Change Detection
In the activityLogUpdatedFieldsAndValues function, the equality check originalValues[key] !== updated[key] uses shallow comparison. This may not accurately detect changes in nested objects or arrays because it compares references rather than their contents. Consider using a deep equality check to ensure all changes are captured.
Apply this diff to use deep equality comparison:
+import { isEqual } from 'lodash';
// ...
for (const key of Object.keys(updated)) {
- if (originalValues[key] !== updated[key]) {
+ if (!isEqual(originalValues[key], updated[key])) {
// Add updated field
updatedFields.push(key);
// Add old and new values
previousValues.push({ [key]: originalValues[key] });
updatedValues.push({ [key]: updated[key] });
}
}📝 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.
| export function activityLogUpdatedFieldsAndValues<T>(originalValues: T, updated: Partial<T>) { | |
| const updatedFields: string[] = []; | |
| const previousValues: IActivityLogUpdatedValues[] = []; | |
| const updatedValues: IActivityLogUpdatedValues[] = []; | |
| for (const key of Object.keys(updated)) { | |
| if (original[key] !== updated[key]) { | |
| if (originalValues[key] !== updated[key]) { | |
| // Add updated field | |
| updatedFields.push(key); | |
| // Add old and new values | |
| previousValues.push({ [key]: original[key] }); | |
| previousValues.push({ [key]: originalValues[key] }); | |
| import { isEqual } from 'lodash'; | |
| export function activityLogUpdatedFieldsAndValues<T>(originalValues: T, updated: Partial<T>) { | |
| const updatedFields: string[] = []; | |
| const previousValues: IActivityLogUpdatedValues[] = []; | |
| const updatedValues: IActivityLogUpdatedValues[] = []; | |
| for (const key of Object.keys(updated)) { | |
| if (!isEqual(originalValues[key], updated[key])) { | |
| // Add updated field | |
| updatedFields.push(key); | |
| // Add old and new values | |
| previousValues.push({ [key]: originalValues[key] }); | |
| updatedValues.push({ [key]: updated[key] }); | |
| } | |
| } |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ec297ab. 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
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (2)
147-160: LGTM: Activity logging for task creation is well-implemented.The activity logging for task creation is correctly placed and includes all relevant information. However, consider wrapping the logging call in a try-catch block to prevent any potential errors in logging from affecting the task creation process.
Consider adding error handling:
// Activity Log Task Creation const { organizationId, tenantId } = createdTask; -this.activityLogService.logActivity( - BaseEntityEnum.Task, - createdTask.title, - ActorTypeEnum.System, - organizationId, - tenantId, - ActionTypeEnum.Created, - createdTask -); +try { + await this.activityLogService.logActivity( + BaseEntityEnum.Task, + createdTask.title, + ActorTypeEnum.System, + organizationId, + tenantId, + ActionTypeEnum.Created, + createdTask + ); +} catch (error) { + console.error('Failed to log task creation activity:', error); +}
188-203: LGTM: Activity logging for task update is well-implemented.The activity logging for task update is correctly placed and includes all relevant information, including the existing task and new entity data for change tracking. As with the create method, consider wrapping the logging call in a try-catch block for error handling.
Consider adding error handling:
// Activity Log Task Update const { organizationId, tenantId } = updatedTask; -this.activityLogService.logActivity( - BaseEntityEnum.Task, - updatedTask.title, - ActorTypeEnum.System, - organizationId, - tenantId, - ActionTypeEnum.Updated, - updatedTask, - existingTask, - entity -); +try { + await this.activityLogService.logActivity( + BaseEntityEnum.Task, + updatedTask.title, + ActorTypeEnum.System, + organizationId, + tenantId, + ActionTypeEnum.Updated, + updatedTask, + existingTask, + entity + ); +} catch (error) { + console.error('Failed to log task update activity:', error); +}packages/core/src/tasks/task.service.ts (1)
102-116: Address the TODO regardingActorTypeEnumin activity loggingThere's a TODO comment indicating the need to handle the
ActorTypeEnumfor GitHub Integration in thelogActivitymethod call. Specifically, to accommodate scenarios where the actor might be 'System' instead of 'User'. Consider updating theActorTypeEnumor the logic to support system-generated events.Would you like assistance in implementing this functionality or should we open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- packages/core/src/activity-log/activity-log.module.ts (2 hunks)
- packages/core/src/activity-log/activity-log.service.ts (4 hunks)
- packages/core/src/activity-log/events/handlers/activity-log.handler.ts (1 hunks)
- packages/core/src/organization-project-module/organization-project-module.service.ts (4 hunks)
- packages/core/src/organization-project/organization-project.service.ts (3 hunks)
- packages/core/src/organization-sprint/organization-sprint.service.ts (5 hunks)
- packages/core/src/resource-link/resource-link.service.ts (4 hunks)
- packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (4 hunks)
- packages/core/src/tasks/task.service.ts (3 hunks)
- packages/core/src/tasks/views/view.service.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/organization-project-module/organization-project-module.service.ts
- packages/core/src/organization-sprint/organization-sprint.service.ts
- packages/core/src/resource-link/resource-link.service.ts
- packages/core/src/tasks/views/view.service.ts
🧰 Additional context used
🔇 Additional comments (13)
packages/core/src/activity-log/events/handlers/activity-log.handler.ts (1)
18-18: LGTM! Consider verifying method name change impact.The change from
logActivitytocreateis a good improvement in naming convention and aligns well with standard CRUD operations. The explicit return of the created log entry is also beneficial, as it allows for potential error handling or further processing by the caller.To ensure this change doesn't introduce any issues, please verify that all calls to
logActivityhave been updated tocreatethroughout the codebase. You can use the following script to check for any remaining occurrences oflogActivity:If any occurrences are found, they should be updated to use the new
createmethod.packages/core/src/activity-log/activity-log.module.ts (2)
2-2: LGTM: ActivityLogModule is now globally available.The changes to import and use the
@Global()decorator are correct and will make theActivityLogModuleglobally available throughout the application. This can simplify its usage in other modules.Also applies to: 12-12
Line range hint
1-24: Verify the necessity of global scope for ActivityLogModule.While making the module global can simplify its usage, it's important to ensure this is the intended behavior. Global modules can potentially make dependencies less explicit and affect maintainability.
Please run the following script to analyze the usage of
ActivityLogModuleacross the application:Based on the results, we can determine if the global scope is necessary or if it might be better to keep the module's scope more limited.
✅ Verification successful
Global Scope Confirmation
Based on the analysis,
ActivityLogModuleis appropriately set as a global module to provideActivityLogServiceacross the application without requiring explicit imports in every consuming module.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of ActivityLogModule across the application # Test 1: Check for explicit imports of ActivityLogModule echo "Explicit imports of ActivityLogModule:" rg --type typescript "import.*ActivityLogModule" --glob "!**/activity-log.module.ts" # Test 2: Check for usage of ActivityLog entity or service echo "\nUsage of ActivityLog entity or service:" rg --type typescript "ActivityLog[^M]" --glob "!**/activity-log*" # Test 3: Check for any remaining imports from the activity-log module echo "\nOther imports from activity-log module:" rg --type typescript "from ['\"].*activity-log" --glob "!**/activity-log*"Length of output: 611
Script:
#!/bin/bash # Description: Re-analyze the usage of ActivityLogModule across the application with corrected file type # Test 1: Check for explicit imports of ActivityLogModule echo "Explicit imports of ActivityLogModule:" rg --type ts "import.*ActivityLogModule" --glob "!**/activity-log.module.ts" # Test 2: Check for usage of ActivityLog entity or service echo "\nUsage of ActivityLog entity or service:" rg --type ts "ActivityLog[^M]" --glob "!**/activity-log*" # Test 3: Check for any remaining imports from the activity-log module echo "\nOther imports from activity-log module:" rg --type ts "from ['\"].*activity-log" --glob "!**/activity-log*"Length of output: 5114
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (2)
4-13: LGTM: Import statements and constructor changes are appropriate.The new imports and the addition of
ActivityLogServiceto the constructor are consistent with the implementation of activity logging functionality. This change follows good dependency injection practices.Also applies to: 18-18, 36-38
Line range hint
1-210: Overall changes look good, consider updating tests.The addition of activity logging in the
AutomationTaskSyncHandlerclass enhances the traceability of task-related actions. The implementation is consistent for both create and update operations. However, it's important to ensure that the test suite is updated to cover this new functionality.Please verify that the test suite has been updated to cover the new activity logging functionality. Run the following script to check for test files related to this handler:
If no results are found for the second command, consider updating the test files to include coverage for the new activity logging functionality.
packages/core/src/organization-project/organization-project.service.ts (4)
25-25: LGTM: Import statement added correctly.The import for
ActivityLogServiceis properly placed and aligns with the changes made in the class.
48-48: LGTM: Constructor updated to use ActivityLogService.The constructor parameter has been correctly updated to use
ActivityLogServiceinstead ofEventBus, maintaining consistent naming conventions.
125-133: LGTM: Activity logging updated to use ActivityLogService.The implementation for logging activity has been successfully updated to use the
ActivityLogService. This change provides a more direct method of logging and is consistent with the removal ofEventBus. All necessary parameters are correctly passed to thelogActivitymethod.
Line range hint
1-693: Overall assessment: Changes improve activity logging mechanism.The modifications in this file successfully replace the
EventBuswithActivityLogServicefor logging activities. The changes are consistent throughout the file and align with the PR objectives. The new implementation provides a more direct and potentially more efficient way of logging activities for organization projects.packages/core/src/activity-log/activity-log.service.ts (1)
Line range hint
82-90: Verify the correctness of pagination 'skip' calculationThe calculation of the
skipvalue for pagination starts from 1:const skip = filters.skip && Number.isInteger(filters.skip) && filters.skip > 0 ? filters.skip : 1;And then calculates the offset:
skip: take * (skip - 1) // Calculate offset (skip) based on validated skip valueSince TypeORM uses zero-based indexing for the
skipoption, startingskipfrom 1 may lead to confusion or unintended behavior. Consider adjusting the logic to ensure accurate pagination and that no records are unintentionally skipped.packages/core/src/tasks/task.service.ts (3)
24-25: Imports are appropriateThe addition of
PermissionsEnumandActionTypeEnumto the imports ensures that the necessary enums are available for use in the code.
32-32: Correctly importedActivityLogServiceThe import statement for
ActivityLogServiceis correctly added, allowing access to activity logging functionalities.
47-47:ActivityLogServiceinjected into the constructorInjecting
ActivityLogServiceinto the constructor ensures that activity logging methods are available throughout theTaskService.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/tasks/views/view.service.ts (1)
47-58: LGTM: Improved activity logging implementation.The use of
ActivityLogService.logActivityis a good improvement. It provides a more direct and type-safe approach to logging activities.Consider extracting the activity log parameters into a separate object for better readability:
this.activityLogService.logActivity({ entityType: BaseEntityEnum.TaskView, entityName: view.name, actorType: ActorTypeEnum.User, organizationId, tenantId, action: ActionTypeEnum.Created, data: view });packages/core/src/activity-log/activity-log.service.ts (1)
117-169: LGTM: NewlogActivitymethod enhances logging capabilities.The new
logActivitymethod is a well-structured and flexible approach to handling activity logging for various entity types and actions. The use ofEventBusfor publishingActivityLogEventaligns well with event-driven architecture principles.Minor suggestion for improvement:
Consider adding a return type annotation to the method for better code clarity. If the method doesn't return anything, you can explicitly set it tovoid.logActivity<T>( // ... parameters ... ): void { // ... method body ... }This change would enhance the method's type safety and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/src/activity-log/activity-log.service.ts (4 hunks)
- packages/core/src/tasks/views/view.service.ts (5 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/core/src/tasks/views/view.service.ts (3)
1-1: LGTM: Import and constructor changes are appropriate.The changes in the import statements and constructor align well with the shift from
EventBustoActivityLogServicefor activity logging. The addition ofNotFoundExceptionimport is also beneficial for improved error handling.Also applies to: 15-15, 29-29
70-71: LGTM: Updated method comments.The comments have been appropriately updated to reflect the use of
NotFoundExceptionfor better error handling.
Line range hint
1-116: Overall improvements with one critical issue to address.This PR introduces significant improvements to the
TaskViewService:
- Enhanced activity logging using
ActivityLogService.- Improved error handling with the use of
NotFoundException.- More consistent and type-safe approach to logging activities.
However, there's a critical issue in the
updatemethod that needs to be addressed:
- The method is using
super.createinstead ofsuper.update, which may lead to unexpected behavior.Please address the issues raised in the individual comments, particularly the update logic in the
updatemethod. Once these are resolved, the changes will significantly enhance the robustness and maintainability of theTaskViewService.packages/core/src/activity-log/activity-log.service.ts (4)
3-11: LGTM: New imports added for enhanced functionality.The new imports for enums (
ActionTypeEnum,ActorTypeEnum,BaseEntityEnum) andEventBusfrom@nestjs/cqrsare appropriate for the new event-driven logging functionality implemented in this service.Also applies to: 18-20
26-27: LGTM: Constructor updated to include EventBus.The addition of the
_eventBusparameter in the constructor is appropriate for implementing the new event-driven logging functionality.
82-82: LGTM: Improved skip value calculation for pagination.The adjustment to the
skipvalue calculation ensures that it's a positive integer starting from 1, which is a good practice for pagination. This change improves the robustness of thefindActivityLogsmethod.
Line range hint
106-114: Method renamed for clarity, but error handling concerns remain.The renaming of
logActivitytocreateimproves the clarity of the method's purpose. However, the previously raised concerns about error handling are still valid:
- Consider using
console.errorinstead ofconsole.logfor error logging.- Consider throwing a more appropriate exception, such as
InternalServerErrorException, instead ofBadRequestExceptionfor server-side errors.Please address these concerns to improve the robustness of the error handling in this method.
| // Retrieve existing view. | ||
| const existingView = await this.findOneByIdString(id); | ||
|
|
||
| if (!existingView) { | ||
| throw new NotFoundException('View not found'); | ||
| } | ||
|
|
||
| const updatedTaskView = await super.create({ | ||
| ...input, | ||
| tenantId, | ||
| id | ||
| }); | ||
|
|
||
| // Generate the activity log description. | ||
| const description = generateActivityLogDescription( | ||
| ActionTypeEnum.Updated, | ||
| // Generate the activity log | ||
| const { organizationId } = updatedTaskView; | ||
| this.activityLogService.logActivity( | ||
| BaseEntityEnum.TaskView, | ||
| updatedTaskView.name | ||
| ); | ||
|
|
||
| // Compare values before and after update then add updates to fields | ||
| const { updatedFields, previousValues, updatedValues } = activityLogUpdatedFieldsAndValues( | ||
| updatedTaskView.name, | ||
| ActorTypeEnum.User, | ||
| organizationId, | ||
| tenantId, | ||
| ActionTypeEnum.Updated, | ||
| updatedTaskView, | ||
| existingView, |
There was a problem hiding this comment.
Improved error handling and logging, but update logic needs revision.
The addition of the existence check and use of NotFoundException is a good improvement in error handling. The activity logging changes are also consistent with the create method.
However, there's a critical issue in the update logic:
The method is using super.create instead of super.update. This may lead to unexpected behavior, potentially creating a new record instead of updating the existing one. Please modify the code as follows:
const updatedTaskView = await super.update(id, {
...input,
tenantId
});Consider extracting the activity log parameters into a separate object for better readability, similar to the suggestion for the create method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/core/src/tasks/task.service.ts (3)
102-115: LGTM: Improved activity logging in update methodThe refactoring of the update method to use the centralized ActivityLogService is a good improvement. It simplifies the code and promotes consistency in activity logging across the application.
However, I have a minor suggestion:
Consider extracting the activity logging call into a separate private method within the TaskService class. This would further improve readability and make it easier to reuse the logging logic if needed in other methods. For example:
private logTaskActivity(task: Task, action: ActionTypeEnum, oldTask?: Task, input?: Partial<ITaskUpdateInput>) { const { organizationId, tenantId } = task; this.activityLogService.logActivity<Task>( BaseEntityEnum.Task, task.title, task.id, ActorTypeEnum.User, organizationId, tenantId, action, task, oldTask, input ); }Then in the update method, you could simply call:
this.logTaskActivity(updatedTask, ActionTypeEnum.Updated, task, input);This approach would make the update method even cleaner and more focused on its primary responsibility.
Line range hint
119-122: Enhance error handling in update methodThe use of HttpException with detailed error information is good for debugging purposes. However, it's important to ensure that sensitive information is not exposed in production environments.
Consider implementing a custom error handling strategy that logs detailed errors for internal use but returns sanitized error messages to the client. For example:
} catch (error) { console.error(`Error while updating task: ${error.message}`, error); throw new HttpException( { message: 'An error occurred while updating the task' }, HttpStatus.BAD_REQUEST ); }This approach would log the full error details for debugging while providing a generic error message to the client, enhancing security and maintaining a consistent API response structure.
Line range hint
1-1023: Overall assessment of TaskService changesThe primary focus of this update was to improve the activity logging mechanism in the TaskService, particularly in the update method. The integration of ActivityLogService and the refactoring of the update method contribute to a more centralized and consistent approach to activity logging.
These changes are likely to improve the maintainability of the code and provide better insights into task-related activities. The rest of the TaskService remains largely unchanged, which helps maintain stability in other areas of the service.
As the application evolves, consider applying similar activity logging patterns to other methods in the TaskService where appropriate. This would ensure consistency across all task-related operations and provide a comprehensive activity trail for auditing and debugging purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/core/src/activity-log/activity-log.service.ts (4 hunks)
- packages/core/src/organization-project-module/organization-project-module.service.ts (4 hunks)
- packages/core/src/organization-project/organization-project.service.ts (3 hunks)
- packages/core/src/organization-sprint/organization-sprint.service.ts (5 hunks)
- packages/core/src/resource-link/resource-link.service.ts (4 hunks)
- packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (4 hunks)
- packages/core/src/tasks/commands/handlers/task-create.handler.ts (2 hunks)
- packages/core/src/tasks/task.service.ts (3 hunks)
- packages/core/src/tasks/views/view.service.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/activity-log/activity-log.service.ts
- packages/core/src/organization-project-module/organization-project-module.service.ts
- packages/core/src/organization-sprint/organization-sprint.service.ts
- packages/core/src/resource-link/resource-link.service.ts
- packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts
- packages/core/src/tasks/commands/handlers/task-create.handler.ts
- packages/core/src/tasks/views/view.service.ts
🧰 Additional context used
🔇 Additional comments (4)
packages/core/src/organization-project/organization-project.service.ts (3)
25-25: LGTM: Logging mechanism updatedThe change from
EventBustoActivityLogServicealigns with the PR objective of using a generic method for activity logging. This update should provide a more standardized approach to logging activities across the application.Also applies to: 48-48
125-134: LGTM: Enhanced activity logging implementedThe new implementation of activity logging using
ActivityLogServiceprovides more detailed and structured information about the created project. This change enhances the traceability and context of project creation events, which should improve debugging and auditing capabilities.
25-25: Summary: Improved activity logging for project creationThe changes in this file successfully implement a more robust activity logging mechanism for project creation. By replacing
EventBuswithActivityLogService, the code now provides more detailed and structured logging. This update aligns well with the PR objectives and should enhance the overall traceability and auditability of project-related activities in the system.Also applies to: 48-48, 125-134
packages/core/src/tasks/task.service.ts (1)
24-25: LGTM: ActivityLogService integrationThe addition of ActivityLogService and ActionTypeEnum imports, along with the constructor update, aligns well with the integration of activity logging functionality. This change enhances the service's ability to track and log task-related activities.
Also applies to: 32-32, 47-47
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
Refactor
Documentation