test: Add more coverage for connect card validators #3411
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request enhances the Changes
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
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: 4
🔭 Outside diff range comments (1)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
Line range hint
254-262: Let's add some punch dialogues to our error messages!The error handling is good, but we could make the error messages more specific based on different failure scenarios.
Consider categorizing errors and providing more specific messages:
private handleEnrollmentFailures(error: Error, cardId?: string): void { - const enrollmentFailureMessage = error.message || 'Something went wrong. Please try after some time.'; + const enrollmentFailureMessage = error.message || this.getSpecificErrorMessage(error); if (this.enrollableCards.length > 0) { this.fg.controls[`card_number_${cardId}`].setErrors({ enrollmentError: true }); this.cardValuesMap[cardId].enrollment_error = enrollmentFailureMessage; } else { this.fg.controls.card_number.setErrors({ enrollmentError: true }); this.singularEnrollmentFailure = enrollmentFailureMessage; } } + + private getSpecificErrorMessage(error: Error): string { + // Add specific error messages based on error types + if (error instanceof HttpErrorResponse) { + switch (error.status) { + case 400: return 'Invalid card details provided'; + case 409: return 'This card is already enrolled'; + default: return 'Something went wrong. Please try after some time'; + } + } + return 'Something went wrong. Please try after some time'; + }
📜 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.spec.ts(6 hunks)src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.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.
🪛 Biome (1.9.4)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 14-14: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🪛 eslint
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 14-14: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[failure] 14-14:
'error' is defined but never used
🪛 GitHub Actions: Lint
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts
[error] 3-3: 'ChangeDetectorRef' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (2)
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.ts (1)
Line range hint
154-181: Superstar code right here!The async/await implementation is clean and handles the popover display elegantly. Mind-blowing error handling, macha!
src/app/fyle/spender-onboarding/spender-onboarding-connect-card-step/spender-onboarding-connect-card-step.component.spec.ts (1)
177-215: Kabali style testing, macha!The test coverage for generateMessage is solid! All scenarios are covered with proper assertions.
| @@ -11,7 +11,8 @@ | |||
| } from '@angular/core'; | |||
| import { AbstractControl, FormBuilder, FormControl, FormGroup, ValidationErrors, Validators } from '@angular/forms'; | |||
| import { PopoverController } from '@ionic/angular'; | |||
| import { error } from 'console'; | |||
There was a problem hiding this comment.
Mind it! Remove unused import, macha!
The import 'error' from 'console' is not being used anywhere in the code. Let's keep our imports clean and tidy!
-import { error } from 'console';🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🪛 eslint
[error] 14-14: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Run linters
[failure] 14-14:
'error' is defined but never used
Comment on lines
+143
to
+150
| } else if (this.cardsList.failedCards.length > 1) { | ||
| return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | ||
| .slice(0, this.cardsList.failedCards.length - 1) | ||
| .join(', ')} and ${this.cardsList.failedCards.slice(-1)}. | ||
| You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| .join(', ')} and ${this.cardsList.failedCards.slice( | ||
| -1 | ||
| )}. You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| } else { | ||
| return ` | ||
| We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. | ||
| You can cancel and retry connecting the failed card or proceed to the next step.`; | ||
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah style! Let's make this code more stylish!
The string concatenation can be simplified using template literals for better readability, macha!
} else if (this.cardsList.failedCards.length > 1) {
- return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards
- .slice(0, this.cardsList.failedCards.length - 1)
- .join(', ')} and ${this.cardsList.failedCards.slice(
- -1
- )}. You can cancel and retry connecting the failed card or proceed to the next step.`;
+ const allButLast = this.cardsList.failedCards.slice(0, -1).join(', ');
+ const lastCard = this.cardsList.failedCards.slice(-1);
+ return `We ran into an issue while processing your request for the cards ${allButLast} and ${lastCard}. You can cancel and retry connecting the failed card or proceed to the next step.`;📝 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
| } else if (this.cardsList.failedCards.length > 1) { | |
| return `We ran into an issue while processing your request for the cards ${this.cardsList.failedCards | |
| .slice(0, this.cardsList.failedCards.length - 1) | |
| .join(', ')} and ${this.cardsList.failedCards.slice(-1)}. | |
| You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| .join(', ')} and ${this.cardsList.failedCards.slice( | |
| -1 | |
| )}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } else { | |
| return ` | |
| We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. | |
| You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } else if (this.cardsList.failedCards.length > 1) { | |
| const allButLast = this.cardsList.failedCards.slice(0, -1).join(', '); | |
| const lastCard = this.cardsList.failedCards.slice(-1); | |
| return `We ran into an issue while processing your request for the cards ${allButLast} and ${lastCard}. You can cancel and retry connecting the failed card or proceed to the next step.`; | |
| } else { | |
| return `We ran into an issue while processing your request for the card ${this.cardsList.failedCards[0]}. You can cancel and retry connecting the failed card or proceed to the next step.`; |
Comment on lines
+296
to
+329
| it('showErrorPopover(): should display a popover and handle its actions', () => { | ||
| const popoverSpy = jasmine.createSpyObj('popover', ['present', 'onWillDismiss']); | ||
| spyOn(component, 'generateMessage').and.returnValue('Error message'); | ||
| popoverSpy.onWillDismiss.and.resolveTo({ | ||
| data: { | ||
| action: 'close', | ||
| }, | ||
| }); | ||
| popoverController.create.and.resolveTo(popoverSpy); | ||
| component.cardsList = { | ||
| successfulCards: [], | ||
| failedCards: ['**** 1111'], | ||
| }; | ||
| fixture.detectChanges(); | ||
| component.showErrorPopover(); | ||
|
|
||
| expect(popoverController.create).toHaveBeenCalledOnceWith({ | ||
| componentProps: { | ||
| title: 'Failed connecting', | ||
| message: 'Error message', | ||
| primaryCta: { | ||
| text: 'Proceed anyway', | ||
| action: 'close', | ||
| }, | ||
| secondaryCta: { | ||
| text: 'Cancel', | ||
| action: 'cancel', | ||
| }, | ||
| cardsList: {}, | ||
| }, | ||
| component: PopupAlertComponent, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Time to level up our testing game!
The test verifies popover creation, but let's add a test for the dismiss scenario too!
Add this test case:
it('should handle popover dismiss with cancel action', async () => {
const popoverSpy = jasmine.createSpyObj('popover', ['present', 'onWillDismiss']);
spyOn(component, 'generateMessage').and.returnValue('Error message');
popoverSpy.onWillDismiss.and.resolveTo({
data: {
action: 'cancel'
}
});
popoverController.create.and.resolveTo(popoverSpy);
const stepCompleteSpy = spyOn(component.isStepComplete, 'emit');
await component.showErrorPopover();
expect(stepCompleteSpy).not.toHaveBeenCalled();
});
Comment on lines
+342
to
+343
| it('should update card_type for the given card or singleEnrollableCardDetails', () => { | ||
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate test description spotted, thalaiva!
Both test cases have the same description. Let's make them more specific!
- it('should update card_type for the given card or singleEnrollableCardDetails', () => {
+ it('should update card_type in cardValuesMap for multiple enrollable cards', () => {📝 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
| it('should update card_type for the given card or singleEnrollableCardDetails', () => { | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); | |
| it('should update card_type in cardValuesMap for multiple enrollable cards', () => { | |
| realTimeFeedService.getCardTypeFromNumber.and.returnValue(CardNetworkType.VISA); |
arjunaj5
approved these changes
Jan 9, 2025
Chethan-Fyle
approved these changes
Jan 9, 2025
bistaastha
merged commit 24fb103
into
FYLE-86cx2t82k-tests-for-connect-card-2
5 of 6 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Coverage:
Summary by CodeRabbit
Tests
Bug Fixes
New Features
Refactor
showErrorPopovermethod to asynchronous functionhandleEnrollmentFailuresmethod more flexible with optional parameters