Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Staging API] To Use Staging Server or not to use staging server #15517

Merged
merged 10 commits into from
Mar 6, 2023
Merged

[Staging API] To Use Staging Server or not to use staging server #15517

merged 10 commits into from
Mar 6, 2023

Conversation

Prince-Mendiratta
Copy link
Contributor

@Prince-Mendiratta Prince-Mendiratta commented Feb 27, 2023

Details

With this PR, we are adding the ability to map all requests to production / staging API depending on the "Use staging server" toggle in user preferences. This change only affects the Development and Staging Environment.

Fixed Issues

$ #12315
PROPOSAL: #12315 (comment)

Tests

.env is not defined

  1. Login to ND.
  2. Go to Settings > Preferences > toggle the Use staging server toggle ON.
  3. Go to Settings > Payments > Add payment Method > Bank Account.
  4. Ensure that the Sandbox mode of Plaid is initiated. On Web and Desktop, this can be seen very clearly as the disclaimer is shown at the bottom. For native devices and mWeb, a good way to determine if we are in sandbox or not is the presence of the reCaptcha after selecting the bank institution. If it is present, then we are in production mode. If not, we are in sandbox mode. Another method to determine this on native and mWeb is that the flow redirects to the sandbox URL so it can be seen pretty clearly there as well.
  5. Continue to add account normally in sandbox mode with any details. Notice that the account is added successfully after completing the flow.
  6. Go to Settings > Preferences > toggle the Use staging server toggle OFF.
  7. Go to Settings > Payments > Add payment Method > Bank Account.
  8. Ensure that the Production mode of Plaid is initiated. Ways to confirm this are same as mentioned above.
  9. Exit out of the production plaid flow
  10. Open any chat.
  11. Add a new attachment. Upload the attachment. Open it.
  12. Ensure all action in step 11 happen seamlessly.
Expected Console Output

Screenshot_20230228_175752

.env is defined
This is basically to confirm that all the requests will be proxied to the server depending on the contents of the .env file. To test this, create a .env file with this content:

EXPENSIFY_URL=https://staging.expensify.com/
STAGING_EXPENSIFY_URL=https://staging.expensify.com/
STAGING_SECURE_EXPENSIFY_URL=https://staging-secure.expensify.com/
  1. Login to ND.
  2. Go to Settings > Preferences > toggle the Use staging server toggle ON.
  3. Go to Settings > Payments > Add payment Method > Bank Account.
  4. Ensure that the Sandbox mode of Plaid is initiated. Ways to confirm this are same as mentioned above.
  5. Continue to add account normally in sandbox mode with any details. Notice that the account is added successfully after completing the flow.
  6. Go to Settings > Preferences > toggle the Use staging server toggle OFF.
  7. Go to Settings > Payments > Add payment Method > Bank Account.
  8. Ensure that the Sandbox mode of Plaid is initiated. Ways to confirm this are same as mentioned above.
  9. Continue to add account normally in sandbox mode with any details. Notice that the account is added successfully after completing the flow.
Expected Console Output

Screenshot_20230228_175705

Note: There is a known issue in Android where when repeating the above tests will lead to an error. This has been reported in slack and will be tackled in a separate issue. The above tests should work well for all other platforms.

Note 2: You might see an "Internal React error: Attempted to capture" error while testing but that is being tackled in #13917 and is not related to current changes.

  • Verify that no errors appear in the JS console

Offline tests

N/A, doesn't affect the offline functionality, should behave the same as expected behaviour when offline.

QA Steps

