-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add getCardSettings utility for nested card settings format #83669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
16ae04d
762a0b5
d23f75f
3156a2d
b0eb483
3e3c924
a61d76f
f314957
9c4b713
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import type { | |
| CompanyCardFeed, | ||
| CurrencyList, | ||
| ExpensifyCardSettings, | ||
| ExpensifyCardSettingsBase, | ||
| PersonalDetailsList, | ||
| Policy, | ||
| PrivatePersonalDetails, | ||
|
|
@@ -1071,6 +1072,21 @@ function isExpensifyCardFullySetUp(policy?: OnyxEntry<Policy>, cardSettings?: On | |
| return !!(policy?.areExpensifyCardsEnabled && cardSettings?.paymentBankAccountID); | ||
| } | ||
|
|
||
| function getCardSettings(cardSettings: OnyxEntry<ExpensifyCardSettings>, feedCountry?: string): ExpensifyCardSettingsBase | undefined { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be an onyx selector instead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could work as a selector today since no call site passes But in Phase 2 when some call sites need to pass const selector = useCallback(
(cs: OnyxEntry<ExpensifyCardSettings>) => getCardSettings(cs, feedCountry),
[feedCountry]
);
const [settings] = useOnyx(`...`, { selector }, [feedCountry]);Also right now without Keeping it as a plain utility for now keeps things simple and avoids a second refactor when Phase 2 lands. But happy to convert if you'd prefer the selector pattern regardless.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I just think the change would be smaller overall because you would not have to rename everything from cardSettings to settings, but all good. |
||
| if (!cardSettings) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (feedCountry) { | ||
| const feedCountryCardSettings = cardSettings[feedCountry as keyof typeof cardSettings]; | ||
| if (feedCountryCardSettings && typeof feedCountryCardSettings === 'object' && !Array.isArray(feedCountryCardSettings)) { | ||
| return feedCountryCardSettings as ExpensifyCardSettingsBase; | ||
| } | ||
| } | ||
|
|
||
| return cardSettings; | ||
| } | ||
|
|
||
| function isCardPendingIssue(card?: Card) { | ||
| return card?.state === CONST.EXPENSIFY_CARD.STATE.STATE_NOT_ISSUED; | ||
| } | ||
|
|
@@ -1368,6 +1384,7 @@ export { | |
| normalizeCardName, | ||
| hasIssuedExpensifyCard, | ||
| isExpensifyCardFullySetUp, | ||
| getCardSettings, | ||
| filterAllInactiveCards, | ||
| filterInactiveCards, | ||
| isCardPendingIssue, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hook now calls
getCardSettings(lastSelectedCardSettings)without afeedCountry, so when the backend returns the nested shape described in this PR (for exampleUS/TRAVEL_USbuckets), the helper returns the root object andlastSelectedSettings?.paymentBankAccountIDcan be missing. In that case thelastSelectedExpensifyCardFeedbranch is skipped and we fall back to another fund ID, which can open the wrong feed’s card data after Phase 2 rollout. Pass the selected feed country when reading settings here (and in the related call sites) so default feed selection remains correct for nested payloads.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the theory, but this is intentional for Phase 1. Today the backend only sends the flat format, so calling
getCardSettingswithoutfeedCountrycorrectly returns the root object withpaymentBankAccountIDintact.In Phase 2 when the backend starts sending nested format (
US,TRAVEL_USkeys), we'll update these call sites to pass the appropriatefeedCountry. This PR just sets up the abstraction layer so that transition is a small diff rather than a large refactor.