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

Use app environment to determine olddot URL and update staging env #14944

Merged
merged 7 commits into from
Mar 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,9 @@ const CONST = {
CFPB_PREPAID_URL: 'https://cfpb.gov/prepaid',
STAGING_NEW_EXPENSIFY_URL: 'https://staging.new.expensify.com',
NEWHELP_URL: 'https://help.expensify.com',
INTERNAL_DEV_EXPENSIFY_URL: 'https://www.expensify.com.dev',
STAGING_EXPENSIFY_URL: 'https://staging.expensify.com',
EXPENSIFY_URL: 'https://www.expensify.com',

// Use Environment.getEnvironmentURL to get the complete URL with port number
DEV_NEW_EXPENSIFY_URL: 'http://localhost:',
Expand Down
17 changes: 17 additions & 0 deletions src/libs/Environment/Environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ const ENVIRONMENT_URLS = {
[CONST.ENVIRONMENT.PRODUCTION]: CONST.NEW_EXPENSIFY_URL,
};

const OLDDOT_ENVIRONMENT_URLS = {
[CONST.ENVIRONMENT.DEV]: CONST.INTERNAL_DEV_EXPENSIFY_URL,
[CONST.ENVIRONMENT.STAGING]: CONST.STAGING_EXPENSIFY_URL,
[CONST.ENVIRONMENT.PRODUCTION]: CONST.EXPENSIFY_URL,
};

/**
* Are we running the app in development?
*
Expand All @@ -31,8 +37,19 @@ function getEnvironmentURL() {
});
}

/**
* Get the corresponding oldDot URL based on the environment we are in
*
* @returns {Promise<string>}
*/
function getOldDotEnvironmentURL() {
return getEnvironment()
Copy link
Contributor

Choose a reason for hiding this comment

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

Using getEnvironment will break olddot urls for those who don't use .env file. The olddot url will fallback to the dev url which is not accessible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how one can not have .env file. I think you meant contributors.

Copy link
Contributor

@kidroca kidroca Mar 9, 2023

Choose a reason for hiding this comment

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

Yes, external contributors are setup so that they don't need an .env file, even if they have one, they won't be able to access https://www.expensify.com.dev (this value comes directly from CONST and not from .env)

Is it OK if this functionality doesn't work for external contributors, when they are using the DEV build?

Since external contributors are using the PROD API (In DEV via proxy), perhaps this link should point to https://www.expensify.com for external contributors during DEV

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a low priority bug, it's only at DEV time, if we can fix this with the least work it would be great otherwise we can just point this on the README file so contributors can overwrite the getOldDotEnvironmentURL for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it,
So technically this doesn't sound like a bug.

As @s77rt, We can ask contributors to make it work by overriding the URL in CONST or updating getOldDotEnvironmentURL()

Not sure, if we should add it to README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can post in slack and give a heads up to contributors

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah README is an overkill for such minor detail that contributors may not even have to deal with. Let's do nothing for now, if we get reports on this we may consider adding it to README then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Posting this on Slack seems as a good idea, let's do it.

.then(environment => OLDDOT_ENVIRONMENT_URLS[environment]);
}

export {
getEnvironment,
isDevelopment,
getEnvironmentURL,
getOldDotEnvironmentURL,
};
22 changes: 16 additions & 6 deletions src/libs/actions/Link.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import ONYXKEYS from '../../ONYXKEYS';
import Growl from '../Growl';
import * as Localize from '../Localize';
import CONST from '../../CONST';
import CONFIG from '../../CONFIG';
import asyncOpenURL from '../asyncOpenURL';
import * as API from '../API';
import * as Environment from '../Environment/Environment';
import * as Url from '../Url';

let isNetworkOffline = false;
Onyx.connect({
Expand Down Expand Up @@ -38,7 +39,7 @@ function showGrowlIfOffline() {
function openOldDotLink(url) {
/**
* @param {String} [shortLivedAuthToken]
* @returns {String}
* @returns {Promise<string>}
*/
function buildOldDotURL(shortLivedAuthToken) {
const hasHashParams = url.indexOf('#') !== -1;
Expand All @@ -49,8 +50,13 @@ function openOldDotLink(url) {

const params = _.compact([authTokenParam, emailParam]).join('&');

// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters.
return `${CONFIG.EXPENSIFY.EXPENSIFY_URL}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`;
return Environment.getOldDotEnvironmentURL()
.then((environmentURL) => {
const oldDotDomain = Url.addTrailingForwardSlash(environmentURL);

// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters.
return `${oldDotDomain}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`;
MonilBhavsar marked this conversation as resolved.
Show resolved Hide resolved
});
}

if (isNetworkOffline) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@MonilBhavsar A bit late, but don't we need to make changes here? Linking.openURL expects a string not a promise.

        buildOldDotURL().then((oldDotUrl) => {
            Linking.openURL(oldDotUrl);
        });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We missed it. It was fixed here #15923

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh too late then. Sorry about that

Expand All @@ -63,9 +69,13 @@ function openOldDotLink(url) {
API.makeRequestWithSideEffects(
'OpenOldDotLink', {}, {},
).then((response) => {
Linking.openURL(buildOldDotURL(response.shortLivedAuthToken));
buildOldDotURL(response.shortLivedAuthToken).then((oldDotUrl) => {
Linking.openURL(oldDotUrl);
});
}).catch(() => {
Linking.openURL(buildOldDotURL());
buildOldDotURL().then((oldDotUrl) => {
Linking.openURL(oldDotUrl);
});
});
}

Expand Down