This doesn't affect the production behaviour. For staging,

  1. Login to ND.
  2. Go to Settings > Preferences > toggle the Use staging server toggle ON.
  3. Go to Settings > Payments > Add payment Method > Bank Account.
  4. Ensure that the Sandbox mode of Plaid is initiated. On Web and Desktop, this can be seen very clearly as the disclaimer is shown at the bottom. For native devices and mWeb, a good way to determine if we are in sandbox or not is the presence of the reCaptcha after selecting the bank institution. If it is present, then we are in production mode. If not, we are in sandbox mode. Another method to determine this on native and mWeb is that the flow redirects to the sandbox URL so it can be seen pretty clearly there as well.
  5. Continue to add account normally in sandbox mode with any details. Notice that the account is added successfully after completing the flow.
  6. Go to Settings > Preferences > toggle the Use staging server toggle OFF.
  7. Go to Settings > Payments > Add payment Method > Bank Account.
  8. Ensure that the Production mode of Plaid is initiated. Ways to confirm this are same as mentioned above.
  9. Continue to add account normally in sandbox mode with any details. Notice that the account is added successfully after completing the flow.
  10. Open any chat.
  11. Add a new attachment. Upload the attachment. Open it.
  12. Ensure all action in step 11 happen seamlessly.
  • Verify that no errors appear in the JS console

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 / Chrome
    • iOS / native
    • iOS / 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 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 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 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 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 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 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

without .env defined

Web
2023-02-28.02-13-17.mp4
Mobile Web - Chrome
2023-02-28.02-37-30.mp4
Mobile Web - Safari
mWeb-Safari.mp4
Desktop
desktop.mp4
iOS
2023-02-28.02-06-56.mp4
Android
2023-02-28.02-35-08.mp4

.env defined as mentioned in above tests

Web
2023-02-28.18-05-34.mp4
Mobile Web - Chrome
2023-02-28.18-13-08.mp4
Mobile Web - Safari
2023-02-28.20-48-37.mp4
Desktop
2023-02-28.20-42-09.mp4
iOS
2023-02-28-20-32-23_3RJY0UXT.mp4
Android
2023-02-28.18-14-36.mp4

@Prince-Mendiratta Prince-Mendiratta requested a review from a team as a code owner February 27, 2023 21:25
@melvin-bot melvin-bot bot requested review from neil-marcellini and sobitneupane and removed request for a team February 27, 2023 21:25
@MelvinBot
Copy link

@neil-marcellini @sobitneupane 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]

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
web/proxy.js Outdated Show resolved Hide resolved
Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshots/Videos

Web
Screen.Recording.2023-02-28.at.17.01.51.mov
Mobile Web - Chrome
Screen.Recording.2023-02-28.at.17.17.44.mov
Mobile Web - Safari
Screen.Recording.2023-02-28.at.17.13.32.mov
Desktop
Screen.Recording.2023-02-28.at.17.08.43.mov
iOS
Screen.Recording.2023-02-28.at.17.24.10.mov
Android
Screen.Recording.2023-02-28.at.17.29.56.mov

@sobitneupane
Copy link
Contributor

@Prince-Mendiratta Can you please add screenshots for the condition ".env is defined" in Tests?

@Prince-Mendiratta
Copy link
Contributor Author

@sobitneupane Sure, will do so. Is it required for all platforms?

@sobitneupane
Copy link
Contributor

@Prince-Mendiratta I think so. Web and Native behave differently. So, it is good to have screenshots for all platforms.

@Prince-Mendiratta
Copy link
Contributor Author

Got it, I'll ping you once I've added the recordings.

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
@Prince-Mendiratta
Copy link
Contributor Author

@sobitneupane updated with tests!

Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The code tests well and it's looking pretty good. I would like to see a few tweaks to make the code a bit easier to understand.

Also, I don't think we want to ask people to add a bank account with Plaid on production for testing. I think it should be enough to see that Plaid links to the production environment. Adding a bank account in the sandbox is a good idea however. I'm curious, what credentials do you use to test in the Sandbox mode?

src/libs/HttpUtils.js Outdated Show resolved Hide resolved
src/libs/HttpUtils.js Outdated Show resolved Hide resolved
web/proxy.js Outdated Show resolved Hide resolved
@neil-marcellini
Copy link
Contributor

cc @iwiznia @mountiny in case you guys want to take a look since you participated in the Slack conversation.

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
@sobitneupane
Copy link
Contributor

.env.staging file has the following content:

