Added roleSetCacheModule and added cache invalidation on invitation / application removal #4955
Conversation
… application removal
WalkthroughThis pull request integrates a new caching module and service for role set functionality into multiple domains. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ApplicationService
participant Repository
participant RoleSetCacheService
Client->>ApplicationService: deleteApplication(deleteData)
ApplicationService->>Repository: Delete application record
Repository-->>ApplicationService: Deletion confirmation
ApplicationService->>RoleSetCacheService: deleteOpenApplicationFromCache(userId, roleSetId)
RoleSetCacheService-->>ApplicationService: Cache update confirmation
ApplicationService-->>Client: Return deletion result
sequenceDiagram
participant Client
participant InvitationService
participant Repository
participant RoleSetCacheService
Client->>InvitationService: deleteInvitation(deleteData)
InvitationService->>Repository: Delete invitation record
Repository-->>InvitationService: Deletion confirmation
InvitationService->>RoleSetCacheService: deleteOpenInvitationFromCache(invitedContributorID, roleSetId)
RoleSetCacheService-->>InvitationService: Cache update confirmation
InvitationService-->>Client: Return deletion result
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/domain/access/application/application.service.ts (1)
81-86: Add error handling for cache operations.Cache operations could fail independently of the main operation. Consider wrapping the cache invalidation in a try-catch block to ensure the application deletion succeeds even if cache invalidation fails.
if (application.user?.id && application.roleSet?.id) { + try { await this.roleSetCacheService.deleteOpenApplicationFromCache( application.user?.id, application.roleSet?.id ); + } catch (error) { + this.logger.warn( + `Failed to invalidate cache for application ${applicationID}`, + { error, context: LogContext.COMMUNITY } + ); + } }src/domain/access/role-set/role.set.resolver.mutations.ts (1)
866-871: Extract duplicated cache invalidation logic into a private method.The cache invalidation logic is duplicated between
eventOnApplicationandeventOnInvitationmethods.Consider extracting the logic into a private method:
+ private async invalidateOpenApplicationCache( + userID: string | undefined, + roleSet: IRoleSet | undefined + ): Promise<void> { + if (userID && roleSet) { + await this.roleSetCacheService.deleteOpenApplicationFromCache( + userID, + roleSet.id + ); + } + } async eventOnApplication( @Args('eventData') eventData: ApplicationEventInput, @CurrentUser() agentInfo: AgentInfo ): Promise<IApplication> { // ... existing code ... - if (agentInfo.userID && application.roleSet) { - await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - application.roleSet?.id - ); - } + await this.invalidateOpenApplicationCache(agentInfo.userID, application.roleSet); return await this.applicationService.getApplicationOrFail( eventData.applicationID ); } async eventOnInvitation( @Args('eventData') eventData: InvitationEventInput, @CurrentUser() agentInfo: AgentInfo ): Promise<IInvitation> { // ... existing code ... - if (agentInfo.userID && invitation.roleSet) { - await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - invitation.roleSet?.id - ); - } + await this.invalidateOpenApplicationCache(agentInfo.userID, invitation.roleSet); return await this.invitationService.getInvitationOrFail(invitation.id); }Also applies to: 937-942
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/domain/access/application/application.module.ts(2 hunks)src/domain/access/application/application.service.ts(3 hunks)src/domain/access/invitation/invitation.module.ts(2 hunks)src/domain/access/invitation/invitation.service.ts(3 hunks)src/domain/access/role-set/role.set.module.ts(2 hunks)src/domain/access/role-set/role.set.resolver.mutations.ts(7 hunks)src/domain/access/role-set/role.set.service.cache.module.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/domain/access/role-set/role.set.service.cache.module.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/invitation/invitation.module.tssrc/domain/access/application/application.module.tssrc/domain/access/application/application.service.tssrc/domain/access/invitation/invitation.service.tssrc/domain/access/role-set/role.set.module.tssrc/domain/access/role-set/role.set.resolver.mutations.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (10)
src/domain/access/application/application.module.ts (1)
15-15: LGTM! Clean integration of the RoleSetCacheModule.The module integration follows NestJS best practices and properly sets up the caching functionality.
Also applies to: 25-25
src/domain/access/invitation/invitation.module.ts (1)
17-17: LGTM! Clean integration of the RoleSetCacheModule.The module integration follows NestJS best practices and properly sets up the caching functionality.
Also applies to: 29-29
src/domain/access/role-set/role.set.module.ts (1)
37-37: LGTM! Clean integration of the RoleSetCacheModule.The module integration follows NestJS best practices and properly sets up the caching functionality.
Also applies to: 64-64
src/domain/access/application/application.service.ts (1)
26-26: LGTM! Clean dependency injection of RoleSetCacheService.The service injection follows NestJS best practices.
Also applies to: 38-38
src/domain/access/invitation/invitation.service.ts (2)
27-27: LGTM!The import statement is correctly placed and follows the project's import conventions.
78-83: LGTM!The cache invalidation logic is correctly placed after the database operation and properly checks for required properties before execution.
src/domain/access/role-set/role.set.resolver.mutations.ts (4)
72-72: LGTM!The import statement is correctly placed and follows the project's import conventions.
98-98: LGTM!The dependency injection follows NestJS best practices.
467-470: LGTM!The cache invalidation is correctly placed at the start of the method to ensure cache consistency.
839-844: LGTM!The roleSet relation is correctly added to support the cache invalidation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/domain/access/application/application.service.ts(4 hunks)src/domain/access/invitation/invitation.service.ts(4 hunks)src/domain/access/role-set/role.set.resolver.mutations.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/application/application.service.tssrc/domain/access/invitation/invitation.service.tssrc/domain/access/role-set/role.set.resolver.mutations.ts
🔇 Additional comments (6)
src/domain/access/application/application.service.ts (2)
26-26: Clean dependency injection implementation!The
RoleSetCacheServiceis properly injected following NestJS best practices.Also applies to: 38-38
83-88: Well-implemented cache invalidation with null safety!The cache invalidation is properly placed after the database deletion and uses optional chaining (
?.) to safely handle potential null values foruserandroleSet.src/domain/access/invitation/invitation.service.ts (2)
27-27: Clean dependency injection implementation!The
RoleSetCacheServiceis properly injected following NestJS best practices.Also applies to: 39-39
82-87: Well-implemented cache invalidation with proper null checks!The cache invalidation is properly placed after the database deletion and uses appropriate null checks for
createdByandroleSet.src/domain/access/role-set/role.set.resolver.mutations.ts (2)
72-72: Clean dependency injection implementation!The
RoleSetCacheServiceis properly injected following NestJS best practices.Also applies to: 98-98
870-875: Well-implemented cache invalidation after event processing!The cache invalidation is properly placed after the event processing and uses appropriate null checks for
agentInfo.userIDandroleSet.Also applies to: 941-946
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/domain/access/role-set/role.set.resolver.mutations.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/role-set/role.set.resolver.mutations.ts
🔇 Additional comments (6)
src/domain/access/role-set/role.set.resolver.mutations.ts (6)
72-72: LGTM! Dependency injection is properly implemented.The RoleSetCacheService is correctly injected following NestJS best practices.
Also applies to: 98-98
467-470: Consider moving cache invalidation after successful creation.The cache invalidation is currently performed before creating the application. If the creation fails, the cache would be unnecessarily invalidated.
Apply this pattern:
- await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - applicationData.roleSetID - ); const roleSet = await this.roleSetService.getRoleSetOrFail( applicationData.roleSetID, { relations: { parentRoleSet: true, }, } ); // ... rest of the creation logic ... + await this.roleSetCacheService.deleteOpenApplicationFromCache( + agentInfo.userID, + applicationData.roleSetID + );
843-849: LGTM! Cache management is properly implemented.The changes include:
- Proper relations loading for roleSet
- Appropriate cache invalidation based on application state
- Correct member status caching
Also applies to: 870-891
543-546: Consider moving cache invalidation after successful creation.The cache invalidation is currently performed before creating the invitation. If the creation fails, the cache would be unnecessarily invalidated.
Apply this pattern:
- await this.roleSetCacheService.deleteOpenInvitationFromCache( - agentInfo.userID, - invitationData.roleSetID - ); const roleSet = await this.roleSetService.getRoleSetOrFail( invitationData.roleSetID, { relations: { parentRoleSet: { authorization: true, }, license: { entitlements: true, }, }, } ); // ... rest of the creation logic ... + await this.roleSetCacheService.deleteOpenInvitationFromCache( + agentInfo.userID, + invitationData.roleSetID + );
908-913: LGTM! Relations are properly included.The roleSet relation is correctly included in the query.
870-891: LGTM! Cache management follows consistent patterns.The implementation demonstrates:
- Consistent cache invalidation patterns
- Proper null checks and error handling
- Clear separation of concerns
Also applies to: 957-978
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts (1)
82-96:❓ Verification inconclusive
Inconsistent authorization pattern across resolver methods
The
spacemethod now deviates from the consistent authorization pattern used in other methods (innovationHub,innovationPack,user,virtualContributor,template). This inconsistency makes the code harder to maintain and could lead to security gaps.Consider one of these approaches:
- If authorization is truly not needed for spaces, document why and remove it from other similar methods
- If this is temporary, implement a consistent interim solution across all methods
- Create an interface or base class that enforces consistent authorization patterns
Let's check if
lookupByIDtruly has no authorization:
🏁 Script executed:
#!/bin/bash # Search for lookupByID implementation to verify authorization pattern rg -A 10 "lookupByID.*space"Length of output: 30
Action Required: Manual Verification for Authorization Consistency
We've reviewed the changes in the
spaceresolver insrc/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts. The authorization check (usingauthorizationService.grantAccessOrFail) is commented out here, which deviates from the pattern seen in other resolver methods (e.g.,innovationHub,innovationPack,user,virtualContributor,template).Our initial attempt to verify the authorization pattern in the corresponding
lookupByIDimplementation (using the shell script search for"lookupByID.*space") returned no results. This outcome makes it unclear whetherlookupByIDactually implements authorization or whether it similarly omits it.Next Steps:
- Manual Verification: Please manually verify if
lookupByID(and any similar resolver) either enforces the authorization logic or intentionally omits it.- Consistency Decision: If authorization is required for the
spacemethod, update it (and any related resolvers) to enforce a consistent authorization pattern. Alternatively, if the omission is intentional, document the reasoning and adjust the other methods for clarity.- Refactoring Consideration: To avoid such inconsistencies, consider refactoring by creating a common interface or base class that encapsulates the authorization logic for all resolver methods.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts
src/services/api/lookup-by-name/lookup.by.name.resolver.fields.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/domain/access/role-set/role.set.resolver.mutations.ts(9 hunks)src/domain/access/role-set/role.set.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/role-set/role.set.service.tssrc/domain/access/role-set/role.set.resolver.mutations.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (8)
src/domain/access/role-set/role.set.resolver.mutations.ts (6)
72-72: LGTM! Proper dependency injection.The RoleSetCacheService is correctly injected following NestJS dependency injection patterns.
Also applies to: 98-98
467-470: Consider moving cache invalidation after successful creation.The cache invalidation is currently performed before creating the application. If the creation fails, the cache would be unnecessarily invalidated.
Apply this pattern:
- await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - applicationData.roleSetID - ); const roleSet = await this.roleSetService.getRoleSetOrFail( applicationData.roleSetID, { relations: { parentRoleSet: true, }, } ); // ... rest of the creation logic ... + await this.roleSetCacheService.deleteOpenApplicationFromCache( + agentInfo.userID, + applicationData.roleSetID + );
543-546: Consider moving cache invalidation after successful creation.The cache invalidation is currently performed before creating the invitation. If the creation fails, the cache would be unnecessarily invalidated.
Apply this pattern:
- await this.roleSetCacheService.deleteOpenInvitationFromCache( - agentInfo.userID, - invitationData.roleSetID - ); const roleSet = await this.roleSetService.getRoleSetOrFail( invitationData.roleSetID, { relations: { parentRoleSet: { authorization: true, }, license: { entitlements: true, }, }, } ); // ... rest of the creation logic ... + await this.roleSetCacheService.deleteOpenInvitationFromCache( + agentInfo.userID, + invitationData.roleSetID + );
843-848: LGTM! Proper cache management after state changes.The cache operations are correctly placed after application state changes, and the necessary relations are loaded.
Also applies to: 870-887
904-909: LGTM! Proper cache management after state changes.The cache operations are correctly placed after invitation state changes, and the necessary relations are loaded.
Also applies to: 959-979
982-992: Remove duplicate cache operations.These cache operations are redundant as they duplicate the functionality already implemented in lines 959-979.
Remove the duplicate block:
- if (agentInfo.userID && invitation.roleSet) { - await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - invitation.roleSet?.id - ); - await this.roleSetCacheService.setAgentIsMemberCache( - agentInfo.agentID, - invitation.roleSet?.id, - true - ); - }src/domain/access/role-set/role.set.service.ts (2)
829-832: LGTM! Proper cache invalidation with error handling.The cache operation is correctly placed after successful role assignment and is properly protected within a try-catch block.
1900-1903: LGTM! Proper cache invalidation after successful approval.The cache operation is correctly placed after successful role assignment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/domain/access/application/application.service.ts (2)
26-26: Use alias import path for consistency.Consider using an alias import path to match the project's import style:
-import { RoleSetCacheService } from '../role-set/role.set.service.cache'; +import { RoleSetCacheService } from '@domain/access/role-set/role.set.service.cache';
83-88: Remove redundant optional chaining operators.The if condition already ensures that both
application.user?.idandapplication.roleSet?.idexist, making the optional chaining operators in the method call redundant.if (application.user?.id && application.roleSet?.id) { await this.roleSetCacheService.deleteOpenApplicationFromCache( - application.user?.id, - application.roleSet?.id + application.user.id, + application.roleSet.id ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/domain/access/application/application.service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/application/application.service.ts
🔇 Additional comments (2)
src/domain/access/application/application.service.ts (2)
38-38: LGTM! Proper dependency injection.The RoleSetCacheService is correctly injected following NestJS dependency injection pattern.
65-67: LGTM! Explicit relation loading.Good practice to explicitly load the required relations for cache invalidation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/domain/access/invitation/invitation.service.ts (1)
82-87: Add error handling for cache invalidation.While the cache invalidation is properly guarded with null checks, a failure in
deleteOpenInvitationFromCachecould go unnoticed. Consider wrapping it in a try-catch block to log any cache-related issues without failing the deletion operation.if (invitation.invitedContributorID && invitation.roleSet) { + try { await this.roleSetCacheService.deleteOpenInvitationFromCache( invitation.invitedContributorID, invitation.roleSet.id ); + } catch (error) { + this.logger.warn( + `Failed to invalidate cache for invitation ${invitationID}`, + { + error, + context: LogContext.COMMUNITY, + } + ); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/domain/access/invitation/invitation.service.ts(4 hunks)src/domain/access/role-set/role.set.resolver.mutations.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/domain/access/role-set/role.set.resolver.mutations.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/invitation/invitation.service.ts
🔇 Additional comments (3)
src/domain/access/invitation/invitation.service.ts (3)
27-27: LGTM! Clean dependency injection setup.The RoleSetCacheService is properly imported and injected following NestJS best practices.
Also applies to: 39-39
92-102: LGTM! Well-structured query options handling.The method signature update elegantly supports flexible relation loading while maintaining backward compatibility and proper query condition merging.
67-71: LGTM! Proper relation loading setup.The roleSet relation is correctly loaded to support cache invalidation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/domain/access/role-set/role.set.resolver.mutations.ts (2)
862-879: Refactor: Consolidate duplicate cache operations.The cache operations and null checks are duplicated in multiple blocks. Consider extracting them into a private method to improve maintainability and reduce duplication.
+ private async updateApplicationCache( + agentInfo: AgentInfo, + roleSetId: string, + isMember: boolean + ): Promise<void> { + if (!agentInfo.userID) return; + + await this.roleSetCacheService.deleteOpenApplicationFromCache( + agentInfo.userID, + roleSetId + ); + await this.roleSetCacheService.setAgentIsMemberCache( + agentInfo.agentID, + roleSetId, + isMember + ); + } - if (agentInfo.userID && application.roleSet) { - applicationState = this.lifecycleService.getState( - application.lifecycle, - this.roleSetServiceLifecycleApplication.getApplicationMachine() - ); - const isMember = applicationState === ApplicationLifecycleState.APPROVED; - if (agentInfo.userID && application.roleSet) { - await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - application.roleSet?.id - ); - await this.roleSetCacheService.setAgentIsMemberCache( - agentInfo.agentID, - application.roleSet?.id, - isMember - ); - } - } + if (application.roleSet) { + applicationState = this.lifecycleService.getState( + application.lifecycle, + this.roleSetServiceLifecycleApplication.getApplicationMachine() + ); + const isMember = applicationState === ApplicationLifecycleState.APPROVED; + await this.updateApplicationCache(agentInfo, application.roleSet.id, isMember); + }
951-984: Refactor: Remove duplicate cache operations.The cache operations are duplicated across multiple blocks. Consider extracting them into a private method and removing the duplicate block at lines 974-984.
+ private async updateInvitationCache( + agentInfo: AgentInfo, + roleSetId: string, + isMember: boolean, + isOpenInvitation: boolean + ): Promise<void> { + if (!agentInfo.userID) return; + + if (!isOpenInvitation) { + await this.roleSetCacheService.deleteOpenInvitationFromCache( + agentInfo.userID, + roleSetId + ); + } + await this.roleSetCacheService.setAgentIsMemberCache( + agentInfo.agentID, + roleSetId, + isMember + ); + } - if (agentInfo.userID && invitation.roleSet) { - const isOpenInvitation = - await this.invitationService.isFinalizedInvitation(invitation.id); - invitationState = this.lifecycleService.getState( - invitation.lifecycle, - this.roleSetServiceLifecycleApplication.getApplicationMachine() - ); - const isMember = invitationState === ApplicationLifecycleState.APPROVED; - if (agentInfo.userID && invitation.roleSet) { - if (!isOpenInvitation) { - await this.roleSetCacheService.deleteOpenInvitationFromCache( - agentInfo.userID, - invitation.roleSet.id - ); - } - await this.roleSetCacheService.setAgentIsMemberCache( - agentInfo.agentID, - invitation.roleSet.id, - isMember - ); - } - } - - if (agentInfo.userID && invitation.roleSet) { - await this.roleSetCacheService.deleteOpenApplicationFromCache( - agentInfo.userID, - invitation.roleSet.id - ); - await this.roleSetCacheService.setAgentIsMemberCache( - agentInfo.agentID, - invitation.roleSet.id, - true - ); - } + if (invitation.roleSet) { + const isOpenInvitation = + await this.invitationService.isFinalizedInvitation(invitation.id); + invitationState = this.lifecycleService.getState( + invitation.lifecycle, + this.roleSetServiceLifecycleApplication.getApplicationMachine() + ); + const isMember = invitationState === ApplicationLifecycleState.APPROVED; + await this.updateInvitationCache( + agentInfo, + invitation.roleSet.id, + isMember, + isOpenInvitation + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/domain/access/role-set/role.set.resolver.mutations.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handlerGuidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases.- Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices.- Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices.- Refer to the design overview in the context files for better understanding.
src/domain/access/role-set/role.set.resolver.mutations.ts
🔇 Additional comments (3)
src/domain/access/role-set/role.set.resolver.mutations.ts (3)
72-72: LGTM: Clean dependency injection.The RoleSetCacheService is properly imported and injected following NestJS best practices.
Also applies to: 98-98
835-841: LGTM: Proper relation loading.The roleSet relation is correctly loaded to ensure the required data is available for cache operations.
896-901: LGTM: Proper relation loading.The roleSet relation is correctly loaded to ensure the required data is available for cache operations.
Summary by CodeRabbit
New Features
Refactor