Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add additional trackers for onboarding #3477

Merged
merged 6 commits into from
Feb 4, 2025
+22 −5
Merged

fix: Add additional trackers for onboarding #3477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b811db2
Minor
bistaastha Feb 4, 2025
9994e9c
Minor
bistaastha Feb 4, 2025
3ca6f8c
Update spender-onboarding.page.ts
bistaastha Feb 4, 2025
2e0a3f0
Minor
bistaastha Feb 4, 2025
585f780
Merge branch 'master' into FYLE-adding-additional-trackers
bistaastha Feb 4, 2025
df07ff8
Merge branch 'FYLE-adding-additional-trackers' of github.com:fylein/f…
bistaastha Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...ng/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ export class SpenderOnboardingConnectCardStepComponent implements OnInit, OnChan
this.cardValuesMap[card.id].enrollment_success = true;
}),
catchError((error: HttpErrorResponse) => {
this.trackingService.eventTrack('Connect Cards Onboarding Step - Failed', {
CardNumber: card.card_number.slice(-4),
error,
});
Comment on lines +81 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

this.setupErrorMessages(error, `${card.card_number.slice(-4)}`, card.id);
return of(error);
})
Expand All @@ -98,6 +102,10 @@ export class SpenderOnboardingConnectCardStepComponent implements OnInit, OnChan
this.cardsList.successfulCards.push(`**** ${(this.fg.controls.card_number.value as string).slice(-4)}`);
}),
catchError((error: HttpErrorResponse) => {
this.trackingService.eventTrack('Connect Cards Onboarding Step - Failed', {
CardNumber: (this.fg.controls.card_number.value as string).slice(-4),
error,
});
this.setupErrorMessages(error, (this.fg.controls.card_number.value as string).slice(-4));
return of(error);
}),
Expand Down
5 changes: 3 additions & 2 deletions src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
import { orgSettingsCardsDisabled } from 'src/app/core/test-data/org-settings.service.spec.data';
// (The unused import line has been removed)


describe('SpenderOnboardingPage', () => {
let component: SpenderOnboardingPage;
Expand Down Expand Up @@ -102,7 +103,7 @@ describe('SpenderOnboardingPage', () => {

it('should go to Opt in step when RTF is disabled', (done) => {
loaderService.showLoader.and.resolveTo();
orgUserService.getCurrent.and.returnValue(of(extendedOrgUserResponse));
orgUserService.getCurrent.and.returnValue(of(apiEouRes));
orgSettingsService.get.and.returnValue(of(orgSettingsWoTaxAndRtf));
spenderOnboardingService.getOnboardingStatus.and.returnValue(of(onboardingStatusData));
corporateCreditCardExpenseService.getCorporateCards.and.returnValue(of([statementUploadedCard]));
Expand All @@ -113,7 +114,7 @@ describe('SpenderOnboardingPage', () => {
fixture.detectChanges();

expect(loaderService.showLoader).toHaveBeenCalledTimes(1);
expect(component.userFullName).toBe('Aiyush');
expect(component.userFullName).toBe('Abhishek Jain');
expect(component.currentStep).toBe(OnboardingStep.OPT_IN);
expect(component.isLoading).toBeFalse();
done();
Expand Down
14 changes: 11 additions & 3 deletions src/app/fyle/spender-onboarding/spender-onboarding.page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export class SpenderOnboardingPage {
this.onboardingComplete = true;
this.startCountdown();
} else {
this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Skip');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

this.router.navigate(['/', 'enterprise', 'my_dashboard']);
}
}
Expand All @@ -82,13 +83,17 @@ export class SpenderOnboardingPage {
this.showOneStep = true;
}
} else if (rtfCards.length > 0) {
this.trackingService.eventTrack('Skip Connect Cards Onboarding Step - Cards Already Enrolled', {
numberOfEnrolledCards: rtfCards.length,
});
this.areCardsEnrolled = true;
this.currentStep = OnboardingStep.OPT_IN;
this.showOneStep = true;
this.spenderOnboardingService.skipConnectCardsStep().subscribe();
} else {
this.currentStep = OnboardingStep.CONNECT_CARD;
if (this.isMobileVerified(this.eou)) {
this.trackingService.eventTrack('Skip Sms Opt In Onboarding Step - Mobile Already Verified');
this.showOneStep = true;
}
}
Expand All @@ -112,17 +117,19 @@ export class SpenderOnboardingPage {
this.orgSettings = orgSettings;
const isRtfEnabled =
orgSettings.visa_enrollment_settings.enabled || orgSettings.mastercard_enrollment_settings.enabled;
const isAmexFeedEnabled = orgSettings.amex_feed_enrollment_settings.enabled;
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)) {
Comment on lines +120 to +122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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('Redirect To Dashboard From Onboarding As No Steps To Show');
this.completeOnboarding().subscribe();
} else if (isAmexFeedEnabled && !isRtfEnabled) {
} else if (onlyAmexEnabled) {
this.currentStep = OnboardingStep.OPT_IN;
this.showOneStep = true;
} else if (isRtfEnabled) {
// If Connect Card was skipped earlier or Cards are already enrolled, then go to OPT_IN step
this.setUpRtfSteps(onboardingStatus, rtfCards);
}
this.trackingService.eventTrack('Spender Onboarding', { numberOfEnrollableCards: rtfCards.length });
}),
finalize(() => {
this.isLoading = false;
Expand Down Expand Up @@ -193,6 +200,7 @@ export class SpenderOnboardingPage {
this.redirectionCount--;
if (this.redirectionCount === 0) {
clearInterval(interval);
this.trackingService.eventTrack('Redirect To Dashboard After Onboarding Success');
this.router.navigate(['/', 'enterprise', 'my_dashboard']);
}
}, 1000);
Expand Down