-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Using new splited emojis and webhooks permissions #2339
Conversation
The tests are failing because it needs the mattermost-redux PR merged. |
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.
Just one thing I think needs fixing.
components/admin_console/permission_schemes_settings/permission_schemes_settings.jsx
Show resolved
Hide resolved
@@ -875,6 +881,10 @@ | |||
"admin.permissions.permission.manage_team.name": "Manage team", | |||
"admin.permissions.permission.manage_webhooks.description": "Create, edit, and delete incoming and outgoing webhooks.", | |||
"admin.permissions.permission.manage_webhooks.name": "Manage Webhooks", | |||
"admin.permissions.permission.manage_incoming_webhooks.description": "Create, edit, and delete incoming webhooks.", |
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.
Do the old translation strings also need to be removed from here, or will that get done automatically now?
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.
Can be done automatically using the tool, but must be committed.
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.
👍 Just one question.
{roleName: 'team_user', permission: Permissions.MANAGE_SLASH_COMMANDS, shouldHave: false}, | ||
{roleName: 'system_user', permission: Permissions.MANAGE_OAUTH, shouldHave: false}, | ||
], | ||
false: [ | ||
{roleName: 'team_user', permission: Permissions.MANAGE_WEBHOOKS, shouldHave: true}, | ||
{roleName: 'team_user', permission: Permissions.MANAGE_INCOMING_WEBHOOKS, shouldHave: true}, | ||
{roleName: 'team_user', permission: Permissions.MANAGE_OUTGOING_WEBHOOKS, shouldHave: true}, |
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.
Ah OK I see you updated the adapter here 👍
"admin.permissions.permission.delete_emojis.description": "Delete custom emoji.", | ||
"admin.permissions.permission.delete_emojis.name": "Delete Custom Emoji", | ||
"admin.permissions.permission.delete_others_emojis.description": "Delete others' custom emoji.", | ||
"admin.permissions.permission.delete_others_emojis.name": "Delete Others' Custom Emoji", |
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.
Do we need the descriptions if they simply echo the names or could they be left blank?
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.
I would say they must be different string, no matter if they are equal, because in the future they can change, and they can change independently. From my perspective they are different text that are temporary equal because are related, but aren't the same text.
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.
LGTM
9c475ad
to
136c905
Compare
@mkraft please could you re-review this when you have a moment? I'd like to get it merged sooner rather than later. |
* Using new splited emojis and webhooks permissions * Fixed review comments * Removed unnecesary translations * Fixing tests after redux is merged
Summary
Using new splited emojis and webhooks permissions
Ticket Link
MM-11101
Checklist
make check-style
to check for style errors (required for all pull requests)make test
to ensure unit and component tests passed