Fix - Workspace - Currencies do not load after transition from OldDot when other user logged in ND #31789 #32245

AmjedNazzal · 2023-11-30T08:12:31Z

Details

We are adding a check for when the user uses short lived token while deeplinking from OD, this is to solve the issue where currencies don't load in these cases.

As per the discussion in the PR, we have come to the conclusion that this bug is not reproducible on Android/iOS native in any typical workflow. This is evident in the component App.ts, where the beginDeepLinkRedirect function explicitly calls Browser.openRouteInDesktopApp. This suggests that there is no natural workflow where deep link redirection could result in the URL opening in the Android/iOS native app.

Fixed Issues

$ #31789
PROPOSAL: #31789 (comment)

Tests

For web/mweb:

Login to OD from with account A
Login to ND from account B
From OD go to Settings > Workspaces and click on a workspace
Wait until the browser address bar gets populated with the url and stop the loading
Replace new.expensify with dev.new.expensify.com:8082 in the address bar url
Once the app loads and the workspace settings are mounted, check that currencies have loaded

For MacOS Desktop:

Follow the same steps as web for steps 1-3
Once ND loads, it will prompt you to open new expensify in your local Electron build
Accept the prompt and wait until the workspace settings load in your desktop app

Verify that no errors appear in the JS console

Offline tests

No offline test

QA Steps

Login to OldDot (staging) from with account A
Login to NewDot (staging) from account B
From OldDot, go to Settings > Workspaces and click on a workspace
Once the app loads and the workspace settings are mounted, check that currencies have loaded

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

N/A

Android: mWeb Chrome

mweb-chrome.mov

iOS: Native

N/A

iOS: mWeb Safari

mweb-safari.2.mov

MacOS: Chrome / Safari

web-chrome.mov

MacOS: Desktop

mac-desktop.mov

melvin-bot · 2023-11-30T08:12:36Z

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

AmjedNazzal · 2023-11-30T08:16:18Z

Note: I have considered the use of union type as suggested but upon looking into it, changing authToken into a union type will mean that there will be changes needed accross multiple areas of the app because the use of authToken would change and typescript will throw errors otherwise. So this made me wonder, is it worth making changes in the current code and future code whenever authToken is used just for the sake of this somewhat edgecase localized issue we are trying to solve? let me know your thoughts on this.

cubuspl42 · 2023-11-30T12:30:21Z

Note: I have considered the use of union type as suggested but upon looking into it, changing authToken into a union type will mean that there will be changes needed accross multiple areas of the app because the use of authToken would change and typescript will throw errors otherwise. So this made me wonder, is it worth making changes in the current code and future code whenever authToken is used just for the sake of this somewhat edgecase localized issue we are trying to solve? let me know your thoughts on this.

I don't think this union was a good idea anymore. You're right, let's keep this as is.

src/libs/actions/Session/index.ts

cubuspl42 · 2023-12-01T13:46:14Z

Relating to QA Steps.

First, use "OldDot" and "NewDot", don't confuse new people in the project too much (these terms are already cryptic for newcomers)

Wait until the browser address bar gets populated with the url and stop the loading then replace new.expensify with staging.new.expensify

It doesn't seem to be necessary: (video)

Replace "OD" with "OldDot (staging)" in the beginning and we should be good

AmjedNazzal · 2023-12-01T13:50:25Z

On it, I will fix that QA and fix a lint issue right away

cubuspl42 · 2023-12-01T14:43:29Z

In "Tests", would you specify what exact steps did you follow on non-Web platforms? Including any tricks and shell commands.

AmjedNazzal · 2023-12-01T14:59:31Z

@cubuspl42 I've added steps for other platforms, let me know how it looks

cubuspl42 · 2023-12-04T11:52:07Z

I have this issue:

launching-loop.mp4

AmjedNazzal · 2023-12-04T12:04:28Z

@cubuspl42 from the video it seem like you are getting an API error trying to open the workspace from OD before you even try using it on the native app correct? are you sure it's related to our PR here? because in the video it seem like the issue is happening on your end between OD and ND production?

AmjedNazzal · 2023-12-05T06:55:34Z

Okay I managed to reproduce the issue you are having on my physical android device, but I have noticed the issue happens with or without the changes we made in this PR so it must be an Android issue that was always there, either way I will look into the cause and let you know what I find.

AmjedNazzal · 2023-12-05T09:51:06Z

@cubuspl42 This issue seem to be more about Android security features, I've been doing some tests and it seem like clicking a workspace in OD, whether it's from OD app or website, will not redirect to the native app in the normal natural flow because clicking it will always open ND in a browser so do we really need to worry about this? is there any case at all where clicking workspace from OD would open up the native app?

cubuspl42 · 2023-12-05T10:48:21Z

I'm just trying to ensure we test the Android platform properly!

is there any case at all where clicking workspace from OD would open up the native app?

I just investigated it. It shouldn't open directly.

https://new.expensify.com/.well-known/assetlinks.json

App/android/app/src/main/AndroidManifest.xml

Lines 58 to 74 in fe6a2cb

    
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/r"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/a"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/settings"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/details"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/v"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/bank-account"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/iou"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/request"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/enable-payments"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/statements"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/concierge"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/split"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/request"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/new"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/search"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/send"/> 
        
           <data android:scheme="https" android:host="new.expensify.com" android:pathPrefix="/money2020"/>

cubuspl42 · 2023-12-05T10:49:15Z

from the video it seem like you are getting an API error trying to open the workspace from OD

It's not an API error, I think, I just aborted all requests as soon as possible.

