[HOLD for payment 2022-03-17] Cleanup: URL Configs

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Coming from this issue. This is a non-standard issue in that we're just asking for some code cleanup.

Problem

We currently have a number of different URL constants in this repo:

• expensifyCashURL
• URL_EXPENSIFY_CASH
• EXPENSIFY_URL_CASH
• expensifyURL
• EXPENSIFY_URL_COM

The list goes on...

Why this is important

This is confusing for developers and might sometimes lead to the wrong URL being used.

Solution

@NikkiWines Suggested a solution in this comment. Generically, I would request the following traits in a solution:

1. URLs are defined in the .env files (and thus are environment-specific), but use a fallback if the url is missing from the config for some reason.
2. URLs do not refer to CASH, because this platform is no longer called Expensify.cash

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @laurenreidexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[roryabraham]

cc @mananjadhav if you're interested

[mateusbra]

I would like to work on this one if possible

[mananjadhav]

@roryabraham Yeah I am interested. Here's my proposal.

Based on the comment here:
#7316 (comment)

I'll update the references in Config.js, it involves, updating:

1. expensifyURL --> expensifyURLCom
2. expensifyCashURL --> expensifyURLNew
3. URL_EXPENSIFY_CASH --> EXPENSIFY_URL_NEW
   This will require updating the references in HttpUtils, getPlaidLinkTokenParameters and BankAccountStep
4. ACTIVE_ENVIRONMENT_NEW_EXPENSIFY_URL --> ACTIVE_EXPENSIFY_URL
   This needs to be updated in GenericError and CONST

These values have to be updated in .env as well.

I am guessing we don't need to change anything for EXPENSIFY_URL_SECURE

Apart from these, there are three other URLs in CONST, NEWDOT, NEW_EXPENSIFY_URL, STAGING_NEW_EXPENSIFY_URL.

1. NEW_EXPENSIFY_URL --> EXPENSIFY_URL_NEW can be done
   This is used in AnchorRenderer, linkingConfig.
2. NEWDOT is used for content in LongTermsForm. Not sure about this
3. STAGING_NEW_EXPENSIFY_URL --> Not sure about this. But it is used in AnchorRenderer and we should just move that check to Utils.

App/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

Lines 22 to 25 in 2c14f01

	const attrHref = htmlAttribs.href || '';
	const internalExpensifyPath = (attrHref.startsWith(CONST.NEW_EXPENSIFY_URL) && attrHref.replace(CONST.NEW_EXPENSIFY_URL, ''))
	|| (attrHref.startsWith(CONST.STAGING_NEW_EXPENSIFY_URL) && attrHref.replace(CONST.STAGING_NEW_EXPENSIFY_URL, ''));

cc - @NikkiWines

[laurenreidexpensify]

https://www.upwork.com/jobs/~01042f26b35521c7fd

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

[MelvinBot]

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[rushatgabhane]

expensifyURL --> expensifyURLCom
expensifyCashURL --> expensifyURLNew

@mananjadhav Should we name it the way url appears? i.e. subdomain then domain.

expensifyURL stays the same
expensifyCashURL --> newExpensifyURL
URL_EXPENSIFY_CASH --> NEW_EXPENSIFY_URL

Then STAGING_NEW_EXPENSIFY_URL will also be consistent.

[mananjadhav]

Well I guess that works too.

[rushatgabhane]

🎀👀🎀 C+ reviewed

@iwiznia I recommend that we hire @mananjadhav to fix this.

Left to right URL naming (subdomain then domain) seems the simplest, so I think we should go for that.
Let me know if you think otherwise as I don't have any strong opinions on this

[iwiznia]

Looks good but let's first define:

NEWDOT is used for content in LongTermsForm. Not sure about this

What exactly are you not sure about?

[mananjadhav]

NEWDOT is used for content in LongTermsForm. Not sure about this

What exactly are you not sure about?

LongTermsForm component uses CONST.NEWDOT which is set as new.expensify.com. We also have NEW_EXPENSIFY_URL which will have https://new.expensify.com. So should we be keeping the CONST.NEWDOT as is or change it to NEW_EXPENSIFY_URL.

[rushatgabhane]

We're using NEWDOT in only one place. We shouldn't change it to NEW_EXPENSIFY_URL because all other links in this page don't have https.

App/src/pages/EnablePayments/TermsPage/LongTermsForm.js

Line 119 in 6976b4a

{CONST.NEWDOT}

Don't have any good ideas on the name.
Is NEW_EXPENSIFY_HOST_NAME okay?

@mananjadhav @iwiznia

[mananjadhav]

My thought was add a util: getRelativeUrl(url) with url.replace(/(^\w+:|^)\/\//, '') and then use NEW_EXPENSIFY_URL.

[iwiznia]

So should we be keeping the CONST.NEWDOT as is or change it to NEW_EXPENSIFY_URL.

This sounds reasonable to me

We shouldn't change it to NEW_EXPENSIFY_URL because all other links in this page don't have https.

But is that on purpose? Why wouldn't we want to use https in all links?

[mananjadhav]

But is that on purpose? Why wouldn't we want to use https in all links?

Reading https://, etc. is not reader-friendly?

[iwiznia]

🤷 let's just use the one with https, I don't think it's a big deal.
Anyway, @laurenreidexpensify let's hire @mananjadhav please

[MelvinBot]

📣 @mananjadhav You have been assigned to this job by @laurenreidexpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[mananjadhav]

Was a bit unwell, will push an update tonight.

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.41-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Config URL Cleanup #7662

If no regressions arise, payment will be issued on 2022-03-17. 🎊

[laurenreidexpensify]

@mananjadhav @rushatgabhane paid 👍🏽