Ensure accounts with a private domain secondary login can create a workspace #5037

luacmartins · 2021-09-02T23:12:52Z

Details

Ensure accounts with a private domain secondary login can create a workspace via the "Get Started" button in workspace configuration.

Related Web-E PR: https://github.com/Expensify/Web-Expensify/pull/31862

cc @roryabraham

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/176225

Tests

Create an account with a public domain. e.g. @gmail.com.
Login to NewDot and navigate to + > New Workspace and verify that you see Add Email in the card page.
Click on Add email.
Add a private domain secondary login to your account, e.g. @expensify.com.
Validate the secondary login by following the instructions in this SO to generate a link. Note: Don't use the clitools script! It does not hit the API endpoint and will not trigger the pusher event to update NewDot.
Go back to NewDot and verify that the button now says Get Started and clicking it starts the VBA flow.

QA Steps

Steps above.

Tested On

Screenshots

Web

web.mov

Mobile Web

Desktop

desktop.mov

iOS

Android

luacmartins · 2021-09-13T21:08:25Z

No longer on hold!

roryabraham · 2021-09-13T21:15:37Z

src/libs/actions/Report.js

+ *
+ * @param {Boolean} isFromPublicDomain
+ */
+function updateIsFromPublicDomain(isFromPublicDomain) {


Suggested change

function updateIsFromPublicDomain(isFromPublicDomain) {

function setIsFromPublicDomain(isFromPublicDomain) {

roryabraham · 2021-09-13T21:17:44Z

src/libs/getDomainFromEmail.js

+ * @param {String} email
+ * @returns {String}
+ */
+function getDomainFromEmail(email) {


No need for this – use Str.extractEmailDomain from expensify-common.

Oooh didn't know about this... TIL! Thanks!

roryabraham · 2021-09-13T21:30:46Z

src/libs/API.js

 * @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,


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.

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.

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

Pretty sure this happens when you use FormData

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

luacmartins · 2021-09-13T23:36:19Z

Updated!

roryabraham · 2021-09-13T23:44:45Z

src/libs/API.js

 * @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,


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

yuwenmemon · 2021-09-14T05:31:14Z

src/libs/actions/User.js

+function getDomainInfo(loginList) {
+    // 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;


NAB: Could use a numeric seperator here:

Suggested change

const RETRY_TIMEOUT = 600000;

const RETRY_TIMEOUT = 600_000;

Ah, not your code anyway

TIL about numeric separators

OSBotify · 2021-09-14T05:33:45Z

✋ 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 · 2021-09-15T21:16:26Z

🚀 Deployed to staging by @yuwenmemon in version: 1.0.98-2 🚀

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

marcaaron

Leaving random comments as I look at what this PR is doing.

marcaaron · 2021-09-16T00:18:06Z

src/libs/actions/Report.js

+        setIsFromPublicDomain(pushJSON.isFromPublicDomain);
+    }, false,
+    () => {
+        NetworkConnection.triggerReconnectionCallbacks('pusher re-subscribed to private user channel');


Is there some reason we must call this here ?

Looks like we also do it here

App/src/libs/actions/Report.js

Line 744 in 57eed8b

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

and here

App/src/libs/actions/Report.js

Line 724 in 57eed8b

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

and here

App/src/libs/actions/Report.js

Line 704 in 57eed8b

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

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.

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.

marcaaron · 2021-09-16T00:21:36Z

src/libs/actions/User.js

+ *
+ * @param {String} loginList
+ */
+function getDomainInfo(loginList) {


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?

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 ['[email protected]', '[email protected]', '[email protected]'] we would only call the API to check [email protected]. If we were to access the loginList in Web-E, we would have to filter the list again.

marcaaron · 2021-09-16T00:26:57Z

src/libs/actions/User.js

+            } 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);


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);

});

}

Revert PR #5037

Revert PR #5037 (cherry picked from commit d7e7d77)

OSBotify · 2021-09-20T21:01:34Z

🚀 Deployed to production by @Jag96 in version: 1.0.99-4 🚀

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

refactor getDomainInfo for an array of logins

369003b

luacmartins self-assigned this Sep 2, 2021

luacmartins added 10 commits September 2, 2021 17:16

fix some styles

2364a5d

revert changes

9e3c033

remove space

fc260d9

filter common domains

33f5ef4

add getDomainFromEmail docs

8b7c1fb

change comment

58ae918

add emailList to API call

2ca78a0

fix race condition in getUserDetails and getDomainInfo

d780736

move getDomainFromEmail

24f79f6

fix style

917c4f7

