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

Store User_IsFromPublicDomain in Onyx #3460

Merged
merged 7 commits into from
Jun 14, 2021
Merged

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jun 9, 2021

Details

This is a weird API that relies on Bedrock cache. If there is no domain info in the bedrock cache, we create a separate bedrock job to get the domain info from clearbit. In this case, the User.getPublicDomainInfo Onyx action will auto-retry itself after 10 minutes.

Fixed Issues

Fixes (partial) https://github.com/Expensify/Expensify/issues/120480

Tests / QA Steps (Web/Desktop)

  1. Login to Expensify.cash as a public domain user, such as roryabrahamtest@gmail.com
  2. Give the app a few seconds to load, then open the JS console, and look in Application -> Local Storage. Look for a key "user", and verify that it contains the data isFromPublicDomain: true.
  3. Login to Expensify.cash as a private domain user, such as rory@expensify.com
  4. Give the app a few seconds to load, then open the JS console, and look in Application -> Local Storage. Look for a key "user", and verify that it contains the data isFromPublicDomain: false.

On dev only

  1. Verify that your dev console shows debug-level logs

  2. Create and validate a new account with a private domain name, such as rory@mynewdomain.com

  3. Login to E.cash with the new account.

  4. Check the local storage – there should be no value isFromPublicDomain under the user key.

  5. There should also be a debug-level console log saying:

    Command User_IsFromPublicDomain returned error code: 666. Most likely, this means that the domain mynewdomain.com is not in the bedrock cache. Retrying in 10 minutes
    
  6. Go into the bedrock sql database, and check for a bedrock job:

    SELECT * FROM jobs WHERE name = 'www-prod/ClearbitCheckPublicEmail?domain=mynewdomain.com'
  7. Run bwm.sh

  8. After 10 minutes has passed, check the local storage again. Verify that the value isFromPublicDomain: false is set under the user key.

Tests / QA (iOS/Android/mWeb)

QA is dependent on easy access to JS console, so just regular regression testing - no extra QA.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Desktop

image

@roryabraham roryabraham self-assigned this Jun 9, 2021
@roryabraham roryabraham changed the title Store User_IsFromPublicDomain in Onyx [HOLD Web-E #31169] Store User_IsFromPublicDomain in Onyx Jun 9, 2021
@roryabraham roryabraham marked this pull request as ready for review June 9, 2021 18:37
@roryabraham roryabraham requested a review from a team as a code owner June 9, 2021 18:37
@MelvinBot MelvinBot requested review from pecanoro and removed request for a team June 9, 2021 18:37
@roryabraham
Copy link
Contributor Author

Web-E PR is merged, so this can be tested locally using Web-E master. Should still be on hold until our next web deploy though, because it will fail staging QA until https://github.com/Expensify/Web-Expensify/pull/31169 is deployed to production.

@roryabraham
Copy link
Contributor Author

https://github.com/Expensify/Web-Expensify/pull/31169 was deployed to staging, not on prod yet

@roryabraham
Copy link
Contributor Author

roryabraham commented Jun 10, 2021

cc @TomatoToaster I saw you HELD https://github.com/Expensify/Expensify/issues/166039 on this ... feel free to follow along here and review if you want

Onyx.merge(ONYXKEYS.USER, {isFromPublicDomain});
} 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`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not E.cash knowledgable, so not a blocker, is there a reason why we do console.debug() in e.cash comparing to our Web code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well we can use this library for server logs. Those should show up in LogSearch I believe. I just didn't think we needed this in server logs, but we could put them there. What do you think?

But if you're asking why we used console.debug instead of console.log, it's mostly because we have some ESLint rule that bans console.log. I'm honestly not too sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know honestly, we don't usually log errors in the console and just keep them in the server, that's why I am asking. But maybe since now the repo is public, this makes easier for contributors to debug, no idea haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well if this happens, we'll likely have server logs for the ClearbitCheckPublicEmail bedrock job that was just created, so let's just leave this for now. I think it'll maybe be useful for debugging, esp. since it's not obvious why this request might fail.

@roryabraham roryabraham changed the title [HOLD Web-E #31169] Store User_IsFromPublicDomain in Onyx Store User_IsFromPublicDomain in Onyx Jun 14, 2021
@roryabraham
Copy link
Contributor Author

No longer on hold!

@TomatoToaster TomatoToaster merged commit 53b1596 into main Jun 14, 2021
@TomatoToaster TomatoToaster deleted the Rory-IsUserPublicDomain branch June 14, 2021 19:40
@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

🚀 Deployed to staging in version: 1.0.68-5🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

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.

4 participants