Moving workspace currency to a separate page by abzokhattab · Pull Request #70575 · Expensify/App

abzokhattab · 2025-09-15T14:58:29Z

Explanation of Change

Fixed Issues

$ #69737
PROPOSAL: #69737 (comment)

Tests

in the workspace list page click on 'New workspace'
Validate that currency field shows default currency with symbol (e.g., "USD - $")
Click on the currency field
Select a different currency (e.g., EUR, GBP, CAD)
Validate that you navigate back and currency field shows the newly selected currency with symbol (e.g., "EUR - €")
Click on the currency field again
Search for invalid currency and press enter
Validate that nothing happens and workspace is not created
Search for a valid one
Press enter on the keyboard
Validate that workspace is created successfully with the selected currency

Offline tests

Same as tests

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2025-09-17.at.20.17.23.mov

Android: mWeb Chrome

Screen.Recording.2025-09-17.at.20.18.45.mov

iOS: Native

Screen.Recording.2025-09-17.at.20.17.23.mov

iOS: mWeb Safari

Screen.Recording.2025-09-17.at.20.18.45.mov

MacOS: Chrome / Safari

Screen.Recording.2025-09-17.at.20.16.25.mov

MacOS: Desktop

Screen.Recording.2025-09-17.at.20.23.34.mov

melvin-bot · 2025-09-15T14:58:37Z

@truph01 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]

abzokhattab · 2025-09-17T17:53:12Z

@truph01

I kept the backTo parameter because the background page in the workspace confirmation and currency selection pages is determined based on backTo. Since the “Create Workspace” RHP can be triggered in multiple scenarios, I believe removing backTo could introduce breaking changes. It also doesn’t seem fully related to the current issue.

We could handle cleaning this up in a separate task by removing backTo from the “Create Workspace” flow entirely. Let me know what you think.

truph01 · 2025-09-22T16:19:40Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified there are no new alerts related to the canBeMissing param for useOnyx
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Screen.Recording.2025-10-06.at.13.45.04.mov

Android: mWeb Chrome

Screen.Recording.2025-10-06.at.13.37.59.mov

iOS: HybridApp

Screen.Recording.2025-10-06.at.13.36.16.mov

iOS: mWeb Safari

Screen.Recording.2025-10-06.at.13.37.09.mov

MacOS: Chrome / Safari

Screen.Recording.2025-10-06.at.13.32.24.mov

MacOS: Desktop

Screen.Recording.2025-10-06.at.13.35.36.mov

truph01 · 2025-09-23T10:25:37Z

@abzokhattab Could you help merge main?

abzokhattab · 2025-09-23T10:39:17Z

sure!! done

src/components/CurrencySelector.tsx

truph01 · 2025-09-25T18:19:48Z

src/components/WorkspaceConfirmationForm.tsx

    const [workspaceNameFirstCharacter, setWorkspaceNameFirstCharacter] = useState(defaultWorkspaceName ?? '');

-    const userCurrency = allPersonalDetails?.[session?.accountID ?? CONST.DEFAULT_NUMBER_ID]?.localCurrencyCode ?? CONST.CURRENCY.USD;
+    const userCurrency = draftValues?.currency ?? allPersonalDetails?.[session?.accountID ?? CONST.DEFAULT_NUMBER_ID]?.localCurrencyCode ?? CONST.CURRENCY.USD;


It seems we currently lack logic to clear draftValues?.currency when this page is closed. As a result, if a user selects a currency, closes the page, and then reopens it, the previously selected currency is incorrectly retained as the default.

However, based on the behavior observed in staging, each time the page is opened, the default currency should be determined using:

allPersonalDetails?.[session?.accountID ?? CONST.DEFAULT_NUMBER_ID]?.localCurrencyCode ?? CONST.CURRENCY.USD;

I added this use effect to clear the draft value on closing the page

useEffect(() => { return () => { clearDraftValues(ONYXKEYS.FORMS.WORKSPACE_CONFIRMATION_FORM); }; }, []);

just like how we are doing in other pages ... what do u think ?

src/components/WorkspaceConfirmationForm.tsx

src/pages/CurrencySelectionPage.tsx

truph01 · 2025-09-25T18:40:48Z

src/components/CurrencySelector.tsx

-            title={currency}
+            title={shouldShowCurrencySymbol && currency ? `${currency} - ${getCurrencySymbol(currency)}` : currency}
            ref={ref}
            descriptionTextStyle={currencyTitleDescStyle}


It looks like it will break the description's style by comparing with the staging behavior.

good catch ... i modified the descritpion style to check if the shouldShowCurrencySymbol is false

https://github.com/Expensify/App/pull/70575/files#diff-918190f9217d561422ca355cc5ee85de4f9dba8e69b672566b197b2d3d74200cR62

what do u think.. this ensures backward combilitability and works well for the new component as well

Was this resolved?

src/components/CurrencySelector.tsx

truph01 · 2025-09-25T18:51:34Z

Bug 1 (I am not sure whether it is bug, so we need to ask design/internal team to confirm):

Open /workspace/confirmation/currency?backTo=workspaces screen.
Refresh the page.
Observe that user is still in /workspace/confirmation/currency?backTo=workspaces screen. But comparing with staging behavior, user is navigated to Confirm Workspace screen.

truph01 · 2025-09-25T18:51:54Z

@abzokhattab I left a few comments

codecov · 2025-09-28T17:29:46Z

Codecov Report

❌ Patch coverage is 24.00000% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pages/CurrencySelectionPage.tsx	0.00%	11 Missing ⚠️
src/components/WorkspaceConfirmationForm.tsx	0.00%	3 Missing ⚠️
src/libs/actions/Policy/Policy.ts	0.00%	2 Missing ⚠️
src/ROUTES.ts	0.00%	1 Missing ⚠️
src/components/CurrencySelector.tsx	85.71%	0 Missing and 1 partial ⚠️
...gation/AppNavigator/ModalStackNavigators/index.tsx	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

abzokhattab · 2025-09-28T18:24:55Z

Bug 1 (I am not sure whether it is bug, so we need to ask design/internal team to confirm):

is that considered a bug? i would expect that to the intended behavior now since we are moving the currency to a new seperate route. we need to confirm that behaviour

abzokhattab · 2025-09-28T18:25:29Z

@abzokhattab I left a few comments

thanks for the review .. i covered ur comments .. please take another look when u have time

truph01 · 2025-09-29T02:58:42Z

is that considered a bug? i would expect that to the intended behavior now since we are moving the currency to a new seperate route. we need to confirm that behaviour

@abzokhattab Please help ask internal team to confirm it. From my side, I also think it isn't a bug

src/components/WorkspaceConfirmationForm.tsx

abzokhattab · 2025-09-29T22:48:10Z

Hi @aldo-expensify, we need your help regarding the following new behavior. Can you confirm if this is expected?

User clicks Create Workspace.
Click the Currency menu option.
Select a currency.
Open the currency list again.
Refresh the page.
After the refresh, the page remains on the currency list, and the option selected in step 3 is still selected.

Refer to #70575 (comment) and #70575 (comment)

Screen.Recording.2025-09-30.at.00.42.20.mov

truph01 · 2025-10-01T10:15:15Z

After the refresh, the page remains on the currency list, and the option selected in step 3 is still selected.

@abzokhattab Regarding the part where "the option selected in step 3 is still selected"—I believe this is a bug. It should match the current behavior on staging. Let's update the PR to address this.

As for "After the refresh, the page remains on the currency list," I think that's expected behavior now, since we're using the currency picker as a full screen instead of a modal.

abzokhattab · 2025-10-01T13:09:16Z

@truph01 I know it’s not the same as staging, but since we moved currency selection to a separate page, it’s now possible to persist the user’s choice—just like we do in other flows, such as the Merchant field in Create Expense (attached a video).

so previously, we couldn’t keep the currency modal open because the currency URL was the same as Create Workspace, so we couldn’t tell them apart. Now that they’re separate, why wouldn’t we keep the selected option?

Screen.Recording.2025-10-01.at.15.04.02.mov

cc @aldo-expensify

aldo-expensify · 2025-10-01T17:33:29Z

@truph01 I know it’s not the same as staging, but since we moved currency selection to a separate page, it’s now possible to persist the user’s choice—just like we do in other flows, such as the Merchant field in Create Expense (attached a video).

I other fields edited in their own pages retain the edited value after refresh, I think it is fine to do it here and be consistent with that (even if staging is behaving differently right now)

so previously, we couldn’t keep the currency modal open because the currency URL was the same as Create Workspace, so we couldn’t tell them apart. Now that they’re separate, why wouldn’t we keep the selected option?

How it works in your PR makes sense to me, I would keep it like that if it is consistent with other fields with separate pages.

abzokhattab · 2025-10-03T01:27:34Z

what do u think @truph01

truph01 · 2025-10-03T02:52:46Z

@abzokhattab Based on this comment, the current behavior in our PR looks good

truph01 · 2025-10-03T03:01:08Z

@abzokhattab Could you merge main again?

truph01 · 2025-10-03T10:30:55Z

@abzokhattab We have regression in workspace creation flow. The test step mentioned in this PR

Screen.Recording.2025-10-03.at.17.14.14.mov

abzokhattab · 2025-10-04T12:03:41Z

Good point @truph01 ... i have fixed this bug here is the behaviour after the fix

Screen.Recording.2025-10-04.at.14.02.10.mov

OSBotify · 2025-10-06T19:53:02Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2025-10-08T05:10:12Z

🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.2.27-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

