Allow linking an existing card feed to a non-USD workspace - #98509
Allow linking an existing card feed to a non-USD workspace#98509MelvinBot wants to merge 11 commits into
Conversation
Co-authored-by: Chuck Dries <chuckdries@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…elector Co-authored-by: Chuck Dries <chuckdries@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@Eskalifer1 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] |
|
🤖 No code fix needed — all real checks now pass. When the request came in, several checks were still running (the Android builds, perf tests) and the The only remaining failing check is Nothing to push here. Once you've filled out the |
|
🤖 Still nothing to push here — no code failure to fix. I re-checked all checks: every build, test, perf, and checklist check passes. The only remaining red check is This PR needs a human reviewer, not a code change. |
|
I will review it today! |
trjExpensify
left a comment
There was a problem hiding this comment.
conflict, @chuckdries. Looks great though.. thanks for the quick work. 👍
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f8f9c3aa6
ℹ️ 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".
| if (!(policy?.outputCurrency === CONST.CURRENCY.USD || isUkEuCurrencySupported)) { | ||
| // The supported currency restriction only applies to enrolling a brand-new card program. | ||
| // If hasAccessibleFeeds is true, allow the flow to start in order to link an existing feed | ||
| if (!hasAccessibleFeeds && !(policy?.outputCurrency === CONST.CURRENCY.USD || isUkEuCurrencySupported)) { |
There was a problem hiding this comment.
Require a linkable feed before bypassing currency prompt
When the only accessible Expensify card feeds are pending deletion, hasAccessibleFeeds is still true even though the selector disables those feeds via isFeedPendingDelete. On an unsupported-currency workspace such as CAD/AUD, this skips the Update-to-USD prompt and opens the selector where no feed can be linked and the new-program CTA is hidden, leaving the admin unable to continue. Please base this bypass on at least one non-deleted/linkable feed, or keep the currency prompt when no usable feed exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Frankly this feels like an edge case I'm not particularly worried about. @MelvinBot can you check if the user sees a greyed out entry in the list in this case, or an empty panel, or something else? What happens?
There was a problem hiding this comment.
The user sees a greyed-out, non-clickable entry in the selector — not an empty panel. It's a dead end, but a visible one.
Walking the code for the exact scenario Codex described (brand-new CAD/AUD workspace, the only accessible feed is pending deletion on another workspace):
hasAccessibleFeedsis true, so the currency prompt is skipped andstartFlow()opens the "Select cards" pane.- The pending-delete feed isn't linked to this new workspace, so it lands in
otherFeedsand renders under the "From other workspaces" heading. toListItemsetsisDisabled: isFeedPendingDelete, so the row shows the feed name + card icon but is greyed out and can't be tapped.- Because
primaryFeedsis empty and the currency is unsupported,shouldShowIssueCardButtonis false, so the+"set up new feed" button is also hidden.
So the admin lands on a pane with a header, the "From other workspaces" label, and a single disabled row — nothing they can click. No blank/empty panel, no crash, no error toast; just a greyed row with no way forward.
One nuance: if that pending-delete feed happened to already be linked to this workspace (i.e. it's a primaryFeed), it renders greyed-out in the main list instead, and issueCardFundID would resolve to it, so an "Issue card" button would appear — but it points at the feed being deleted. In the fresh-workspace case Codex flagged, though, it's the "other workspaces" greyed-row path above.
Derived from reading the code on this branch (4f8f9c3) rather than a live repro — the pendingAction: DELETE feed state is transient and impractical to stage reliably. Happy to dig further if you want a runtime confirmation.
This comment was marked as resolved.
This comment was marked as resolved.
BTW - We can add the same button(leading to the Workspace currency) to the empty state :) |
That all sounds good to me. Though I'd probably say just use a confirmation modal instead of a RHP with illustration. So I'd just update the copy instead on the modal we have. cc @Expensify/design for thoughts |
|
I like that idea Jon - just updating the existing modal with accurate copy. |
|
Hi @shawnborton @dubielzyk-expensify I think you've overlooked one of the reasons why the approach involving removing the modal window was proposed in the first place. Currently, with the new changes, the user might see a result like this on the selection page: If we simply refresh the modal window, this bug will still remain |
|
So just to make sure I am following, we are only solving for this? "But in the rare edge case where someone deletes the workspace while you have that RHP open, it becomes blank" |
Basically, yes, this proposal arose as a result of this edge case. But @chuckdries also suggested removing the modal window, since it’s currently inaccurate (because we support currencies other than USD). |
|
Okay I think maybe I would do both then: update the modal to be accurate, and then in the rare case where the Select feeds RHP could become empty, use some kind of empty state. |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
|
🤖 No code fix needed. The fresh CI run (after the
Everything else is green — typecheck, ESLint, Oxfmt, Bun tests, all 8 unit-test jobs, Storybook, knip, builds, and both Reassure perf tests. There's nothing for me to push; to clear the two reds a reviewer needs to complete their checklist and give an independent approval. |
cc @jamesdeanexpensify for some copy help. I don't think the dialog headline should be Expensify but something more descriptive |
|
My vote on the copy:
|
|
@trjExpensify I made two tiny tweaks to yours above, but it looks good to me if it works for you! |
|
|
|
@chuckdries can you add a period at the end of the second sentence? Thank you! |
…l locales Co-authored-by: Chuck Dries <chuckdries@users.noreply.github.com>
|
Fixed the failing Root cause: Fix: Added About the other two failing checks:
Please give the translations a quick sanity check — the values are my best-effort renderings of "Get the Expensify Card" using each locale's existing term for the Expensify Card. |
|
|
|
The earlier fix worked —
Nothing else is failing (remaining checks are passing or still running), so no further push is needed from me. |
Forgetting to say please, where are my manners! ;) 👍 |
|
@Eskalifer1 Sounds like we're good to proceed with review on this one |



Explanation of Change
We only allow onboarding for Expensify card in workspaces that use one of our supported currencies: USD, GBP, EUR. But, if you already have an active E-card feed on a different workspace, you're supposed to be able to link that feed to a workspace of any currency. This PR allows that. It all already worked, but you used to have to set it up on OldDot.
This change skips the currency prompt when the workspace has accessible feeds (
hasAccessibleFeeds). In that case the CTA callsstartFlow()directly, which already routes to the feed selector so the admin can link an existing feed.Fixed Issues
$ #98503
PROPOSAL: #98503 (comment)
Tests
Enrolling is disallowed on unsupported currencies
disallows.enrolling.fresh.on.unsupported.currency.mp4
Linking a USD feed is allowed from a non-USD workspace
+icon (which would normally let you set up a new feed)allows.linking.existing.USD.feed.mp4
USD workspaces are allowed to link an existing feed and set up a new one
+icon, and pressing it takes you to the bank account setup flowallows.setting.up.new.USD.feed.mp4
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
See tests