AmjedNazzal · 2023-12-05T10:55:22Z

@cubuspl42 Got you, what would you like to do about this? do I dive into this and investigate why Android is rejecting signInWithShortLivedAuthToken? or do you think since it wouldn't normally open in the native app we shouldn't worry about this?

cubuspl42 · 2023-12-05T10:59:03Z

I'm just trying to ensure we properly test the Android flow, so no regressions arise. I don't have any greater ambitions here.

How did you figure out that the exact form of the new-expensify:/ link? I know that it has the same form as the URL handled by the NewDot web app, but it wasn't guarnteed.

AmjedNazzal · 2023-12-05T11:02:49Z

@cubuspl42 I actually didn't figure it out myself, that was something I found out in the slack channel where a c+ mentioned this link, if you want I can link you that discussion

cubuspl42 · 2023-12-05T11:28:10Z

Sure, share it please

AmjedNazzal · 2023-12-05T11:31:49Z

https://expensify.slack.com/archives/C01GTK53T8Q/p1698849973538749?thread_ts=1698848483.535709&cid=C01GTK53T8Q

cubuspl42 · 2023-12-07T11:32:03Z

I tried inspecting the console / network logs for new.expensify.com/transition (the production flow) on mobile Chrome on ANdroid, and the browser does not attempt to launch a new-expensify://... URI.

Chrome on macOS does, though. The link has different form from what we were testing.

Conclusions: I don't think our Android test is actually testing anything. It this flow ever supposed to be handled by the mobile app? Have you checked this on prod (prod OldDot, prod NewDot, prod Google Play Android app)?

AmjedNazzal · 2023-12-07T11:59:42Z

@cubuspl42 I actually did test and I could not get the deeplinking from OD to ever open ND, I tried production apps both OD and ND and OD app will always open up the browser, same with OD in the browser, it never attempts to launch the native app, so you are correct in saying that the android test is actually testing nothing because it doesn't seem like this issue we are working on would ever occur in native apps anyway unless we make changes that promts OD links to open in the native ND, which I don't know if it will ever happen since commonly apps will open links in the browser anyway.

AmjedNazzal · 2023-12-10T12:25:14Z

@cubuspl42 any updates? should we remove the native tests and state the bug/fix is not applicable to native app? or should we ask on slack?

cubuspl42 · 2023-12-11T15:42:04Z

Let's add "N/A" in the Android / iOS "Screenshots/Videos" sections and add a short description of our findings in the "Details" section. I'll proceed with testing in this manner.

cubuspl42 · 2023-12-11T15:46:30Z

This is an important line:

App/src/libs/Browser/index.website.ts

Line 77 in 820fb32

    
           const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`;

It's a desktop-only logic, which might suggest we're observing the expected behavior

AmjedNazzal · 2023-12-11T16:21:49Z

This is an important line:

App/src/libs/Browser/index.website.ts

Line 77 in 820fb32

const expensifyDeeplinkUrl = `${CONST.DEEPLINK_BASE_URL}${expensifyUrl.host}/transition?${params.toString()}`;

It's a desktop-only logic, which might suggest we're observing the expected behavior

You are correct, I've looked at index.website.ts and the surrounding components and it does appear like in beginDeepLinkRedirect in App.ts, it only calls for opening the URL in desktop app and nothing for Android/iOS native app. I will explain all this in the "Details" section like suggested.

AmjedNazzal · 2023-12-11T16:31:31Z

@cubuspl42 I have removed the test steps for Android/iOS and removed the video for them as well, also I've added a brief explanation in the Details section.

cubuspl42 · 2023-12-11T19:55:36Z

@cubuspl42 I have removed the test steps for Android/iOS and removed the video for them as well, also I've added a brief explanation in the Details section.

Thank you very much! I'll get back to this tomorrow. I had some high-priority things in my queue today, and it's already evening here.

AmjedNazzal · 2023-12-11T21:21:21Z

@cubuspl42 When you have time to look at this again, since you have a physical iphone from what I remember, can you please test opening the URL from iCloud notes? I've seen this issue post about iCloud notes and opening URLs from there into the iOS app and it made me a little bit worried since I've stated there is no natural workflow for deep linking into the iOS app but maybe there is? Thank you!

cubuspl42 · 2023-12-12T13:53:05Z

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 tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / 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 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- 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.js 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(themeColors.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 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.
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

Web

old-dot-transition-web.mp4

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

old-dot-transition-desktop.mp4

iOS

N/A

Android

N/A

cubuspl42 · 2023-12-12T14:00:46Z

@francoisl

The flow we're modifying isn't expected to be handled on mobile native variants of Expensify (iOS / Android). There are no defined deep links for this functionality.

src/libs/actions/Session/index.ts

AmjedNazzal · 2023-12-13T11:20:10Z

@francoisl @cubuspl42 The code now resets with null instead of undefined, I've tested and it seem to be behaving like expected.

OSBotify · 2023-12-14T22:49:38Z

✋ 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-12-19T02:43:03Z

🚀 Deployed to staging by https://github.com/francoisl in version: 1.4.14-0 🚀

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

OSBotify · 2023-12-19T03:46:24Z

🚀 Deployed to staging by https://github.com/francoisl in version: 1.4.14-0 🚀

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

OSBotify · 2023-12-21T05:13:59Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀

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

Fix for issue Expensify#31789

f93f34b

AmjedNazzal requested a review from a team as a code owner November 30, 2023 08:12

melvin-bot bot requested review from cubuspl42 and removed request for a team November 30, 2023 08:12