[IMPROVE] Add error messages to the creation of channels or usernames containing reserved words#21016
[IMPROVE] Add error messages to the creation of channels or usernames containing reserved words#21016sampaiodiego merged 20 commits intodevelopfrom
Conversation
|
Is it possible to centralize all the reserved words into a single place/file? That way, we won't have to modify all files to add a new reserved word. PD: I thought this may be the use case for settings, but not sure if we can create a |
sampaiodiego
left a comment
sampaiodiego
left a comment
There was a problem hiding this comment.
as @KevLehman pointed out, please make a change to have a unified place with invalid names, that would help maintaining the code 👍 having a hidden setting is a good way to achieve that, but along with that I'd consider creating a function to validate it that is called in multiple places.
I'd also not struggle to have the function working on frontend if it becomes challenging.
We need to cover cases where a user or channel might have one of those names already.
And I'd also ask for some API tests =)
…nto reserved-words
|
|
||
| export const validateName = function(name) { | ||
| if (settings.get('Accounts_SystemBlockedUsernameList').includes(name.toLowerCase())) { | ||
| throw new Meteor.Error('invalid-name', `${ name } is a reserved name.`); |
There was a problem hiding this comment.
in this case, since this function is almost "pure", it is better to use regular Error instead of Meteor.Error.. we should use Meteor.Error only on Meteor environments (like Meteor methods), otherwise always use regular Node exceptions
| @@ -0,0 +1,9 @@ | |||
| import { Meteor } from 'meteor/meteor'; | |||
|
|
|||
| import { settings } from '../../../settings'; | |||
There was a problem hiding this comment.
if you cannot import from the final file, pls try at least import from the server folder, for example:
| import { settings } from '../../../settings'; | |
| import { settings } from '../../../settings/server'; |
|
|
||
| import { settings } from '../../../settings'; | ||
|
|
||
| export const validateName = function(name) { |
There was a problem hiding this comment.
can you convert this file to TypeScript pls? <3
| nameValidation = new RegExp('^[0-9a-zA-Z-_.]+$'); | ||
| } | ||
|
|
||
| if (settings.get('Accounts_SystemBlockedUsernameList').includes(username.toLowerCase())) { |
There was a problem hiding this comment.
any reason to not use validateName function here?
app/utils/lib/getValidRoomName.js
Outdated
| } | ||
|
|
||
| if (!nameValidation.test(slugifiedName)) { | ||
| if (!nameValidation.test(slugifiedName) || settings.get('Accounts_SystemBlockedUsernameList').includes(slugifiedName.toLowerCase())) { |
There was a problem hiding this comment.
any reason to not use validateName function here?
app/api/server/v1/channels.js
Outdated
| import { API } from '../api'; | ||
| import { settings } from '../../../settings'; | ||
| import { Team } from '../../../../server/sdk'; | ||
| import { validateName } from '../../../lib'; |
There was a problem hiding this comment.
if you can, import direct from the file:
| import { validateName } from '../../../lib'; | |
| import { validateName } from '../../../lib/server/functions/validateName'; |
tests/end-to-end/api/01-users.js
Outdated
| import { createUser, login, deleteUser, getUserStatus } from '../../data/users.helper.js'; | ||
| import { createRoom } from '../../data/rooms.helper'; | ||
|
|
||
| const reservedWords = [ |
There was a problem hiding this comment.
can you move this array to tests/data/api-data.js ? this way you can use it here and on tests/end-to-end/api/02-channels.js without having to duplicate it =)
3 participants
Proposed changes (including videos or screenshots)
Display error messages when the user attempts to create or edit users' or channels' names with any of the following words (case-insensitive):
Issue(s)
Task - ClickUp
Steps to test or reproduce
Users:
Channels:
Further comments