fix: Add additional trackers for onboarding #3477
Conversation
WalkthroughThe changes enhance error tracking and error handling during the onboarding process. In the card enrollment component, additional tracking events now capture error details and card identification. In the onboarding page, several tracking events log user redirection actions and errors, with modifications added to the control flow. A new private method has been introduced for consistent error management. No exported entities have been altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CardEnrollmentComponent as Component
participant TrackingService as Tracker
User->>Component: Initiate card enrollment
Component-->>User: Error occurs during enrollment
Component->>Tracker: Log tracking event (card error details)
Tracker-->>Component: Acknowledgement of log
sequenceDiagram
participant User
participant OnboardingPage as Page
participant TrackingService as Tracker
User->>Page: Begin onboarding process
Page->>Page: Evaluate redirection conditions (mobile verified, Amex, RTF cards)
Page->>Tracker: Log tracking event for redirection or step skipping
alt Error encountered
Page->>Page: Invoke private handleError method
Page->>Tracker: Log error tracking event
end
Page-->>User: Redirect or display steps
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
270-270: Update TODO comment for Angular version.Kabali da! This TODO comment about Angular 14 is outdated. Time to update it for the latest Angular version.
- // TODO (Angular 14 >): Remove the type casting and directly use string type for the form control + // TODO (Angular 16+): Remove the type casting and use strongly typed forms
📜 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.ts(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Run linters
src/app/fyle/spender-onboarding/spender-onboarding.page.ts
[failure] 2-2:
'catchError' is defined but never used
[failure] 211-211:
Unsafe assignment of an any value
[failure] 212-212:
Unsafe return of an any typed value
[failure] 212-212:
Unsafe call of an any typed value
🪛 ESLint
src/app/fyle/spender-onboarding/spender-onboarding.page.ts
[error] 2-2: 'catchError' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Actions: Unit Tests
src/app/fyle/spender-onboarding/spender-onboarding.page.ts
[error] 212-212: Cannot find name 'of'.
🪛 GitHub Actions: Lint
src/app/fyle/spender-onboarding/spender-onboarding.page.ts
[error] 2-2: 'catchError' is defined but never used @typescript-eslint/no-unused-vars
| @@ -1,5 +1,5 @@ | |||
| import { Component } from '@angular/core'; | |||
| import { finalize, forkJoin, from, map, Observable, switchMap } from 'rxjs'; | |||
| import { catchError, finalize, forkJoin, from, map, Observable, switchMap } from 'rxjs'; | |||
There was a problem hiding this comment.
Fix the unused import and missing 'of' operator.
Mind games! You imported 'catchError' but never used it, and 'of' operator is missing from imports.
-import { catchError, finalize, forkJoin, from, map, Observable, switchMap } from 'rxjs';
+import { finalize, forkJoin, from, map, Observable, of, switchMap } from 'rxjs';📝 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 { catchError, finalize, forkJoin, from, map, Observable, switchMap } from 'rxjs'; | |
| import { finalize, forkJoin, from, map, Observable, of, switchMap } from 'rxjs'; |
🧰 Tools
🪛 GitHub Check: Run linters
[failure] 2-2:
'catchError' is defined but never used
🪛 ESLint
[error] 2-2: 'catchError' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Actions: Lint
[error] 2-2: 'catchError' is defined but never used @typescript-eslint/no-unused-vars
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| private handleError(error: any, event: string): Observable<Record<string, never>> { | ||
| this.trackingService.eventTrack(event, { error }); | ||
| return of({}); | ||
| } |
There was a problem hiding this comment.
Fix type safety issues in handleError method.
Listen up! The handleError method needs proper type safety. Let's fix those TypeScript errors like a boss!
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- private handleError(error: any, event: string): Observable<Record<string, never>> {
- this.trackingService.eventTrack(event, { error });
- return of({});
+ private handleError(error: Error | unknown, event: string): Observable<Record<string, never>> {
+ this.trackingService.eventTrack(event, { error: error instanceof Error ? error.message : String(error) });
+ return of({});📝 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.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| private handleError(error: any, event: string): Observable<Record<string, never>> { | |
| this.trackingService.eventTrack(event, { error }); | |
| return of({}); | |
| } | |
| private handleError(error: Error | unknown, event: string): Observable<Record<string, never>> { | |
| this.trackingService.eventTrack(event, { error: error instanceof Error ? error.message : String(error) }); | |
| return of({}); | |
| } |
🧰 Tools
🪛 GitHub Check: Run linters
[failure] 211-211:
Unsafe assignment of an any value
[failure] 212-212:
Unsafe return of an any typed value
[failure] 212-212:
Unsafe call of an any typed value
🪛 GitHub Actions: Unit Tests
[error] 212-212: Cannot find name 'of'.
| const onlyAmexEnabled = orgSettings.amex_feed_enrollment_settings.enabled && !isRtfEnabled; | ||
| const rtfCards = corporateCards.filter((card) => card.is_visa_enrolled || card.is_mastercard_enrolled); | ||
| if (this.isMobileVerified(this.eou) && rtfCards.length > 0) { | ||
| if (this.isMobileVerified(this.eou) && (onlyAmexEnabled || rtfCards.length > 0)) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve variable naming for better clarity.
Style, my friend! Let's make that variable name more descriptive.
- const onlyAmexEnabled = orgSettings.amex_feed_enrollment_settings.enabled && !isRtfEnabled;
+ const isOnlyAmexEnabled = orgSettings.amex_feed_enrollment_settings.enabled && !isRtfEnabled;Committable suggestion skipped: line range outside the PR's diff.
| this.trackingService.eventTrack('Connect Cards Onboarding Step - Failed', { | ||
| CardNumber: card.card_number.slice(-4), | ||
| error, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance error tracking with structured error details.
Mind it! Let's structure those error details better for tracking.
- this.trackingService.eventTrack('Connect Cards Onboarding Step - Failed', {
- CardNumber: card.card_number.slice(-4),
- error,
- });
+ this.trackingService.eventTrack('Connect Cards Onboarding Step - Failed', {
+ cardNumber: card.card_number.slice(-4),
+ errorCode: error.status,
+ errorMessage: error.message || 'Unknown error',
+ });Apply similar changes to the singular card enrollment error tracking at lines 105-108.
Also applies to: 105-108
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/fyle/spender-onboarding/spender-onboarding.page.ts(4 hunks)
🔇 Additional comments (2)
src/app/fyle/spender-onboarding/spender-onboarding.page.ts (2)
2-2: Mind it! Fix those imports, they are like my signature moves - they need to be perfect!The import statement needs the 'of' operator but doesn't need 'catchError'. Let's style it up!
-import { finalize, forkJoin, from, map, Observable, switchMap } from 'rxjs'; +import { finalize, forkJoin, from, map, Observable, of, switchMap } from 'rxjs';
120-122: Style, my friend! Let's make that variable name more descriptive.-const onlyAmexEnabled = orgSettings.amex_feed_enrollment_settings.enabled && !isRtfEnabled; +const isOnlyAmexEnabled = orgSettings.amex_feed_enrollment_settings.enabled && !isRtfEnabled;
| @@ -62,6 +62,7 @@ export class SpenderOnboardingPage { | |||
| this.onboardingComplete = true; | |||
| this.startCountdown(); | |||
| } else { | |||
| this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Skip'); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen up! Your tracking events need to follow a style, just like my movies follow a style!
The tracking event names could be more consistent. Some use "Redirect To" while others use different formats. Let's make them all follow the same pattern:
- "Redirect To Dashboard After Onboarding Skip"
- "Skip Connect Cards Onboarding Step - Cards Already Enrolled"
- "Skip Sms Opt In Onboarding Step - Mobile Already Verified"
- "Redirect To Dashboard From Onboarding As No Steps To Show"
- "Spender Onboarding"
- "Redirect To Dashboard After Onboarding Success"
Consider standardizing the event names. For example:
-this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Skip');
+this.trackingService.eventTrack('Onboarding: Dashboard Redirect - Skip');
-this.trackingService.eventTrack('Skip Connect Cards Onboarding Step - Cards Already Enrolled');
+this.trackingService.eventTrack('Onboarding: Skip Cards Step - Already Enrolled');Also applies to: 86-88, 96-96, 123-123, 132-132, 203-203
🛠️ Refactor suggestion
When you handle errors like a boss, your code becomes legendary!
The tracking service calls need proper error handling. Let's add a private method to handle tracking errors:
+private handleTrackingError(error: Error | unknown): void {
+ console.error('Tracking failed:', error instanceof Error ? error.message : String(error));
+}
-this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Skip');
+this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Skip').subscribe({
+ error: (err) => this.handleTrackingError(err)
+});Also applies to: 86-88, 96-96, 123-123, 132-132, 203-203
…yle-mobile-app into FYLE-adding-additional-trackers
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (1)
82-174: Hey, test coverage needs more power!The test suite for
ionViewWillEntershould include additional test cases to verify error handling and edge cases during onboarding.Consider adding test cases for:
- Error scenarios during API calls
- Edge cases with different org settings combinations
- Network failure scenarios
+it('should handle API errors gracefully', (done) => { + loaderService.showLoader.and.resolveTo(); + orgUserService.getCurrent.and.returnValue(throwError(() => new Error('API Error'))); + + component.ionViewWillEnter(); + + fixture.whenStable().then(() => { + fixture.detectChanges(); + expect(component.isLoading).toBeFalse(); + expect(loaderService.hideLoader).toHaveBeenCalled(); + done(); + }); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (1)
106-106: Magizhchi! Test data update looks good!The change from
extendedOrgUserResponsetoapiEouResand corresponding user name update maintains consistency with the new test data structure.Also applies to: 117-117
| @@ -16,6 +16,7 @@ import { orgSettingsWoTaxAndRtf } from 'src/app/core/mock-data/org-settings.data | |||
| import { statementUploadedCard } from 'src/app/core/mock-data/platform-corporate-card.data'; | |||
| import { TrackingService } from 'src/app/core/services/tracking.service'; | |||
| import { apiEouRes } from 'src/app/core/mock-data/extended-org-user.data'; | |||
| import { orgSettingsCardsDisabled } from 'src/app/core/test-data/org-settings.service.spec.data'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Remove unused import, partner!
The imported orgSettingsCardsDisabled is not being used in any test case. Let's keep our code clean and remove this unused import.
-import { orgSettingsCardsDisabled } from 'src/app/core/test-data/org-settings.service.spec.data';📝 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 { orgSettingsCardsDisabled } from 'src/app/core/test-data/org-settings.service.spec.data'; | |
| // (The unused import line has been removed) |
|
Summary by CodeRabbit