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

Restrict room rename for Task #30886

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Restrict room rename for Task #30886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0b46b6
Restrict room rename for Task
esh-g Nov 4, 2023
d420bc6
Merge branch 'main' into task-room-name
esh-g Nov 9, 2023
b6aefc8
Merge branch 'main' into task-room-name
esh-g Nov 20, 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
12 changes: 11 additions & 1 deletion src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3965,7 +3965,7 @@ function getWorkspaceChats(policyID, accountIDs) {
* @returns {Boolean}
*/
function shouldDisableRename(report, policy) {
if (isDefaultRoom(report) || isArchivedRoom(report) || isChatThread(report) || isMoneyRequestReport(report) || isPolicyExpenseChat(report)) {
if (isDefaultRoom(report) || isArchivedRoom(report) || isThread(report) || isMoneyRequestReport(report) || isPolicyExpenseChat(report)) {
return true;
}

Expand All @@ -3980,6 +3980,15 @@ function shouldDisableRename(report, policy) {
return !_.keys(loginList).includes(policy.owner) && policy.role !== CONST.POLICY.ROLE.ADMIN;
}

/**
* @param {Object|null} report
* @param {Object|null} policy - the workspace the report is on, null if the user isn't a member of the workspace
* @returns {Boolean}
*/
function canEditWriteCapability(report, policy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we modifying the write capability conditions? Is this relevant to the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ollyws I included it in my proposal and it is modified to have consistency as it is not shown for normal tasks

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see. Makes sense, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the API already check for this? Or are we only checking this on the front end?

Copy link
Contributor

@Ollyws Ollyws Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems to be determined by the front end, I can seemingly change the write-capability for a workspace room that I'm not admin of if I change the front-end to allow it.

return PolicyUtils.isPolicyAdmin(policy) && !isAdminRoom(report) && !isArchivedRoom(report) && !isThread(report);
}

/**
* Returns the onyx data needed for the task assignee chat
* @param {Number} accountID
Expand Down Expand Up @@ -4437,4 +4446,5 @@ export {
getChannelLogMemberMessage,
getRoom,
shouldDisableWelcomeMessage,
canEditWriteCapability,
};
2 changes: 1 addition & 1 deletion src/pages/settings/Report/ReportSettingsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ function ReportSettingsPage(props) {
const writeCapability = ReportUtils.isAdminRoom(report) ? CONST.REPORT.WRITE_CAPABILITIES.ADMINS : report.writeCapability || CONST.REPORT.WRITE_CAPABILITIES.ALL;

const writeCapabilityText = translate(`writeCapabilityPage.writeCapability.${writeCapability}`);
const shouldAllowWriteCapabilityEditing = lodashGet(linkedWorkspace, 'role', '') === CONST.POLICY.ROLE.ADMIN && !ReportUtils.isAdminRoom(report) && !isMoneyRequestReport;
const shouldAllowWriteCapabilityEditing = useMemo(() => ReportUtils.canEditWriteCapability(report, linkedWorkspace), [report, linkedWorkspace]);

const shouldShowNotificationPref = !isMoneyRequestReport && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const roomNameLabel = translate(isMoneyRequestReport ? 'workspace.editor.nameInputLabel' : 'newRoomPage.roomName');
Expand Down
3 changes: 1 addition & 2 deletions src/pages/settings/Report/WriteCapabilityPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import SelectionList from '@components/SelectionList';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import compose from '@libs/compose';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportUtils from '@libs/ReportUtils';
import withReportOrNotFound from '@pages/home/report/withReportOrNotFound';
import reportPropTypes from '@pages/reportPropTypes';
Expand Down Expand Up @@ -38,7 +37,7 @@ function WriteCapabilityPage(props) {
isSelected: value === (props.report.writeCapability || CONST.REPORT.WRITE_CAPABILITIES.ALL),
}));

const isAbleToEdit = !ReportUtils.isAdminRoom(props.report) && PolicyUtils.isPolicyAdmin(props.policy) && !ReportUtils.isArchivedRoom(props.report);
const isAbleToEdit = ReportUtils.canEditWriteCapability(props.report, props.policy);

return (
<ScreenWrapper
Expand Down