Look at the platform to decide how to choose API by mountiny · Pull Request #12880 · Expensify/App

mountiny · 2022-11-19T12:06:44Z

Details

The shouldUseStaging variable will be false by default and in production App user does not have access to the toggle hence they cannot switch Prod app to use Staging API.

In case of Android and iOS, the Config file is not accessible in staging build (we use production env during building), therefore we need to rely only on the staging toggle in this case and that is why we add this platform specific check.

Fixed Issues

$ #11561

Tests

I have tested locally by building the app in simulator with env.staging and also with the .env.production. Then I used the toggle in settings (for staging) and confirmed the API went to staging servers

Internal tests

Copy the contents from the .env.production to your .env file
Run the iOS and Android app and it should show as Staging
Make sure the toggle for staging api is turned on
Performs some actions in the app
Go to logs and search for the most recent actions by this account
Make sure the servers these request hit are staging servers
Go back to the settings and switch to production API
Repeat and confirm in the logs the request are hitting the production servers
Run the app in dektop with staging env
Confirm the correct servers are hit based on the toggle value

Verify that no errors appear in the JS console

Offline tests

No offline tests required

QA Steps

Verify that no errors appear in the JS console

Open the iOS and Android staging app
Make sure the toggle for staging api is turned on
Performs some actions in the app
Go to logs and search for the most recent actions by this account
Make sure the servers these request hit are staging servers
Go back to the settings and switch to production API
Repeat and confirm in the logs the request are hitting the production servers
Run the staging web
By checkling the logs, confirm the correct servers are being hit based on what stage the toggle is at

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Cannot build web locally pointing to Staging, done in Desktop and worked fine. See below.

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Desktop Staging, you can see the time and the logs at the same time on the staging servers

Switching to production the logs confirm we are hitting production

iOS

Android

mountiny · 2022-11-21T16:46:09Z

Desktop Staging, you can see the time and the logs at the same time on the staging servers

Switching to production the logs confirm we are hitting production

melvin-bot · 2022-11-21T20:20:22Z

@parasharrajat @NikkiWines 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 · 2022-11-21T20:21:26Z

@roryabraham @AndrewGable @MariaHCD I have confirmed locally this works as expected. If you could give it a test as well that would be great. Thank you!

parasharrajat · 2022-11-21T20:25:41Z

+    const nativeStagingSwitcher = (platform === CONST.OS.ANDROID || platform === CONST.OS.IOS) && shouldUseStagingServer;
+    const webStagingSwitcher = CONFIG.IS_IN_STAGING && shouldUseStagingServer;


Maybe create a lib called shouldUseStagingServer with two files index.native.js and index.js. Then replace all of these with that lib.

If I am not correct, nativeStagingSwitcher is only used for native checks.

Interesting, I think this abstraction would be a bit premature, I can be persuaded the other way too, but at the moment his precise logic is used only here.

We discourage platform checks so I still think it is fine to create a small lib but I am fine with this. It might have one issue, let me check.

@parasharrajat What issue did you have in mind?

On, Mobile web nativeStagingSwitcher will be true and thus webStagingSwitcher will never be evaluated.

On, Mobile web nativeStagingSwitcher will be true and thus webStagingSwitcher will never be evaluated.

So lets have a look into this:

if you are in staging web, you want to decide which API to use based on the toggle

That will be achieved in this case too since we will in the end decide based in the shouldUseStagingServer value

in production the nativeStagingSwitcher will be false because the shouldUseStagingServer is always false there as no toggle is shown to change that

that means we would have to consider webStagingSwitcher in production otto and that will be false in mobile web since we can read the config so CONFIG.IS_IN_STAGING is false

I think we are fine then, do you agree @parasharrajat ?

This logic is very confusing to me so I'd have to agree with moving it to separate modules to make this easier to read.

Also I can't help but mention that fixing our staging build so that it actually uses the staging .env is the preferable fix. I'm not sure if there's anything blocking that but it might be worth considering dropping this change and just working towards that fix instead

@arosiclair

Also I can't help but mention that fixing our staging build so that it actually uses the staging .env is the preferable fix. I'm not sure if there's anything blocking that but it might be worth considering dropping this change and just working towards that fix instead

