feat(auth-admin): only update/delete delegation index of the types that are being indexed. #16627
Conversation
Otherwise we are deleting all that are of a type not in the current array being indexed
WalkthroughThe pull request introduces enhancements to the delegation indexing functionality by adding a new test case for custom delegations with personal representatives, and a new test suite for indexing custom delegations without overriding existing ones. Additionally, modifications to 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
|
Datadog ReportAll test runs ✅ 13 Total Test Services: 0 Failed, 13 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16627 +/- ##
==========================================
+ Coverage 36.53% 36.60% +0.07%
==========================================
Files 6890 6877 -13
Lines 143641 142995 -646
Branches 40926 40715 -211
==========================================
- Hits 52482 52347 -135
+ Misses 91159 90648 -511
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 38 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
magnearun
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: 1
🧹 Outside diff range and nitpick comments (4)
apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts (1)
52-76: LGTM! Well-structured test case for combined delegation types.
The new test case effectively validates the coexistence of custom delegations and personal representatives. The implementation:
- Correctly configures the client with both delegation type supports
- Properly structures the test data with both delegation types
- Clearly defines the expected outcomes
Consider adding a brief comment above the test case to document its purpose:
// Validates that custom delegations can coexist with personal representative
// delegations without interfering with each other
customAndPersonalRepresentative: new TestCase(
// ... rest of the implementation
),apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.service.spec.ts (2)
509-509: Fix typo in describe block name
The describe block name contains a typo: "Exisiting" should be "Existing".
- describe('indexCustomDelegationWithExisitingDelegations', () => {
+ describe('indexCustomDelegationWithExistingDelegations', () => {516-535: LGTM! Consider adding negative test case
The test case effectively verifies that indexing custom delegations preserves existing representative delegations, which directly addresses the PR objective. The test structure follows NestJS testing best practices with clear arrange-act-assert patterns.
Consider adding a negative test case to verify the previous behavior where delegations were being overridden, to ensure the fix is working as intended.
Example negative test structure:
it('should not delete existing delegations when indexing custom delegations', async () => {
const nationalId = user.nationalId;
// Arrange - Index representative delegations first
await delegationIndexService.indexRepresentativeDelegations(nationalId, user);
const delegationsBeforeCustom = await delegationIndexModel.findAll({
where: { toNationalId: nationalId },
});
// Act - Index custom delegations
await delegationIndexService.indexCustomDelegations(nationalId, user);
// Assert - Verify representative delegations still exist
const delegationsAfter = await delegationIndexModel.findAll({
where: {
toNationalId: nationalId,
type: AuthDelegationType.PersonalRepresentative
},
});
expect(delegationsAfter.length).toEqual(delegationsBeforeCustom.length);
});libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (1)
Line range hint 432-436: Replace console.error with injected logger.
The error handling currently uses console.error, but the service has a logger injected through the constructor. For consistency and better error tracking, consider using the injected logger.
indexingPromises.forEach((p) => {
if (p.status === 'rejected') {
- console.error(p.reason)
+ this.logger.error('Failed to save delegations to index', {
+ error: p.reason,
+ context: 'DelegationsIndexService.saveToIndex'
+ })
}
})📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts (1 hunks)
- apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.service.spec.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts (2)
Pattern 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."
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."
apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index.service.spec.ts (2)
Pattern 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."
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/auth-api-lib/src/lib/delegations/delegations-index.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 (2)
apps/services/auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts (1)
Line range hint 1-76: Verify test coverage for delegation scenarios.
Let's ensure we have comprehensive test coverage for all delegation scenarios.
✅ Verification successful
Test coverage for delegation scenarios is comprehensive and well-structured
The test file under review is part of a comprehensive test suite that covers all delegation types and scenarios:
- Test cases cover all supported delegation types: Custom, LegalGuardian, ProcurationHolder, PersonalRepresentative, LegalRepresentative, GeneralMandate
- Age-based scenarios are tested with different child ages (under 16, exactly 16, 17-18, exactly 18)
- Multiple test files validate delegation functionality across different APIs and contexts:
- Delegation indexing and filtering
- Access control (incoming/outgoing)
- Authentication and authorization
- Client-specific delegation rules
- Admin operations
The test cases in this file specifically focus on delegation indexing with proper test data setup and expected outcomes for each scenario.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related test files and coverage
# Look for related test files
echo "Looking for related test files..."
fd -e ts -e spec.ts "delegation.*test|spec" "apps/services/auth"
# Check for additional delegation types in the codebase
echo -e "\nChecking for all delegation types..."
rg "AuthDelegationType\." "apps/services/auth"
Length of output: 47021
libs/auth-api-lib/src/lib/delegations/delegations-index.service.ts (1)
394-401: LGTM! The changes effectively fix the delegation indexing issue.
The introduction of the types variable and its usage in the where clause ensures that only records of the same types as the incoming delegations are considered during the indexing process. This prevents the unintended deletion of other delegation types when indexing a specific type.
However, let's verify that this change doesn't have any unintended side effects:
...auth/delegation-api/src/app/delegations/test/delegation-index/delegation-index-test-cases.ts
Show resolved
Hide resolved
magnearun
added
the
deprecated:automerge
magnearun
removed
the
deprecated:automerge
magnearun
added
the
deprecated:automerge
…at are being indexed. (#16627) * Only update/delete delegation index of the types that are being indexed. Otherwise we are deleting all that are of a type not in the current array being indexed * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is>
…at are being indexed. (#16627) * Only update/delete delegation index of the types that are being indexed. Otherwise we are deleting all that are of a type not in the current array being indexed * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is>
What
When we save to index we send an array of delegations, some times of all types, sometimes of just one type, when the latter happens we are deleting all pre existing rows that are not of that type, since the function checks if the row is in the incoming array, if it was not in the index it is created, if it was in the index but also in the incoming array it is updated, if it is in the index but not in the incoming it is deleted. So when indexing only custom delegations we would be deleting all rows of other types since they will not be in the oncoming array.
Why
Have correct data in the index table
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes