feat(ids-api): Add alive check to legal representative delegation type #16284
Conversation
WalkthroughThe changes introduce the Changes
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
|
valurefugl
changed the title
valurefugl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
libs/auth-api-lib/src/lib/delegations/alive-status.service.ts (2)
99-101: Consider inlining the error handler for simplicityThe
handlerGetErrormethod simply returns the error object without any additional processing. You can inline the error handling in thecatchblocks to simplify the code.You can replace
.catch(this.handlerGetError)with.catch((error) => error)directly in the promises.
58-97: Consider removing the outer try-catch block for claritySince each individual promise handles its own errors by returning an error object, the
Promise.allcall will not reject, and the outertry-catchblock might be unnecessary. Removing it could simplify the code and improve readability.libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
Line range hint
232-285: Refactor duplicated logic for checking alive status into a separate method.The logic for checking the alive status of delegations and handling deceased delegations is duplicated in both
findAllIncomingGeneralMandatesandfindAllIncomingmethods. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this logic into a private helper method.Here's how you might refactor the code:
private async filterAliveDelegations(delegations: Delegation[]): Promise<{ delegations: Delegation[]; fromNameInfo: NameInfo[] }> { // Check live status, i.e., dead or alive for delegations const { aliveNationalIds, deceasedNationalIds, aliveNameInfo } = await this.aliveStatusService.getStatus( delegations.map((d) => d.fromNationalId), ); if (deceasedNationalIds.length > 0) { const deceasedDelegations = delegations.filter((d) => deceasedNationalIds.includes(d.fromNationalId), ); // Delete all deceased delegations by deleting them and their scopes. const deletePromises = deceasedDelegations.map((delegation) => delegation.destroy(), ); await Promise.all(deletePromises); this.auditService.audit({ action: 'deleteDelegationsForMissingPeople', resources: deceasedDelegations.map(({ id }) => id).filter(isDefined), system: true, }); } return { delegations: delegations.filter((d) => aliveNationalIds.includes(d.fromNationalId), ), fromNameInfo: aliveNameInfo, }; }Then, modify the original methods to use this helper:
async findAllIncomingGeneralMandates(...) { // Existing code to fetch delegations const delegations = await this.delegationModel.findAll({ ... }); - // Original logic duplicated here - // [Code for checking alive status] + // Use the new helper method + const { delegations: aliveDelegations, fromNameInfo } = await this.filterAliveDelegations(delegations); // Continue processing with aliveDelegations and fromNameInfo } async findAllIncoming(...) { // Existing code to fetch delegations const delegations = await this.delegationModel.findAll({ ... }); + // Use the new helper method + const { delegations: aliveDelegations, fromNameInfo } = await this.filterAliveDelegations(delegations); // Continue processing with aliveDelegations and fromNameInfo }Also applies to: 335-363
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/auth-api-lib/src/lib/delegations/alive-status.service.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (9 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming-representative.service.ts (3 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (5 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations.module.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/auth-api-lib/src/lib/delegations/alive-status.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."
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.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."
libs/auth-api-lib/src/lib/delegations/delegations-incoming-representative.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."
libs/auth-api-lib/src/lib/delegations/delegations-incoming.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."
libs/auth-api-lib/src/lib/delegations/delegations.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."
🔇 Additional comments (8)
libs/auth-api-lib/src/lib/delegations/delegations.module.ts (3)
12-12: LGTM: Import organization improvementThe relocation of the FeatureFlagModule import improves code organization by grouping external imports together.
16-16: LGTM: Import organization improvementThe relocation of the environment import improves code organization by grouping internal imports together.
26-26: Verify the implementation of AliveStatusServiceThe addition of AliveStatusService to the providers array aligns with the PR objective to refactor the alive status check into a separate service. This change looks good, but let's verify its implementation.
Let's check the implementation of AliveStatusService:
Also applies to: 87-87
✅ Verification successful
AliveStatusService implementation verified successfully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of AliveStatusService # Test 1: Check if AliveStatusService file exists echo "Checking if AliveStatusService file exists:" fd -t f "alive-status.service.ts" # Test 2: Inspect the content of AliveStatusService echo "Inspecting the content of AliveStatusService:" ast-grep --lang typescript --pattern $'class AliveStatusService { $$$ }' # Test 3: Check for usage of AliveStatusService in other files echo "Checking for usage of AliveStatusService in other files:" rg "AliveStatusService" --type tsLength of output: 10397
libs/auth-api-lib/src/lib/delegations/alive-status.service.ts (1)
15-18: Proper use of TypeScript for defining typesThe
NameInfotype is well-defined, enhancing type safety and code readability.libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (2)
17-23: Ensure TypeScript types are defined and exported correctly.The imports of
AliveStatusService,NameInfo, and related models are appropriate. This aligns with the coding guidelines for TypeScript usage in defining and exporting types, enhancing reusability across different NextJS apps.
46-46: Dependency injection ofAliveStatusServiceis correctly implemented.The
aliveStatusServiceis properly injected into the constructor, ensuring that the service is available throughout the class. This adheres to best practices for dependency injection in NestJS.libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (2)
409-415: Well-implemented method for retrieving name from NameInfoThe
getNameFromNameInfomethod efficiently retrieves the name associated with a national ID, defaulting toUNKNOWN_NAMEif not found. This adheres to good TypeScript practices and enhances code readability.
362-367: Ensure 'aliveNationalIds' is not empty before filteringIf
aliveNationalIdsis empty or undefined, the subsequent filtering may not behave as expected. Confirm thataliveNationalIdscontains the expected data before using it to filter records.Run the following script to check if
aliveNationalIdsis ever empty:✅ Verification successful
'aliveNationalIds' is properly populated before filtering. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for instances where 'aliveNationalIds' might be empty. # Test: Look for assignments to 'aliveNationalIds' and check usage. rg --type typescript -A 3 'const { aliveNationalIds' | rg 'length === 0'Length of output: 189
Script:
#!/bin/bash # Description: Search for instances where 'aliveNationalIds' might be empty. # Corrected Test: Look for assignments to 'aliveNationalIds' and check usage. rg --type typescript -A 3 'const \{ aliveNationalIds' | rg 'length === 0'Length of output: 110
Script:
#!/bin/bash # Description: Search for instances where 'aliveNationalIds' might be empty. # Corrected Test: Look for assignments to 'aliveNationalIds' and check usage. rg --type ts -A 3 'const \{ aliveNationalIds' | rg 'length === 0'Length of output: 67
Script:
#!/bin/bash # Description: Search for instances where 'aliveNationalIds' might be empty. # Corrected Test: Look for assignments to 'aliveNationalIds' and check usage. rg -g "*.ts" -F 'aliveNationalIds.length === 0' -A 3Length of output: 54
libs/auth-api-lib/src/lib/delegations/delegations-incoming-representative.service.ts
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16284 +/- ##
==========================================
+ Coverage 35.55% 35.58% +0.03%
==========================================
Files 6931 6916 -15
Lines 146661 147219 +558
Branches 41695 42361 +666
==========================================
+ Hits 52139 52388 +249
- Misses 94522 94831 +309
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 172 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 68 Total Test Services: 0 Failed, 66 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (6)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (1)
337-341: Consider adding error handling for AliveStatusService.getStatus callWhile the implementation is correct, it's important to handle potential errors that may occur when calling
aliveStatusService.getStatus. This will improve the robustness of the code.Consider wrapping the call in a try-catch block:
+ try { const { aliveNationalIds, deceasedNationalIds, aliveNameInfo } = await this.aliveStatusService.getStatus( uniq(records.map((d) => d.fromNationalId)), ) + } catch (error) { + logger.error('Failed to retrieve alive status', { error }) + // Handle the error appropriately, e.g., return an empty array or rethrow + throw new Error('Unable to retrieve alive status') + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/auth-api-lib/src/lib/delegations/delegations-incoming.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 (4)
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (4)
19-19: LGTM: Import changes align with service updatesThe import changes correctly reflect the introduction of
AliveStatusServiceandNameInfo, while removing the no longer usedNationalRegistryClientService. This aligns well with the refactoring of the alive status check into a separate service.
67-67: LGTM: Constructor updated to use AliveStatusServiceThe constructor has been correctly updated to inject
AliveStatusServiceinstead ofNationalRegistryClientService. This change is consistent with the refactoring goal of moving the alive status check into a separate service.
337-341: LGTM: Implemented alive status check using AliveStatusServiceThe implementation correctly uses the new
AliveStatusServiceto retrieve the alive/deceased status of individuals. This aligns with the PR objective of refactoring the alive status check into a separate service.
413-419: LGTM: Efficient implementation of getNameFromNameInfo methodThe new
getNameFromNameInfomethod is well-implemented, using optional chaining (?.) and nullish coalescing (??) operators for safe and concise property access. It correctly handles the case where a name might not be found, falling back toUNKNOWN_NAME. This implementation aligns well with modern JavaScript best practices.
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts
Outdated
Show resolved
Hide resolved
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts
Outdated
Show resolved
Hide resolved
valurefugl
added
the
deprecated:automerge
https://www.notion.so/Add-deceased-check-using-National-Registry-1115a76701d68092bb72fec8a14a94c4?pvs=4
What
Refactor alive status check into separate service. Add check to legal representative delegation type.
Enable using national registry v3 api for the check.
Why
So we don't show delegations for deceased individuals.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
AliveStatusServicefor determining the alive or deceased status of individuals and companies based on national IDs.Improvements
Bug Fixes
Documentation
AliveStatusService, making it available for dependency injection.