Auto-detect nested card program in getCardSettings for Phase 2 compatibility#84285
Conversation
melvin-bot
bot
requested review from
heyjennahay and
mananjadhav
and removed request for
a team
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
allgandalf
requested review from
a team
and removed request for
heyjennahay and
mananjadhav
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/libs/CardUtils.ts
Outdated
| // Auto-detect: try known card programs in priority order so callers that | ||
| // don't pass feedCountry still get the right program sub-object when the | ||
| // backend sends nested settings (Phase 2 of fixing shared Onyx key). | ||
| const result = tryNested(CONST.COUNTRY.US) ?? tryNested('CURRENT') ?? tryNested('GB'); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The string literals 'CURRENT' and 'GB' are hardcoded magic strings. 'GB' already has a constant defined at CONST.COUNTRY.GB (the same line already uses CONST.COUNTRY.US, so this is inconsistent). 'CURRENT' has no constant defined anywhere and should be added to a relevant CONST object (e.g., CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT or similar) so its meaning is discoverable and centralized.
Additionally, 'CURRENT' and 'GB' are not represented in the ExpensifyCardSettings type definition (only US and TRAVEL_US are typed as optional nested keys). This means cardSettings[key as keyof typeof cardSettings] relies on an unsafe as cast to access keys the type system doesn't know about. Consider adding CURRENT?: ExpensifyCardSettingsBase and GB?: ExpensifyCardSettingsBase to the ExpensifyCardSettings type to make these accesses type-safe.
Suggested fix:
// 1. Add to CONST (or appropriate location):
CARD_PROGRAM: {
CURRENT: 'CURRENT',
} as const,
// 2. Update ExpensifyCardSettings type to include GB and CURRENT:
type ExpensifyCardSettings = OnyxCommon.OnyxValueWithOfflineFeedback<
ExpensifyCardSettingsBase & {
US?: ExpensifyCardSettingsBase;
GB?: ExpensifyCardSettingsBase;
CURRENT?: ExpensifyCardSettingsBase;
TRAVEL_US?: ExpensifyCardSettingsBase;
hasOnceLoaded?: boolean;
}
>;
// 3. Use constants on line 1102:
const result = tryNested(CONST.COUNTRY.US) ?? tryNested(CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT) ?? tryNested(CONST.COUNTRY.GB);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
addressed in 90e7daf — replaced magic strings with CONST.COUNTRY.GB and CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT, added GB? and CURRENT? to ExpensifyCardSettings type, renamed tryNested to getMergedProgramSettings
allgandalf
requested review from
Gonals,
blimpich and
rlinoz
and removed request for
Gonals
allgandalf
changed the title
blimpich
left a comment
blimpich
left a comment
There was a problem hiding this comment.
LGTM. I checked out the branch locally and merged in the latest main so that it will have the 1:1:1 changes, and it works perfect! Toggled it on and off and inspected the onyx data being returned. Handled it perfectly 👍
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a40d674b45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const getMergedProgramSettings = (programKey: string): ExpensifyCardSettingsBase | undefined => { | ||
| const programSettings = cardSettings[programKey as keyof typeof cardSettings]; | ||
| if (programSettings && typeof programSettings === 'object' && !Array.isArray(programSettings)) { | ||
| return {...cardSettings, ...(programSettings as ExpensifyCardSettingsBase)} as ExpensifyCardSettingsBase; |
There was a problem hiding this comment.
This merge order makes nested values always override root values, so root-level optimistic writes are dropped whenever a nested program object exists. In this repo, updateSettlementAccount and updateSettlementFrequency still write paymentBankAccountID/monthlySettlementDate at the root of PRIVATE_EXPENSIFY_CARD_SETTINGS (src/libs/actions/Card.ts), so after this change getCardSettings() can keep returning stale nested values and the settings UI may not reflect account/frequency changes until a full server refresh (or at all if no nested update arrives).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch — flipped the merge order to {...programSettings, ...cardSettings} so root-level optimistic writes (e.g. paymentBankAccountID from updateSettlementAccount) take precedence over stale nested values. nested-only fields still come through since root won't have them
|
@codex review |
Thanksssssssss 🕺 |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
https://github.com/codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36db661fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/libs/CardUtils.ts
Outdated
| if (programSettings && typeof programSettings === 'object' && !Array.isArray(programSettings)) { | ||
| // Spread nested first so root-level optimistic writes (e.g. paymentBankAccountID | ||
| // from updateSettlementAccount) take precedence over stale nested values. | ||
| return {...(programSettings as ExpensifyCardSettingsBase), ...cardSettings} as ExpensifyCardSettingsBase; |
There was a problem hiding this comment.
Spreading programSettings before cardSettings lets root-level fields overwrite the selected program payload, which can surface the wrong card config once Phase 2 sends multiple nested programs under one key. In this repo, Travel Invoicing actions still write fields like paymentBankAccountID and isEnabled at the root of the same private_expensifyCardSettings_* entry, so after a travel update a US card read can be overwritten by travel/root values even when US nested data is correct; this affects callers like workspace Expensify Card pages that rely on getCardSettings() for settlement/account state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
reverted back to {...root, ...nested} — nested program values are the authoritative source from the backend, this matches the existing getTravelSettings pattern. optimistic writes are temporary and get corrected by the server response, so nested should win to avoid cross-program leakage (e.g. travel paymentBankAccountID at root overwriting the correct US nested value)
blimpich
left a comment
blimpich
left a comment
There was a problem hiding this comment.
I tested this locally again and it works
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.3.32-0 🚀
|
|
This PR failing because of the issue #84394 1.mp4 |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
5 participants
Explanation of Change
Phase 1 (PR #83669, already merged) added the
getCardSettings(cardSettings, feedCountry?)utility and routed all callers through it. However, no caller currently passesfeedCountry.Phase 2 (Auth PR #20194) switches the backend to push full nested card settings (
{ US: {...}, GB: {...}, TRAVEL_US: {...} }) instead of flat per-feed objects. Without this change, all callers ofgetCardSettingswould receive the full nested object back, andsettings?.paymentBankAccountIDetc. would beundefinedsince those fields are nested under program keys rather than at root.This PR makes
getCardSettingssmart enough to handle both formats:feedCountryis provided: extracts the nested sub-object and merges it with root-level properties (likedomainName,preferredPolicy) so callers still have access to everythingfeedCountryis NOT provided: auto-detects the program by trying known card program keys in priority order (US→CURRENT→GB), then mergesThis follows the same merge pattern already used by
getTravelSettingsinTravelInvoicingUtils.ts.Fixed Issues
$ #81472
Tests
npx jest tests/unit/CardUtilsTest.ts --testNamePattern="getCardSettings"— all 11 tests passOffline tests
N/A — no network behavior changes, this is a pure data-layer utility change.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — no UI changes, pure utility function update with unit tests.
Android: mWeb Chrome
N/A — no UI changes, pure utility function update with unit tests.
iOS: Native
N/A — no UI changes, pure utility function update with unit tests.
iOS: mWeb Safari
N/A — no UI changes, pure utility function update with unit tests.
MacOS: Chrome / Safari
N/A — no UI changes, pure utility function update with unit tests.