[HybridApp] Share auth token in HybridApp #48007

mateuuszzzzz · 2024-08-26T14:49:47Z

Details

TO DO:

Implement cache clearing (different PR handled by @war-in)

Fixed Issues

$ #46815
PROPOSAL:

Tests

Verify that transition works, it's not impacted by slow connection.
- When account changes, data on NewDot changes as well and previous data is not visible
NewDot on HybridApp can be used without any issues the same way as pure version of NewDot.

Offline tests

Verify that NewDot opens without issues when offline. (NewDot should have loading skeletons when this is a first transition to NewDot and we're already offline)

QA Steps

Same as tests and offline

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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.
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.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

…ybridapp

# Conflicts: # src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx

AndrewGable · 2024-09-03T21:38:31Z

Can we remove or suppress this log:

 LOG  Updating props for Surface "NewExpensify" with {"rootTag":1,"initialProps":{"url":"/[email protected]&authToken=

We cannot have authTokens in the logs.

AndrewGable

Overall code looks a lot cleaner than it was before 👍

@ntdiary - Feel free to review the code, no need to test.

mateuuszzzzz · 2024-09-04T08:31:40Z

Can we remove or suppress this log:
 LOG  Updating props for Surface "NewExpensify" with {"rootTag":1,"initialProps":{"url":"/[email protected]&authToken=
We cannot have authTokens in the logs.

It comes from AppRegistry.js. I'll introduce a HybridApp patch to hide real values of sensitive data

ntdiary · 2024-09-04T10:00:11Z

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: mWeb Chrome
- iOS: Native
- 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 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 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.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(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.
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: Native

N/A

Android: mWeb Chrome

N/A

iOS: Native

N/A

iOS: mWeb Safari

N/A

MacOS: Chrome / Safari

N/A

MacOS: Desktop

N/A

mateuuszzzzz · 2024-09-04T13:32:30Z

@AndrewGable logs should be redacted now:

Updating props for Surface "NewExpensify" with {"rootTag":1,"initialProps":{"url":"/[email protected]&authToken=<REDACTED>&accountID=16078163&autoGeneratedLogin=<REDACTED>&autoGeneratedPassword=<REDACTED>&clearOnyxOnStart=false"},"fabric":true}

ntdiary

LGTM! BTW, just curious, is the size of this latest patch file normal? I noticed it's much larger than the others. 😂

melvin-bot · 2024-09-04T13:48:18Z

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

mateuuszzzzz · 2024-09-04T14:01:42Z

LGTM! BTW, just curious, is the size of this latest patch file normal? I noticed it's much larger than the others. 😂

I also noticed this and I'm curious about it as well. I simply created a patch and it modified existing one 🤔
cc: @staszekscp @war-in
Let's hold on with merging until we confirm everything is fine here.

This reverts commit 1aab4f0.

This reverts commit 6ecbb5e.

war-in · 2024-09-04T15:16:50Z

@AndrewGable @ntdiary The patch should be fixed now :) the changes introduced by @mateuuszzzzz are here and the log behaves as expected 🎉

Updating props for Surface "NewExpensify" with {"rootTag":1,"initialProps":{"url":"/[email protected]&authToken=<REDACTED>&accountID=16530855&autoGeneratedLogin=<REDACTED>&autoGeneratedPassword=<REDACTED>&clearOnyxOnStart=true"},"fabric":true}

war-in · 2024-09-04T15:17:09Z

I think we're ready for merge 👀

…ybridapp [HybridApp] Share auth token in HybridApp (cherry picked from commit 217774b) (CP triggered by AndrewGable)

OSBotify · 2024-09-04T16:48:24Z

✋ 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 · 2024-09-04T17:19:48Z

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 9.0.29-5 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2024-09-05T19:17:02Z

🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.29-12 🚀

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

aimane-chnaif · 2024-11-06T00:10:08Z

src/pages/ErrorPage/SessionExpiredPage.tsx

+                            onPress={() => {
+                                if (!NativeModules.HybridAppModule) {
+                                    Session.clearSignInData();
+                                    Navigation.navigate();


This navigation does nothing - #51546
We fixed it by calling Navigation.goBack() instead.

eVoloshchak · 2024-12-10T12:07:50Z

src/libs/actions/Session/index.ts

+        Onyx.multiSet({
+            [ONYXKEYS.SESSION]: {email, authToken, accountID: Number(accountID)},
+            [ONYXKEYS.CREDENTIALS]: {autoGeneratedLogin, autoGeneratedPassword},
+        }).then(App.openApp);


Hey folks👋
This has caused an issue in #49964

Explanation from #52694

The problem was caused by openApp being called each time ND was opened in hybrid app. It downloads all information needed to open app for the first time. On subsequent start ups Onyx is filled with the data so reconnectApp should be called.

mateuuszzzzz added 3 commits August 26, 2024 14:05

Get more necessary session data from OldDot

7c84a1a

Get rid of custom BootSplash logic for HybridApp

140a84f

Remove invalid prop from HybridAppMiddleware

7f1c4e9

mateuuszzzzz mentioned this pull request Aug 26, 2024

[HybridApp] Setup separate HybridAppPublicScreens #47322

Closed

56 tasks

mateuuszzzzz and others added 25 commits August 30, 2024 10:29

Clean up code

3173707

clear onyx on new sign in

a9e3fef

Refactor code and temporary do not propagate encryptedAuthToken

8dbf093

pass shouldClearOnyxOnStart in url

fd8d79e

Change HybridApp BootSplash condition

938b2b7

Merge branch 'main' into share-token-in-hybridapp

9641969

fix signout button

09ac532

Merge branch 'war-in/clear-onyx-on-new-sign-in' into share-token-in-h…

f998be4

…ybridapp

Use App.openApp on transition

e68f007

Fix openApp

4c305a6

Add React import

3acf03e

Merge branch 'refs/heads/main' into share-token-in-hybridapp

46833c2

# Conflicts: # src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx

Improve cache clearing logic

c111894

add splashScreenStateContext

1e82645

fix lint

a3aa755

move ready to be hidden to InitialURLContextProvider

320de61

opened -> visible

01a93f0

remove unused useExitTo hook

3b5e1eb

Allow to navigate to routes different than transition

c47905b

Do not reset home on SignOut

2b772d0

fix initialUrl while OD -> ND

b052157

reintroduce onboarding

2e955e9

Fallback to SessionExpiredPage on HybridApp instead of SignUp page

620150b

Remove unused code

94f6727

Merge branch 'main' into share-token-in-hybridapp

1b1b474

mateuuszzzzz marked this pull request as ready for review September 3, 2024 15:51

AndrewGable reviewed Sep 3, 2024

View reviewed changes

rename BOOT_SPLASH_STATE values

ca8bfac

reset initial url on return to OD

9d43159

staszekscp mentioned this pull request Sep 4, 2024

[HybridApp] [$250] Update 3DTouch/ForceTouch menu so it's not broken when set to boot the NewDot experience on Hybrid #47469

Closed

6 tasks

mateuuszzzzz added 2 commits September 4, 2024 12:38

Change naming conventions

f650f9b

Redact sensitive data logs

6ecbb5e

ntdiary approved these changes Sep 4, 2024

View reviewed changes

Improve redacting

1aab4f0

war-in added 3 commits September 4, 2024 16:44

Revert "Improve redacting"

31085f9

This reverts commit 1aab4f0.

Revert "Redact sensitive data logs"

0e41e99

This reverts commit 6ecbb5e.

fix patch

f3d1d7f

AndrewGable approved these changes Sep 4, 2024

View reviewed changes

AndrewGable merged commit 217774b into Expensify:main Sep 4, 2024
15 checks passed

OSBotify pushed a commit that referenced this pull request Sep 4, 2024

Merge pull request #48007 from software-mansion-labs/share-token-in-h…

e815cb9

…ybridapp [HybridApp] Share auth token in HybridApp (cherry picked from commit 217774b) (CP triggered by AndrewGable)

github-actions bot added the CP Staging marks PRs that have been CP'd to staging label Sep 4, 2024

mallenexpensify mentioned this pull request Sep 6, 2024

[HybridApp] Users stuck at white screen during app open #46815

Closed

6 tasks

aimane-chnaif reviewed Nov 6, 2024

View reviewed changes

eVoloshchak reviewed Dec 10, 2024

View reviewed changes

eVoloshchak mentioned this pull request Dec 10, 2024

[HOLD for payment 2024-12-03] [$250] Hybrid app - Loading skeleton when notification tapped and app is killed state #49964

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HybridApp] Share auth token in HybridApp #48007

[HybridApp] Share auth token in HybridApp #48007

mateuuszzzzz commented Aug 26, 2024 •

edited

Loading

AndrewGable commented Sep 3, 2024

AndrewGable left a comment

mateuuszzzzz commented Sep 4, 2024 •

edited

Loading

ntdiary commented Sep 4, 2024 •

edited

Loading

mateuuszzzzz commented Sep 4, 2024 •

edited

Loading

ntdiary left a comment

melvin-bot bot commented Sep 4, 2024

mateuuszzzzz commented Sep 4, 2024

war-in commented Sep 4, 2024

war-in commented Sep 4, 2024

OSBotify commented Sep 4, 2024

OSBotify commented Sep 4, 2024

OSBotify commented Sep 5, 2024

aimane-chnaif Nov 6, 2024

eVoloshchak Dec 10, 2024

[HybridApp] Share auth token in HybridApp #48007

[HybridApp] Share auth token in HybridApp #48007

Conversation

mateuuszzzzz commented Aug 26, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

AndrewGable commented Sep 3, 2024

AndrewGable left a comment

Choose a reason for hiding this comment

mateuuszzzzz commented Sep 4, 2024 • edited Loading

ntdiary commented Sep 4, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

mateuuszzzzz commented Sep 4, 2024 • edited Loading

ntdiary left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 4, 2024

mateuuszzzzz commented Sep 4, 2024

war-in commented Sep 4, 2024

war-in commented Sep 4, 2024

OSBotify commented Sep 4, 2024

OSBotify commented Sep 4, 2024

OSBotify commented Sep 5, 2024

aimane-chnaif Nov 6, 2024

Choose a reason for hiding this comment

eVoloshchak Dec 10, 2024

Choose a reason for hiding this comment

mateuuszzzzz commented Aug 26, 2024 •

edited

Loading

mateuuszzzzz commented Sep 4, 2024 •

edited

Loading

ntdiary commented Sep 4, 2024 •

edited

Loading

mateuuszzzzz commented Sep 4, 2024 •

edited

Loading