Add ability to allow roles to remove sounds from config.json #85
Conversation
There was a problem hiding this comment.
Hello boss! Thank you so much for using the bot, and now even contributing AGAIN.
I saw you initially added to and edited the Spanish translation, so feel free to open up a separate PR for that. Thank you so much for updating that! 🇪🇸
There a couple of issues here, especially the fact that other commands need the same changes, as we have a number of commands that are restricted. So we definitely need to make some more changes.
src/bot/commands/RemoveCommand.ts
Outdated
|
|
||
| let allowedToRemove = false; | ||
|
|
||
| if (this.config.rolesAllowedToRemoveSounds !== undefined) { | ||
| allowedToRemove = message.member!.roles.cache.some( | ||
| r => this.config.rolesAllowedToRemoveSounds.indexOf(r.name) >= 0 | ||
| ); | ||
| } | ||
|
|
||
| if (!message.member.hasPermission(Permissions.FLAGS.ADMINISTRATOR!) && !allowedToRemove) return; |
There was a problem hiding this comment.
There is multiple things here.
The way you implemented it, the user would have to be in the rolesAllowedToRemoveSounds AND they would still need to have the administrator flag. That does not change the functionality all too much, and maybe that is a bit too overkill. We are allowing users to set the roles explicitly, so I don't think we need the extra check anymore. What do you think?
You can use Array.includes instead of .indexOf.
I don't think you need the bang ! after message.member, as the first line of the method checks for it already. With the ! you tell TypeScript to not care about nullable values any more, but we already made the explicit check for it.
You need to add rolesAllowedToRemoveSounds to both Config (see other examples), ConfigInterface and DefaultConfig. After that it cannot be undefined anymore, so the check becomes obsolete.
This is one of the commands that need elevated rights. The same check for the ADMINISTRATOR flag, seems to appear 6 times overall in the code base, so I expect us to make the same change across the whole code base. Now, we also don't want to repeat ourselves all too much, right? So if the code needed to check for the rights is a couple lines (like here), we can put it in a different file and call that piece of code instead (e.g. hasElevatedRights(member), as well as re-use it across the other 5 occurrences.
There was a problem hiding this comment.
I think I applied all of your comments, and sorry, I'm terribly new and bad to typescript 🤦♂️
Hope this is up to your standards :(