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.

Sign up for GitHub

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

Don't restrict accessing members for non-public UCRs #30613

Merged
merged 14 commits into from
Dec 5, 2023
Merged

Don't restrict accessing members for non-public UCRs #30613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d4a1eea
don't restrict accessing members for non-public UCRs
jasperhuangg Oct 30, 2023
f1c7358
remove beta checks from members screens
jasperhuangg Nov 3, 2023
f2a7398
remove unused
jasperhuangg Nov 3, 2023
6329e16
remove unused
jasperhuangg Nov 3, 2023
6a0bf33
fix display
jasperhuangg Nov 3, 2023
211f2e5
remove 'remove' button for non workspace members
jasperhuangg Nov 3, 2023
e4edb38
add back
jasperhuangg Nov 3, 2023
949e900
merge main
jasperhuangg Nov 17, 2023
5ecef86
Merge branch 'main' of github.com:Expensify/App into jasper-fixMembers
jasperhuangg Nov 30, 2023
13fb8d0
resolve conflicts
jasperhuangg Nov 30, 2023
c61b669
remove unused hubs
jasperhuangg Dec 4, 2023
7caf102
undo
jasperhuangg Dec 4, 2023
c4eac0e
Merge branch 'jasper-fixMembers' of github.com:Expensify/App into jas…
jasperhuangg Dec 4, 2023
2dc18d1
undo
jasperhuangg Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/pages/ReportDetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function ReportDetailsPage(props) {
return items;
}

if ((!isUserCreatedPolicyRoom && participants.length) || (isUserCreatedPolicyRoom && isPolicyMember)) {
if ((!isUserCreatedPolicyRoom && participants.length) || (isUserCreatedPolicyRoom && (!ReportUtils.isPublicRoom(props.report) || isPolicyMember))) {
items.push({
key: CONST.REPORT_DETAILS_MENU_ITEM.MEMBERS,
translationKey: 'common.members',
Expand All @@ -108,7 +108,7 @@ function ReportDetailsPage(props) {
}
},
});
} else if ((!participants.length || !isPolicyMember) && isUserCreatedPolicyRoom && !props.report.parentReportID) {
} else if (isUserCreatedPolicyRoom && (!participants.length || !isPolicyMember) && !props.report.parentReportID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the order changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because sometimes the participants won't exist if the report is a User Created Policy Room

items.push({
key: CONST.REPORT_DETAILS_MENU_ITEM.INVITE,
translationKey: 'common.invite',
Expand Down
11 changes: 0 additions & 11 deletions src/pages/RoomInvitePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import * as Browser from '@libs/Browser';
import compose from '@libs/compose';
import Navigation from '@libs/Navigation/Navigation';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import Permissions from '@libs/Permissions';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportUtils from '@libs/ReportUtils';
Expand Down Expand Up @@ -73,16 +72,6 @@ function RoomInvitePage(props) {
// Any existing participants and Expensify emails should not be eligible for invitation
const excludedUsers = useMemo(() => [...PersonalDetailsUtils.getLoginsByAccountIDs(lodashGet(props.report, 'participantAccountIDs', [])), ...CONST.EXPENSIFY_EMAILS], [props.report]);

useEffect(() => {
// Kick the user out if they tried to navigate to this via the URL
if (Permissions.canUsePolicyRooms(props.betas)) {
return;
}
Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report.reportID));

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
const inviteOptions = OptionsListUtils.getMemberInviteOptions(props.personalDetails, props.betas, searchTerm, excludedUsers);

Expand Down
15 changes: 1 addition & 14 deletions src/pages/RoomMembersPage.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check here if the user is member of the room here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is handled already by withReportOrNotFound

Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import compose from '@libs/compose';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import Permissions from '@libs/Permissions';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as UserUtils from '@libs/UserUtils';
Expand All @@ -31,9 +30,6 @@ import withReportOrNotFound from './home/report/withReportOrNotFound';
import reportPropTypes from './reportPropTypes';

const propTypes = {
/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),

/** The report currently being looked at */
report: reportPropTypes.isRequired,

Expand Down Expand Up @@ -69,7 +65,6 @@ const defaultProps = {
},
report: {},
policies: {},
betas: [],
...withCurrentUserPersonalDetailsDefaultProps,
};

Expand All @@ -90,11 +85,6 @@ function RoomMembersPage(props) {
}, [props.report.reportID]);

useEffect(() => {
// Kick the user out if they tried to navigate to this via the URL
if (!PolicyUtils.isPolicyMember(props.report.policyID, props.policies) || !Permissions.canUsePolicyRooms(props.betas)) {
Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report.reportID));
return;
}
getRoomMembers();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Expand Down Expand Up @@ -247,7 +237,7 @@ function RoomMembersPage(props) {
testID={RoomMembersPage.displayName}
>
<FullPageNotFoundView
shouldShow={_.isEmpty(props.report) || !isPolicyMember}
shouldShow={_.isEmpty(props.report) || (ReportUtils.isPublicRoom(props.report) && !isPolicyMember)}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the user is accessing a deep link to a room in which he is neither a member nor part of the workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is handled by withReportOrNotFound

subtitleKey={_.isEmpty(props.report) ? undefined : 'roomMembersPage.notAuthorized'}
onBackButtonPress={() => Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report.reportID))}
>
Expand Down Expand Up @@ -322,9 +312,6 @@ export default compose(
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
betas: {
key: ONYXKEYS.BETAS,
},
}),
withCurrentUserPersonalDetails,
)(RoomMembersPage);
Loading