luacmartins changed the title ~~Ensure accounts with a private domain secondary login can create a workspace~~ [Hold Web-E #31862] Ensure accounts with a private domain secondary login can create a workspace Sep 10, 2021

luacmartins changed the title ~~[Hold Web-E #31862] Ensure accounts with a private domain secondary login can create a workspace~~ [Hold Web-E #31862][InternalQA] Ensure accounts with a private domain secondary login can create a workspace Sep 10, 2021

luacmartins added 2 commits September 10, 2021 14:11

merge main

dab1faf

resolve conflicts

4283ec3

luacmartins changed the title ~~[Hold Web-E #31862][InternalQA] Ensure accounts with a private domain secondary login can create a workspace~~ [InternalQA] Ensure accounts with a private domain secondary login can create a workspace Sep 10, 2021

add pusher event listener for validateAccount

a50ab75

luacmartins changed the title ~~[InternalQA] Ensure accounts with a private domain secondary login can create a workspace~~ [Hold Web-E #31862][InternalQA] Ensure accounts with a private domain secondary login can create a workspace Sep 10, 2021

luacmartins changed the title ~~[Hold Web-E #31862][InternalQA] Ensure accounts with a private domain secondary login can create a workspace~~ [Hold Web-E #31862] Ensure accounts with a private domain secondary login can create a workspace Sep 10, 2021

luacmartins marked this pull request as ready for review September 10, 2021 23:47

luacmartins requested a review from a team as a code owner September 10, 2021 23:47

MelvinBot requested review from yuwenmemon and removed request for a team September 10, 2021 23:48

luacmartins changed the title ~~[Hold Web-E #31862] Ensure accounts with a private domain secondary login can create a workspace~~ Ensure accounts with a private domain secondary login can create a workspace Sep 13, 2021

roryabraham requested changes Sep 13, 2021

View reviewed changes

address change requests

6b15611

roryabraham approved these changes Sep 13, 2021

View reviewed changes

yuwenmemon approved these changes Sep 14, 2021

View reviewed changes

yuwenmemon merged commit 478e9cf into main Sep 14, 2021

yuwenmemon deleted the cmartins-isSecondaryLoginFromPublicDomain branch September 14, 2021 05:33

OSBotify mentioned this pull request Sep 15, 2021

Deploy Checklist: New Expensify 2021-09-15 #5279

Closed

27 tasks

luacmartins mentioned this pull request Sep 16, 2021

"Add email" button showing incorrectly on a private domain's workspace #5285

Closed

marcaaron reviewed Sep 16, 2021

View reviewed changes

luacmartins mentioned this pull request Sep 16, 2021

Revert PR #5037 #5288

Merged

5 tasks

kavimuru mentioned this pull request Sep 16, 2021

Chat - New Workspace isn't available for public domain account #5289

Closed

luacmartins added a commit that referenced this pull request Sep 16, 2021

Merge pull request #5288 from Expensify/cmartins-revert-5037

d7e7d77

Revert PR #5037

github-actions bot pushed a commit that referenced this pull request Sep 16, 2021

Merge pull request #5288 from Expensify/cmartins-revert-5037

54bc1fa

Revert PR #5037 (cherry picked from commit d7e7d77)

This was referenced Sep 29, 2021

[Hold Web-E #32014] Check all emails in loginList for a private domain #5457

Closed

Check all emails in loginList for a private domain #5596

Closed

luacmartins restored the cmartins-isSecondaryLoginFromPublicDomain branch October 5, 2021 18:27

roryabraham deleted the cmartins-isSecondaryLoginFromPublicDomain branch June 8, 2023 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure accounts with a private domain secondary login can create a workspace #5037

Ensure accounts with a private domain secondary login can create a workspace #5037

luacmartins commented Sep 2, 2021 •

edited

Loading

luacmartins commented Sep 13, 2021

roryabraham Sep 13, 2021

roryabraham Sep 13, 2021

luacmartins Sep 13, 2021 •

edited

Loading

roryabraham Sep 13, 2021

luacmartins Sep 13, 2021

roryabraham Sep 13, 2021

marcaaron Sep 16, 2021

luacmartins Sep 16, 2021

luacmartins commented Sep 13, 2021

roryabraham Sep 13, 2021

yuwenmemon Sep 14, 2021

yuwenmemon Sep 14, 2021

roryabraham Sep 14, 2021

OSBotify commented Sep 14, 2021

OSBotify commented Sep 15, 2021

marcaaron left a comment

marcaaron Sep 16, 2021

luacmartins Sep 16, 2021

marcaaron Sep 16, 2021

marcaaron Sep 16, 2021

luacmartins Sep 16, 2021

marcaaron Sep 16, 2021

luacmartins Sep 16, 2021

OSBotify commented Sep 20, 2021

	function updateIsFromPublicDomain(isFromPublicDomain) {
	function setIsFromPublicDomain(isFromPublicDomain) {

	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);
	});
	}

Ensure accounts with a private domain secondary login can create a workspace #5037

Ensure accounts with a private domain secondary login can create a workspace #5037

Conversation

luacmartins commented Sep 2, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

luacmartins commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Sep 14, 2021

OSBotify commented Sep 15, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Sep 20, 2021

luacmartins commented Sep 2, 2021 •

edited

Loading

luacmartins Sep 13, 2021 •

edited

Loading