feat: Implementation for connect cards part 2 #3389
Conversation
WalkthroughListen up, superstar! 🌟 We've rolled out a brand new component for spender onboarding that connects corporate cards with flair! This change introduces a dynamic HTML form that allows users to enroll multiple cards, complete with validation and error handling. The user interface is designed to be smooth and responsive, ensuring a seamless onboarding experience that makes corporate card enrollment feel like a blockbuster hit! Changes
Possibly related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
bistaastha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
src/assets/svg/check.svgis excluded by!**/*.svgsrc/assets/svg/progress-bar.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
src/app/core/mock-data/onboarding-status.data.ts(1 hunks)src/app/core/services/spender-onboarding.service.spec.ts(1 hunks)src/app/core/services/spender-onboarding.service.ts(1 hunks)src/app/fyle/fyle-routing.module.ts(1 hunks)src/app/fyle/spender-onboarding/models/onboarding-step.enum.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.scss(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-routing.module.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.module.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.html(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.scss(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(1 hunks)src/app/shared/components/popup-alert/popup-alert.component.html(1 hunks)src/app/shared/components/popup-alert/popup-alert.component.scss(1 hunks)src/app/shared/components/popup-alert/popup-alert.component.ts(2 hunks)
🧰 Additional context used
🪛 eslint
src/app/shared/components/popup-alert/popup-alert.component.ts
[error] 3-3: 'CardNetworkType' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 4-4: 'FormArray' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 12-12: 'finalize' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 14-14: 'statementUploadedCard' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 14-14: 'visaRTFCard' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/app/fyle/spender-onboarding/spender-onboarding.page.ts
[error] 3-3: 'ExtendedOrgUser' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (45)
src/app/fyle/spender-onboarding/models/onboarding-step.enum.ts (1)
1-4: "Thalaiva, your OnboardingStep Enum is sleek!"
Super job defining these steps, boss. No obstacles spotted!
src/app/fyle/spender-onboarding/spender-onboarding-routing.module.ts (1)
1-16: "Route-a set panna, perfect-u da!"
This routing module is crisp and does the job well. Everything is neat and clean, and no refactor needed now.
src/app/core/mock-data/onboarding-status.data.ts (1)
1-14: "Data freeze-u, no messing around!"
Your onboardingStatusData is well-structured. Just ensure any future additions align with OnboardingState.
src/app/shared/components/popup-alert/popup-alert.component.ts (3)
4-4: "PopoverCardsList import: super cool"
This import for PopoverCardsList sets the stage for interesting card displays, boss!
21-21: "New property, new possibilities!"
cardsList input is a power move for dynamic updates. Great job!
25-25: "Function signature, locked and loaded!"
Explicit parameter and return type keep things nice and safe. Smooth ride ahead!
src/app/core/services/spender-onboarding.service.ts (6)
7-7: Spot-on new import, boss!
The addition of OnboardingStatus fits neatly, just like a bold punch in the right spot. Everything looks perfect at first glance.
12-12: Renaming the service – a heroic move!
The rename to SpenderOnboardingService is grand, da! Makes the intention crisp and clear.
15-17: Time for a quick check!
The new getOnboardingStatus returning OnboardingStatus is correct as per the updated endpoint. Ensure all calling modules handle the new response type, my friend.
23-23: Endpoint updates are shining bright like a Superstar!
/onboarding/process_step_connect_cards aligns with the new route. Code is strong, boss.
31-31: SMS Opt-In is looking solid!
/onboarding/process_step_sms_opt_in is correct. Make sure you have enough testing coverage.
37-37: Welcome modal route – a clean finish!
/onboarding/process_step_show_welcome_modal is consistent with the rest. Properly orchestrated, my friend.
src/app/fyle/spender-onboarding/spender-onboarding.page.ts (2)
35-72: Smooth concurrency approach, da!
Using forkJoin to load user data, org settings, onboarding status, and corporate card info in parallel is brilliance. Just be cautious about error handling if one of them fails.
74-81: Skipping steps, Rajini style!
The logic is straightforward. Confirm that skipping triggers the correct transitions in the UI, macha.
src/app/fyle/fyle-routing.module.ts (1)
48-52: New route in town, boss!
Aiyo! 'spender_onboarding' path is well placed for lazy loading. Test the navigation thoroughly to ensure unstoppable user flow.
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (5)
1-14: Superb Import Statements, Thalaiva!
All necessary dependencies are well organized and clearly declared. This is good usage of Angular testing modules, da.
15-24: Aiyoo, Setup Looking Solid!
The spy objects and service dependencies are well structured in these lines. Excellent approach for isolating and mocking the required services, da.
25-64: TestBed Configuration Flows Smoothly, Boss!
Wonderful usage of TestBed.configureTestingModule to provide the correct spies. This ensures each test runs in a clean environment.
87-95: Skipping the Steps Like a Superstar!
Implementation for skipping steps is perfect:
- Correct calls to the service methods based on the current step.
- This test ensures each skip function is tested for different steps.
No changes required.
97-105: Marking Steps as Complete Like Lightning!
Super job verifying the correct service methods are called for each step. The test coverage is thorough, da.
src/app/core/services/spender-onboarding.service.spec.ts (11)
1-8: Top-Tier Imports for Testing, Boss!
Ensures you have everything needed: from test modules to final boarding step models. Perfect.
9-30: Setting Up the Spies with Style!
The beforeEach method properly injects the dependency, which is absolutely superb for repeatable tests. Perfect clarity on what is being mocked.
32-41: Aiyoo, Onboarding Status is in Good Hands!
This test verifies we get the right data from '/onboarding'. Crisp and clean.
43-57: Process Connect Cards Step: Mass Hero Entry!
- Perfect usage of a mocked post call.
- Good job verifying that you pass the correct endpoint and data.
All is well, Superstar.
59-73: Process SMS Opt-In Step: Mass Scenes Continue!
Checks everything from the returned result to the endpoint. Great alignment with the rest of the code.
75-88: Welcome Modal Step: Applause!
Neatly tests the flow and correctness of the posted data. Solid approach, boss.
90-99: Marking Welcome Modal Step As Complete: Smooth as Silk!
The spy’s usage is correct, validating the data flow thoroughly.
101-110: Connect Cards Step is a Full-On Success!
Neatly verifies the next step call. Good job ensuring the result is the same as the mock.
112-121: Skipping Cards, Mass Style!
- The skip function calls the process method with
is_skipped: true. - Return value is verified thoroughly.
Tip-top, Machi!
123-132: Marking SMS Opt-In Step As Complete: Bliss!
Aye, usage is consistent, verifying the correct method call.
134-144: Skipping SMS Opt-In: Perfect Execution!
The final test ensures coverage for all skip scenarios. You’ve covered everything, boss.
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (11)
23-34: Component Decorator: Yes, Mokka!
The selector, template, and style are well-defined. Perfect approach for a dedicated step component.
35-45: Fabulous Card Data Structures, Boss!
Variables are properly typed: outputs, inputs, form controls. Great job.
46-52: Popover Cards List: Clear as Crystal!
The structure is well-defined for success/failure tracking.
54-61: Lovely Setup in Constructor, Superstar!
Injecting the necessary services is done well, so no extra overhead is present.
63-86: enrollCards(): Superstar Implementation!
- Using
concatMapto sequentially enroll each card is wise. - The
cardsListapproach to track success/failure is easy to read. - Emitting
isStepCompletedafter success is perfect.
All top standard, my friend.
104-134: showErrorPopover(): Cinematic Flair!
- The popover usage is neat: gives user chance to proceed or cancel.
- Good DRY approach of using
generateMessage()for the text.
Keep rocking, Superstar.
136-141: OnChanges Hook: Great Sensible Use!
You properly enable/disable flags based on orgSettings. Crisp logic, no wasted checks.
143-172: ngOnInit: Marvelous Work with Dynamic Form Controls!
- Filtering out the enrollable cards is a neat approach.
- Adding form controls for each card is well-coded.
No major issues, Thalaiva.
174-190: Card Number Formatting: Style is Slick!
- Removing non-numeric characters.
- Grouping digits in sets of 4.
Super user-friendly approach.
192-205: Card Number Validator: Very Good!
It checks valid numbers and enumerates the card type. Thorough job.
207-219: Card Network Validator: Another Round of Applause!
- Checks if Visa/Mastercard RTF is enabled.
- Returns a consistent error object.
Well implemented, Superstar.
src/app/fyle/spender-onboarding/spender-onboarding.page.html (3)
1-10: HTML Structure is Entertaining and Efficient!
- Using
*ngIf="!isLoading"for conditional rendering is genius. - The user greeting with wave is endearing.
All good, my friend.
12-26: Stepper Container is Superb, da!
A dual progress bar approach is visually appealing. The Skip CTA is placed well, connecting to a click method that is easily discovered.
28-33: Connect Card Step Component Integration: Classy Move!
Props to you for binding [orgSettings] and hooking into (isStepCompleted) to skip the next step. This ensures modular flow.
| @@ -1,5 +1,7 @@ | |||
| import { Component, Input } from '@angular/core'; | |||
| import { PopoverController } from '@ionic/angular'; | |||
| import { CardNetworkType } from 'src/app/core/enums/card-network-type'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
"Unused import, simplify life da!"
You’re importing CardNetworkType but not actually using it. Time to remove that if not needed.
- import { CardNetworkType } from 'src/app/core/enums/card-network-type';🧰 Tools
🪛 eslint
[error] 3-3: 'CardNetworkType' is defined but never used.
(@typescript-eslint/no-unused-vars)
| import { SharedModule } from 'src/app/shared/shared.module'; | ||
| import { SpenderOnboardingPage } from './spender-onboarding.page'; | ||
| import { IonicModule } from '@ionic/angular'; | ||
| import { FormsModule, ReactiveFormsModule } from '@angular/forms'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate FormsModule imports, boss!
Aiyo! FormsModule is imported twice, superstar! Removing the extra import can keep things clean like a swift knockout punch.
imports: [
SharedModule,
CommonModule,
FormsModule,
IonicModule,
MatButtonModule,
SpenderOnboardingRoutingModule,
- FormsModule,
ReactiveFormsModule,
],Also applies to: 19-19
| markStepAsComplete(): void { | ||
| if (this.currentStep === OnboardingStep.CONNECT_CARD) { | ||
| this.spenderOnboardingService.markConnectCardsStepAsComplete(); | ||
| } | ||
| if (this.currentStep === OnboardingStep.OPT_IN) { | ||
| this.spenderOnboardingService.markSmsOptInStepAsComplete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Marking steps to success!
All good, boss—but ensure the backend handles parallel calls if the user taps quickly to skip or complete. Watch for concurrency issues, my friend.
| import { Component } from '@angular/core'; | ||
| import { forkJoin, from, map, switchMap } from 'rxjs'; | ||
| import { ExtendedOrgUser } from 'src/app/core/models/extended-org-user.model'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unused import, Thalaivar!
ExtendedOrgUser is declared but not used. Let’s remove it to keep everything neat—like a polished blockbuster.
- import { ExtendedOrgUser } from 'src/app/core/models/extended-org-user.model';
+ // Removed since not referenced📝 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 { Component } from '@angular/core'; | |
| import { forkJoin, from, map, switchMap } from 'rxjs'; | |
| import { ExtendedOrgUser } from 'src/app/core/models/extended-org-user.model'; | |
| import { Component } from '@angular/core'; | |
| import { forkJoin, from, map, switchMap } from 'rxjs'; | |
| // Removed since not referenced |
🧰 Tools
🪛 eslint
[error] 3-3: 'ExtendedOrgUser' is defined but never used.
(@typescript-eslint/no-unused-vars)
| it('ionViewWillEnter(): should show loader and fetch onboarding data on ionViewWillEnter', (done) => { | ||
| loaderService.showLoader.and.resolveTo(); | ||
| orgUserService.getCurrent.and.returnValue(of(extendedOrgUserResponse)); | ||
| orgSettingsService.get.and.returnValue(of(orgSettingsData)); | ||
| spenderOnboardingService.getOnboardingStatus.and.returnValue(of(onboardingStatusData)); | ||
| corporateCreditCardExpenseService.getCorporateCards.and.returnValue(of([])); | ||
|
|
||
| component.ionViewWillEnter(); | ||
|
|
||
| fixture.whenStable().then(() => { | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(loaderService.showLoader).toHaveBeenCalledTimes(1); | ||
| expect(component.userFullName).toBe('Aiyush'); | ||
| expect(component.currentStep).toBe(OnboardingStep.CONNECT_CARD); | ||
| expect(component.isLoading).toBeFalse(); | ||
| expect(loaderService.hideLoader).toHaveBeenCalled(); | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Aha! Checking the Loader and Onboarding Data is Marvelous!
- The test flow is crisp: show loader, fetch user data, set component fields, hide loader.
- Ensuring the user name and step are set is top-class.
Everything looks top-notch, but do be sure there are fallback checks in case data is not returned.
| &__input-errors { | ||
| color: $red; | ||
| font-size: 12px; | ||
| line-height: 1.3; | ||
|
|
||
| & > :not(:first-child) { | ||
| // Only show one error message at a time | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mind it! Let's make our styles accessible to all!
The error messages need some style power-up for better accessibility, thalaiva!
Here's how we roll with accessibility:
&__input-errors {
color: $red;
font-size: 12px;
line-height: 1.3;
+ // Add for screen readers
+ &::before {
+ content: "Error: ";
+ clip: rect(1px, 1px, 1px, 1px);
+ position: absolute;
+ }
& > :not(:first-child) {
display: none;
}
}📝 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.
| &__input-errors { | |
| color: $red; | |
| font-size: 12px; | |
| line-height: 1.3; | |
| & > :not(:first-child) { | |
| // Only show one error message at a time | |
| display: none; | |
| } | |
| } | |
| &__input-errors { | |
| color: $red; | |
| font-size: 12px; | |
| line-height: 1.3; | |
| // Add for screen readers | |
| &::before { | |
| content: "Error: "; | |
| clip: rect(1px, 1px, 1px, 1px); | |
| position: absolute; | |
| } | |
| & > :not(:first-child) { | |
| // Only show one error message at a time | |
| display: none; | |
| } | |
| } |
| &__input-container { | ||
| padding: 16px 16px 8px 16px; | ||
| border-radius: 8px; | ||
| border: 1px solid $grey; | ||
|
|
||
| &:focus-within:not(&--error) { | ||
| border: 1px solid $black-light; | ||
| } | ||
| } | ||
|
|
||
| &__input-label { | ||
| font-size: 12px; | ||
| color: $black-light; | ||
| margin-bottom: 6px; | ||
| } | ||
|
|
||
| &__input-inner-container { | ||
| display: flex; | ||
| align-items: center; | ||
| border-bottom: 1px solid $grey; | ||
| padding-bottom: 6px; | ||
| margin-bottom: 2px; | ||
|
|
||
| &--error { | ||
| border-bottom: 1px solid $red; | ||
| } | ||
|
|
||
| &:focus-within:not(&--error) { | ||
| border-bottom: 1px solid $black-light; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style like a boss! Let's make these styles more powerful!
The input container styles are looking sharp, but let's make them even more maintainable with CSS custom properties, macha!
Here's a style transformation that would make even superstar proud:
+ :root {
+ --input-border-color: #{$grey};
+ --input-border-color-focus: #{$black-light};
+ --input-border-color-error: #{$red};
+ }
&__input-container {
padding: 16px 16px 8px 16px;
border-radius: 8px;
- border: 1px solid $grey;
+ border: 1px solid var(--input-border-color);
&:focus-within:not(&--error) {
- border: 1px solid $black-light;
+ border: 1px solid var(--input-border-color-focus);
}
}Committable suggestion skipped: line range outside the PR's diff.
| <span *ngIf="fg.controls['card_number_' + i]?.errors.invalidCardNumber" | ||
| >Please enter a valid card number.</span | ||
| > | ||
|
|
||
| <ng-container *ngIf="fg.controls['card_number_' + i]?.errors.invalidCardNetwork"> | ||
| <span *ngIf="isVisaRTFEnabled && isMastercardRTFEnabled; else visaOnlyOrg" | ||
| >Enter a valid Visa or Mastercard number. If you have other cards, please contact your admin.</span | ||
| > | ||
|
|
||
| <ng-template #visaOnlyOrg> | ||
| <!-- Check if only visa is enabled --> | ||
| <span | ||
| *ngIf=" | ||
| fg.controls['card_number_' + i]?.errors.invalidCardNetwork && isVisaRTFEnabled; | ||
| else mastercardOnlyOrg | ||
| " | ||
| >Enter a valid Visa number. If you have other cards, please contact your admin.</span | ||
| > | ||
| </ng-template> | ||
|
|
||
| <ng-template #mastercardOnlyOrg> | ||
| <!-- Check if only mastercard is enabled --> | ||
| <span *ngIf="fg.controls['card_number_' + i]?.errors.invalidCardNetwork && isMastercardRTFEnabled" | ||
| >Enter a valid Mastercard number. If you have other cards, please contact your admin.</span | ||
| > | ||
| </ng-template> | ||
| </ng-container> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Time to simplify these error messages like a boss!
These nested error messages are more complex than my punch dialogues! Let's make them simple and powerful!
Move the error message logic to the component:
// In component
getErrorMessage(index: number): string {
const control = this.fg.controls[`card_number_${index}`];
if (control?.errors?.invalidCardNumber) {
return 'Please enter a valid card number.';
}
if (control?.errors?.invalidCardNetwork) {
if (this.isVisaRTFEnabled && this.isMastercardRTFEnabled) {
return 'Enter a valid Visa or Mastercard number. If you have other cards, please contact your admin.';
}
if (this.isVisaRTFEnabled) {
return 'Enter a valid Visa number. If you have other cards, please contact your admin.';
}
if (this.isMastercardRTFEnabled) {
return 'Enter a valid Mastercard number. If you have other cards, please contact your admin.';
}
}
return '';
}Then simplify the template:
- <span *ngIf="fg.controls['card_number_' + i]?.errors.invalidCardNumber">Please enter a valid card number.</span>
- <ng-container *ngIf="fg.controls['card_number_' + i]?.errors.invalidCardNetwork">
- <!-- Complex nested conditions -->
- </ng-container>
+ <span>{{ getErrorMessage(i) }}</span>| <ion-button | ||
| class="btn-primary connect-card__primary-cta" | ||
| fill="clear" | ||
| aria-label="Navigate back to sign in page" | ||
| role="button" | ||
| (click)="enrollCards()" | ||
| > | ||
| Continue | ||
| </ion-button> |
There was a problem hiding this comment.
Mind it! This button needs some style!
The continue button's aria-label is talking about signing in when it should be talking about enrollment! Also, where's the loading state, thalaiva?
Let's fix this like a boss:
<ion-button
class="btn-primary connect-card__primary-cta"
fill="clear"
- aria-label="Navigate back to sign in page"
+ aria-label="Continue with card enrollment"
role="button"
+ [disabled]="fg.invalid || isLoading"
(click)="enrollCards()"
>
- Continue
+ <span>{{ isLoading ? 'Enrolling...' : 'Continue' }}</span>
</ion-button>Committable suggestion skipped: line range outside the PR's diff.
| <form formGroup="fg" *ngIf="fg"> | ||
| <div *ngIf="enrollableCards?.length > 0"> | ||
| <div *ngFor="let card of enrollableCards; let i = index"> | ||
| <div class="connect-card__input-label"> | ||
| <span>Corporate card</span> | ||
| </div> | ||
|
|
||
| <div | ||
| class="connect-card__input-inner-container" | ||
| [ngClass]="{ 'connect-card__input-inner-container--error': false }" | ||
| > | ||
| <input | ||
| class="smartlook-show connect-card__card-number-input pl-0" | ||
| inputmode="numeric" | ||
| placeholder="xxxx xxxx xxxx" | ||
| data-testid="card-number-input" | ||
| appAutofocus | ||
| [timeout]="500" | ||
| [formControlName]="'card_number_' + i" | ||
| required | ||
| (input)="onCardNumberUpdate(card, 'card_number_' + i)" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen up! The form needs some extra punch!
Your form validation is strong, but let's make it stronger than vibhuti!
Add these power-packed attributes to level up your form accessibility:
- <form formGroup="fg" *ngIf="fg">
+ <form formGroup="fg" *ngIf="fg" (ngSubmit)="enrollCards()" novalidate>
<div *ngIf="enrollableCards?.length > 0">
<div *ngFor="let card of enrollableCards; let i = index">
<div class="connect-card__input-label">
- <span>Corporate card</span>
+ <span [attr.id]="'card-label-' + i">Corporate card {{i + 1}}</span>
</div>
<input
class="smartlook-show connect-card__card-number-input pl-0"
inputmode="numeric"
placeholder="xxxx xxxx xxxx"
data-testid="card-number-input"
+ [attr.aria-labelledby]="'card-label-' + i"
+ [attr.aria-invalid]="fg.controls['card_number_' + i]?.invalid && fg.controls['card_number_' + i]?.touched"
appAutofocus
[timeout]="500"
[formControlName]="'card_number_' + i"
required
(input)="onCardNumberUpdate(card, 'card_number_' + i)"
/>📝 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.
| <form formGroup="fg" *ngIf="fg"> | |
| <div *ngIf="enrollableCards?.length > 0"> | |
| <div *ngFor="let card of enrollableCards; let i = index"> | |
| <div class="connect-card__input-label"> | |
| <span>Corporate card</span> | |
| </div> | |
| <div | |
| class="connect-card__input-inner-container" | |
| [ngClass]="{ 'connect-card__input-inner-container--error': false }" | |
| > | |
| <input | |
| class="smartlook-show connect-card__card-number-input pl-0" | |
| inputmode="numeric" | |
| placeholder="xxxx xxxx xxxx" | |
| data-testid="card-number-input" | |
| appAutofocus | |
| [timeout]="500" | |
| [formControlName]="'card_number_' + i" | |
| required | |
| (input)="onCardNumberUpdate(card, 'card_number_' + i)" | |
| /> | |
| <form formGroup="fg" *ngIf="fg" (ngSubmit)="enrollCards()" novalidate> | |
| <div *ngIf="enrollableCards?.length > 0"> | |
| <div *ngFor="let card of enrollableCards; let i = index"> | |
| <div class="connect-card__input-label"> | |
| <span [attr.id]="'card-label-' + i">Corporate card {{i + 1}}</span> | |
| </div> | |
| <div | |
| class="connect-card__input-inner-container" | |
| [ngClass]="{ 'connect-card__input-inner-container--error': false }" | |
| > | |
| <input | |
| class="smartlook-show connect-card__card-number-input pl-0" | |
| inputmode="numeric" | |
| placeholder="xxxx xxxx xxxx" | |
| data-testid="card-number-input" | |
| [attr.aria-labelledby]="'card-label-' + i" | |
| [attr.aria-invalid]="fg.controls['card_number_' + i]?.invalid && fg.controls['card_number_' + i]?.touched" | |
| appAutofocus | |
| [timeout]="500" | |
| [formControlName]="'card_number_' + i" | |
| required | |
| (input)="onCardNumberUpdate(card, 'card_number_' + i)" | |
| /> |
…-implementation-connect-cards-2
|
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) |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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.ts(2 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.html(1 hunks)
🔇 Additional comments (2)
src/app/fyle/spender-onboarding/spender-onboarding.page.html (1)
29-32: Mind it! The component integration looks super stylish! 🌟
The way you've connected the component with proper input and output bindings is as smooth as my signature moves! The orgSettings input and isStepCompleted event handler are perfectly choreographed.
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
1-21: Mind it! Clean up those unused imports, macha!
The following imports are not being used in the code:
FormArrayfinalizestatementUploadedCardvisaRTFCard
Let's keep our code as clean as my style! 😎
| <div class="spender-onboarding__component-container"> | ||
| <app-spender-onboarding-connect-card-step | ||
| [orgSettings]="orgSettings" | ||
| (isStepCompleted)="skipOnboardingStep()" | ||
| ></app-spender-onboarding-connect-card-step> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hey boss! Let's add some loading state magic! ✨
The component container could use a loading state to handle those micro-moments when the card data is being fetched. When you're as fast as lightning like me, every millisecond counts!
Here's a style-packed solution:
<div class="spender-onboarding__component-container">
+ <div *ngIf="isCardDataLoading" class="spender-onboarding__loading">
+ <ion-spinner name="crescent"></ion-spinner>
+ </div>
<app-spender-onboarding-connect-card-step
[orgSettings]="orgSettings"
(isStepCompleted)="skipOnboardingStep()"
+ *ngIf="!isCardDataLoading"
></app-spender-onboarding-connect-card-step>
</div>Committable suggestion skipped: line range outside the PR's diff.
| generateMessage(): string { | ||
| if (this.cardsList.successfulCards.length > 0) { | ||
| return 'We ran into an issue while processing your request. You can cancel and retry connecting the failed card or proceed to the next step.'; | ||
| } else if (this.cardsList.successfulCards.length > 0) { | ||
| 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 { | ||
| return ` | ||
| We ran into an issue while processing your request for the card ${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.`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Aiyyo! We've got a duplicate condition here, machan!
The first two conditions are exactly the same:
if (this.cardsList.successfulCards.length > 0) {
// First block
} else if (this.cardsList.successfulCards.length > 0) {
// Second block
}This means the second block will never execute! 🎭
Here's how to fix it with style:
- if (this.cardsList.successfulCards.length > 0) {
+ if (this.cardsList.successfulCards.length > 0 && this.cardsList.failedCards.length > 1) {
return 'We ran into an issue while processing your request. You can cancel and retry connecting the failed card or proceed to the next step.';
- } else if (this.cardsList.successfulCards.length > 0) {
+ } else if (this.cardsList.successfulCards.length > 0 && this.cardsList.failedCards.length === 1) {| ngOnInit(): void { | ||
| this.fg = this.fb.group({}); | ||
| this.corporateCreditCardExpensesService | ||
| .getCorporateCards() | ||
| .pipe( | ||
| map((corporateCards) => { | ||
| // Filter enrollable cards | ||
| this.enrollableCards = corporateCards.filter((card) => card.data_feed_source === 'STATEMENT_UPLOAD'); | ||
|
|
||
| // Add form controls for each enrollable card | ||
| this.enrollableCards.forEach((card, index) => { | ||
| const controlName = `card_number_${index}`; | ||
| this.cardValuesMap[card.id] = { | ||
| card_number: card.card_number, | ||
| card_type: CardNetworkType.OTHERS, | ||
| }; | ||
| this.fg.addControl( | ||
| controlName, | ||
| this.fb.control('', [ | ||
| Validators.required, | ||
| Validators.maxLength(12), | ||
| this.cardNumberValidator.bind(this), | ||
| this.cardNetworkValidator.bind(this), | ||
| ]) | ||
| ); | ||
| }); | ||
| }) | ||
| ) | ||
| .subscribe(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Looking sharp! But let's make it TypeScript-style strong, partner! 💪
The form initialization is solid, but we can make it even better with proper TypeScript types for the form controls.
+ interface CardFormControls {
+ [key: `card_number_${number}`]: FormControl<string>;
+ }
- fg: FormGroup;
+ fg: FormGroup<CardFormControls>;Also, consider using the new typed forms feature when upgrading to Angular 14+:
this.fg = this.fb.group<CardFormControls>({});| showErrorPopover(): void { | ||
| const errorPopover = this.popoverController.create({ | ||
| componentProps: { | ||
| title: 'Status summary', | ||
| message: this.generateMessage(), | ||
| primaryCta: { | ||
| text: 'Proceed anyway', | ||
| action: 'close', | ||
| }, | ||
| secondaryCta: { | ||
| text: 'Cancel', | ||
| action: 'cancel', | ||
| }, | ||
| cardsList: this.cardsList.successfulCards.length > 0 ? this.cardsList : {}, | ||
| }, | ||
| component: PopupAlertComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
|
|
||
| from(errorPopover) | ||
| .pipe( | ||
| tap((errorPopover) => errorPopover.present()), | ||
| switchMap((errorPopover) => errorPopover.onWillDismiss()), | ||
| map((response: OverlayResponse<{ action?: string }>) => { | ||
| if (response?.data?.action === 'close') { | ||
| this.isStepCompleted.emit(true); | ||
| } | ||
| }) | ||
| ) | ||
| .subscribe(noop); |
There was a problem hiding this comment.
Mind it! Another subscription needs cleanup, machan!
The subscription in showErrorPopover() also needs proper cleanup. Style is in the details! 😎
Modify your subscription:
- .subscribe(noop);
+ .pipe(takeUntil(this.destroy$))
+ .subscribe(noop);Committable suggestion skipped: line range outside the PR's diff.
| enrollCards(): void { | ||
| const cards = this.enrollableCards; | ||
| from(cards) | ||
| .pipe( | ||
| concatMap((card) => | ||
| this.realTimeFeedService.enroll(card.card_number, card.id).pipe( | ||
| map(() => { | ||
| this.cardsList.successfulCards.push(`**** ${card.card_number.slice(-4)}`); | ||
| }), | ||
| catchError(() => { | ||
| this.cardsList.failedCards.push(`**** ${card.card_number.slice(-4)}`); | ||
| return of(null); | ||
| }) | ||
| ) | ||
| ) | ||
| ) | ||
| .subscribe(() => { | ||
| if (this.cardsList.failedCards.length > 0) { | ||
| this.showErrorPopover(); | ||
| } else { | ||
| this.isStepCompleted.emit(true); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ey! Don't forget to clean up those subscriptions, partner!
The subscription in enrollCards() needs proper cleanup to prevent memory leaks. Let's handle it with style!
Add this to your class:
+ private destroy$ = new Subject<void>();
ngOnDestroy(): void {
+ this.destroy$.next();
+ this.destroy$.complete();
}Then modify your subscription:
- .subscribe(() => {
+ .pipe(takeUntil(this.destroy$))
+ .subscribe(() => {Committable suggestion skipped: line range outside the PR's diff.
| generateMessage(): string { | ||
| if (this.cardsList.successfulCards.length > 0) { | ||
| return 'We ran into an issue while processing your request. You can cancel and retry connecting the failed card or proceed to the next step.'; | ||
| } else if (this.cardsList.successfulCards.length > 0) { |
There was a problem hiding this comment.
why the same condition on both the if and else if? Even code rabbit did not notice this :D @bistaastha
| 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 |
There was a problem hiding this comment.
could be for the card or for the cards no? better be for the card(s)
There was a problem hiding this comment.
fixing this in follow up
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/app/core/models/popover-cards-list.model.ts(1 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(2 hunks)src/app/shared/components/popup-alert/popup-alert.component.scss(1 hunks)src/app/shared/components/popup-alert/popup-alert.component.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Run linters
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[failure] 4-4:
'FormArray' is defined but never used
[failure] 12-12:
'finalize' is defined but never used
[failure] 14-14:
'statementUploadedCard' is defined but never used
[failure] 14-14:
'visaRTFCard' is defined but never used
🪛 eslint
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 4-4: 'FormArray' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 12-12: 'finalize' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 14-14: 'statementUploadedCard' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 14-14: 'visaRTFCard' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (9)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (4)
64-87: Oi, Remember to Clean Up That Subscription, Machan!
This .subscribe in enrollCards() can cause memory leaks if the component sticks around. Pipe it with takeUntil(destroy$) and destroy it in ngOnDestroy(). This was pointed out previously too, so let's keep it short.
105-135: Subscription Cleanup Again, Superstar!
Same show in showErrorPopover()—it’s best to pipe our subscription with takeUntil(destroy$). Doing so ensures no leftover subscriptions that could hamper performance.
89-103: Check Your Message Logic, Da!
When this.cardsList.successfulCards.length > 0, you show a general error. Else if this.cardsList.failedCards.length > 0, you show a different message. The final else block references this.cardsList.failedCards again. Verify whether any scenario ends up in that final else accidentally. Might want to unify or clarify the conditions, machan.
144-173: Mightier with Typed Forms, Thalaiva!
You’re programmatically creating form controls. If you’re on Angular 14 or higher, typed reactive forms can add more power and clarity. But you’ve heard this tune before, yeah?
src/app/shared/components/popup-alert/popup-alert.component.ts (2)
3-3: Import Looks Good, But Verify Necessity, Da!
PopoverCardsList is used for the new input property below. If there's no usage beyond that, it's fine. If you plan to manipulate card statuses within this component, proceed, else reconsider. Keep code crisp, machan.
20-20: Well-Structured Inputs and Method, Superstar!
Adding @Input() cardsList: PopoverCardsList and the typed ctaClickedEvent(action: string): void is neat. This improves clarity for your popover interactions. Simply glorious, da!
Also applies to: 24-24
src/app/core/models/popover-cards-list.model.ts (1)
1-4: Sparkling Clean Interface, Machan!
This PopoverCardsList structure is tight and crisp. Perfect for consolidating success and failure states. Carry on, boss!
src/app/shared/components/popup-alert/popup-alert.component.scss (2)
57-109: Impressive Visuals, Superstar!
These success/error tick containers stand out well. Great job giving users strong visual feedback!
90-100: 🧹 Nitpick (assertive)
Duplicate Padding, Thalaiva!
You're setting padding: 12px; twice in .popup-alert--content-container. Remove one to keep a single punch, da!
&--content-container {
...
padding: 12px;
...
- padding: 12px;
...
}Likely invalid or redundant comment.
| import { Component, EventEmitter, Input, OnChanges, OnInit, Output, SimpleChanges } from '@angular/core'; | ||
| import { | ||
| AbstractControl, | ||
| FormArray, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Aiyyo, Unused Imports Detected, Da!
FormArray, finalize, statementUploadedCard, and visaRTFCard are not used anywhere. Time to remove them and keep your code dancing light, machan!
- import {
- FormArray,
- ...
- } from '@angular/forms';
- import { catchError, concatMap, finalize, from, ... } from 'rxjs';
- import { statementUploadedCard, visaRTFCard } from 'src/app/core/mock-data/platform-corporate-card.data';Also applies to: 12-12, 14-14
🧰 Tools
🪛 GitHub Check: Run linters
[failure] 4-4:
'FormArray' is defined but never used
🪛 eslint
[error] 4-4: 'FormArray' is defined but never used.
(@typescript-eslint/no-unused-vars)
Changes in the PR:
Fixes to be added in follow up:
Preview:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation