[Feat] Subscribe user on assignment / Un-assignment #8574
Conversation
WalkthroughThe pull request introduces significant enhancements to subscription management across various services within the application. Key updates include the integration of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (1)
packages/core/src/tasks/commands/handlers/task-create.handler.ts (1)
41-41: Consider adding type safety for members arrayThe destructured members array lacks type information.
-const { organizationId, mentionUserIds = [], members = [], ...data } = input; +const { organizationId, mentionUserIds = [], members = [] as Array<{ id: string }>, ...data } = input;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/core/src/organization-project/organization-project.service.ts(7 hunks)packages/core/src/organization-sprint/organization-sprint.service.ts(6 hunks)packages/core/src/organization-team-employee/organization-team-employee.module.ts(2 hunks)packages/core/src/organization-team-employee/organization-team-employee.service.ts(3 hunks)packages/core/src/organization-team/organization-team.service.ts(6 hunks)packages/core/src/subscription/subscription.module.ts(3 hunks)packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts(7 hunks)packages/core/src/tasks/commands/handlers/task-create.handler.ts(4 hunks)packages/core/src/tasks/task.service.ts(5 hunks)
🔇 Additional comments (8)
packages/core/src/organization-team/organization-team.service.ts (4)
1-1: Import EventBus from '@nestjs/cqrs'
The EventBus is correctly imported to enable event-driven functionality within the service.
23-24: Import ID and SubscriptionTypeEnum from '@gauzy/contracts'
The imports of ID and SubscriptionTypeEnum are appropriate for usage in the service logic.
41-41: Import CreateSubscriptionEvent
The CreateSubscriptionEvent is properly imported, enabling the service to publish subscription events.
51-51: Inject EventBus in the constructor
Injecting the EventBus into the constructor allows the service to publish events as part of its operations.
packages/core/src/tasks/task.service.ts (1)
158-174: 🛠️ Refactor suggestion
Avoid empty catch blocks: Log or handle errors when subscribing new task members
An empty catch block could conceal errors during the subscription of new task members. Logging the error can aid in debugging.
Apply this diff:
try {
// Existing code...
} catch (error) {
-}
+ console.error('Error subscribing new members to the task:', error);
}Likely invalid or redundant comment.
packages/core/src/subscription/subscription.module.ts (1)
13-13: Verify the necessity of global module scope
Making the module global is a significant architectural decision that could lead to tight coupling. Consider if explicit imports would be more maintainable.
✅ Verification successful
Let me gather more context about the usage patterns to make a more informed decision.
@global() decorator is justified for the SubscriptionModule
The global scope is appropriate here because:
- The SubscriptionService is used across multiple core domains (tasks, mentions, organization teams, projects, sprints)
- All usages follow a consistent pattern for subscription management (primarily deletion operations)
- The service is a cross-cutting concern that manages subscriptions for various entity types through a unified interface
- Direct imports are limited (only in app.module.ts and mention.module.ts), showing controlled access
The global scope reduces boilerplate while maintaining a clean architecture for this cross-cutting functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how many files are importing SubscriptionModule directly
rg -l "import.*SubscriptionModule"
# Check usage of exported services/repositories
rg -l "import.*SubscriptionService|TypeOrmSubscriptionRepository"
Length of output: 842
Script:
#!/bin/bash
# Check if SubscriptionService is injected as a dependency in other services
ast-grep --pattern 'constructor($$$, private readonly subscriptionService: SubscriptionService, $$$)'
# Check if there are any circular dependencies
rg -l "import.*from.*subscription.*" packages/core/src/mention/
rg -l "import.*from.*subscription.*" packages/core/src/tasks/
rg -l "import.*from.*subscription.*" packages/core/src/organization-team-employee/
rg -l "import.*from.*subscription.*" packages/core/src/organization-project/
rg -l "import.*from.*subscription.*" packages/core/src/organization-sprint/
# Check the nature of usage in services
rg "subscriptionService\." -A 2 packages/core/src/organization-team-employee/
rg "subscriptionService\." -A 2 packages/core/src/tasks/
rg "subscriptionService\." -A 2 packages/core/src/organization-project/
rg "subscriptionService\." -A 2 packages/core/src/organization-sprint/
Length of output: 2923
packages/core/src/organization-team-employee/organization-team-employee.module.ts (1)
1-1: LGTM! Verify CqrsModule usage
The addition of CqrsModule is appropriate for event handling.
Also applies to: 17-17
✅ Verification successful
CqrsModule import is correctly used and implemented
The verification confirms that CqrsModule is properly used in this module:
- The service injects and uses
EventBusfrom '@nestjs/cqrs' to publish events - CqrsModule is correctly imported and included in the module's imports array
- The implementation shows actual usage of CQRS patterns in the service layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify CqrsModule is being used in the service
rg -A 5 "EventBus.*CqrsEventBus|CommandHandler|EventHandler" "packages/core/src/organization-team-employee/"
Length of output: 108
Script:
#!/bin/bash
# Let's check for any CQRS-related patterns in the directory
rg -i "cqrs|eventbus|commandbus|@command|@event|@query" "packages/core/src/organization-team-employee/"
Length of output: 920
Script:
#!/bin/bash
# Let's also check imports of CQRS-related components
ast-grep --pattern 'import { $$$ } from "@nestjs/cqrs"'
Length of output: 186
Script:
#!/bin/bash
# Check the module file for CQRS usage
cat "packages/core/src/organization-team-employee/organization-team-employee.module.ts"
Length of output: 1244
packages/core/src/tasks/commands/handlers/task-create.handler.ts (1)
26-26: LGTM! Dependency injection is properly implemented
The EmployeeService is correctly injected for handling employee-related operations.
packages/core/src/organization-team/organization-team.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/organization-team-employee/organization-team-employee.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/organization-project/organization-project.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/organization-project/organization-project.service.ts
Outdated
Show resolved
Hide resolved
packages/core/src/tasks/commands/handlers/task-create.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/core/src/tasks/task.service.ts (1)
137-153: Simplify 'Promise.all' by removing unnecessary 'async' and 'await'The
asynckeyword andawaitinside themapfunction are unnecessary sincethis._subscriptionService.deletereturns a promise. Removing them simplifies the code without affecting functionality.Apply this diff to simplify the code:
- await Promise.all( - removedMembers.map( - async (member) => - await this._subscriptionService.delete({ - entity: BaseEntityEnum.Task, - entityId: updatedTask.id, - userId: member.userId, - type: SubscriptionTypeEnum.ASSIGNMENT - }) - ) - ); + await Promise.all( + removedMembers.map((member) => + this._subscriptionService.delete({ + entity: BaseEntityEnum.Task, + entityId: updatedTask.id, + userId: member.userId, + type: SubscriptionTypeEnum.ASSIGNMENT + }) + ) + );packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (1)
Line range hint
225-289: Avoid redundant 'async' and 'await' inside 'Promise.all'In the
updateTaskmethod, theasynckeyword andawaitinside thePromise.allare unnecessary when callingthis._subscriptionService.deleteandthis._eventBus.publish.Apply this diff to simplify the code:
- if (removedMembers.length > 0) { - try { - await Promise.all( - removedMembers.map( - async (member) => - await this._subscriptionService.delete({ - entity: BaseEntityEnum.Task, - entityId: updatedTask.id, - userId: member.userId, - type: SubscriptionTypeEnum.ASSIGNMENT - }) - ) - ); - } catch (error) { - console.error('Error subscribing new members to the task:', error); - } - } + if (removedMembers.length > 0) { + try { + await Promise.all( + removedMembers.map((member) => + this._subscriptionService.delete({ + entity: BaseEntityEnum.Task, + entityId: updatedTask.id, + userId: member.userId, + type: SubscriptionTypeEnum.ASSIGNMENT + }) + ) + ); + } catch (error) { + console.error('Error unsubscribing members from the task:', error); + } + } - if (newMembers.length) { - try { - await Promise.all( - newMembers.map(({ userId }) => - this._eventBus.publish( - new CreateSubscriptionEvent({ - entity: BaseEntityEnum.Task, - entityId: updatedTask.id, - userId, - type: SubscriptionTypeEnum.ASSIGNMENT, - organizationId, - tenantId - }) - ) - ) - ); - } catch (error) { - console.error('Error subscribing new members to the task:', error); - } - } + if (newMembers.length) { + try { + newMembers.forEach(({ userId }) => + this._eventBus.publish( + new CreateSubscriptionEvent({ + entity: BaseEntityEnum.Task, + entityId: updatedTask.id, + userId, + type: SubscriptionTypeEnum.ASSIGNMENT, + organizationId, + tenantId + }) + ) + ); + } catch (error) { + console.error('Error subscribing new members to the task:', error); + } + }packages/core/src/organization-project/organization-project.service.ts (1)
302-316: Simplify 'Promise.all' usage in unsubscription logicIn the unsubscription logic, you can remove the unnecessary
asyncandawaitinsidePromise.all. Additionally, handle errors appropriately.Apply this diff to simplify the code:
- try { - await Promise.all( - removedMembers.map( - async (member) => - await this._subscriptionService.delete({ - entity: BaseEntityEnum.OrganizationProject, - entityId: organizationProjectId, - userId: member.employee.userId, - type: SubscriptionTypeEnum.ASSIGNMENT - }) - ) - ); - } catch (error) { - console.error('Error unsubscribing members from the project:', error); - } + try { + await Promise.all( + removedMembers.map((member) => + this._subscriptionService.delete({ + entity: BaseEntityEnum.OrganizationProject, + entityId: organizationProjectId, + userId: member.employee.userId, + type: SubscriptionTypeEnum.ASSIGNMENT + }) + ) + ); + } catch (error) { + console.error('Error unsubscribing members from the project:', error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/core/src/organization-project/organization-project.service.ts(7 hunks)packages/core/src/organization-team-employee/organization-team-employee.service.ts(3 hunks)packages/core/src/organization-team/organization-team.service.ts(6 hunks)packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts(7 hunks)packages/core/src/tasks/commands/handlers/task-create.handler.ts(4 hunks)packages/core/src/tasks/task.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/organization-team-employee/organization-team-employee.service.ts
🔇 Additional comments (7)
packages/core/src/tasks/task.service.ts (1)
156-175: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
The EventBus.publish() method is synchronous and doesn't return a promise. Using await Promise.all with it is unnecessary and may cause confusion.
Apply this diff to correct the issue:
- if (newMembers.length) {
- try {
- await Promise.all(
- newMembers.map(({ userId }) =>
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.Task,
- entityId: updatedTask.id,
- userId,
- type: SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
- } catch (error) {
- console.error('Error publishing CreateSubscriptionEvent:', error);
- }
- }
+ if (newMembers.length) {
+ try {
+ newMembers.forEach(({ userId }) =>
+ this._eventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.Task,
+ entityId: updatedTask.id,
+ userId,
+ type: SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error publishing CreateSubscriptionEvent:', error);
+ }
+ }packages/core/src/tasks/commands/handlers/task-create.handler.ts (1)
117-130: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
The _cqrsEventBus.publish() method is synchronous and doesn't return a promise. Using await Promise.all with it is unnecessary and may lead to confusion.
Apply this diff to correct the issue:
- await Promise.all(
- employees.map(({ userId }) =>
- this._cqrsEventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.Task,
- entityId: task.id,
- userId,
- type: SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
+ employees.forEach(({ userId }) =>
+ this._cqrsEventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.Task,
+ entityId: task.id,
+ userId,
+ type: SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );packages/core/src/tasks/commands/handlers/automation-task.sync.handler.ts (2)
155-166: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
The EventBus.publish() method is synchronous and doesn't return a promise. Using await with Promise.all is unnecessary and may lead to confusion.
Apply this diff to correct the issue:
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.Task,
- entityId: createdTask.id,
- userId: createdTask.creatorId,
- type: SubscriptionTypeEnum.CREATED_ENTITY,
- organizationId,
- tenantId
- })
- );(Note: Since the publish method is synchronous, the existing code is correct. No need to wrap it in await or Promise.all.)
168-195: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
In the block where you subscribe assignees to the task, this._eventBus.publish() is synchronous. Using await Promise.all is unnecessary.
Apply this diff to correct the issue:
- if (entity.members.length > 0) {
- try {
- const employeeIds = entity.members.map(({ id }) => id);
- const employees = await this._employeeService.findActiveEmployeesByEmployeeIds(
- employeeIds,
- organizationId,
- tenantId
- );
- await Promise.all(
- employees.map(({ userId }) =>
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.Task,
- entityId: createdTask.id,
- userId,
- type: SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
- } catch (error) {
- console.error('Error subscribing new members to the task:', error);
- }
- }
+ if (entity.members.length > 0) {
+ try {
+ const employeeIds = entity.members.map(({ id }) => id);
+ const employees = await this._employeeService.findActiveEmployeesByEmployeeIds(
+ employeeIds,
+ organizationId,
+ tenantId
+ );
+ employees.forEach(({ userId }) =>
+ this._eventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.Task,
+ entityId: createdTask.id,
+ userId,
+ type: SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error subscribing new members to the task:', error);
+ }
+ }packages/core/src/organization-project/organization-project.service.ts (2)
131-153: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
The EventBus.publish() method is synchronous. Using await Promise.all with it is unnecessary and may cause confusion.
Apply this diff to correct the issue:
- try {
- await Promise.all(
- employees.map(({ id, userId }) =>
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.OrganizationProject,
- entityId: project.id,
- userId,
- type:
- id === employeeId
- ? SubscriptionTypeEnum.CREATED_ENTITY
- : SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
- } catch (error) {
- console.error('Error subscribing creators and assignees to the project:', error);
- }
+ try {
+ employees.forEach(({ id, userId }) =>
+ this._eventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.OrganizationProject,
+ entityId: project.id,
+ userId,
+ type:
+ id === employeeId
+ ? SubscriptionTypeEnum.CREATED_ENTITY
+ : SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error subscribing creators and assignees to the project:', error);
+ }349-366: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
When publishing events, this._eventBus.publish() is synchronous. There's no need to use await Promise.all.
Apply this diff to correct the issue:
- try {
- await Promise.all(
- newMembers.map(({ userId }) =>
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.OrganizationProject,
- entityId: organizationProjectId,
- userId,
- type: SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
- } catch (error) {}
+ try {
+ newMembers.forEach(({ userId }) =>
+ this._eventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.OrganizationProject,
+ entityId: organizationProjectId,
+ userId,
+ type: SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error subscribing new members to the project:', error);
+ }packages/core/src/organization-team/organization-team.service.ts (1)
266-287: 🛠️ Refactor suggestion
Avoid using 'await Promise.all' with synchronous 'publish' method
The EventBus.publish() method is synchronous. Using await Promise.all with it is unnecessary.
Apply this diff to correct the issue:
- try {
- await Promise.all(
- employees.map(({ id, userId }) =>
- this._eventBus.publish(
- new CreateSubscriptionEvent({
- entity: BaseEntityEnum.OrganizationTeam,
- entityId: organizationTeam.id,
- userId,
- type:
- id === employeeId
- ? SubscriptionTypeEnum.CREATED_ENTITY
- : SubscriptionTypeEnum.ASSIGNMENT,
- organizationId,
- tenantId
- })
- )
- )
- );
- } catch (error) {
- console.error('Error publishing CreateSubscriptionEvent:', error);
- }
+ try {
+ employees.forEach(({ id, userId }) =>
+ this._eventBus.publish(
+ new CreateSubscriptionEvent({
+ entity: BaseEntityEnum.OrganizationTeam,
+ entityId: organizationTeam.id,
+ userId,
+ type:
+ id === employeeId
+ ? SubscriptionTypeEnum.CREATED_ENTITY
+ : SubscriptionTypeEnum.ASSIGNMENT,
+ organizationId,
+ tenantId
+ })
+ )
+ );
+ } catch (error) {
+ console.error('Error publishing CreateSubscriptionEvent:', error);
+ }
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ae3ea5e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation