fix(auth-api): Delegation notifications #18106
Conversation
WalkthroughThis pull request refactors the delegation record retrieval process. A new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DIS as DelegationsIndexService
participant FFS as FeatureFlagService
participant ASTM as ApiScopeDelegationTypeModel
Client->>+DIS: getDelegationRecords(request)
DIS->>+ASTM: Fetch supported delegation types
ASTM-->>-DIS: Return supported delegation types
DIS->>+FFS: Retrieve feature flag values
FFS-->>-DIS: Return feature flags
DIS->>DIS: Filter delegation records (filterByFeatureFlaggedDelegationTypes)
DIS-->>-Client: Return filtered delegation records
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/feature-flags/src/lib/features.ts (1)
105-105: Consider aligning the naming convention for consistency.
Throughout this enum, many flags start with “is” or “should”. You might rename this flag for uniformity:- delegationTypesWithNotificationsEnabled = 'delegationTypesWithNotificationsEnabled', + isDelegationTypesWithNotificationsEnabled = 'isDelegationTypesWithNotificationsEnabled',libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (1)
162-198: Add unit tests for the new filtering logic.
While the logic seems correct, adding targeted tests forfilterByFeatureFlaggedDelegationTypeswould help ensure correct handling of edge cases, such as invalid string formats or partial matches.Would you like me to propose test code covering various input scenarios for this method?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts(5 hunks)libs/clients/auth/delegation-api/src/lib/auth-delegation-api-client.config.ts(1 hunks)libs/feature-flags/src/lib/features.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`libs/**/*`: "Confirm that the code adheres to the following...
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/auth/delegation-api/src/lib/auth-delegation-api-client.config.tslibs/feature-flags/src/lib/features.tslibs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
🪛 ESLint
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
[error] 41-41: 'lodash' import is restricted from being used.
(no-restricted-imports)
🔇 Additional comments (7)
libs/clients/auth/delegation-api/src/lib/auth-delegation-api-client.config.ts (1)
21-21: Verify the default scope.
Enabling the@island.is/auth/delegations/index:systemscope by default may allow unintended usage if the environment variable is unset. Please verify that this is the desired behavior, and that it will not expand permissions unexpectedly.libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (6)
16-18: Imports look good.
These added service imports appear valid and do not introduce immediate concerns.
35-36: Enum imports look good.
No issues noted with these additional imports.
39-40: No issues with the model import.
This reference toApiScopeDelegationTypealigns with the new usage below.
155-156: Injected FeatureFlagService is appropriate.
Dependency injection here is standard for using feature flags.
220-225: Delegation types fetching is coherent.
QueryingApiScopeDelegationTypeand mapping them to delegation types is straightforward and consistent with the service’s purpose.
230-259: Combined query logic is valid.
Using a single query for both custom and non-custom delegations improves efficiency and maintainability.
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 03b6d5d.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (5)
164-170: Consider adding error handling for feature flag retrieval.The code handles empty or wildcard values well, but doesn't have explicit error handling if the feature flag service throws an exception.
const featureFlaggedDelegationTypes = await this.featureFlagService - .getValue(Features.delegationTypesWithNotificationsEnabled, '') - .then((types): Set<string> | '*' | undefined => { + .getValue(Features.delegationTypesWithNotificationsEnabled, '') + .then((types): Set<string> | '*' | undefined => { if (!types?.trim()) return undefined; // Empty value means no delegation types allowed if (types.trim() === '*') return '*'; // All delegation types allowed return new Set(types.split(',').map((type) => type.trim())); + }) + .catch((error) => { + this.logger.error('Failed to get delegation types from feature flags', { error }); + return undefined; // Default to no allowed types on error });
219-224: Consider adding error handling for API scope delegation types retrieval.You're using a Promise chain to map the results, but there's no error handling if the database query fails.
- const delegationTypesSupportedByScope = await this.apiScopeDelegationTypeModel.findAll({ - where: { - apiScopeName: apiScope.name, - }, - }).then(x => x.map((d) => d.delegationType)) + let delegationTypesSupportedByScope = []; + try { + delegationTypesSupportedByScope = await this.apiScopeDelegationTypeModel.findAll({ + where: { + apiScopeName: apiScope.name, + }, + }).then(x => x.map((d) => d.delegationType)); + } catch (error) { + this.logger.error('Failed to fetch delegation types supported by scope', { + error, + scope: apiScope.name + }); + // Continue with empty array + }
254-254: Use empty array instead of empty object in[Op.or]condition.When
supportsCustomis false, returning an empty object could potentially cause unexpected behavior. An empty array is more semantically correct for this OR condition.- supportsCustom ? { - ...where, - type: AuthDelegationType.Custom, - customDelegationScopes: { [Op.contains]: [apiScope.name] }, - } : {}, + supportsCustom ? { + ...where, + type: AuthDelegationType.Custom, + customDelegationScopes: { [Op.contains]: [apiScope.name] }, + } : [] as any,
258-258: Consider splitting the promise chain for better readability.The current chain mixes model querying and filtering, making it harder to follow. Splitting these operations would improve readability without affecting functionality.
- .then((d) => d.flat().map((d) => d.toDTO())).then((d) => this.filterByFeatureFlaggedDelegationTypes(d)) + .then((delegations) => delegations.flat().map((delegation) => delegation.toDTO())) + .then((delegationDTOs) => this.filterByFeatureFlaggedDelegationTypes(delegationDTOs))
499-501: Replace console.error with logger service.Using direct console.error bypasses the configured logger. Consider using the injected logger service for consistent error handling.
indexingPromises.forEach((p) => { if (p.status === 'rejected') { - console.error(p.reason) + this.logger.error('Error during delegation indexing', { error: p.reason }) } })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`libs/**/*`: "Confirm that the code adheres to the following...
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
🔇 Additional comments (3)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (3)
161-197: Impressive implementation of feature flag filtering!This new method effectively filters delegation records based on feature flags, handling different scenarios elegantly. The three-case approach (no types allowed, all types allowed, specific types allowed) provides comprehensive control over which delegation types receive notifications.
231-258: Great refactoring to consolidate queries!This is a significant improvement that achieves the PR's objective of consolidating delegation record queries into a single operation. The query structure is clear and effectively handles both regular delegation types and custom delegations.
40-40: Address restricted import of lodash.The static analysis tool previously flagged this type of import usage. Even though you're using
lodash/union(which is correct), you should verify there are no other lodash imports in the file.#!/bin/bash # Check for any full lodash imports in the file rg -n "import \{.*\} from 'lodash'" libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts (1)
73-80: 🛠️ Refactor suggestionMissing type definition for
supportedDelegationTypesproperty.The
Scopetype definition should be updated to include the newsupportedDelegationTypesproperty that has been added to all scope instances in thescopesobject.Apply this diff to update the type definition:
type Scope = { name: string grantToLegalGuardians?: boolean grantToProcuringHolders?: boolean allowExplicitDelegationGrant?: boolean grantToPersonalRepresentatives?: boolean personalRepresentativeRightTypePermissions?: string[] + supportedDelegationTypes?: (AuthDelegationType | PersonalRepresentativeDelegationType)[] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts(3 hunks)apps/services/auth/delegation-api/test/mocks/featureFlagService.mock.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`apps/services/**/*`: "Confirm that the code adheres to the ...
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
apps/services/auth/delegation-api/test/mocks/featureFlagService.mock.tsapps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/auth/delegation-api/test/mocks/featureFlagService.mock.tsapps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare / install
🔇 Additional comments (2)
apps/services/auth/delegation-api/test/mocks/featureFlagService.mock.ts (1)
2-2: Method return type changed to support async feature flag filtering.The
getValuemethod was updated to return a Promise resolving to a wildcard string ('*') instead of a boolean value. This change aligns with the new asynchronous feature flag filtering mechanism mentioned in the PR objectives for delegation types.This change is consistent with NestJS testing practices and properly supports the testing of asynchronous operations in the feature flag service. The wildcard value ('*') will allow tests to match any delegation type, providing flexibility in the testing environment.
apps/services/auth/delegation-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts (1)
86-86: Good implementation of delegation type support for scopes.The addition of
supportedDelegationTypesto each scope definition enhances the test fixtures by explicitly defining which delegation types are supported by each scope. This aligns with the PR objective of improving delegation record querying and filtering.Also applies to: 91-91, 96-96, 101-101, 109-109, 120-125, 129-129, 171-171
...ion-api/src/app/delegations/test/delegations-controller/delegations.controller-test-types.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
Outdated
Show resolved
Hide resolved
libs/clients/auth/delegation-api/src/lib/auth-delegation-api-client.config.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (4)
161-197: Implement clear documentation for feature flag format.The
filterByFeatureFlaggedDelegationTypesmethod implements a well-structured filtering mechanism. However, the specific format of the feature flag value (especially thetype:personandtype:companyformat) is not documented anywhere in the code.Add a descriptive comment above the method explaining the expected format of the feature flag value:
+/** + * Filters delegation records based on types specified in the feature flag. + * The feature flag value can be: + * - Empty string: No delegation types allowed + * - "*": All delegation types allowed + * - Comma-separated list of delegation types: Only those types allowed + * - For Custom and GeneralMandate types, can specify ":person" or ":company" suffix + * (e.g., "Custom:person,Custom:company,LegalGuardian") + */ private async filterByFeatureFlaggedDelegationTypes( delegations: DelegationRecordDTO[], ): Promise<DelegationRecordDTO[]> {
183-196: Consider extracting person/company checking logic to a helper function.The logic for checking if a delegation is from a person or company is relatively complex and would benefit from extraction to improve readability.
return delegations.filter((delegation) => { if (delegation.type === AuthDelegationType.Custom || delegation.type === AuthDelegationType.GeneralMandate) { - const isFromPerson = - featureFlaggedDelegationTypes.has(`${delegation.type}:person`) && - kennitala.isPerson(delegation.fromNationalId) - - const isFromCompany = - featureFlaggedDelegationTypes.has(`${delegation.type}:company`) && - kennitala.isCompany(delegation.fromNationalId) - - return isFromPerson || isFromCompany; + return this.isDelegationAllowedByEntityType(delegation, featureFlaggedDelegationTypes); } return featureFlaggedDelegationTypes.has(delegation.type); }); // Add this as a new private method private isDelegationAllowedByEntityType( delegation: DelegationRecordDTO, allowedTypes: Set<string>, ): boolean { const isFromPerson = allowedTypes.has(`${delegation.type}:person`) && kennitala.isPerson(delegation.fromNationalId); const isFromCompany = allowedTypes.has(`${delegation.type}:company`) && kennitala.isCompany(delegation.fromNationalId); return isFromPerson || isFromCompany; }
219-223: Improve variable naming for better readability.The variable name
xused in the.then()callback doesn't convey meaningful information about what it represents.-const delegationTypesSupportedByScope = await this.apiScopeDelegationTypeModel.findAll({ - where: { - apiScopeName: apiScope.name, - }, -}).then(x => x.map((d) => d.delegationType)) +const delegationTypesSupportedByScope = await this.apiScopeDelegationTypeModel.findAll({ + where: { + apiScopeName: apiScope.name, + }, +}).then(delegationTypes => delegationTypes.map((d) => d.delegationType))
258-258: Consider breaking down chained operations for readability.The long chain of operations makes it difficult to understand the flow. Consider breaking it down into separate steps.
- .then((d) => d.flat().map((d) => d.toDTO())).then((d) => this.filterByFeatureFlaggedDelegationTypes(d)) + .then((records) => records.flat()) + .then((flattenedRecords) => flattenedRecords.map((record) => record.toDTO())) + .then((dtoRecords) => this.filterByFeatureFlaggedDelegationTypes(dtoRecords))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`libs/**/*`: "Confirm that the code adheres to the following...
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare / install
🔇 Additional comments (6)
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (6)
3-3: Address restricted import of lodash.This import of lodash was previously flagged by static analysis in past reviews.
Consider refactoring to use limited-scope import:
-import { isEmpty } from 'lodash' +import isEmpty from 'lodash/isEmpty'Also applies to: 41-41
39-40: Good decision to inject the appropriate model and feature flag service.The injections for
ApiScopeDelegationTypemodel andFeatureFlagServiceare well-placed in the constructor. These additions support the consolidated query approach.
142-143: Properly injected dependencies.The dependency injection approach follows the established patterns in the codebase, maintaining consistency with the existing design.
Also applies to: 154-154
231-257: Well-structured consolidated query approach.The refactored query elegantly consolidates what were previously separate queries into a single query using the
Op.oroperator. This approach is more efficient and maintainable.
167-167: 🛠️ Refactor suggestionAdd safeguard against undefined values.
The current logic assumes
typesis always a string or undefined, but doesn't handle cases where it might be null.-if (!types?.trim()) return undefined; // Empty value means no delegation types allowed +if (!types || !types.trim()) return undefined; // Empty value means no delegation types allowedLikely an incorrect or invalid review comment.
219-258:❓ Verification inconclusive
Validate the query performance under load.
The refactored consolidated query approach is more elegant, but it's important to verify that it performs well under load, especially for users with many delegation records.
🏁 Script executed:
#!/bin/bash # Description: Look for any existing performance tests or tools for database operations # Check for performance test files echo "Checking for performance test files..." fd "performance|perf" -t f -e ts -x grep -l "delegations\|query" {} # Look for monitoring setup echo "Checking for monitoring setup..." fd "monitor|metrics" -t f -e ts -x grep -l "query\|database" {} # Check if Sequelize is configured with logging echo "Checking sequelize logging configuration..." fd "sequelize" -t f -x grep -l "logging:" {}Length of output: 602
Action Required: Validate Load Performance of Consolidated Query
Before merging, please ensure that the consolidated query performs efficiently under heavy load, especially for users with many delegation records. While the refactored approach is more elegant, it's crucial to:
- Run Performance Tests: Verify the query's efficiency under simulated high-load scenarios. We identified a potential performance test file at
apps/native/app/src/graphql/performance-link.tsthat may help kickstart this process.- Monitor Logging & Metrics: Confirm that Sequelize logging (as seen in files like
apps/services/university-gateway/src/sequelizeConfig.service.tsandlibs/nest/sequelize/src/lib/sequelize.ts) is configured to capture any performance bottlenecks.- Review Database Optimizations: Based on the load test outcomes, consider database indexing or query restructuring if performance issues arise.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Tests