This has been considered as a first thing but the way how we deploy is normal and standard procedure. We build just once, for staging using the production .env. This way when we run all the test on this build, fix all the issues, we then just put this build to production. We dont have to build again and risk some odd quirks when building the production and essentially giving our users untested app.

This logic is very confusing to me so I'd have to agree with moving it to separate modules to make this easier to read.

I can add more comments to this, just not sure if adding this to a module of its own will help with clarity. Will additional comments be good enough in your opinion @arosiclair ?

Thank you for the review 🙇

I was about to write the same thing.

nativeStagingSwitcher should only be used for native all the time.

webStagingSwitcher should only be used for web | Desktop all the time.

A easier cleanup would be.

Create a lib shouldUseStagingServer.

//index.native.js function shouldUseStagingServer (shouldUseStagingServer){ return shouldUseStagingServer; }

//index.js function shouldUseStagingServer (shouldUseStagingServer){ return CONFIG.IS_IN_STAGING && shouldUseStagingServer; }

@parasharrajat Thank you!

Updated with this solution, can you please give this a test?

parasharrajat · 2022-11-22T13:45:01Z

Testing this now.

parasharrajat · 2022-11-22T14:42:43Z

Localhost is giving CORS errors on the web. I think that is fine.

mountiny · 2022-11-22T15:10:47Z

Localhost is giving CORS errors on the web. I think that is fine.

Yes that is expected, staging env should work in desktop though.

@parasharrajat how do you normally test the web when you have App PRs which need to point to staging?

parasharrajat · 2022-11-22T16:00:51Z

I use the desktop build to verify.

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

…fy/App into vit-experiementWithStagingNative

mountiny · 2022-11-24T15:57:08Z

@parasharrajat Thanks for the testing! Updated and fixed.

parasharrajat

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:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and 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 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

LGTM.

🎀 👀 🎀 C+ reviewed

parasharrajat · 2022-11-24T16:58:21Z

Not sure whom to tag. Anyone can merge.

melvin-bot · 2022-11-24T17:23:00Z

