[Feat] Employee Notification & Employee Notification Setting #8758
Conversation
|
Warning Rate limit exceeded@rahul-rocket has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
WalkthroughThis pull request refactors and extends the system’s configuration and data models without altering core functionality. The changes reformat the existing Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Action
participant T as TaskCreateHandler
participant ENS as EmployeeNotificationService
participant EHE as EmployeeCreateNotificationEventHandler
participant R as Notification Repository
U->>T: Create/Assign Task
T->>ENS: Publish Notification Event (with entityName)
ENS->>EHE: Emit EmployeeCreateNotificationEvent
EHE->>R: Create Notification Entry in DB
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 (
|
|
View your CI Pipeline Execution ↗ for commit 3487de9.
☁️ Nx Cloud last updated this comment at |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/core/src/lib/activity-log/activity-log.entity.ts (1)
69-71: Incorrect decorator for JSON data type.The
dataproperty is decorated with@IsArray()but its type isJsonDatawhich can be either an object or string.- @IsArray() + @IsOptional() @MultiORMColumn({ type: isPostgres() ? 'jsonb' : isMySQL() ? 'json' : 'text', nullable: true }) data?: JsonData;
🧹 Nitpick comments (26)
packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.entity.ts (1)
1-3: Remove or justify the ESLint disable comment.
If there's no longer a need for "eslint-disable-next-line @typescript-eslint/no-unused-vars" after these changes, consider removing it to maintain a clean codebase.packages/core/src/lib/mention/mention.entity.ts (1)
12-12: Confirm removal of entity references.
With entityId and entity removed, confirm the rest of the codebase has been updated to avoid referencing these fields (e.g., mentions might need a new strategy for linking to their entities).packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.subscriber.ts (1)
23-37: Consider using a standardized logger over console.error.
The try/catch block ensures robust error handling, but using a logging service (e.g., NestJS Logger) instead of console.error is often preferred for uniform logging and potential log-level filtering.packages/core/src/lib/user-notification/user-notification.entity.ts (1)
44-66: Consider synchronizing isRead with readAt.
When isRead is true, readAt could be automatically set (and reset to null if isRead is toggled off), ensuring a consistent data model.packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts (1)
38-83: Optional: Add explicit HttpStatus for POST method.
The POST endpoint returns 201 (created) by default, which is generally correct. However, adding an explicit @httpcode(HttpStatus.CREATED) can improve clarity for anyone reading the code.packages/core/src/lib/user-notification/user-notification.service.ts (2)
1-28: Use a logging mechanism instead of console.log.
Internally, Nest recommends using a Logger service (e.g., Logger from '@nestjs/common') for better log management and consistency.
81-156: Consider user feedback when ignoring notifications.
In the scenario where notifications are disallowed by user settings, the method silently does nothing. If business requirements require you to track or confirm that a notification was skipped, consider logging this event or returning a status indicating that creation was intentionally bypassed.packages/core/src/lib/database/migrations/1738217979804-CreateUserNotificationTable.ts (2)
14-14: Consider using a proper logger instead of console.log.
Using a dedicated logging service or methodology (e.g., a NestJS logger) can make logs more configurable, consistent, and maintainable than raw console statements.
213-214: Consider text-type columns for message fields.
For notification messages, 255 characters may be too restrictive if more verbose content is needed. You might consider using TEXT (Postgres) or TEXT-type columns (MySQL) to accommodate longer messages.- "message" character varying(255), + "message" text,packages/core/src/lib/user-notification/events/user-notification.event.ts (1)
4-6: Add descriptive comments for the event properties.
Documenting the purpose of the 'input' parameter can improve code readability and help other developers understand the event's payload at a glance.packages/core/src/lib/user-notification/user-notification-setting/commands/user-notification-setting.create.command.ts (1)
4-8: Consider using a more specific command type string.The current type string
[UserNotificationSetting] Createcould potentially conflict with other similar commands. Consider making it more specific to this exact use case.- static readonly type = '[UserNotificationSetting] Create'; + static readonly type = '[User/NotificationSetting] Create Setting';packages/core/src/lib/user-notification/user-notification-setting/commands/user-notification-setting.update.command.ts (1)
4-8: LGTM! Consider adding JSDoc documentation.The command implementation follows CQRS pattern correctly with proper immutability. Consider adding JSDoc documentation to describe the command's purpose and parameters.
+/** + * Command to update user notification settings + * @param id - The ID of the notification setting to update + * @param input - The update data for the notification setting + */ export class UserNotificationSettingUpdateCommand implements ICommand {packages/core/src/lib/user-notification/repository/type-orm-user-notification.repository.ts (1)
7-11: LGTM! Consider adding custom query methods.The repository implementation follows TypeORM patterns correctly. Consider adding custom query methods for common operations specific to user notifications (e.g., findUnreadNotifications, markAsRead).
@Injectable() export class TypeOrmUserNotificationRepository extends Repository<UserNotification> { // ... existing code ... async findUnreadNotifications(userId: string): Promise<UserNotification[]> { return this.find({ where: { user: { id: userId }, isRead: false } }); } async markAsRead(id: string): Promise<void> { await this.update(id, { isRead: true }); } }packages/core/src/lib/user-notification/user-notification-setting/dto/create-user-notification-setting.dto.ts (1)
9-11: Consider adding validation decorators.While the DTO structure is correct, consider adding class-validator decorators to ensure input validation at the DTO level.
import { IsNotEmpty, IsBoolean, IsOptional } from 'class-validator'; export class CreateUserNotificationSettingDTO extends IntersectionType(TenantOrganizationBaseDTO, UserNotificationSetting) implements IUserNotificationSettingCreateInput { @IsNotEmpty() @IsBoolean() isEnabled: boolean; @IsOptional() @IsBoolean() isArchived?: boolean; }packages/contracts/src/lib/user-notification-setting.model.ts (2)
7-13: Consider adding default values and validation for notification flags.The boolean flags are all optional without default values. Consider:
- Setting default values for better predictability
- Adding validation constraints
- Making some flags required if they are essential
13-13: Consider adding structure to preferences.The
preferencesfield usingJsonDatatype is very flexible but might benefit from a more structured type definition to ensure type safety and better documentation of expected values.packages/core/src/lib/user-notification/user-notification-setting/commands/handlers/user-notification-setting.update.handler.ts (1)
17-22: Consider standardizing the return type.The method returns either
IUserNotificationSetting | UpdateResult. This dual return type could make error handling more complex for consumers. Consider standardizing to always returnIUserNotificationSettingand handle the update result internally.- public async execute( - command: UserNotificationSettingUpdateCommand - ): Promise<IUserNotificationSetting | UpdateResult> { + public async execute( + command: UserNotificationSettingUpdateCommand + ): Promise<IUserNotificationSetting> { const { id, input } = command; - return this.userNotificationSettingService.update(id, input); + const result = await this.userNotificationSettingService.update(id, input); + return this.userNotificationSettingService.findOneByIdString(id); }packages/core/src/lib/user-notification/user-notification.helper.ts (1)
12-36: Consider adding input validation and error handling.The function should validate inputs and handle missing templates gracefully.
Consider this enhanced implementation:
export function generateNotificationTitle( action: NotificationActionTypeEnum, entity: BaseEntityEnum, entityName: string, userName: string ) { + if (!action || !entity || !entityName || !userName) { + throw new Error('Missing required parameters for notification title generation'); + } + // Get the template corresponding to the action const template = UserNotificationTemplates[action]; + if (!template) { + throw new Error(`No template found for action: ${action}`); + } // Replace placeholders in the template with actual values return template.replace(/\{(\w+)\}/g, (_, key) => { switch (key) { case 'action': return action; case 'entity': return entity; case 'entityName': return entityName; case 'userName': return userName; default: - return ''; + return `{${key}}`; // Preserve unrecognized placeholders } }); }packages/contracts/src/lib/user-notification.model.ts (1)
20-27: Remove misleading DB storage comments.The comments about enum values being stored as numbers in DB are implementation-specific and might be misleading.
Apply this diff to remove the comments:
export enum UserNotificationTypeEnum { - PAYMENT = 'Payment', // Will be stored as 0 in DB - ASSIGNMENT = 'Assignment', // Will be stored as 1 in DB - INVITATION = 'Invitation', // Will be stored as 2 in DB - MENTION = 'Mention', // Will be stored as 3 in DB - COMMENT = 'Comment', // Will be stored as 4 in DB - MESSAGE = 'Message' // Will be stored as 5 in DB + PAYMENT = 'Payment', + ASSIGNMENT = 'Assignment', + INVITATION = 'Invitation', + MENTION = 'Mention', + COMMENT = 'Comment', + MESSAGE = 'Message' }packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.service.ts (1)
24-36: Enhance error handling with specific error types.The current error handling catches all errors and converts them to HttpException with BAD_REQUEST status. Consider handling specific error types differently.
async create(input: IUserNotificationSettingCreateInput): Promise<UserNotificationSetting> { try { const userId = input.userId || RequestContext.currentUserId(); const tenantId = RequestContext.currentTenantId() || input.tenantId; return super.create({ ...input, userId, tenantId }); } catch (error) { - throw new HttpException( - `Failed to create the notification setting: ${error.message}`, - HttpStatus.BAD_REQUEST - ); + if (error instanceof TypeError || error instanceof ValidationError) { + throw new HttpException( + `Invalid notification setting data: ${error.message}`, + HttpStatus.BAD_REQUEST + ); + } + throw new HttpException( + 'Internal server error while creating notification setting', + HttpStatus.INTERNAL_SERVER_ERROR + ); } }packages/core/src/lib/shared/pipes/notification-type-transform.pipe.ts (2)
15-32: Consider using a map for enum-to-number conversion.The switch statement implementation could be simplified using a mapping object for better maintainability.
+private static readonly enumToNumber = new Map<UserNotificationTypeEnum, number>([ + [UserNotificationTypeEnum.PAYMENT, 0], + [UserNotificationTypeEnum.ASSIGNMENT, 1], + [UserNotificationTypeEnum.INVITATION, 2], + [UserNotificationTypeEnum.MENTION, 3], + [UserNotificationTypeEnum.COMMENT, 4], + [UserNotificationTypeEnum.MESSAGE, 5], +]); to(value: UserNotificationTypeEnum): number { - switch (value) { - case UserNotificationTypeEnum.PAYMENT: - return 0; - case UserNotificationTypeEnum.ASSIGNMENT: - return 1; - case UserNotificationTypeEnum.INVITATION: - return 2; - case UserNotificationTypeEnum.MENTION: - return 3; - case UserNotificationTypeEnum.COMMENT: - return 4; - case UserNotificationTypeEnum.MESSAGE: - return 5; - default: - throw new Error(`Unknown notification type: ${value}`); - } + const number = UserNotificationTypeTransformerPipe.enumToNumber.get(value); + if (number === undefined) { + throw new Error(`Unknown notification type: ${value}`); + } + return number; }
40-57: Apply similar map-based approach for number-to-enum conversion.Use the reverse mapping for consistency and maintainability.
+private static readonly numberToEnum = new Map<number, UserNotificationTypeEnum>( + Array.from(UserNotificationTypeTransformerPipe.enumToNumber.entries()) + .map(([key, value]) => [value, key]) +); from(value: number): UserNotificationTypeEnum { - switch (value) { - case 0: - return UserNotificationTypeEnum.PAYMENT; - case 1: - return UserNotificationTypeEnum.ASSIGNMENT; - case 2: - return UserNotificationTypeEnum.INVITATION; - case 3: - return UserNotificationTypeEnum.MENTION; - case 4: - return UserNotificationTypeEnum.COMMENT; - case 5: - return UserNotificationTypeEnum.MESSAGE; - default: - throw new Error(`Unknown notification type value: ${value}`); - } + const enumValue = UserNotificationTypeTransformerPipe.numberToEnum.get(value); + if (enumValue === undefined) { + throw new Error(`Unknown notification type value: ${value}`); + } + return enumValue; }packages/core/src/lib/user-notification/user-notification.controller.ts (1)
46-51: Pagination params not needed for single entity fetch.The
findByIdmethod includes pagination parameters which aren't typically needed when fetching a single entity by ID.async findById( @Param('id', UUIDValidationPipe) id: ID, - @Query() params: PaginationParams<UserNotification> ): Promise<UserNotification> { - return this.userNotificationService.findOneByIdString(id, params); + return this.userNotificationService.findOneByIdString(id); }packages/contracts/src/lib/base-entity.model.ts (1)
63-66: Add JSDoc documentation for IBasePerEntityType interface.The interface lacks documentation explaining its purpose and the significance of extending IBasePerTenantAndOrganizationEntityModel.
+/** + * Represents a base interface for entities that are associated with a specific entity type + * and belong to a tenant and organization. + * + * @extends IBasePerTenantAndOrganizationEntityModel + */ export interface IBasePerEntityType extends IBasePerTenantAndOrganizationEntityModel { entityId: ID; entity: BaseEntityEnum; }packages/core/src/lib/comment/comment.entity.ts (1)
7-8: LGTM! Improved entity inheritance structure.The change to extend
BasePerEntityTypesimplifies the entity hierarchy while maintaining the necessary functionality. This aligns with the broader refactoring of entity inheritance in the codebase.Also applies to: 21-21
packages/core/src/lib/mention/mention.service.ts (1)
83-86: Consider implementing email notifications.The TODO comment indicates a need for email notifications. This would enhance the user experience by providing multiple notification channels.
Would you like me to help implement the email notification functionality or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (62)
nx.json(1 hunks)packages/contracts/src/index.ts(2 hunks)packages/contracts/src/lib/activity-log.model.ts(3 hunks)packages/contracts/src/lib/base-entity.model.ts(1 hunks)packages/contracts/src/lib/comment.model.ts(2 hunks)packages/contracts/src/lib/employee-settings.model.ts(1 hunks)packages/contracts/src/lib/favorite.model.ts(1 hunks)packages/contracts/src/lib/mention.model.ts(2 hunks)packages/contracts/src/lib/resource-link.model.ts(1 hunks)packages/contracts/src/lib/subscription.model.ts(1 hunks)packages/contracts/src/lib/user-notification-setting.model.ts(1 hunks)packages/contracts/src/lib/user-notification.model.ts(1 hunks)packages/core/src/lib/activity-log/activity-log.entity.ts(1 hunks)packages/core/src/lib/app/app.module.ts(2 hunks)packages/core/src/lib/comment/comment.entity.ts(2 hunks)packages/core/src/lib/comment/comment.service.ts(1 hunks)packages/core/src/lib/comment/dto/create-comment.dto.ts(2 hunks)packages/core/src/lib/core/entities/entity-type-base.entity.ts(1 hunks)packages/core/src/lib/core/entities/index.ts(2 hunks)packages/core/src/lib/core/entities/internal.ts(3 hunks)packages/core/src/lib/core/entities/subscribers/index.ts(2 hunks)packages/core/src/lib/database/migrations/1738217979804-CreateUserNotificationTable.ts(1 hunks)packages/core/src/lib/database/migrations/1738326053345-CreateUserNotificationSettingTable.ts(1 hunks)packages/core/src/lib/favorite/favorite.entity.ts(1 hunks)packages/core/src/lib/mention/mention.entity.ts(1 hunks)packages/core/src/lib/mention/mention.module.ts(2 hunks)packages/core/src/lib/mention/mention.service.ts(3 hunks)packages/core/src/lib/resource-link/resource-link.entity.ts(1 hunks)packages/core/src/lib/shared/pipes/index.ts(1 hunks)packages/core/src/lib/shared/pipes/notification-type-transform.pipe.ts(1 hunks)packages/core/src/lib/subscription/subscription.entity.ts(1 hunks)packages/core/src/lib/tasks/commands/handlers/task-create.handler.ts(7 hunks)packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts(1 hunks)packages/core/src/lib/tasks/task.module.ts(2 hunks)packages/core/src/lib/tasks/task.service.ts(6 hunks)packages/core/src/lib/user-notification/events/handlers/index.ts(1 hunks)packages/core/src/lib/user-notification/events/handlers/user-notification.handler.ts(1 hunks)packages/core/src/lib/user-notification/events/index.ts(1 hunks)packages/core/src/lib/user-notification/events/user-notification.event.ts(1 hunks)packages/core/src/lib/user-notification/repository/mikro-orm-user-notification.repository.ts(1 hunks)packages/core/src/lib/user-notification/repository/type-orm-user-notification.repository.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/handlers/index.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/handlers/user-notification-setting.create.handler.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/handlers/user-notification-setting.update.handler.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/index.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/user-notification-setting.create.command.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/commands/user-notification-setting.update.command.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/dto/create-user-notification-setting.dto.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/dto/index.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/dto/update-user-notification-setting.dto.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/repository/mikro-orm-user-notification-setting.repository.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/repository/type-orm-user-notification-setting.repository.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.entity.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.module.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.service.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.subscriber.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.controller.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.entity.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.helper.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.module.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/core/src/lib/user-notification/events/handlers/index.ts
- packages/core/src/lib/shared/pipes/index.ts
- packages/core/src/lib/user-notification/user-notification-setting/commands/index.ts
- packages/core/src/lib/user-notification/events/index.ts
- packages/contracts/src/lib/employee-settings.model.ts
- packages/core/src/lib/user-notification/user-notification-setting/dto/index.ts
- nx.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (63)
packages/core/src/lib/mention/mention.module.ts (1)
12-12: LGTM! Clean integration of UserNotificationModule.The addition of UserNotificationModule is well-structured and follows the existing module organization pattern.
Let's verify the UserNotificationModule integration:
Also applies to: 22-22
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify UserNotificationModule existence and integration # Check if UserNotificationModule exists echo "Checking UserNotificationModule existence..." fd -e ts "user-notification.module.ts$" # Check for any potential circular dependencies echo "Checking for potential circular dependencies..." rg -l "from.*mention" $(fd -e ts "user-notification")Length of output: 2658
I'm generating an additional shell script to explicitly check if the integration introduced any circular dependency—specifically, whether the user-notification module is (unexpectedly) importing the mention module and vice versa.
Please run this script and share the results so we can confirm that the dependency integration is correct and that no circular dependency issues exist.
🏁 Script executed:
#!/bin/bash # Check for any circular dependency between MentionModule and UserNotificationModule echo "Searching for import of user-notification.module in mention/mention.module.ts" rg "import.*user-notification\.module" packages/core/src/lib/mention/mention.module.ts echo "Searching for import of mention.module in user-notification/user-notification.module.ts" rg "import.*mention\.module" packages/core/src/lib/user-notification/user-notification.module.tsLength of output: 625
UserNotificationModule Integration Verified
The UserNotificationModule is correctly imported into MentionModule, and there’s no evidence of a circular dependency (i.e., UserNotificationModule does not import MentionModule). The integration and module configuration are as expected.
packages/core/src/lib/subscription/subscription.entity.ts (2)
6-7: Verify removal of former entity references.
You've replaced "TenantOrganizationBaseEntity" with "BasePerEntityType" and presumably removed properties like "entityId" and "entity." Ensure that all references to those properties are either updated or removed throughout the codebase.
12-12: Confirm compatibility of Subscription with the new base class.
Switching to "BasePerEntityType" looks correct at first glance. Just confirm that inherited fields and logic (like ID handling and multi-tenant fields) still meet the requirements of Subscription scenarios.packages/core/src/lib/resource-link/resource-link.entity.ts (2)
5-6: Cross-check removed validators or imports.
"IsEnum" and "IsUUID" were removed, along with "BaseEntityEnum." Verify no references rely on them and confirm that any replaced validations are still enforced.
8-13: Check for missing fields after changing base entity type.
Extending "BasePerEntityType" removes tenant/organization fields. If ResourceLink still needs multi-tenant or organizational context, consider reintroducing them or clarifying where that context is captured.packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.entity.ts (4)
16-50: Evaluate default true booleans for optional notifications.
All notification-related fields ("payment", "assignment", "invitation", "mention", "comment", "message") default to true. Verify that this meets user expectations (e.g., might cause spammy notifications initially). You may want to handle default settings more dynamically.
52-56: Consider making preferences optional or providing a default.
"@isnotempty()" on "preferences" suggests it must always be provided, yet no default is assigned. This could cause errors if not supplied on creation. Assess whether a safe default or removing the "IsNotEmpty()" constraint would be better.
65-75: Revisit one-to-one ownership details.
By declaring this entity as the "owner" side of the relationship with a "cascade" on delete, removing the user will automatically remove notification settings. Confirm that this fits the desired domain model.
77-81: Ensure consistency between "user" and "userId" fields.
"creatorId" is optional in similar contexts, but here "userId" is a required ID. Check if that’s consistent with the rest of the system. If "user" is optional, its ID might need to be optional as well.packages/core/src/lib/mention/mention.entity.ts (1)
7-7: Ensure all references to the new base class are updated consistently.
Switching to BasePerEntityType is fine, but verify that any references or assumptions tied to TenantOrganizationBaseEntity have been removed or updated (particularly around tenant or organization context).packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.subscriber.ts (4)
6-7: Nice use of a dedicated subscriber.
This approach cleanly encapsulates entity lifecycle event logic for UserNotificationSetting.
46-48: Good practice to serialize data before insert.
Pre-serializing preferences for SQLite ensures consistency. Keep an eye on potential user input that might interfere with JSON structures.
57-59: Consistent pre-update handling.
Serializing preferences before updates mirrors the insert flow, preventing validation discrepancies.
69-83: Validate fallback handling on JSON parse errors.
The catch block resets preferences to an empty object on parsing errors. Verify that discarding invalid data is the intended approach, as it can silently lose user data.packages/core/src/lib/user-notification/user-notification.entity.ts (4)
12-14: Clean inheritance approach.
Extending BasePerEntityType centralizes shared logic. Ensure the other fields that rely on tenant or organization context remain valid after this design shift.
34-39: Confirm enum storage strategy.
Storing the notification type as an integer is fine, but confirm it aligns with your usage patterns and that conversions in the code (e.g., from numeric to enum) are consistent.
74-95: Optional author references are well-structured.
The nullable relationship with sentBy is appropriate. Just confirm that any actions dependent on a valid author handle this field carefully.
100-118: Receiver relationship is properly indexed.
Storing receiverId with a relation index ensures efficient lookups. This should scale well for queries filtering user-specific notifications.packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts (2)
1-36: Consider specifying explicit permissions.
You have applied the TenantPermissionGuard and the @permissions() decorator without specifying any permissions. If the intention is to restrict or differentiate certain endpoints based on user roles or scopes, consider explicitly defining permissions to ensure that unauthorized access is properly blocked.
85-120: Ensure exceptions align with expected responses on update/delete.
The controller returns the updated or deleted record(s) but does not explicitly handle the case where the record might not exist. Verify that your service or command handlers throw NotFoundException or a meaningful error when a record is missing, so the correct HttpStatus.NOT_FOUND is returned.packages/core/src/lib/database/migrations/1738326053345-CreateUserNotificationSettingTable.ts (2)
1-37: Check migration coverage in tests or CI.
Because this migration handles multiple database engines, verify through integration tests or pipeline checks that the up/down routines work correctly in each engine.
59-254: Migration logic appears correct.
The creation of unique indexes, foreign keys, and table structures for PostgreSQL, SQLite, and MySQL is well-structured for multi-DB support. Make sure to keep this file updated if future changes to user notification settings require altering the table schema.packages/core/src/lib/database/migrations/1738217979804-CreateUserNotificationTable.ts (2)
60-74: Re-evaluate index usage.
There are many indexes on columns like isActive, isArchived, entityId, etc. While indexes can optimize specific queries, an overabundance could degrade write performance. Verify that each index is essential for common queries.
59-86: 🏁 Script executed:#!/bin/bash # Checking if pgcrypto extension is installed in Postgres. # Adjust connection details as appropriate and run from the environment with psql. psql -d your_database -c "CREATE EXTENSION IF NOT EXISTS pgcrypto;"Length of output: 164
packages/contracts/src/lib/favorite.model.ts (1)
4-4: Verify removal of IBasePerTenantAndOrganizationEntityModel references.
By removing this interface from IFavorite, ensure that dependent code doesn’t rely on tenant or organization properties previously included.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Searching references to the removed interface in the codebase. fd --extension ts --exec rg "IBasePerTenantAndOrganizationEntityModel"Length of output: 36870
Action: Confirm IFavorite no longer directly depends on IBasePerTenantAndOrganizationEntityModel
After reviewing the file in question, IFavorite now extends IEmployeeEntityInput and IBasePerEntityType rather than directly extending IBasePerTenantAndOrganizationEntityModel. A focused search in packages/contracts/src/lib/favorite.model.ts (using, for example, a grep for "IBasePerTenantAndOrganizationEntityModel") shows that the file no longer contains any direct reference or import of that interface. Although many other parts of the codebase still reference IBasePerTenantAndOrganizationEntityModel (as seen in the search output), there are no indications that code dependent on IFavorite expects it to extend the removed interface. In other words, any required tenant/organization properties—if still needed—should now be provided through the IBasePerEntityType abstraction, and no direct dependency on IBasePerTenantAndOrganizationEntityModel exists for favorites.
This change appears safe, and no dependent code requiring a direct reference to IBasePerTenantAndOrganizationEntityModel has been identified.
packages/core/src/lib/user-notification/repository/mikro-orm-user-notification.repository.ts (1)
1-4: LGTM! Clean and standard repository implementation.The repository follows best practices by extending the base repository and properly typing it with the UserNotification entity.
packages/core/src/lib/user-notification/user-notification-setting/repository/mikro-orm-user-notification-setting.repository.ts (1)
1-4: LGTM! Clean and standard repository implementation.The repository follows best practices by extending the base repository and properly typing it with the UserNotificationSetting entity.
packages/core/src/lib/user-notification/user-notification-setting/commands/handlers/index.ts (1)
1-4: LGTM! Clean command handlers organization.The implementation follows best practices for organizing command handlers in a CQRS pattern.
packages/core/src/lib/user-notification/user-notification-setting/dto/update-user-notification-setting.dto.ts (1)
8-10: LGTM! Well-structured DTO implementation.The DTO correctly uses NestJS utility types to create a partial update type while omitting sensitive fields. Good use of documentation.
packages/core/src/lib/user-notification/user-notification-setting/repository/type-orm-user-notification-setting.repository.ts (1)
1-11: LGTM! Clean implementation following TypeORM and NestJS patterns.The repository implementation correctly follows standard patterns:
- Proper use of dependency injection
- Clean extension of TypeORM's Repository class
- Correct super call with required parameters
packages/contracts/src/lib/subscription.model.ts (1)
1-1: LGTM! Clean inheritance refactoring.The simplified inheritance chain is valid since
IBasePerEntityTypenow includes tenant/organization properties.Also applies to: 4-4
packages/contracts/src/lib/resource-link.model.ts (1)
2-2: LGTM! Clean inheritance refactoring.The simplified inheritance chain is valid since
IBasePerEntityTypenow includes tenant/organization properties.Also applies to: 5-5
packages/contracts/src/lib/mention.model.ts (2)
4-11: LGTM! Interface simplification improves maintainability.The removal of
IBasePerTenantAndOrganizationEntityModelinheritance in favor ofIBasePerEntityTypealigns with the broader refactoring effort and reduces interface complexity.
13-15: LGTM! Enhanced input interface with optional entity name.The addition of the optional
entityNameproperty toIMentionCreateInputmaintains consistency with similar changes in other interfaces.packages/core/src/lib/comment/dto/create-comment.dto.ts (1)
16-22: LGTM! Well-structured DTO with proper validation.The addition of
entityNameproperty with appropriate validation decorators (@IsOptional,@IsString) and API documentation (@ApiPropertyOptional) ensures type safety and maintainability.packages/core/src/lib/user-notification/events/handlers/user-notification.handler.ts (1)
5-19: LGTM! Well-implemented CQRS event handler.The implementation follows best practices:
- Proper use of dependency injection
- Clear documentation
- Single responsibility principle
- Async/await for service call
packages/core/src/lib/core/entities/entity-type-base.entity.ts (2)
8-18: LGTM! Well-structured base class with proper validation for entity type.The
entityproperty implementation includes:
- Clear documentation
- Proper validation with
@IsNotEmptyand@IsEnum- Database indexing with
@ColumnIndex- ORM compatibility with
@MultiORMColumn
19-28: LGTM! Well-structured base class with proper validation for entity ID.The
entityIdproperty implementation includes:
- Clear documentation
- Proper validation with
@IsNotEmptyand@IsUUID- Database indexing with
@ColumnIndex- ORM compatibility with
@MultiORMColumnpackages/core/src/lib/user-notification/user-notification-setting/commands/handlers/user-notification-setting.create.handler.ts (1)
1-20: LGTM! Well-structured command handler implementation.The implementation follows CQRS best practices with proper dependency injection and clear documentation.
packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.module.ts (1)
14-15: Verify the necessity of dual ORM configuration.The module is configured to use both TypeORM and MikroORM simultaneously. This could lead to:
- Increased complexity in managing database operations
- Potential data inconsistencies
- Higher maintenance overhead
Please confirm if this dual setup is intentional and necessary.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other modules using both ORMs to verify if this is a pattern echo "Searching for modules using both TypeOrmModule and MikroOrmModule..." rg "TypeOrmModule.forFeature|MikroOrmModule.forFeature" -A 5 -B 5Length of output: 65862
Dual ORM Configuration Confirmed as Intentional
The codebase consistently applies both TypeORM and MikroORM across numerous modules (e.g., GitHub, changelog, wakatime, proposals, etc.). This pattern indicates that the dual ORM setup is intentional and part of the overall architectural design.
- The user-notification-setting module aligns with similar configurations elsewhere.
- This practice is consistently applied, suggesting a deliberate design choice rather than an oversight.
packages/core/src/lib/user-notification/user-notification.module.ts (2)
15-16: Verify the necessity of dual ORM configuration.Similar to the settings module, this module also uses both TypeORM and MikroORM. Please confirm if this dual setup is required.
13-24: LGTM! Well-structured module configuration.The module is well-organized with appropriate imports, providers, and exports. The integration with role permissions and notification settings is properly configured.
packages/contracts/src/lib/comment.model.ts (2)
7-7: LGTM! Simplified interface inheritance.The removal of
IBasePerTenantAndOrganizationEntityModelaligns with the broader refactoring effort to streamline entity models.
24-30: LGTM! Added entityName for notification support.The addition of the optional
entityNameproperty enables better context in notifications when comments are created.packages/core/src/lib/favorite/favorite.entity.ts (1)
1-12: LGTM! Simplified entity inheritance.The changes align with the broader refactoring effort:
- Switched to BasePerEntityType
- Removed redundant properties
- Updated imports appropriately
packages/core/src/lib/tasks/task.module.ts (1)
19-19: LGTM! Verify notification handling in tasks.The UserNotificationModule integration looks correct. Ensure that task-related notifications are properly triggered in the TaskService and handlers.
Also applies to: 32-32
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if TaskService properly utilizes UserNotificationService ast-grep --pattern 'class TaskService { $$$ constructor($$$) { $$$ private readonly userNotificationService: UserNotificationService $$$ } $$$ }'Length of output: 167
I'm not seeing an output from the initial AST search. Let's gather more evidence to ensure TaskService is properly triggering notifications. I'll run a script to search for direct references to "userNotificationService" in TaskService and associated files.
We'll use the output of this script to verify that TaskService is properly utilizing the notification service.
🏁 Script executed:
#!/bin/bash # Search for usage of "this.userNotificationService" in files related to task logic. rg "this\.userNotificationService" -n -C 5 .Length of output: 7167
Notification handling in tasks appears properly integrated.
Our grep investigation confirms that the TaskService (in packages/core/src/lib/tasks/task.service.ts) and the task create handler (in packages/core/src/lib/tasks/commands/handlers/task-create.handler.ts) both invoke the userNotificationService.publishNotificationEvent method for task-related events. This verifies that task-related notifications are triggered as expected.
packages/contracts/src/lib/activity-log.model.ts (1)
11-11: Verify impact of removing tenant/organization base model.The removal of IBasePerTenantAndOrganizationEntityModel might affect tenant isolation. Ensure that tenant/organization scoping is handled appropriately through IBasePerEntityType.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if IBasePerEntityType includes tenant/organization fields ast-grep --pattern 'interface IBasePerEntityType { $$$ tenantId?: $_; organizationId?: $_; $$$ }'Length of output: 107
I'm not seeing evidence from the previous script. Let's run a more relaxed search to explicitly check if the IBasePerEntityType interface includes tenantId and organizationId definitions. We'll search for these fields within the interface block.
🏁 Script executed:
#!/bin/bash echo "Searching for tenant/organization fields in IBasePerEntityType interface:" rg -n "interface IBasePerEntityType" -A 50 | grep -E "(tenantId\?:|organizationId\?:)"Length of output: 245
Let's verify whether references to the removed IBasePerTenantAndOrganizationEntityModel are still present in the codebase, to better understand the impact of its removal.
🏁 Script executed:
#!/bin/bash # Search for any references to IBasePerTenantAndOrganizationEntityModel in the entire repository. rg -n "IBasePerTenantAndOrganizationEntityModel"Length of output: 62861
Tenant/Organization scoping remains intact.
The investigation shows that IBasePerEntityType extends IBasePerTenantAndOrganizationEntityModel (as seen in packages/contracts/src/lib/base-entity.model.ts near line 63), which means that tenantId and organizationId are still part of the inheritance chain for IActivityLog. This removal does not compromise the tenant/organization scoping since the necessary fields continue to be inherited.
packages/core/src/lib/core/entities/subscribers/index.ts (1)
49-50: LGTM! Export order matches array order.The export order of subscribers matches their order in the
coreSubscribersarray, maintaining consistency.packages/core/src/lib/activity-log/activity-log.entity.ts (1)
14-14: LGTM! Proper inheritance from BasePerEntityType.The class correctly extends BasePerEntityType and implements IActivityLog, maintaining proper type safety.
packages/core/src/lib/comment/comment.service.ts (1)
69-70: LGTM! Enhanced mention publishing with entity name.The addition of
entityNameto the mention publishing process enhances the context provided for notifications.packages/core/src/lib/mention/mention.service.ts (2)
4-12: LGTM! Well-structured notification system integration.The addition of UserNotificationService and related enums sets up a proper foundation for the notification system.
Also applies to: 20-20, 27-27
67-82: LGTM! Comprehensive notification event publishing.The implementation properly handles all necessary details for notification publishing:
- Entity context
- User information
- Organization and tenant context
- Action type and user display name
packages/core/src/lib/core/entities/index.ts (1)
153-154: LGTM! Proper integration of notification entities.The UserNotification and UserNotificationSetting entities are correctly added to both imports and exports, maintaining consistency in the core entities system.
Also applies to: 313-314
packages/contracts/src/index.ts (1)
145-146: LGTM! New notification model exports added.The new exports for user notification models are correctly placed and follow the existing pattern.
packages/core/src/lib/tasks/commands/handlers/task-create.handler.ts (4)
10-12: LGTM! Notification-related imports added.The new imports for notification enums are correctly placed and necessary for the notification functionality.
40-41: LGTM! User notification service properly injected.The service is correctly injected in the constructor following dependency injection best practices.
59-60: LGTM! User context retrieval added.Properly retrieving the current user context for notification purposes.
154-167: LGTM! Notification publishing logic implemented.The notification publishing logic is well-structured with all required parameters:
- Correct entity type and ID
- Proper notification type for assignments
- Includes sender and receiver IDs
- Includes task title and user name for context
packages/core/src/lib/tasks/screening-tasks/screening-tasks.service.ts (1)
102-103: LGTM! Entity name added to mention publishing.The addition of
entityNameparameter enhances the mention functionality by providing task context.packages/core/src/lib/core/entities/internal.ts (1)
6-6: LGTM! Notification-related exports added.The new exports are properly organized:
- Base entity type export with other base entities
- User notification entities with other core entities
- Notification setting subscriber with other subscribers
Also applies to: 159-160, 212-212
packages/core/src/lib/app/app.module.ts (1)
165-166: LGTM! Module imports are correctly added.The UserNotificationModule and UserNotificationSettingModule are properly imported and added to the module's imports array.
Also applies to: 475-476
packages/core/src/lib/tasks/task.service.ts (2)
31-33: LGTM! Required imports are correctly added.The necessary enums and service for user notifications are properly imported.
Also applies to: 51-51
63-64: LGTM! Dependency injection is properly implemented.The UserNotificationService is correctly injected following the established pattern.
packages/core/src/lib/user-notification/user-notification.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/user-notification/user-notification.helper.ts
Outdated
Show resolved
Hide resolved
...ore/src/lib/user-notification/user-notification-setting/user-notification-setting.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/user-notification/user-notification.controller.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/user-notification/user-notification.controller.ts
Outdated
Show resolved
Hide resolved
|
@GloireMutaliko21 please review coderabbit suggestions! |
g missing fields
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/lib/user-notification/user-notification.helper.ts (1)
3-10:⚠️ Potential issueFix inconsistencies in notification templates.
There are several issues in the templates:
- Inconsistent placeholder usage: The templates use
{userName}while some might expect{user}.- The "Messaged" template has an extra quote character at the end.
Apply this diff to fix the issues:
const UserNotificationTemplates = { [NotificationActionTypeEnum.Paid]: `You have been {action}`, [NotificationActionTypeEnum.Commented]: `{userName} {action} on {entity} "{entityName}"`, [NotificationActionTypeEnum.Assigned]: `{userName} {action} you to the {entity} "{entityName}"`, [NotificationActionTypeEnum.Invited]: `{userName} {action} you to join the {entity} "{entityName}"`, [NotificationActionTypeEnum.Mentioned]: `{userName} {action} you on the {entity} "{entityName}"`, - [NotificationActionTypeEnum.Messaged]: `{userName} {action} you a new message"` + [NotificationActionTypeEnum.Messaged]: `{userName} {action} you a new message` };
🧹 Nitpick comments (3)
packages/core/src/lib/user-notification/user-notification.helper.ts (1)
12-36: Consider enhancing error handling in the template processor.The function silently returns an empty string for unknown placeholders, which might hide template issues.
Consider throwing an error for unknown placeholders:
export function generateNotificationTitle( action: NotificationActionTypeEnum, entity: BaseEntityEnum, entityName: string, userName: string ) { const template = UserNotificationTemplates[action]; return template.replace(/\{(\w+)\}/g, (_, key) => { switch (key) { case 'action': return action; case 'entity': return entity; case 'entityName': return entityName; case 'userName': return userName; default: - return ''; + throw new Error(`Unknown placeholder: {${key}}`); } }); }packages/core/src/lib/user-notification/user-notification.service.ts (2)
89-103: Improve error handling in markAllAsRead method.The method should handle the case where no user ID is available in the request context.
Apply this change:
async markAllAsRead(): Promise<any> { try { const userId = RequestContext.currentUserId(); + if (!userId) { + throw new BadRequestException('User ID not found in request context'); + } const readAt = new Date(); return await super.update( { isRead: false, isArchived: false, receiverId: userId }, { isRead: true, readAt } ); } catch (error) { throw new BadRequestException('Error while updating notifications', error); } }
135-155: Consider using a Map for notification type mapping.The switch statement could be replaced with a Map for better maintainability and performance.
Consider this alternative implementation:
+private readonly notificationTypeMap = new Map<UserNotificationTypeEnum, (settings: IUserNotificationSetting) => boolean>([ + [UserNotificationTypeEnum.PAYMENT, (settings) => settings.payment ?? true], + [UserNotificationTypeEnum.ASSIGNMENT, (settings) => settings.assignment ?? true], + [UserNotificationTypeEnum.INVITATION, (settings) => settings.invitation ?? true], + [UserNotificationTypeEnum.MENTION, (settings) => settings.mention ?? true], + [UserNotificationTypeEnum.COMMENT, (settings) => settings.comment ?? true], + [UserNotificationTypeEnum.MESSAGE, (settings) => settings.message ?? true], +]); + private shouldCreateUserNotification( userNotificationSetting: IUserNotificationSetting, type: UserNotificationTypeEnum ): boolean { - switch (type) { - case UserNotificationTypeEnum.PAYMENT: - return userNotificationSetting.payment ?? true; - case UserNotificationTypeEnum.ASSIGNMENT: - return userNotificationSetting.assignment ?? true; - case UserNotificationTypeEnum.INVITATION: - return userNotificationSetting.invitation ?? true; - case UserNotificationTypeEnum.MENTION: - return userNotificationSetting.mention ?? true; - case UserNotificationTypeEnum.COMMENT: - return userNotificationSetting.comment ?? true; - case UserNotificationTypeEnum.MESSAGE: - return userNotificationSetting.message ?? true; - default: - return false; - } + const handler = this.notificationTypeMap.get(type); + return handler ? handler(userNotificationSetting) : false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/lib/user-notification/user-notification.helper.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.service.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 (1)
packages/core/src/lib/user-notification/user-notification.service.ts (1)
37-81:⚠️ Potential issueFix return type mismatch and improve error handling.
- The method returns
undefinedwhen notification is not allowed, but the return type isPromise<IUserNotification>.- Error logging should use a proper logging service instead of console.log.
Apply these changes:
-async create(input: IUserNotificationCreateInput): Promise<IUserNotification> { +async create(input: IUserNotificationCreateInput): Promise<IUserNotification | undefined> { try { const tenantId = RequestContext.currentTenantId() || input.tenantId; let userSetting: IUserNotificationSetting; try { userSetting = await this.userNotificationSettingService.findOneByWhereOptions({ userId: input.receiverId, tenantId, organizationId: input.organizationId }); } catch (error) { if (error instanceof NotFoundException) { userSetting = await this.userNotificationSettingService.create({ userId: input.receiverId, assignment: true, comment: true, invitation: true, mention: true, message: true, payment: true, preferences: { email: true, inApp: true }, organizationId: input.organizationId, tenantId }); } } const isAllowedNotification = this.shouldCreateUserNotification(userSetting, input.type); if (!isAllowedNotification) { - return; + return undefined; } return await super.create({ ...input, tenantId }); } catch (error) { - console.log('Error while creating notification:', error); + this.logger.error('Error while creating notification:', error); throw new BadRequestException('Error while creating notification', error); } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/src/lib/user-notification/user-notification.service.ts (2)
89-102: Improve error handling and response inmarkAllAsRead.
- The method returns a hardcoded success message.
- Error handling could be more informative.
Apply this diff to improve the implementation:
- return { message: 'successful' }; + return { message: 'All notifications marked as read', count: result.affected };- throw new BadRequestException('Error while updating notifications', error); + throw new BadRequestException(error.message || 'Error while updating notifications');
134-154: Improve default case handling inshouldCreateUserNotification.The default case returns false without any explanation, which could make debugging difficult.
Apply this diff to improve error handling:
- return false; + console.warn(`Unknown notification type: ${type}`); + return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/contracts/src/lib/user-notification.model.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.helper.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts/src/lib/user-notification.model.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (2)
packages/core/src/lib/user-notification/user-notification.helper.ts (1)
3-10:⚠️ Potential issueFix inconsistency in the "Messaged" template.
The "Messaged" template has a different format and an extra quote character compared to other templates.
Apply this diff to maintain consistency:
- [NotificationActionTypeEnum.Messaged]: `{userName} sent you a new message"` + [NotificationActionTypeEnum.Messaged]: `{userName} {action} you a new message`Likely invalid or redundant comment.
packages/core/src/lib/user-notification/user-notification.service.ts (1)
112-125: LGTM!The method is well-documented and follows the single responsibility principle.
packages/core/src/lib/user-notification/user-notification.helper.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/user-notification/user-notification.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/shared/pipes/notification-type-transform.pipe.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/user-notification/events/user-notification.event.ts
Outdated
Show resolved
Hide resolved
.../lib/user-notification/user-notification-setting/dto/create-user-notification-setting.dto.ts
Outdated
Show resolved
Hide resolved
.../src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…om/ever-co/ever-gauzy into feat/internal-system-notifications
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/lib/user-notification/pipes/notification-type-transform.pipe.ts (2)
15-32: Consider enhancing type safety and maintainability.While the implementation is functional, consider these improvements:
- Define the integer mappings as constants to avoid magic numbers
- Add type safety for future enum additions
Here's a suggested implementation:
+const NOTIFICATION_TYPE_MAPPING = { + [UserNotificationTypeEnum.PAYMENT]: 0, + [UserNotificationTypeEnum.ASSIGNMENT]: 1, + [UserNotificationTypeEnum.INVITATION]: 2, + [UserNotificationTypeEnum.MENTION]: 3, + [UserNotificationTypeEnum.COMMENT]: 4, + [UserNotificationTypeEnum.MESSAGE]: 5, +} as const; + +type NotificationTypeValues = typeof NOTIFICATION_TYPE_MAPPING[keyof typeof NOTIFICATION_TYPE_MAPPING]; + to(value: UserNotificationTypeEnum): number { - switch (value) { - case UserNotificationTypeEnum.PAYMENT: - return 0; - case UserNotificationTypeEnum.ASSIGNMENT: - return 1; - case UserNotificationTypeEnum.INVITATION: - return 2; - case UserNotificationTypeEnum.MENTION: - return 3; - case UserNotificationTypeEnum.COMMENT: - return 4; - case UserNotificationTypeEnum.MESSAGE: - return 5; - default: - throw new Error(`Unknown notification type: ${value}`); + const mappedValue = NOTIFICATION_TYPE_MAPPING[value]; + if (mappedValue === undefined) { + throw new Error(`Invalid notification type: ${value}. Expected one of: ${Object.keys(NOTIFICATION_TYPE_MAPPING).join(', ')}`); } + return mappedValue; }
40-57: Refactor to use the same mapping for bidirectional conversion.The
frommethod can be simplified by using the same constant mapping as thetomethod, making the code more maintainable.Here's a suggested implementation that complements the previous changes:
from(value: number): UserNotificationTypeEnum { - switch (value) { - case 0: - return UserNotificationTypeEnum.PAYMENT; - case 1: - return UserNotificationTypeEnum.ASSIGNMENT; - case 2: - return UserNotificationTypeEnum.INVITATION; - case 3: - return UserNotificationTypeEnum.MENTION; - case 4: - return UserNotificationTypeEnum.COMMENT; - case 5: - return UserNotificationTypeEnum.MESSAGE; - default: - throw new Error(`Unknown notification type value: ${value}`); + const entry = Object.entries(NOTIFICATION_TYPE_MAPPING).find(([_, val]) => val === value); + if (!entry) { + throw new Error(`Invalid notification type value: ${value}. Expected one of: ${Object.values(NOTIFICATION_TYPE_MAPPING).join(', ')}`); } + return entry[0] as UserNotificationTypeEnum; }packages/core/src/lib/subscription/subscription.service.ts (1)
57-58: Consider implementing the optional notification feature.There's a TODO comment about optional subscription notifications. Since this PR is about internal system notifications, this might be a good opportunity to implement this feature.
Would you like me to help implement the subscription notification feature or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/contracts/src/index.ts(2 hunks)packages/contracts/src/lib/subscription.model.ts(2 hunks)packages/contracts/src/lib/user-notification-setting.model.ts(1 hunks)packages/core/src/lib/core/entities/index.ts(2 hunks)packages/core/src/lib/core/entities/internal.ts(3 hunks)packages/core/src/lib/subscription/commands/handlers/subscription.create.handler.ts(2 hunks)packages/core/src/lib/subscription/commands/subscription.create.command.ts(1 hunks)packages/core/src/lib/subscription/dto/create-subscription.dto.ts(1 hunks)packages/core/src/lib/subscription/dto/subscription-find-input.dto.ts(1 hunks)packages/core/src/lib/subscription/events/handlers/subscription.create.handler.ts(2 hunks)packages/core/src/lib/subscription/events/subscription.create.event.ts(1 hunks)packages/core/src/lib/subscription/subscription.controller.ts(3 hunks)packages/core/src/lib/subscription/subscription.entity.ts(1 hunks)packages/core/src/lib/subscription/subscription.service.ts(3 hunks)packages/core/src/lib/user-notification/events/handlers/index.ts(1 hunks)packages/core/src/lib/user-notification/events/handlers/user-notification.handler.ts(1 hunks)packages/core/src/lib/user-notification/events/user-notification.event.ts(1 hunks)packages/core/src/lib/user-notification/pipes/index.ts(1 hunks)packages/core/src/lib/user-notification/pipes/notification-type-transform.pipe.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/dto/create-user-notification-setting.dto.ts(1 hunks)packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.entity.ts(1 hunks)packages/core/src/lib/user-notification/user-notification.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/lib/user-notification/pipes/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/lib/user-notification/events/handlers/index.ts
- packages/core/src/lib/core/entities/index.ts
- packages/core/src/lib/user-notification/events/user-notification.event.ts
- packages/core/src/lib/user-notification/user-notification-setting/user-notification-setting.controller.ts
- packages/core/src/lib/user-notification/user-notification.entity.ts
- packages/contracts/src/lib/user-notification-setting.model.ts
- packages/core/src/lib/user-notification/user-notification.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (20)
packages/contracts/src/index.ts (2)
146-147: LGTM! New notification models are correctly exported.The new exports for user notification models are properly placed and maintain consistency with the existing export pattern.
161-162: LGTM! Base entity exports are properly updated.The addition of
IBasePerEntityTypeexport and the trailing comma improvement maintain good code organization.packages/core/src/lib/user-notification/user-notification-setting/dto/create-user-notification-setting.dto.ts (2)
1-4: LGTM! Imports are well-organized and necessary.The imports follow a logical organization pattern and each import serves a clear purpose in the implementation.
6-11: LGTM! Well-structured DTO implementation.The DTO correctly:
- Combines base DTOs using IntersectionType
- Excludes user-related fields as previously requested
- Implements the contract interface
packages/core/src/lib/core/entities/internal.ts (1)
6-6: LGTM! New exports are well-organized.The new exports for user notification entities and subscriber are properly placed in their respective sections and follow the established naming conventions.
Let's verify the implementation of these new entities and subscriber:
Also applies to: 160-161, 213-213
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and basic structure of the new notification-related files # Check for entity-type-base.entity echo "Checking entity-type-base.entity..." fd "entity-type-base.entity.ts$" # Check for user notification entities echo "Checking user notification entities..." fd "user-notification.entity.ts$" fd "user-notification-setting.entity.ts$" # Check for user notification subscriber echo "Checking user notification subscriber..." fd "user-notification-setting.subscriber.ts$" # Verify basic structure of the entities echo "Checking entity implementations..." for file in $(fd -t f "(entity-type-base|user-notification|user-notification-setting)\.(entity|subscriber)\.ts$"); do echo "=== Content of $file ===" cat "$file" doneLength of output: 13372
LGTM! All new exports have been verified.
- The export in the barrel file is correctly updated.
- The files for
entity-type-base.entity,user-notification.entity,user-notification-setting.entity, anduser-notification-setting.subscriberexist in the expected directories.- Their implementations align with the project’s naming conventions and structure.
packages/core/src/lib/user-notification/events/handlers/user-notification.handler.ts (2)
1-4: LGTM! Clean and well-organized imports.The imports are properly organized, following the convention of external dependencies first, followed by internal modules.
5-7: LGTM! Well-structured event handler class.The class follows NestJS best practices with proper dependency injection and event handler implementation. Good use of the readonly modifier for immutability.
packages/core/src/lib/user-notification/pipes/notification-type-transform.pipe.ts (2)
1-8: Well-structured class with clear documentation!The class follows Angular's pipe naming convention and is well-documented with clear JSDoc comments explaining its purpose.
1-58: Verify enum values and module registration.Please ensure:
- The enum values in
UserNotificationTypeEnummatch these mappings- The pipe is properly registered in the
UserNotificationModule✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check enum values in contracts rg -A 10 "export enum UserNotificationTypeEnum" # Check module registration rg "UserNotificationTypeTransformerPipe" -A 5 -B 5Length of output: 5446
Enum mappings and transformer integration verified.
- The enum values in
UserNotificationTypeEnumcorrectly match the expected mappings.- The transformer is instantiated within the entity (via the
MultiORMColumndecorator inuser-notification.entity.ts), confirming its integration with the user notification system.packages/core/src/lib/subscription/events/subscription.create.event.ts (1)
1-6: LGTM! Interface update is consistent with the refactoring.The change from
ISubscriptionCreateInputtoIEntitySubscriptionCreateInputmaintains type safety while aligning with the broader interface renaming pattern.packages/core/src/lib/subscription/dto/subscription-find-input.dto.ts (1)
1-5: LGTM! DTO update maintains type safety.The change from
ISubscriptionFindInputtoIEntitySubscriptionFindInputis consistent with the refactoring while preserving the existing inheritance structure.packages/core/src/lib/subscription/commands/subscription.create.command.ts (1)
1-8: LGTM! Command interface update is consistent.The change from
ISubscriptionCreateInputtoIEntitySubscriptionCreateInputmaintains the command pattern while aligning with the interface renaming.packages/core/src/lib/subscription/dto/create-subscription.dto.ts (1)
9-11: Verify omitted properties after interface change.While the change from
ISubscriptionCreateInputtoIEntitySubscriptionCreateInputis consistent, please verify that omitting 'userId' and 'user' properties is still valid with the new interface.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the interface definition to verify omitted properties ast-grep --pattern 'interface IEntitySubscriptionCreateInput { $$$ }'Length of output: 73
I'm not seeing the interface definition for
IEntitySubscriptionCreateInputwith the initial AST query. It appears that the interface might be defined using a different pattern (or as a type alias) that our AST query didn't catch. Before drawing a conclusion about the validity of omitting the properties, we should verify howIEntitySubscriptionCreateInputis defined and whether it indeed includes (or excludes) the'userId'and'user'properties.Let's gather additional information by searching for the export of
IEntitySubscriptionCreateInputusing a general regex search. Please run the following script:If this search still yields no results or if the definition is unclear, a manual verification of the interface's definition will be necessary to confirm that omitting
'userId'and'user'remains valid.
🏁 Script executed:
#!/bin/bash rg -e "export (interface|type) IEntitySubscriptionCreateInput" -n .Length of output: 293
I'll now gather additional context from the definition of
IEntitySubscriptionCreateInputto see if it includes the properties'userId'and'user'. Please run the following shell script to output the surrounding lines of the interface:
🏁 Script executed:
#!/bin/bash rg -A 20 "export interface IEntitySubscriptionCreateInput" -n packages/contracts/src/lib/subscription.model.tsLength of output: 489
Omission of 'user' and 'userId' is Valid Per the New Interface.
The interface
IEntitySubscriptionCreateInput(defined inpackages/contracts/src/lib/subscription.model.tsat line 18) extends:
Omit<IEntitySubscription, 'user' | 'userId'>— excluding bothuseranduserIdPartial<Pick<IEntitySubscription, 'userId'>>— reintroducinguserIdas an optional propertyThis means that omitting both properties in the DTO via:
OmitType(Subscription, ['userId', 'user'])is consistent with the expected structure, as the interface does not mandate
userId(it is optional) and completely omitsuser.packages/core/src/lib/subscription/commands/handlers/subscription.create.handler.ts (1)
1-1: LGTM! Interface updates are consistent with the broader refactoring.The changes correctly update the import and return type from
ISubscriptiontoIEntitySubscriptionwhile maintaining the same handler logic.Also applies to: 10-10
packages/contracts/src/lib/subscription.model.ts (1)
1-1: LGTM! Interface updates simplify the model structure.The changes correctly:
- Remove the tenant/organization base model dependency
- Rename interfaces to include "Entity" prefix
- Maintain type consistency across the model
Also applies to: 4-4, 18-22
packages/core/src/lib/subscription/events/handlers/subscription.create.handler.ts (1)
2-2: LGTM! Handler and documentation updates are consistent.The changes correctly update both the implementation and documentation to use
IEntitySubscription.Also applies to: 14-14, 17-17
packages/core/src/lib/subscription/subscription.entity.ts (2)
11-12: Rename class to EntitySubscription as previously requested.As discussed in previous reviews, please rename the class to
EntitySubscriptionto maintain consistency with the new naming convention.
11-11: Verify table name update in migrations.Since the table name will be renamed to
entity_subscription, please ensure that:
- A migration file exists for this table rename
- The entity decorator is updated accordingly
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for migration files containing table rename rg -l "entity_subscription" packages/core/src/database/migrations/Length of output: 145
I'll run another command to search the entire repository for any migration file reference to the new table name (
entity_subscription) using a likely SQL phrase (e.g., aRENAME TOclause):
🏁 Script executed:
#!/bin/bash rg -l "RENAME TO entity_subscription" .Length of output: 41
Action Required: Migration Update and Entity Decorator Inconsistency
Our searches for any migration file indicating a rename to
entity_subscription(via patterns like"entity_subscription"and"RENAME TO entity_subscription") returned no evidence of such a migration. Additionally, the entity decorator still uses'subscription'rather than the expected new table name of'entity_subscription'.
- Migration File: There is no migration script that renames the table to
entity_subscription.- Entity Decorator: The decorator remains unchanged (
@MultiORMEntity('subscription', ...)) instead of reflecting the new table name.Please verify and update:
- Add or update a migration file to rename the table from
subscriptiontoentity_subscription.- Update the entity decorator to use
'entity_subscription'accordingly.packages/core/src/lib/subscription/subscription.controller.ts (1)
5-5: LGTM! Interface renaming changes are consistent.The changes correctly update the return types to use the new
IEntitySubscriptioninterface, maintaining consistency with the broader refactoring effort across the codebase.Also applies to: 36-36, 70-70
packages/core/src/lib/subscription/subscription.service.ts (1)
2-7: LGTM! Interface renaming changes are consistent.The changes correctly update the imports, method signatures, and JSDoc comments to use the new interface names, maintaining consistency with the broader refactoring effort across the codebase.
Also applies to: 27-31, 71-71, 78-78
packages/core/src/lib/user-notification/events/handlers/user-notification.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (24)
nx.json (1)
173-179: Nitpick: Consider Reassessing the "defaultBase" ValueThe final configuration settings, including "useInferencePlugins", "defaultBase", and "release", are standard. Note that "defaultBase" is set to "master". If your repository has migrated to using "main", it might be prudent to update this value for consistency.
packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.subscriber.ts (2)
23-37: Consider using a structured logger instead ofconsole.error.Here, the error is currently being logged using
console.error(error). Using a more structured logging framework (e.g., the NestJS logger) can help with better log formatting and maintain consistent logging across services. This also helps ensure sensitive data isn’t inadvertently printed to stdout.- console.error(error); + Logger.error(error.message, error.stack, 'EmployeeNotificationSettingSubscriber');
69-83: Validate the structure of parsedpreferences.After parsing, it might be valuable to verify that the parsed object adheres to expected properties or structure, especially since errors are recovered by setting
preferencesto an empty object.packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.controller.ts (3)
27-29: Clarify or customize permissions.You are using
@UseGuards(TenantPermissionGuard, PermissionGuard)and@Permissions()without specifying explicit permissions or roles. It may be helpful to define relevant permission keys for improved clarity and maintainability of access control.
77-83: Add error handling for not found records.Although
findOneByIdStringcan throw an error or return undefined if the record is not found, the code does not explicitly handle that scenario. Consider throwing an HTTP exception when the record is not found to return the correct HTTP 404 status.const entity = await this.employeeNotificationSettingService.findOneByIdString(id, params); - return entity; + if (!entity) { + throw new NotFoundException(`EmployeeNotificationSetting with ID ${id} not found`); + } + return entity;
145-147: Consider returning a custom response or status.Returning
DeleteResultdirectly might leak low-level details (e.g., how many records were deleted). It can be beneficial to return a more semantically relevant response object orHttpStatus.OKwith a success message to clients.packages/core/src/lib/employee-notification/employee-notification.service.ts (3)
37-86: Add unit tests for thecreatemethod.
Thiscreatemethod contains important business logic—such as handling default notification settings, verifying notification allowances, and persisting the entity—which should be thoroughly tested to prevent regressions and ensure correctness.Would you like help generating a set of unit tests to validate the different branches of this logic?
76-78: Clarify the return ofundefined.
When notifications are disallowed, returningundefinedmay cause confusion for consumers expecting a valid notification object. Ensure that calling code accounts for this possibility or consider throwing an exception for clearer handling.
83-83: Use NestJS logger instead ofconsole.log.
For consistency with NestJS logging best practices, replaceconsole.logstatements with Nest's built-in logger (e.g.,Logger.log).- console.log('Error while creating notification:', error); + Logger.error('Error while creating notification:', error);packages/core/src/lib/employee-notification/employee-notification-setting/commands/employee-notification-setting.create.command.ts (1)
4-8: LGTM! Consider adding class documentation.The command implementation follows CQRS pattern best practices with proper typing and immutability.
Consider adding JSDoc documentation to describe the command's purpose and input structure:
+/** + * Command to create a new employee notification setting + * @class EmployeeNotificationSettingCreateCommand + */ export class EmployeeNotificationSettingCreateCommand implements ICommand {packages/core/src/lib/employee-notification/employee-notification-setting/commands/employee-notification-setting.update.command.ts (1)
4-8: LGTM! Consider adding class documentation.The command implementation follows CQRS pattern best practices with proper typing and immutability.
Consider adding JSDoc documentation to describe the command's purpose and parameters:
+/** + * Command to update an existing employee notification setting + * @class EmployeeNotificationSettingUpdateCommand + * @param id - The ID of the notification setting to update + * @param input - The update data + */ export class EmployeeNotificationSettingUpdateCommand implements ICommand {packages/core/src/lib/employee-notification/repository/type-orm-employee-notification.repository.ts (1)
6-11: LGTM! Consider adding error handling and documentation.The repository implementation follows TypeORM and NestJS patterns correctly.
Consider these improvements:
- Add error handling for repository initialization:
constructor(@InjectRepository(EmployeeNotification) readonly repository: Repository<EmployeeNotification>) { + if (!repository) { + throw new Error('EmployeeNotification repository not provided'); + } super(repository.target, repository.manager, repository.queryRunner); }
- Add class documentation:
+/** + * TypeORM repository for managing employee notifications + * @class TypeOrmEmployeeNotificationRepository + */ @Injectable() export class TypeOrmEmployeeNotificationRepository extends Repository<EmployeeNotification> {packages/core/src/lib/employee-notification/employee-notification-setting/repository/type-orm-employee-notification-setting.repository.ts (1)
7-13: Consider adding custom repository methods for common queries.While extending the TypeORM Repository provides basic CRUD operations, consider adding custom methods for frequently used queries to improve code reusability and maintainability.
Example methods to consider:
async findByEmployeeId(employeeId: string): Promise<EmployeeNotificationSetting> { return this.findOne({ where: { employeeId } }); } async findByTenantAndOrganization(tenantId: string, organizationId: string): Promise<EmployeeNotificationSetting[]> { return this.find({ where: { tenantId, organizationId } }); }packages/contracts/src/lib/employee-notification-setting.model.ts (1)
4-15: Enhance JSDoc documentation and type definitions.While the interface is well-structured, the documentation and type definitions could be improved.
Consider these improvements:
/** * Interface for employee notification settings. * Defines the structure for managing notification preferences per employee. * * @property payment - Whether to receive payment-related notifications * @property assignment - Whether to receive task assignment notifications * @property invitation - Whether to receive invitation notifications * @property mention - Whether to receive mention notifications * @property comment - Whether to receive comment notifications * @property message - Whether to receive message notifications * @property preferences - Additional notification preferences in JSON format */ export interface IEmployeeNotificationSetting extends IEmployeeEntityInput, IBasePerTenantAndOrganizationEntityModel { payment?: boolean; assignment?: boolean; invitation?: boolean; mention?: boolean; comment?: boolean; message?: boolean; preferences?: Record<string, unknown>; // More specific than JsonData }packages/core/src/lib/employee-notification/employee-notification.module.ts (1)
16-17: Consider standardizing ORM usage.The module is configured to use both TypeORM and MikroORM simultaneously. This dual ORM setup could lead to:
- Increased maintenance overhead
- Potential data consistency issues
- Added complexity in repository implementations
Consider standardizing on a single ORM across the application to reduce complexity and maintenance overhead.
packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.module.ts (1)
20-20: Consider explicit command handler registration.Using spread operator with CommandHandlers could make it harder to track which handlers are actually registered.
Consider listing command handlers explicitly for better code clarity:
- providers: [EmployeeNotificationSettingService, TypeOrmEmployeeNotificationSettingRepository, ...CommandHandlers], + providers: [ + EmployeeNotificationSettingService, + TypeOrmEmployeeNotificationSettingRepository, + EmployeeNotificationSettingCreateHandler, + EmployeeNotificationSettingUpdateHandler + ],packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.service.ts (2)
24-36: Enhance error handling with specific error types.The current error handling catches all errors and converts them to
HttpException. Consider handling specific error types differently.async create(input: IEmployeeNotificationSettingCreateInput): Promise<EmployeeNotificationSetting> { try { const tenantId = RequestContext.currentTenantId() || input.tenantId; const employeeId = input.employeeId || RequestContext.currentEmployeeId(); return super.create({ ...input, employeeId, tenantId }); } catch (error) { - throw new HttpException( - `Failed to create the notification setting: ${error.message}`, - HttpStatus.BAD_REQUEST - ); + if (error instanceof TypeError) { + throw new HttpException( + 'Invalid input data for notification setting', + HttpStatus.BAD_REQUEST + ); + } + if (error.code === '23505') { // PostgreSQL unique violation + throw new HttpException( + 'Notification setting already exists', + HttpStatus.CONFLICT + ); + } + throw new HttpException( + 'Failed to create notification setting', + HttpStatus.INTERNAL_SERVER_ERROR + ); } }
18-23: Improve method documentation.The JSDoc comment could be more descriptive about the parameters and potential errors.
/** * Creates an employee notification setting record * - * @param {IEmployeeNotificationSetting} input - The input data for creating a notification setting + * @param {IEmployeeNotificationSettingCreateInput} input - The input data containing notification preferences + * @param {string} [input.tenantId] - Optional tenant ID. If not provided, current tenant ID is used + * @param {string} [input.employeeId] - Optional employee ID. If not provided, current employee ID is used * @returns {Promise<EmployeeNotificationSetting>} The created notification setting + * @throws {HttpException} When creation fails or input is invalid */packages/core/src/lib/employee-notification/employee-notification-type-transform.ts (1)
15-32: Refactor enum mapping to reduce maintenance overhead.The current implementation uses magic numbers and requires maintaining two switch statements. Consider using a mapping object.
+private static readonly enumToNumber = { + [EmployeeNotificationTypeEnum.PAYMENT]: 0, + [EmployeeNotificationTypeEnum.ASSIGNMENT]: 1, + [EmployeeNotificationTypeEnum.INVITATION]: 2, + [EmployeeNotificationTypeEnum.MENTION]: 3, + [EmployeeNotificationTypeEnum.COMMENT]: 4, + [EmployeeNotificationTypeEnum.MESSAGE]: 5, +} as const; + +private static readonly numberToEnum = Object.fromEntries( + Object.entries(EmployeeNotificationTypeTransformer.enumToNumber) + .map(([key, value]) => [value, key]) +) as Record<number, EmployeeNotificationTypeEnum>; + to(value: EmployeeNotificationTypeEnum): number { - switch (value) { - case EmployeeNotificationTypeEnum.PAYMENT: - return 0; - // ... other cases - default: - throw new Error(`Unknown notification type: ${value}`); + const result = EmployeeNotificationTypeTransformer.enumToNumber[value]; + if (result === undefined) { + throw new Error(`Unknown notification type: ${value}`); } + return result; }packages/core/src/lib/employee-notification/employee-notification.controller.ts (1)
15-17: Follow naming convention for service injection.The service property name should be camelCase to follow TypeScript naming conventions.
-constructor(readonly EmployeeNotificationService: EmployeeNotificationService) { +constructor(readonly employeeNotificationService: EmployeeNotificationService) { super(EmployeeNotificationService); }packages/core/src/lib/employee-notification/employee-notification.helper.ts (1)
31-55: Consider adding template validation.While the implementation is solid, consider adding validation to ensure all required placeholders in a template are provided in the parameters.
export function generateNotificationTitle( action: NotificationActionTypeEnum, entity: BaseEntityEnum, entityName: string, employeeName: string ): string { // Retrieve the template corresponding to the notification action const template = EmployeeNotificationTemplates[action]; + + // Extract placeholders from template + const requiredPlaceholders = template.match(/\{(\w+)\}/g)?.map(p => p.slice(1, -1)) || []; + + // Validate all required placeholders have corresponding values + const missingPlaceholders = requiredPlaceholders.filter(placeholder => { + switch (placeholder) { + case 'action': return !action; + case 'entity': return !entity; + case 'entityName': return !entityName; + case 'employeeName': return !employeeName; + default: return true; + } + }); + + if (missingPlaceholders.length > 0) { + throw new Error(`Missing values for placeholders: ${missingPlaceholders.join(', ')}`); + } // Replace placeholders in the template with actual values return template.replace(/\{(\w+)\}/g, (_, key) => {packages/core/src/lib/mention/mention.service.ts (1)
67-82: Consider adding error handling for notification publishing.The notification publishing should be wrapped in a try-catch block to prevent notification failures from affecting the main mention creation flow.
// Trigger internal system notification for mentioned user + try { this._employeeNotificationService.publishNotificationEvent( { entity: parentEntityType ?? entity, entityId: parentEntityId ?? entityId, type: EmployeeNotificationTypeEnum.MENTION, sentById: user.id, receiverId: mentionedUserId, organizationId, tenantId }, NotificationActionTypeEnum.Mentioned, entityName, `${user.firstName} ${user.lastName}` ); + } catch (notificationError) { + console.warn('Failed to publish notification:', notificationError); + // Continue execution as notification failure shouldn't block mention creation + }packages/core/src/lib/tasks/commands/handlers/task-create.handler.ts (2)
33-42: Fix the service name casing.The service name should follow camelCase convention.
- private readonly EmployeeNotificationService: EmployeeNotificationService + private readonly employeeNotificationService: EmployeeNotificationService
140-169: Consider adding error handling for individual notification failures.The notification publishing should handle errors individually to prevent a single notification failure from affecting other notifications.
// Publish subscription events for each employee and send internal notification to users await Promise.all( employees.map(({ userId }: IEmployee) => { - this._cqrsEventBus.publish( - new CreateSubscriptionEvent({ - entity: BaseEntityEnum.Task, - entityId: task.id, - userId, - type: SubscriptionTypeEnum.ASSIGNMENT, - organizationId, - tenantId - }) - ); + return Promise.all([ + this._cqrsEventBus.publish( + new CreateSubscriptionEvent({ + entity: BaseEntityEnum.Task, + entityId: task.id, + userId, + type: SubscriptionTypeEnum.ASSIGNMENT, + organizationId, + tenantId + }) + ).catch(error => { + this.logger.error(`Failed to publish subscription for user ${userId}:`, error); + }), - this.EmployeeNotificationService.publishNotificationEvent( - { - entity: BaseEntityEnum.Task, - entityId: task.id, - type: EmployeeNotificationTypeEnum.ASSIGNMENT, - sentById: task.creatorId, - receiverId: userId, - organizationId, - tenantId - }, - NotificationActionTypeEnum.Assigned, - task.title, - `${user.firstName} ${user.lastName}` - ); + this.employeeNotificationService.publishNotificationEvent( + { + entity: BaseEntityEnum.Task, + entityId: task.id, + type: EmployeeNotificationTypeEnum.ASSIGNMENT, + sentById: task.creatorId, + receiverId: userId, + organizationId, + tenantId + }, + NotificationActionTypeEnum.Assigned, + task.title, + `${user.firstName} ${user.lastName}` + ).catch(error => { + this.logger.error(`Failed to publish notification for user ${userId}:`, error); + }) + ]); }) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
nx.json(1 hunks)packages/contracts/src/index.ts(2 hunks)packages/contracts/src/lib/base-entity.model.ts(2 hunks)packages/contracts/src/lib/employee-notification-setting.model.ts(1 hunks)packages/contracts/src/lib/employee-notification.model.ts(1 hunks)packages/core/src/lib/activity-log/activity-log.entity.ts(2 hunks)packages/core/src/lib/app/app.module.ts(2 hunks)packages/core/src/lib/comment/comment.entity.ts(3 hunks)packages/core/src/lib/core/entities/index.ts(2 hunks)packages/core/src/lib/core/entities/internal.ts(3 hunks)packages/core/src/lib/core/entities/subscribers/index.ts(2 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/employee-notification-setting.create.command.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/employee-notification-setting.update.command.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/handlers/employee-notification-setting.create.handler.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/handlers/employee-notification-setting.update.handler.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/handlers/index.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/commands/index.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/dto/create-employee-notification-setting.dto.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/dto/index.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/dto/update-employee-notification-setting.dto.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.controller.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.entity.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.module.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.service.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.subscriber.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/repository/mikro-orm-employee-notification-setting.repository.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-setting/repository/type-orm-employee-notification-setting.repository.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification-type-transform.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.controller.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.entity.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.helper.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.module.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.service.ts(1 hunks)packages/core/src/lib/employee-notification/events/employee-notification.event.ts(1 hunks)packages/core/src/lib/employee-notification/events/handlers/employee-notification.handler.ts(1 hunks)packages/core/src/lib/employee-notification/events/handlers/index.ts(1 hunks)packages/core/src/lib/employee-notification/events/index.ts(1 hunks)packages/core/src/lib/employee-notification/repository/mikro-orm-employee-notification.repository.ts(1 hunks)packages/core/src/lib/employee-notification/repository/type-orm-employee-notification.repository.ts(1 hunks)packages/core/src/lib/mention/mention.module.ts(2 hunks)packages/core/src/lib/mention/mention.service.ts(3 hunks)packages/core/src/lib/shared/pipes/actor-type-transform.ts(1 hunks)packages/core/src/lib/shared/pipes/index.ts(1 hunks)packages/core/src/lib/tasks/commands/handlers/task-create.handler.ts(7 hunks)packages/core/src/lib/tasks/task.module.ts(2 hunks)packages/core/src/lib/tasks/task.service.ts(6 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/lib/employee-notification/events/index.ts
- packages/core/src/lib/shared/pipes/index.ts
- packages/core/src/lib/employee-notification/employee-notification-setting/dto/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/lib/tasks/task.module.ts
- packages/core/src/lib/mention/mention.module.ts
- packages/core/src/lib/app/app.module.ts
- packages/core/src/lib/core/entities/subscribers/index.ts
- packages/core/src/lib/core/entities/index.ts
- packages/contracts/src/lib/base-entity.model.ts
- packages/core/src/lib/core/entities/internal.ts
- packages/contracts/src/index.ts
🧰 Additional context used
🪛 Gitleaks (8.21.2)
nx.json
88-88: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (29)
nx.json (4)
2-18: Approved: "namedInputs" Configuration Looks GoodThe "namedInputs" section is well structured and consistent with Nx conventions. All the defined input paths and groupings (default, sharedGlobals, production) appear correctly set up.
19-27: Approved: "cli" Settings are Configured ProperlyThe CLI configuration disables specific warnings and correctly sets the package manager and analytics options. This aligns with the project’s operational preferences.
28-85: Approved: "generators" Section is ComprehensiveThe generators configuration for Angular applications, libraries, and components is detailed and follows established conventions in the repository. All properties (linter, style, test runners, etc.) are well organized.
90-171: Approved: "targetDefaults" Section is DetailedThe "targetDefaults" configuration for various tasks such as build, lint, test, and e2e is comprehensive and appears to follow best practices in caching and dependency handling. No changes required here.
packages/core/src/lib/shared/pipes/actor-type-transform.ts (1)
5-8: LGTM! The class name change better reflects its purpose.The renaming from
ActorTypeTransformerPipetoActorTypeTransformeris more accurate as this class is a TypeORM value transformer, not a pipe. The documentation has been appropriately updated to reflect this change.packages/core/src/lib/activity-log/activity-log.entity.ts (3)
7-8: LGTM! Import statements have been cleaned up.The imports have been appropriately updated to reflect the new base class and removed unused imports.
28-28: LGTM! Transformer reference has been updated.The transformer reference has been correctly updated to use the renamed
ActorTypeTransformerclass.
14-14: Verify the impact of changing the base class.The change from
TenantOrganizationBaseEntitytoBasePerEntityTypemight affect tenant/organization relationships. Please ensure that:
- All required tenant/organization functionality is preserved
- Existing queries and filters still work as expected
- Data migration plans are in place
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for references to ActivityLog entity in queries and repositories rg -A 5 "ActivityLog.*tenant|ActivityLog.*organization" --type tsLength of output: 67
Manual Verification Required: Impact of Base Class Change on Tenant/Organization Logic
The automated search did not produce any direct references linking ActivityLog to tenant or organization filtering. However, this absence of results doesn’t definitively confirm that all tenant/organization functionality and data migration considerations have been preserved. Please manually verify that:
- The necessary tenant/organization fields and relationships in ActivityLog are maintained after switching from TenantOrganizationBaseEntity to BasePerEntityType.
- All existing queries and filters that might implicitly rely on the previous base class still operate as expected.
- There is an appropriate data migration strategy in place for transitioning any tenant/organization-specific data.
packages/core/src/lib/comment/comment.entity.ts (4)
7-8: LGTM! Clean import organization.The import statements are well-organized and correctly aligned with the class changes.
Also applies to: 17-17
63-179: LGTM! Well-structured entity with proper relationships.The entity maintains clean and well-documented relationships with appropriate cascade rules and comprehensive validation decorators.
34-34: Verify the transformer functionality.The change from
ActorTypeTransformerPipetoActorTypeTransformerappears to be a refactoring. Let's verify that the new transformer maintains the same functionality for theactorTypefield.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of both transformers # to ensure functionality remains the same echo "Checking ActorTypeTransformer implementation..." ast-grep --pattern 'class ActorTypeTransformer { $$$ }' echo "Checking for any remaining references to ActorTypeTransformerPipe..." rg "ActorTypeTransformerPipe" -lLength of output: 344
Transformer functionality verified.
- The new
ActorTypeTransformerclass is present in the codebase.- No occurrences of the old
ActorTypeTransformerPipewere detected.Based on the inspection, the refactoring appears to have been applied correctly, preserving the expected transformer functionality for the
actorTypefield.
21-21: Verify the inheritance change impact across the codebase.The change from
TenantOrganizationBaseEntitytoBasePerEntityTypeis a significant architectural modification. Let's verify that this change is consistently applied across related entities and doesn't break existing functionality.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to TenantOrganizationBaseEntity # and verify consistent usage of BasePerEntityType in related files echo "Checking for remaining TenantOrganizationBaseEntity references..." rg "TenantOrganizationBaseEntity" -l echo "Checking BasePerEntityType usage in related entities..." rg "extends BasePerEntityType" -l echo "Checking for potential migration files related to this change..." fd -e ts -p ".*migration.*" -x rg -l "comment.*table|BasePerEntityType"Length of output: 10960
Inheritance Change Impact Verified
- The
Commententity now extendsBasePerEntityType—just like several related entities (e.g., the ones in activity-log, favorite, mention, resource-link, subscription, and employee-notification).- A search shows that many entities still extend
TenantOrganizationBaseEntity; however, this appears to be an intentional, incremental migration rather than an oversight.- No migration files specific to this change in the comment module were found, suggesting no unexpected side effects for this particular update.
Overall, the inheritance change in
comment.entity.tsis applied consistently within its intended context. Please ensure that the selective migration strategy (updating only some entities) is documented and that the entities retainingTenantOrganizationBaseEntityare meant to behave differently.packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.subscriber.ts (1)
6-7: Good use of TypeORM event subscriber.The declaration correctly identifies that this subscriber will listen to events for the
EmployeeNotificationSettingentity. This is aligned with the recommended approach for TypeORM event subscribers and helps keep domain logic clean.packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.controller.ts (2)
54-58: Ensure pagination correctness and edge case handling.Within the
findAllmethod, consider validating whether pagination parameters (like page size or page number) are within acceptable ranges. This helps protect system performance and user experience.
95-99: Validate default boolean flags in the DTO.When creating a new notification setting, ensure your DTO or command handler either merges default boolean flags (e.g.,
trueforpayment,assignment) or references them from the server-side defaults. This can protect against partial data or injection attacks.packages/core/src/lib/employee-notification/employee-notification-setting/employee-notification-setting.entity.ts (1)
112-128: Double check “cascade” implications for employee relation.With
cascade: trueandonDelete: 'CASCADE', deleting an employee will remove all notification settings automatically. Verify it’s intentional. If not, you might want to handle cascade options more selectively to prevent accidental data loss.packages/core/src/lib/employee-notification/employee-notification.service.ts (1)
138-158: Verify default notification settings.
TheshouldCreateEmployeeNotificationmethod defaults all unset notification preferences totrue. Confirm this is the intended design, as users with unconfigured settings might receive more notifications than desired.packages/core/src/lib/employee-notification/employee-notification-setting/commands/index.ts (1)
1-2: LGTM.
Re-exporting commands via the index file simplifies imports across the codebase.packages/core/src/lib/employee-notification/events/handlers/index.ts (1)
1-3: LGTM.
ExportingEventHandlersas an array of handlers is a standard NestJS convention, making it straightforward to provide these handlers to any module.packages/core/src/lib/employee-notification/events/employee-notification.event.ts (1)
1-6: LGTM! Clean implementation following CQRS pattern.The event class is well-structured with proper immutability using readonly modifier and follows the CQRS pattern for event handling.
packages/core/src/lib/employee-notification/repository/mikro-orm-employee-notification.repository.ts (1)
1-4: LGTM! Clean repository implementation.The repository follows best practices by extending the base repository and properly typing it for EmployeeNotification entities.
packages/core/src/lib/employee-notification/employee-notification-setting/repository/mikro-orm-employee-notification-setting.repository.ts (1)
1-4: LGTM! Clean repository implementation.The repository follows best practices by extending the base repository and properly typing it for EmployeeNotificationSetting entities.
packages/core/src/lib/employee-notification/employee-notification-setting/commands/handlers/index.ts (1)
1-4: LGTM! Clean barrel export implementation.The file follows best practices by using the barrel pattern to export multiple command handlers, making it easier to import them elsewhere.
packages/core/src/lib/employee-notification/employee-notification-setting/dto/update-employee-notification-setting.dto.ts (1)
5-10: LGTM! Well-documented DTO implementation.The DTO implementation follows best practices by:
- Using PartialType to make all properties optional for updates
- Implementing the contract interface for type safety
- Including clear documentation
packages/core/src/lib/employee-notification/employee-notification.helper.ts (1)
3-10: LGTM! Well-structured notification templates.The templates are clearly defined and cover various notification types with consistent placeholder usage.
packages/core/src/lib/employee-notification/employee-notification.entity.ts (1)
12-132: LGTM! Well-designed entity with proper validation and indexing.The entity is well-structured with:
- Proper validation using class-validator
- Indexed columns for performance
- Correct cascade delete behavior for relations
- Clear documentation
packages/core/src/lib/mention/mention.service.ts (1)
24-31: LGTM! Proper dependency injection.The service correctly injects the required dependencies.
packages/core/src/lib/tasks/task.service.ts (2)
31-33: LGTM! Clean dependency injection for notification service.The imports and constructor injection for the employee notification service are properly implemented.
Also applies to: 44-44, 64-64
81-81: Consider handling subscription and notification events atomically.While the notification implementation is correct, publishing the subscription and notification events separately could lead to a race condition where a user might receive a notification before being subscribed to the task.
Consider wrapping both events in a transaction or using an event aggregator:
-this._eventBus.publish( - new CreateSubscriptionEvent({ - entity: BaseEntityEnum.Task, - entityId: updatedTask.id, - userId, - type: SubscriptionTypeEnum.ASSIGNMENT, - organizationId, - tenantId - }) -); - -this.employeeNotificationService.publishNotificationEvent( - { - entity: BaseEntityEnum.Task, - entityId: task.id, - type: EmployeeNotificationTypeEnum.ASSIGNMENT, - sentById: user.id, - receiverId: userId, - organizationId, - tenantId - }, - NotificationActionTypeEnum.Assigned, - task.title, - `${user.firstName} ${user.lastName}` -); +// Create an aggregated event that handles both subscription and notification +await this.handleTaskAssignment({ + task: updatedTask, + userId, + user, + organizationId, + tenantId +});Also applies to: 193-206
| /** | ||
| * Stores additional, custom notification preferences in a JSON format. | ||
| * | ||
| * This field allows for flexible storage of any extra preferences not covered by the | ||
| * standard boolean properties above. The underlying database column type adapts based on the | ||
| * database in use: | ||
| * - PostgreSQL: Uses 'jsonb' | ||
| * - MySQL: Uses 'json' | ||
| * - Other databases: Defaults to 'text' | ||
| * | ||
| * This field is required and must not be empty. | ||
| */ | ||
| @ApiProperty({ type: () => Object }) | ||
| @IsNotEmpty() | ||
| @MultiORMColumn({ type: isPostgres() ? 'jsonb' : isMySQL() ? 'json' : 'text' }) | ||
| preferences?: JsonData; | ||
|
|
There was a problem hiding this comment.
Reconcile the “required” property with its optional type.
The docstring indicates "preferences is required," but the property is typed as preferences?: JsonData;. Additionally, @IsNotEmpty() changes the validation semantics. Consider making preferences mandatory in the type definition or adjusting the docs/validation to reflect optional usage.
- @IsNotEmpty()
- preferences?: JsonData;
+ @IsNotEmpty()
+ preferences: JsonData;📝 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.
| /** | |
| * Stores additional, custom notification preferences in a JSON format. | |
| * | |
| * This field allows for flexible storage of any extra preferences not covered by the | |
| * standard boolean properties above. The underlying database column type adapts based on the | |
| * database in use: | |
| * - PostgreSQL: Uses 'jsonb' | |
| * - MySQL: Uses 'json' | |
| * - Other databases: Defaults to 'text' | |
| * | |
| * This field is required and must not be empty. | |
| */ | |
| @ApiProperty({ type: () => Object }) | |
| @IsNotEmpty() | |
| @MultiORMColumn({ type: isPostgres() ? 'jsonb' : isMySQL() ? 'json' : 'text' }) | |
| preferences?: JsonData; | |
| /** | |
| * Stores additional, custom notification preferences in a JSON format. | |
| * | |
| * This field allows for flexible storage of any extra preferences not covered by the | |
| * standard boolean properties above. The underlying database column type adapts based on the | |
| * database in use: | |
| * - PostgreSQL: Uses 'jsonb' | |
| * - MySQL: Uses 'json' | |
| * - Other databases: Defaults to 'text' | |
| * | |
| * This field is required and must not be empty. | |
| */ | |
| @ApiProperty({ type: () => Object }) | |
| @IsNotEmpty() | |
| @MultiORMColumn({ type: isPostgres() ? 'jsonb' : isMySQL() ? 'json' : 'text' }) | |
| preferences: JsonData; |
...e-notification/employee-notification-setting/dto/create-employee-notification-setting.dto.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/employee-notification/events/handlers/employee-notification.handler.ts
Outdated
Show resolved
Hide resolved
...loyee-notification-setting/commands/handlers/employee-notification-setting.create.handler.ts
Outdated
Show resolved
Hide resolved
...loyee-notification-setting/commands/handlers/employee-notification-setting.update.handler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/employee-notification/employee-notification.controller.ts
Outdated
Show resolved
Hide resolved
rahul-rocket
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/core/src/lib/employee-notification/employee-notification.controller.ts (1)
71-80:⚠️ Potential issueFix inconsistent HTTP status and improve return type.
The endpoint has the following issues:
- The
@ApiResponsestatus (CREATED) doesn't match the actual status code (ACCEPTED)- The return type is too generic (
any)Apply this diff to fix the inconsistencies:
@ApiOperation({ summary: 'Mark all notifications as read' }) @ApiResponse({ - status: HttpStatus.CREATED, + status: HttpStatus.ACCEPTED, description: 'The records have been successfully updated.' }) @HttpCode(HttpStatus.ACCEPTED) @Put('/mark-all-read') -async markAllAsRead(): Promise<any> { +async markAllAsRead(): Promise<{ message: string }> { return await this._employeeNotificationService.markAllAsRead(); }
🧹 Nitpick comments (11)
packages/core/src/lib/employee-notification-setting/employee-notification-setting.subscriber.ts (3)
1-5: Consider using a centralized logger instead ofconsole.error.Using a logging service provided by the framework (e.g., NestJS Logger or a custom logger) can ensure consistent formatting, improved traceability, and the ability to manage log levels centrally.
23-37: Avoid redundant async if no asynchronous operations are performed.While marking this method as
asyncis not harmful, it’s also not necessary if you do not perform anyawaitcalls. Consider removing theasynckeyword to improve clarity.- private async serializePreferencesForSQLite(entity: EmployeeNotificationSetting): Promise<void> { + private serializePreferencesForSQLite(entity: EmployeeNotificationSetting): void { try { ... } catch (error) { ... } }
69-83: Consider stricter error handling or rethrowing parse errors.When parsing fails, you reset the data to an empty object—this may mask issues and might make debugging more difficult. If intended, you could at least include an additional warning or insight about the caller context before resetting to
{}.packages/core/src/lib/employee-notification-setting/repository/type-orm-employee-notification-setting.repository.ts (1)
1-13: LGTM! Clean repository implementation.The implementation follows best practices by:
- Using dependency injection
- Properly extending TypeORM Repository
- Correctly initializing the repository with injected dependencies
Consider adding custom query methods.
Consider adding commonly used query methods to encapsulate complex queries and improve reusability.
Example methods to consider:
async findByEmployeeId(employeeId: string): Promise<EmployeeNotificationSetting | null> { return this.findOne({ where: { employeeId } }); } async findByTenantAndEmployee(tenantId: string, employeeId: string): Promise<EmployeeNotificationSetting | null> { return this.findOne({ where: { tenantId, employeeId } }); }packages/core/src/lib/employee-notification-setting/commands/handlers/employee-notification-setting.create.handler.ts (1)
1-22: LGTM! Well-implemented CQRS command handler.The implementation follows best practices by:
- Following CQRS pattern
- Using dependency injection
- Including comprehensive documentation
Consider adding error handling.
The execute method should handle potential errors that might occur during creation.
Example implementation:
public async execute(command: EmployeeNotificationSettingCreateCommand): Promise<IEmployeeNotificationSetting> { const { input } = command; + try { return this.employeeNotificationSettingService.create(input); + } catch (error) { + // Log the error for debugging + console.error('Failed to create employee notification setting:', error); + // Rethrow as a more specific error + throw new Error(`Failed to create employee notification setting: ${error.message}`); + } }packages/core/src/lib/employee-notification-setting/employee-notification-setting.module.ts (2)
14-15: Consider consolidating ORM usage.The module imports both TypeORM and MikroORM for the same entity. While this might be necessary for a transition period, consider documenting the migration strategy and timeline to avoid maintaining dual ORM support indefinitely.
20-20: Consider explicit command handler registration.Using spread operator with
CommandHandlersmakes it harder to track which handlers are actually registered. Consider listing handlers explicitly for better code maintainability and clarity.- providers: [EmployeeNotificationSettingService, TypeOrmEmployeeNotificationSettingRepository, ...CommandHandlers], + providers: [ + EmployeeNotificationSettingService, + TypeOrmEmployeeNotificationSettingRepository, + EmployeeNotificationSettingCreateHandler, + EmployeeNotificationSettingUpdateHandler + ],packages/core/src/lib/employee-notification-setting/commands/handlers/employee-notification-setting.update.handler.ts (2)
21-21: Consider consistent return type.The method returns either
IEmployeeNotificationSetting | UpdateResult. Consider transforming theUpdateResultinto the entity type for a more consistent API.
13-18: Enhance method documentation.The JSDoc could be improved by documenting the return type variants and when each type is returned.
/** * Handles the EmployeeNotificationSettingUpdateCommand to Update an employee notification setting. * * @param command - The command containing the input data for employee notification setting update. - * @returns A promise that resolves to the updated employee notification setting. + * @returns Promise<IEmployeeNotificationSetting> when the entity is returned + * @returns Promise<UpdateResult> when only the update status is returned */packages/core/src/lib/employee-notification/employee-notification.module.ts (1)
22-22: Consider explicit event handler registration.Using spread operator with
EventHandlersmakes it harder to track which handlers are actually registered. Consider listing handlers explicitly for better code maintainability and clarity.- providers: [EmployeeNotificationService, TypeOrmEmployeeNotificationRepository, ...EventHandlers], + providers: [ + EmployeeNotificationService, + TypeOrmEmployeeNotificationRepository, + // List each event handler explicitly + NotificationCreatedHandler, + NotificationUpdatedHandler + ],packages/core/src/lib/employee-notification-setting/employee-notification-setting.service.ts (1)
30-35: Improve error handling specificity.The current error handling catches all errors and returns them as BAD_REQUEST. Consider handling specific error types differently.
- } catch (error) { - throw new HttpException( - `Failed to create the notification setting: ${error.message}`, - HttpStatus.BAD_REQUEST - ); + } catch (error) { + if (error instanceof TypeError || error instanceof Error) { + throw new HttpException( + `Invalid input: ${error.message}`, + HttpStatus.BAD_REQUEST + ); + } + throw new HttpException( + 'Internal server error during notification setting creation', + HttpStatus.INTERNAL_SERVER_ERROR + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/core/src/lib/app/app.module.ts(2 hunks)packages/core/src/lib/core/entities/index.ts(2 hunks)packages/core/src/lib/core/entities/internal.ts(3 hunks)packages/core/src/lib/core/entities/subscribers/index.ts(2 hunks)packages/core/src/lib/employee-notification-setting/commands/employee-notification-setting.create.command.ts(1 hunks)packages/core/src/lib/employee-notification-setting/commands/employee-notification-setting.update.command.ts(1 hunks)packages/core/src/lib/employee-notification-setting/commands/handlers/employee-notification-setting.create.handler.ts(1 hunks)packages/core/src/lib/employee-notification-setting/commands/handlers/employee-notification-setting.update.handler.ts(1 hunks)packages/core/src/lib/employee-notification-setting/commands/handlers/index.ts(1 hunks)packages/core/src/lib/employee-notification-setting/commands/index.ts(1 hunks)packages/core/src/lib/employee-notification-setting/dto/create-employee-notification-setting.dto.ts(1 hunks)packages/core/src/lib/employee-notification-setting/dto/index.ts(1 hunks)packages/core/src/lib/employee-notification-setting/dto/update-employee-notification-setting.dto.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.controller.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.module.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.service.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.subscriber.ts(1 hunks)packages/core/src/lib/employee-notification-setting/repository/mikro-orm-employee-notification-setting.repository.ts(1 hunks)packages/core/src/lib/employee-notification-setting/repository/type-orm-employee-notification-setting.repository.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.controller.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.entity.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.module.ts(1 hunks)packages/core/src/lib/employee-notification/employee-notification.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/lib/employee-notification-setting/dto/index.ts
- packages/core/src/lib/employee-notification-setting/commands/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/lib/core/entities/index.ts
- packages/core/src/lib/app/app.module.ts
- packages/core/src/lib/core/entities/subscribers/index.ts
- packages/core/src/lib/employee-notification/employee-notification.entity.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
🔇 Additional comments (20)
packages/core/src/lib/employee-notification-setting/employee-notification-setting.subscriber.ts (2)
6-13: Class definition andlistenTomethod look good.The extension of
BaseEntityEventSubscriberand the overriddenlistenTomethod correctly focus onEmployeeNotificationSetting. No concerns here.
46-59: Reuse the same helper method for create and update.It’s good practice to keep repetitive tasks (like serializing preferences) in the same helper method to ensure consistency. The current usage of
serializePreferencesForSQLitein bothbeforeEntityCreateandbeforeEntityUpdateis a great example of DRY principles.packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.ts (4)
18-88: Boolean configuration fields are well-defined.Each boolean property has default
true, optional status, and validation with@IsBoolean(). This provides a clean mechanism for controlling notification toggles.
90-105: Excellent use of dynamic column types forpreferences.The conditional
jsonb,json, ortextapproach ensures broad database support. The@IsNotEmpty()decorator enforces consistent usage across various DB engines.
107-128: One-to-one relationship and cascading look appropriate.Cascading insert/update and delete appear to match the intended behavior for the employee link. Ensuring this aligns with the overarching domain model is crucial, but the setup here is standard and valid.
129-140: Add a unique constraint if only one record should exist per employee.If the use case allows only one notification setting per employee, consider adding a unique constraint on
employeeIdto enforce this at the DB level.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Searching for any unique index or unique constraints on employeeId ast-grep --pattern ' @Entity($_) class $_ { $$$ }'Length of output: 54
I'm not seeing any evidence that a unique constraint exists for the
employeeIdcolumn. To get additional clarity, please run further checks—for example, searching for any occurrence of an@Uniquedecorator or a column option usingunique: truein the file. Here’s a shell script you can run:#!/bin/bash # Search for the @Unique decorator in the EmployeeNotificationSetting entity file rg "@Unique" packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.ts # Also search for any unique column options (e.g., "unique: true") rg "unique:" packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.tsIf these searches confirm that there’s no unique constraint on
employeeId, and your business logic requires that each employee have only one notification setting, then you’ll need to add a unique constraint at the database level to enforce this.Action Required:
- Confirm whether a unique constraint is indeed absent by running the provided checks.
- If confirmed and the business rule is one setting per employee, update the entity to enforce uniqueness.
packages/core/src/lib/employee-notification-setting/employee-notification-setting.controller.ts (7)
1-16: Import statements and usage of NestJS modules are correct.You’re importing the right modules for CQRS, Swagger, guards, and pipes. Overall, a consistent and maintainable pattern.
27-36: Controller setup is consistent with CRUD best practices.Applying
@UseGuardsand the@Permissions()decorator at the class level properly secures these endpoints.
58-63: Pagination-based findAll is straightforward.The typed
PaginationParams<EmployeeNotificationSetting>ensures type-safe pagination and filtering. Good usage of generics and returningIPagination<EmployeeNotificationSetting>.
72-90: Parameter validation and direct usage of the service is appropriate.Using
UUIDValidationPipeprevents invalid IDs from reaching the service layer. The overall approach is well-structured.
100-109: CommandBus usage for creation is well-handled.Dispatching
EmployeeNotificationSettingCreateCommandmaintains a clean separation of concerns. The approach aligns with NestJS CQRS principles.
121-142: Update method follows recommended patterns.Properly validating the ID and using a dedicated command to handle update logic ensures consistency with the create operation.
150-164: Deletion approach is consistent, but confirm usage ofDeleteResult.Make sure your front-end or consumers handle the
DeleteResultresponse gracefully, as it differs from simply returning an entity.packages/core/src/lib/employee-notification-setting/repository/mikro-orm-employee-notification-setting.repository.ts (1)
1-4: LGTM! Clean implementation of the repository pattern.The repository is correctly typed with the EmployeeNotificationSetting entity and extends the base repository, providing all necessary CRUD operations.
packages/core/src/lib/employee-notification-setting/commands/handlers/index.ts (1)
1-4: LGTM! Clean implementation of command handlers aggregation.The command handlers are correctly imported and exported as an array, following the command pattern best practices.
packages/core/src/lib/employee-notification-setting/commands/employee-notification-setting.create.command.ts (1)
4-8: LGTM! Clean implementation of the create command.The command class is well-structured with:
- Proper implementation of ICommand interface
- Immutable input parameter
- Clear type identifier following naming conventions
packages/core/src/lib/employee-notification-setting/commands/employee-notification-setting.update.command.ts (1)
4-8: LGTM! Clean implementation of the update command.The command class is well-structured with:
- Proper implementation of ICommand interface
- Immutable id and input parameters
- Clear type identifier following naming conventions
packages/core/src/lib/employee-notification-setting/dto/update-employee-notification-setting.dto.ts (1)
1-10: LGTM! Well-structured DTO implementation.The implementation follows best practices by:
- Using
PartialTypeto make all properties optional for updates- Implementing the contract interface for type safety
- Including clear documentation
packages/core/src/lib/employee-notification-setting/dto/create-employee-notification-setting.dto.ts (1)
1-14: LGTM! Well-structured DTO composition.The implementation follows best practices by:
- Using
IntersectionTypeto combine DTOs- Using
OmitTypeto exclude unnecessary properties- Implementing the contract interface for type safety
- Including clear documentation
packages/core/src/lib/core/entities/internal.ts (1)
160-161: LGTM!The new exports for employee notification entities and subscribers follow the established pattern and are correctly placed in their respective sections.
Also applies to: 177-178
packages/core/src/lib/employee-notification-setting/employee-notification-setting.service.ts
Show resolved
Hide resolved
packages/core/src/lib/employee-notification/employee-notification.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/lib/employee-notification/employee-notification.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/lib/database/migrations/1739273982991-CreateEmployeeNotificationSettingTable.ts (1)
13-30: Consider adding a console log in thedownmethod for consistency.
You log migration progress in theupmethod; doing the same indownensures consistent debug information.public async down(queryRunner: QueryRunner): Promise<void> { + console.log(chalk.yellow(this.name + ' start reverting!')); switch (queryRunner.connection.options.type) { // ...packages/core/src/lib/database/migrations/1739269734150-CreateEmployeeNotificationTable.ts (1)
13-30: Console log usage inupis helpful.
Consider echoing a similar log indownto maintain a consistent debug trail.public async down(queryRunner: QueryRunner): Promise<void> { + console.log(chalk.yellow(this.name + ' start reverting!')); switch (queryRunner.connection.options.type) { // ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/lib/database/migrations/1739269734150-CreateEmployeeNotificationTable.ts(1 hunks)packages/core/src/lib/database/migrations/1739273982991-CreateEmployeeNotificationSettingTable.ts(1 hunks)packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/lib/employee-notification-setting/employee-notification-setting.entity.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (11)
packages/core/src/lib/database/migrations/1739273982991-CreateEmployeeNotificationSettingTable.ts (6)
5-6: Class naming is consistent and clear.
The class name aligns with the file name and remains descriptive for future reviewers.
59-87: Postgres column definitions look correct.
The use ofjsonbis appropriate for storing structured data in PostgreSQL. Indices and constraints appear consistent with typical usage.
117-166: SQLite up migration approach is valid.
Dropping and recreating indexes is a common workaround for SQLite’s limitations on altering tables. Well-structured.
173-210: SQLite down migration flow is consistent.
Renaming the original table to a temporary table before recreating is a standard pattern for SQLite.
217-230: MySQL up migration is properly defined.
All columns, constraints, and default values match the intended schema.
238-254: MySQL down migration thoroughly cleans up.
Dropping foreign keys and indexes in the correct order avoids potential errors in MySQL.packages/core/src/lib/database/migrations/1739269734150-CreateEmployeeNotificationTable.ts (5)
5-6: Class name and structure align with TypeORM conventions.
This descriptive naming makes it easy to identify and run the migration.
59-98: Postgres definitions match advanced features.
Use ofuuidas primary key andjsonbor textual columns aligns well with typical Postgres usage.
127-194: SQLite migration logic is well executed.
Sequentially dropping indexes and handling a temporary table is necessary for schema changes in SQLite.
259-275: MySQL migration structure is aligned with InnoDB defaults.
Foreign keys and indexes look well-defined, ensuring proper relational integrity.
282-305: MySQLdownmethod completes cleanup gracefully.
Dropping indexes and foreign keys in the correct order helps avoid reference errors.
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