Skip to content
Merged
4 changes: 2 additions & 2 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -741,13 +741,13 @@ function User_GetBetas() {

/**
* @param {Object} parameters
* @param {String} parameters.email
* @param {String} parameters.emailList
* @param {Boolean} [parameters.requireCertainty]
* @returns {Promise}
*/
function User_IsFromPublicDomain(parameters) {
const commandName = 'User_IsFromPublicDomain';
requireParameters(['email'], parameters, commandName);
requireParameters(['emailList'], parameters, commandName);
return Network.post(commandName, {
...{requireCertainty: true},
...parameters,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Am I mistaken in thinking that User_IsFromPublicDomain expects a comma-separated list of emails instead of an array? Should this be something like:

return Network.post(commandName, {
    ...{
        requireCertainty: true,
        emailList: parameters.emailList.join(','),
    },
    ...(_.omit(parameters, 'emailList')),
});

Or perhaps more simply:

return Network.post(commandName, {
    ...parameters,
    ...{
        requireCertainty: true,
        emailList: parameters.emailList.join(','),
    },
});

The second is better as long as we want requireCertainty to always be true, which I think we do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right! In fact, I'm a bit confused by that too because even though we're passing an array, the request is actually sent as a comma separated list! I can't seem to find where it's being converted though 🤔

Anyway, I updated the call to User_IsFromPublicDomain in User.js to make it clearer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB but I think it would be a bit cleaner to do the .join in here and have it take an array.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pretty sure this happens when you use FormData

2021-09-15_14-08-01

But fun fact last time I checked it did not work this way on iOS 😃

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TIL

Expand Down
1 change: 0 additions & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ class AuthScreens extends React.Component {
PersonalDetails.fetchPersonalDetails();
User.getUserDetails();
User.getBetas();
User.getDomainInfo();
PersonalDetails.fetchLocalCurrency();
fetchAllReports(true, true);
fetchCountryCodeByRequestIP();
Expand Down
1 change: 1 addition & 0 deletions src/libs/Pusher/EventType.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export default {
REPORT_COMMENT_EDIT: 'reportCommentEdit',
REPORT_TOGGLE_PINNED: 'reportTogglePinned',
PREFERRED_LOCALE: 'preferredLocale',
ACCOUNT_VALIDATED: 'accountValidated',
};
29 changes: 29 additions & 0 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,15 @@ function updateReportPinnedState(reportID, isPinned) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {isPinned});
}

/**
* Updates isFromPublicDomain in Onyx.
*
* @param {Boolean} isFromPublicDomain
*/
function setIsFromPublicDomain(isFromPublicDomain) {
Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain});
}

/**
* Get the private pusher channel name for a Report.
*
Expand Down Expand Up @@ -741,6 +750,26 @@ function subscribeToUserEvents() {
{error, pusherChannelName, eventName: Pusher.TYPE.REPORT_TOGGLE_PINNED},
);
});

// Live-update if a user has private domains listed as primary or secondary logins.
Pusher.subscribe(pusherChannelName, Pusher.TYPE.ACCOUNT_VALIDATED, (pushJSON) => {
Log.info(
`[Report] Handled ${Pusher.TYPE.ACCOUNT_VALIDATED} event sent by Pusher`,
false,
{isFromPublicDomain: pushJSON.isFromPublicDomain},
);
setIsFromPublicDomain(pushJSON.isFromPublicDomain);
}, false,
() => {
NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there some reason we must call this here ?

Looks like we also do it here

NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel');

and here

NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel');

and here

NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just a bad example copied over? We already call triggerReconnectionCallbacks when a user comes back online in listenForReconnect. All these calls seem unnecessary then. I can refactor that on v2 of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah ok, maybe or we can create a follow up to look into a better way. Probably shouldn't need to block these changes. Just thought it was worth mentioning.

})
.catch((error) => {
Log.info(
'[Report] Failed to subscribe to Pusher channel',
false,
{error, pusherChannelName, eventName: Pusher.TYPE.ACCOUNT_VALIDATED},
);
});
}

/**
Expand Down
97 changes: 54 additions & 43 deletions src/libs/actions/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,56 @@ function getBetas() {
});
}

/**
* Fetch the public domain info for the current user (includes secondary logins).
*
* This API is a bit weird in that it sometimes depends on information being cached in bedrock.
* If the info for the domain is not in bedrock, then it creates an asynchronous bedrock job to gather domain info.
* If that happens, this function will automatically retry itself in 10 minutes.
*
* @param {String} loginList
*/
function getDomainInfo(loginList) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am probably just not seeing why it's necessary but we can access the loginList from Web-Expensify so I'm confused about why we pass it as a parameter here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is that we would filter the loginList on the frontend and only make API calls with logins that are not in the common domain list. So for example, if the loginList was ['a@gmail.com', 'b@yahoo.com', 'c@privatedomain.com'] we would only call the API to check c@privatedomain.com. If we were to access the loginList in Web-E, we would have to filter the list again.

// If this command fails, we'll retry again in 10 minutes,
// arbitrarily chosen giving Bedrock time to resolve the ClearbitCheckPublicEmail job for this email.
const RETRY_TIMEOUT = 600000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB: Could use a numeric seperator here:

Suggested change
const RETRY_TIMEOUT = 600000;
const RETRY_TIMEOUT = 600_000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, not your code anyway

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TIL about numeric separators


// First we filter out any domains that are in the list of common public domains
const emailList = _.filter(loginList, email => (
!_.contains(COMMON_PUBLIC_DOMAINS, Str.extractEmailDomain(email))
));

// If there are no emails left, we have a public domain
if (!emailList.length) {
Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain: true});
return;
}

