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

Load policies after we load betas as well #4084

Merged
merged 2 commits into from
Jul 15, 2021
Merged

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Jul 15, 2021

@Luke9389, please review
cc: @tgolen, because we discussed this solution

Details

The first time a user logs in, we will load their betas and based on which betas they are in, we will load their policy infos. Currently, since this all happens in the componentDidMount() function, the betas aren't loaded for the beta check and so we don't load the policy info for the user. This will work after the user logs in and refreshes, since we store the betas locally.

This change will make it so we check for the betas again after we have loaded them in componentDidUpdate.

Fixed Issues

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

Tests

Web QA done locally, but to get the full effect of this you should change this line to remove the isDevelopment() otherwise you'll just always load every beta behavior on dev.

QA Steps

Requires you to be in the Free Plan beta (reach out to @TomatoToaster to test if you can't access it)

  1. Create a Workspace
  2. Log out of e.cash
  3. Log back into e.cash
  4. Go to settings and see no Workspaces exist.
  5. Refresh and see the Workspace

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

@TomatoToaster TomatoToaster requested a review from a team as a code owner July 15, 2021 18:31
@TomatoToaster TomatoToaster self-assigned this Jul 15, 2021
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team July 15, 2021 18:31
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Looks good! That's quite the gotcha. I'm sure we're not going to be the last people to run into this. I'm kind of wondering if we could make a higher level component that essentially triggers a callback when the betas are available (like withBetas), but it might be kind of awkward to have several nested HOCs all using withOnyx.

It's fine for now, but something we can keep in mind for the future.

@tgolen tgolen merged commit b228656 into main Jul 15, 2021
@tgolen tgolen deleted the amal-load-policy-beta branch July 15, 2021 19:37
@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.78-3🚀

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

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Jul 16, 2021

SO clean! The workspaces popped up right away.
image

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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