Ask non-USD Workspaces to update currency before adding VBBA #19638

luacmartins · 2023-05-25T23:27:34Z

Details

Part 1 of 2 from #16744
Here we'll start preventing users from initiating/completing the VBBA flow if their Workspace currency is different than USD.

Fixed Issues

$ #16744

Tests

Create a Workspace
Navigate to Settings > Workspace > Settings > Default Currency
Set the currency to AED
Navigate to Settings > Workspace > Bank account
Verify that you see the modal below
Tap Cancel
Verify that the modal is closed
Tap Bank account again and verify that the modal opens again
Tap Update to USD
Verify that you are redirected to the Connect bank account page
Navigate to Settings > Workspace > Settings > Default Currency
Verify that the currency is now set to USD
Change the currency to AED again

Steps below are for Web only:

In the URL bar, copy the Workspace id. For example 9283AFFC0378797B in workspace/9283AFFC0378797B/settings
Navigate directly to /bank-account/CompanyStep?policyID=<Workspace id>, replacing <Workspace id> with the value copied in step 14
Verify that you see the following error message Oops! It appears that your workspace currency is set to a different currency than USD. To proceed, please set it to USD and try again

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

melvin-bot · 2023-06-08T19:51:59Z

@rushatgabhane @thienlnam One of you needs to 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]

mountiny · 2023-06-12T15:37:26Z

@rushatgabhane Will you be able to complete the checklist on this one or should we ask some other C+?

rushatgabhane · 2023-06-12T18:01:45Z

@mountiny I'll be able to complete it

luacmartins · 2023-06-13T20:05:38Z

This is a race condition between Policy.updateGeneralSettings and OpenReimbursementAccountPage. I think we have two options here:

Use makeRequestWithSideEffects to first update the currency and then navigate to the bank page, which calls OpenReimbursementAccountPage
Create a new API command to update the currency and return the data in OpenReimbursementAccountPage. This solution would normally be preferred, but in this case I'm unsure. We'd have to somehow prevent calling OpenReimbursementAccountPage if we're being navigated from the currency update modal but then call it in every other instance.

@thienlnam @mountiny would love your thoughts on the above solutions

thienlnam · 2023-06-13T21:58:07Z

We're run into a similar situation where we're navigating to a page and the onyx data hasn't been set yet. Creating a new room and immediately navigating to it.

What we ended up doing was using onyx.set for the items we needed for navigation bc it was much faster, would that be possible here as well?

mountiny · 2023-06-14T16:05:01Z

Is what Jack proposed possible here?

Otherwise I feel like if new command works for this then it could be a way to go. It matches the 1:1:1 strategy

luacmartins · 2023-06-14T18:17:54Z

Now that I'm thinking more about it, I'm not sure if the issue @rushatgabhane is seeing is a race condition. I'll try to reproduce it on my end to investigate.

luacmartins · 2023-06-14T18:29:48Z

So it does seem to be an Onyx race condition. I updated updateGeneralSettings to use SET. @rushatgabhane can you test the changes and see if you can still reproduce the issue?

rushatgabhane

LGTM

cc: @mountiny

melvin-bot · 2023-06-16T02:29:18Z

We did not find an internal engineer to review this PR, trying to assign a random engineer to #16744... Please reach out for help on Slack if no one gets assigned!

mountiny

One change requested, otherwise LGTM

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

luacmartins · 2023-06-16T15:14:20Z

@mountiny updated!

thienlnam

Thanks, sending it!

thienlnam · 2023-06-16T18:07:49Z

Jk, can't need Vit to re-approve

mountiny

Thanks everyone!

OSBotify · 2023-06-19T14:59:00Z

✋ 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 · 2023-06-21T02:27:16Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.30-0 🚀

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

…VBBA" This reverts commit 158c34b, reversing changes made to 057a182.

OSBotify · 2023-06-22T16:54:54Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.30-5 🚀

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

fedirjh · 2023-06-30T06:01:53Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

