feat: Trackers for onboarding flow #3428
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. WalkthroughListen up, superstar! This pull request brings a seismic shift in the onboarding arena! We’re rolling out the powerful Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant TrackingService
User->>Component: Starts Onboarding
Component->>TrackingService: Track Initialization Event
User->>Component: Completes Step
Component->>TrackingService: Track Step Completion Event
Possibly related PRs
Suggested Labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (2)
Line range hint
71-266: Thalaiva suggests better test organization!Your test cases are strong like Rajini's punch dialogues, but we can make them even more powerful! Let's reorganize them into focused describe blocks, superstar style:
describe('Card Enrollment', () => { describe('Single Card', () => { // Move single card enrollment tests here }); describe('Multiple Cards', () => { // Move multiple card enrollment tests here }); describe('Error Handling', () => { // Move error handling tests here }); });
Line range hint
27-69: Optimize test setup like my perfect timing in action scenes!The test setup is good, but we can make it run faster than my signature cigarette flip! Here's how:
- Move common spy setup to a helper function:
function setupCommonSpies() { const trackingServiceSpy = jasmine.createSpyObj('TrackingService', ['eventTrack']); const realTimeFeedServiceSpy = jasmine.createSpyObj('RealTimeFeedService', [ 'enroll', 'getCardTypeFromNumber', 'isCardNumberValid', ]); return { trackingServiceSpy, realTimeFeedServiceSpy }; }
- Reuse mock data instead of creating new objects in each test:
const MOCK_CARD = Object.freeze({ ...statementUploadedCard });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/app/fyle/my-profile/my-profile.page.ts(0 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts(4 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(4 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.spec.ts(0 hunks)src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts(4 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts(5 hunks)src/app/fyle/spender-onboarding/spender-onboarding.page.ts(4 hunks)
💤 Files with no reviewable changes (2)
- src/app/fyle/my-profile/my-profile.page.ts
- src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (10)
src/app/fyle/spender-onboarding/spender-onboarding.page.ts (3)
14-14: Mind it! The TrackingService integration looks perfect!The service is properly imported and injected into the constructor. This is how we do it in style!
Also applies to: 48-49
122-122: Superstar! The tracking event for skipping connect cards is spot on!The event is strategically placed after the skip action, maintaining a clear cause-and-effect relationship.
134-134: Kabali da! The SMS opt-in skip tracking is perfectly placed!The event follows the same consistent pattern as the connect cards tracking. That's what I call style!
src/app/fyle/spender-onboarding/spender-onboarding-opt-in-step/spender-onboarding-opt-in-step.component.ts (3)
138-138: Thalaiva style! The view tracking is perfectly initialized!The component view tracking is added right where it should be - in ngOnInit. That's how we roll!
150-150: Mind it! The edit number tracking is perfectly timed!The tracking event captures the user's intention to edit their number at just the right moment.
289-289: Marana mass! The completion tracking is perfectly executed!The successful completion event is placed right after the verification success. Perfect timing!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (2)
224-224: Style! The connect cards view tracking is perfectly placed!The component view tracking is initialized at the right moment in ngOnInit.
293-295: Superstar! The completion tracking with card count is mass!The event includes the number of successfully connected cards, providing valuable context for analytics.
src/app/fyle/spender-onboarding/spender-onboarding.page.spec.ts (2)
17-17: Mind it! The test setup for TrackingService is perfect!The service is properly imported and a spy object is created for testing.
Also applies to: 28-28
58-58: Kabali da! The test module setup is perfect!The TrackingService is properly provided in the test module configuration.
| @@ -227,6 +218,7 @@ export class SpenderOnboardingOptInStepComponent implements OnInit, OnChanges { | |||
| 'msb-failure-with-camera-icon' | |||
| ); | |||
| this.disableResendOtp = true; | |||
| this.trackingService.eventTrack('Sms Opt In Onboarding Step - Max Otp Attempts Reached'); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen up! Let's make this OTP tracking even more powerful!
While the tracking for max OTP attempts is good, we could make it even better by including additional context.
Consider adding these details to the event:
-this.trackingService.eventTrack('Sms Opt In Onboarding Step - Max Otp Attempts Reached');
+this.trackingService.eventTrack('Sms Opt In Onboarding Step - Max Otp Attempts Reached', {
+ attemptsCount: this.otpAttemptsLeft,
+ mobileNumber: this.mobileNumberInputValue?.slice(-4) || 'unknown'
+});Committable suggestion skipped: line range outside the PR's diff.
| @@ -42,6 +44,7 @@ describe('SpenderOnboardingPage', () => { | |||
| 'getCorporateCards', | |||
| ]); | |||
| const routerSpy = jasmine.createSpyObj('Router', ['navigate']); | |||
| const trackingServiceSpy = jasmine.createSpyObj('TrackingService', ['eventTrack']); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen up! We need more power in our tests!
The spy is created but we're missing test cases to verify the tracking events.
Add these test cases to verify tracking events:
it('should track event when connect cards step is skipped', fakeAsync(() => {
component.currentStep = OnboardingStep.CONNECT_CARD;
component.skipOnboardingStep();
tick();
expect(trackingService.eventTrack).toHaveBeenCalledWith('Connect Cards Onboarding Step - Skipped');
}));
it('should track event when sms opt-in step is skipped', fakeAsync(() => {
component.currentStep = OnboardingStep.OPT_IN;
component.skipOnboardingStep();
tick();
expect(trackingService.eventTrack).toHaveBeenCalledWith('Sms Opt In Onboarding Step - Skipped');
}));| @@ -13,6 +13,7 @@ import { SimpleChanges } from '@angular/core'; | |||
| import { orgSettingsData } from 'src/app/core/test-data/org-settings.service.spec.data'; | |||
| import { PopupAlertComponent } from 'src/app/shared/components/popup-alert/popup-alert.component'; | |||
| import { enrollmentErrorPopoverData1, enrollmentErrorPopoverData2 } from 'src/app/core/mock-data/modal-controller.data'; | |||
| import { TrackingService } from 'src/app/core/services/tracking.service'; | |||
There was a problem hiding this comment.
Mind it! The TrackingService needs test coverage, macha!
You've added the TrackingService like a boss, but where are the tests to verify the tracking events? Just like how I never miss my signature moves in movies, we shouldn't miss testing these critical tracking calls!
Here's what you need to add, style ah?
- Test cases for tracking initialization events
- Test cases for card enrollment completion events
- Verification of event parameters
Example test case to add:
it('should track card enrollment completion', fakeAsync(() => {
component.enrollCards();
tick();
expect(trackingService.eventTrack).toHaveBeenCalledWith('Onboarding Card Enrollment Completed', {
// Add expected parameters here
});
}));Also applies to: 25-25, 32-32, 47-47, 61-61
bistaastha
changed the base branch from
FYLE-qa-branch
to
FYLE-86cx2t82k-base-feature-branch
bistaastha
merged commit c176bc8
into
FYLE-86cx2t82k-base-feature-branch
Summary by CodeRabbit
New Features
Tests
TrackingService.SpenderOnboardingOptInStepComponent.Refactor
TrackingServiceinto multiple onboarding components.