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

Config URL Cleanup #7662

Merged
merged 11 commits into from
Feb 28, 2022
Merged

Config URL Cleanup #7662

merged 11 commits into from
Feb 28, 2022

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Feb 9, 2022

Details

Fixed Issues

$ #7489

Tests

  1. Tested that .env file changes load the correct environment
  2. Verified changes in the Readme
  3. Tested App Download link
  4. Google Api test done CORS origin from localhost:8080
  5. Plaid Test done CORS origin from localhost:8080
  • Verify that no errors appear in the JS console

QA Steps

Check correct URL is picked

  1. While PR changes configs and variables, one thing to check is that the App works as expected and all API requests are running to env specific APIs, ie, Staging connects to Staging URL, and so on

App Download

  1. Got the settings by clicking on Avatar on LHN
  2. Click on About -> App Download links menu
  3. Click on macOS download link, and it should download the correct bundle

Plaid Link

  1. Got the settings by clicking on Avatar on LHN
  2. Click on Payments -> Add Payment menu
  3. Click on "Bank Account" and ensure that Plaid flow is working fine
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2022-02-20 at 8 47 51 PM

Screenshot 2022-02-20 at 9 14 03 PM

Mobile Web

Screenshot 2022-02-20 at 8 49 55 PM

Desktop

Screenshot 2022-02-20 at 8 57 28 PM

iOS

Screenshot 2022-02-20 at 8 54 17 PM

Android

Screenshot 2022-02-20 at 8 58 30 PM

@mananjadhav
Copy link
Collaborator Author

@rushatgabhane @iwiznia, three quick questions:

  1. How to best test this with .env changes?
  2. I've also renamed EXPENSIFY_URL_COM to EXPENSIFY_URL matching the variable expensifyUrl
  3. The content variable CONST.NEWDOT I am using NEW_EXPENSIFY_URL, but note that when on staging it might show Staging URL. I am guessing it should be fine but let me know if you think otherwise.

@iwiznia
Copy link
Contributor

iwiznia commented Feb 9, 2022

How to best test this with .env changes?

I guess you can change the URLs there and test pointing to staging and prod?

The content variable CONST.NEWDOT I am using NEW_EXPENSIFY_URL, but note that when on staging it might show Staging URL. I am guessing it should be fine but let me know if you think otherwise.

I think that's perfect. Staging should point to staging and prod should point to prod.

…/change-url-vars

# Conflicts:
#	src/CONST/index.js
@mananjadhav mananjadhav marked this pull request as ready for review February 20, 2022 14:50
@mananjadhav mananjadhav requested a review from a team as a code owner February 20, 2022 14:50
@MelvinBot MelvinBot removed the request for review from a team February 20, 2022 14:50
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Feb 20, 2022

@iwiznia @rushatgabhane Will this go via QA? I am not sure what to add for screenshots for different variable changes and QA steps.

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@mananjadhav looks good, just requesting a tiny change.

.env.production Outdated
@@ -1,6 +1,6 @@
EXPENSIFY_URL_CASH=https://new.expensify.com/
NEW_EXPENSIFY_URL=https://new.expensify.com/
EXPENSIFY_URL_SECURE=https://secure.expensify.com/
Copy link
Member

Choose a reason for hiding this comment

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

Let's change it to SECURE_EXPENSIFY_URL everywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UPDATED

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 21, 2022

Will this go via QA?

I don't know. We need to make sure all URLs are used correctly.

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

I guess you can change the URLs there and test pointing to staging and prod?

This works as expected.

@iwiznia LGTM! 🎉️

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 24, 2022

Pending: Google Api test, CORS origin from localhost:8080
Pending: Plaid Test, CORS origin from localhost:8080

@mananjadhav This chrome extension gets the job done for me.

Copy link
Contributor

@iwiznia iwiznia 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, but holding merge on:

  • The pending tests
  • Since this is renaming some widely used variables, you should merge main into this branch right before we merge, to reduce the possibility of some other PR having added usages of the old variables

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Feb 27, 2022

Merged latest main. Checked the Plaid flow and Google API. I didn't complete the whole flow, just tested that it is working fine.

@iwiznia iwiznia merged commit 53581fe into Expensify:main Feb 28, 2022
@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

OSBotify commented Mar 1, 2022

🚀 Deployed to staging by @iwiznia in version: 1.1.41-0 🚀

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

@mvtglobally
Copy link

@mananjadhav @rushatgabhane @iwiznia Anything QA team need to run here ?

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

🚀 Deployed to staging by @iwiznia in version: 1.1.41-0 🚀

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

@francoisl
Copy link
Contributor

@mananjadhav @rushatgabhane @iwiznia Bump on the above please, looks like QA doesn't need to do anything other than check there are overall no errors in the console, can you confirm please?

@mananjadhav
Copy link
Collaborator Author

We could add maybe general app loading and URL checks, but as these are just code cleanup QA can look at console errors. Keen on hearing from @rushatgabhane and @iwiznia

@rushatgabhane
Copy link
Member

I'm so sorry about this.
@mananjadhav yes, could you please add the QA steps related to app download, terms page and plaid link.

@iwiznia
Copy link
Contributor

iwiznia commented Mar 7, 2022

I assumed it was no QA because most of it would be caught in regression testing, but if there are specific things to QA here, please let's add them.

@mananjadhav
Copy link
Collaborator Author

Updated

@marcaaron
Copy link
Contributor

Do we need an [Action Required] update or email for this?

@rushatgabhane
Copy link
Member

Do we need an [Action Required] update or email for this?

Are you asking if we change the title of PR?

@marcaaron
Copy link
Contributor

marcaaron commented Mar 8, 2022

No, sorry to clarify... if we change the variable names for the environment variables that everyone who works on the codebase uses - then they might need to know about this change. They may have a bunch of pointless or undefined stuff in their local .env file because we changed the names of things.

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 8, 2022

@marcaaron oh yeah, good call. Here are the renames that need to be made in a .env

I've posted it on #expensify-open-source here
It'd best if you also post it internally for more eyes. Thanks!

EXPENSIFY_URL_CASH -> NEW_EXPENSIFY_URL

EXPENSIFY_URL_SECURE -> SECURE_EXPENSIFY_URL

EXPENSIFY_URL_COM -> EXPENSIFY_URL

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

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.

7 participants