NEW_EXPENSIFY_URL=https://staging.new.expensify.com/
SECURE_EXPENSIFY_URL=https://secure.expensify.com/
EXPENSIFY_URL=https://www.expensify.com/
EXPENSIFY_PARTNER_NAME=chat-expensify-com
EXPENSIFY_PARTNER_PASSWORD=e21965746fd75f82bb66
PUSHER_APP_KEY=268df511a204fbb60884
USE_WEB_PROXY=false
ENVIRONMENT=staging
SEND_CRASH_REPORTS=true

We are expecting to have EXPENSIFY_URL, STAGING_EXPENSIFY_URL , STAGING_SECURE_EXPENSIFY_URL to be set on .env.staging. I am little confused why we don't have STAGING_EXPENSIFY_URL and STAGING_SECURE_EXPENSIFY_URL on .env.staging as of now.

EXPENSIFY_URL=https://staging.expensify.com/
STAGING_EXPENSIFY_URL=https://staging.expensify.com/
STAGING_SECURE_EXPENSIFY_URL=https://staging-secure.expensify.com/

cc: @neil-marcellini

@Prince-Mendiratta
Copy link
Contributor Author

Prince-Mendiratta commented Mar 1, 2023

@sobitneupane As far as I understand, the NEW_EXPENSIFY_URL is the website's URL and the EXPENSIFY_URL is the URL used to hit the API. On staging, all requests are sent to the prod server by default.

As for the staging and secure staging URL, even if they are not defined in the staging .env, they are automatically set with lodash with this configuration -

App/src/CONFIG.js

Lines 16 to 17 in 44c19cb

const stagingExpensifyURL = Url.addTrailingForwardSlash(lodashGet(Config, 'STAGING_EXPENSIFY_URL', 'https://staging.expensify.com/'));
const stagingSecureExpensifyUrl = Url.addTrailingForwardSlash(lodashGet(Config, 'STAGING_SECURE_EXPENSIFY_URL', 'https://staging-secure.expensify.com/'));

We only need to set the .env file if the contributor specifically needs to set different production and staging URLs, as would be done by internal employees in this manner:

EXPENSIFY_URL=https://www.expensify.com.dev/
STAGING_EXPENSIFY_URL=https://staging.expensify.com.dev/
STAGING_SECURE_EXPENSIFY_URL=https://staging-secure.expensify.com.dev/

@sobitneupane
Copy link
Contributor

@Prince-Mendiratta Thanks. I was just looking for confirmation from CME.

@sobitneupane
Copy link
Contributor

@neil-marcellini

I'm curious, what credentials do you use to test in the Sandbox mode?

username: user_good
password: pass_good

@mountiny
Copy link
Contributor

mountiny commented Mar 1, 2023

Oof, I think we now how 3 PRs touching a similar code #14944 and #15178

cc @kidroca @MonilBhavsar would you want to review this one too?

Copy link
Contributor

@kidroca kidroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some notes

src/CONFIG.js Outdated Show resolved Hide resolved
src/libs/HttpUtils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really close to being ready. I agree with @kidroca's comment about moving the api map constant.

src/CONFIG.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with kidroca comment, had one other question

src/libs/HttpUtils.js Outdated Show resolved Hide resolved
web/proxy.js Outdated Show resolved Hide resolved
@Prince-Mendiratta
Copy link
Contributor Author

@neil-marcellini I tried to generate a production build for web and used serve dist to run it locally but I kept getting the CORS error, any guide on how to test production build locally?

Also, is it required to test the production build for all platforms?

mountiny
mountiny previously approved these changes Mar 2, 2023
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although always anxious about these changes, gotta make sure to test everything really well after deploy (shouldnt be an issue, but we had in past let regressions slip because we did not test properly and it was not easy to spot)

@Prince-Mendiratta
Copy link
Contributor Author

I tested the production builds for android, iOS, desktop and it works well. Since the staging toggle is not visible, it doesn't affect the functionality and does not break anything.

