Improve error handling for uploading custom emojis and other minor modifications. #18885
Conversation
zulipbot
added
size: M
area: emoji
|
Hello @zulip/server-emoji, @zulip/server-settings members, this pull request was labeled with the "area: emoji", "area: settings (admin/org)" labels, so you may want to check it out! |
aryanshridhar
force-pushed
the
Fix-CustomEmojiMisc
branch
from
5e9c99d
to
37e11cf
Compare
|
Screenshots look good to me -- thanks! |
|
I've merged all but the last commit as the series ending with 2d8fcef, thanks @aryanshridhar! The last commit would be a security bug -- any authorization check needs to happen on the server; otherwise you could work around it by using the API. We should add an equivalent check in |
aryanshridhar
force-pushed
the
Fix-CustomEmojiMisc
branch
2 times, most recently
from
c5236a9
to
9b7bbb2
Compare
|
Updated it @timabbott by adding an authorization check in |
zerver/tests/test_realm_emoji.py
Outdated
| self.assert_json_error( | ||
| result, | ||
| "An emoji with this name already exists. Only administrators can override built-in emoji.", | ||
| ) |
There was a problem hiding this comment.
See our testing philosophy page, but we should do both the failure and success tasks in a single test.
Previously, even non-admins had the option to override built-in emojis in the `Settings Emoji` UI. This commits essentially limits the functionality of overriding custom and allows only realm administrators to override built-in emojis with their custom emojis by adding an authorization check in the backend. It also adds relevant tests in `test_realm_emoji` which tests for the cases where an admin and non admin tries to override the built-in emoji. Fixes zulip#18860.
timabbott
force-pushed
the
Fix-CustomEmojiMisc
branch
from
9b7bbb2
to
7dc56fd
Compare
|
Merged, after making the small testing style changes noted above, and also making the error message just the second sentence, to save a bit of work for our translators. Thanks @aryanshridhar! |
Improved error handling and modified the
emoji_settings_warning_modaltext for a clearer understanding.@alya FYI.
Screenshots:
Commit 9229de0 and 9d91121:
Commit aa01c62:
Commit 5e9c99d:
Fixes #18860 and #18269.