fix: Adds missing tests for the component #3385
Conversation
Walkthrough[In Rajinikanth's stylish swagger] Superstar style update incoming! We've turbocharged the tasks and corporate card management in our Angular application. The changes are like a mass action sequence - adding new methods, importing powerful services, and creating epic test scenarios that'll make even run configurations bow down! From JUnit configurations to corporate card enrollment, this PR is a full-blown masala movie of code transformation. Changes
Sequence DiagramsequenceDiagram
participant User
participant TasksComponent
participant PopoverController
participant CorporateCreditCardService
User->>TasksComponent: Click Add Corporate Card
TasksComponent->>CorporateCreditCardService: Check Card Eligibility
CorporateCreditCardService-->>TasksComponent: Return Eligibility
TasksComponent->>PopoverController: Create Card Addition Popover
PopoverController-->>User: Display Popover
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
[Throws sunglasses dramatically] 🪧 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
android/.idea/runConfigurations.xml(1 hunks)src/app/auth/sign-in/sign-in.page.html(1 hunks)src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts(4 hunks)src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts(1 hunks)src/app/fyle/dashboard/tasks/tasks.component.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
🔇 Additional comments (4)
android/.idea/runConfigurations.xml (1)
6-13: Mind it! These configuration changes are perfectly aligned with our testing mission!
The transformation from Compose preview to JUnit and Kotlin test configurations is exactly what we need for better test coverage. Like a well-choreographed action sequence, each configuration producer serves its purpose:
- JUnit configurations for different test discovery methods
- Kotlin-specific test producers for seamless integration
src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts (1)
73-73: When I add a spy method, I make sure it's well tested!
The addition of clearCache to the spy object is good, but let's verify its test coverage like a true superstar!
✅ Verification successful
Let me broaden the search to catch this elusive method like a true superstar! flips sunglasses
Mind games won't work with me! Let me find those test cases with the precision of a superstar! flips glasses
Mind it! The clearCache method is already well tested, superstar!
I can see with my signature style that the clearCache method is thoroughly tested across multiple components:
- The method implementation exists in
corporate-credit-card-expense.service.ts - It's being tested in
tasks-2.component.spec.tswithin thehandleEnrollmentSuccesstest case - Additional test coverage exists in
manage-corporate-cardsandcard-statscomponents
When you add a spy method, make sure it's tested - and this one definitely is! The test coverage is as solid as my action sequences! flips sunglasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for clearCache method
# Search for test cases using clearCache
rg -l "clearCache.*CorporateCreditCardExpenseService" --type ts
# Look for actual method implementation
ast-grep --pattern 'clearCache() {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Let's search with broader patterns to understand the context better, style!
# Find all occurrences of clearCache in spec files
rg "clearCache" --type ts -A 3 -B 3 "spec.ts$"
# Find the service implementation
rg "CorporateCreditCardExpenseService" --type ts -A 5 -B 5
# Find all test files related to this component
fd "tasks.component.*spec.ts$"
Length of output: 75945
Script:
#!/bin/bash
# Let's find those test cases with style, thalaiva!
# Search for test cases using clearCache in tasks component specs
rg "clearCache" "src/app/fyle/dashboard/tasks/tasks.*spec.ts" -B 3 -A 3
# Look for the actual implementation of clearCache in the service
rg "clearCache" "src/app/core/services/corporate-credit-card-expense.service.ts" -B 3 -A 3
# Find test cases that might be using this service method
rg "corporateCreditCardExpenseService.*clearCache" --type ts -A 3 -B 3
Length of output: 9383
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (2)
57-59: Mind it! These imports are perfectly placed!
The new imports are essential for testing the corporate card functionality. They bring in the required components and mock data.
Line range hint 208-227: Kabali style test case, I tell you!
The test case is solid, covering:
- Service mocking for org settings
- Popover creation verification
- Component props validation
| @@ -7,6 +7,7 @@ | |||
| height="auto" | |||
| onloadedmetadata="this.muted = true" | |||
| oncanplay="this.play()" | |||
| poster="../../../assets/images/video-default-background.png" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen carefully! The path needs more power, like my punch!
The relative path ../../../assets/images/video-default-background.png is like a weak villain. Let's make it stronger!
Make it absolute for better reliability:
-poster="../../../assets/images/video-default-background.png"
+poster="/assets/images/video-default-background.png"📝 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.
| poster="../../../assets/images/video-default-background.png" | |
| poster="/assets/images/video-default-background.png" |
| it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.clearCache.and.returnValue(of(null)); | ||
|
|
||
| const mockPopover = { | ||
| present: jasmine.createSpy('present').and.resolveTo(), | ||
| onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo(), | ||
| }; | ||
| popoverController.create.and.resolveTo(mockPopover as any); | ||
|
|
||
| component.handleEnrollmentSuccess(); | ||
| tick(); | ||
|
|
||
| expect(corporateCreditCardExpenseService.clearCache).toHaveBeenCalled(); | ||
| expect(popoverController.create).toHaveBeenCalledWith({ | ||
| component: CardAddedComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
| expect(mockPopover.present).toHaveBeenCalled(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style-u! Let's make this test even more powerful!
While the test covers the basic flow, consider adding these assertions:
- Verify the popover dismissal handling
- Add error case scenarios
it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => {
corporateCreditCardExpenseService.clearCache.and.returnValue(of(null));
const mockPopover = {
present: jasmine.createSpy('present').and.resolveTo(),
- onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo(),
+ onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo({ data: { success: true } }),
};
popoverController.create.and.resolveTo(mockPopover as any);
component.handleEnrollmentSuccess();
tick();
expect(corporateCreditCardExpenseService.clearCache).toHaveBeenCalled();
expect(popoverController.create).toHaveBeenCalledWith({
component: CardAddedComponent,
cssClass: 'pop-up-in-center',
});
expect(mockPopover.present).toHaveBeenCalled();
+ expect(mockPopover.onDidDismiss).toHaveBeenCalled();
+
+ // Error case
+ corporateCreditCardExpenseService.clearCache.and.returnValue(throwError(() => new Error('Cache clear failed')));
+ component.handleEnrollmentSuccess();
+ tick();
+ expect(mockPopover.present).not.toHaveBeenCalled();
}));Committable suggestion skipped: line range outside the PR's diff.
| const orgSettings$ = this.orgSettingsService.get(); | ||
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | ||
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | ||
| ); | ||
|
|
||
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | ||
| map( | ||
| (orgSettings) => | ||
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | ||
| ) | ||
| ); | ||
|
|
||
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | ||
| map( | ||
| ([orgSettings, orgUserSettings]) => | ||
| orgSettings.bank_data_aggregation_settings.allowed && | ||
| orgSettings.bank_data_aggregation_settings.enabled && | ||
| orgUserSettings.bank_data_aggregation_settings.enabled | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Mind it! Let's make this code as powerful as Thalaiva!
While the implementation is good, we can enhance it with these improvements:
- Add error handling for the observables
- Optimize the stream by sharing the orgSettings$ observable
onAddCorporateCardClick(): void {
- const orgSettings$ = this.orgSettingsService.get();
+ const orgSettings$ = this.orgSettingsService.get().pipe(
+ shareReplay(1),
+ catchError(err => {
+ console.error('Failed to fetch org settings:', err);
+ return of(null);
+ })
+ );
this.isVisaRTFEnabled$ = orgSettings$.pipe(
+ filter(settings => !!settings),
map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled)
);
this.isMastercardRTFEnabled$ = orgSettings$.pipe(
+ filter(settings => !!settings),
map(
(orgSettings) =>
orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled
)
);
this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe(
+ catchError(err => {
+ console.error('Failed to fetch settings:', err);
+ return of([null, null]);
+ }),
+ filter(([settings, userSettings]) => !!settings && !!userSettings),
map(
([orgSettings, orgUserSettings]) =>
orgSettings.bank_data_aggregation_settings.allowed &&
orgSettings.bank_data_aggregation_settings.enabled &&
orgUserSettings.bank_data_aggregation_settings.enabled
)
);Why these changes, I tell you:
shareReplay(1)prevents multiple HTTP requests when subscribing to the same observable- Error handling with
catchErrorprevents the app from crashing filteroperator ensures we don't process null settings
Consider implementing a retry mechanism for failed API calls using retryWhen operator.
📝 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.
| const orgSettings$ = this.orgSettingsService.get(); | |
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | |
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | |
| ); | |
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | |
| map( | |
| (orgSettings) => | |
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | |
| ) | |
| ); | |
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | |
| map( | |
| ([orgSettings, orgUserSettings]) => | |
| orgSettings.bank_data_aggregation_settings.allowed && | |
| orgSettings.bank_data_aggregation_settings.enabled && | |
| orgUserSettings.bank_data_aggregation_settings.enabled | |
| ) | |
| ); | |
| const orgSettings$ = this.orgSettingsService.get().pipe( | |
| shareReplay(1), | |
| catchError(err => { | |
| console.error('Failed to fetch org settings:', err); | |
| return of(null); | |
| }) | |
| ); | |
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | |
| filter(settings => !!settings), | |
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | |
| ); | |
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | |
| filter(settings => !!settings), | |
| map( | |
| (orgSettings) => | |
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | |
| ) | |
| ); | |
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | |
| catchError(err => { | |
| console.error('Failed to fetch settings:', err); | |
| return of([null, null]); | |
| }), | |
| filter(([settings, userSettings]) => !!settings && !!userSettings), | |
| map( | |
| ([orgSettings, orgUserSettings]) => | |
| orgSettings.bank_data_aggregation_settings.allowed && | |
| orgSettings.bank_data_aggregation_settings.enabled && | |
| orgUserSettings.bank_data_aggregation_settings.enabled | |
| ) | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Line range hint
208-227: Magizhchi! The test is good, but let's make it legendary!The test covers the happy path like a well-choreographed action sequence, but we need to handle the villain scenarios too!
Add these test cases to make it blockbuster:
- Error handling when service calls fail
- Edge case when card enrollment is disabled
- Cancellation scenario when user dismisses the popover
it('should handle service error gracefully', () => { orgSettingsService.get.and.returnValue(throwError(() => new Error())); component.onAddCorporateCardClick(); expect(popoverController.create).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
android/.idea/runConfigurations.xml(1 hunks)src/app/auth/sign-in/sign-in.page.html(1 hunks)src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts(4 hunks)src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts(1 hunks)src/app/fyle/dashboard/tasks/tasks.component.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
🔇 Additional comments (3)
android/.idea/runConfigurations.xml (1)
6-13: Mind it! These configuration changes are perfectly balanced, like my signature sunglasses!
The addition of JUnit configuration producers to the ignored list is a strategic move to prevent test configuration conflicts. But remember what I always say - "When something comes in your way, don't ask it to move. Ask yourself why you can't move it!"
Let me verify if these changes affect any existing test configurations:
✅ Verification successful
I need to see the complete configuration file and related test configurations to make a solid verification, like checking all angles before performing my signature move! 🕶️
Mind it! Before making my final verdict, I need to see if these configuration changes are in harmony with the test setup, like checking all the pieces before the grand finale! 🕶️
Mind it! These JUnit configuration changes are perfectly safe, like my signature stunts!
The changes in runConfigurations.xml are simply telling Android Studio which test configuration producers to ignore, while our build.gradle shows proper JUnit setup with testImplementation "junit:junit:$junitVersion". The example test files are present and untouched. When you want to make a change, make it with style! 🕶️
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing JUnit configurations that might be affected
find . -type f -name "*.xml" -exec grep -l "junit" {} \;
Length of output: 97
Script:
#!/bin/bash
# Let's check the content of runConfigurations.xml to understand the full context
cat ./android/.idea/runConfigurations.xml
Length of output: 1008
Script:
#!/bin/bash
# Let's also check if there are any other configuration files that might be impacted
fd -e xml -x grep -l "configuration" {} \;
Length of output: 45
Script:
#!/bin/bash
# Check for any test-related files that might be affected
fd -e gradle -e xml -x grep -l "test" {} \;
Length of output: 73
Script:
#!/bin/bash
# Let's check the test configurations in build.gradle
cat ./android/app/build.gradle
Length of output: 1692
Script:
#!/bin/bash
# Let's see if there are any existing JUnit test files
fd -e java -e kt -x grep -l "@Test" {} \;
Length of output: 203
src/app/fyle/dashboard/tasks/tasks.component.setup.spec.ts (1)
73-73: When I do something, I do it in style! This test spy update is perfect!
Adding clearCache to the spy object shows you understand the importance of thorough test coverage. Like I always say, "Testing is not just a process, it's an art!"
Let me check if this method is properly tested:
✅ Verification successful
Let me style-check this method's implementation and test coverage, thalaiva! 🕶️
Mind it! This spy addition is perfectly synchronized with the codebase, like a well-choreographed action sequence!
The clearCache method in the service returns an Observable of void, and the tasks component is using it stylishly before showing the card-added modal. Adding this method to the spy object is absolutely necessary for the test coverage - when I do testing, I do complete testing! 🕶️
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of clearCache method
rg -l "clearCache.*\(" --type ts
Length of output: 1175
Script:
#!/bin/bash
# Let's check the implementation of clearCache in corporate-credit-card-expense.service.ts
ast-grep --pattern 'clearCache() {
$$$
}'
# And check if it's tested in other spec files
rg "clearCache" "src/app/core/services/corporate-credit-card-expense.service.spec.ts"
Length of output: 129
Script:
#!/bin/bash
# Mind it! Let's check the service file with more style
rg -A 5 "clearCache" "src/app/core/services/corporate-credit-card-expense.service.ts"
# And let's see how this method is being used in tasks component
rg -A 5 "clearCache" "src/app/fyle/dashboard/tasks/tasks.component.ts"
Length of output: 765
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
118-119: Mind it! Network setup looks perfect!
The network connectivity mocks are set up with style, just like how I handle my stunts - with precision and grace.
| @@ -7,6 +7,7 @@ | |||
| height="auto" | |||
| onloadedmetadata="this.muted = true" | |||
| oncanplay="this.play()" | |||
| poster="../../../assets/images/video-default-background.png" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Listen carefully! The path to success should be absolute, just like my path to stardom!
The poster attribute is a stylish addition, but the relative path ../../../assets/images/video-default-background.png could break if the component structure changes.
Make it robust like my punch dialogues with this change:
- poster="../../../assets/images/video-default-background.png"
+ poster="/assets/images/video-default-background.png"📝 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.
| poster="../../../assets/images/video-default-background.png" | |
| poster="/assets/images/video-default-background.png" |
| it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.clearCache.and.returnValue(of(null)); | ||
|
|
||
| const mockPopover = { | ||
| present: jasmine.createSpy('present').and.resolveTo(), | ||
| onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo(), | ||
| }; | ||
| popoverController.create.and.resolveTo(mockPopover as any); | ||
|
|
||
| component.handleEnrollmentSuccess(); | ||
| tick(); | ||
|
|
||
| expect(corporateCreditCardExpenseService.clearCache).toHaveBeenCalled(); | ||
| expect(popoverController.create).toHaveBeenCalledWith({ | ||
| component: CardAddedComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
| expect(mockPopover.present).toHaveBeenCalled(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Style-u! The test is powerful, but needs more punch!
Like how I track my enemies in movies, we should track our user actions in code!
Add tracking verification to make this test complete:
it('handleEnrollmentSuccess(): should track successful enrollment', fakeAsync(() => {
spyOn(trackingService, 'cardEnrollmentSuccess');
component.handleEnrollmentSuccess();
tick();
expect(trackingService.cardEnrollmentSuccess).toHaveBeenCalled();
}));| const orgSettings$ = this.orgSettingsService.get(); | ||
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | ||
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | ||
| ); | ||
|
|
||
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | ||
| map( | ||
| (orgSettings) => | ||
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | ||
| ) | ||
| ); | ||
|
|
||
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | ||
| map( | ||
| ([orgSettings, orgUserSettings]) => | ||
| orgSettings.bank_data_aggregation_settings.allowed && | ||
| orgSettings.bank_data_aggregation_settings.enabled && | ||
| orgUserSettings.bank_data_aggregation_settings.enabled | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Kabali da! The code needs proper cleanup!
Like how I clean up the streets in my movies, we need to clean up our subscriptions!
Add proper error handling and subscription management:
- forkJoin([this.isVisaRTFEnabled$, this.isMastercardRTFEnabled$, this.isYodleeEnabled$]).subscribe(
+ forkJoin([this.isVisaRTFEnabled$, this.isMastercardRTFEnabled$, this.isYodleeEnabled$])
+ .pipe(
+ catchError((error) => {
+ this.showToastMessage('Failed to load card settings', 'failure');
+ return EMPTY;
+ }),
+ takeUntil(this.destroy$)
+ )
+ .subscribe(Don't forget to:
- Add
private destroy$ = new Subject<void>();as class property - Implement
ngOnDestroyto callthis.destroy$.next(); this.destroy$.complete();
Committable suggestion skipped: line range outside the PR's diff.
| <option value="com.intellij.execution.junit.UniqueIdConfigurationProducer" /> | ||
| <option value="com.intellij.execution.junit.testDiscovery.JUnitTestDiscoveryConfigurationProducer" /> | ||
| <option value="org.jetbrains.kotlin.idea.junit.KotlinJUnitRunConfigurationProducer" /> | ||
| <option value="org.jetbrains.kotlin.idea.junit.KotlinPatternConfigurationProducer" /> |
There was a problem hiding this comment.
lets remove these lines if you are not making any native changes
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/core/services/tasks.service.spec.ts(3 hunks)src/app/core/services/tasks.service.ts(6 hunks)src/app/fyle/dashboard/tasks/tasks-1.component.spec.ts(6 hunks)src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts(5 hunks)src/app/fyle/dashboard/tasks/tasks.component.ts(7 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (1)
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-11-12T04:53:12.395Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
🔇 Additional comments (21)
src/app/fyle/dashboard/tasks/tasks-2.component.spec.ts (4)
2-2: Magizhchi! The import looks correct.
All good here, Thala! The use of PopoverController from '@ionic/angular' is well-suited for testing popovers.
51-59: All set with these imports.
Great additions, my friend. These new references (e.g., AddCorporateCardComponent, CardAddedComponent) align well with the updated tests.
83-86: No problem with the popoverController spy.
This spy object initialization is neat and helps ensure we’re testing popover interactions effectively.
110-119: Solid injection.
Injecting PopoverController for the test suite is cleanly done, and integration with the test bed is straightforward.
src/app/core/services/tasks.service.ts (4)
323-323: New addCorporateCard addition is neat.
Incorporating getAddCorporateCardTask into forkJoin is a solid approach to unify tasks.
336-336: Properly mapped addCorporateCard.
No issues spotted here, Thala. The new addition merges seamlessly with existing tasks.
347-348: Comprehensive totalTaskCount.
Adding setCommuteDetails and addCorporateCard to the count ensures an accurate tally of tasks.
372-373: The chain of tasks is unstoppable!
Chaining addCorporateCard with other tasks is well-placed. No concerns.
src/app/core/services/tasks.service.spec.ts (3)
44-44: New corporate card mocks imported.
No friction here. Mocks look ready for their cameo in the test.
432-444: Test coverage for getAddCorporateCardTask with no enrolled cards.
Perfect scenario test, my friend. Ensures the function returns a valid task.
446-453: Test coverage for getAddCorporateCardTask with enrolled cards.
This is a neat negative scenario ensuring an empty array is returned. Paired well with the previous test.
src/app/fyle/dashboard/tasks/tasks-1.component.spec.ts (5)
35-35: taskCtaData11 introduction is smooth.
This new CTA helps test addCorporateCard tasks. Nicely done.
45-46: Adding OrgSettingsService references looks correct.
No issues: the new imports mesh well with the rest of the test suite.
66-66: Introducing the orgSettingsService spy.
All good. This spy will help in verifying any org settings logic.
341-341: Spying on onAddCorporateCardClick.
One step closer to verifying card popover interactions.
483-499: Rockstar test for addCorporateCard CTA.
Ensures onAddCorporateCardClick is called. That’s how you verify real user flows, my friend.
src/app/fyle/dashboard/tasks/tasks.component.ts (5)
5-5: Importing RefresherEventDetail is good.
This improves clarity on the typed event structure. Well done.
75-80: Visually see your new Observables.
You’re introducing isVisaRTFEnabled$, isMastercardRTFEnabled$, and isYodleeEnabled$. This separation is clean.
99-103: Constructor injection spree!
Adding orgUserSettingsService, orgService, popoverController, and creditCardExpenseService is fully cohesive.
396-410: handleEventsWithoutTaskConfig
Such a function decouples the event logic gracefully. Good layering.
421-428: Mighty calls to handle both config and non-config events.
This pattern is flexible and maintainable. Good to see them in action.
| it('onAddCorporateCardClick(): should open card popover', () => { | ||
| orgSettingsService.get.and.returnValue(of(orgSettingsPendingRestrictions)); | ||
| orgUserSettingsService.get.and.returnValue(of(orgUserSettingsData)); | ||
| const addCardPopoverSpy = jasmine.createSpyObj('HTMLIonPopoverElement', ['present', 'onDidDismiss']); | ||
| addCardPopoverSpy.present.and.resolveTo(); | ||
| addCardPopoverSpy.onDidDismiss.and.resolveTo({ success: true }); | ||
| popoverController.create.and.resolveTo(addCardPopoverSpy); | ||
| spyOn(component, 'handleEnrollmentSuccess'); | ||
|
|
||
| fixture.detectChanges(); | ||
| component.onAddCorporateCardClick(); | ||
| expect(popoverController.create).toHaveBeenCalledOnceWith({ | ||
| component: AddCorporateCardComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| componentProps: { | ||
| isVisaRTFEnabled: true, | ||
| isMastercardRTFEnabled: true, | ||
| isYodleeEnabled: true, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Aiyoo! Consider adding negative path coverage.
The test ensures the popover is presented on success. Testing an error scenario or a dismissed popover would strengthen the suite further.
| it('handleEnrollmentSuccess(): should handle enrollment success and trigger subsequent actions', fakeAsync(() => { | ||
| corporateCreditCardExpenseService.clearCache.and.returnValue(of(null)); | ||
|
|
||
| const mockPopover = { | ||
| present: jasmine.createSpy('present').and.resolveTo(), | ||
| onDidDismiss: jasmine.createSpy('onDidDismiss').and.resolveTo(), | ||
| }; | ||
| popoverController.create.and.resolveTo(mockPopover as any); | ||
|
|
||
| component.handleEnrollmentSuccess(); | ||
| tick(); | ||
|
|
||
| expect(corporateCreditCardExpenseService.clearCache).toHaveBeenCalled(); | ||
| expect(popoverController.create).toHaveBeenCalledWith({ | ||
| component: CardAddedComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
| expect(mockPopover.present).toHaveBeenCalled(); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Looks stylish!
Your test for handleEnrollmentSuccess covers the happy path well. However, consider verifying what happens if clearCache or popover creation fails, to cover edge cases.
| getAddCorporateCardTask(): Observable<DashboardTask[]> { | ||
| return forkJoin([this.orgSettingsService.get(), this.corporateCreditCardExpenseService.getCorporateCards()]).pipe( | ||
| map(([orgSettings, cards]) => { | ||
| 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[]; | ||
| } | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Superstar feature: getAddCorporateCardTask.
• Nicely checks orgSettings and card statuses.
• Potential improvement: handle errors from orgSettings or the corporate card call to avoid silent failures.
| mapAddCorporateCardTask(): DashboardTask[] { | ||
| const task = [ | ||
| { | ||
| hideAmount: true, | ||
| header: 'Add Corporate Card', | ||
| subheader: 'Add your corporate card to track expenses.', | ||
| icon: TaskIcon.CARD, | ||
| ctas: [ | ||
| { | ||
| content: 'Add Card', | ||
| event: TASKEVENT.addCorporateCard, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
| return task; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
mapAddCorporateCardTask is a star cameo.
The structure is consistent with other tasks. Perhaps add a quick test to confirm each CTA property is correct.
| orgSettingsService.get.and.returnValue(of(orgSettingsPendingRestrictions)); | ||
| component.isVisaRTFEnabled$ = of(true); | ||
| component.isMastercardRTFEnabled$ = of(true); | ||
| component.isYodleeEnabled$ = of(true); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simulating RTF and Yodlee flags.
Ensuring the test environment sets these flags to true is straightforward. Consider adding an additional test for false flags to be thorough.
| import { CorporateCreditCardExpenseService } from 'src/app/core/services/corporate-credit-card-expense.service'; | ||
| import { OrgUserSettingsService } from 'src/app/core/services/org-user-settings.service'; | ||
| import { AddCorporateCardComponent } from '../../manage-corporate-cards/add-corporate-card/add-corporate-card.component'; | ||
| import { CardAddedComponent } from '../../manage-corporate-cards/card-added/card-added.component'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
New imports for corporate card flow.
The additions align with your popover approach. Keep an eye on error handling if popover fails to load.
| handleEventsWithTaskConfig(taskCta: TaskCta, task: DashboardTask): void { | ||
| switch (taskCta.event) { | ||
| case TASKEVENT.expensesAddToReport: | ||
| this.onExpensesToReportTaskClick(); | ||
| break; | ||
| case TASKEVENT.openDraftReports: | ||
| this.onOpenDraftReportsTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.openSentBackReport: | ||
| this.onSentBackReportTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.reviewExpenses: | ||
| this.onReviewExpensesTaskClick(); | ||
| break; | ||
| case TASKEVENT.openTeamReport: | ||
| this.onTeamReportsTaskClick(taskCta, task); | ||
| break; | ||
| case TASKEVENT.openPotentialDuplicates: | ||
| this.onPotentialDuplicatesTaskClick(); | ||
| break; | ||
| case TASKEVENT.openSentBackAdvance: | ||
| this.onSentBackAdvanceTaskClick(taskCta, task); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
handleEventsWithTaskConfig
A separate switch handling distinct events is well-organized. Consider including fallback logging if an event is not recognized.
| handleEnrollmentSuccess(): void { | ||
| this.corporateCreditCardExpenseService.clearCache().subscribe(async () => { | ||
| const cardAddedModal = await this.popoverController.create({ | ||
| component: CardAddedComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
|
|
||
| await cardAddedModal.present(); | ||
| await cardAddedModal.onDidDismiss(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Kabali da! handleEnrollmentSuccess is unstoppable.
• Properly calls clearCache.
• Popover flow is nice.
• For a robust approach, handle error callbacks from clearCache.
| onAddCorporateCardClick(): void { | ||
| const orgSettings$ = this.orgSettingsService.get(); | ||
| this.isVisaRTFEnabled$ = orgSettings$.pipe( | ||
| map((orgSettings) => orgSettings.visa_enrollment_settings.allowed && orgSettings.visa_enrollment_settings.enabled) | ||
| ); | ||
|
|
||
| this.isMastercardRTFEnabled$ = orgSettings$.pipe( | ||
| map( | ||
| (orgSettings) => | ||
| orgSettings.mastercard_enrollment_settings.allowed && orgSettings.mastercard_enrollment_settings.enabled | ||
| ) | ||
| ); | ||
|
|
||
| this.isYodleeEnabled$ = forkJoin([orgSettings$, this.orgUserSettingsService.get()]).pipe( | ||
| map( | ||
| ([orgSettings, orgUserSettings]) => | ||
| orgSettings.bank_data_aggregation_settings.allowed && | ||
| orgSettings.bank_data_aggregation_settings.enabled && | ||
| orgUserSettings.bank_data_aggregation_settings.enabled | ||
| ) | ||
| ); | ||
| forkJoin([this.isVisaRTFEnabled$, this.isMastercardRTFEnabled$, this.isYodleeEnabled$]).subscribe( | ||
| async ([isVisaRTFEnabled, isMastercardRTFEnabled, isYodleeEnabled]) => { | ||
| const addCorporateCardPopover = await this.popoverController.create({ | ||
| component: AddCorporateCardComponent, | ||
| cssClass: 'fy-dialog-popover', | ||
| componentProps: { | ||
| isVisaRTFEnabled, | ||
| isMastercardRTFEnabled, | ||
| isYodleeEnabled, | ||
| }, | ||
| }); | ||
|
|
||
| await addCorporateCardPopover.present(); | ||
| const popoverResponse = (await addCorporateCardPopover.onDidDismiss()) as { success: boolean }; | ||
|
|
||
| if (popoverResponse.success) { | ||
| this.handleEnrollmentSuccess(); | ||
| } | ||
| } | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
A full-blown function for onAddCorporateCardClick.
• Thorough approach, reading org settings, user settings, and then presenting the popover.
• Consider adding error handling for each subscription to gracefully handle failures.
|
Fixes for test coverage.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests