test: test for opt in and connect card #3406
Conversation
WalkthroughThalaiva style 🌟 This pull request is a superstar transformation of the corporate card enrollment process! We've supercharged the user experience with enhanced error handling, refined input management, and sleek UI improvements across multiple components. The changes focus on making card enrollment smoother than my signature style - with better feedback, more robust error messages, and a more intuitive flow that'll make users go "Wow!" 💥 Changes
Possibly related PRs
Suggested Reviewers
Poem
🪧 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
CodeRabbit Configuration File (
|
| @@ -183,68 +184,76 @@ export class SpenderOnboardingOptInStepComponent implements OnInit, OnChanges { | |||
| } | |||
| } | |||
|
|
|||
| handleOtpSuccess(otpDetails: Partial<OtpDetails>, action: string): void { | |||
There was a problem hiding this comment.
refactored this and added tests from older component
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html(5 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.scss(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(6 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.spec.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts(2 hunks)src/app/shared/components/sidemenu/sidemenu.component.spec.ts(9 hunks)
🧰 Additional context used
🪛 eslint
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 3-3: 'ChangeDetectorRef' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[failure] 3-3:
'ChangeDetectorRef' is defined but never used
🪛 GitHub Actions: Lint
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 3-3: 'ChangeDetectorRef' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (5)
src/app/shared/components/sidemenu/sidemenu.component.spec.ts (1)
463-464: Mind it! The setupSideMenu test is now updated with the new parameters!The test has been updated to include the new parameters in the spy method calls, maintaining consistency with the component changes.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.spec.ts (1)
162-162: Same issue here, let's set it right!Again,
expectwithout an assertion won't do the trick. Let's callvalidateInput()properly.Apply the same fix:
- expect(component.validateInput()); + component.validateInput();src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html (3)
Line range hint
8-74: Superstar move on the error handling, partner!The way you've handled card validation and enrollment errors is top-notch! The error messages are clear and the form validation is tight as a drum.
155-157: Style ah? Perfect error handling for single card!The single card error handling matches the multiple cards style. That's what I call maintaining consistency!
Line range hint
1-196: Let's verify the form state handling, shall we?The form structure looks solid, but we should verify the form state handling during card enrollment.
✅ Verification successful
Mind it! The form state handling is perfect, like a well-choreographed action sequence!
The component is handling form states with style, macha! Let me break it down:
- Loading states ✓
- Enrollment states ✓
- Dynamic form controls ✓
- Error handling ✓
- User feedback ✓
No changes needed here. This code is already a superstar!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper form state handling in the component ast-grep --pattern 'fg.disable()' || ast-grep --pattern 'this.fg.disable()' # Check for form reset after enrollment ast-grep --pattern 'fg.reset()' || ast-grep --pattern 'this.fg.reset()'Length of output: 69
Script:
#!/bin/bash # Find and check the component implementation fd "spender-onboarding-connect-card-step.component.ts" --exec cat {} # Backup search for enrollment method ast-grep --pattern 'enrollCards() { $$$ }'Length of output: 10527
| @@ -373,7 +373,7 @@ describe('SidemenuComponent', () => { | |||
| describe('getSecondarySidemenuOptions():', () => { | |||
| it('should get the secondary options', () => { | |||
| const resData = getSecondarySidemenuOptionsRes1; | |||
| const result = component.getSecondarySidemenuOptions(orgData1, true, true); | |||
| const result = component.getSecondarySidemenuOptions(orgData1, true, true, false); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style-ah! The getSecondarySidemenuOptions tests are now more powerful with the new parameter!
The test cases have been updated with the new boolean parameter, but we should add more test cases to cover all possible combinations.
Add test cases for different parameter combinations:
+ it('should handle different combinations of boolean parameters', () => {
+ const combinations = [
+ [true, true, true, true],
+ [true, true, true, false],
+ [true, true, false, true],
+ [true, false, true, true]
+ ];
+
+ combinations.forEach(([param1, param2, param3, param4]) => {
+ const result = component.getSecondarySidemenuOptions(orgData1, param1, param2, param3);
+ expect(result).toBeDefined();
+ });
+ });Also applies to: 389-389, 439-439
| @@ -203,7 +203,7 @@ | |||
| }, | |||
| }; | |||
|
|
|||
| const cardOpt = component.getCardOptions(); | |||
| const cardOpt = component.getCardOptions(false); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind-blowing! The getCardOptions test is now more powerful with the new boolean parameter, but let's make it even better!
The test case could be enhanced by adding coverage for both true and false scenarios of the new parameter.
Add another test case like this, superstar:
+ it('getCardOptions: should handle opt-in flag correctly', () => {
+ component.orgSettings = {
+ ...orgSettingsRes,
+ org_personal_cards_settings: {
+ allowed: true,
+ enabled: true,
+ },
+ };
+
+ const cardOptWithOptIn = component.getCardOptions(true);
+ const cardOptWithoutOptIn = component.getCardOptions(false);
+
+ expect(cardOptWithOptIn).not.toEqual(cardOptWithoutOptIn);
+ });Committable suggestion skipped: line range outside the PR's diff.
| @@ -248,7 +248,7 @@ | |||
| const resData = primOpt.filter((option) => option.title !== 'Personal Cards'); | |||
| const cardOptSpy = spyOn(component, 'getCardOptions').and.returnValue([]); | |||
| const teamOptSpy = spyOn(component, 'getTeamOptions').and.returnValue([]); | |||
| const result = component.getPrimarySidemenuOptions(true); | |||
| const result = component.getPrimarySidemenuOptions(true, false); | |||
There was a problem hiding this comment.
💡 Codebase verification
Mind it! The parameters are not what they seem, and we need more test power!
The method getPrimarySidemenuOptions takes (isConnected, isOnboardingPending) parameters, and we're missing critical test scenarios:
- Test when user is offline (isConnected: false)
- Test during onboarding state (isOnboardingPending: true)
- Test all combinations as they affect different aspects:
- (true, true): Connected but onboarding pending
- (true, false): Connected with completed onboarding
- (false, true): Offline during onboarding
- (false, false): Offline with completed onboarding
🔗 Analysis chain
Thalaiva style! The getPrimarySidemenuOptions tests are now updated with the new parameter, but we need more power!
The test cases have been updated to include the new boolean parameter, but we should verify both true and false scenarios for comprehensive coverage.
Let's verify the test coverage with this script:
Also applies to: 274-274, 297-297, 361-361
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for getPrimarySidemenuOptions
rg -A 5 "getPrimarySidemenuOptions\(.*true.*false" src/app/shared/components/sidemenu/sidemenu.component.spec.ts
# Check if there are any tests with true/true combination
rg -A 5 "getPrimarySidemenuOptions\(.*true.*true" src/app/shared/components/sidemenu/sidemenu.component.spec.ts
Length of output: 1409
Script:
#!/bin/bash
# Check for false,true and false,false combinations
rg -A 5 "getPrimarySidemenuOptions\(.*false.*true" src/app/shared/components/sidemenu/sidemenu.component.spec.ts
rg -A 5 "getPrimarySidemenuOptions\(.*false.*false" src/app/shared/components/sidemenu/sidemenu.component.spec.ts
# Let's also check the method definition to understand expected behavior
ast-grep --pattern 'getPrimarySidemenuOptions($_,$_) {
$$$
}'
Length of output: 296
Script:
#!/bin/bash
# Find and check the component implementation
fd sidemenu.component.ts --type f --exec cat {}
# Alternative pattern search for the method
ast-grep --pattern 'getPrimarySidemenuOptions(isLoggedIn: boolean, isAdmin: boolean) {
$$$
}'
Length of output: 13585
| ValidationErrors, | ||
| Validators, | ||
| } from '@angular/forms'; | ||
| ChangeDetectorRef, |
There was a problem hiding this comment.
Ayyo! Unused import spotted!
ChangeDetectorRef is imported but sitting idle. Let's remove it to keep our code sharp, naan sonna, code super aagum (as I say, the code becomes super)!
Here's the fix:
import {
- ChangeDetectorRef,
Component,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ChangeDetectorRef, | |
| import { | |
| Component, |
🧰 Tools
🪛 eslint
[error] 3-3: 'ChangeDetectorRef' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 3-3:
'ChangeDetectorRef' is defined but never used
🪛 GitHub Actions: Lint
[error] 3-3: 'ChangeDetectorRef' is defined but never used
| describe('validateInput():', () => { | ||
| it('should set mobileNumberError if mobile number field is empty', () => { | ||
| component.mobileNumberInputValue = ''; | ||
| expect(component.validateInput()); |
There was a problem hiding this comment.
Expecting an assertion, but found none!
expect is for making assertions, thambi (my friend). Seems like you just want to call validateInput() without it.
Fix it like this:
- expect(component.validateInput());
+ component.validateInput();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(component.validateInput()); | |
| component.validateInput(); |
| handleOtpError(err: HttpErrorResponse): void { | ||
| if (err.status === 400) { | ||
| const error = err.error as { message: string }; | ||
| const errorMessage = error.message?.toLowerCase() || ''; | ||
| if (errorMessage.includes('out of attempts') || errorMessage.includes('max send attempts reached')) { | ||
| this.trackingService.optInFlowError({ | ||
| message: 'OTP_MAX_ATTEMPTS_REACHED', | ||
| }); | ||
| this.toastWithoutCTA( | ||
| 'You have reached the limit for 6 digit code requests. Try again after 24 hours.', | ||
| ToastType.FAILURE, | ||
| 'msb-failure-with-camera-icon' | ||
| ); | ||
| this.ngOtpInput?.setValue(''); | ||
| this.disableResendOtp = true; | ||
| } else if (errorMessage.includes('invalid parameter')) { | ||
| this.toastWithoutCTA( | ||
| 'Invalid mobile number. Please try again.', | ||
| ToastType.FAILURE, | ||
| 'msb-failure-with-camera-icon' | ||
| ); | ||
| } else if (errorMessage.includes('expired')) { | ||
| this.toastWithoutCTA( | ||
| 'The code has expired. Please request a new one.', | ||
| ToastType.FAILURE, | ||
| 'msb-failure-with-camera-icon' | ||
| ); | ||
| this.ngOtpInput?.setValue(''); | ||
| } else { | ||
| this.toastWithoutCTA('Code is invalid', ToastType.FAILURE, 'msb-failure-with-camera-icon'); | ||
| this.ngOtpInput?.setValue(''); | ||
| } | ||
| } | ||
|
|
||
| this.sendCodeLoading = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't let errors play hide and seek!
In handleOtpError, you're only handling err.status === 400. Other errors might sneak past. Let's catch them all and show the user we're on top of things.
Here's how we can improve:
handleOtpError(err: HttpErrorResponse): void {
if (err.status === 400) {
// existing handling
} else {
+ this.toastWithoutCTA('An unexpected error occurred. Please try again later.', ToastType.FAILURE, 'msb-failure-with-camera-icon');
+ this.ngOtpInput?.setValue('');
}
this.sendCodeLoading = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleOtpError(err: HttpErrorResponse): void { | |
| if (err.status === 400) { | |
| const error = err.error as { message: string }; | |
| const errorMessage = error.message?.toLowerCase() || ''; | |
| if (errorMessage.includes('out of attempts') || errorMessage.includes('max send attempts reached')) { | |
| this.trackingService.optInFlowError({ | |
| message: 'OTP_MAX_ATTEMPTS_REACHED', | |
| }); | |
| this.toastWithoutCTA( | |
| 'You have reached the limit for 6 digit code requests. Try again after 24 hours.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| this.ngOtpInput?.setValue(''); | |
| this.disableResendOtp = true; | |
| } else if (errorMessage.includes('invalid parameter')) { | |
| this.toastWithoutCTA( | |
| 'Invalid mobile number. Please try again.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| } else if (errorMessage.includes('expired')) { | |
| this.toastWithoutCTA( | |
| 'The code has expired. Please request a new one.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| this.ngOtpInput?.setValue(''); | |
| } else { | |
| this.toastWithoutCTA('Code is invalid', ToastType.FAILURE, 'msb-failure-with-camera-icon'); | |
| this.ngOtpInput?.setValue(''); | |
| } | |
| } | |
| this.sendCodeLoading = false; | |
| } | |
| handleOtpError(err: HttpErrorResponse): void { | |
| if (err.status === 400) { | |
| const error = err.error as { message: string }; | |
| const errorMessage = error.message?.toLowerCase() || ''; | |
| if (errorMessage.includes('out of attempts') || errorMessage.includes('max send attempts reached')) { | |
| this.trackingService.optInFlowError({ | |
| message: 'OTP_MAX_ATTEMPTS_REACHED', | |
| }); | |
| this.toastWithoutCTA( | |
| 'You have reached the limit for 6 digit code requests. Try again after 24 hours.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| this.ngOtpInput?.setValue(''); | |
| this.disableResendOtp = true; | |
| } else if (errorMessage.includes('invalid parameter')) { | |
| this.toastWithoutCTA( | |
| 'Invalid mobile number. Please try again.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| } else if (errorMessage.includes('expired')) { | |
| this.toastWithoutCTA( | |
| 'The code has expired. Please request a new one.', | |
| ToastType.FAILURE, | |
| 'msb-failure-with-camera-icon' | |
| ); | |
| this.ngOtpInput?.setValue(''); | |
| } else { | |
| this.toastWithoutCTA('Code is invalid', ToastType.FAILURE, 'msb-failure-with-camera-icon'); | |
| this.ngOtpInput?.setValue(''); | |
| } | |
| } else { | |
| this.toastWithoutCTA('An unexpected error occurred. Please try again later.', ToastType.FAILURE, 'msb-failure-with-camera-icon'); | |
| this.ngOtpInput?.setValue(''); | |
| } | |
| this.sendCodeLoading = false; | |
| } |
| &__multiple-cards { | ||
| max-height: 200px; | ||
| overflow-y: scroll; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 20px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! The multiple cards layout is stylish, but needs a small touch!
The flex layout is perfect, but let me tell you something - for a smoother scrolling experience on mobile devices, add -webkit-overflow-scrolling: touch. That's how we roll in style!
&__multiple-cards {
max-height: 200px;
overflow-y: scroll;
+ -webkit-overflow-scrolling: touch;
display: flex;
flex-direction: column;
gap: 20px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &__multiple-cards { | |
| max-height: 200px; | |
| overflow-y: scroll; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 20px; | |
| } | |
| &__multiple-cards { | |
| max-height: 200px; | |
| overflow-y: scroll; | |
| -webkit-overflow-scrolling: touch; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 20px; | |
| } |
| width: 50%; | ||
| align-self: flex-end; | ||
| margin-bottom: 20px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen up! The button needs more power on small screens!
The 50% width might make the button too small on mobile devices. Let's make it responsive like a boss!
&__primary-cta {
- width: 50%;
+ width: min(50%, 300px);
+ min-width: 200px;
align-self: flex-end;
margin-bottom: 20px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| width: 50%; | |
| align-self: flex-end; | |
| margin-bottom: 20px; | |
| width: min(50%, 300px); | |
| min-width: 200px; | |
| align-self: flex-end; | |
| margin-bottom: 20px; |
| appFormButtonValidation | ||
| [loading]="cardsEnrolling" | ||
| [loadingText]="'Continue'" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The button is powerful, but needs a voice for all users!
The loading state and validation are spot on! But let's make it accessible for everyone.
appFormButtonValidation
[loading]="cardsEnrolling"
[loadingText]="'Continue'"
+ [attr.aria-label]="cardsEnrolling ? 'Enrolling cards...' : 'Continue to next step'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| appFormButtonValidation | |
| [loading]="cardsEnrolling" | |
| [loadingText]="'Continue'" | |
| appFormButtonValidation | |
| [loading]="cardsEnrolling" | |
| [loadingText]="'Continue'" | |
| [attr.aria-label]="cardsEnrolling ? 'Enrolling cards...' : 'Continue to next step'" |
bistaastha
merged commit 8f4abaf
into
FYLE-86cx2t82k-base-feature-branch
Will Add coverage in follow up. This PR adds:
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Bug Fixes
User Experience
The updates focus on providing a smoother, more transparent user experience during the onboarding and card enrollment process.