test: Add tests for Corporate Card task #3370
Conversation
WalkthroughWith the power of a thousand supercomputers, I bring forth a transformation in the corporate card management realm! A new task event has emerged, expanding the horizons of our application. The changes introduce a comprehensive approach to corporate card enrollment, weaving new methods into the fabric of our tasks service and component. Like a digital superhero, the code now dances with enhanced capabilities to handle corporate card interactions! Changes
Sequence DiagramsequenceDiagram
participant User
participant TasksComponent
participant TasksService
participant PopoverController
participant CorporateCreditCardService
User->>TasksComponent: Click Add Corporate Card
TasksComponent->>PopoverController: Create Popover
PopoverController-->>User: Show Add Card Options
User->>CorporateCreditCardService: Initiate Card Enrollment
CorporateCreditCardService-->>TasksComponent: Update Task Status
TasksComponent->>TasksService: Refresh Tasks
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🪧 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 (
|
| Asset: 'Mobile', | ||
| header: task.header, | ||
| }); | ||
| // eslint-disable-next-line complexity |
There was a problem hiding this comment.
will raise a follow up PR to fix this
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
src/app/fyle/dashboard/tasks/tasks.component.ts (1)
Line range hint
403-447: Switch-case style: Crisp, clear, and unstoppable!
Renaming from 'onTaskClicked' to 'loadTasksMap' is okay, but the new name might be less intuitive for the uninitiated. Keep that in mind, thalaiva!
🛑 Comments failed to post (9)
src/app/fyle/dashboard/tasks/tasks.component.ts (2)
474-484: 🧹 Nitpick (assertive)
Enrollment success flow is top-notch!
Clearing the cache and showing the success popover is a triumphant approach. Perhaps also handle potential errors fromclearCache()to keep the user journey silky smooth.
486-507: 🛠️ Refactor suggestion
Corporate card popover? That’s how we do it, boss!
The forking setup is impressive. If an error occurs in these calls, you might want to handle or notify the user, though. Keep your fans happy.+ // Add error handling inside subscribe + // e.g., catchError(...) or a graceful user-facing messageCommittable suggestion skipped: line range outside the PR's diff.
src/app/fyle/dashboard/tasks/tasks-1.component.spec.ts (2)
44-45: 🧹 Nitpick (assertive)
Mind-blowing! But let's add more test coverage, macha!
The imports look good, but we need more test coverage for OrgSettingsService, like testing different scenarios when org settings are not available.
93-96: 🛠️ Refactor suggestion
Style ah style! But let's make it more robust!
The test setup looks good, but consider adding negative test cases:
orgSettingsService.get.and.returnValue(of(orgSettingsPendingRestrictions)); component.isVisaRTFEnabled$ = of(true); component.isMastercardRTFEnabled$ = of(true); component.isYodleeEnabled$ = of(true); + +// Add negative test case +it('should handle when org settings are not available', () => { + orgSettingsService.get.and.returnValue(of(null)); + fixture.detectChanges(); + expect(component.setupNetworkWatcher).toHaveBeenCalledTimes(1); +});Committable suggestion skipped: line range outside the PR's diff.
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
203-232: 🛠️ Refactor suggestion
Kabali style testing! But let's add more power to it!
The test for onAddCorporateCardClick is well-written, but let's add error handling scenarios:
it('onAddCorporateCardClick(): should open card popover', fakeAsync(() => { // Existing test code... })); +it('should handle popover creation failure', fakeAsync(() => { + component.isVisaRTFEnabled$ = of(true); + component.isMastercardRTFEnabled$ = of(true); + component.isYodleeEnabled$ = of(true); + + popoverController.create.and.rejectWith(new Error('Failed to create popover')); + + component.onAddCorporateCardClick(); + tick(); + + expect(popoverController.create).toHaveBeenCalled(); +}));Committable suggestion skipped: line range outside the PR's diff.
src/app/core/services/tasks.service.ts (2)
585-601: 🧹 Nitpick (assertive)
Single-ah style! But let's make it type-safe!
The mapAddCorporateCardTask implementation needs type safety:
-mapAddCorporateCardTask(): DashboardTask[] { +mapAddCorporateCardTask(): DashboardTask[] { + interface CardTask { + hideAmount: boolean; + header: string; + subheader: string; + icon: TaskIcon; + ctas: Array<{ + content: string; + event: TASKEVENT; + }>; + } + const task = [ - { + <CardTask>{ hideAmount: true, header: 'Add Corporate Card', subheader: 'Add your corporate card to track expenses.',Committable suggestion skipped: line range outside the PR's diff.
534-550:
⚠️ Potential issueMind it! Let's add some error handling, thalaiva!
The getAddCorporateCardTask implementation needs error handling:
getAddCorporateCardTask(): Observable<DashboardTask[]> { - return forkJoin([this.orgSettingsService.get(), this.corporateCreditCardExpenseService.getCorporateCards()]).pipe( + return forkJoin([ + this.orgSettingsService.get().pipe( + catchError((error) => { + console.error('Failed to fetch org settings:', error); + return of(null); + }) + ), + this.corporateCreditCardExpenseService.getCorporateCards().pipe( + catchError((error) => { + console.error('Failed to fetch corporate cards:', error); + return of([]); + }) + ) + ]).pipe( map(([orgSettings, cards]) => { + if (!orgSettings) { + return [] as DashboardTask[]; + } const isRtfEnabled = (orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) || (orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled);📝 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.getAddCorporateCardTask(): Observable<DashboardTask[]> { return forkJoin([ this.orgSettingsService.get().pipe( catchError((error) => { console.error('Failed to fetch org settings:', error); return of(null); }) ), this.corporateCreditCardExpenseService.getCorporateCards().pipe( catchError((error) => { console.error('Failed to fetch corporate cards:', error); return of([]); }) ) ]).pipe( map(([orgSettings, cards]) => { if (!orgSettings) { return [] as DashboardTask[]; } const isRtfEnabled = (orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) || (orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled); const isCCCEnabled = orgSettings.corporate_credit_card_settings.allowed && orgSettings.corporate_credit_card_settings.enabled; const rtfCards = cards.filter((card) => card.is_visa_enrolled || card.is_mastercard_enrolled); if (isRtfEnabled && isCCCEnabled && rtfCards.length === 0) { return this.mapAddCorporateCardTask(); } else { return [] as DashboardTask[]; } }) ); }src/app/core/services/tasks.service.spec.ts (2)
437-444: 🧹 Nitpick (assertive)
Style-ah test case implementation, but let's add more power to it!
The test case correctly verifies the scenario with enrolled cards. However, we should add one more test case to handle error scenarios.
Add this test case to handle error scenarios:
it('should handle errors gracefully', (done) => { corporateCreditCardExpenseService.getCorporateCards.and.returnValue(throwError(() => new Error('API Error'))); tasksService.getAddCorporateCardTask().subscribe({ error: (err) => { expect(corporateCreditCardExpenseService.getCorporateCards).toHaveBeenCalled(); expect(err.message).toEqual('API Error'); done(); } }); });
806-806:
⚠️ Potential issueAiyyo! We've got a duplicate setup here, machan!
This line is duplicating the corporate card setup that's already done at line 805. Remove this duplicate setup to avoid any potential test inconsistencies.
Apply this change:
- corporateCreditCardExpenseService.getCorporateCards.and.returnValue(of([mastercardRTFCard]));
|
Follow up to: #3362
Summary by CodeRabbit
New Features
Bug Fixes
Tests