Filter Amex Direct parent card from company cards page#82448
Conversation
… picker For Amex Direct (FDX) feeds, accountList[0] is the parent card (primary account holder) which aggregates child accounts. This card should not appear in the company cards list or be assignable, as it does not represent a distinct card. Adds a feedName parameter to getFilteredCardList and buildCompanyCardEntries. When the feed is Amex Direct, both functions skip accountList[0] when building the list of unassigned cards. Already-assigned cards are unaffected. Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
MelvinBot
force-pushed
the
claude-filterAmexParentCard
branch
from
7090ed0 to
6c68930
Compare
|
Fixed both failing checks: 1. 2. Changes implemented:
|
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Fixed the failing |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/hooks/useCompanyCards.ts
Outdated
| for (const name of accountList ?? []) { | ||
| // For Amex Direct (FDX) feeds, accountList[0] is the parent card (primary account holder) | ||
| // which aggregates child accounts and should not be assignable. | ||
| const isAmexDirectFeed = feedName ? getCompanyCardFeed(feedName).startsWith(CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT) : false; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The Amex Direct parent card filtering logic is duplicated between useCompanyCards.ts (here) and CardUtils.ts (getFilteredCardList). Both locations compute isAmexDirectFeed and slice/skip accountList[0] in the same way. This duplicated logic should be extracted into a shared helper in CardUtils.ts (e.g., filterAmexDirectParentCard) so both call sites can reuse it.
Suggested fix — add a helper to CardUtils.ts and use it in both places:
// In CardUtils.ts
function filterAmexDirectParentCard(accountList: string[], feedName?: CompanyCardFeedWithDomainID): string[] {
const isAmexDirectFeed = feedName ? getCompanyCardFeed(feedName).startsWith(CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT) : false;
return isAmexDirectFeed ? accountList.slice(1) : accountList;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch. Extracted the duplicated Amex Direct parent card filtering logic into a shared filterAmexDirectParentCard helper in CardUtils.ts. Both getFilteredCardList and buildCompanyCardEntries now call it instead of computing isAmexDirectFeed and slicing inline.
…x Direct parent card filtering logic Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
The only failing check is All code-related checks are passing:
The |
src/libs/CardUtils.ts
Outdated
| const isAmexDirectFeed = feedName ? getCompanyCardFeed(feedName).startsWith(CONST.COMPANY_CARD.FEED_BANK_NAME.AMEX_DIRECT) : false; | ||
| return isAmexDirectFeed ? accountList.slice(1) : accountList; |
There was a problem hiding this comment.
@carlosmiceli Is the first card a guaranteed parent card? If not, we might have a bug causing missing cards in newDot, and we should also confirm if this logic is necessary for oldDot.
There was a problem hiding this comment.
That's what I've seen in every case so far, have you seen a different case at some point? Let's see if @robertjchen or @amyevans agree on this approach 👍
There was a problem hiding this comment.
Is the first card a guaranteed parent card?
Yes, just based on the way that we build the data in the Scraper class the first card will always be a "parent" card.
However just removing the first card in the list is going to be an incomplete fix... Plenty of Amex connections have multiple card programs, so we'd want to ideally filter out all parent cards. Perhaps this could be achieved by looking for the matches and removing the card without a name in the found pair. So for example if the accountList has:
Business Platinum Card - 02015
Business Platinum Card - BOB ARNOLD - 02015
It removes Business Platinum Card - 02015
There was a problem hiding this comment.
Ah good to know, thanks Amy, will do.
There was a problem hiding this comment.
Updated filterAmexDirectParentCard to handle multiple card programs. Instead of naively removing accountList[0], it now:
- Scans the list for child cards (3+ segments when split by
-) and collects their card type prefixes - Filters out any parent card (exactly 2 segments:
CardType - Digits) whose card type has at least one child
This handles feeds with multiple card programs (e.g., Platinum + Gold) where each program has its own parent card at any position in the list. Added a dedicated test for the multi-program scenario.
|
The only failing check is This is not a code issue — no code changes can fix it. The reviewer needs to post their checklist comment to resolve this check. |
|
Reviewed the feedback on this PR. Here's the status of each comment thread:
No outstanding actionable code changes remain from the current review feedback. |
|
The only failing check is All code-related checks (typecheck, ESLint, tests, spellcheck, builds, Prettier, etc.) are passing. |
|
The only failing check is This check requires a reviewer to post the reviewer checklist (from All code-related checks are passing: ESLint, typecheck, Prettier, tests (all 8 jobs), builds (all platforms), spellcheck, verifySignedCommits, and storybook tests. |
|
The only failing check is This check will pass once the assigned reviewer goes through and checks the boxes in their reviewer checklist comment. No code changes are needed. |
Updated filterAmexDirectParentCard to detect parent cards structurally by checking segment count (CardType - Digits vs CardType - Name - Digits) instead of naively removing accountList[0]. This handles multiple card programs per Amex Direct feed. Also replaced real card data in tests with fake data. Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Fixed the failing The other failing check ( |
Co-authored-by: Carlos Miceli <carlosmiceli@users.noreply.github.com>
|
Fixed the failing checks:
|
|
All checks are now passing on the latest run. The previous failures were:
The current run shows all checks green: typecheck, all 8 test jobs, ESLint, Changed files ESLint, Prettier, builds, perf-tests, Storybook tests, spellcheck, and verifySignedCommits all pass. No fix was needed from my end. |
|
Reviewed all open feedback threads — here's the current status:
All code-related CI checks are passing (ESLint, typecheck, Prettier, tests, builds, signed commits). The only failing check is the PR Reviewer Checklist, which requires manual completion by a reviewer. |
|
The only failing check is PR Reviewer Checklist, which is not related to the code changes in this PR. It fails when a reviewer hasn't completed their checklist items. This is not something that can be fixed with a code change — it requires a reviewer to complete the checklist. All code-related checks (ESLint, typecheck, Prettier, tests, builds, spellcheck, verifySignedCommits) are passing. |
|
The only failing check is All other checks (ESLint, typecheck, Prettier, tests, builds, spellcheck, verifySignedCommits, etc.) are passing. |
|
The only failing check is PR Reviewer Checklist, which requires a reviewer to manually complete the checklist items. This is not a code or CI issue — all other checks (ESLint, typecheck, Prettier, tests, builds, signed commits, etc.) are passing. No code fix is needed. |
|
Reviewed all open feedback threads. All review comments are addressed in the current code:
All CI checks are passing. |
|
The only failing check is the PR Reviewer Checklist ( All code-related checks (ESLint, typecheck, Prettier, tests, builds, spellcheck, signed commits, etc.) are passing. No code fix is needed here — a reviewer just needs to complete their checklist. |
|
The only failing check is the PR Reviewer Checklist — a reviewer posted their checklist but hasn't checked off all the items yet (0 out of 57 completed). This is not a code issue and can't be fixed with a push. All code checks are passing: ESLint, typecheck, Prettier, tests (all 8 shards), builds (all 4), spellcheck, signed commits, CLA, unused styles, storybook, perf-tests, and codecov. |
|
The only failing check is the PR Reviewer Checklist ( |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safarimain: branch: |
|
The only failing check is PR Reviewer Checklist — a reviewer posted their checklist but hasn't checked all the boxes yet (0 out of 57 items completed). All code checks (tests, typecheck, ESLint, Prettier, builds, signed commits, etc.) are passing. This is not a code issue — it requires the reviewer to complete their checklist. No code fix is needed. |
|
The only failing check is PR Reviewer Checklist ( |
|
The only failing check is the PR Reviewer Checklist ( All CI checks are passing: ESLint, Prettier, typecheck, all 8 test jobs, builds, spellcheck, signed commits, CLA, and security scans. |
|
Mkay, the checklist failing is a bug (I opened a new issue for it here: https://github.com/Expensify/Expensify/issues/604937) but for now I'm just going to manually edit Melvin's comment in #82448 (comment) to remove the checklist mention |
|
Checks are passing now. Code LGTM, but @fedirjh @chrispader please review. I will merge tomorrow if y'all are happy with it! |
|
🚧 @amyevans 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/amyevans in version: 9.3.28-0 🚀
|
|
@carlosmiceli @amyevans @heyjennahay Can this one please be tested internally, Currently we are blocked from using Amex credentials due to 2FA authentication, Reference here |
|
Yes, I can test it tomorrow. |
|
@m-natarajan just checked, works great. |
|
@carlosmiceli thanks for QAing! can you check it off the list in #83947? |
|
🚀 Deployed to staging by https://github.com/amyevans in version: 9.3.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.30-3 🚀
|
8 participants
Explanation of Change
For Amex Direct (FDX) feeds,
accountList[0]is the parent card (primary account holder), which aggregates child accounts rather than representing a distinct individual card. This parent card should not appear in the company cards page or be assignable, as assigning it causes confusion and duplicate transactions.This PR adds a
feedNameparameter togetFilteredCardList(inCardUtils.ts) andbuildCompanyCardEntries(inuseCompanyCards.ts). When the feed is an Amex Direct feed (oauth.americanexpressfdx.com), both functions now skipaccountList[0]when building the list of unassigned cards. Already-assigned cards (Phase 1 ofbuildCompanyCardEntries) are unaffected — if the parent card was somehow already assigned, it would still display.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/600138
PROPOSAL: https://github.com/Expensify/Expensify/issues/600138#issuecomment-3897665859
Tests
accountList, which has no cardholder name — e.g., "Business Platinum Card - 03005") does not appear in the card listOffline tests
No offline-specific behavior changes. The filtering is applied purely to the in-memory card list rendering.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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, only filtering logic
Android: mWeb Chrome
N/A - No UI changes, only filtering logic
iOS: Native
N/A - No UI changes, only filtering logic
iOS: mWeb Safari
N/A - No UI changes, only filtering logic
MacOS: Chrome / Safari
N/A - No UI changes, only filtering logic