-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(user-profile): Remove user-profile V1 from api layer #17249
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request involves significant refactoring of user profile management services and components. The changes primarily focus on removing version 1 (V1) of the user profile service, simplifying verification processes, and streamlining the user profile functionality. Key modifications include deleting specific verification-related methods, removing DTOs for SMS and email verification, and updating the service and resolver to work exclusively with version 2 (V2) of the user profile service. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProfileService
participant UserProfileServiceV2
User->>ProfileService: Request user profile
ProfileService->>UserProfileServiceV2: Get user profile
UserProfileServiceV2-->>ProfileService: Return user profile
ProfileService-->>User: Display user profile
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libs/portals/my-pages/information/src/components/PersonalInformation/Forms/ProfileForm/ProfileForm.tsx (1)
Line range hint
1-324
: Consider breaking down this large componentThe ProfileForm component has grown quite large and handles multiple responsibilities (email, phone, bank info, paper mail, modals). Consider splitting it into smaller, more focused components to improve maintainability.
Suggested structure:
// CompanyProfileForm.tsx const CompanyProfileForm = () => { // Company-specific logic and editable inputs } // IndividualProfileForm.tsx const IndividualProfileForm = () => { // Individual-specific logic and read-only inputs } // ProfileForm.tsx const ProfileForm = () => { const isCompany = userInfo?.profile?.subjectType === 'legalEntity' return isCompany ? <CompanyProfileForm /> : <IndividualProfileForm /> }libs/api/domains/user-profile/src/lib/userProfile.service.ts (1)
4-5
: Unused importAuth
from@island.is/auth-nest-tools
The imported
Auth
is not used within theuserProfile.service.ts
file. Unused imports can lead to unnecessary code bloat.Consider removing the unused import:
-import { Auth, AuthMiddleware, User } from '@island.is/auth-nest-tools' +import { AuthMiddleware, User } from '@island.is/auth-nest-tools'libs/api/domains/user-profile/src/lib/userProfile.resolver.ts (1)
16-18
: Review the necessity of imported models and inputsThe imports for
UpdateUserProfileInput
,UserDeviceTokenInput
, andDeleteIslykillSettings
should be used within the resolver. Ensure that these are utilized; otherwise, consider removing unused imports to keep the code clean.libs/api/domains/user-profile/src/lib/V2/userProfile.service.ts (1)
3-6
: Optimize imports from@island.is/clients/user-profile
Consider importing all necessary enums and classes in a single import statement to enhance readability.
import { Auth, AuthMiddleware, User } from '@island.is/auth-nest-tools' -import { - PostNudgeDtoNudgeTypeEnum, - UserProfileControllerFindUserProfileClientTypeEnum, - V2MeApi, - V2UsersApi, -} from '@island.is/clients/user-profile' +import * as UserProfileClient from '@island.is/clients/user-profile' ... + private v2MeApi: UserProfileClient.V2MeApi, + private v2UserProfileApi: UserProfileClient.V2UsersApi, ... + nudgeType: UserProfileClient.PostNudgeDtoNudgeTypeEnum.NUDGE, ... + clientType: UserProfileClient.UserProfileControllerFindUserProfileClientTypeEnum.FirstParty,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
libs/api/domains/user-profile/src/lib/V1/userProfile.service.ts
(0 hunks)libs/api/domains/user-profile/src/lib/V2/userProfile.service.ts
(2 hunks)libs/api/domains/user-profile/src/lib/dto/confirmEmailVerificationInput.ts
(0 hunks)libs/api/domains/user-profile/src/lib/dto/confirmSmsVerificationInput.ts
(0 hunks)libs/api/domains/user-profile/src/lib/userProfile.module.ts
(0 hunks)libs/api/domains/user-profile/src/lib/userProfile.resolver.ts
(2 hunks)libs/api/domains/user-profile/src/lib/userProfile.service.ts
(2 hunks)libs/portals/my-pages/graphql/src/hooks/profile/useResendEmailVerification.ts
(0 hunks)libs/portals/my-pages/graphql/src/index.ts
(0 hunks)libs/portals/my-pages/graphql/src/lib/mutations/resendEmailVerification.ts
(0 hunks)libs/portals/my-pages/information/src/components/PersonalInformation/Forms/ProfileForm/ProfileForm.tsx
(2 hunks)
💤 Files with no reviewable changes (7)
- libs/portals/my-pages/graphql/src/index.ts
- libs/portals/my-pages/graphql/src/lib/mutations/resendEmailVerification.ts
- libs/api/domains/user-profile/src/lib/userProfile.module.ts
- libs/api/domains/user-profile/src/lib/dto/confirmSmsVerificationInput.ts
- libs/api/domains/user-profile/src/lib/dto/confirmEmailVerificationInput.ts
- libs/portals/my-pages/graphql/src/hooks/profile/useResendEmailVerification.ts
- libs/api/domains/user-profile/src/lib/V1/userProfile.service.ts
🧰 Additional context used
📓 Path-based instructions (4)
libs/portals/my-pages/information/src/components/PersonalInformation/Forms/ProfileForm/ProfileForm.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/api/domains/user-profile/src/lib/userProfile.resolver.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/user-profile/src/lib/V2/userProfile.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/api/domains/user-profile/src/lib/userProfile.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 (11)
libs/portals/my-pages/information/src/components/PersonalInformation/Forms/ProfileForm/ProfileForm.tsx (3)
Line range hint 195-224
: LGTM: Email input section changes align with V1 removal
The conditional rendering now correctly differentiates between company and individual users, providing appropriate input components for each case. Companies can edit directly while individuals use the IDS user profile system.
Line range hint 254-283
: LGTM: Phone input section changes align with V1 removal
Similar to the email section, the phone number management has been updated to use the isCompany
check for determining edit permissions.
Line range hint 1-324
: Verify the impact of V1 removal on feature flags
Since we're removing the V1 user profile feature flag, let's verify there are no remaining references to it.
libs/api/domains/user-profile/src/lib/userProfile.service.ts (4)
69-70
: Ensure consistent return types for getUserProfile
The method getUserProfile
returns a value from userProfileServiceV2.getUserProfile(user)
. Verify that the returned value aligns with the UserProfile
type defined in your models, ensuring type safety and consistency.
95-100
:
Missing user
parameter in createSmsVerification
method signature
The method createSmsVerification
calls this.userProfileServiceV2.createSmsVerification(input, user)
but user
is not defined in the method scope due to the missing parameter.
Add user
to the method parameters:
async createSmsVerification(
input: CreateSmsVerificationInput,
+ user: User,
): Promise<void> {
return this.userProfileServiceV2.createSmsVerification(input, user)
}
Likely invalid or redundant comment.
73-77
:
Missing user
parameter in createUserProfile
method signature
The createUserProfile
method uses the user
object but does not include it in the method's parameters. This will cause a ReferenceError
since user
is undefined within the method scope.
Please update the method signature to include the user
parameter:
async createUserProfile(
input: CreateUserProfileInput,
+ user: User,
): Promise<UserProfile> {
return this.userProfileServiceV2.createUserProfile(input, user)
}
Likely invalid or redundant comment.
87-92
:
Missing user
parameter in updateUserProfile
method signature
The updateUserProfile
method references user
but does not include it in its parameters. This will result in a ReferenceError
at runtime.
Include the user
parameter in the method signature:
async updateUserProfile(
nationalId: string,
input: UpdateUserProfileInput,
+ user: User,
): Promise<UserProfile> {
return this.userProfileServiceV2.updateUserProfile(input, user, nationalId)
}
Likely invalid or redundant comment.
libs/api/domains/user-profile/src/lib/userProfile.resolver.ts (3)
2-2
: Correct import path for Args
, Mutation
, Query
, and Resolver
Ensure that the import path for @nestjs/graphql
is correct and that these imports are necessary for the resolver's functionality.
12-12
: Verify the import of CreateSmsVerificationInput
Confirm that CreateSmsVerificationInput
is correctly defined in the specified path and that it matches the expected structure for the input.
20-22
: Confirm the usage of imported models
Verify that Response
, UserDeviceToken
, and UserProfile
are correctly used in the resolver methods and correspond to the GraphQL schema definitions.
libs/api/domains/user-profile/src/lib/V2/userProfile.service.ts (1)
172-192
: Ensure proper error handling and response in deleteIslykillValue
method
The deleteIslykillValue
method updates the user profile and checks for the presence of nationalId
. Ensure that:
- The
updateMeUserProfile
method returns an object containingnationalId
. - The error handling correctly captures scenarios where the update might fail.
- The returned
DeleteIslykillSettings
object aligns with the expected structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17249 +/- ##
==========================================
+ Coverage 35.71% 35.74% +0.03%
==========================================
Files 6944 6935 -9
Lines 148537 148093 -444
Branches 42405 42208 -197
==========================================
- Hits 53047 52935 -112
+ Misses 95490 95158 -332
Flags with carried forward coverage won't be shown. Click here to find out more. see 68 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 4 Passed, 0 Skipped, 2.57s Total Time |
What
Remove v1 from user-profile graphql and only reference v2 and remove feature flag that switched between them
Why
V1 has become redundant
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation