[REJECTED] Tsubaki Cogs

Applications
#1

Discord Name:
Aradia Megido#2552

GitHub Repository (Must be V3):

Description:
A collection of miscellaneous moderation/administration/general purpose cogs that I wanted to share with the public.

#2
#3

Hi The Tsubaki Bot Team, thank you for your patience while I review your repo.

Commit hash at time of review - c9de3adb18c6d314a6fe1f7f7bfc462013196779

tsutils

tsutils.py

This isn’t really normally how things are done but I will be including your tsutils in this review since it’s a required library.

  • Line 69: This is available under guild.get_role in discord.py. (Optional)
  • Line 109: This is blocking in how it’s utilized elsewhere in your cogs. A very large json file could take a long time to write. Consider using cores Config to handle this for you.
  • Line 190-333: You should give proper attribution to where you’re borrowing code from https://github.com/Awoonar/Dusty-Cogs/blob/master/menu/menu.py
  • Lines 506-514: More duplication of Red functions already available.
  • Lines 585-591: While not super critical as these aren’t used in any of your cogs here it is worth mentioning that you have zero cleanup for the ThreadPoolExecutor meaning that if anything is run in this and something happens say a cog reload or bot shuts down this executor will not stop functioning properly. Consider using with concurrent.futures.ThreadPoolExecutor(max_workers=1): inside run_in_loop instead to automatically handle closing the ThreadPoolExecutor when tasks are complete.
  • Lines 621-639: You might consider cleaning this up by using reaction predicates in redbot.core.utils.predicates https://docs.discord.red/en/stable/framework_utils.html#event-predicates (Optional)

automod2

automod2.py

  • Missing class docstrings.
  • You should not be pulling from globals() for something inside a cog.
  • Line 631: This is not a safe method for allowing user defined regular expressions. For example the following pattern is valid ^(?:a+)+$/, however if a user sends a message with about 1000 of the letter a in a row the bot will hang and fail to continue processing any actions. This is only one example where this can occur I would recommend you look into reDOS for more information on this type of attack. It’s crucial that this is dealt with since not everyone is familiar with regular expressions enough to know how to avoid these types of scenarios. If you want so guidance on how you might look to my rettrigger cog which utilizes one approach to solve the problem of user defined regular expressions by using multiprocessing threads. Another alternative is to figure out how to install and setup re2.
  • Lines 302, 306, 628: You should not use bare excepts, preferably you use except Exception or catch exceptions you know will occur. (Optional)

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

baduser

baduser.py

  • Missing class docstrings.
  • Line 18, 188, 196: This is available under guild.get_role in discord.py. (Optional)
  • Lines 182, 190, 198: You assign variable e but never use it. (Optional)
  • Lines 291, 504: You should not use bare excepts, preferably you use except Exception or catch exceptions you know will occur. (Optional)
  • Line 368: Making opt-out owner only is not a good idea here, server may want to opt-out later from utilizing this and they won’t be able to themselves.
  • Missing docstrings for addban, rmban, opt_in, and opt_out.

info.json

  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.
  • Your end user data statement is incorrect for what this cog does. It stores user messages temporarily and potentially permanently depending on what is said. You need to be up-front about what data you’re actually tracking in the end user data statement since if a user requests this data they should feel confident that their data is released or if they request their data they should know that this cog does store their messages and furthermore not just their messages but everyones messages are stored temporarily until a command is called to make those messages permanent.

calculator

calculator.py

  • Missing class docstrings.
  • Line 48: BytesIO is called but never imported.
  • Line 50-52: You never store anything in the user field so this is unnecessary. (Optional)
  • Line 58: This will fail if someone has blocked the bot or otherwise has DM’s disabled.
  • Line 86: This is blocking, one or more people could run this command across multiple channels or servers and prevent the bot from executing any other commands for the 2 seconds this is processing for large calculations.

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

channelmod