I tried out the staging build too for the above platforms and it was working well as expected including the tests mentioned above.

cc @neil-marcellini @mountiny

Copy link
Contributor

@kidroca kidroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some notes on the regex usage

I lean towards NAB, but perhaps it would be simpler if we don't use regex

web/proxy.js Outdated Show resolved Hide resolved
web/proxy.js Outdated Show resolved Hide resolved
Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>
@Prince-Mendiratta
Copy link
Contributor Author

gentle bump @neil-marcellini

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Prince-Mendiratta @kidroca I will go ahead and merge this since @neil-marcellini already gave his approval before.

@mountiny mountiny merged commit f95292a into Expensify:main Mar 6, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

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

@mountiny
Copy link
Contributor

mountiny commented Mar 6, 2023

image

gonna have to revert this as its breaking internal dev setup

@Prince-Mendiratta
Copy link
Contributor Author

@mountiny can you please tell the issue? Is it because of the requests not hitting the .dev domain?

@mountiny
Copy link
Contributor

mountiny commented Mar 6, 2023

@Prince-Mendiratta correct, for internal engineers, the requests should be hitting local dev VM, this is my env:

NEW_EXPENSIFY_URL=https://new.expensify.com/
SECURE_EXPENSIFY_URL=https://secure.expensify.com.dev/
EXPENSIFY_URL=https://www.expensify.com.dev/
EXPENSIFY_PARTNER_NAME=chat-expensify-com
EXPENSIFY_PARTNER_PASSWORD=removed
PUSHER_APP_KEY=removed
SECURE_NGROK_URL=removed
NGROK_URL=removed
USE_NGROK=true
USE_WEB_PROXY=false
PUSHER_DEV_SUFFIX=removed

@Prince-Mendiratta
Copy link
Contributor Author

@mountiny You'll have to modify the .env slightly to include this case too:

STAGING_EXPENSIFY_URL=https://staging.expensify.com.dev/
STAGING_SECURE_EXPENSIFY_URL=https://staging-secure.expensify.com.dev/

Can you please try with these and see if the issue is resolved?

Thanks.

@Prince-Mendiratta
Copy link
Contributor Author

My analysis based on the limited information from the above screenshot, looks like the local storage has the shouldUseStagingServer toggled on and the login request is hitting the staging API. Since the .env is not configured to match .dev domains, it's hitting the actual staging API and thus, the error.

As per a summary of the slack discussion, we should send in a disclaimer for internal employees to update their .env files to reflect the changes with this PR.
We should also potentially update the example .env files in the repo.

@mountiny
Copy link
Contributor

mountiny commented Mar 6, 2023

image

no user key in the onyx so that wont be the issue

@Prince-Mendiratta Can we achieve this without everyone needing to change their local env?

@Prince-Mendiratta
Copy link
Contributor Author

no user key in the onyx so that wont be the issue

I see, I wonder why the app is hitting the staging API then. 🤔

@Prince-Mendiratta Can we achieve this without everyone needing to change their local env?

@mountiny as we discussed in the slack thread that the easiest way to implement this would be to let the .env determine which endpoint to hit, do we want to revisit that decision?

@mountiny
Copy link
Contributor

mountiny commented Mar 6, 2023

@Prince-Mendiratta it does not work even with the updated env.

In a call now, can you please create a thread in Slack to discuss this/test this easier :) thanks 🙇

Comment on lines 10 to 12

// Desktop and web use staging config too so we we should default to staging API endpoint if on those platforms
const shouldDefaultToStaging = _.contains([CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP], getPlatform());
Copy link
Contributor

@kidroca kidroca Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the reason you're hitting staging @mountiny

The default should always be false now be false unless we're on staging

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

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

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

@mvtglobally
Copy link

Has this PR been reverted? Can we check it off? #15678

@neil-marcellini
Copy link
Contributor

@mvtglobally yes it was reverted here #15678

@kidroca kidroca mentioned this pull request Mar 6, 2023
74 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Mar 7, 2023

🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.79-0 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants