[Fix]: Activity Log update entities include relations #8508
Conversation
WalkthroughThe pull request introduces significant modifications across several service classes, including Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/core/src/resource-link/resource-link.service.ts (1)
Line range hint 88-93: Critical: Replace super.create with super.update for update operations.
The current implementation uses super.create() for updates, which is incorrect and could lead to unexpected behavior. This should be using the update method instead.
-const updatedResourceLink = await super.create({
+const updatedResourceLink = await super.update(id, {
...input,
- id
});packages/core/src/organization-sprint/organization-sprint.service.ts (1)
Line range hint 249-338: Consider wrapping member management operations in a transaction.
The member management logic handles different cases well, but multiple database operations (delete, update, insert) are performed without transaction boundaries. This could lead to partial updates if any operation fails.
Consider wrapping the operations in a transaction:
async updateOrganizationSprintMembers(...): Promise<void> {
+ // Start a transaction
+ const queryRunner = this.typeOrmOrganizationSprintEmployeeRepository.manager.connection.createQueryRunner();
+ await queryRunner.connect();
+ await queryRunner.startTransaction();
+
+ try {
// ... existing member categorization code ...
// 1. Remove members
if (removedMembers.length) {
- await this.deleteMemberByIds(removedMembers.map((member) => member.id));
+ await queryRunner.manager.delete(OrganizationSprintEmployee, removedMembers.map(member => member.id));
}
// 2. Update roles
await Promise.all(
updatedMembers.map(async (member) => {
// ... role check logic ...
if (newRole && newRole.id !== member.roleId) {
- await this.typeOrmOrganizationSprintEmployeeRepository.update(member.id, { role: newRole });
+ await queryRunner.manager.update(OrganizationSprintEmployee, member.id, { role: newRole });
}
})
);
// 3. Add new members
if (newSprintMembers.length) {
- await this.typeOrmOrganizationSprintEmployeeRepository.save(newSprintMembers);
+ await queryRunner.manager.save(OrganizationSprintEmployee, newSprintMembers);
}
+ // Commit the transaction
+ await queryRunner.commitTransaction();
+ } catch (error) {
+ // Rollback on error
+ await queryRunner.rollbackTransaction();
+ throw error;
+ } finally {
+ // Release the query runner
+ await queryRunner.release();
+ }
}packages/core/src/tasks/task.service.ts (1)
Line range hint 101-103: Avoid exposing internal error details in HTTP responses.
Including the entire error object in the HttpException may expose sensitive internal details. Consider logging the error internally and returning a user-friendly error message.
Apply this diff to address the issue:
- throw new HttpException({ message: error?.message, error }, HttpStatus.BAD_REQUEST);
+ console.error(`Error while updating task: ${error.message}`, error.stack);
+ throw new HttpException('Failed to update the task.', HttpStatus.BAD_REQUEST);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/core/src/organization-project-module/organization-project-module.service.ts (1 hunks)
- packages/core/src/organization-project/organization-project.service.ts (1 hunks)
- packages/core/src/organization-sprint/organization-sprint.service.ts (1 hunks)
- packages/core/src/resource-link/resource-link.service.ts (1 hunks)
- packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1 hunks)
- packages/core/src/tasks/task.service.ts (1 hunks)
🔇 Additional comments (7)
packages/core/src/resource-link/resource-link.service.ts (2)
Line range hint 95-108: LGTM: Improved activity logging with better context.
The enhanced activity logging now provides better tracking by including:
- Both old and new resource link states
- Clear entity context in the title
- Proper organization and tenant context
Line range hint 95-108: Verify activity log handling of relation data.
Let's verify that other services follow the same activity logging pattern and that the activity log system properly handles the relation data.
✅ Verification successful
Let me try different commands to gather the necessary information.
Let me search for more specific patterns to understand the activity logging implementation.
Based on the gathered information, I can now provide the final response:
Activity logging implementation is consistent and properly handles relation data
The verification shows that:
- The activity logging pattern is consistently used across multiple services (tasks, sprints, resource-links, etc.)
- All services follow the same pattern of providing entity type, action type, actor type, and relation data (organizationId, tenantId)
- The ActivityLog entity is properly structured to handle all the required fields including relation data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other services follow the same activity logging pattern
echo "Checking activity logging patterns across services..."
rg -A 3 "logActivity.*BaseEntityEnum.*ActionTypeEnum" --type typescript
# Check activity log entity structure
echo "Checking activity log entity structure..."
ast-grep --pattern 'class ActivityLog {
$$$
}'
Length of output: 369
Script:
#!/bin/bash
# Check activity logging patterns across services using ripgrep
echo "Checking activity logging patterns..."
rg -A 3 "logActivity.*BaseEntity.*ActionType"
# Check activity log related files and their structure
echo "Finding activity log related files..."
fd -e ts activity
# Check the activity log entity definition
echo "Checking activity log entity definition..."
rg -A 10 "class ActivityLog"
Length of output: 11779
Script:
#!/bin/bash
# Check the activity log entity structure for relation fields
echo "Checking ActivityLog entity fields..."
rg -A 20 "class ActivityLog extends" packages/core/src/activity-log/activity-log.entity.ts
# Check other services using logActivity to compare patterns
echo "Checking logActivity usage patterns..."
rg "this\.activityLogService\.logActivity" -A 3
# Check the logActivity method implementation
echo "Checking logActivity method implementation..."
rg -A 10 "logActivity.*\(" packages/core/src/activity-log/activity-log.service.ts
Length of output: 6192
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1)
178-182: Verify activity log handling of related entities.
The changes ensure that relations are included when updating tasks, but we should verify that the activity log service can properly handle and store the increased data volume from these relations.
Let's check the activity log implementation:
✅ Verification successful
Let me gather more information about the activity log implementation to verify how it handles entity data.
Let me check one more aspect to understand how the activity log stores entity data and metadata.
Let me check one final aspect about the activity log entity's data storage.
Based on the gathered information, I can now provide the final response regarding the activity log's ability to handle relations.
Activity log is properly designed to handle related entities data
The activity log implementation is well-equipped to handle relations and their changes:
- The
IActivityLoginterface includes dedicated fields for storing related entities:previousEntitiesandupdatedEntitiesspecifically store IDs and values for related entitiesupdatedFieldstracks which fields (including relations) were modified- The implementation uses
IActivityLogUpdatedValuesinterface that can store any record type
The activity log service's logActivity method already receives the complete entity data and can properly track changes in relations through its originalValues and newValues parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check activity log implementation for handling relations
# Test 1: Check if activity log service has similar relation handling
echo "Checking activity log service implementation..."
rg -A 5 "logActivity.*Task" --type ts
# Test 2: Check activity log entity structure
echo "Checking activity log entity structure..."
ast-grep --pattern 'class ActivityLog {
$$$
}'
Length of output: 3557
Script:
#!/bin/bash
# Check activity log service and entity implementation
# Check activity log service implementation
echo "Checking activity log service implementation..."
rg -A 10 "class ActivityLogService" --type ts
# Check for activity log entity definition
echo "Checking activity log entity definition..."
rg -A 10 "class ActivityLog" --type ts
# Check how metadata is stored in activity log
echo "Checking metadata handling in activity log..."
rg -A 5 "metadata.*activity.*log" --type ts -i
Length of output: 7204
Script:
#!/bin/bash
# Check activity log metadata and data storage
# Check logActivity method implementation
echo "Checking logActivity method..."
rg -A 15 "logActivity.*<.*>" --type ts
# Check for metadata or data columns in activity log entity
echo "Checking activity log entity columns..."
rg -A 5 "@.*Column.*activity.*log" --type ts -i
# Check for any JSON or metadata fields
echo "Checking JSON/metadata fields..."
rg -A 3 "@Column.*json" --type ts -i
Length of output: 22147
Script:
#!/bin/bash
# Check activity log entity data storage fields
# Look for metadata or data fields in activity log entity
echo "Checking activity log entity fields..."
rg -A 10 "class ActivityLog.*implements IActivityLog" --type ts
# Check for any data or metadata properties
echo "Checking data properties..."
rg -A 3 "data:.*" packages/core/src/activity-log/ --type ts
# Check interface definition
echo "Checking IActivityLog interface..."
rg -A 10 "interface IActivityLog" --type ts
Length of output: 4277
packages/core/src/organization-sprint/organization-sprint.service.ts (1)
Line range hint 187-201: LGTM! Well-implemented activity logging.
The activity log implementation follows best practices by:
- Including both old and new states for change tracking
- Preserving organizational context
- Maintaining proper entity and action type specifications
packages/core/src/organization-project-module/organization-project-module.service.ts (1)
84-91: Consider filtering relations for performance and security.
While dynamically retrieving relations improves maintainability, loading all relations could impact performance and potentially expose sensitive data. Consider implementing relation filtering based on the update requirements.
Here's a script to analyze the entity's relations:
Consider implementing relation filtering like this:
-const relations = this.typeOrmProjectModuleRepository.metadata.relations.map(
- (relation) => relation.propertyName
-);
+// Define required relations for update operation
+const allowedRelations = ['members', 'teams'];
+const relations = this.typeOrmProjectModuleRepository.metadata.relations
+ .filter((relation) => allowedRelations.includes(relation.propertyName))
+ .map((relation) => relation.propertyName);✅ Verification successful
Based on examining the entity file, I can now provide a final response regarding the review comment's concern about loading all relations:
Loading all relations is appropriate in this context
The OrganizationProjectModule entity has well-defined relations that are essential for the module's functionality:
- Parent-child module hierarchy
- Project association
- Creator and manager users
- Task associations
- Sprint associations
- Team associations
- Member associations
All these relations represent core business data that would typically be needed when working with project modules. The relations are properly configured with:
- Appropriate cascade rules
- Nullable settings where needed
- Join table configurations
- Clear ownership rules
Loading all relations in this case is a valid approach since:
- The relations represent essential business data
- The entity uses lazy loading by default (no eager loading decorators)
- The relations have proper indexing via @ColumnIndex
- The relations follow a clear domain model structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze OrganizationProjectModule relations and their potential impact
# Expected: List of relations and their types to help assess performance and security implications
# Search for relation decorators in the entity file
echo "Analyzing entity relations..."
rg --type typescript "@(ManyToOne|OneToMany|ManyToMany|OneToOne)" -B 1 -A 2
# Search for sensitive fields in the entity
echo "Checking for potentially sensitive fields..."
rg --type typescript "@Column.*password|secret|key|token|credential" -B 1 -A 2
Length of output: 396
Script:
#!/bin/bash
# Let's try a different approach without type filters
echo "Analyzing entity relations..."
rg "@(ManyToOne|OneToMany|ManyToMany|OneToOne)" -B 1 -A 2
echo -e "\nChecking entity structure..."
# Find the entity file first
fd "organization-project-module.entity.ts"
# If we find the entity file, let's examine its content
echo -e "\nExamining entity content..."
fd "organization-project-module.entity.ts" -x cat {}
Length of output: 97241
packages/core/src/tasks/task.service.ts (2)
Line range hint 299-321: Filtering logic in pagination method is appropriate.
The additional filtering logic for isDraft and organizationSprintId correctly handles invalid values.
Line range hint 77-79: Verify whether super.create is appropriate for updating an existing task.
Using super.create in the update method may lead to unintended behavior if it does not handle updating existing records. Consider using super.update instead to ensure the task is updated correctly.
Run the following script to verify how super.create and super.update are used in the codebase:
✅ Verification successful
Using super.create in the update method is appropriate and intentional
The implementation in task.service.ts shows that super.create is used correctly in the update method. Here's why:
- The
updatemethod first retrieves the existing task with all its relations - It handles project-related updates if needed
- Then uses
super.createwith the existing ID to perform an upsert operation:
const updatedTask = await super.create({
...input,
id // Providing ID makes it an update operation
});- The method also handles additional logic like sprint history tracking and activity logging
This pattern is a valid approach for updating entities where you want to ensure all fields are properly updated while maintaining the existing ID.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of super.create and super.update in the codebase to verify their intended usage.
# Search for super.create calls
rg -A 5 $'super.create('
# Search for super.update calls
rg -A 5 $'super.update('
Length of output: 205
Script:
#!/bin/bash
# Search for create and update method implementations in the codebase
ast-grep --pattern 'class $_ extends $_ {
$$$
create($_) {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ extends $_ {
$$$
update($_) {
$$$
}
$$$
}'
# Also search for the specific file to understand the context
rg -l "task.service.ts$"
Length of output: 235788
Script:
#!/bin/bash
# Search for task.service.ts files and their content
fd "task.service.ts$" -x cat {}
Length of output: 29461
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts
Show resolved
Hide resolved
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 7907cd2. 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. |
| @@ -81,12 +81,14 @@ export class OrganizationProjectModuleService extends TenantAwareCrudService<Org | |||
| const tenantId = RequestContext.currentTenantId() || entity.tenantId; | |||
|
|
|||
| try { | |||
| // Find Organization Project Module relations | |||
| const relations = this.typeOrmProjectModuleRepository.metadata.relations.map( | |||
There was a problem hiding this comment.
@GloireMutaliko21 I don't understand why you retrieve here all relations?
| // Retrieve existing module. | ||
| const existingProjectModule = await this.findOneByIdString(id, { | ||
| relations: { | ||
| members: true, | ||
| manager: true |
There was a problem hiding this comment.
@GloireMutaliko21 Why you removed those relations and add whole relations here?
| // Search for existing Organization Sprint | ||
| const organizationSprint = await super.findOneByIdString(id, { | ||
| where: { organizationId, tenantId, projectId }, | ||
| relations: { | ||
| members: true, |
There was a problem hiding this comment.
@GloireMutaliko21 Similar thing here. Can you please explain here?
|
@rahul-rocket please see this comment #8507 (comment) |
|
@GloireMutaliko21 I read it but don't get it why we used all relations here? |
|
@rahul-rocket let me try to explain again. When user is updating a given record (Task, Project, Team, ....) :
That's why (because searching record without relations) you will see for example previous values for relations are simple empty objects because we do not have in retrieved record to be updated.
|
CC: @evereq |
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements