Entitlements + license services #4593
Conversation
…odule still present until move all data to be via defaults
…ut to space defaults
… for templatesManager
…prevent template used as default to be deleted; fixed removal of template set on template manager
…into subspaceTemplates
…ing limits, removing notion of soft limits
![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
🧹 Outside diff range and nitpick comments (4)
src/core/bootstrap/bootstrap.service.ts (3)
454-457: Remove unnecessary blank line.The license policy application logic is correct, but there's an unnecessary blank line that should be removed for better code consistency.
await this.authorizationPolicyService.saveAll(accountAuthorizations); - const accountEntitlements = await this.accountLicenseService.applyLicensePolicy(account.id); await this.licenseService.saveAll(accountEntitlements);
512-515: Remove unnecessary blank line.The license policy application logic is correct, but there's an unnecessary blank line that should be removed for better code consistency.
const accountEntitlements = await this.accountLicenseService.applyLicensePolicy(account.id); await this.licenseService.saveAll(accountEntitlements); - return this.spaceService.getSpaceOrFail(space.id);
454-457: Consider adding specific error handling for license operations.The license policy application and entitlement saving operations are critical for the bootstrap process. Consider wrapping these operations in try-catch blocks with specific error messages to better identify and handle license-related issues.
await this.authorizationPolicyService.saveAll(accountAuthorizations); + try { const accountEntitlements = await this.accountLicenseService.applyLicensePolicy(account.id); await this.licenseService.saveAll(accountEntitlements); + } catch (error) { + throw new BootstrapException( + `Failed to apply license policy: ${error.message}`, + { originalException: error } + ); + }Also applies to: 512-514
src/domain/access/role-set/role.set.resolver.mutations.ts (1)
172-179: Consider extracting relation options to a constant.The relation options object for getRoleSetOrFail could be extracted to improve reusability and maintainability.
+const ROLE_SET_LICENSE_RELATIONS = { + relations: { + license: { + entitlements: true, + }, + }, +}; const roleSet = await this.roleSetService.getRoleSetOrFail( roleData.roleSetID, - { - relations: { - license: { - entitlements: true, - }, - }, - } + ROLE_SET_LICENSE_RELATIONS );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/app.module.ts(2 hunks)src/common/enums/alkemio.error.status.ts(1 hunks)src/common/enums/authorization.policy.type.ts(1 hunks)src/common/enums/authorization.privilege.ts(1 hunks)src/core/bootstrap/bootstrap.service.ts(4 hunks)src/domain/access/role-set/role.set.resolver.mutations.ts(5 hunks)src/domain/access/role-set/role.set.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app.module.ts
- src/common/enums/alkemio.error.status.ts
- src/common/enums/authorization.policy.type.ts
- src/common/enums/authorization.privilege.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/core/bootstrap/bootstrap.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/access/role-set/role.set.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/access/role-set/role.set.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (10)
src/core/bootstrap/bootstrap.service.ts (1)
58-59: LGTM: License service imports and injection follow NestJS best practices.
The new license services are properly imported and injected following dependency injection principles.
Also applies to: 87-88
src/domain/access/role-set/role.set.resolver.mutations.ts (3)
57-58: LGTM! Import statements are properly organized.
The new imports for license-related functionality are correctly placed and follow the module's organization pattern.
Also applies to: 67-68
91-91: LGTM! Dependency injection is properly configured.
The LicenseService is correctly injected through constructor injection, following NestJS best practices.
204-208: Verify error handling for license entitlement check.
The license entitlement check is correctly implemented, but we should verify that appropriate error handling exists in the LicenseService for cases where:
- The license is null/undefined
- The entitlement doesn't exist
✅ Verification successful
Error handling for license entitlement check is properly implemented
The LicenseService has comprehensive error handling for all cases:
- Null/undefined license: Throws
RelationshipNotFoundException - Missing entitlements: Throws
RelationshipNotFoundException - Non-existent entitlement type: Throws
RelationshipNotFoundException - Disabled entitlement: Throws
LicenseEntitlementNotAvailableException
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in LicenseService
ast-grep --pattern 'class LicenseService {
$$$
isEntitlementEnabledOrFail($license, $entitlement) {
$$$
}
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Let's try to find the LicenseService implementation using ripgrep
rg -l "class LicenseService"
# Also search for the specific method implementation
rg "isEntitlementEnabledOrFail" -A 10
Length of output: 1941
Script:
#!/bin/bash
# Let's check the isEntitlementEnabled method implementation and any related error handling
rg "isEntitlementEnabled\(" -A 10 src/domain/common/license/license.service.ts
# Also check for the exception class definition
rg "class LicenseEntitlementNotAvailableException" -A 5
Length of output: 1533
Script:
#!/bin/bash
# Let's check the implementation of getEntitlementsFromLicenseOrFail and getEntitlementFromEntitlementsOrFail
rg "getEntitlementsFromLicenseOrFail|getEntitlementFromEntitlementsOrFail" -A 10 src/domain/common/license/license.service.ts
Length of output: 2844
src/domain/access/role-set/role.set.service.ts (6)
58-61: Imports for licensing functionality are correctly added
149-149: Loading of license in relations is correctly implemented
157-158: Null checks include license property appropriately
194-194: Removal of the associated license is correctly implemented
81-81: Ensure that LicenseService is properly provided in the module
Please make sure that LicenseService is included in the module's providers array to enable dependency injection.
Run the following script to verify if LicenseService is provided in any module:
#!/bin/bash
# Description: Verify that LicenseService is provided in the module's providers array.
# Find all module files and check if LicenseService is included in providers
fd --type f --extension ts '.*\.module\.ts$' | while read module_file; do
# Check if LicenseService is included in providers
if rg -q 'providers:\s*\[.*\bLicenseService\b.*\]' "$module_file"; then
echo "LicenseService is provided in $module_file"
exit 0
fi
done
echo "LicenseService is not provided in any module's providers array"109-120: Ensure that license is properly defined in the RoleSet entity
Please verify that the license property is correctly defined in the RoleSet entity, and that the relationship between RoleSet and License is properly set up in TypeORM, including any necessary decorators and join columns.
Run the following script to check if license is defined in the RoleSet entity:
✅ Verification successful
Let me try another script to find the RoleSet entity definition.
Based on the search results, I found the RoleSet entity definition in src/domain/access/role-set/role.set.entity.ts which shows that the license property is properly defined with TypeORM decorators:
@OneToOne(() => License, {
eager: false,
cascade: true,
onDelete: 'SET NULL',
})
@JoinColumn()
license?: License;The property is correctly defined as a one-to-one relationship with the License entity, including proper TypeORM decorators for the relationship and join column. The cascade option is set to true which allows for automatic persistence of the license when saving the RoleSet entity.
The license property is properly defined in the RoleSet entity with correct TypeORM decorators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `license` is defined in the `RoleSet` entity
# Search for the `license` property in the `RoleSet` entity
ast-grep --pattern $'class RoleSet {
$$$
license: $_;
$$$
}'
Length of output: 20896
Script:
#!/bin/bash
# Search for the RoleSet entity definition
rg -A 10 "class RoleSet" --type ts
Length of output: 14595
…ver; moved migration to be last
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
src/services/auth-reset/reset.event.type.ts (2)
1-8: Add JSDoc documentation for the enum and its members.While the enum implementation is correct, adding documentation would improve maintainability and help other developers understand the purpose of each reset type.
+/** + * Defines the types of reset events that can be triggered in the system. + * These events are used to reset various authorization and license states. + */ export enum RESET_EVENT_TYPE { + /** Resets all authorization settings for an account */ AUTHORIZATION_RESET_ACCOUNT = 'auth-reset-account', + /** Resets authorization settings for a specific user */ AUTHORIZATION_RESET_USER = 'auth-reset-user', + /** Resets authorization settings for an organization */ AUTHORIZATION_RESET_ORGANIZATION = 'auth-reset-organization', + /** Resets platform-wide authorization settings */ AUTHORIZATION_RESET_PLATFORM = 'auth-reset-platform', + /** Resets AI server authorization settings */ AUTHORIZATION_RESET_AI_SERVER = 'auth-reset-ai-server', + /** Resets license settings for an account */ LICENSE_RESET_ACCOUNT = 'license-reset-account', }
1-8: Consider separating authorization and license reset types.While the current implementation works, consider splitting this enum into two separate ones (
AUTH_RESET_EVENT_TYPEandLICENSE_RESET_EVENT_TYPE) to better align with the PR's objective of separating authorization and entitlement logic. This would make the separation of concerns more explicit in the type system.-export enum RESET_EVENT_TYPE { +/** + * Defines the types of authorization reset events. + */ +export enum AUTH_RESET_EVENT_TYPE { AUTHORIZATION_RESET_ACCOUNT = 'auth-reset-account', AUTHORIZATION_RESET_USER = 'auth-reset-user', AUTHORIZATION_RESET_ORGANIZATION = 'auth-reset-organization', AUTHORIZATION_RESET_PLATFORM = 'auth-reset-platform', AUTHORIZATION_RESET_AI_SERVER = 'auth-reset-ai-server', +} + +/** + * Defines the types of license reset events. + */ +export enum LICENSE_RESET_EVENT_TYPE { LICENSE_RESET_ACCOUNT = 'license-reset-account', } + +/** + * Union type combining all reset event types. + */ +export type RESET_EVENT_TYPE = AUTH_RESET_EVENT_TYPE | LICENSE_RESET_EVENT_TYPE;src/services/auth-reset/subscriber/auth-reset.subscriber.module.ts (1)
23-24: Consider grouping related modules together.The module imports look good, but consider grouping the LicenseModule with AuthorizationPolicyModule since they are both related to access control, as mentioned in the PR objectives about separating authorization and entitlements.
Consider this organization:
imports: [ AuthorizationPolicyModule, + LicenseModule, AccountModule, SpaceModule, UserModule, PlatformModule, OrganizationModule, TaskModule, - AiServerModule, - LicenseModule, + AiServerModule, ],src/services/auth-reset/publisher/auth-reset.service.ts (2)
82-82: Improve type safety in emit calls.The emit calls use
<any, any>or<any, AuthResetEventPayload>type parameters. Consider using more specific types to improve type safety.- this.authResetQueue.emit<any, AuthResetEventPayload>( + this.authResetQueue.emit<void, AuthResetEventPayload>( - this.authResetQueue.emit<any, any>( + this.authResetQueue.emit<void, void>( - this.authResetQueue.emit<any, any>( + this.authResetQueue.emit<void, void>(Also applies to: 143-143, 151-151
Line range hint
1-154: Consider splitting authorization and license reset services.The service currently handles both authorization and license resets. Given the PR's objective of separating authorization and entitlement logic, consider splitting this into two separate services:
AuthorizationResetServicefor authorization-related resetsLicenseResetServicefor license-related resetsThis would better align with the single responsibility principle and make the codebase more maintainable.
src/domain/space/account/account.resolver.mutations.ts (1)
81-83: Consider extracting common account loading optionsThe same license relations configuration is repeated across multiple methods. Consider extracting it to a constant or method to improve maintainability.
+ private readonly DEFAULT_ACCOUNT_RELATIONS = { + license: { + entitlements: true, + }, + }; - const account = await this.accountService.getAccountOrFail( - spaceData.accountID, - { - relations: { - license: { - entitlements: true, - }, - }, - } - ); + const account = await this.accountService.getAccountOrFail( + spaceData.accountID, + { + relations: this.DEFAULT_ACCOUNT_RELATIONS, + } + );Also applies to: 145-147, 188-190, 248-250
src/services/auth-reset/subscriber/auth-reset.controller.ts (3)
97-142: Refactor duplicate error handling and retry logicThe
licenseResetAccountmethod contains error handling and retry logic similar to other methods likeauthResetAccountandauthResetAiServer. To improve maintainability and reduce code duplication, consider extracting the common logic into a shared helper function or middleware.
130-132: Correct the extra space in log messagesThere is an extra space in the log message string:
`Processing license reset for account with id ${payload.id} failed. Retrying (${retryCount + 1}/${MAX_RETRIES})`Remove the extra space between "Processing" and "license" to ensure consistent log formatting.
Apply this diff to fix the issue:
- `Processing license reset for account with id ${ + `Processing license reset for account with id ${
114-114: Pass the entire account object toapplyLicensePolicyCurrently, only
account.idis passed tothis.accountLicenseService.applyLicensePolicy(account.id). IfapplyLicensePolicyrequires more information about the account in the future, passing the entireaccountobject might be more extensible.src/migrations/1730877510666-licenseEntitlements.ts (2)
3-3: Remove unused import ofqueryfrom 'express'The import
queryfrom 'express' at line 3 is unnecessary and can be safely removed to clean up the code.Apply this diff to remove the unused import:
-import { query } from 'express';
408-408: Typographical error in 'Templatet'There's a misspelling in the
namefield; it should be'Space Save As Template'instead of'Space Save As Templatet'.Apply this diff to correct the typo:
- name: 'Space Save As Templatet', + name: 'Space Save As Template',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/domain/space/account/account.resolver.mutations.ts(8 hunks)src/migrations/1730877510666-licenseEntitlements.ts(1 hunks)src/services/auth-reset/auth-reset.payload.interface.ts(2 hunks)src/services/auth-reset/event.type.ts(0 hunks)src/services/auth-reset/publisher/auth-reset.service.ts(7 hunks)src/services/auth-reset/reset.event.type.ts(1 hunks)src/services/auth-reset/subscriber/auth-reset.controller.ts(5 hunks)src/services/auth-reset/subscriber/auth-reset.subscriber.module.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/services/auth-reset/event.type.ts
🧰 Additional context used
📓 Path-based instructions (7)
src/domain/space/account/account.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/migrations/1730877510666-licenseEntitlements.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/auth-reset.payload.interface.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/publisher/auth-reset.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/reset.event.type.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/subscriber/auth-reset.controller.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/subscriber/auth-reset.subscriber.module.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (7)
src/services/auth-reset/auth-reset.payload.interface.ts (1)
1-1: Verify the impact of RESET_EVENT_TYPE migration.
The change from AUTH_RESET_EVENT_TYPE to RESET_EVENT_TYPE looks good and aligns with the PR objectives of separating authorization and entitlement logic. However, this is a breaking change that requires verification of all consumers.
Let's verify all usages of this interface and enum:
Also applies to: 10-10
✅ Verification successful
Migration from AUTH_RESET_EVENT_TYPE to RESET_EVENT_TYPE is complete and consistent
The verification results show that:
- No references to the old
AUTH_RESET_EVENT_TYPEremain in the codebase - All usages of
AuthResetEventPayloadinterface have been updated to use the newRESET_EVENT_TYPE - The new enum is consistently used across the auth-reset service:
- Interface definition in
auth-reset.payload.interface.ts - Publisher implementation in
auth-reset.service.ts - Subscriber implementation in
auth-reset.controller.ts
- Interface definition in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration from AUTH_RESET_EVENT_TYPE to RESET_EVENT_TYPE
# and identify any potential missed updates
# Check for any remaining references to the old enum
echo "Checking for any remaining AUTH_RESET_EVENT_TYPE references:"
rg "AUTH_RESET_EVENT_TYPE"
# Find all files using AuthResetEventPayload to ensure they're updated
echo -e "\nChecking usage of AuthResetEventPayload interface:"
rg "AuthResetEventPayload"
# Find all files using the new RESET_EVENT_TYPE
echo -e "\nChecking usage of new RESET_EVENT_TYPE enum:"
rg "RESET_EVENT_TYPE"
Length of output: 4092
src/services/auth-reset/subscriber/auth-reset.subscriber.module.ts (1)
10-11: LGTM! Import statements are properly organized.
The new imports follow the project's path aliasing convention and are correctly placed with other module imports.
src/services/auth-reset/publisher/auth-reset.service.ts (1)
31-37: Verify the order of reset operations.
The current implementation resets authorizations first, followed by licenses. Please verify if this order is intentional and won't cause any issues with interdependencies between authorizations and licenses.
✅ Verification successful
Let me search for any dependencies between authorization and license resets in the implementation.
Let me try a different approach to search for license reset implementation.
Based on the code analysis, I can now provide a final response about the order of reset operations.
The order of authorization and license resets is correct and intentional.
The current implementation follows a logical sequence:
- Authorization resets are performed first to ensure proper access control updates
- License resets are performed after, as licenses often depend on authorization context
This is evidenced by:
- License operations use authorization checks (e.g.,
LICENSE_RESETprivilege inauthorization.privilege.ts) - License policies are applied with consideration of agent credentials and entitlements, as seen in
account.service.license.ts - The reset handlers (
auth-reset.controller.ts) maintain this separation, processing authorization resets before license resets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependencies between authorization and license resets
rg -A 5 "AUTHORIZATION_RESET|LICENSE_RESET" src/
Length of output: 20971
Script:
#!/bin/bash
# Search for license reset implementation to check dependencies
ast-grep --pattern 'async licenseResetAccount($$$) {
$$$
}'
Length of output: 63
Script:
#!/bin/bash
# Search for license reset implementation and any dependencies with authorization
rg -A 10 "licenseResetAccount|applyLicensePolicy" src/
Length of output: 17034
src/domain/space/account/account.resolver.mutations.ts (2)
42-45: LGTM: License-related dependencies properly integrated
The new imports and constructor injection follow NestJS dependency injection patterns correctly.
Also applies to: 52-52, 66-66
307-331: LGTM: License reset mutation properly implemented
The mutation follows the established pattern and includes proper authorization checks. However, let's verify the privilege constant usage across the codebase.
✅ Verification successful
LICENSE_RESET privilege is properly defined and consistently used
The privilege is correctly defined in the authorization enum and is used appropriately in both the account resolver mutation and authorization service. The implementation follows the established pattern for privilege checks.
- Defined in
src/common/enums/authorization.privilege.ts - Used in authorization checks in
account.resolver.mutations.ts - Properly integrated in privilege list in
account.service.authorization.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of LICENSE_RESET privilege
rg -l "AuthorizationPrivilege\.LICENSE_RESET"
Length of output: 160
Script:
#!/bin/bash
# Check the implementation and usage context of LICENSE_RESET privilege
rg -A 5 "AuthorizationPrivilege\.LICENSE_RESET"
Length of output: 1260
Script:
#!/bin/bash
# Check the enum definition of AuthorizationPrivilege
ast-grep --pattern 'enum AuthorizationPrivilege {
$$$
}'
Length of output: 4519
src/services/auth-reset/subscriber/auth-reset.controller.ts (2)
188-230: Refactor duplicate error handling and retry logic
The authResetAiServer method also replicates the error handling and retry logic found in other methods. Refactoring this repeated code into a shared utility function would enhance code maintainability and reduce redundancy.
36-38: Ensure injected services are utilized appropriately
The injected licenseService and accountLicenseService are new additions. Confirm that they're used correctly throughout the class and consider removing them if they're unnecessary to avoid unused dependencies.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/core/bootstrap/bootstrap.service.ts (3)
89-91: Review constructor parameter count for maintainabilityThe constructor now includes additional services:
AccountLicenseService,LicenseService, andLicensingFrameworkService. While dependency injection is essential in NestJS, having too many injected services can make the class harder to manage and test. Consider refactoring to reduce the number of dependencies, possibly by grouping related services into a module or service.
Line range hint
280-293: Consolidate license plan operations within a single serviceIn the
createLicensePlansmethod,licensePlanByNameExistsis called fromlicensePlanService, whilecreateLicensePlanis called fromlicensingFrameworkService. For consistency and adherence to the single responsibility principle, consider consolidating these operations into a single service. This will improve maintainability and make the codebase clearer.
463-465: Eliminate duplication by extracting common license policy applicationThe logic for applying license policies and saving account entitlements is duplicated here and in the
ensureSpaceSingletonmethod. Consider extracting this code into a separate method to reduce duplication and enhance maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/common/enums/authorization.privilege.ts(1 hunks)src/core/bootstrap/bootstrap.module.ts(2 hunks)src/core/bootstrap/bootstrap.service.ts(5 hunks)src/domain/collaboration/collaboration/collaboration.service.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/enums/authorization.privilege.ts
- src/core/bootstrap/bootstrap.module.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/core/bootstrap/bootstrap.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/collaboration/collaboration/collaboration.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (4)
src/domain/collaboration/collaboration/collaboration.service.ts (2)
57-60: LGTM! Import statements are properly organized.
The new license-related imports are correctly added and follow the established pattern.
77-78: LGTM! Dependency injection is properly implemented.
The LicenseService is correctly injected as a private dependency following NestJS best practices.
src/core/bootstrap/bootstrap.service.ts (2)
58-59: New licensing services imported correctly
The import statements for LicenseService, AccountLicenseService, and LicensingFrameworkService are necessary to support the new licensing functionalities and are correctly added.
Also applies to: 61-61
520-523: Duplicate code detected
As previously mentioned, this code segment duplicates the logic for applying license policies. Refactoring will help maintain consistency and reduce potential errors.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
src/migrations/1731077703010-allowEventsFromSubspaces.ts (1)
Line range hint
44-91: Improve helper functions and type definitionsSeveral improvements needed for the helper functions:
- Don't swallow JSON parsing errors
- Make error handling consistent between functions
- Consider importing types from their canonical location
+import { ISpaceSettings, ISpaceSettingsCollaboration } from '../domain/space/space.types'; + const addEventsFromSubspacesSetting = (existingSettings: string): string => { - let settings: ISpaceSettings | undefined; try { - settings = JSON.parse(existingSettings); + const settings: ISpaceSettings = JSON.parse(existingSettings); + settings.collaboration.allowEventsFromSubspaces = true; + return JSON.stringify(settings); } catch (e) { - console.error('Error parsing settings JSON:', e); + throw new Error(`Failed to parse or update space settings: ${e.message}`); } - - if (!settings) { - return existingSettings; - } - settings.collaboration.allowEventsFromSubspaces = true; - return JSON.stringify(settings); }; const removeEventsFromSubspacesSetting = (existingSettings: string): string => { - let settings: ISpaceSettings | undefined; try { - settings = JSON.parse(existingSettings); + const settings: ISpaceSettings = JSON.parse(existingSettings); + const { allowEventsFromSubspaces, ...rest } = settings.collaboration; + settings.collaboration = rest; + return JSON.stringify(settings); } catch (e) { - console.error('Error parsing settings JSON:', e); + throw new Error(`Failed to parse or update space settings: ${e.message}`); } - - if (!settings) { - return existingSettings; - } - - const { allowEventsFromSubspaces, ...rest } = settings.collaboration; - (settings.collaboration as Omit< - ISpaceSettingsCollaboration, - 'allowEventsFromSubspaces' - >) = rest; - - return JSON.stringify(settings); }; -type ISpaceSettings = { - collaboration: ISpaceSettingsCollaboration; -}; - -type ISpaceSettingsCollaboration = { - allowEventsFromSubspaces: boolean; -};src/domain/space/space/space.module.ts (1)
Line range hint
1-78: Consider organizing licensing-related imports into a feature module.Given the growing number of licensing-related dependencies (LicensingFrameworkModule, LicenseModule, LicenseIssuerModule, etc.), consider creating a dedicated SpaceLicensingModule to encapsulate all licensing-related functionality. This would:
- Improve modularity and maintainability
- Reduce the number of direct dependencies in SpaceModule
- Make the licensing feature more portable and testable
Example structure:
@Module({ imports: [ LicensingFrameworkModule, LicenseModule, LicenseIssuerModule, LicenseEngineModule ], providers: [SpaceLicenseService], exports: [SpaceLicenseService] }) export class SpaceLicensingModule {}Then import just SpaceLicensingModule in SpaceModule.
src/services/auth-reset/publisher/auth-reset.service.ts (2)
Line range hint
49-93: Consider extracting common reset logicThe account reset methods share similar patterns. Consider extracting the common logic into a reusable function to improve maintainability and reduce duplication.
private async publishEntityReset<T>( entityType: new () => T, eventType: RESET_EVENT_TYPE, taskId?: string ) { const entities = await this.manager.find(entityType, { select: { id: true }, }); const task = taskId ? { id: taskId } : await this.taskService.create(entities.length); entities.forEach(({ id }) => this.authResetQueue.emit<any, AuthResetEventPayload>( eventType, { id, type: eventType, task: task.id, } ) ); return task.id; }
141-154: Maintain consistency in platform and AI server reset implementationsThe platform and AI server reset methods:
- Don't track tasks unlike other reset operations
- Send empty payloads
- Have different implementation patterns
Consider:
- Adding task tracking for consistency if these operations can be long-running
- Including relevant metadata in the payload
- Following the same pattern as other reset methods
src/core/bootstrap/bootstrap.service.ts (2)
280-290: Consider enhancing error handlingWhile the basic error handling is present, consider adding more specific error handling for different scenarios:
- When licensing framework is not found
- When license plan creation fails
const licensing = await this.licensingFrameworkService.getDefaultLicensingOrFail(); +if (!licensing) { + throw new BootstrapException('Default licensing framework not found'); +} for (const licensePlanData of licensePlansData) { const planExists = await this.licensePlanService.licensePlanByNameExists( licensePlanData.name ); if (!planExists) { await this.licensingFrameworkService.createLicensePlan({ ...licensePlanData, licensingID: licensing.id, }); } }
520-523: Consider extracting common license policy application logicThe license policy application logic is duplicated between
ensureOrganizationSingletonandensureSpaceSingleton. Consider extracting this into a reusable method.+private async applyAndSaveAccountLicensePolicy(accountId: string): Promise<void> { + const accountEntitlements = await this.accountLicenseService.applyLicensePolicy(accountId); + await this.licenseService.saveAll(accountEntitlements); +} // Use in both methods: -const accountEntitlements = await this.accountLicenseService.applyLicensePolicy(account.id); -await this.licenseService.saveAll(accountEntitlements); +await this.applyAndSaveAccountLicensePolicy(account.id);src/domain/space/space/space.service.ts (1)
165-205: Consider using constants for entitlement limits.The license creation logic is well-structured, but consider extracting the magic number
0into named constants for better maintainability and clarity.+const DEFAULT_ENTITLEMENT_LIMIT = 0; + space.license = await this.licenseService.createLicense({ type: LicenseType.SPACE, entitlements: [ { type: LicenseEntitlementType.SPACE_FREE, dataType: LicenseEntitlementDataType.FLAG, - limit: 0, + limit: DEFAULT_ENTITLEMENT_LIMIT, enabled: false, }, // ... apply to other entitlements ], });src/domain/access/role-set/role.set.service.ts (2)
81-81: Consider grouping related service injections.The
licenseServiceinjection should be grouped with other authorization-related services at the top of the constructor for better code organization.constructor( private authorizationPolicyService: AuthorizationPolicyService, + private licenseService: LicenseService, private applicationService: ApplicationService, private invitationService: InvitationService, private platformInvitationService: PlatformInvitationService, private formService: FormService, private roleService: RoleService, private agentService: AgentService, private contributorService: ContributorService, private userService: UserService, private organizationService: OrganizationService, private virtualContributorService: VirtualContributorService, private communityResolverService: CommunityResolverService, private roleSetEventsService: RoleSetEventsService, private aiServerAdapter: AiServerAdapter, private communityCommunicationService: CommunityCommunicationService, - private licenseService: LicenseService, @InjectRepository(RoleSet) private roleSetRepository: Repository<RoleSet>, @Inject(WINSTON_MODULE_NEST_PROVIDER) private readonly logger: LoggerService )
149-158: Extract required relations to a constant.Consider extracting the list of required relations to a constant for better maintainability and reusability.
+ private readonly REQUIRED_ROLESET_RELATIONS = [ + 'roles', + 'applications', + 'invitations', + 'platformInvitations', + 'applicationForm', + 'license', + ] as const; async removeRoleSet(roleSetID: string): Promise<boolean> { const roleSet = await this.getRoleSetOrFail(roleSetID, { relations: { - roles: true, - applications: true, - invitations: true, - platformInvitations: true, - applicationForm: true, - license: true, + ...Object.fromEntries( + this.REQUIRED_ROLESET_RELATIONS.map(relation => [relation, true]) + ), }, }); if ( - !roleSet.roles || - !roleSet.applications || - !roleSet.invitations || - !roleSet.platformInvitations || - !roleSet.applicationForm || - !roleSet.license + !this.REQUIRED_ROLESET_RELATIONS.every( + relation => roleSet[relation as keyof typeof roleSet] + ) )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/core/bootstrap/bootstrap.service.ts(5 hunks)src/domain/access/role-set/role.set.service.ts(5 hunks)src/domain/space/space/space.module.ts(4 hunks)src/domain/space/space/space.service.spec.ts(3 hunks)src/domain/space/space/space.service.ts(9 hunks)src/domain/timeline/calendar/calendar.resolver.fields.ts(0 hunks)src/migrations/1731077703010-allowEventsFromSubspaces.ts(3 hunks)src/services/auth-reset/publisher/auth-reset.service.ts(7 hunks)
💤 Files with no reviewable changes (1)
- src/domain/timeline/calendar/calendar.resolver.fields.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/domain/space/space/space.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/core/bootstrap/bootstrap.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/access/role-set/role.set.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/space/space.module.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/space/space.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/migrations/1731077703010-allowEventsFromSubspaces.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/auth-reset/publisher/auth-reset.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (13)
src/migrations/1731077703010-allowEventsFromSubspaces.ts (2)
1-6: LGTM: Migration class structure follows TypeORM conventions
The class properly implements MigrationInterface and the naming follows the timestamp convention.
Line range hint 1-91: Verify integration with licensing system and existing events
Since this migration is part of the entitlements and licensing changes, please verify:
- How this feature integrates with the new licensing system
- The impact on existing events and their visibility
✅ Verification successful
Let me check the actual implementation of these files to understand how the feature is integrated.
Migration is correctly integrated with event visibility and space settings
The verification shows that the migration is properly integrated into the system:
- Calendar service correctly filters events using
visibleOnParentCalendarflag - Event service handles the visibility flag during event updates
- Space settings for
allowEventsFromSubspacesis properly used in calendar resolver to control event bubbling - Test coverage exists for the space settings functionality
The changes do not require integration with the licensing system as they implement independent visibility controls. The migration safely handles existing events by:
- Adding
visibleOnParentCalendarcolumn with a default value - Preserving existing space settings while adding the new flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related licensing configurations and event visibility logic
# Look for related license entitlements
ast-grep --pattern 'allowEventsFromSubspaces'
# Check for event visibility checks in the codebase
rg -l 'visibleOnParentCalendar'
# Look for related space settings usage
rg -l 'collaboration.*allowEventsFromSubspaces'
Length of output: 622
Script:
#!/bin/bash
# Check calendar service implementation for visibility logic
rg -A 5 'visibleOnParentCalendar' src/domain/timeline/calendar/calendar.service.ts
# Check event service implementation
rg -A 5 'visibleOnParentCalendar' src/domain/timeline/event/event.service.ts
# Check space service test cases
rg -A 5 'allowEventsFromSubspaces' src/domain/space/space/space.service.spec.ts
# Check calendar resolver implementation
rg -A 5 'allowEventsFromSubspaces' src/domain/timeline/calendar/calendar.resolver.fields.ts
Length of output: 2150
src/domain/space/space/space.module.ts (3)
32-34: LGTM! Import changes align with licensing framework integration.
The new imports correctly introduce the licensing framework components as described in the PR objectives.
76-76: LGTM! Service export enables proper module composition.
The SpaceLicenseService is correctly exported, making it available for use by other modules.
Let's verify the service usage across the codebase:
✅ Verification successful
Service export is correctly used across the codebase
The SpaceLicenseService is properly imported and utilized in the following locations:
- Used in admin licensing mutations (
admin.licensing.resolver.mutations.ts) - Integrated with account licensing service (
account.service.license.ts) - Correctly declared and exported in the space module (
space.module.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SpaceLicenseService usage in other modules
# Expect: Import statements and dependency injection of the service
rg "SpaceLicenseService" --type ts
Length of output: 894
70-70: LGTM! Provider registration follows NestJS patterns.
The SpaceLicenseService is correctly registered as a provider.
Let's verify the service implementation:
✅ Verification successful
Let me verify if the SpaceLicenseService has the @Injectable decorator since it wasn't visible in the first search.
SpaceLicenseService implementation verified and properly configured
The service implementation follows NestJS best practices with:
- Proper
@Injectable()decorator - Correct dependency injection in constructor
- Well-structured license policy management methods
- Appropriate error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SpaceLicenseService implementation
# Expect: Class with @Injectable() decorator and proper dependency injection
ast-grep --pattern 'class SpaceLicenseService {
$$$
}'
Length of output: 13484
Script:
#!/bin/bash
# Check for @Injectable decorator on SpaceLicenseService
ast-grep --pattern '@Injectable()
export class SpaceLicenseService {
$$$
}'
Length of output: 13570
src/services/auth-reset/publisher/auth-reset.service.ts (2)
11-11: LGTM: Import statement is correctly updated
The import of RESET_EVENT_TYPE aligns with the new event type constants used throughout the service.
31-37: Verify the order of reset operations
The sequence of reset operations looks logical, but please verify that:
- The order of resets (accounts → organizations → users → platform → AI server → licenses) doesn't create any dependencies or race conditions
- The platform and AI server resets don't require task tracking like the others
✅ Verification successful
Let me generate another script to check for any dependencies or ordering requirements in the implementation of these reset handlers.
Let me try a different pattern to search for the reset handler implementations.
Based on the analysis of the implementation, I can now provide the final response:
Reset operations sequence is correctly implemented
The current implementation is safe because:
- Each reset operation is handled independently through RabbitMQ events, preventing any direct dependencies or race conditions
- Platform and AI server resets don't require task tracking as they are system-wide operations without specific entity IDs
- All handlers have proper error handling and retry mechanisms through RabbitMQ channel
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependencies between reset operations
rg -A 5 "RESET_EVENT_TYPE\.(AUTHORIZATION_RESET|LICENSE_RESET)" --type ts
Length of output: 7224
Script:
#!/bin/bash
# Search for implementation details of reset handlers
ast-grep --pattern 'async authReset$_($$$) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Search for implementation details of reset handlers in the controller
rg -B 2 -A 10 "async (authReset|licenseReset)" --type ts
Length of output: 8551
src/core/bootstrap/bootstrap.service.ts (3)
58-61: LGTM: License service imports are properly organized
The new imports follow NestJS conventions and are correctly organized by their domain.
89-91: LGTM: License services are properly injected
The dependency injection follows NestJS best practices with proper access modifiers.
462-465: Verify license policy application on existing organizations
The changes look good, but we should verify how this affects existing organizations.
✅ Verification successful
The search results show that applyLicensePolicy is consistently used across the codebase in various services (AccountLicenseService, SpaceLicenseService, etc.) and is already being called in multiple scenarios including:
- Role management (platform.role.resolver.mutations.ts)
- Auth reset handling (auth-reset.controller.ts)
- Admin licensing operations (admin.licensing.resolver.mutations.ts)
- Account and space management (account.resolver.mutations.ts, space.service.license.ts)
Let me verify the implementation of these services to ensure consistent behavior.
Let me try a different approach to verify the license policy implementations.
Based on the comprehensive analysis of the license policy implementation across the codebase, I can now provide a final response:
License policy changes are properly integrated and safe for existing organizations
The license policy application is consistently implemented across the codebase with proper handling for both new and existing organizations:
- The
applyLicensePolicymethod is called in multiple scenarios including organization creation, role management, auth resets, and admin operations - The implementation follows a hierarchical structure (Account → Space → RoleSet → Collaboration) with proper type checking and validation
- License entitlements are well-defined with specific types and proper error handling
- The changes maintain backward compatibility with existing license types and entitlements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing organizations that might be affected
ast-grep --pattern 'class OrganizationService {
$$$
getOrganization($$$) {
$$$
}
$$$
}'
# Check for existing license policy applications
rg -A 3 "applyLicensePolicy"
Length of output: 7009
Script:
#!/bin/bash
# Check the implementation of license policy services
ast-grep --pattern 'class AccountLicenseService {
$$$
applyLicensePolicy($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class SpaceLicenseService {
$$$
applyLicensePolicy($$$) {
$$$
}
$$$
}'
# Check for any license policy tests
rg -l "license.*policy.*test"
Length of output: 243
Script:
#!/bin/bash
# Check the actual implementation files
rg -A 10 "export class AccountLicenseService"
rg -A 10 "export class SpaceLicenseService"
# Look for any license policy related interfaces or types
rg -A 5 "interface.*License"
rg -A 5 "type.*License"
Length of output: 22056
src/domain/space/space/space.service.ts (2)
107-109: LGTM: Constructor changes align with licensing framework separation.
The injection of licensingFrameworkService and licenseService aligns with the PR's objective of separating authorization and entitlement logic.
333-333: LGTM: Proper license cleanup implementation.
The changes ensure that the license is properly loaded and cleaned up during space deletion, preventing orphaned license records.
Also applies to: 346-346, 367-367
src/domain/access/role-set/role.set.service.ts (1)
58-61: LGTM: Clean import structure.
The new license-related imports are well-organized and follow TypeScript/NestJS conventions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
src/platform/licensing-framework/licensing.framework.resolver.fields.ts (1)
21-26: Consider adding error handling for getLicensePlansOrFailWhile using
getLicensePlansOrFailis good for ensuring data integrity, consider adding try-catch block to handle potential failures gracefully and return appropriate GraphQL errors.async plans( @Parent() licensing: ILicensingFramework ): Promise<ILicensePlan[]> { + try { return await this.licensingFrameworkService.getLicensePlansOrFail( licensing.id ); + } catch (error) { + throw new Error(`Failed to fetch license plans: ${error.message}`); + } }src/domain/space/account.host/account.host.service.ts (2)
63-103: Consider extracting license entitlement configurationWhile the initialization is secure with disabled entitlements by default, the current implementation has repeated code structures that could be made more maintainable.
Consider extracting the entitlement configuration to a constant or configuration file:
// license.config.ts export const DEFAULT_ACCOUNT_ENTITLEMENTS = [ { type: LicenseEntitlementType.ACCOUNT_SPACE_FREE, dataType: LicenseEntitlementDataType.LIMIT, limit: 0, enabled: false, }, // ... other entitlements ]; // account.host.service.ts account.license = await this.licenseService.createLicense({ type: LicenseType.ACCOUNT, entitlements: DEFAULT_ACCOUNT_ENTITLEMENTS, });
Line range hint
138-163: Consider optimizing database operationsWhile the licensing framework integration is correct, there are potential performance improvements to consider:
- Multiple database calls in the loop could be optimized
- License plan assignments should potentially be wrapped in a transaction
Consider this approach:
// Wrap in transaction await this.entityManager.transaction(async transactionalEntityManager => { // Batch assign license plans await Promise.all(licensePlansToAssign.map(licensePlan => this.licenseIssuerService.assignLicensePlan( spaceAgent, licensePlan, spaceID, transactionalEntityManager ) )); });src/domain/space/account/account.resolver.mutations.ts (2)
81-83: Consider eager loading optimization for license relationsThe license relations with entitlements are loaded in multiple queries. Consider creating a reusable constant or method for these common relation options to maintain consistency and reduce duplication.
+ private readonly LICENSE_RELATIONS = { + license: { + entitlements: true, + }, + }; async createSpace( @CurrentUser() agentInfo: AgentInfo, @Args('spaceData') spaceData: CreateSpaceOnAccountInput ): Promise<ISpace> { const account = await this.accountService.getAccountOrFail( spaceData.accountID, { relations: { - license: { - entitlements: true, - }, + ...this.LICENSE_RELATIONS, }, } );Also applies to: 145-147, 188-190, 248-250
577-577: Remove trailing whitespaceThere is unnecessary trailing whitespace on line 577.
🧰 Tools
🪛 eslint
[error] 577-577: Delete
····(prettier/prettier)
src/domain/space/space/space.service.ts (1)
166-206: Consider extracting license entitlements configuration.The hardcoded entitlements array could be moved to a configuration constant or service to improve maintainability and reusability. This would make it easier to update entitlements and their default values across different space types.
Consider this approach:
+// Create a new file: src/domain/space/space/constants/space.license.entitlements.ts +export const DEFAULT_SPACE_LICENSE_ENTITLEMENTS = [ + { + type: LicenseEntitlementType.SPACE_FREE, + dataType: LicenseEntitlementDataType.FLAG, + limit: 0, + enabled: false, + }, + // ... other entitlements +]; // In space.service.ts -space.license = await this.licenseService.createLicense({ - type: LicenseType.SPACE, - entitlements: [ - { - type: LicenseEntitlementType.SPACE_FREE, - dataType: LicenseEntitlementDataType.FLAG, - limit: 0, - enabled: false, - }, - // ... other entitlements - ], -}); +space.license = await this.licenseService.createLicense({ + type: LicenseType.SPACE, + entitlements: DEFAULT_SPACE_LICENSE_ENTITLEMENTS, +});src/platform/licensing-framework/licensing.framework.service.ts (2)
43-44: Consistency in return typesConsider removing
| neverfrom the return typePromise<ILicensingFramework | never>ingetDefaultLicensingOrFail. Functions that throw exceptions do not need to includeneverin their return types. This change promotes consistency and clarity across the service methods.Apply this diff:
- ): Promise<ILicensingFramework | never> { + ): Promise<ILicensingFramework> {
64-66: Consistency in return typesSimilar to earlier, consider removing
| neverfrom the return typePromise<ILicensePlan[] | never>ingetLicensePlansOrFail. This promotes consistency across your methods and aligns with TypeScript best practices.Apply this diff:
- ): Promise<ILicensePlan[] | never> { + ): Promise<ILicensePlan[]> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/domain/space/account.host/account.host.service.ts(5 hunks)src/domain/space/account/account.resolver.mutations.ts(8 hunks)src/domain/space/space/space.module.ts(3 hunks)src/domain/space/space/space.service.ts(9 hunks)src/migrations/1731077703010-allowEventsFromSubspaces.ts(3 hunks)src/platform/licensing-framework/licensing.framework.resolver.fields.ts(1 hunks)src/platform/licensing-framework/licensing.framework.service.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/space/space/space.module.ts
- src/migrations/1731077703010-allowEventsFromSubspaces.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/domain/space/account.host/account.host.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/account/account.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/space/space.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/platform/licensing-framework/licensing.framework.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/platform/licensing-framework/licensing.framework.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🪛 eslint
src/domain/space/account/account.resolver.mutations.ts
[error] 577-577: Delete ····
(prettier/prettier)
🔇 Additional comments (19)
src/platform/licensing-framework/licensing.framework.resolver.fields.ts (1)
6-7: LGTM! Clean framework transition
The changes to imports, class name, and service injection properly reflect the transition to the new licensing framework structure while maintaining NestJS best practices.
Also applies to: 11-13
src/domain/space/account.host/account.host.service.ts (3)
27-31: LGTM: Dependencies properly structured for new licensing system
The new dependencies and imports are well-organized and follow NestJS best practices for dependency injection. The changes align with the PR's objective of introducing a new licensing structure.
Also applies to: 38-39
115-115: LGTM: Improved error message clarity
The error message now clearly indicates that the account lookup is specifically on the Host system.
67-97: Verify license entitlement type definitions
Let's ensure all used license entitlement types are properly defined in the enum.
✅ Verification successful
All license entitlement types are properly defined and used consistently
The verification shows that all license entitlement types used in account.host.service.ts are properly defined in the enum at src/common/enums/license.entitlement.type.ts:
- ACCOUNT_SPACE_FREE
- ACCOUNT_SPACE_PLUS
- ACCOUNT_SPACE_PREMIUM
- ACCOUNT_VIRTUAL_CONTRIBUTOR
- ACCOUNT_INNOVATION_HUB
- ACCOUNT_INNOVATION_PACK
The usage of these types is consistent across the codebase, with proper references in related services and migrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all license entitlement types used in the code are defined in the enum
# Search for the LicenseEntitlementType enum definition
echo "Searching for LicenseEntitlementType enum definition..."
rg -A 10 "export enum LicenseEntitlementType"
# Search for all usages of these entitlement types
echo "Searching for all usages of these entitlement types..."
rg "LicenseEntitlementType\.[A-Z_]+"
Length of output: 11210
src/domain/space/account/account.resolver.mutations.ts (2)
42-45: LGTM: Dependencies properly injected
The new license-related dependencies are correctly imported and injected following NestJS dependency injection patterns.
Also applies to: 52-52, 66-66
307-331: LGTM: Well-structured license reset mutation
The new mutation follows NestJS patterns with proper:
- GraphQL guard
- Documentation
- Parameter validation
- Authorization check
- Error handling
src/domain/space/space/space.service.ts (3)
108-110: LGTM: Constructor changes align with the new licensing architecture.
The injection of LicensingFrameworkService and LicenseService follows NestJS dependency injection best practices and properly implements the separation of concerns between licensing framework and license management.
334-334: LGTM: Proper license cleanup implementation.
The changes ensure that:
- License relation is properly loaded
- Null checks prevent runtime errors
- License deletion is performed in the correct order
Also applies to: 346-347, 368-368
1288-1304: LGTM: Robust error handling for licensing framework.
The implementation properly handles potential errors in licensing framework retrieval with appropriate error logging and graceful fallback, addressing the concerns raised in the previous review.
src/platform/licensing-framework/licensing.framework.service.ts (10)
8-10: Imports are correctly updated
The imports have been appropriately updated to reflect the new LicensingFramework context.
16-16: Class name change is appropriate
Renaming the class to LicensingFrameworkService accurately represents the new framework structure.
19-20: Updated repository injection is correct
Injecting LicensingFramework repository is correctly implemented using @InjectRepository(LicensingFramework).
26-26: Method parameter type updated appropriately
The options parameter in getLicensingOrFail is correctly updated to FindOneOptions<LicensingFramework> to match the new entity.
28-28: Repository usage is consistent
Using licensingFrameworkRepository.findOne() aligns with the updated repository and entity.
45-45: Repository find method usage
The use of licensingFrameworkRepository.find() correctly retrieves all LicensingFramework entities.
58-61: Updated save method aligns with new entity
The save method now correctly handles ILicensingFramework entities with the updated repository.
85-85: Method call updated correctly
The call to getLicensePlansOrFail is appropriately updated to reflect the new method signature.
98-98: Method signature and property access updated
The parameter type for createLicensePlan and the property licensingFrameworkID are correctly updated to match the new input interface.
Also applies to: 101-101
117-117: Persisting changes to the licensing entity
Saving the licensing entity after modifying its plans array ensures that changes are persisted correctly.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (28)
src/services/api/conversion/conversion.resolver.mutations.ts (2)
Line range hint
47-96: Consider adding license/entitlement checksGiven that this PR introduces new licensing and entitlements functionality, consider adding appropriate license checks before performing these conversions.
Would you like me to provide an example implementation of how to integrate license checks into these mutation methods?
Inconsistent authorization privileges detected between conversion methods.
convertChallengeToSpaceusesPLATFORM_ADMINprivilege.convertOpportunityToChallengeusesCREATEprivilege.🔗 Analysis chain
Line range hint
47-65: Inconsistent authorization privileges between conversion methodsThe
convertChallengeToSpacemethod usesPLATFORM_ADMINprivilege whileconvertOpportunityToChallengeusesCREATEprivilege. Consider aligning these privileges for consistency unless there's a specific reason for the difference.Also applies to: 67-96
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other conversion-related methods to verify privilege consistency rg -A 2 "AuthorizationPrivilege\." "src/services/api/conversion/"Length of output: 937
src/domain/collaboration/collaboration/collaboration.resolver.mutations.ts (4)
60-60: Improved error handling with consistent fail-fast approach.The change to use
deleteCollaborationOrFailaligns well with the fail-fast pattern already used ingetCollaborationOrFailandgrantAccessOrFail. This ensures consistent error handling throughout the method chain.Consider adding a more specific error message to help with debugging:
- return this.collaborationService.deleteCollaborationOrFail(deleteData.ID); + return this.collaborationService.deleteCollaborationOrFail(deleteData.ID, `Failed to delete collaboration: ${deleteData.ID}`);
Line range hint
65-146: Update authorization to use the new licensing system.Based on the PR objectives of separating authorization and entitlements, this method should be updated to check both user authorization and entity entitlements.
Consider updating the authorization check to include entitlement verification:
this.authorizationService.grantAccessOrFail( agentInfo, collaboration.authorization, AuthorizationPrivilege.CREATE_CALLOUT, `create callout on collaboration: ${collaboration.id}` ); + + // Verify entity-level entitlements + await this.licenseService.isEntitlementEnabledOrFail( + collaboration.license, + 'CREATE_CALLOUT', + `create callout on collaboration: ${collaboration.id}` + );
Line range hint
149-176: Update authorization to use the new licensing system.Similar to the
createCalloutOnCollaborationmethod, this should be updated to check both user authorization and entity entitlements.Consider updating the authorization check to include entitlement verification:
this.authorizationService.grantAccessOrFail( agentInfo, collaboration.authorization, AuthorizationPrivilege.UPDATE, `update callouts sort order on collaboration: ${collaboration.id}` ); + + // Verify entity-level entitlements + await this.licenseService.isEntitlementEnabledOrFail( + collaboration.license, + 'UPDATE_CALLOUTS', + `update callouts sort order on collaboration: ${collaboration.id}` + );
Line range hint
22-34: Inject LicenseService for entitlement checks.To support the new licensing system, the class needs access to the LicenseService.
Update the constructor to inject the LicenseService:
constructor( private communityResolverService: CommunityResolverService, private contributionReporter: ContributionReporterService, private calloutAuthorizationService: CalloutAuthorizationService, private authorizationPolicyService: AuthorizationPolicyService, private authorizationService: AuthorizationService, private collaborationService: CollaborationService, private activityAdapter: ActivityAdapter, private notificationAdapter: NotificationAdapter, private calloutService: CalloutService, private namingService: NamingService, - private temporaryStorageService: TemporaryStorageService + private temporaryStorageService: TemporaryStorageService, + private licenseService: LicenseService ) {}src/domain/space/space/space.resolver.mutations.ts (2)
Line range hint
28-37: Integrate LicenseService for entitlement checksGiven that this PR introduces a new licensing structure, the SpaceResolverMutations should integrate with LicenseService to validate space-related entitlements before performing operations.
Add LicenseService to the constructor:
constructor( private contributionReporter: ContributionReporterService, private activityAdapter: ActivityAdapter, private authorizationService: AuthorizationService, private authorizationPolicyService: AuthorizationPolicyService, private spaceService: SpaceService, private spaceAuthorizationService: SpaceAuthorizationService, + private licenseService: LicenseService, @Inject(SUBSCRIPTION_SUBSPACE_CREATED) private subspaceCreatedSubscription: PubSubEngine, private namingReporter: NameReporterService ) {}
Line range hint
89-106: Add license entitlement check before space deletionThe deleteSpace mutation should validate both authorization and license entitlements before proceeding with the deletion.
Add license check before deletion:
async deleteSpace( @CurrentUser() agentInfo: AgentInfo, @Args('deleteData') deleteData: DeleteSpaceInput ): Promise<ISpace> { const space = await this.spaceService.getSpaceOrFail(deleteData.ID); + // Verify license entitlements + await this.licenseService.validateEntitlementOrFail( + space.id, + 'space.delete', + `delete space: ${space.nameID}` + ); + this.authorizationService.grantAccessOrFail( agentInfo, space.authorization, AuthorizationPrivilege.DELETE, `deleteSpace: ${space.nameID}` ); return await this.spaceService.deleteSpaceOrFail(deleteData); }src/services/api/registration/registration.service.ts (1)
186-186: Consider wrapping account deletion in a transaction.While the change to
deleteAccountOrFailimproves error handling, there's a potential race condition between user deletion and account deletion. If the account deletion fails, we'll have a deleted user but a dangling account.Consider wrapping both operations in a transaction:
await this.accountService.transaction(async () => { user = await this.userService.deleteUser(deleteData); await this.accountService.deleteAccountOrFail(account); });src/domain/common/license/license.service.ts (6)
19-28: Add interface documentation for the service classConsider adding JSDoc documentation to describe the service's purpose, responsibilities, and usage examples. This will improve maintainability and help other developers understand the service's role in the license management system.
Example documentation:
/** * Service responsible for managing licenses and their entitlements. * Handles CRUD operations for licenses and provides methods for * entitlement validation and management. * * @example * // Create a new license with entitlements * const license = await licenseService.createLicense({ * entitlements: [...] * }); */ @Injectable() export class LicenseService {
63-63: Improve code comment clarityThe current comment is not descriptive enough. Consider expanding it to explain why all entities need to be loaded.
Replace with:
// Load the complete license entity graph including all related entities (entitlements, authorization) // to ensure proper cascading deletion and cleanup of all associated records
80-84: Consider implementing soft delete and review ID restorationTwo potential issues in the deletion logic:
- No soft delete mechanism, which could be important for audit trails
- Manual ID restoration after deletion might cause issues with TypeORM's change tracking
Consider:
- Implementing soft delete using TypeORM's
@DeleteDateColumn- Using TypeORM's
softRemove()instead ofremove()- If hard delete is required, document why ID restoration is necessary
101-113: Optimize license loading in getEntitlementsThe current implementation potentially loads the license twice if entitlements are not loaded. Consider optimizing this logic.
Suggested improvement:
public async getEntitlements(licenseInput: ILicense): Promise<ILicenseEntitlement[]> { if (licenseInput.entitlements) { return licenseInput.entitlements; } const license = await this.getLicenseOrFail(licenseInput.id, { relations: { entitlements: true, }, }); return this.getEntitlementsFromLicenseOrFail(license); }
135-148: Consider implementing caching for entitlement checksThe
isEntitlementAvailablemethod might be called frequently. Consider implementing caching to improve performance.Consider using NestJS's caching mechanism:
@Injectable() export class LicenseService { constructor( @Inject(CACHE_MANAGER) private cacheManager: Cache, // ... other dependencies ) {} @Cacheable('license-entitlement-availability', ttl: 300) public async isEntitlementAvailable( license: ILicense, entitlementType: LicenseEntitlementType ): Promise<boolean> { // ... existing implementation } }
219-221: Improve error message clarityThe error message includes raw JSON stringification which might be hard to read in logs.
Consider formatting the error message more clearly:
- `Unable to find entitlement of type ${type} in Entitlements for License: ${JSON.stringify(entitlements)}`, + `Unable to find entitlement of type '${type}' in License. Available entitlement types: ${entitlements.map(e => e.type).join(', ')}`,src/domain/community/community/community.service.ts (2)
Line range hint
156-195: Consider wrapping cleanup sequence in a transaction.The cleanup sequence could leave the system in an inconsistent state if an error occurs during the process. Consider:
- Wrapping the entire cleanup sequence in a transaction
- Adding error handling for each cleanup step
- Rolling back on failure
Here's a suggested implementation:
async removeCommunityOrFail(communityID: string): Promise<boolean | never> { const community = await this.getCommunityOrFail(communityID, { relations: { roleSet: true, groups: true, communication: true, guidelines: true, }, }); if ( !community.communication || !community.communication.updates || !community.roleSet || !community.groups || !community.guidelines ) { throw new RelationshipNotFoundException( `Unable to load child entities for community for deletion: ${community.id} `, LogContext.COMMUNITY ); } + // Use QueryRunner for transaction management + const queryRunner = this.communityRepository.manager.connection.createQueryRunner(); + await queryRunner.connect(); + await queryRunner.startTransaction(); + + try { await this.roleSetService.removeAllRoleAssignments(community.roleSet); for (const group of community.groups) { await this.userGroupService.removeUserGroup({ ID: group.id, }); } if (community.authorization) await this.authorizationPolicyService.delete(community.authorization); await this.communicationService.removeCommunication( community.communication.id ); await this.roleSetService.removeRoleSetOrFail(community.roleSet.id); await this.communityGuidelinesService.deleteCommunityGuidelines( community.guidelines.id ); await this.communityRepository.remove(community as Community); + + await queryRunner.commitTransaction(); return true; + } catch (error) { + await queryRunner.rollbackTransaction(); + throw error; + } finally { + await queryRunner.release(); + } }
Line range hint
156-174: Enhance validation error messages and readability.The validation block could be more specific and maintainable.
Consider this improvement:
- if ( - !community.communication || - !community.communication.updates || - !community.roleSet || - !community.groups || - !community.guidelines - ) { - throw new RelationshipNotFoundException( - `Unable to load child entities for community for deletion: ${community.id} `, - LogContext.COMMUNITY - ); - } + const validations = [ + { entity: 'communication', value: community.communication?.updates }, + { entity: 'roleSet', value: community.roleSet }, + { entity: 'groups', value: community.groups }, + { entity: 'guidelines', value: community.guidelines }, + ]; + + const missingEntity = validations.find(v => !v.value); + if (missingEntity) { + throw new RelationshipNotFoundException( + `Missing required entity "${missingEntity.entity}" for community deletion: ${community.id}`, + LogContext.COMMUNITY + ); + }src/domain/space/account/account.service.ts (2)
141-141: Method name and return type improvements neededThe method name change to include 'OrFail' is good for clarity, but there's inconsistency with called methods. For example,
spaceService.deleteSpaceOrFailfollows this pattern whileagentService.deleteAgentdoesn't.Consider updating all called delete methods to follow the same naming pattern for consistency.
Also applies to: 151-151, 161-162
Line range hint
141-175: Consider implementing transaction managementThe deletion process involves multiple entities (agent, storage, license, virtual contributors, etc.) but lacks transaction management. If any operation fails after some deletions are completed, it could leave the system in an inconsistent state.
Consider wrapping the operations in a transaction:
async deleteAccountOrFail(accountInput: IAccount): Promise<IAccount | never> { + return await this.accountRepository.manager.transaction(async transactionalEntityManager => { const accountID = accountInput.id; const account = await this.getAccountOrFail(accountID, { relations: { // ... existing relations ... }, }); // ... existing checks ... - await this.agentService.deleteAgent(account.agent.id); + await this.agentService.deleteAgent(account.agent.id, transactionalEntityManager); // ... update other service methods to accept transactionalEntityManager ... - const result = await this.accountRepository.remove(account as Account); + const result = await transactionalEntityManager.remove(Account, account); result.id = accountID; return result; + }); }src/services/api/conversion/conversion.service.ts (3)
Line range hint
46-91: Consider consolidating entity validation logic.The multiple null checks could be simplified into a utility function to improve readability and maintainability.
+ private validateSpaceEntities(space: ISpace, context: string): void { + const requiredEntities = { + community: space.community, + 'community.roleSet': space.community?.roleSet, + context: space.context, + profile: space.profile, + collaboration: space.collaboration, + 'collaboration.callouts': space.collaboration?.callouts, + storageAggregator: space.storageAggregator, + }; + + const missingEntities = Object.entries(requiredEntities) + .filter(([_, value]) => !value) + .map(([key]) => key); + + if (missingEntities.length > 0) { + throw new EntityNotInitializedException( + `Unable to locate entities on ${context}: ${missingEntities.join(', ')}`, + LogContext.CONVERSION + ); + } + }
Line range hint
489-524: Consider atomic role reassignment operation.The current implementation performs multiple individual role operations that could leave the system in an inconsistent state if any operation fails. Consider implementing this as a single atomic operation.
+ private async reassignRolesAtomically( + sourceRoleSet: IRoleSet, + targetRoleSet: IRoleSet, + roles: { users: IUser[], organizations: IOrganization[] }, + roleType: CommunityRoleType + ): Promise<void> { + await this.roleSetService.bulkReassignRoles( + sourceRoleSet, + targetRoleSet, + roles, + roleType + ); + }
Line range hint
479-487: Implement cleanup for temporary communication.The temporary communication created during the swap operation should be cleaned up, even if the operation fails.
- const tmpCommunication = - await this.communicationService.createCommunication('temp', ''); + try { + const tmpCommunication = + await this.communicationService.createCommunication('temp', ''); + // ... existing swap logic ... + } finally { + if (tmpCommunication) { + await this.communicationService.removeCommunication(tmpCommunication.id); + } + }src/domain/space/account/account.resolver.mutations.ts (1)
307-331: Inconsistent error message format in authorization checkThe implementation is good, but there's an inconsistency in the error message format. Other methods use
userID.Consider updating the error message for consistency:
- `reset license definition on Account: ${agentInfo.userID}` + `reset license definition on Account: ${agentInfo.email}`src/domain/space/space/space.service.ts (1)
166-206: Consider extracting entitlements configuration.The hardcoded entitlements configuration could be moved to a separate configuration file or service for better maintainability and reusability.
Consider creating a dedicated service or configuration:
+// license.entitlements.config.ts +export const DEFAULT_SPACE_ENTITLEMENTS = [ + { + type: LicenseEntitlementType.SPACE_FREE, + dataType: LicenseEntitlementDataType.FLAG, + limit: 0, + enabled: false, + }, + // ... other entitlements +]; +// space.service.ts -space.license = await this.licenseService.createLicense({ - type: LicenseType.SPACE, - entitlements: [ - { - type: LicenseEntitlementType.SPACE_FREE, - // ... other entitlements - }, - ], -}); +space.license = await this.licenseService.createLicense({ + type: LicenseType.SPACE, + entitlements: DEFAULT_SPACE_ENTITLEMENTS, +});src/domain/access/role-set/role.set.service.ts (1)
149-149: Improve error message specificity for license validation.The error message could be more specific about the missing license relationship.
- `Unable to load child entities for roleSet for deletion: ${roleSet.id} `, + `Unable to load child entities (roles, applications, invitations, platformInvitations, applicationForm, license) for roleSet deletion: ${roleSet.id}`,Also applies to: 157-158
src/domain/common/license-entitlement/license.entitlement.service.ts (3)
138-142: Use a more appropriate exception for unexpected license typesIn the methods
getEntitlementUsageUsingEntitiesandisEntitlementAvailableUsingEntities, throwingEntityNotFoundExceptionwhen an unexpectedlicense.typeis encountered may not accurately represent the error scenario.Consider defining and throwing a more suitable exception, such as
InvalidLicenseTypeExceptionorUnsupportedLicenseTypeException, to clearly indicate that the license type is invalid or not supported.Also applies to: 181-185
118-120: Review returning-1forFLAGdata type ingetEntitlementUsageWhen
licenseEntitlement.dataTypeisFLAG, the method returns-1(line 119). Using-1to represent non-applicable usage might lead to confusion or errors in code that consumes this method.Consider returning
nullor throwing an exception to indicate that usage is not applicable for flag-based entitlements. Ensure that the method's return type reflects this change.
55-59: Ensure consistent use of braces inifstatementsThe
ifstatement at line 55 does not use braces, while otherifstatements in the code do. For consistency and to prevent potential errors during future modifications, it's recommended to always use braces withifstatements.Apply this diff to add braces:
if (!entitlement) - throw new EntityNotFoundException( + { + throw new EntityNotFoundException( `Not able to locate entitlement with the specified ID: ${entitlementID}`, LogContext.SPACES ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
src/domain/access/role-set/role.set.service.ts(6 hunks)src/domain/collaboration/collaboration/collaboration.resolver.mutations.ts(1 hunks)src/domain/collaboration/collaboration/collaboration.service.ts(5 hunks)src/domain/common/license-entitlement/license.entitlement.service.ts(1 hunks)src/domain/common/license/license.service.ts(1 hunks)src/domain/community/community/community.service.ts(2 hunks)src/domain/space/account/account.resolver.mutations.ts(8 hunks)src/domain/space/account/account.service.ts(7 hunks)src/domain/space/space/space.resolver.mutations.ts(1 hunks)src/domain/space/space/space.service.ts(10 hunks)src/domain/template/template/template.service.ts(1 hunks)src/domain/timeline/event/event.entity.ts(0 hunks)src/services/api/conversion/conversion.resolver.mutations.ts(2 hunks)src/services/api/conversion/conversion.service.ts(3 hunks)src/services/api/registration/registration.service.ts(2 hunks)test/mocks/account.service.mock.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/domain/timeline/event/event.entity.ts
🧰 Additional context used
📓 Path-based instructions (14)
src/domain/access/role-set/role.set.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/collaboration/collaboration/collaboration.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/collaboration/collaboration/collaboration.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/common/license-entitlement/license.entitlement.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/common/license/license.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/community/community/community.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/account/account.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/account/account.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/space/space.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/space/space.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/template/template/template.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/conversion/conversion.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/conversion/conversion.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/api/registration/registration.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🪛 Biome
src/domain/common/license-entitlement/license.entitlement.service.ts
[error] 173-173: Duplicate case label.
The first similar label is here:
(lint/suspicious/noDuplicateCase)
🔇 Additional comments (24)
src/services/api/conversion/conversion.resolver.mutations.ts (2)
Line range hint 1-46: LGTM! Well-structured class following NestJS best practices
The class demonstrates proper dependency injection, clear separation of concerns, and follows TypeScript/NestJS conventions.
57-60: LGTM! Improved error handling with OrFail variants
The change to use OrFail variants improves error handling by making failure cases more explicit and consistent.
Also applies to: 86-90
src/services/api/registration/registration.service.ts (1)
186-186: Verify consistent error handling patterns across related services.
The switch to deleteAccountOrFail suggests a broader pattern change. Let's verify this pattern is consistently applied across related services.
Also applies to: 209-209
✅ Verification successful
Error handling patterns are consistent across related services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential inconsistent error handling patterns
# Look for deleteAccount calls that might need updating to deleteAccountOrFail
# Check for direct deleteAccount calls
rg "deleteAccount\(" --type ts
# Check for similar patterns in other services
ast-grep --pattern 'async delete$_($_) {
$$$
await this.$_.delete($_);
$$$
}'
Length of output: 118
Script:
#!/bin/bash
# Search for all instances of deleteAccount and deleteAccountOrFail in TypeScript files
rg "deleteAccountOrFail?\(" --type ts
# Analyze async delete methods and their error handling in services
ast-grep --pattern 'async delete$_($_) {
$$$
await this.$_.deleteAccountOrFail($_);
$$$
}'
# Additionally, search for try-catch blocks around deleteAccount or deleteAccountOrFail
rg -A 3 "try\s*\{[^}]*await\s+this\.(deleteAccountOrFail|deleteAccount)\(" --type ts
Length of output: 578
src/domain/community/community/community.service.ts (1)
Line range hint 156-195: Improved error handling and cleanup sequence in community removal.
The changes enhance the robustness of the community removal process through:
- More explicit method naming with
OrFailsuffix - Comprehensive validation of child entities before proceeding
- Clear cleanup sequence for associated entities
Let's verify the cleanup sequence is complete by checking for any other potential relationships:
src/domain/space/account/account.service.ts (2)
39-39: LGTM: Clean dependency injection implementation
The LicenseService integration follows NestJS dependency injection best practices.
Also applies to: 55-55
174-175: Verify license removal impact
The license removal is performed before deleting spaces and other entities. Ensure that these entities don't have dependencies on the license that could cause issues during their deletion.
✅ Verification successful
License removal impact verified
All dependent entities properly handle license removal without leaving unresolved dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for license references in entity relationships
ast-grep --pattern 'class $_ {
$$$
@ManyToOne(() => License)
license: License;
$$$
}'
# Search for license usage in deletion methods
rg -A 5 'delete.*License|remove.*License'
Length of output: 6096
src/domain/template/template/template.service.ts (1)
337-339: LGTM! Improved error handling consistency.
The change from deleteCollaboration to deleteCollaborationOrFail aligns with NestJS best practices and ensures consistent error handling behavior.
src/services/api/conversion/conversion.service.ts (2)
41-44: LGTM: Improved method signatures with explicit error handling.
The renaming to include "OrFail" suffix and the addition of never return type clearly indicates that these methods may throw exceptions, improving API clarity.
Also applies to: 220-223
Line range hint 367-396: Ensure storage aggregator cleanup on failure.
The storage aggregator relationship changes lack cleanup mechanisms if the operation fails midway. Consider implementing a cleanup strategy or using a transaction.
#!/bin/bash
# Check for orphaned storage aggregators in the codebase
ast-grep --pattern 'storageAggregator.parentStorageAggregator = undefined'src/domain/space/account/account.resolver.mutations.ts (3)
42-45: LGTM! Dependencies are properly imported and injected
The new license-related services are correctly imported and injected following NestJS dependency injection patterns.
Also applies to: 52-52, 66-66
81-83: Well-structured implementation of license validation
The license validation pattern is consistently implemented across all creation methods:
- Loading license entitlements in relations
- Validating against the appropriate license type
- Using type-specific entitlements (SPACE_FREE, INNOVATION_HUB, etc.)
Also applies to: 89-92, 145-147, 153-156, 188-190, 196-199, 248-250, 256-259
557-603: Improve error message clarity in license validation
The error message could be more user-friendly by explaining what the entitlement limit means and how to resolve it.
Consider updating the error message:
- `Unable to create ${licenseType} on account: ${account.id}. Entitlement limit of ${entitlementLimit} of type ${licenseType} reached`,
+ `Unable to create ${licenseType} on account: ${account.id}. You have reached the maximum limit of ${entitlementLimit} for this type of resource. Please upgrade your license to increase this limit.`,The following issues from previous reviews are still valid:
- The typo in
isEntitleMentEnabled - Opportunity to use optional chaining for null checks
src/domain/collaboration/collaboration/collaboration.service.ts (4)
57-60: LGTM: Import statements are well-organized
The new license-related imports are correctly structured and follow TypeScript/NestJS conventions.
77-78: LGTM: Proper dependency injection
The LicenseService is correctly injected following NestJS dependency injection patterns.
134-151: Error handling concern already addressed
A previous review has already identified the need for error handling during license creation.
Line range hint 357-396: LGTM: Proper cleanup sequence for collaboration deletion
The deletion process correctly:
- Loads all necessary relations including the license
- Validates the existence of required relations
- Follows a logical cleanup sequence, removing dependent entities before the parent
A previous review has already suggested wrapping these operations in a transaction.
src/domain/space/space/space.service.ts (3)
17-17: LGTM: New imports align with licensing framework changes.
The added imports properly support the new licensing and entitlements functionality.
Also applies to: 80-84, 86-86
Line range hint 323-372: LGTM: Proper license cleanup implementation.
The license cleanup is properly integrated into the space deletion process with appropriate error handling.
1292-1308: LGTM: Error handling implementation matches previous review.
src/domain/access/role-set/role.set.service.ts (4)
58-61: LGTM: License-related imports are properly structured.
The imports follow NestJS conventions and are correctly organized.
81-81: LGTM: LicenseService is properly injected.
The dependency injection follows NestJS best practices.
109-119: Skip comment: Error handling feedback already provided.
194-194: LGTM: License removal is properly ordered.
The license is correctly removed before the role set deletion, maintaining referential integrity.
src/domain/common/license-entitlement/license.entitlement.service.ts (1)
70-71: Verify type consistency when removing an entitlement
In deleteEntitlementOrFail, the entitlement is cast to LicenseEntitlement (line 70). This casting suggests a potential type mismatch between ILicenseEntitlement and LicenseEntitlement.
Ensure that entitlement is of the correct type expected by the repository's remove method. If necessary, adjust the types to maintain consistency between interfaces and entities.
src/domain/common/license-entitlement/license.entitlement.service.ts
Outdated
Show resolved
Hide resolved
src/domain/common/license-entitlement/license.entitlement.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
src/domain/common/license/license.service.ts (3)
62-85: Improve documentation and type safety in removeLicenseOrFail
- Convert the inline comment to JSDoc
- Consider removing the type casting by ensuring proper typing
- Document why the ID needs to be restored after removal
Apply these improvements:
+ /** + * Removes a license and all its related entities (entitlements and authorization policy). + * Loads the complete license with all relations to ensure proper cleanup. + * @param licenseID - The ID of the license to remove + * @returns The removed license with its ID restored + * @throws EntityNotFoundException if the license is not found + */ async removeLicenseOrFail(licenseID: string): Promise<ILicense | never> { // ... existing code ... - const deletedLicense = await this.licenseRepository.remove(license as License); + const deletedLicense = await this.licenseRepository.remove(license); + // Restore the ID since TypeORM's remove operation nullifies it deletedLicense.id = license.id; return deletedLicense; }
101-113: Optimize getEntitlements methodThe method can be simplified by using optional chaining and avoiding unnecessary variable reassignment.
Apply this improvement:
public async getEntitlements( licenseInput: ILicense ): Promise<ILicenseEntitlement[]> { - let license = licenseInput; - if (!license.entitlements) { - license = await this.getLicenseOrFail(licenseInput.id, { + if (!licenseInput.entitlements) { + const license = await this.getLicenseOrFail(licenseInput.id, { relations: { entitlements: true, }, }); + return this.getEntitlementsFromLicenseOrFail(license); } - return this.getEntitlementsFromLicenseOrFail(license); + return this.getEntitlementsFromLicenseOrFail(licenseInput); }
211-225: Improve error message in getEntitlementFromEntitlementsOrFailThe error message includes the entire entitlements array, which could be overwhelming and unnecessary.
Apply this improvement:
private getEntitlementFromEntitlementsOrFail( entitlements: ILicenseEntitlement[], type: LicenseEntitlementType ): ILicenseEntitlement { const entitlement = entitlements.find( entitlement => entitlement.type === type ); if (!entitlement) { throw new RelationshipNotFoundException( - `Unable to find entitlement of type ${type} in Entitlements for License: ${JSON.stringify(entitlements)}`, + `Unable to find entitlement of type ${type} in Entitlements`, LogContext.LICENSE ); } return entitlement; }src/domain/common/license-entitlement/license.entitlement.service.ts (6)
51-53: Use the correctLogContextfor entitlement-related exceptionsThe exception in
getEntitlementOrFailis usingLogContext.SPACES, which may not accurately represent the context of a license entitlement. Consider usingLogContext.LICENSEfor exceptions related to licenses and entitlements.
112-114: Reconsider returning-1for flag-type entitlement usageReturning
-1ingetEntitlementUsagefor flag-type entitlements might be misleading. Consider returning0or another value that clearly indicates that usage is not applicable for flag-type entitlements.
132-136: Throw a more appropriate exception for unexpected license typesIn
getEntitlementUsageUsingEntities, throwing anEntityNotFoundExceptionfor an unexpected license type may not be the most accurate choice. Consider creating and throwing a custom exception likeInvalidLicenseTypeExceptionto more precisely indicate the error.
175-179: Ensure consistent exception handling for unexpected license typesIn
isEntitlementAvailableUsingEntities, throwing anEntityNotFoundExceptionwhen encountering an unexpected license type may not accurately describe the issue. Consider using a more specific exception or a custom exception to improve clarity.
125-136: Refactor duplicated switch statements to reduce code duplicationThe switch statements in
getEntitlementUsageUsingEntitiesandisEntitlementAvailableUsingEntitiesare similar. Consider abstracting common logic into a helper method to reduce code duplication and improve maintainability.Also applies to: 160-179
29-39: Optimize object creation using constructors or object spreadIn
createEntitlement, consider using a constructor or object spread syntax to create theLicenseEntitlementinstance for better readability and maintainability.Apply this diff to use object spread:
- const entitlement = new LicenseEntitlement(); - entitlement.limit = entitlementInput.limit; - entitlement.enabled = entitlementInput.enabled; - entitlement.type = entitlementInput.type; - entitlement.dataType = entitlementInput.dataType; + const entitlement = Object.assign(new LicenseEntitlement(), entitlementInput);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/domain/common/license-entitlement/license.entitlement.service.ts(1 hunks)src/domain/common/license/license.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/domain/common/license-entitlement/license.entitlement.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/common/license/license.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🪛 Biome
src/domain/common/license-entitlement/license.entitlement.service.ts
[error] 167-167: Duplicate case label.
The first similar label is here:
(lint/suspicious/noDuplicateCase)
🔇 Additional comments (2)
src/domain/common/license/license.service.ts (2)
1-28: LGTM: Well-structured class setup with proper dependency injection
The service follows NestJS best practices with appropriate decorators and dependencies.
30-44:
Critical: Fix async/await and add error handling in createLicense
The createEntitlement call on line 39 is not awaited, which could lead to race conditions. Additionally, the method needs proper error handling.
Apply this fix:
async createLicense(licenseData: CreateLicenseInput): Promise<ILicense> {
const license: ILicense = License.create(licenseData);
license.authorization = new AuthorizationPolicy(
AuthorizationPolicyType.LICENSE
);
license.entitlements = [];
for (const entitlementData of licenseData.entitlements) {
const entitlement =
- this.licenseEntitlementService.createEntitlement(entitlementData);
+ await this.licenseEntitlementService.createEntitlement(entitlementData);
license.entitlements.push(entitlement);
}
return license;
}The previous review comment about adding validation and transaction management is still valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/migrations/1731500015640-licenseEntitlements.ts (3)
3-3: Remove unused import of 'query' from 'express'The import statement on line 3 is unnecessary and can be removed to clean up the code.
Apply this diff to remove the unused import:
-import { query } from 'express';
357-367: Consider using parameterized queries for SQL statementsConstructing SQL queries using string interpolation can lead to security vulnerabilities and syntax errors. Even though the variables are internally generated, it's a good practice to use parameterized queries provided by
queryRunnerto enhance security and maintainability.Here's how you can modify the code to use parameterized queries:
await queryRunner.query( `INSERT INTO license_entitlement (id, version, type, dataType, \`limit\`, enabled, licenseId) VALUES (?, ?, ?, ?, ?, ?, ?)`, [licenseEntitlementID, 1, entitlementType, entitlementDataType, 0, 0, licenseID] );
613-613: Fix typo in 'Space Save As Template'There's a typo in the
namefield on line 613. The word "Templatet" should be corrected to "Template".Apply this diff to fix the typo:
- name: 'Space Save As Templatet', + name: 'Space Save As Template',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/migrations/1731500015640-licenseEntitlements.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/migrations/1731500015640-licenseEntitlements.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
* first pass at two new modules for TemplatesManager, TemplateDefault * added templates manager to space; removed the SpaceDefaults entity (module still present until move all data to be via defaults * added templatesManager to platform * moved creating of default innovatin flow input to space defaults * back out space type on Template; tidy up Template module to use switch statements * created template applier module * tidy up naming * updated set of default template types * fixed circular dependency; moved logic for creating collaboration input to space defaults * removed loading of defaults from files for collaboration content * removed code based addition of callouts, innovation flow states * tidy up naming * added loading of default templates at platform level in to bootstrap * removed option to create new innovation flow template * added in migration: * loading in templates on bootstrap * added field for collaboration templates on templatesSet; added lookup for templatesManager * added mutation to create template from collaboration; added logic to prevent template used as default to be deleted; fixed removal of template set on template manager * initial creation of license + entitlements modules * add license into account * updated account to have license service + use that in mutations checking limits, removing notion of soft limits * ensure data is loaded properly on account for license checking * added mutation to reset the license calculations on account, including new auth privilege to be able to do so * renamed Licensing module to LicensingFramework module; trigger license reset on Account after assigning / removing license * removed usage of LicenseEngine outside of license services on space or account * renamed entitlement to licenseEntitlement as entity; first pass at migration * fixed issues in migration * fixed issues related to auth reset; tidied up loader creator imports * fixed auth cascade for templates of type post * license reset running * reset licenses on space after adding / removing license plans * removed need for license check in community; added entitlement check in roleset when adding a VC * remove auth reset when assigning / removing license plans * added License to RoleSet * added license to collaboration * tidied up retrieval of license for whiteboard; added license to collaboration in migration * fix typo; fix space spec file * fix additional tests * moved tempaltesManager to last migration in the list * fixed retrieval of template when creating collaboration * added logging * fixed bootstrap setting of templates * refactored inputCreator to do the data loading closer to usage; fixed picking up of templates; fixed bootstrap usage of templates * added ability to retrieve limits on entitlements + current usage * updated field names on entitlements * updated field names on entitlements * fixed account mutaiton logic bug * ensure that licenses are reset when assigning beta tester or vc campaign role to a user * added reset all account licenses mutation * fixed bug on space entitlements; refactored code to reduce duplication * fixed url generation for templates inside of TempaltesManager * fixed bootstrap order to create forum earlier * ensure collaboration creation on template provides some defaults for callouts * fix deletion of templates of type post * ensure more data is defaulted inside of template service for collaboration; add setting of isTemplate field on Collaboration, and also on contained Callouts * ensure isTempalte is passed to Collaboration entity * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * reworked applying collaboraiton template to collaboration * improved error message in wrong type of ID passed in * fixed build * made migration last in the list * rename migration to be last * removed read check when looking up collaboration * track free / plus / premium space entitlements separately * updated migration order * removed duplicate migration * moved auth reset to mutation for applying the template to another collaboration * extend lookup of entitlement usage to cover new types * updaed license policy to reflect new entitlements; made license engine work with entitlements, not license privileges; removed license privilege (no longer relevant) * updated migration to not drop indexes already removed * fix for license reset on space * added license policy rule for free space credential * ensure license entitlements are reset as part of the bootstrap * fixed typo * extended reset all to include resetting licenses on accounts + AI server; moved migration to be last * Address pr comment * Address PR feedback * Address PR comment * Address PR comments * Address PR comments * Address PR comment Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Improved types & naming * Address PR comments * Fixed switch-case logic in entitlements * Converge entitlements schema * Remove unused AuthorizationPrivilege --------- Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* initial setup of invoking engines and receiving reponses trough CQRS * adding the new mesages/handlers * rename question to input and ask to invoke related to VCs adds room controller to be used by the server post replies to rooms and threads * complete implementation for the Expert engine * fix build * address comments and minor cleanups * change ask guidence question query to mutation back the guidance chat with a room implement guidence communication thorugh the event bus * update quickstart services * handle guidance room creaton trough the room service * Generate room on demand * Chat guidance room created only when needed * pending reset chat * it was already done * address comments * address some comments * Entitlements + license services (#4593) * first pass at two new modules for TemplatesManager, TemplateDefault * added templates manager to space; removed the SpaceDefaults entity (module still present until move all data to be via defaults * added templatesManager to platform * moved creating of default innovatin flow input to space defaults * back out space type on Template; tidy up Template module to use switch statements * created template applier module * tidy up naming * updated set of default template types * fixed circular dependency; moved logic for creating collaboration input to space defaults * removed loading of defaults from files for collaboration content * removed code based addition of callouts, innovation flow states * tidy up naming * added loading of default templates at platform level in to bootstrap * removed option to create new innovation flow template * added in migration: * loading in templates on bootstrap * added field for collaboration templates on templatesSet; added lookup for templatesManager * added mutation to create template from collaboration; added logic to prevent template used as default to be deleted; fixed removal of template set on template manager * initial creation of license + entitlements modules * add license into account * updated account to have license service + use that in mutations checking limits, removing notion of soft limits * ensure data is loaded properly on account for license checking * added mutation to reset the license calculations on account, including new auth privilege to be able to do so * renamed Licensing module to LicensingFramework module; trigger license reset on Account after assigning / removing license * removed usage of LicenseEngine outside of license services on space or account * renamed entitlement to licenseEntitlement as entity; first pass at migration * fixed issues in migration * fixed issues related to auth reset; tidied up loader creator imports * fixed auth cascade for templates of type post * license reset running * reset licenses on space after adding / removing license plans * removed need for license check in community; added entitlement check in roleset when adding a VC * remove auth reset when assigning / removing license plans * added License to RoleSet * added license to collaboration * tidied up retrieval of license for whiteboard; added license to collaboration in migration * fix typo; fix space spec file * fix additional tests * moved tempaltesManager to last migration in the list * fixed retrieval of template when creating collaboration * added logging * fixed bootstrap setting of templates * refactored inputCreator to do the data loading closer to usage; fixed picking up of templates; fixed bootstrap usage of templates * added ability to retrieve limits on entitlements + current usage * updated field names on entitlements * updated field names on entitlements * fixed account mutaiton logic bug * ensure that licenses are reset when assigning beta tester or vc campaign role to a user * added reset all account licenses mutation * fixed bug on space entitlements; refactored code to reduce duplication * fixed url generation for templates inside of TempaltesManager * fixed bootstrap order to create forum earlier * ensure collaboration creation on template provides some defaults for callouts * fix deletion of templates of type post * ensure more data is defaulted inside of template service for collaboration; add setting of isTemplate field on Collaboration, and also on contained Callouts * ensure isTempalte is passed to Collaboration entity * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * fixed groups in bootstrap space template; updated signature for creating callout from collaboration * fixed missing field * fixed type on mutation to create from collaboration * fixed typo * reworked applying collaboraiton template to collaboration * improved error message in wrong type of ID passed in * fixed build * made migration last in the list * rename migration to be last * removed read check when looking up collaboration * track free / plus / premium space entitlements separately * updated migration order * removed duplicate migration * moved auth reset to mutation for applying the template to another collaboration * extend lookup of entitlement usage to cover new types * updaed license policy to reflect new entitlements; made license engine work with entitlements, not license privileges; removed license privilege (no longer relevant) * updated migration to not drop indexes already removed * fix for license reset on space * added license policy rule for free space credential * ensure license entitlements are reset as part of the bootstrap * fixed typo * extended reset all to include resetting licenses on accounts + AI server; moved migration to be last * Address pr comment * Address PR feedback * Address PR comment * Address PR comments * Address PR comments * Address PR comment Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Improved types & naming * Address PR comments * Fixed switch-case logic in entitlements * Converge entitlements schema * Remove unused AuthorizationPrivilege --------- Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Major version bump * Add license lookup * removed redundant checks * Fixed proper L0 space agent selection in recursive license assignment (#4708) * Fixed proper L0 account agent selection in recursive license assignment * provide root space agent for sub-sub space * Look up correct entitlement for VC creation (#4707) Co-authored-by: Evgeni Dimitrov <[email protected]> * filter out demo spaces for unauthenticated users search in elastic (#4712) * filter out demo spaces for unauthenticated users search in elastic * improvements --------- Co-authored-by: Svetoslav <[email protected]> * get guidance room from user and not from me * Finish * wrapping up * fix roomId missing * Put the GuidanceVC into the platform * Guidance VC created on bootstrap * add support for guidance ingest * cleanup and handle sources label * fix build * address comments and update engine images --------- Co-authored-by: Valentin Yanakiev <[email protected]> Co-authored-by: Carlos Cano <[email protected]> Co-authored-by: Neil Smyth <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Evgeni Dimitrov <[email protected]> Co-authored-by: Svetoslav <[email protected]>
Details in https://alkem.io/building-alkemio-org/challenges/technicaldesigns-9853
Depends on subspaceTemplates PR.
Design choice: fully separate out the authorization (user) and entitlement (entity) logic; they simply overlay each other but have to be independent.
New module: License
New module: LicenseEntitlement
Separate tree of license services, similar to authorization services. Rational is that authorization is per user, entitlements are per entity.
Admin: new mutation to reset all Licenses on accounts
LicensePolicy + LicenseEngine switched to work with and issue boolean* entitlements; later it will need to work with limits also
Summary by CodeRabbit
Release Notes
New Features
LoaderCreatorModulefor enhanced loader functionalities.LicenseEntitlementDataType,LicenseEntitlementType, andLicenseTypefor improved license management.LicenseEntitlementNotAvailableExceptionandLicenseEntitlementNotSupportedException.RoleSetLicenseServiceandCollaborationLicenseServicefor managing license policies in role sets and collaborations.AccountLicenseServiceto manage license policies associated with accounts.SpaceLicenseServiceandLicenseServicefunctionalities for better license management.Bug Fixes
Refactor
Chores