[Fix] Alter Activity Log Entity Based On Employee #8771
Conversation
WalkthroughThe changes refactor activity log functionality to shift from a user-centric model to an employee-centric one. In the contracts package, interface definitions are updated to extend employee-related types and remove user-specific properties. In the core package, both the entity and service classes replace "creator" and "creatorId" with "employee" and "employeeId," including updates to validation and comment descriptions. A new migration updates the database table accordingly, modifying constraints, indexes, and column mappings across PostgreSQL, SQLite, and MySQL. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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: 1
🧹 Nitpick comments (7)
packages/core/src/lib/database/migrations/1739268372038-AlterActivityLogTableUsingEmployeeId.ts (4)
1-7: Consider using a standardized logging approach in migrations.
While usingconsole.logandchalkfor feedback is acceptable during development, you might consider leveraging the built-inqueryRunner.loggeror a consistent application logger for production environments.
13-30: Wrap theupmigration in a transaction if supported.
For safer schema updates, consider wrapping theupactions in a transaction (if the underlying database supports it). This ensures that either all schema changes apply successfully or none do, preventing partial updates in case of errors.
32-52: Consider transactional rollback for thedownmigration.
Similar reasoning applies here: using transactions (where supported) can protect against partial rollbacks if an error occurs during thedownsteps.
90-168: Be aware of potential data migration overhead on large tables in SQLite.
The strategy of creating a temporary table, copying data, and dropping the original table is common for SQLite, but can be slow for large datasets. If performance is a concern for largeactivity_logtables, consider scheduling downtime or batched copies.packages/core/src/lib/activity-log/activity-log.service.ts (3)
33-39: Update method documentation to reflect employee-centric changes.The documentation still refers to "current user" instead of "current employee".
/** - * Creates a new activity log entry with the provided input, while associating it with the current user and tenant. + * Creates a new activity log entry with the provided input, while associating it with the current employee and tenant. * * @param input - The data required to create an activity log entry. * @returns The created activity log entry. * @throws BadRequestException when the log creation fails. */
42-42: Update comment to reflect employee context.The comment still refers to "user's ID".
- // Retrieve the current user's ID from the request context + // Retrieve the current employee's ID from the request context
1-176: Consider architectural implications of employee-centric logging.The shift from user-centric to employee-centric logging is a significant architectural change. Please consider:
- What happens when non-employee users perform actions?
- How does this affect system-generated logs?
- Are there any reporting or auditing requirements that might need both user and employee context?
Let's check for system-generated logs and non-employee actions:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for system-generated logs and non-employee actions rg "ActorTypeEnum.SYSTEM" --type ts rg "logActivity.*ActorTypeEnum" -A 5 --type tsLength of output: 84
Architectural Implication: Verify Handling of Non‐Employee and System Actions
Our current implementation inActivityLogServiceconsistently uses the employee ID obtained viaRequestContext.currentEmployeeId(), and a repository search did not reveal any usage ofActorTypeEnum.SYSTEMor any alternative handling for non‐employee user actions. This means that system-generated events or actions performed by users who aren’t employees might not be logged as expected.
- The log creation in the
create()method relies solely oncurrentEmployeeId().- No evidence was found that non-employee or system-originated actions are handled differently (e.g., via
ActorTypeEnum.SYSTEMor an alternative context check).- This might impact auditing or reporting requirements if both employee and non-employee contexts are needed.
Please manually verify if system events or actions by non-employees are expected to be logged with different semantics or if additional logic is needed to address these cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/contracts/src/lib/activity-log.model.ts(2 hunks)packages/core/src/lib/activity-log/activity-log.entity.ts(2 hunks)packages/core/src/lib/activity-log/activity-log.service.ts(1 hunks)packages/core/src/lib/database/migrations/1739268372038-AlterActivityLogTableUsingEmployeeId.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
🔇 Additional comments (7)
packages/core/src/lib/database/migrations/1739268372038-AlterActivityLogTableUsingEmployeeId.ts (3)
59-67: Postgres foreign key constraint rename looks correct.
The new constraint referencing theemployeetable with cascade behavior aligns with the rest of the employee-centric changes. This appears correct.
261-269: MySQL column rename and foreign key update look good.
ChangingcreatorIdtoemployeeIdaligns with the rest of the updates and references the correct table.
277-284: Reverse logic for MySQL is consistent.
Dropping the new constraint and reintroducing the old one reverts the schema correctly.packages/contracts/src/lib/activity-log.model.ts (2)
12-15: Employee-based extension inIActivityLogis coherent.
Replacing user references withIEmployeeEntityInputmatches the new employee-centric approach across the codebase.
40-43: Updated omission forIActivityLogInputis aligned.
Switching from omitting'creatorId' | 'creator'to'employeeId' | 'employee'keeps the interface consistent with the new model. Verify that all code referencing old props is updated.packages/core/src/lib/activity-log/activity-log.entity.ts (2)
7-8: Imports switched fromUsertoEmployee.
The removal of theUserimport and addition of theEmployeeimport fits the shift to an employee-based logging mechanism.
94-109: References changed toemployeeandemployeeId.
The property rename fromcreatortoemployeealigns with the updated database schema and model interfaces. The newRelationIdusage and on-delete cascade policy appear consistent with the application’s intended logic.
|
View your CI Pipeline Execution ↗ for commit 5c35909.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/lib/activity-log/activity-log.entity.ts (1)
91-100: Fix typo in documentation.There's a typo in the documentation comment: "Actvity" should be "Activity".
Apply this diff to fix the typo:
- * Actvity Log Author ID + * Activity Log Author ID🧰 Tools
🪛 GitHub Check: Cspell
[warning] 92-92:
Unknown word (Actvity)🪛 GitHub Actions: Check Spelling and Typos with cspell
[warning] 92-92: Unknown word (Actvity)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/contracts/src/lib/activity-log.model.ts(2 hunks)packages/core/src/lib/activity-log/activity-log.entity.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Cspell
packages/core/src/lib/activity-log/activity-log.entity.ts
[warning] 92-92:
Unknown word (Actvity)
🪛 GitHub Actions: Check Spelling and Typos with cspell
packages/core/src/lib/activity-log/activity-log.entity.ts
[warning] 92-92: Unknown word (Actvity)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
🔇 Additional comments (4)
packages/contracts/src/lib/activity-log.model.ts (2)
7-17: LGTM! Well-structured employee-centric activity log interface.The interface successfully transitions from user-centric to employee-centric logging while maintaining comprehensive documentation and type safety.
32-35: LGTM! Consistent input interface definition.The interface correctly uses OmitFields to exclude employee-related fields, maintaining consistency with the employee-centric model.
packages/core/src/lib/activity-log/activity-log.entity.ts (2)
5-8: LGTM! Proper imports and validation.The imports are correctly updated to support employee-centric logging and include proper validation decorators.
79-89: LGTM! Well-defined employee relation.The ManyToOne relation is properly configured with appropriate cascade and nullable settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (2)
59-101: Consider chunk-based or batched updates when reassigning IDs.
For large tables, using a single UPDATE statement that references another table can potentially lock or slow down the database. If theactivity_logtable contains a high volume of data, chunk-based updates may improve performance and reduce lock contention.
108-150: Replicate the batching strategy for the down migration.
Thedown()migration uses a single UPDATE to revertemployeeIdback tocreatorId. As with theup()process, consider chunk-based approaches for large data sets to avoid performance bottlenecks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/contracts/src/lib/activity-log.model.ts(2 hunks)packages/core/src/lib/activity-log/activity-log.entity.ts(2 hunks)packages/core/src/lib/activity-log/activity-log.service.ts(1 hunks)packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/lib/activity-log/activity-log.service.ts
- packages/core/src/lib/activity-log/activity-log.entity.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (4)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (1)
72-84:❓ Verification inconclusive
Verify presence of corresponding employee records.
When transferringcreatorIdtoemployeeIdusing a sub-query on theemployeetable, any missing captures (e.g., ifcreatorIddoesn't map to an existingemployee.userId) will result in nullemployeeIdvalues.Below is a script to identify whether orphaned
creatorIdentries exist in theactivity_logtable:You could then decide how to handle these orphaned rows gracefully.
🏁 Script executed:
#!/bin/bash # This script checks for any activity_log rows whose creatorId cannot be mapped to an employee's userId echo "Detecting orphaned creatorId entries..." cat <<EOF | psql SELECT COUNT(*) AS orphan_count FROM "activity_log" al WHERE al."creatorId" IS NOT NULL AND NOT EXISTS ( SELECT 1 FROM "employee" e WHERE e."userId" = al."creatorId" ); EOFLength of output: 197
Action Required: Manual Verification of Orphaned Creator IDs
The intended query to detect orphaned
creatorIdentries could not be executed due to a missing PostgreSQL (psql) command in the current environment. Please verify manually or through an appropriate environment that supportspsqlto ensure noactivity_logrecords contain unmappedcreatorIdvalues.
- Issue: The provided script errored with
psql: command not found, so its output is inconclusive.- Next Steps:
• Re-run the query in an environment with PostgreSQL installed or via an alternative method to check for orphanedcreatorIdentries.
• Confirm that every non-nullcreatorIdinactivity_logmaps to an existingemployee.userId.packages/contracts/src/lib/activity-log.model.ts (3)
1-2: Good addition of IEmployeeEntityInput import.
The import statements at the top cleanly integrate employee-related types. This aligns theIActivityLoginterface with the new employee-based model.
7-7: Shifting to employee-based interfaces seems consistent.
Replacing the prior user linkage withIEmployeeEntityInputinIActivityLogproperly reflects the new reliance on the employee model. Verify all references across the codebase are similarly updated (e.g., any lingeringcreatorIdusage).
35-35: Use of OmitFields in IActivityLogInput appears valid.
UtilizingOmitFields<IActivityLog>is a cleaner approach than manually specifying omitted fields. Ensure that the omitted fields match your intended omissions (i.e., no accidental override of required properties).
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts
Show resolved
Hide resolved
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (2)
157-164: 🛠️ Refactor suggestionSQLite migration placeholders remain unimplemented
These methods are empty and will result in inconsistent database states across SQLite deployments.Consider implementing the migration steps or throwing an error to handle SQLite properly.
16-30: 🛠️ Refactor suggestionImplement or explicitly disallow SQLite/better-sqlite3 migrations
Currently, thesqliteUpQueryRunnerandsqliteDownQueryRunnermethods are called for SQLite, but they are empty. This creates incomplete schema evolution for SQLite-based deployments.You could either implement the SQLite-specific migration steps or throw a clear error indicating that these engines are not supported:
case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: - await this.sqliteUpQueryRunner(queryRunner); + throw new Error('SQLite is not fully supported by this migration yet.'); break;
🧹 Nitpick comments (1)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (1)
14-14: Consider using migration logging abstractions instead of console.log
While printing to the console in a migration is acceptable for quick feedback, leveraging TypeORM or a dedicated logging library can produce more consistent logs across various environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (2)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (2)
60-85: Verify behavior for users with no matching employee record
When copyingcreatorIdtoemployeeId, a mapping query setsemployeeIdto the last created employee for a matching user. If a user has multiple employees or none at all, the result may be unexpected (null or mismatched references).Ensure that all employees can be mapped consistently or that a fallback mechanism handles users without employees.
185-196: Handle multiple possible employees for a single user
In MySQL, the query usesORDER BY e.\createdAt` DESC LIMIT 1`, which may unpredictably map to the most recently created employee. Verify that the data model supports this assumption or confirm that each user has only one employee entry.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (3)
16-29: Consider separate migration files per DB vendor for maintainability.While using a switch statement in a single migration class is convenient, splitting each database type’s logic into its own migration file can make maintenance and troubleshooting more straightforward, especially when differences in SQL syntax or constraints arise in future changes.
157-470: Reduce complexity of repeated dropping and recreating indexes.Within the SQLite Up migration, multiple indexes are dropped and re-created several times in the same flow. While valid, this operation can inflate migration execution time on large datasets. Consider consolidating index operations to minimize overhead.
852-894: Consider transactional safety or compensating steps in MySQL Down.Dropping and adding columns alongside foreign keys in multiple steps might expose intermediate states if the migration is partially applied and fails. Evaluate using a transaction (if supported) or providing a robust retry mechanism to guarantee a complete rollback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (2)
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts (2)
60-85: Validate the employee-user mapping before updating.The PostgresUpQueryRunner relies on the assumption that each creatorId maps to a valid employee row (via userId). If no matching record exists, employeeId will be set to NULL. Consider adding logs or gracefully handling missing employee entries to avoid silently losing references in this step.
477-796: Ensure thorough testing for the multi-step SQLite rollback.The SQLite Down migration merges multiple rename, copy, and re-creation cycles. This approach may be fragile if an error occurs mid-migration. Ensure thorough testing on real or staging datasets to confirm complete data integrity and correct rollback in all scenarios (e.g., partial data, existing constraints).
packages/core/src/lib/database/migrations/1739602849367-AlterActivityLogEntityTable.ts
Show resolved
Hide resolved
|
@rahul-rocket / @GloireMutaliko21 see suggestions by Coderabbit. Also can we get this merged? |
rahul-rocket
changed the title
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