[APPROVED] OB13-Cogs

Applications
#1

Discord Name: Obi-Wan3#0003

GitHub Repository (Must be V3): https://github.com/Obi-Wan3/OB13-Cogs

Description: Assorted utility cogs that I have created over the past few months.

#2
#3

Hi Obi-Wan3, thank you for your patience while I review your repo.

Commit hash at time of review - 578364513502be320f9dc19b690a003521c74f2d

brainshop

brainshop.py

  • Line 172: I don’t think it’s wise to even allow the bot owner to post the API key publicly like this only saved by spoilers. Users may have spoilers disabled entirely making the attempt entirely moot. I would rather see this replaced with something else like it’s been set and then if the owner really wants it hide it behind a reaction predicate or an optional command arg to display it and make sure they know the risks. (Optional)

counting

counting.py

  • Line 58 and 139: This will never be reached since you already compare message.channel.id != counting_channel If counting_channel is None then message.channel.id will never be equal. (Optional)
  • Line 162: I recommend adding an additional permission check rather than only checking mod role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.mod_or_permissions(manage_messages=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Line 186: You need to check that the user setting the role here has permission to assign said role in the hierarchy otherwise this could be used to escalate permissions within a server. Additionally this needs to be guarded only to people who have manage_roles permission.
  • Lines 224 and 226: This will error the command if the channel or role is deleted.

createchannels

createchannels.py

  • Line 100: [p]createvoice Needs to have additional checks for manage_channels. The @commands.bot_has_permissions(manage_channels=True) only checks for channel specific permissions. You need to also check inside the function for ctx.guild.me.guild_permissions.manage_channels.
  • Line 160: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Lines 217 and 310: This will error if any of the previous roles setup have been deleted.

directmessage

directmessage.py

embedreact

embedreact.py

  • Line 33: I’m curious if you really intend for this to work with ftp links which don’t event create hyperlinks inside discord. (Optional)
  • Line 33: It is best practice to escape the // with \/\/ otherwise you may get warnings from future python versions about issues compiling this regex. (Optional)
  • Line 132: This will error if any of the channels setup have been deleted.

emojitools

emojitools.py

  • Lines 455-518: The commands [p]emojitools tozip should have checks that the file size is under the limit of the server it’s sending to. While it’s unlikely to encounter a set of emojis that are zipped and larger thant the server’s upload limit, it is possible when you consider there was a brief period of about a week where there was no size limit on discord emojis and people uploaded very large emojis in that time, some already compressed as much as possible.

github

github.py

  • Line 356: I don’t know if it’s entirely an issue here but you should consider putting anything that should be waited before a loop in a before_loop function. E.g.
@tasks.loop(minutes=3)
async def _github_rss(self):
    # Your code
    ...


@_github_rss.before_loop
async def _before_github_rss(self):
    await self.bot.wait_until_red_ready()

This is listed under the discord.py tasks API documentation under “Waiting until the bot is ready before the loop starts” https://discordpy.readthedocs.io/en/latest/ext/tasks/index.html#recipes

  • Lines 114 and 277: The [p]github setchannel and [p]github channel commands should also check that the designated channel has permission to send and embed links before saving. Otherwise someone could setup a channel with incorrect permissions and not know why it’s not working.
  • Lines 382 and 388: You should be checking that you actually have permission to send a message and embed links in the channel otherwise this could fill people’s logs with error messages.

info.json

  • Line 10: Your min_bot_version is too low, since you’re using get_or_fetch_user which was introduced in Red 3.4.1.

improvtime

improvtime.py

  • Line 66: You should be checking that you have permission to read message history here otherwise this will fill people’s logs with error messages.
  • Line 69: You should be checking that you have permission to manage messages here otherwise this will fill people’s logs with error messages.
  • Line 95: You should be checking that you actually have permission to send a message in the destination otherwise this could fill people’s logs with error messages.
  • Line 171: This will error if any of the channels setup were deleted.

mentionhelp

mentionhelp.py

  • Line 77: You should be checking that you actually have permission to send a message in the destination otherwise this could fill people’s logs with error messages.

privaterooms

privaterooms.py

  • Lines 129, 136, 150, 154, 182, 201: You should be checking that you actually have permission to manage this channel otherwise this could fill people’s logs with error messages.
  • Lines 85, 141, 159, 168, 208: You should be checking that you actually have permission to send messages in this channel otherwise this could fill people’s logs with error messages.
  • Line 221: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Line 233: In the [p]privaterooms add command you should be checking that you actually have manage channels permission either on a guild level or within the designated category channels so that users setting this up are not confused why it doesn’t work for them after setting it up.
  • Line 320: In the [p]privaterooms logchannel command you should be checking that the provided channel if present has correct permissions for the bot to send to so that once it’s setup users aren’t confused why the logs aren’t working.
  • Lines 378, 379, 382: These will error the command if any of hte channels were deleted.

publicrooms

publicrooms.py

  • Lines 116, 146, 151, 185, 213, 220: You should be checking that you actually have permission to manage this channel otherwise this could fill people’s logs with error messages.
  • Lines 118, 158, 167, 233, 245: You should be checking that you actually have permission to send messages in this channel otherwise this could fill people’s logs with error messages.
  • Line 253: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Line 265: In the [p]publicrooms add command you should be checking that you actually have manage channels permission either on a guild level or within the designated category channels so that users setting this up are not confused why it doesn’t work for them after setting it up.
  • Line 340: In the [p]publicrooms logchannel command you should be checking that the provided channel if present has correct permissions for the bot to send to so that once it’s setup users aren’t confused why the logs aren’t working.
  • Lines 439, 442: These will error if any of the channels were deleted.

quizrole

quizrole.py

  • Line 63: You should check that you have permission to delete a users message before attempting this.
  • Line 65-66: This is unnecessary since you can just call await user.send(message) https://discordpy.readthedocs.io/en/latest/api.html#discord.User.create_dm
  • Line 67-68: This will not error with discord.errors.Forbidden in fact you can create a DM with anyone even if you can’t DM them. You also don’t return after sending the message that the user does not have DMs enabled meaning further commands will all fail with an ugly message.
  • Lines 84-86: You should use if cond is True or if cond is False not if cond == True. (Optional)
  • Line 94: You should be checking that the role is valid and still exists here otherwise you will get AttributeError trying to get role.name.
  • Lines 95-122: Your usage of MessagePredicate is almost right here although it might be simpler to do this:
pred = MessagePredicate.lower_contained_in(['cancel', 'start'], channel=ctx.author.dm_channel, user=ctx.author)
try:
    await self.bot.wait_for("message", check=pred, timeout=60)
except asyncio.TimeoutError:
    return await ctx.author.send("The operation has timed out. Please try again.")
if pred.result == 0:  # pred.result returns the index number of the item in the collection
    return await ctx.author.send("Quiz cancelled.")
else:
    # continue with the quizz
    pass

You can work this in with the entire quiz although I will make this an optional change. (Optional)

  • Line 125: You need to check that the bot has permission to add this role still here.
  • Lines 127 and 131: You need to check that the bot still has permission to send messages to the loog channel.
  • Lines 171-178: You need to check that the bot has permission to send messages in the channel being set here.
  • Line 160: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Lines 181 and 244: In commands [p]quizroleset newquiz and [p]quizroleset edit you need to check that the bot actually has permissions to manage roles.
  • Line 212: You might consider allowing server owner with no roles to also allow this command to be run. (Optional)
  • Lines 147-148: This will error if any of the roles setup were deleted.
  • Lines 402-403: This will error if any of the roles setup were deleted.

referrals

referrals.py

  • Lines 65, 80, 87, 98: You should be checking that the bot has permission to send messages in this channel first.
  • Line 147: In command [p]referset logchannel you should be checking that the bot has permission to send messages in this channel.
  • Line 103: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Line 169: You might consider using AsyncIter here because iterating through a server like Red with over 10000 members can take a lot of time and potentially block the bot from responding in other servers until this function is done. (Optional)
  • Line 207: This will error if the log channel was deleted.

reply

reply.py

  • Line 48: You can avoid this error by checking that the message.channel.id == ctx.channel.id (Optional)

restrictedroleperms

restrictedroleperms.py

  • Lines 77, 87, 89, 103, 113, 115, 129, 139, 141, 155, 168, 170, 204, 208, 232, 234, 236, 310, 321: You might consider adding allowed_mentions=discord.AllowedMentions(roles=False) to this so you’re not mass pinging people who are using these commands. This will also increase min_bot_version to 3.4.1. (Optional)
  • Line 172: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Lines 193, 221: This will error if any of the roles previously setup were deleted.

sitestatus

sitestatus.py

  • Line 69: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Line 220: You should consider putting anything that should be waited before a loop in a before_loop function. E.g.
@tasks.loop(minutes=3)
async def _fetch_statuses(self):
    # Your code
    ...


@_fetch_statuses.before_loop
async def _before_fetch_statuses(self):
    await self.bot.wait_until_red_ready()

This is listed under the discord.py tasks API documentation under “Waiting until the bot is ready before the loop starts” https://discordpy.readthedocs.io/en/latest/ext/tasks/index.html#recipes

  • Line 240-244: You assigned online in self._fill_template for the offline_fille variable. I am not sure if this was intentional or not but now offline is an unused variable. (Optional)

statusrole

statusrole.py

#4
  • Line 190: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Lines 142, 202: User defined regex can be dangerous to run in an async environment because it can lead to catastrophic backtracking or reDOS attacks. A server admin could add a pattern that can take hours to actually parse through which would essentially lockup the bot. I recommend either making the regex part owner only or emplying some method to mitigate this such as Facebook’s RE2 module which is incredibly difficult to install or using process pools if you absolutely need this to be regex. This also runs every time a users activity changes which includes songs changing in spotify.
  • Line 341: This will error if any of the roles setup have been deleted.

streamrole

streamrole.py

  • Line 91: I recommend adding an additional permission check rather than only checking admin role. 90% of the time the server owner may not be aware to setup the bots admin and mod roles which would lock this functionality to only server owner and bot owner. Doing @commands.admin_or_permissions(administrator=True) will allow these commands to function even if the server owner has not setup mod and admin roles in the server prior to starting. (Optional)
  • Lines 148 and 153: these will error if any of the roles or channels have been deleted.

templateposts

templateposts.py

  • Line 104: You should be checking that you have permission to send messages in this channel yourself.
  • Line 131 and 185: You might consider checking that you have your required permissions for the channel set here. Because you’re catching exceptions this is optional. (Optional)
  • Line 269, 276: These will error if any of the roles or channels have been delted.

temprole

temprole.py

  • Line 82: I would recommend adding allowed_mentions=discord.AllowedMentions(roles=False) here or using role.name so that you don’t have accidental mass pinging. (Optional)
  • Line 84: You should also be checking that the role being assigned is not higher than the bots role to prevent error messages.
  • Line 144: You might consider using from redbot.core.utils.chat_formatting import humanize_timedelta here to display a nice user friendly time until. (Optional)
  • Line 157 and 163: This will error if any of the roles have been deleted or the member is no longer in the guild.
  • Line 172: You should be checking in this command that you actually have permission to send messages in the log channel first.
  • Lines 177-183: You should be checking that you have permissions to send messages in the log channel otherwise you could end up with several error messages cascading from failed send messages.

translate

translate.py

  • Line 24: This library is blocking. I would recommend you run anything using it in executor or find a library that uses aiohttp or httpx which are non-blocking web call libraries.

uploadstreaks

uploadstreaks.py

  • Lines 66, 72, 73, 74, 180, 183, 200, 202, 383: I recommend using timezone aware timestamps they’ll act more consistantly across systems than these naieve ones you’re using. (Optional)
  • Line 385 and 386: This will error with NoneType object has no attribute 'mention' if the roles or channels have been deleted.

Some things I wanted to comment on in general. You tend to use ctx.tick() a LOT which is fine although it often gives little feedback to users on what actually changed. It’s nicer to know what the setting was changed to without having to dig for another command to see what the settings are now. This is also somewhat problematic where Red doesn’t guarantee the add_reactions permission for bots and we barely guarantee send messages permission but in this case ctx.tick() will fail silently if no permission to add reactions is present leaving users unsure if the command actually functioned if their permission structure prevents the bot from adding reactions. These things make it difficult for you to try and diagnose issues with why your cog may not be working for users. I would also recommend looking at setting up proper logging for things. You have channels setup for a lot of cogs to log error messages, etc. but there’s no checks that the logging channel actually exists or that the bot even has permission to send messages in it which in some cases lead to cascading errors that you won’t exactly know how to fix.

I know it seems like there’s a lot of issues here but they should be relatively straight forward and many of them are the same issue between cogs so once you solve the problem in one it’s simply following the same idea in the next.

1 Like
#5

Hi Trusty, thank you so much for your review! I have made the changes on my dev branch (all of the required changes and the majority of the optional changes); please let me know if anything is missing or seems incorrect (I will push to main once it is confirmed that everything seems valid). For the ctx.tick() issue, I have decided to stick with using that instead of adding individual confirmation messages for quite a few of the commands. I will definitely look further into logger in the future, but for now I have added checks for send message permissions to the in-Discord log channels for my cogs. For StatusRole, I decided to go with the core Filter approach and removed the user-set regex feature.

#6

Thanks for making these changes. Once they’re on your main branch I am happy to mark this as approved!

#7
#8