channelmod.py

  • Line 12: Importing all of tsutils here leaves the previously mentioned ThreadPoolExecutor open with zero cleanup of said ThreadPoolExecutor even though it’s not used.
  • Lines 154-156: This can be avoided by using the on_message_without_command listener. (Optional)
  • Line 179-184: Considering this is sent on_message this should use discord.py’s session which accounts for rate limits when accessing attachments on discords CDN. This is done by doing attachment_bytes = await attachment.read() or alternatively you can save to a discord.File object with attachment_file = await attachment.to_file()
  • Some important things need to be understood about what this cog is attempting to do. While the cog does not locally store anything end user related the cog is silently re-uploading user content elsewhere which is not good. While general message logging for the most part is fine logging attachements is something I strongly don’t agree with doing especially when its downloading and re-uploading. A user could join a server and start posting something grotesque or illegal and this cog will re-upload that content potentially leaving the bot owner liable for re-sharing said illegal content. If you must log the attachment provide the link to said attachment where if it’s deleted by the owner of the attachment or by some other means it’s not copied elsewhere.

info.json

  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

datatransfer

datatransfer.py

  • Missing doc strings for the following commands: import alias, export alias, import customcom, export customcom, import memes, expoort memes.

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

donations

donations.py

  • Lines 74, 81: You should not use bare excepts, preferably you use except Exception or catch exceptions you know will occur. (Optional)
  • Line 137: You might consider using from redbot.core.utils.chat_formatting import escape to do escape(msg, mass_mentions=True) instead. alternatively by default with Red 3.4.0 all but user mentions are disallowed with the allowed_mentions parameter by doing await channel.send(msg, allowed_mentions=discord.AllowedMentions(everyone=False, roles=False, users=True)) (Optional)

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.
  • Line 9: This causes an AttributeError the first time the cog is loaded on a bot.

fancysay

fancysay.py

  • Missing class docstrings.
  • Line 106 and 112: There’s no check here for the maximum length of the title and footer which will error with no response making the user think the command doesn’t work at all.
  • Line 110: This will fail if the provided image is not a valid URL also erroring with no response making the user think the command doesn’t work at all.

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

globaladmin

globaladmin.py

  • Missing class docstrings.

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

globalban

globalban.py

  • Missing class docstrings.
  • There needs to be a warning under optin that tells the user that this command will immediately synchronize all bans in the bots global ban settings. This could also potentially cause a mod to be banned from their own server when enabled.
  • Similarly with optin the optout needs to have warning that this will unban anyone who is also in the global ban settings.
  • Line 57-62: There’s no checks here that you’re passing an int only which will cause the update to fail all subsequent updates for each guild.
  • Line 79: If a guild is opted in and the bot has its ban members permission revoked or the bot has been removed from the server this will raise errors.
  • Line 92: This should use datetime.datetime.now(datetime.timezone.utc) in order to accurately reflect UTC time in modlog.

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json
  • install_msg should explain how this cog works and that it is very API intense. For example if this cog was used on the Official Fortnite server to sync bans between all the Fortnite servers you would be waiting indefinitely just to get the bans and then waiting who knows how long for rate limit wait times to apply mass banning across each server.

memes

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

memes.py

  • Lines 62, 83, 105: This is blocking, if a lot of memes are saved the bot will spend a lot of time writing the json file here before continuing on with other tasks. Consider using cores config for saving instead although considering this entire cog is just a copy of cores customcom I don’t see a reason for it being here.
  • Line 160: This is available under guild.get_role in discord.py. (Optional)
  • Lines 190-212: You need to supply proper attribution for borrowed code. https://github.com/Cog-Creators/Red-DiscordBot/blob/V3/develop/redbot/cogs/customcom/customcom.py#L734-L758

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

modnotes

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

modnotes.py

  • Missing class docstrings.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

modtools

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

modtools.py

  • Missing class docstrings.
  • The revertname command will fail if the bot does not have manage nicknames permission. This is also an available slash command in discord by doing /nick.
  • Missing docstrings for onlinecount and servercount commands.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

msgutils

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

msgutils.py

  • Missing class docstrings.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

playground

info.json

  • hidden or disabled should be set to True if this cog is not meant to be used.
  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

playground.py

  • Missing class docstrings.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

selfroleoverride

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

selfroleoverride.py

  • Lines 87, 100, 111: Redefining self here is not ideal, consder calling it admin_cog or something else instead. (Optional)

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

seniority

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

seniority.py

  • Line 243, 383: This is available under guild.get_role in discord.py. (Optional)
  • Line 558: You should consider using role_name: discord.Role here to automatically find the appropriate role object. (Optional)
  • seniority config role command needs a check that the issuer of the command is not attempting to assign a role higher than their own in the hierarchy. For example any mod with manage guild permission can set a role which is higher than their current role but lower than the bots role granting themselves or others permissions they’re not supposed to have.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

speech

info.json

  • You should have instructions on how to get and set the API keys for this cog.
  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

speech.py

  • The google-api library is not async and therefore blocking every time it is called.

__init__.py

sqlactivitylog

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

sqlactivitylog.py

  • Lines 173, 416, 420: This is blocking every time the cog loads, a message is edited, and a message is deleted.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

stickers

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

stickers.py

  • Line 29: You should avoid using global variables like this. (Optional)

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

streamcopy

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

streamcopy.py

  • Missing class docstrings.
  • Line 134: This is available under guild.get_role in discord.py. (Optional)
  • Commands streamcopy setStreamerRole, streamcopy clearStreamerRole, streamcopy addUser, streamcopy rmUser, streamcopy list, and streamcopy refresh are all missing docstrings.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.
  • Line 8: This listener needs to be manually removed on cog unload when added like this.

timecog

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

timecog.py

  • There should be some cleanup for the reminder loop task on cog_unload otherwise you might end up with tasks being repeated.
  • Line 462: This will crash the loop if the user has blocked the bot.
  • Line 471: This will crash the loop if the channel has been deleted or otherwise unavailable.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

todo

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

todo.py

  • Missing class docstrings.
  • Line 6: Importing all of tsutils here leaves the previously mentioned ThreadPoolExecutor open with zero cleanup of said ThreadPoolExecutor even though it’s not used.

translate

info.json

  • You need to have some instructions on how to acquire and set the API keys required for this cog.
  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

translate.py

  • The google-api library is not async and therefore blocking every time it’s called.
  • Commands translate getgkey and translate getakey require docstrings.

__init__.py

trigger

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

trigger.py

  • Line 597: See previously mentioned issue about user defined regular expressions.
  • save_stats needs to be canceled in cog_unload in order for this task to be cancelled properly.

__init__.py

trutils

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

trutils.py

  • Missing class docstrings.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

voicerole

info.json

  • install_msg should not be empty.
  • require_cogs is not a valid key for info.json

voicerole.py

  • Line 48: This is available under guild.get_role in discord.py. (Optional)
  • Line 54: You should consider using role_name: discord.Role here to automatically find the appropriate role object. (Optional)
  • Command voicerole set needs to have a hierarchy check so that an admin cannot set roles higher than their own but lower than the bots role and gain access to permissions they don’t normally have.

__init__.py

  • Line 1: You should not use * imports especially when the only thing you require in this init is the class for the cog.

Some general comments:
Your usage of your own custom backend for storage is mostly fine and we don’t have any problems with that in general but it is highly recommended to use cores config setup which allows for more than just json backends for bots that require different backend structures. From what I saw the config system you built will work fine for smaller bots but once the json files start getting larger will be considered blocking and you need to have a warning in any cogs that utilize it that they’re not using cores config setup.

You have a lot of inconsistencies in coding style and usage of utilities while I don’t qualify based on coding style I felt I needed to mention it as you’ve duplicated a lot of code already built to make your life easier writing cogs. Beyond duplication of code inconsistencies in the coding style are rampant throghout these cogs.

It seems that you have a misunderstanding of how asynchronous programming works and I would encourage you to research and learn from other cog creators. Blocking code was found throughout many cogs you have here and it’s cruicial that it’s considered when we have bots that range from 1-2 guilds all the way up to the tens of thousands that might want to use some of these cogs. If a command or function is not treated with async in mind those bots will slow to a crawl trying to deal with the thousands to millions of users they’re serving.

Why I am rejecting this application:
I have found more than 1 count of you copying other people’s code without providing attribution for said code. This combined with the sheer amount of required changes means I will be rejecting this application and you may not re-apply for 1 month from the time of this review and all non-optional review points have been addressed.

I would encourage you to spend this time participating more in the Red server’s #coding channel and feel free to ask questions there about how things in discord.py and Red work.

#4
#5