test: fix failing tests across the app #3417
Conversation
WalkthroughIn a spectacular display of code transformation, this pull request revolutionizes the onboarding experience across multiple components! The changes introduce a robust Changes
Suggested Reviewers
Poem
Possibly Related PRs
Suggested Labels
Finishing Touches
🪧 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 (
|
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
1 similar comment
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
src/app/app.component.ts(3 hunks)src/app/auth/switch-org/switch-org.page.spec.ts(5 hunks)src/app/auth/switch-org/switch-org.page.ts(1 hunks)src/app/core/services/spender-onboarding.service.ts(3 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts(11 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(9 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html(3 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts(3 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.html(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.scss(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts(3 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(2 hunks)src/app/post-verification/invited-user/invited-user.page.spec.ts(7 hunks)src/app/post-verification/invited-user/invited-user.page.ts(1 hunks)src/app/shared/components/sidemenu/sidemenu.component.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
🪛 eslint
src/app/core/services/spender-onboarding.service.ts
[error] 91-91: Unexpected console statement.
(no-console)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 4-4: 'AbstractControl' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
src/app/core/services/spender-onboarding.service.ts
[failure] 91-91:
Unexpected console statement
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[failure] 4-4:
'AbstractControl' is defined but never used
🪛 GitHub Actions: Lint
src/app/core/services/spender-onboarding.service.ts
[error] 91-91: Unexpected console statement - ESLint rule 'no-console' violation
🔇 Additional comments (27)
src/app/fyle/spender-onboarding/spender-onboarding.page.ts (2)
114-114: Superb placement of the onboarding status event!Setting the event after successful API call ensures proper state synchronization.
136-136: Mind-blowing consistency in implementation!The onboarding status event is handled consistently across both completion paths.
src/app/post-verification/invited-user/invited-user.page.ts (1)
102-103: What a powerful enhancement to the routing logic!The addition of corporate credit card settings check makes the navigation more comprehensive. Excellent use of forkJoin for efficient API calls.
src/app/app.component.ts (1)
72-73: Perfect dependency injection style!Clean addition of SpenderOnboardingService to the constructor.
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (5)
38-38: Mind it! New spy method added for onboarding status.The test setup now includes
setOnboardingStatusEventspy, which is crucial for verifying onboarding status updates.
72-92: Superstar level test organization, macha!The test cases for
ionViewWillEnterare well organized into a dedicated describe block with proper setup and assertions. The first test case thoroughly verifies:
- Loader visibility
- User data fetching
- Step progression
- Loading state management
94-112: Style ah? Testing RTF disabled scenario!Good coverage of the RTF disabled scenario, ensuring the component correctly transitions to the opt-in step.
137-163: Kabali da! Solid test coverage for skip functionality!The skip functionality tests are comprehensive, covering both connect card and opt-in steps. The navigation assertion for opt-in step is particularly important.
165-191: Mind it! Complete test coverage for step completion.The test cases for
markStepAsCompletethoroughly verify both connect card and opt-in step completion scenarios.src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (2)
96-96: Mind it! New state flag added.Good addition of
savingMobileNumberflag to track the saving state.
Line range hint
173-193: Thalaiva style state management!The
finalizeoperator ensures proper cleanup ofsavingMobileNumberstate regardless of success or failure. This prevents UI getting stuck in loading state.src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (4)
35-35: Punch dialogue: New output event for step skipping!Good addition of
isStepSkippedoutput event to handle step skipping scenarios.
47-50: Thalaiva style type enhancement!The
cardValuesMaptype now includesenrollment_successflag, improving type safety.
84-86: Style ah? Helper method for card number!The
getFullCardNumberhelper method improves code organization and reusability.
275-305: Kabali da! Enhanced validators with better type handling!The validators now handle card IDs properly and include helpful TODO comments for future Angular upgrades.
Let's verify if there are any Angular 14+ migrations planned:
✅ Verification successful
Mind it! The TODO comment is spot on, partner!
The type casting is necessary in Angular 13.3.12, and the TODO comment correctly guides future developers about its removal in Angular 14+. That's the Thalaivar way of writing code - thinking about the future while handling the present!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for Angular version plans rg -A 5 '"@angular/core":\s+"[^"]+"|"@angular/core":\s+".+"'Length of output: 380
src/app/post-verification/invited-user/invited-user.page.spec.ts (2)
27-31: Mind-blowing service setup, partner!The new service imports and spy injections are perfectly organized. The test setup is rock solid with proper spy objects for OrgSettingsService and SpenderOnboardingService.
Also applies to: 46-47, 62-63, 79-80, 95-96
105-108: Your service initialization style is superstar level!The mock data setup for spenderOnboardingService and orgSettingsService is perfectly synchronized. Like a well-choreographed dance sequence!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (1)
377-445: These error popover tests are mind-blowing, like my signature moves!The test cases beautifully handle both scenarios:
- Error display without successful cards
- Error display with successful cards present
src/app/auth/switch-org/switch-org.page.spec.ts (1)
608-613: The navigation parameters are perfectly synchronized!The test correctly verifies the navigation parameters including the openSMSOptInDialog flag.
src/app/fyle/spender-onboarding/spender-onboarding.page.scss (2)
6-10: Mind it! These styles are perfectly balanced, like all things should be!The new container class with background properties creates a visually appealing layout.
33-33: Thalaiva approves these padding adjustments!The reduced horizontal padding in menu icon (8px) and adjusted step tracker padding (20px) create better visual harmony.
Also applies to: 41-41
src/app/fyle/spender-onboarding/spender-onboarding.page.html (1)
34-34: When I point one finger at the skip button, four fingers point to this excellent event handler!The new
(isStepSkipped)output event properly handles the skip functionality, maintaining a clean component interaction.src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html (3)
36-36: Style is what you choose to be! These text changes are pure class!Consistent text casing in labels ("Mobile number") and informative messages improve user experience.
Also applies to: 44-44
46-50: Loading states? Mind it! Users should know what's happening!The loading states and OTP input improvements provide better user feedback.
Also applies to: 69-71
112-115: Form validation is like my punch - it never misses!The form button validation with loading states ensures a robust user interaction.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss (1)
213-213: When I style, even margins listen! These changes are simply superstar!The adjusted margins and new OTP styles create a better visual hierarchy and improve the user interface.
Also applies to: 418-429
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html (1)
31-31: Now this is what I call smart thinking!The readonly state for successfully enrolled cards prevents accidental modifications.
| @@ -10,6 +10,8 @@ | |||
| providedIn: 'root', | |||
| }) | |||
| export class SpenderOnboardingService { | |||
| onboardingComplete$: BehaviorSubject<boolean> = new BehaviorSubject(true); | |||
There was a problem hiding this comment.
Mind it! Initialize onboardingComplete$ to false by default!
Starting with true state could mislead subscribers about completion status. Let the actual completion event drive the state to true.
- onboardingComplete$: BehaviorSubject<boolean> = new BehaviorSubject(true);
+ onboardingComplete$: BehaviorSubject<boolean> = new BehaviorSubject(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.
| onboardingComplete$: BehaviorSubject<boolean> = new BehaviorSubject(true); | |
| onboardingComplete$: BehaviorSubject<boolean> = new BehaviorSubject(false); |
| // Emit a new event | ||
| setOnboardingStatusEvent(): void { | ||
| this.onboardingComplete$.next(true); | ||
| console.log('event set'); | ||
| } |
There was a problem hiding this comment.
Style-ah! Remove that console.log statement!
Production code should use proper logging service, not console.log statements.
setOnboardingStatusEvent(): void {
this.onboardingComplete$.next(true);
- console.log('event set');
}📝 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.
| // Emit a new event | |
| setOnboardingStatusEvent(): void { | |
| this.onboardingComplete$.next(true); | |
| console.log('event set'); | |
| } | |
| // Emit a new event | |
| setOnboardingStatusEvent(): void { | |
| this.onboardingComplete$.next(true); | |
| } |
🧰 Tools
🪛 eslint
[error] 91-91: Unexpected console statement.
(no-console)
🪛 GitHub Check: Run linters
[failure] 91-91:
Unexpected console statement
🪛 GitHub Actions: Lint
[error] 91-91: Unexpected console statement - ESLint rule 'no-console' violation
| setOnboardingStatusAsComplete(): Observable<boolean> { | ||
| return this.onboardingComplete$.asObservable(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Macha, rename method to reflect its true purpose!
The method name suggests it sets the status but it only returns an observable. Let's make it crystal clear!
- setOnboardingStatusAsComplete(): Observable<boolean> {
+ getOnboardingStatusComplete$(): Observable<boolean> {
return this.onboardingComplete$.asObservable();
}📝 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.
| setOnboardingStatusAsComplete(): Observable<boolean> { | |
| return this.onboardingComplete$.asObservable(); | |
| } | |
| getOnboardingStatusComplete$(): Observable<boolean> { | |
| return this.onboardingComplete$.asObservable(); | |
| } |
| this.spenderOnboardingService.setOnboardingStatusAsComplete().subscribe(() => { | ||
| if (this.isOnline) { | ||
| this.sidemenuRef.showSideMenuOnline(); | ||
| } else { | ||
| this.sidemenuRef.showSideMenuOffline(); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Watch out! Memory leak alert!
The subscription to onboarding status needs cleanup. Implement OnDestroy and unsubscribe to prevent memory leaks.
+ private onboardingStatusSubscription: Subscription;
ngOnInit(): void {
- this.spenderOnboardingService.setOnboardingStatusAsComplete().subscribe(() => {
+ this.onboardingStatusSubscription = this.spenderOnboardingService.setOnboardingStatusAsComplete().subscribe(() => {
if (this.isOnline) {
this.sidemenuRef.showSideMenuOnline();
} else {
this.sidemenuRef.showSideMenuOffline();
}
});
}
+ ngOnDestroy(): void {
+ if (this.onboardingStatusSubscription) {
+ this.onboardingStatusSubscription.unsubscribe();
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| import { | ||
| AbstractControl, | ||
| FormBuilder, | ||
| FormControl, | ||
| FormGroup, | ||
| ValidationErrors, | ||
| ValidatorFn, | ||
| Validators, | ||
| } from '@angular/forms'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Unused import detected.
The AbstractControl import is not used in the code.
-import {
- AbstractControl,
- FormBuilder,
- FormControl,
- FormGroup,
- ValidationErrors,
- ValidatorFn,
- Validators,
-} from '@angular/forms';
+import {
+ FormBuilder,
+ FormControl,
+ FormGroup,
+ ValidationErrors,
+ ValidatorFn,
+ Validators,
+} from '@angular/forms';📝 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.
| import { | |
| AbstractControl, | |
| FormBuilder, | |
| FormControl, | |
| FormGroup, | |
| ValidationErrors, | |
| ValidatorFn, | |
| Validators, | |
| } from '@angular/forms'; | |
| import { | |
| FormBuilder, | |
| FormControl, | |
| FormGroup, | |
| ValidationErrors, | |
| ValidatorFn, | |
| Validators, | |
| } from '@angular/forms'; |
🧰 Tools
🪛 eslint
[error] 4-4: 'AbstractControl' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 4-4:
'AbstractControl' is defined but never used
| it('should call enrollMultipleCards if enrollableCards has items', () => { | ||
| component.enrollableCards = [statementUploadedCard, { ...statementUploadedCard, id: 'bacc15bbrRGWzg' }]; | ||
| const enrollSingularCardSpy = spyOn(component, 'enrollSingularCard'); | ||
| const enrollMultipleCardsSpy = spyOn(component, 'enrollMultipleCards'); | ||
| component.cardValuesMap.bacc15bbrRGWzf = { | ||
| last_four: '1111', | ||
| enrollment_success: true, | ||
| card_type: CardNetworkType.VISA, | ||
| }; | ||
|
|
||
| component.enrollCards(); | ||
|
|
||
| expect(enrollMultipleCardsSpy).toHaveBeenCalledWith([component.enrollableCards[1]]); | ||
| expect(enrollSingularCardSpy).not.toHaveBeenCalled(); | ||
| expect(component.cardsEnrolling).toBeTrue(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your error handling needs more mass, like my action sequences!
Add test cases for these error scenarios:
- Network timeout during card enrollment
- Server returning 500 error
- Rate limiting errors
- Invalid card format errors
| describe('ngOnInit(): ', () => { | ||
| beforeEach(() => { | ||
| component.enrollableCards = [{ ...statementUploadedCard, card_number: '1111' }]; | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue( | ||
| of([{ ...statementUploadedCard, card_number: '1111' }]) | ||
| ); | ||
| }); | ||
|
|
||
| it('should setup card form controls', () => { | ||
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | ||
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | ||
| component.ngOnInit(); | ||
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNumber).toBeTrue(); | ||
| }); | ||
|
|
||
| it('should run validator for invalid card network', () => { | ||
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | ||
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | ||
| component.isVisaRTFEnabled = false; | ||
| component.ngOnInit(); | ||
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeTrue(); | ||
| }); | ||
|
|
||
| it('should run validator for valid card and valid network', () => { | ||
| spyOn(component, 'getFullCardNumber').and.returnValue('4111111111111111'); | ||
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | ||
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | ||
| component.isVisaRTFEnabled = true; | ||
| component.ngOnInit(); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should run validator for valid card and invalid network', () => { | ||
| spyOn(component, 'getFullCardNumber').and.returnValue('41111111111111'); | ||
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | ||
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | ||
| component.isVisaRTFEnabled = false; | ||
| component.ngOnInit(); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your test cases pack a punch, but let's make the titles more dramatic!
Consider renaming the test cases to be more descriptive:
- it('should setup card form controls', () => {
+ it('should show error when card number is invalid', () => {
- it('should run validator for invalid card network', () => {
+ it('should show error when card network is not supported', () => {
- it('should run validator for valid card and valid network', () => {
+ it('should accept valid Visa card when Visa RTF is enabled', () => {
- it('should run validator for valid card and invalid network', () => {
+ it('should accept non-Visa card when Visa RTF is disabled', () => {📝 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.
| describe('ngOnInit(): ', () => { | |
| beforeEach(() => { | |
| component.enrollableCards = [{ ...statementUploadedCard, card_number: '1111' }]; | |
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue( | |
| of([{ ...statementUploadedCard, card_number: '1111' }]) | |
| ); | |
| }); | |
| it('should setup card form controls', () => { | |
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | |
| component.ngOnInit(); | |
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNumber).toBeTrue(); | |
| }); | |
| it('should run validator for invalid card network', () => { | |
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | |
| component.isVisaRTFEnabled = false; | |
| component.ngOnInit(); | |
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeTrue(); | |
| }); | |
| it('should run validator for valid card and valid network', () => { | |
| spyOn(component, 'getFullCardNumber').and.returnValue('4111111111111111'); | |
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | |
| component.isVisaRTFEnabled = true; | |
| component.ngOnInit(); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | |
| }); | |
| it('should run validator for valid card and invalid network', () => { | |
| spyOn(component, 'getFullCardNumber').and.returnValue('41111111111111'); | |
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | |
| component.isVisaRTFEnabled = false; | |
| component.ngOnInit(); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | |
| }); | |
| describe('ngOnInit(): ', () => { | |
| beforeEach(() => { | |
| component.enrollableCards = [{ ...statementUploadedCard, card_number: '1111' }]; | |
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue( | |
| of([{ ...statementUploadedCard, card_number: '1111' }]) | |
| ); | |
| }); | |
| it('should show error when card number is invalid', () => { | |
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | |
| component.ngOnInit(); | |
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNumber).toBeTrue(); | |
| }); | |
| it('should show error when card network is not supported', () => { | |
| realTimeFeedService.isCardNumberValid.and.returnValue(false); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | |
| component.isVisaRTFEnabled = false; | |
| component.ngOnInit(); | |
| component.fg.controls.card_number_bacc15bbrRGWzf.setValue('4111111111'); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeTrue(); | |
| }); | |
| it('should accept valid Visa card when Visa RTF is enabled', () => { | |
| spyOn(component, 'getFullCardNumber').and.returnValue('4111111111111111'); | |
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | |
| component.isVisaRTFEnabled = true; | |
| component.ngOnInit(); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | |
| }); | |
| it('should accept non-Visa card when Visa RTF is disabled', () => { | |
| spyOn(component, 'getFullCardNumber').and.returnValue('41111111111111'); | |
| realTimeFeedService.isCardNumberValid.and.returnValue(true); | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.OTHERS); | |
| component.isVisaRTFEnabled = false; | |
| component.ngOnInit(); | |
| fixture.detectChanges(); | |
| expect(component.fg.controls.card_number_bacc15bbrRGWzf.errors.invalidCardNetwork).toBeUndefined(); | |
| }); |
| (orgSettings.corporate_credit_card_settings.enabled || | ||
| orgSettings.visa_enrollment_settings.enabled || | ||
| orgSettings.mastercard_enrollment_settings.enabled || | ||
| orgSettings.amex_feed_enrollment_settings.enabled) && | ||
| onboardingStatus.state !== OnboardingState.COMPLETED |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your navigation logic is like a perfectly choreographed fight scene!
The conditions for card settings are well-structured:
(orgSettings.corporate_credit_card_settings.enabled ||
orgSettings.visa_enrollment_settings.enabled ||
orgSettings.mastercard_enrollment_settings.enabled ||
orgSettings.amex_feed_enrollment_settings.enabled)However, consider extracting this into a helper method for better readability:
private isCardEnrollmentEnabled(orgSettings: OrgSettings): boolean {
return (
orgSettings.corporate_credit_card_settings.enabled ||
orgSettings.visa_enrollment_settings.enabled ||
orgSettings.mastercard_enrollment_settings.enabled ||
orgSettings.amex_feed_enrollment_settings.enabled
);
}| it('navigateToDashboard(): should navigate to spender onboarding when onboarding status is not complete', fakeAsync(() => { | ||
| spenderOnboardingService.getOnboardingStatus.and.returnValue( | ||
| of({ ...onboardingStatusData, state: OnboardingState.YET_TO_START }) | ||
| ); | ||
| orgSettingsService.get.and.returnValue(of(orgSettingsData)); | ||
| component.navigateToDashboard(); | ||
| tick(); | ||
| expect(router.navigate).toHaveBeenCalledOnceWith(['/', 'enterprise', 'spender_onboarding']); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your test case is like a perfect entry scene, but we need more action!
Consider adding test cases for:
- Navigation when different card settings are enabled/disabled
- Navigation with different combinations of onboarding states
- Error handling during navigation
| @@ -22,12 +22,13 @@ | |||
| <input | |||
| class="smartlook-show connect-card__card-number-input pl-0" | |||
| inputmode="numeric" | |||
| mask="0000 0000 0000 0000" | |||
| mask="0000 0000 0000" | |||
There was a problem hiding this comment.
💡 Codebase verification
Listen here, partner! This mask situation is no small matter!
The mask 0000 0000 0000 (12 digits) won't cut it for our card numbers! Both Visa and Mastercard need 16 digits to work their magic, as shown in the test case using '4111111111111111'. This mask change would leave users hanging with 4 digits short!
• Change the mask back to 0000 0000 0000 0000 to handle 16-digit card numbers properly
🔗 Analysis chain
Hold on! This card mask needs a double take!
The mask has been reduced from 16 to 12 digits, which might not accommodate all valid card numbers.
Please verify if all supported card types (Visa, Mastercard) can be properly entered with the 12-digit mask.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for card number validation logic
rg -A 5 "card.*number.*valid" src/
Length of output: 18158
This PR:
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores