feat(auth-admin): Use birtNafn instead of fulltNafn #16684
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (1)
libs/clients/national-registry/v3/src/clientConfig.json (1)
210-210: Update registration date in example response.The example response shows a future date (
2024-10-31). Consider using a more realistic past or current date to avoid confusion.-"nyskraningDagur": "2024-10-31T10:33:19.2136626+00:00" +"nyskraningDagur": "2023-10-31T10:33:19.2136626+00:00"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts(2 hunks)libs/clients/national-registry/v3/src/clientConfig.json(8 hunks)libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.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/clients/national-registry/v3/src/clientConfig.json (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/national-registry/v3/src/lib/nationalRegistryV3.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 (7)
libs/clients/national-registry/v3/src/lib/nationalRegistryV3.service.ts (2)
16-16: LGTM! Type import aligns with the new name field requirement.
The import of EinstaklingurDTONafnItar is correctly added to support the transition from fulltNafn to birtNafn.
Line range hint 100-105: LGTM! Method signature and implementation are consistent with the service pattern.
The changes maintain:
- Consistent error handling using
handle204 - Type safety with the new
EinstaklingurDTONafnItarreturn type - Proper API endpoint usage with
NafnItarGetRaw
Let's verify the consistency of this change across the codebase:
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts (3)
9-9: LGTM: Import statement follows NestJS conventions
The addition of EinstaklingurDTONafnItar type maintains proper import organization.
480-480: Consider renaming method for clarity
While changing from fulltNafn to birtNafn aligns with the PR objectives, the method name getFullName might be misleading now that it returns birtNafn. Consider renaming it to getDisplayName or similar for better clarity.
#!/bin/bash
# Description: Verify the birtNafn property exists in the DTO and check for similar method names
# Check the DTO definition
echo "Checking DTO definition for birtNafn property:"
ast-grep --pattern 'interface EinstaklingurDTONafnItar {
$$$
birtNafn: $_
$$$
}'
# Look for similar method names for consistency
echo -e "\nChecking for similar name-related methods:"
rg "get.*Name" --type ts472-472: Verify type compatibility across the codebase
The type change from EinstaklingurDTONafnAllt to EinstaklingurDTONafnItar looks correct. Let's verify its usage across the codebase.
✅ Verification successful
Let's search for the interface definition and related name methods to understand the type compatibility and usage patterns. This will help verify if the type change is consistent with the codebase.
Let's search for both type definitions and their usages to understand the type change and verify its compatibility.
Type change is safe and consistent with the codebase
The change from EinstaklingurDTONafnAllt to EinstaklingurDTONafnItar in the notifications worker service aligns with the type used in the national registry V3 service (nationalRegistryV3.service.ts). The national registry service's getName() method specifically returns EinstaklingurDTONafnItar, making this type change a necessary alignment with the underlying service implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of EinstaklingurDTONafnAllt that might need updating
# Search for any remaining references to the old type
echo "Checking for remaining usages of EinstaklingurDTONafnAllt:"
rg "EinstaklingurDTONafnAllt" --type ts
# Search for all usages of the new type to ensure consistency
echo -e "\nChecking usages of new type EinstaklingurDTONafnItar:"
rg "EinstaklingurDTONafnItar" --type ts
Length of output: 88227
Script:
#!/bin/bash
# Search for EinstaklingurDTONafnItar interface definition and usage
echo "Checking DTO definition for birtNafn property:"
ast-grep --pattern 'interface EinstaklingurDTONafnItar {
$$$
birtNafn: $_
$$$
}'
echo -e "\nChecking for similar name-related methods:"
rg 'get.*Name' --type ts
Length of output: 88132
Script:
#!/bin/bash
# Search for EinstaklingurDTONafnItar and EinstaklingurDTONafnAllt definitions and usages
echo "Checking for EinstaklingurDTONafnItar interface definition and usage:"
ast-grep --pattern 'interface EinstaklingurDTONafnItar {
$$$
}'
echo -e "\nChecking for EinstaklingurDTONafnAllt interface definition and usage:"
ast-grep --pattern 'interface EinstaklingurDTONafnAllt {
$$$
}'
echo -e "\nChecking for direct usages of both types:"
rg "EinstaklingurDTONafnItar|EinstaklingurDTONafnAllt" --type ts
Length of output: 1385
libs/clients/national-registry/v3/src/clientConfig.json (2)
28-28: LGTM: Example responses are consistent with the schema.
The example responses across different endpoints consistently use the nafn field to represent individual names, which aligns with the schema definitions.
Also applies to: 47-47, 171-171, 190-190, 217-217, 229-229, 1136-1136
Line range hint 1467-1478: Verify the schema changes align with the PR objectives.
The schema for EinstaklingurDTONafnItar includes both fulltNafn and birtNafn fields, which aligns with the PR objective of transitioning from fulltNafn to birtNafn. However, we should verify that all consumers of this API are updated to handle both fields correctly.
Datadog ReportAll test runs ✅ 101 Total Test Services: 0 Failed, 98 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/mocks.ts (1)
222-222: Consider differentiating mock values forfulltNafnandbirtNafnCurrently, both
fulltNafnandbirtNafnuse the same value (user?.nameor mock value). In real-world scenarios, these fields might contain different values. Consider updating the mock data structure to better reflect this potential difference, ensuring more accurate test coverage.Example improvement:
getName(nationalId: string) { const user = userProfiles.find((u) => u.nationalId === nationalId) return { - fulltNafn: user?.name ?? mockFullName, - birtNafn: user?.name ?? mockBirtNafn, + fulltNafn: user?.name ?? mockFullName, + birtNafn: user?.name?.split(' ')[0] ?? mockBirtNafn, // Use first name as birtNafn } }apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts (1)
Test coverage for birtNafn is missing and should be added
The code shows that
birtNafnis being used from the National Registry service response, but the current test assertions only verify the presence of emails and notifications without explicitly checking the name field's content. The test should be updated to verify thatbirtNafnis used instead offulltNafn.
- In
notificationsWorker.service.spec.ts, update the email verification assertions to check thatbirtNafnfromMockNationalRegistryV3ClientServiceis used in the email content.- Add specific test cases to verify the behavior when
birtNafnis different fromfulltNafn.Example test assertion to add:
expect(emailService.sendEmail).toHaveBeenCalledWith( expect.objectContaining({ to: expect.objectContaining({ name: mockBirtNafn, // verify birtNafn is used instead of mockFullName }), }), )🔗 Analysis chain
Line range hint
1-600: Add test coverage for birtNafn functionality.While the test suite is comprehensive, it should be extended to explicitly verify that notifications are using
birtNafninstead offulltNafnin the email content, as this is a key change in this PR.Consider adding a test case that verifies the correct name field is used in notifications:
it('should use birtNafn instead of fulltNafn in notifications', async () => { await addToQueue(userWithNoDelegations.nationalId) expect(emailService.sendEmail).toHaveBeenCalledWith( expect.objectContaining({ to: expect.objectContaining({ name: 'expectedBirtNafn', // verify correct name field is used }), }), ) })
Let me gather more specific information about the birtNafn implementation.
Let me check the actual implementation of the notification service to see how names are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for birtNafn vs fulltNafn usage # Check if there are any existing tests for birtNafn rg -l 'birtNafn' --type ts # Check test assertions related to name fields rg -A 3 'expect\(.+name' --type tsLength of output: 25386
Script:
#!/bin/bash # Check the implementation of createEmail and related code in notificationsWorker.service.ts ast-grep --pattern 'createEmail($$$) { $$$ }' # Check the mock data to see how birtNafn is used cat apps/services/user-notification/src/app/modules/notifications/notificationsWorker/mocks.tsLength of output: 7010
Script:
#!/bin/bash # Check the implementation of the notification service rg -A 10 "createEmail|getName" apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.tsLength of output: 1074
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/mocks.ts(2 hunks)apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/mocks.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/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.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."
🔇 Additional comments (2)
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/mocks.ts (1)
18-18: LGTM: New mock constant follows existing patterns
The addition of mockBirtNafn constant aligns with the PR objectives and follows the same pattern as the existing mockFullName constant.
apps/services/user-notification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.spec.ts (1)
239-239: LGTM! Good defensive programming practice.
The addition of optional chaining (?.) for accessing recipientMessage.id improves code robustness by safely handling potential undefined values.
...otification/src/app/modules/notifications/notificationsWorker/notificationsWorker.service.ts
Show resolved
Hide resolved
What
use "birtNafn" instead of "fulltNafn" in emails
Why
In emails we want to display "birtNafn" instead of "fulltNafn"
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation