fix(service-portal): Use full name instead of display name #16497
Conversation
WalkthroughThis pull request introduces several changes related to the handling of user information within the application. A new GraphQL query, 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
|
![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 (6)
libs/api/domains/national-registry/src/lib/shared/models/name.model.ts (1)
16-18: LGTM! Consider adding JSDoc comment for the new field.The addition of the
displayNamefield is well-implemented and aligns with the PR objectives. The field is correctly defined using the@Fielddecorator and follows the same pattern as other fields in the class.Consider adding a JSDoc comment to explain the purpose and usage of the
displayNamefield. This would improve code documentation and maintainability. For example:/** * The display name of the person, which may differ from the full name. * This field can be used for presentation purposes where a shortened or alternate name is preferred. */ @Field(() => String, { nullable: true }) displayName?: string | nulllibs/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.graphql (1)
22-28: Approve new query with minor suggestionThe addition of the
UserInfoOverviewNamequery is a good approach to retrieve only the name information when needed. This aligns well with the PR objective and provides a more focused way to fetch data.Consider adding a comment to explain the purpose of this query for better maintainability:
+# Query to retrieve only the full name of the national registry person query UserInfoOverviewName($useFakeData: Boolean) { nationalRegistryPerson(useFakeData: $useFakeData) { name { fullName } } }libs/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.tsx (2)
116-116: LGTM! Consider adding a fallback for empty name.The change correctly accesses the full name through the new nested structure, aligning with the PR objectives. The use of optional chaining is a good practice for handling potentially undefined properties.
Consider adding a fallback string for cases where the full name might be empty:
-title={data.nationalRegistryPerson?.name?.fullName || ''} +title={data.nationalRegistryPerson?.name?.fullName || 'N/A'}This ensures that a meaningful value is always displayed, improving user experience.
Line range hint
1-143: Overall, the changes look good and adhere to the project guidelines.The modification successfully implements the PR objective of using the full name instead of the display name. The component maintains its reusability across different NextJS apps, effectively uses TypeScript for type safety, and its structure supports efficient tree-shaking and bundling.
Consider extracting the child data fetching logic into a custom hook to improve modularity and reusability. This would separate concerns and make the component easier to test and maintain.
libs/service-portal/information/src/screens/UserInfo/UserInfo.tsx (1)
42-42: LGTM! Consider using null coalescing for consistency.The change correctly implements the use of full name from the national registry, aligning with the PR objectives. Good use of optional chaining to handle potential undefined values.
For consistency with TypeScript best practices, consider using null coalescing (
??) instead of logical OR (||):title={nationalRegistryPerson?.name?.fullName ?? ''}This ensures that only
nullorundefinedvalues are replaced with an empty string, whereas||would also replace falsy values like0orfalse.libs/api/domains/national-registry/src/lib/v3/mapper.ts (1)
93-99: Approve changes with a minor suggestionThe new
nameproperty provides a more structured approach to handling person names, which is a good improvement. The fallback mechanism forfullNameensures data consistency.However, consider adding a comment explaining the difference between
fullNameanddisplayNameto improve code clarity.Consider adding a comment like this:
name: { // ... existing properties ... fullName: individual.fulltNafn?.fulltNafn ?? individual.nafn, displayName: individual.nafn, // Used for display purposes, may differ from fullName },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- apps/portals/my-pages/src/components/Greeting/userInfoData.graphql (1 hunks)
- libs/api/domains/national-registry/src/lib/shared/models/name.model.ts (1 hunks)
- libs/api/domains/national-registry/src/lib/shared/models/personBase.model.ts (1 hunks)
- libs/api/domains/national-registry/src/lib/v3/mapper.ts (1 hunks)
- libs/service-portal/information/src/screens/UserInfo/UserInfo.graphql (1 hunks)
- libs/service-portal/information/src/screens/UserInfo/UserInfo.tsx (2 hunks)
- libs/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.graphql (2 hunks)
- libs/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/portals/my-pages/src/components/Greeting/userInfoData.graphql (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/api/domains/national-registry/src/lib/shared/models/name.model.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/api/domains/national-registry/src/lib/shared/models/personBase.model.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/api/domains/national-registry/src/lib/v3/mapper.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/service-portal/information/src/screens/UserInfo/UserInfo.graphql (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/service-portal/information/src/screens/UserInfo/UserInfo.tsx (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/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.graphql (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/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.tsx (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)
apps/portals/my-pages/src/components/Greeting/userInfoData.graphql (1)
1-7: LGTM! The new query aligns with PR objectives and best practices.The
UserInfoDashboardNamequery is well-structured and consistent with GraphQL best practices. It correctly implements the change from "display-name" to "fullName" as per the PR objectives. The optionaluseFakeDataparameter provides flexibility for different environments.To ensure consistency across the codebase, let's verify the usage of
fullName:libs/api/domains/national-registry/src/lib/shared/models/personBase.model.ts (2)
Line range hint
1-24: Overall, changes align with guidelines and PR objectivesThe modifications to the
PersonBaseclass adhere to the coding guidelines forlibs/**/*files:
- The class remains reusable across different NextJS apps.
- TypeScript is used effectively for defining props and types.
- The structure supports efficient tree-shaking and bundling.
The changes to the
fullNamefield introduce a deprecation notice, which is a good practice for managing API evolution. However, ensure that the apparent contradiction in the deprecation message is addressed and that documentation is updated accordingly.
8-12: Approved changes, but clarification needed on deprecation messageThe addition of the deprecation reason for the
fullNamefield is a good practice and aligns with the PR objectives. It adheres to TypeScript usage and supports effective tree-shaking.However, there seems to be a contradiction:
- The PR aims to use full name instead of display name.
- The deprecation message suggests this field might return a display name.
Could you clarify this apparent contradiction?
Also, consider updating the relevant documentation to reflect these changes and guide developers on using the recommended name object.
To ensure consistency across the codebase, let's check for other occurrences of
fullName:✅ Verification successful
Deprecation of 'fullName' field is valid and intentional
The deprecation of the 'fullName' field in the PersonBase model is part of a controlled refactoring effort within the national-registry domain. The change promotes using a more detailed name object structure, which aligns with best practices for handling personal names.
While 'fullName' is used widely in the codebase, this specific deprecation affects models directly extending PersonBase, primarily within the national-registry domain. The deprecation message provides clear guidance for future development.
Consider updating documentation to reflect this change and plan for gradual migration in affected models.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other occurrences of fullName field in GraphQL types # Expected: Similar deprecation messages or updated usage rg -t ts 'fullName.*Field\(' libsLength of output: 35
Script:
#!/bin/bash # Description: Comprehensive search for `fullName` field across all TypeScript files in libs # Expected: List all occurrences of `fullName` field declarations to ensure consistent deprecation or usage rg -t ts 'fullName' libs --context 3Length of output: 292022
Script:
#!/bin/bash # Search for files that import or extend PersonBase rg "import.*PersonBase|extends PersonBase" --type tsLength of output: 1143
libs/service-portal/information/src/screens/UserInfo/UserInfo.graphql (1)
11-11: LGTM: Addition offullNameto thenameobject aligns with PR objectives.The addition of the
fullNamefield within thenameobject is consistent with the PR's goal of using the full name instead of the display name. This change enhances the query's flexibility by providing access to both individual name components and the full name.To ensure this change is consistently applied across the codebase, let's verify the usage:
libs/service-portal/information/src/screens/UserInfo/UserInfo.tsx (2)
Line range hint
1-231: Overall assessment: Changes align with PR objectives and coding guidelines.The modifications in this file successfully implement the use of full name instead of display name, as per the PR objectives. The code adheres to TypeScript best practices and maintains consistency with the coding guidelines for the
libs/**/*pattern.Key points:
- Reusability of the component is preserved.
- TypeScript usage for props and types is maintained.
- Changes don't negatively impact tree-shaking or bundling.
63-63: LGTM! Verify data structure consistency.The change correctly implements the use of full name from the national registry, aligning with the PR objectives. Good use of optional chaining and null coalescing.
To ensure consistency across the codebase, let's verify that this new structure (
name.fullName) is used consistently:This will help identify any places where the old structure might still be in use and need updating.
libs/api/domains/national-registry/src/lib/v3/mapper.ts (2)
Line range hint
293-293: LGTM: Consistent fullName handlingThe update to the
fullNameproperty in theformatUserfunction aligns well with the changes made informatPerson. This ensures consistency in how full names are handled across the codebase and supports the PR objective of using full names.
Line range hint
1-393: Overall assessment: Changes align with guidelines and improve code qualityThe modifications in this file enhance the handling of user names, providing a more structured and consistent approach across different functions. These changes align well with the PR objectives and adhere to the coding guidelines for
libs/**/*files:
- Reusability: The new
nameproperty informatPersonand the updatedfullNameinformatUserimprove consistency and reusability across different NextJS apps.- TypeScript usage: The changes maintain proper TypeScript usage for defining props and exporting types.
- Tree-shaking and bundling: The modifications don't introduce any obvious issues that would affect tree-shaking or bundling practices.
These improvements contribute to better maintainability and consistency in handling user information throughout the application.
libs/service-portal/information/src/screens/UserInfoOverview/UserInfoOverview.graphql
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16497 +/- ##
==========================================
- Coverage 36.77% 36.75% -0.02%
==========================================
Files 6847 6810 -37
Lines 141871 140920 -951
Branches 40422 40105 -317
==========================================
- Hits 52167 51793 -374
+ Misses 89704 89127 -577
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 270 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
What
Use full name instead of display name
Why
We are currently using display-name (birtingarnafn) in a few places where we should be using fullName.
Checklist:
Summary by CodeRabbit
New Features
UserInfoDashboardNamefor enhanced user information retrieval.UserInfoOverviewNameto fetch thefullNamemore efficiently.Improvements
fullName, now nested within anameobject for various entities.UserInfoandUserInfoOverviewcomponents to reflect the updated data structure.Bug Fixes
SubjectInfoandInfoLinecomponents for better accuracy.