OSBotify · 2025-10-08T21:34:43Z

🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

jayeshmangwani · 2025-12-02T08:53:37Z

@truph01 @abzokhattab Coming from this issue #73396 , this PR adds a new route for workspace currency selection. However, it introduced a problem in the WorkspaceConfirmationForm, which prevented navigation from the book-travel -> workspace-upgrade page flow to the workspace currency selection. We fixed it here #74035 by adding the new route for the confirmation instead of using the modal. More context here: #73396 (comment)

abzokhattab added 3 commits September 11, 2025 02:32

Fix workspace currency selection issue

4273e4a

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

fc3b2cd

moving currency to a seperate page

c0a51b0

abzokhattab requested a review from a team as a code owner September 15, 2025 14:58

melvin-bot bot assigned abzokhattab Sep 15, 2025

melvin-bot bot requested review from truph01 and removed request for a team September 15, 2025 14:58

abzokhattab added 4 commits September 15, 2025 18:00

cleaning

cbeb086

fixing prettier

6b2fb15

fixing eslint

39a7d89

fixing eslint

feedb0f

abzokhattab marked this pull request as draft September 15, 2025 15:14

abzokhattab added 2 commits September 15, 2025 20:47

using CurrencySelector instead of WorkspaceCurrencyPicker

e53d1b6

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

38ab876

abzokhattab marked this pull request as ready for review September 17, 2025 17:53

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

5d4f19f

truph01 reviewed Sep 25, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

3d06394

abzokhattab added 3 commits September 28, 2025 20:04

fix: adjust currency selector description style logic

d50429e

Fix draft values persistence in workspace confirmation form

ce6f713

cleanup

b391aa5

abzokhattab added 2 commits September 28, 2025 20:26

cleanup style prop

c32d72e

cleanup styles prop

4032a02

truph01 reviewed Sep 29, 2025

View reviewed changes

src/components/WorkspaceConfirmationForm.tsx Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

68bcce9

fixing create workspace back navigation

7c592c3

Merge remote-tracking branch 'origin/main' into workspace-currency-fix

2d85c66

truph01 approved these changes Oct 6, 2025

View reviewed changes

melvin-bot bot requested a review from aldo-expensify October 6, 2025 08:54

aldo-expensify approved these changes Oct 6, 2025

View reviewed changes

aldo-expensify merged commit 83b1cd4 into Expensify:main Oct 6, 2025
28 of 29 checks passed

OSBotify mentioned this pull request Oct 8, 2025

Deploy Checklist: New Expensify 2025-10-08 #72057

Closed

62 tasks

melvin-bot bot mentioned this pull request Oct 8, 2025

[$250] [Due for payment 2025-10-15] Workspace - Able to create new Workspace with invalid currency #69737

Closed

8 tasks

jayeshmangwani mentioned this pull request Dec 2, 2025

[Due for payment 2025-11-20] [$250] Workspace - changing the workspace default currency is unresponsive in book travel flow #73396

Closed

8 tasks

This was referenced Dec 4, 2025

[CLEAN] Synthetic Benchmark PR #70575 - Moving workspace currency to a separate page qodo-benchmark/App#33

Open

[CORRUPTED] Synthetic Benchmark PR #70575 - Moving workspace currency to a separate page qodo-benchmark/App#34

Open

Conversation

abzokhattab commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Sep 15, 2025

Uh oh!

abzokhattab commented Sep 17, 2025

Uh oh!

truph01 commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

truph01 commented Sep 23, 2025

Uh oh!

abzokhattab commented Sep 23, 2025

Uh oh!

Uh oh!

truph01 Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

abzokhattab Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

truph01 Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

abzokhattab Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

truph01 commented Sep 25, 2025

Uh oh!

truph01 commented Sep 25, 2025

Uh oh!

codecov bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

abzokhattab commented Sep 28, 2025

Uh oh!

abzokhattab commented Sep 28, 2025

Uh oh!

truph01 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

abzokhattab commented Sep 29, 2025

Uh oh!

truph01 commented Oct 1, 2025

Uh oh!

abzokhattab commented Oct 1, 2025

Uh oh!

aldo-expensify commented Oct 1, 2025

Uh oh!

abzokhattab commented Oct 3, 2025

Uh oh!

truph01 commented Oct 3, 2025

Uh oh!

truph01 commented Oct 3, 2025

Uh oh!

truph01 commented Oct 3, 2025

Uh oh!

abzokhattab commented Oct 4, 2025

Uh oh!

Uh oh!

OSBotify commented Oct 6, 2025

Uh oh!

OSBotify commented Oct 8, 2025

abzokhattab commented Sep 15, 2025 •

edited

Loading

truph01 commented Sep 22, 2025 •

edited

Loading

codecov bot commented Sep 28, 2025 •

edited

Loading

truph01 commented Sep 29, 2025 •

edited

Loading