+        if (this.state.shouldShowContinueSetupButton) {
+            return (
+                <ContinueBankAccountSetup
+                    reimbursementAccount={this.props.reimbursementAccount}
+                    continue={this.continue}
+                    policyName={policyName}
+                />


cc @luacmartins, This block is duplicated now, check line 384 ,we intentionally changed the order of rendering in this PR #20406.

ah nice catch. Do we have a PR to fix this already?

No we don't have a PR yet, needed to confirm before moving forward. I will create a PR.

PR is ready : #22253

fedirjh · 2023-07-11T08:33:20Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

-                    <Text>{this.props.translate('bankAccount.hasBeenThrottledError')}</Text>
-                </View>
-            );
+        const hasUnsupportedCurrency = lodashGet(this.props.policy, 'outputCurrency', '') !== CONST.CURRENCY.USD;


This line caused an edge case regression, when the policy is empty (deleted from other device) it will display the CurrencyError, we fixed that by displaying FullPageNotFoundView when policy is deleted.

rushatgabhane · 2024-04-25T05:49:55Z

src/libs/actions/Policy.js

@@ -512,9 +512,12 @@ function clearAvatarErrors(policyID) {
 function updateGeneralSettings(policyID, name, currency) {
    const optimisticData = [
        {
-            onyxMethod: Onyx.METHOD.MERGE,
+            // We use SET because it's faster than merge and avoids a race condition when setting the currency and navigating the user to the Bank account page in confirmCurrencyChangeAndHideModal
+            onyxMethod: Onyx.METHOD.SET,


Coming from #37254 (comment)

Because we're using Onyx.set, we should have spread the old pendingFields.

ask to change currency to USD

109421e

luacmartins self-assigned this May 25, 2023

luacmartins added 11 commits May 30, 2023 10:18

resolve conflicts

dfd7ab4

create WorkspaceUpdateCurrencyModal

4ed31b8

add state, callback methods

742936a

rename method

852597f

redirect to workspace page

ea7b72b

add error message

cac93b3

update translate

d408b8c

fix callback type

6e3b32c

update copy path

f091adf

rm WorkspaceUpdateCurrencyModal

c31bed2

fix style

f5fe5f9

luacmartins mentioned this pull request May 30, 2023

Workspace - Inconsistency between workspace default currency and explanation message below #16744

Closed

6 tasks

luacmartins added 7 commits June 7, 2023 12:48

Merge branch 'main' into cmartins-blockNonUSDVBBA

339785a

resolve conflicts

d9d0f0b

update modal copy

0b49f6c

update error copy

e8af5b0

use more explicit copy

1841745

fix typo

23e5c97

fix style

96dee89

luacmartins marked this pull request as ready for review June 8, 2023 19:51

luacmartins requested a review from a team as a code owner June 8, 2023 19:51

melvin-bot bot requested review from rushatgabhane and thienlnam and removed request for a team June 8, 2023 19:51

mountiny self-requested a review June 12, 2023 15:36

use onyx set

d3d6256

luacmartins requested a review from rushatgabhane June 15, 2023 19:44

rushatgabhane approved these changes Jun 16, 2023

View reviewed changes

mountiny requested changes Jun 16, 2023

View reviewed changes

src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated Show resolved Hide resolved

add correct error

e3bc7d9

luacmartins requested a review from mountiny June 16, 2023 15:13

thienlnam approved these changes Jun 16, 2023

View reviewed changes

mountiny approved these changes Jun 19, 2023

View reviewed changes

mountiny merged commit 158c34b into main Jun 19, 2023

mountiny deleted the cmartins-blockNonUSDVBBA branch June 19, 2023 14:54

OSBotify mentioned this pull request Jun 21, 2023

Deploy Checklist: New Expensify 2023-06-21 #21172

Closed

67 tasks

Li357 pushed a commit that referenced this pull request Jun 22, 2023

Revert "Merge pull request #19638 from Expensify/cmartins-blockNonUSD…

97a78cf

…VBBA" This reverts commit 158c34b, reversing changes made to 057a182.

dhairyasenjaliya referenced this pull request Jun 30, 2023

Merge branch 'main' into offlineIndicator

ede530b

fedirjh reviewed Jun 30, 2023

View reviewed changes

fedirjh mentioned this pull request Jul 5, 2023

[Fix] Remove dupe code inside reimbursement account page #22253

Merged

57 tasks

fedirjh reviewed Jul 11, 2023

View reviewed changes

fedirjh mentioned this pull request Jul 11, 2023

[HOLD for payment 2023-07-10] [$1000] Deleting workspace in connect bank account provides update currency error #21604

Closed

6 tasks

rushatgabhane reviewed Apr 25, 2024

View reviewed changes

rushatgabhane mentioned this pull request Apr 25, 2024

[HOLD for payment 2024-04-05] [$500] Workspace - If title is changed offline, the description is no longer greyed out #37254

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ask non-USD Workspaces to update currency before adding VBBA #19638

Ask non-USD Workspaces to update currency before adding VBBA #19638

luacmartins commented May 25, 2023 •

edited by kok-hong

Loading

melvin-bot bot commented Jun 8, 2023

mountiny commented Jun 12, 2023

rushatgabhane commented Jun 12, 2023

luacmartins commented Jun 13, 2023 •

edited

Loading

thienlnam commented Jun 13, 2023

mountiny commented Jun 14, 2023

luacmartins commented Jun 14, 2023

luacmartins commented Jun 14, 2023

rushatgabhane left a comment •

edited

Loading

melvin-bot bot commented Jun 16, 2023

mountiny left a comment

luacmartins commented Jun 16, 2023

thienlnam left a comment

thienlnam commented Jun 16, 2023

mountiny left a comment

OSBotify commented Jun 19, 2023

OSBotify commented Jun 21, 2023

OSBotify commented Jun 22, 2023

fedirjh Jun 30, 2023

luacmartins Jul 4, 2023 •

edited

Loading

fedirjh Jul 4, 2023

fedirjh Jul 5, 2023

fedirjh Jul 11, 2023

rushatgabhane Apr 25, 2024

Ask non-USD Workspaces to update currency before adding VBBA #19638

Ask non-USD Workspaces to update currency before adding VBBA #19638

Conversation

luacmartins commented May 25, 2023 • edited by kok-hong Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jun 8, 2023

mountiny commented Jun 12, 2023

rushatgabhane commented Jun 12, 2023

luacmartins commented Jun 13, 2023 • edited Loading

thienlnam commented Jun 13, 2023

mountiny commented Jun 14, 2023

luacmartins commented Jun 14, 2023

luacmartins commented Jun 14, 2023

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

melvin-bot bot commented Jun 16, 2023

mountiny left a comment

Choose a reason for hiding this comment

luacmartins commented Jun 16, 2023

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam commented Jun 16, 2023

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 19, 2023

OSBotify commented Jun 21, 2023

OSBotify commented Jun 22, 2023

fedirjh Jun 30, 2023

Choose a reason for hiding this comment

luacmartins Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

fedirjh Jul 4, 2023

Choose a reason for hiding this comment

fedirjh Jul 5, 2023

Choose a reason for hiding this comment

fedirjh Jul 11, 2023

Choose a reason for hiding this comment

rushatgabhane Apr 25, 2024

Choose a reason for hiding this comment

luacmartins commented May 25, 2023 •

edited by kok-hong

Loading

luacmartins commented Jun 13, 2023 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

luacmartins Jul 4, 2023 •

edited

Loading