test: Add tests for Connnect Card - 1 #3410
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSuperstar style, let me break it down! 🌟 This pull request enhances the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (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: 1
🔭 Outside diff range comments (1)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
Line range hint
3-3: Mind it! Remove the unused import, partner!The
ChangeDetectorRefis imported but never used in the code. Let's keep our imports clean and tidy!-import { ChangeDetectorRef, Component, EventEmitter, Input, OnChanges, OnInit, Output, SimpleChanges } from '@angular/core'; +import { Component, EventEmitter, Input, OnChanges, OnInit, Output, SimpleChanges } from '@angular/core';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(3 hunks)
🧰 Additional context 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/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (3)
91-91: Superstar style security handling, macha!Excellent work on masking the card numbers by only using the last 4 digits in error messages. This is a top-notch security practice that protects sensitive card information!
214-215: Mind-blowing fix for the validator binding!The
.bind(this)ensures our validators maintain the correct context. Without this, the validators could lose their 'this' context and crash like a villain's lair!
115-115: Style-ah implemented security, thalaiva!The same card number masking is consistently applied here too. This kind of consistency in security practices is what separates heroes from zeros!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (2)
24-59: What a grand entrance for our test setup, machan!The test setup is as perfectly orchestrated as a mass hero entry scene! Excellent job with:
- Proper spy creation for all services
- Complete module configuration
- Default mock responses
83-124: These test cases are punching like my signature moves!Superb coverage of error scenarios with special focus on card number masking. The test cases verify that sensitive data is always protected, just like how I protect the innocent in my movies!
| describe('enrollMultipleCards(): ', () => { | ||
| it('should handle successful card enrollment', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue( | ||
| of([statementUploadedCard, { ...statementUploadedCard, id: 'bacc15bbrRGWzg' }]) | ||
| ); | ||
| component.ngOnInit(); | ||
|
|
||
| const stepCompleteSpy = spyOn(component.isStepComplete, 'emit'); | ||
| const showErrorPopoverSpy = spyOn(component, 'showErrorPopover'); | ||
| const setupErrorMessagesSpy = spyOn(component, 'setupErrorMessages'); | ||
| realTimeFeedService.enroll.and.returnValues(of(statementUploadedCard), of(statementUploadedCard)); | ||
| component.enrollMultipleCards(component.enrollableCards); | ||
| tick(); | ||
| expect(component.cardsList.successfulCards).toEqual(['**** 5555', '**** 5555']); | ||
| expect(component.cardsEnrolling).toBeFalse(); | ||
| expect(stepCompleteSpy).toHaveBeenCalledWith(true); | ||
| expect(showErrorPopoverSpy).not.toHaveBeenCalled(); | ||
| expect(setupErrorMessagesSpy).not.toHaveBeenCalled(); | ||
| })); | ||
|
|
||
| it('should handle unsuccessful card enrollment', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue( | ||
| of([statementUploadedCard, { ...statementUploadedCard, id: 'bacc15bbrRGWzg' }]) | ||
| ); | ||
| component.ngOnInit(); | ||
|
|
||
| const stepCompleteSpy = spyOn(component.isStepComplete, 'emit'); | ||
| const showErrorPopoverSpy = spyOn(component, 'showErrorPopover'); | ||
| realTimeFeedService.enroll.and.returnValues( | ||
| of(statementUploadedCard), | ||
| throwError(() => new Error('This card already exists in the system')) | ||
| ); | ||
| component.enrollMultipleCards(component.enrollableCards); | ||
| tick(); | ||
| expect(component.cardsList.successfulCards).toEqual(['**** 5555']); | ||
| expect(component.cardsList.failedCards).toEqual(['**** 5555']); | ||
| expect(component.cardsEnrolling).toBeFalse(); | ||
| expect(stepCompleteSpy).toHaveBeenCalledWith(true); | ||
| expect(showErrorPopoverSpy).toHaveBeenCalledTimes(1); | ||
| })); | ||
| }); | ||
|
|
||
| describe('enrollSingularCard(): ', () => { | ||
| it('should handle successful card enrollment', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue(of([])); | ||
| component.ngOnInit(); | ||
|
|
||
| component.fg.controls.card_number.setValue('41111111111111111'); | ||
| const stepCompleteSpy = spyOn(component.isStepComplete, 'emit'); | ||
| const showErrorPopoverSpy = spyOn(component, 'showErrorPopover'); | ||
| const setupErrorMessagesSpy = spyOn(component, 'setupErrorMessages'); | ||
| realTimeFeedService.enroll.and.returnValues(of(statementUploadedCard)); | ||
| component.enrollSingularCard(); | ||
| tick(); | ||
| expect(component.cardsList.successfulCards).toEqual(['**** 1111']); | ||
| expect(component.cardsEnrolling).toBeFalse(); | ||
| expect(stepCompleteSpy).toHaveBeenCalledWith(true); | ||
| expect(showErrorPopoverSpy).not.toHaveBeenCalled(); | ||
| expect(setupErrorMessagesSpy).not.toHaveBeenCalled(); | ||
| })); | ||
|
|
||
| it('should handle unsuccessful card enrollment', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue(of([])); | ||
| component.ngOnInit(); | ||
|
|
||
| component.fg.controls.card_number.setValue('41111111111111111'); | ||
| const stepCompleteSpy = spyOn(component.isStepComplete, 'emit'); | ||
| const showErrorPopoverSpy = spyOn(component, 'showErrorPopover'); | ||
| realTimeFeedService.enroll.and.returnValues( | ||
| throwError(() => new Error('This card already exists in the system')) | ||
| ); | ||
| component.enrollSingularCard(); | ||
| tick(); | ||
| expect(component.cardsList.failedCards).toEqual(['**** 1111']); | ||
| expect(component.cardsEnrolling).toBeFalse(); | ||
| expect(showErrorPopoverSpy).toHaveBeenCalledTimes(1); | ||
| })); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
The test coverage is powerful, but let's make it legendary!
The enrollment tests are solid like a punch sequence, but we could add more power with these additional test cases:
- Network timeout scenarios
- Multiple consecutive failures
- Rate limiting responses
Would you like me to help craft these additional test cases?
bistaastha
changed the base branch from
FYLE-86cx2t82k-tests-for-connect-card
to
FYLE-86cx2t82k-base-feature-branch
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
src/app/shared/components/sidemenu/sidemenu.component.ts (1)
Line range hint
396-409: Thalaiva style tip: Handle subscriptions like a boss! 🎬The subscription to
getOnboardingStatus()needs proper cleanup and error handling, macha!+ private destroy$ = new Subject<void>(); setupSideMenu(isConnected?: boolean, orgs?: Org[], isDelegatee?: boolean): void { if (isConnected) { - this.spenderOnboardingService.getOnboardingStatus().subscribe((onboardingStatus) => { + this.spenderOnboardingService.getOnboardingStatus().pipe( + takeUntil(this.destroy$), + catchError((error) => { + console.error('Failed to get onboarding status:', error); + return of({ state: OnboardingState.COMPLETED }); + }) + ).subscribe((onboardingStatus) => { const isOnboardingPending = onboardingStatus.state !== OnboardingState.COMPLETED; // ... rest of the code }); } else { // ... offline case } } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (1)
Line range hint
227-269: Let's make this error handling more stylish!The error handling logic could be simplified using a map of error messages to reduce the nested if conditions.
+ private readonly ERROR_MESSAGES = { + 'out of attempts': { + message: 'You have reached the limit for 6 digit code requests. Try again after 24 hours.', + trackingMessage: 'OTP_MAX_ATTEMPTS_REACHED' + }, + 'max send attempts reached': { + message: 'You have reached the limit for 6 digit code requests. Try again after 24 hours.', + trackingMessage: 'OTP_MAX_ATTEMPTS_REACHED' + }, + 'invalid parameter': { + message: 'Invalid mobile number. Please try again.' + }, + 'expired': { + message: 'The code has expired. Please request a new one.' + } + }; + 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(''); - } + const errorKey = Object.keys(this.ERROR_MESSAGES).find(key => errorMessage.includes(key)); + const errorConfig = errorKey ? this.ERROR_MESSAGES[errorKey] : { message: 'Code is invalid' }; + + if (errorConfig.trackingMessage) { + this.trackingService.optInFlowError({ + message: errorConfig.trackingMessage, + }); + } + + this.toastWithoutCTA( + errorConfig.message, + ToastType.FAILURE, + 'msb-failure-with-camera-icon' + ); + + if (errorKey !== 'invalid parameter') { + this.ngOtpInput?.setValue(''); + } + + if (errorKey === 'out of attempts' || errorKey === 'max send attempts reached') { + this.disableResendOtp = true; + } } this.sendCodeLoading = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
src/app/auth/switch-org/switch-org.page.spec.ts(6 hunks)src/app/auth/switch-org/switch-org.page.ts(1 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(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(7 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss(5 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.spec.ts(6 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts(8 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.scss(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts(4 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(5 hunks)src/app/post-verification/invited-user/invited-user.page.ts(5 hunks)src/app/shared/components/sidemenu/sidemenu.component.spec.ts(8 hunks)src/app/shared/components/sidemenu/sidemenu.component.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (19)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (2)
82-82: Superb work on masking those card numbers! Security first!The implementation consistently masks card numbers in error messages, showing only the last 4 digits. That's how we protect sensitive data!
Also applies to: 106-106
201-202: Perfect binding, like a well-choreographed fight scene!The validator methods are properly bound to maintain the correct context. This is how you keep your code disciplined!
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (1)
89-125: These tests are rock solid, like my punch dialogues!The test implementation is excellent with:
- Proper use of fakeAsync and tick for async operations
- Well-typed mock responses
- Comprehensive coverage of success and failure scenarios
src/app/auth/switch-org/switch-org.page.ts (1)
323-328: Superb logic for handling onboarding navigation, thalaiva! 🌟The enhanced condition properly checks both the enrollment settings and onboarding state before deciding the navigation path. This is the way!
src/app/auth/switch-org/switch-org.page.spec.ts (3)
665-680: Mind it! The test coverage for onboarding navigation is superb!The test case properly verifies navigation to dashboard when onboarding is completed. Kabali approves this test!
682-693: Magizhchi! Perfect test case for incomplete onboarding state!The test properly verifies navigation to spender onboarding when status is not complete. This is how you write tests!
47-51: Thalaiva style service setup, perfect!The new services are properly injected and configured in the TestBed. The mock implementations are set up correctly.
Also applies to: 81-82, 120-121, 162-169, 276-277
src/app/fyle/spender-onboarding/spender-onboarding.page.scss (2)
9-9: Style panrom! Perfect padding addition!The 20px top padding improves the vertical spacing of the onboarding container.
77-77: En vazhi, thani vazhi! Perfect color consistency!Using $pure-black for menubutton and title improves contrast and maintains consistency.
Also applies to: 82-82
src/app/fyle/spender-onboarding/spender-onboarding.page.html (1)
40-40: Kabali style event binding! Perfect!The goToConnectCard event binding enables smooth navigation back to the connect card step.
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.html (2)
16-16: Mind it! Better text capitalization!Proper capitalization of "Mobile Number" improves text consistency.
94-108: En vazhi, thani vazhi! Perfect CTA implementation!The new CTA container with conditional go back button improves navigation UX. The ngClass usage for styling is spot on!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.html (1)
190-190: Style ah! Form validation like my perfect timing!The disabled button binding is a superb addition that prevents invalid form submissions. That's how we maintain quality, with style!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (1)
217-294: Mind it! The test coverage is powerful, but let's make it legendary!The enrollment tests are solid like my punch sequence, but we could add more power with these additional test cases:
- Network timeout scenarios
- Multiple consecutive failures
- Rate limiting responses
Would you like me to help craft these additional test cases?
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (2)
49-50: Mind it! The new properties look perfect!The addition of
goToConnectCardevent emitter andshowGoBackCtaflag enhances the component's navigation capabilities.Also applies to: 94-95
138-142: Superstar level subscription handling!The subscription to onboarding status is well implemented, setting the
showGoBackCtaflag when cards step is skipped.src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.scss (2)
14-25: Stylish BEM naming, partner!The CTA container styles are well-organized with clear separation of concerns using BEM methodology.
274-276: Verify the spacing, machan!The reduced margin-bottom (from 24px to 16px) might affect the visual hierarchy. Please verify if this spacing works well with all screen sizes.
src/app/shared/components/sidemenu/sidemenu.component.spec.ts (1)
Line range hint
458-491: Perfect test coverage, thalaiva!The integration of SpenderOnboardingService in the test suite is well done with proper setup and assertions. The use of
fakeAsyncandtick()ensures proper handling of async operations.
| reloadSidemenu(): void { | ||
| this.setupSideMenu(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! The reloadSidemenu method needs more power, macha! 🌟
The method could lead to unexpected behavior since setupSideMenu uses optional parameters in its logic. Let's make it style-ah perfect!
- reloadSidemenu(): void {
- this.setupSideMenu();
+ async reloadSidemenu(): Promise<void> {
+ const isConnected = await this.networkService.isOnline().toPromise();
+ const orgs = await this.orgService.getOrgs().toPromise();
+ const delegatedAccounts = await this.orgUserService.findDelegatedAccounts().toPromise();
+ this.setupSideMenu(isConnected, orgs, delegatedAccounts?.length > 0);
}Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Mind it! This method is like a hero without a movie, partner! 🕶️
The reloadSidemenu() method is not being used anywhere in the codebase. All menu refresh operations are directly using setupSideMenu(). You can safely remove this unused method.
🔗 Analysis chain
Kabali says: Let's verify the reload trigger points! 🕶️
Let's ensure this method is called at appropriate places in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all places where reloadSidemenu is called to verify proper usage
echo "Searching for reloadSidemenu method calls..."
rg "reloadSidemenu\(" --type ts
echo "Searching for potential places where menu refresh might be needed..."
rg "setupSideMenu\(" --type ts
Length of output: 1158
Script:
#!/bin/bash
# Search for template usage and event bindings
echo "Searching for template references to reloadSidemenu..."
rg "reloadSidemenu" --type html
echo "Searching for potential event emitters or subjects that might trigger reload..."
rg "EventEmitter.*reloadSidemenu|Subject.*reloadSidemenu" --type ts
Length of output: 389
| this.spenderOnboardingService.markConnectCardsStepAsComplete().subscribe(() => { | ||
| this.currentStep = OnboardingStep.OPT_IN; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling, macha! Don't let errors slip through!
The subscription is missing error handling. When you're marking steps as complete, you need to be ready for anything!
- this.spenderOnboardingService.markConnectCardsStepAsComplete().subscribe(() => {
- this.currentStep = OnboardingStep.OPT_IN;
- });
+ this.spenderOnboardingService.markConnectCardsStepAsComplete().subscribe({
+ next: () => {
+ this.currentStep = OnboardingStep.OPT_IN;
+ },
+ error: (error) => {
+ console.error('Failed to mark connect cards step as complete:', error);
+ }
+ });📝 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.
| this.spenderOnboardingService.markConnectCardsStepAsComplete().subscribe(() => { | |
| this.currentStep = OnboardingStep.OPT_IN; | |
| }); | |
| this.spenderOnboardingService.markConnectCardsStepAsComplete().subscribe({ | |
| next: () => { | |
| this.currentStep = OnboardingStep.OPT_IN; | |
| }, | |
| error: (error) => { | |
| console.error('Failed to mark connect cards step as complete:', error); | |
| } | |
| }); |
| setInterval(() => { | ||
| if (this.redirectionCount > 0) { | ||
| this.redirectionCount--; | ||
| } else { | ||
| clearInterval(interval); | ||
| this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true }); | ||
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | ||
| } |
There was a problem hiding this comment.
Watch out for memory leaks, thalaiva! Clear that interval!
The interval timer isn't being cleared and you've got duplicate navigation calls. Let's fix both issues with style!
startCountdown(): void {
- setInterval(() => {
+ const intervalId = setInterval(() => {
if (this.redirectionCount > 0) {
this.redirectionCount--;
} else {
- this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true });
- this.router.navigate(['/', 'enterprise', 'my_dashboard']);
+ this.router.navigate(['/', 'enterprise', 'my_dashboard'], { skipLocationChange: true });
+ clearInterval(intervalId);
}
}, 1000);
}📝 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.
| setInterval(() => { | |
| if (this.redirectionCount > 0) { | |
| this.redirectionCount--; | |
| } else { | |
| clearInterval(interval); | |
| this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true }); | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| const intervalId = setInterval(() => { | |
| if (this.redirectionCount > 0) { | |
| this.redirectionCount--; | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard'], { skipLocationChange: true }); | |
| clearInterval(intervalId); | |
| } |
| this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true }); | ||
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Consolidate these navigation calls, partner!
You're making two navigation calls to the same route. That's like using two bullets when one is enough! Let's keep it simple.
- this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true });
- this.router.navigate(['/', 'enterprise', 'my_dashboard']);
+ this.router.navigate(['/', 'enterprise', 'my_dashboard'], { skipLocationChange: true });📝 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.
| this.router.navigateByUrl('/enterprise/my_dashboard', { skipLocationChange: true }); | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard'], { skipLocationChange: true }); |
| } else if (this.cardsList.failedCards.length > 1) { | ||
| return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | ||
| .slice(0, this.cardsList.failedCards.length - 1) | ||
| .join(', ')} and ${this.cardsList.failedCards.slice( | ||
| -1 | ||
| )}. You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| } else { | ||
| return ` | ||
| We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | ||
| .slice(this.cardsList.failedCards.length - 1) | ||
| .join(', ')} and ${this.cardsList.failedCards.slice(-1)}. | ||
| You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Let's make this message generation more stylish, partner!
The message construction is a bit complex. Let's simplify it with template literals and array methods!
- } else if (this.cardsList.failedCards.length > 1) {
- return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards
- .slice(0, this.cardsList.failedCards.length - 1)
- .join(', ')} and ${this.cardsList.failedCards.slice(
- -1
- )}. You can cancel and retry connecting the failed card or proceed to the next step.`;
+ } else if (this.cardsList.failedCards.length > 1) {
+ const allButLast = this.cardsList.failedCards.slice(0, -1).join(', ');
+ const lastCard = this.cardsList.failedCards[this.cardsList.failedCards.length - 1];
+ return `We ran into an issue while processing your request for the cards ${allButLast} and ${lastCard}. You can cancel and retry connecting the failed card or proceed to the 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.
| } else if (this.cardsList.failedCards.length > 1) { | |
| return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | |
| .slice(0, this.cardsList.failedCards.length - 1) | |
| .join(', ')} and ${this.cardsList.failedCards.slice( | |
| -1 | |
| )}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } else { | |
| return ` | |
| We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | |
| .slice(this.cardsList.failedCards.length - 1) | |
| .join(', ')} and ${this.cardsList.failedCards.slice(-1)}. | |
| You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } | |
| } else if (this.cardsList.failedCards.length > 1) { | |
| const allButLast = this.cardsList.failedCards.slice(0, -1).join(', '); | |
| const lastCard = this.cardsList.failedCards[this.cardsList.failedCards.length - 1]; | |
| return `We ran into an issue while processing your request for the cards ${allButLast} and ${lastCard}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } else { | |
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } |
| navigateToDashboard(): void { | ||
| forkJoin([this.orgSettingsService.get(), this.spenderOnboardingService.getOnboardingStatus()]).subscribe( | ||
| ([orgSettings, onboardingStatus]) => { | ||
| if ( | ||
| (orgSettings.visa_enrollment_settings.enabled || | ||
| orgSettings.mastercard_enrollment_settings.enabled || | ||
| orgSettings.amex_feed_enrollment_settings.enabled) && | ||
| onboardingStatus.state !== OnboardingState.COMPLETED | ||
| ) { | ||
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | ||
| } else { | ||
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to this navigation, thalaiva!
The navigation logic is good, but we need to handle errors like a boss!
navigateToDashboard(): void {
- forkJoin([this.orgSettingsService.get(), this.spenderOnboardingService.getOnboardingStatus()]).subscribe(
- ([orgSettings, onboardingStatus]) => {
+ forkJoin([this.orgSettingsService.get(), this.spenderOnboardingService.getOnboardingStatus()]).subscribe({
+ next: ([orgSettings, onboardingStatus]) => {
if (
(orgSettings.visa_enrollment_settings.enabled ||
orgSettings.mastercard_enrollment_settings.enabled ||
orgSettings.amex_feed_enrollment_settings.enabled) &&
onboardingStatus.state !== OnboardingState.COMPLETED
) {
this.router.navigate(['/', 'enterprise', 'spender_onboarding']);
} else {
this.router.navigate(['/', 'enterprise', 'my_dashboard']);
}
+ },
+ error: (error) => {
+ console.error('Failed to fetch settings or onboarding status:', error);
+ // Fallback to dashboard in case of error
+ this.router.navigate(['/', 'enterprise', 'my_dashboard']);
+ }
);
}📝 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.
| navigateToDashboard(): void { | |
| forkJoin([this.orgSettingsService.get(), this.spenderOnboardingService.getOnboardingStatus()]).subscribe( | |
| ([orgSettings, onboardingStatus]) => { | |
| if ( | |
| (orgSettings.visa_enrollment_settings.enabled || | |
| orgSettings.mastercard_enrollment_settings.enabled || | |
| orgSettings.amex_feed_enrollment_settings.enabled) && | |
| onboardingStatus.state !== OnboardingState.COMPLETED | |
| ) { | |
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| } | |
| ); | |
| } | |
| navigateToDashboard(): void { | |
| forkJoin([this.orgSettingsService.get(), this.spenderOnboardingService.getOnboardingStatus()]).subscribe({ | |
| next: ([orgSettings, onboardingStatus]) => { | |
| if ( | |
| (orgSettings.visa_enrollment_settings.enabled || | |
| orgSettings.mastercard_enrollment_settings.enabled || | |
| orgSettings.amex_feed_enrollment_settings.enabled) && | |
| onboardingStatus.state !== OnboardingState.COMPLETED | |
| ) { | |
| this.router.navigate(['/', 'enterprise', 'spender_onboarding']); | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| }, | |
| error: (error) => { | |
| console.error('Failed to fetch settings or onboarding status:', error); | |
| // Fallback to dashboard in case of error | |
| this.router.navigate(['/', 'enterprise', 'my_dashboard']); | |
| } | |
| }); | |
| } |
| describe('setupErrorMessages(): ', () => { | ||
| beforeEach(() => { | ||
| //@ts-ignore | ||
| spyOn(component, 'handleEnrollmentFailures'); | ||
| }); | ||
|
|
||
| it('should add the masked card number to failedCards', () => { | ||
| const mockError = new HttpErrorResponse({ status: 400, statusText: 'Bad Request' }); | ||
| const mockCardNumber = '5432'; | ||
|
|
||
| component.setupErrorMessages(mockError, mockCardNumber); | ||
|
|
||
| expect(component.cardsList.failedCards.length).toBe(1); | ||
| expect(component.cardsList.failedCards[0]).toBe('**** 5432'); | ||
| }); | ||
|
|
||
| it('should add multiple failed cards to failedCards', () => { | ||
| const mockError1 = new HttpErrorResponse({ status: 400, statusText: 'Bad Request' }); | ||
| const mockCardNumber1 = '5432'; | ||
|
|
||
| const mockError2 = new HttpErrorResponse({ status: 500, statusText: 'Internal Server Error' }); | ||
| const mockCardNumber2 = '5678'; | ||
|
|
||
| component.setupErrorMessages(mockError1, mockCardNumber1); | ||
| component.setupErrorMessages(mockError2, mockCardNumber2); | ||
|
|
||
| expect(component.cardsList.failedCards.length).toBe(2); | ||
| expect(component.cardsList.failedCards).toEqual(['**** 5432', '**** 5678']); | ||
| }); | ||
|
|
||
| it('should handle cases where cardId is undefined', () => { | ||
| const mockError = new HttpErrorResponse({ status: 404, statusText: 'Not Found' }); | ||
| const mockCardNumber = '4444'; | ||
|
|
||
| component.setupErrorMessages(mockError, mockCardNumber); | ||
|
|
||
| //@ts-ignore | ||
| expect(component.handleEnrollmentFailures).toHaveBeenCalledWith(mockError, undefined); | ||
| expect(component.cardsList.failedCards.length).toBe(1); | ||
| expect(component.cardsList.failedCards[0]).toBe('**** 4444'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Your error handling is stylish, but let's add some more masala!
The error message setup is looking sharp, but consider these additional scenarios:
- Network disconnection during enrollment
- Invalid card format responses
- Server-side validation failures
it('should handle network disconnection during enrollment', fakeAsync(() => {
const mockError = new HttpErrorResponse({
status: 0,
statusText: 'Unknown Error',
error: new ErrorEvent('Network Error')
});
const mockCardNumber = '4321';
component.setupErrorMessages(mockError, mockCardNumber);
expect(component.cardsList.failedCards[0]).toBe('**** 4321');
expect(component.singularEnrollmentFailure).toContain('network');
}));| goBackToConnectCard(): void { | ||
| this.goToConnectCard.emit(true); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah style! Let's add a return type!
The goBackToConnectCard method should have an explicit return type annotation.
- goBackToConnectCard(): void {
+ public goBackToConnectCard(): void {
this.goToConnectCard.emit(true);
}📝 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.
| goBackToConnectCard(): void { | |
| this.goToConnectCard.emit(true); | |
| } | |
| public goBackToConnectCard(): void { | |
| this.goToConnectCard.emit(true); | |
| } | |
| import { SpenderOnboardingService } from 'src/app/core/services/spender-onboarding.service'; | ||
| import { onboardingStatusData } from 'src/app/core/mock-data/onboarding-status.data'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! We need more tests!
While the SpenderOnboardingService integration is well done, we're missing test coverage for the new goBackToConnectCard method.
Add these test cases:
describe('goBackToConnectCard():', () => {
it('should emit goToConnectCard event with true value', () => {
spyOn(component.goToConnectCard, 'emit');
component.goBackToConnectCard();
expect(component.goToConnectCard.emit).toHaveBeenCalledOnceWith(true);
});
});Also applies to: 41-41, 68-68, 86-86, 103-103, 115-117
| &__cta-text { | ||
| font-size: 14px; | ||
| font-weight: 500; | ||
| } | ||
|
|
||
| &__arrow-icon { | ||
| margin-right: 6px; | ||
| height: 18px; | ||
| width: 18px; | ||
| } | ||
|
|
||
| &__cta-secondary { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| color: $blue-black; | ||
| flex-direction: row; | ||
| padding: 16px 0; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Let's add some style magic!
The CTA styles look good, but we should add hover states for better user interaction.
&__cta-secondary {
display: flex;
align-items: center;
justify-content: center;
color: $blue-black;
flex-direction: row;
padding: 16px 0;
+ cursor: pointer;
+
+ &:hover {
+ color: darken($blue-black, 10%);
+ }
}📝 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.
| &__cta-text { | |
| font-size: 14px; | |
| font-weight: 500; | |
| } | |
| &__arrow-icon { | |
| margin-right: 6px; | |
| height: 18px; | |
| width: 18px; | |
| } | |
| &__cta-secondary { | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| color: $blue-black; | |
| flex-direction: row; | |
| padding: 16px 0; | |
| } | |
| &__cta-text { | |
| font-size: 14px; | |
| font-weight: 500; | |
| } | |
| &__arrow-icon { | |
| margin-right: 6px; | |
| height: 18px; | |
| width: 18px; | |
| } | |
| &__cta-secondary { | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| color: $blue-black; | |
| flex-direction: row; | |
| padding: 16px 0; | |
| cursor: pointer; | |
| &:hover { | |
| color: darken($blue-black, 10%); | |
| } | |
| } |
…-card-2' into FYLE-86cx2t82k-tests-for-connect-card-2
bistaastha
merged commit e44959e
into
FYLE-86cx2t82k-base-feature-branch
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
Coverage - remaining coverage to be added in follow up:
Summary by CodeRabbit
Bug Fixes
New Features
Tests