Triggered auto assignment to @grgia (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

OSBotify · 2022-11-24T17:27: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.

luacmartins · 2022-11-24T18:52:41Z

I think this PR is causing issues. Requests made on dev seem to be going straight to staging. When I try to log in with a dev account the request fails (and goes to staging). Using a staging account works fine.

ios.mov

luacmartins · 2022-11-24T19:14:21Z

Yep, confirmed that reverting this PR resolves the issue. I think the problem is that for native we rely on the value of stagingServerToggleState alone and we default it to true here, but we don't have this key available until after we sign in and have turned the Use staging server switch on/off

luacmartins · 2022-11-24T19:24:48Z

I'm gonna revert this now since it's preventing us from investigating other deploy blockers.

parasharrajat · 2022-11-24T20:42:26Z

Aha, I missed that.

luacmartins · 2022-11-24T21:06:30Z

Understandably so since you can't access the dev API.

mountiny · 2022-11-24T23:08:58Z

Sorry for not catching this after one of the refactors, a new PR is here #13029 still WIP though

mountiny · 2022-11-25T20:30:41Z

This was reverted no need for QA

OSBotify · 2022-11-25T21:34:15Z

🚀 Deployed to staging by @luacmartins in version: 1.2.32-0 🚀

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

OSBotify · 2022-11-28T21:18:10Z

🚀 Deployed to production by @luacmartins in version: 1.2.32-2 🚀

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

OSBotify · 2022-11-28T21:40:31Z

🚀 Deployed to production by @luacmartins in version: 1.2.32-2 🚀

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

Look at the app platform to decide how to choose api

649d4ad

mountiny requested review from AndrewGable, MariaHCD, parasharrajat and roryabraham November 19, 2022 12:06

mountiny self-assigned this Nov 19, 2022

mountiny added 2 commits November 19, 2022 12:07

Get operating system just once

dccda4f

Move the platform to global

ec71c97

mountiny mentioned this pull request Nov 21, 2022

using environment variable set by Environment.js for ios/android #12546

Closed

92 tasks

Merge branch 'main' into vit-experiementWithStagingNative

50bb98d

mountiny added the InternalQA This pull request required internal QA label Nov 21, 2022

mountiny removed the request for review from parasharrajat November 21, 2022 20:19

mountiny marked this pull request as ready for review November 21, 2022 20:19

mountiny requested a review from a team as a code owner November 21, 2022 20:19

melvin-bot Bot requested review from NikkiWines and parasharrajat and removed request for a team November 21, 2022 20:20

parasharrajat reviewed Nov 21, 2022

View reviewed changes

mountiny added 2 commits November 22, 2022 23:51

Merge branch 'main' into vit-experiementWithStagingNative

c90f3dd

Use library

94ef875

mountiny requested a review from parasharrajat November 23, 2022 00:06

parasharrajat suggested changes Nov 23, 2022

View reviewed changes

Comment thread src/libs/shouldUseStagingServer/index.native.js

Update src/libs/shouldUseStagingServer/index.native.js

3c40e03

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

mountiny added 2 commits November 24, 2022 13:40

Remove the platform

fa09886

Merge branch 'vit-experiementWithStagingNative' of github.com:Expensi…

fa2a066

…fy/App into vit-experiementWithStagingNative

mountiny dismissed arosiclair’s stale review via fa2a066 November 24, 2022 13:40

Fix lint

99424cd

mountiny requested review from arosiclair and parasharrajat November 24, 2022 15:54

parasharrajat approved these changes Nov 24, 2022

View reviewed changes

luacmartins approved these changes Nov 24, 2022

View reviewed changes

luacmartins merged commit 585816c into main Nov 24, 2022

luacmartins deleted the vit-experiementWithStagingNative branch November 24, 2022 17:22

melvin-bot Bot assigned grgia Nov 24, 2022

luacmartins mentioned this pull request Nov 24, 2022

[No QA] Revert "Look at the platform to decide how to choose API" #13013

Merged

47 tasks

mountiny mentioned this pull request Nov 24, 2022

Enable native staging apps to use Staging API #13029

Merged

98 tasks

OSBotify mentioned this pull request Nov 25, 2022

Deploy Checklist: New Expensify 2022-11-25 #13059

Closed

21 tasks

melvin-bot Bot mentioned this pull request Nov 28, 2022

[HOLD for payment 2022-12-20] [$500] Allow native app releases to take staging env variables into consideration #11561

Closed

marcochavezf mentioned this pull request Dec 23, 2022

[HOLD for payment 2022-12-20] Android/IOS- The flow to the transaction amount fields does not work in apps #11068

Closed

		const nativeStagingSwitcher = (platform === CONST.OS.ANDROID \|\| platform === CONST.OS.IOS) && shouldUseStagingServer;
		const webStagingSwitcher = CONFIG.IS_IN_STAGING && shouldUseStagingServer;

Uh oh!

Conversation

mountiny commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

mountiny commented Nov 21, 2022

Uh oh!

melvin-bot Bot commented Nov 21, 2022

Uh oh!

mountiny commented Nov 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Nov 22, 2022

Uh oh!

parasharrajat commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mountiny commented Nov 22, 2022

Uh oh!

parasharrajat commented Nov 22, 2022

Uh oh!

Uh oh!

mountiny commented Nov 24, 2022

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

Uh oh!

parasharrajat commented Nov 24, 2022

Uh oh!

melvin-bot Bot commented Nov 24, 2022

Uh oh!

OSBotify commented Nov 24, 2022

Uh oh!

luacmartins commented Nov 24, 2022

Uh oh!

luacmartins commented Nov 24, 2022

Uh oh!

luacmartins commented Nov 24, 2022

Uh oh!

parasharrajat commented Nov 24, 2022

Uh oh!

luacmartins commented Nov 24, 2022

Uh oh!

mountiny commented Nov 24, 2022

Uh oh!

mountiny commented Nov 25, 2022

mountiny commented Nov 19, 2022 •

edited

Loading

parasharrajat commented Nov 22, 2022 •

edited

Loading