// Check the API for the remaining uncommon domains
API.User_IsFromPublicDomain({emailList: emailList.join(',')})
.then((response) => {
if (response.jsonCode === 200) {
const {isFromPublicDomain} = response;
Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain});

// If the user is not on a public domain we'll want to know whether they are on a domain that has
// already provisioned the Expensify card
if (isFromPublicDomain) {
return;
}

API.User_IsUsingExpensifyCard()
.then(({isUsingExpensifyCard}) => {
Onyx.merge(ONYXKEYS.USER, {isUsingExpensifyCard});
});
} else {
// eslint-disable-next-line max-len
console.debug(`Command User_IsFromPublicDomain returned error code: ${response.jsonCode}. Most likely, this means that the domain ${Str.extractEmail(sessionEmail)} is not in the bedrock cache. Retrying in ${RETRY_TIMEOUT / 1000 / 60} minutes`);
setTimeout(() => getDomainInfo(loginList), RETRY_TIMEOUT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not great to leave timers running because it can lead to stuff like this function getting called when a user is logged out.

This should help ->

App/src/libs/Timers.js

Lines 11 to 27 in 57eed8b

function register(timerID) {
timers.push(timerID);
return timerID;
}
/**
* Clears all timers that we have registered. Use for long running tasks that may begin once logged out.
*/
function clearAll() {
_.each(timers, (timer) => {
// We don't know whether it's a setTimeout or a setInterval, but it doesn't really matter. If the id doesn't
// exist nothing bad happens.
clearTimeout(timer);
clearInterval(timer);
});
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TIL

}
});
}

/**
* Fetches the data needed for user settings
*/
Expand All @@ -90,6 +140,10 @@ function getUserDetails() {
const blockedFromConcierge = lodashGet(response, `nameValuePairs.${CONST.NVP.BLOCKED_FROM_CONCIERGE}`, {});
Onyx.merge(ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE, blockedFromConcierge);

// Get domainInfo for user
const emailList = _.map(loginList, userLogin => userLogin.partnerUserID);
getDomainInfo(emailList);

const preferredSkinTone = lodashGet(response, `nameValuePairs.${CONST.NVP.PREFERRED_EMOJI_SKIN_TONE}`, {});
Onyx.merge(ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
getSkinToneEmojiFromIndex(preferredSkinTone).skinTone);
Expand Down Expand Up @@ -207,49 +261,6 @@ function isBlockedFromConcierge(expiresAt) {
return moment().isBefore(moment(expiresAt), 'day');
}

/**
* Fetch the public domain info for the current user.
*
* This API is a bit weird in that it sometimes depends on information being cached in bedrock.
* If the info for the domain is not in bedrock, then it creates an asynchronous bedrock job to gather domain info.
* If that happens, this function will automatically retry itself in 10 minutes.
*/
function getDomainInfo() {
// If this command fails, we'll retry again in 10 minutes,
// arbitrarily chosen giving Bedrock time to resolve the ClearbitCheckPublicEmail job for this email.
const RETRY_TIMEOUT = 600000;

// First check list of common public domains
if (_.contains(COMMON_PUBLIC_DOMAINS, sessionEmail)) {
Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain: true});
return;
}

// If it is not a common public domain, check the API
API.User_IsFromPublicDomain({email: sessionEmail})
.then((response) => {
if (response.jsonCode === 200) {
const {isFromPublicDomain} = response;
Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain});

// If the user is not on a public domain we'll want to know whether they are on a domain that has
// already provisioned the Expensify card
if (isFromPublicDomain) {
return;
}

API.User_IsUsingExpensifyCard()
.then(({isUsingExpensifyCard}) => {
Onyx.merge(ONYXKEYS.USER, {isUsingExpensifyCard});
});
} else {
// eslint-disable-next-line max-len
console.debug(`Command User_IsFromPublicDomain returned error code: ${response.jsonCode}. Most likely, this means that the domain ${Str.extractEmail(sessionEmail)} is not in the bedrock cache. Retrying in ${RETRY_TIMEOUT / 1000 / 60} minutes`);
setTimeout(getDomainInfo, RETRY_TIMEOUT);
}
});
}

/**
* Initialize our pusher subscription to listen for user changes
*/
Expand Down