fix: migrate delete personal card api to platform #3266
Conversation
WalkthroughThe changes introduce a new constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
| @@ -1,5 +1,5 @@ | |||
| export interface PlatformApiResponse<T> { | |||
| count: number; | |||
| offset: number; | |||
| count?: number; | |||
There was a problem hiding this comment.
in platform API response count & offset will always be there right ?
There was a problem hiding this comment.
This is only when the response is an array. Here in delete personal card api, I am receiving only the data property in the response.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
src/app/core/mock-data/personal-cards.data.ts(1 hunks)src/app/core/models/platform/platform-api-response.model.ts(1 hunks)src/app/core/services/personal-cards.service.spec.ts(3 hunks)src/app/core/services/personal-cards.service.ts(1 hunks)src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts(7 hunks)src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.ts(5 hunks)
🧰 Additional context used
🪛 Biome
src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts
[error] 17-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (14)
src/app/core/models/platform/platform-api-response.model.ts (1)
2-3: Mind blowing changes, but handle the undefined cases like a boss!
Hey machcha! The changes to make count and offset optional are stylish, but remember to handle undefined cases wherever these properties are used. Just like how I handle villains with style, you need to handle these optional properties with proper null checks!
Let me help you verify the usage with this powerful script:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the complete usage pattern of this interface in the codebase, style!
Mind your style, these optional properties are handled like a boss!
After scanning the entire codebase with my signature style, I can see that all services are handling the optional properties properly by:
-
Using optional chaining (
?.) when accessing count/offset:map((response) => response.count)map((policyStates) => (policyStates.count > 0 ? policyStates.data[0] : []))
-
Safely destructuring with defaults:
- All services map the response and handle undefined cases
- Data property is required, so no undefined issues there
Just like how I handle every situation with perfect timing, the codebase is handling these optional properties with perfect defensive programming!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of count and offset properties from PlatformApiResponse
# to ensure proper undefined handling
# Search for direct property access
rg -A 2 "platformApiResponse\.(count|offset)"
# Search for destructuring
rg -A 2 "\{ .*?(count|offset).*? \}: PlatformApiResponse"
Length of output: 107
Script:
#!/bin/bash
# Let's search more broadly for any usage of PlatformApiResponse
rg -A 3 "PlatformApiResponse"
# Also search for any file importing this model
rg -l "platform-api-response\.model"
# Let's also check the interface definition
fd "platform-api-response.model.ts" --exec cat {}
Length of output: 60243
src/app/core/mock-data/personal-cards.data.ts (1)
75-94: Magizhchi! Let's verify the completeness of our platform response, shall we?
Like a well-choreographed fight sequence, we need all our moves (fields) to be perfect!
✅ Verification successful
Superstar verdict: All fields are perfectly in sync, like my signature style!
I've compared the interface definition with our mock implementation, and every single field from PersonalCardPlatform interface is present in the mock data with the correct types. Even the optional yodlee_fastlink_params is handled properly with null. The mock data is as perfect as my punch dialogues!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required fields for PersonalCardPlatform are present in the mock
# Search for PersonalCardPlatform interface definition
ast-grep --pattern 'interface PersonalCardPlatform {
$$$
}'
# Compare with the mock implementation
echo "=== Comparing with mock implementation ==="
rg -A 20 'deletePersonalCardPlatformRes.*PlatformApiResponse<PersonalCardPlatform>'
Length of output: 3369
src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts (3)
16-16: Mind-it! LaunchDarklyService integration looks solid, macha!
The service integration is perfectly orchestrated like a well-choreographed fight scene! The spy setup and provider configuration are on point.
Also applies to: 27-27, 36-36, 65-68, 78-78
104-104: Event handling upgrade! Style ah style!
The change from Event to PointerEvent is like upgrading from a regular punch to a super punch! More precise and powerful!
Also applies to: 108-108
113-113: 🧹 Nitpick (assertive)
Delete account test is now feature-flag ready! What a performance!
The test now handles the feature flag check with style! But wait a minute... let me verify if we're covering all scenarios, like a true superstar would!
Consider adding another test case where getVariation returns true to cover both platform and non-platform API scenarios. Like this:
it('deleteAccount(): should delete account using platform API when flag is enabled', fakeAsync(() => {
launchDarklyService.getVariation.and.returnValue(of(true));
// ... rest of the test setup
expect(personalCardsService.deleteAccount).toHaveBeenCalledOnceWith(component.accountDetails.id, true);
}));Also applies to: 126-127, 134-134
✅ Verification successful
These commands will help us verify the test coverage with the swagger of a superstar! Let's see what we find in the codebase, partner!
Mind it! The test coverage is already a blockbuster, partner!
Listen here, my friend! The search results show that we're already testing both scenarios like a true superstar:
- In
personal-cards.service.spec.ts, we have:- Test with
usePlatformApi = false- testing the classic way - Test with
usePlatformApi = true- testing the platform API way
- Test with
The feature flag implementation is already covered in style across the entire codebase! The suggestion to add another test case was unnecessary because we're already rocking both scenarios in the service layer tests. That's how we do it in the big leagues!
flips sunglasses
Mind it! When you have tests at the service layer, you don't need to duplicate the same scenarios in the component tests. That's not style, that's just repetition!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if we have test coverage for both feature flag scenarios
# Search for other test cases that might need the feature flag
rg -l "deleteAccount.*getVariation"
# Look for platform API related tests
ast-grep --pattern 'it($_, $_) {
$$$
usePlatformApi
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's search with more style and precision!
# First, let's find all spec files that mention deleteAccount
rg -l "deleteAccount" --type ts --glob "*spec.ts"
# Then, let's look for the specific test file content
rg -A 10 "deleteAccount.*getVariation" --type ts
# And finally, check for any platform API related tests with context
rg -A 5 "usePlatformApi" --type ts
Length of output: 12388
src/app/core/services/personal-cards.service.spec.ts (2)
10-10: Mind-blowing addition, thalaiva! The import looks perfect!
The new import for deletePersonalCardPlatformRes aligns perfectly with the existing mock data imports.
37-37: Superstar style spy setup, boss!
The SpenderPlatformV1ApiService spy is properly configured with both 'get' and 'post' methods, ready for action!
src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.ts (6)
14-14: Superstar move! LaunchDarklyService imported brilliantly
Ah, importing LaunchDarklyService? That's style! This brings in feature flagging like a charm.
37-38: Injecting services with finesse
Including DateService and LaunchDarklyService in the constructor is spot on. Dependencies handled like a pro!
47-47: Type safety enhanced
Changing the parameter to PointerEvent in presentPopover method adds precision. Well done!
55-55: Generics applied wisely
Adding <string> to onDidDismiss ensures type clarity. That's the way!
106-106: Type assertion done right
Specifying { action: string } ensures we know exactly what's coming. Perfect!
65-73: Verify the feature flag 'personal_cards_platform'
Double-check that the feature flag is set up correctly and all variations are handled, boss.
Run this script to ensure the feature flag is in action:
✅ Verification successful
Let me gather more information about how this feature flag is being used.
Let me check one more thing to see how this feature flag is being handled in the service layer, mind it!
Mind it! The feature flag implementation is perfect, no changes needed!
Listen here, boss! The feature flag 'personal_cards_platform' is handled like a true superstar:
- In PersonalCardsService, it elegantly switches between two paths based on the flag:
- When false: Uses the classic ExpenseAggregationService
- When true: Goes for the new platform API route
- The component tests are rock solid, checking both variations
- The service has proper unit tests covering both scenarios
This is how you write code with style! flips sunglasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of 'personal_cards_platform' feature flag in the codebase
# Find all occurrences of 'personal_cards_platform'
rg "personal_cards_platform"
# Check where 'getVariation' is called with 'personal_cards_platform'
rg "getVariation\('personal_cards_platform'"
# Reminder to verify the flag in LaunchDarkly dashboard
echo "Please confirm that 'personal_cards_platform' is configured correctly in LaunchDarkly."
Length of output: 1100
Script:
#!/bin/bash
# Let's check the implementation details around this feature flag
# Check the personal-cards.page.ts implementation
rg -A 5 "personal_cards_platform" "src/app/fyle/personal-cards/personal-cards.page.ts"
# Check the bank-account-card component implementation
rg -A 5 "personal_cards_platform" "src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.ts"
# Check the test implementation to understand the expected behavior
rg -A 5 "personal_cards_platform" "src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts"
# Let's also check the PersonalCardsService to see how it handles the flag
ast-grep --pattern 'class PersonalCardsService {
$$$
deleteAccount($$$) {
$$$
}
$$$
}'
Length of output: 1329
Script:
#!/bin/bash
# Let's find the PersonalCardsService implementation
rg -A 10 "class PersonalCardsService"
# And specifically look for the deleteAccount method implementation
rg -B 2 -A 10 "deleteAccount\("
Length of output: 11969
src/app/core/services/personal-cards.service.ts (1)
127-136: 🧹 Nitpick (assertive)
Using POST for deletion? Let's ensure we're on the right track, my friend.
In RESTful design, the DELETE method is the superstar for deletion operations. Using POST might not align with best practices unless the API specifically requires it.
Let's verify if the DELETE method is supported for this endpoint:
...p/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts
Outdated
Show resolved
Hide resolved
src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.ts
Show resolved
Hide resolved
...p/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/app/core/services/personal-cards.service.ts(1 hunks)src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts(7 hunks)
🔇 Additional comments (3)
src/app/shared/components/bank-account-cards/bank-account-card/bank-account-card.component.spec.ts (3)
16-16: Mind it! The setup looks perfect, partner!
The LaunchDarklyService integration is done with style! Like how I handle my stunts - clean, precise, and well-orchestrated! 🕶️
Also applies to: 27-27, 36-36, 65-68, 78-78
104-104: Superstar move! Event type upgrade from Event to PointerEvent!
When you upgrade from Event to PointerEvent, you're showing class! Like upgrading from a regular auto to a super car! 🚗
Also applies to: 108-108
113-113: 🧹 Nitpick (assertive)
Kabali style testing! But let's verify the feature flag behavior!
The test is looking sharp, but like my famous dialogue goes - "When I say it once, it's like saying it a hundred times!" Let's make sure we test both true and false scenarios of the feature flag.
Add another test case to verify the platform API behavior:
+it('deleteAccount(): should delete account using platform API when feature flag is enabled', fakeAsync(() => {
+ launchDarklyService.getVariation.and.returnValue(of(true));
+ spyOn(component.deleted, 'emit');
+ loaderService.showLoader.and.resolveTo();
+ personalCardsService.deleteAccount.and.returnValue(of(deletePersonalCardRes));
+ loaderService.hideLoader.and.resolveTo();
+
+ component.deleteAccount();
+ tick();
+
+ expect(personalCardsService.deleteAccount).toHaveBeenCalledOnceWith(component.accountDetails.id, true);
+ expect(component.deleted.emit).toHaveBeenCalledTimes(1);
+}));Also applies to: 126-127, 134-134
|
Clickup
https://app.clickup.com/t/86cwm1x50
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
New Features
LaunchDarklyServicefor account deletion.Bug Fixes
presentPopovermethod.Tests
deleteAccount()method to ensure proper functionality with both APIs.BankAccountCardComponentto reflect changes in method signatures and dependencies.Documentation
BankAccountCardComponent.