-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow free policies to have their default rooms be shown #9632
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works fine!
return; | ||
} | ||
} else if (ReportUtils.isDefaultRoom(report) && !Permissions.canUseDefaultRooms(betas)) { | ||
if (ReportUtils.isDefaultRoom(report) && !Permissions.canUseDefaultRooms(betas) && ReportUtils.getPolicyType(report, policies) !== CONST.POLICY.TYPE.FREE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, It would be good to have a comment here.
return null; | ||
} | ||
} else if (ReportUtils.isDefaultRoom(this.props.report) && !Permissions.canUseDefaultRooms(this.props.betas)) { | ||
if (!Permissions.canUseDefaultRooms(this.props.betas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can have a comment
Good point - added! |
@MonilBhavsar - should this be working on NewDot staging because it was CP'd as per the above? 🤔 Here’s what I’ve tested: Prerequisites:
Member of a paid plan with no existing workspaces.
Result: PASS Member of a paid policy that creates a workspace
Result: FAIL. The paid plan defaultRooms appear after creating the workspace (and don't disappear after a refresh) |
This isn't on staging yet. Looks like we're having some issues with Github actions. I'll ask in the slack here https://expensify.slack.com/archives/C07J32337/p1656578983148269 |
Also tested the reverse for completeness with the same result. Existing admin of a workspace, creates a new paid plan
Result: FAIL |
Ahhhhh. 👌 |
…efaultRooms Allow free policies to have their default rooms be shown (cherry picked from commit 02f4339)
…ng-9632 🍒 Cherry pick PR #9632 to staging 🍒
🚀 Cherry-picked to staging by @roryabraham in version: 1.1.79-5 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by @roryabraham in version: 1.1.79-17 🚀
|
@MonilBhavsar please review
Details
Branching off a revert of #9460, adjust the logic so that we only show default rooms that belong to free plan workspaces, instead of showing all policy rooms to anyone who has a free plan workspace.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/214193
Tests
change this line to
return false
Log into an account on dev, create some Free Plan policy types. To do this go to the green plus on the bottom left and create a Workspace.
After it's created verify that after creating your workspace you see the default rooms for the workspace show up in the LHN: https://user-images.githubusercontent.com/4741899/173680577-10d77f0f-c3ff-4401-8ff7-01e86d42a281.mp4
Verify that you cannot find the domain room for your domain that you're logged into
Log in via another account that does not belong to a free plan policy (workspace), but belongs to other policies.
Verify that you cannot see any default rooms (policy or domain):
Change that original line back, log in via both accounts, Verify you should be able to see all rooms now (including the domain room)
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps