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

Stop loading default chat room initally for those outside of beta #5338

Merged
merged 4 commits into from
Sep 20, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Sep 17, 2021

@deetergp, please review when you get the chance
CC: @yuwenmemon

Details

There was an edge case we didn't consider when putting default rooms behind betas. If a domain has a domainAll room (which we make behind the scenes for domains that aren't in the defaultRooms beta) and a user gets created after that room is made, they will actually see that room when they first log in (i.e. it'll be their "last accessed chat report"). They should see the concierge chat first, but that chat will be created after their account is made (which happens after the domainAll room was made).

This PR makes it so new users won't land on a default domainAll room the first time they log in. This also prevents users from seeing a default room if they aren't in the beta and somehow managed to open a default chat room (to stop other edge cases we might have missed.

Also this is relevant to N6, since we're likely to see a lot of new users sign up and use NewDot with this.

Fixed Issues

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

Tests

Same as Web QA, but done locally. I used clitools.sh for making the domain, admin account, and second account. To test a user being outside of the default rooms beta locally, I made this line false:

return _.contains(betas, CONST.BETAS.DEFAULT_ROOMS) || canUseAllBetas(betas);

QA Steps

  1. Find a domain whose users won't be in the freePlan beta.
  2. Invite a new account to that domain (can be similar to amal+something@expensifail.com). Avoid signing in with the email sent to the newly created user.
  3. Log into that account on NewDot before joining on OldDot. You can do this by entering the email in NewDot and then using the magic sign in link from there. You might have to set a password at this point too.
  4. Verify that the first report you see is the concierge one and not

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

This is what it looks like now if you try to access a report by URL (11073 is a domainAll room)
image

Mobile Web

Same thing is a blank screen on mobile obviously, but users should never be able to see this report anyway without manually entering into the url like I did.
image

@TomatoToaster TomatoToaster self-assigned this Sep 17, 2021
@TomatoToaster TomatoToaster marked this pull request as ready for review September 20, 2021 15:57
@TomatoToaster TomatoToaster requested a review from a team as a code owner September 20, 2021 15:57
@MelvinBot MelvinBot requested review from deetergp and removed request for a team September 20, 2021 15:58
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

LGTM

@deetergp deetergp merged commit da4c479 into main Sep 20, 2021
@deetergp deetergp deleted the amal-block-domain-room-chats branch September 20, 2021 22:37
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @deetergp in version: 1.0.99-8 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.0-2 🚀

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