feat(auth-admin): system national ids lookup #16568
Conversation
WalkthroughThe changes in this pull request involve updates to the integration of the National Registry API within the application. Specifically, a new client configuration for Changes
Possibly related PRs
Suggested labels
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
|
Datadog ReportAll test runs ✅ 6 Total Test Services: 0 Failed, 6 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16568 +/- ##
==========================================
+ Coverage 36.53% 36.58% +0.04%
==========================================
Files 6890 6880 -10
Lines 143638 143200 -438
Branches 40924 40781 -143
==========================================
- Hits 52481 52384 -97
+ Misses 91157 90816 -341
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 28 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/application-system/api/src/app/app.module.ts (2 hunks)
- libs/clients/identity/src/lib/identityClient.module.ts (1 hunks)
- libs/clients/identity/src/lib/identityClient.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/application-system/api/src/app/app.module.ts (1)
Pattern
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."
libs/clients/identity/src/lib/identityClient.module.ts (1)
Pattern
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/identity/src/lib/identityClient.service.ts (1)
Pattern
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."
🔇 Additional comments (6)
libs/clients/identity/src/lib/identityClient.module.ts (2)
4-4: Verify the V3 client compatibility across the application.The upgrade to NationalRegistryV3 client is aligned with the PR objectives to enable system national IDs lookup.
Let's verify the impact of this upgrade:
#!/bin/bash # Check for any remaining V2 imports that might need updating rg -l "NationalRegistryClientModule" --type ts # Check for any direct usage of V2 types or services that might need migration rg -l "NationalRegistryClientService" --type ts
9-9: LGTM! Module configuration properly updated.The module imports are correctly configured with the V3 client while maintaining the CompanyRegistryClientModule integration.
Let's verify the module is properly exported for reuse:
✅ Verification successful
Module is properly exported and used correctly across the codebase
The verification confirms that:
- The module is correctly exported from its source file
- The module is properly imported and used in several domain modules:
- User Profile module
- Identity module
- Auth module
- Template API modules
- No circular dependencies were detected
- The module exports its service as intended
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for module usage across NextJS apps rg -l "IdentityClientModule" --type ts # Verify no circular dependencies were introduced ast-grep --pattern 'import { $_ } from "./identityClient.module"'Length of output: 506
Script:
#!/bin/bash # Check if the module is properly exported from the barrel file rg -l "export.*IdentityClientModule" --type ts # Check the module's exports cat libs/clients/identity/src/lib/identityClient.module.ts # Check for any potential circular dependencies with corrected syntax ast-grep --pattern 'import { $_ } from "./identityClient.module.ts"' # Verify the module usage pattern in dependent files rg "IdentityClientModule" -A 2 -B 2 --type tsLength of output: 5251
apps/application-system/api/src/app/app.module.ts (2)
64-64: LGTM! Clean import statement.The import follows the established pattern and is correctly placed with other client imports.
125-125: Verify if both National Registry versions are needed.Both
NationalRegistryClientConfig(V2) andNationalRegistryV3ClientConfigare loaded in the ConfigModule. Please verify if this is intentional for a gradual migration or if V2 should be removed.Let's check for any remaining V2 dependencies:
✅ Verification successful
The search results show extensive usage of National Registry V2 across multiple applications and services. Let me verify if V3 is also actively used:
Both National Registry versions are currently needed - gradual migration in progress
The search results confirm that both versions are actively used in the codebase:
- V2 is still heavily used in auth-related services, delegations, and various application templates
- V3 is being adopted in newer services and there's an ongoing migration, evidenced by:
- Feature flag
shouldAuthIdsApiUseNationalRegistryV3controlling the migration- Comment in
user-profile.controller.spec.ts: "TODO: Delete this file when shouldAuthIdsApiUseNationalRegistryV3 feature flag is removed"- Presence of both V2 and V3 implementations in several modules, particularly in auth services
The dual presence is intentional for a controlled migration strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining dependencies on National Registry V2 # Test 1: Search for imports of V2 client echo "Checking for V2 imports..." rg -l "from.*@island.is/clients/national-registry-v2" # Test 2: Search for usage of V2 client service echo "Checking for V2 service usage..." rg "NationalRegistryClientService"Length of output: 19006
Script:
#!/bin/bash # Check for V3 usage echo "Checking for V3 imports and usage..." rg -l "from.*@island.is/clients/national-registry-v3" echo "Checking for V3 service usage..." rg "NationalRegistryV3"Length of output: 16838
libs/clients/identity/src/lib/identityClient.service.ts (2)
5-5: Import the new National Registry V3 Client ServiceThe import statement correctly brings in
NationalRegistryV3ClientServicefrom the updated client package.
80-80: Ensure proper error handling with the new service methodThe method
getAllDataIndividualfromNationalRegistryV3ClientServiceis called to retrieve person data. Verify that this method handles errors appropriately and that any exceptions are correctly propagated or caught, especially if the behavior differs from the previous client service.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/clients/identity/src/lib/identityClient.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/clients/identity/src/lib/identityClient.service.ts (1)
Pattern 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."
🔇 Additional comments (3)
libs/clients/identity/src/lib/identityClient.service.ts (3)
5-5: LGTM: Import statement follows best practices
The import statement correctly imports the new V3 client service and follows TypeScript module import conventions.
17-17: LGTM: Clean dependency injection
The constructor properly injects the V3 client service with a clear and descriptive parameter name.
80-82: Consider performance implications of getAllDataIndividual
The method name suggests it fetches all available data for an individual, which might be more than what's needed for identity verification. Consider using a more specific API endpoint if available to optimize payload size.
magnearun
added
the
deprecated:automerge
magnearun
removed
the
deprecated:automerge
magnearun
added
the
deprecated:automerge
* use national-registry-v3 in identity lookup * Adds missing NationalRegistryV3ClientConfig * chore: nx format:write update dirty files * rename service and small type fix --------- Co-authored-by: andes-it <[email protected]>
* use national-registry-v3 in identity lookup * Adds missing NationalRegistryV3ClientConfig * chore: nx format:write update dirty files * rename service and small type fix --------- Co-authored-by: andes-it <[email protected]>
What
Use NationalRegistryV3 for IdentityModule
Why
V3 can look up system nationalId, V2 can not.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation