Description: 6 cogs (for the moment) for the redbot which are more for moderation and utility. One allows to gather all possible sanction actions (userinfo, warn, ban, softban, tempban, kick, mute, mutechannel, tempmute, tempmutechannel) in a same menu with buttons (+ slash command + contextual menus), another one allows to make as if a command had been placed in another channel (based on mock, soon in another server), another one to clean up a whole channel by cloning it, etc.
All cogs are tested and working so far.
Thank you in advance for approving my repo.
Once your repo is updated to support 3.5 on the default branch, please make a comment in this thread to confirm you are once again ready for review. We will only be reviewing repos which comment to confirm 3.5 support, but we will prioritize repos that have been in the queue for a longer period of time first.
If you wish to revoke your application, please comment accordingly so we can clean up the thread. If you need time to make changes, but are actively working on making updates, please also comment accordingly. Applications which do not see any activity within 1 month from today will be closed.
So I confirm that all my cogs will work under dpy2! Since yesterday, the accounting with dpy1 has been removed from my repo and my utils are now in a PIP package, in line with the comments made.
Update to the original post: My repo now has 31 cogs, including one disabled on the Index (Medicat for the Discord server of the same name) and one hidden (AntiNuke in favor of NoNuke by Vert, even though I still make global changes to my cogs and it is functional). All my cogs use utils (GitHub - AAA3A-AAA3A/AAA3A_utils) as a PIP package.
Thanks in advance for the review,
Thanks for the Red project,
Have a nice day,
AAA3A
Commit hash: 6402a929145489c0556e1ae54f6e4f04da0f69ce unless otherwise noted Red v3.5.1 Consider only bullets prefixed with to be required.
General
You should check bot.cog_disabled_in_guild in your listeners.
CogUtilscheck_permissions_for has a subtle bug that impacts all places where it is used to validate permissions. The return value ands a check that the attribute actually exists with a check that the setting matches the requested setting. However, the check for whether the attribute actually exists does not properly check whether the value is falsey because of non-existence or because of a falsey permissions value, and as such incorrectly returns that the permission is correct in cases where the value is actually False but is expected to be True. The following code fixes this issue, however I would strongly advise instead breaking this into a proper loop that is more easy to read.
return not any(
getattr(permissions, f"{p}", None) is not None # Explicitly check whether the value is None
and (
check[p]
and not getattr(permissions, f"{p}")
or not check[p]
and getattr(permissions, f"{p}")
)
for p in check
)
In many cases where this function is used, the @commands.bot_has_permissions decorator would be strictly better.
I see you use Greedy frequently for the last argument of a command, when it needs to match multiple times. Using Greedy this way leads to bad UX, as if any part of the input is malformed it will silently exit early, rather than telling the user that a part of the command was not processable. Instead, consider using *arg to achieve similar behavior while still requiring successful conversions.
The bot starts typing for all commands, but it doesn’t stop immediately when many of the commands complete as no message is sent when it is done. Instead, a tick is added, which doesn’t stop typing.
Cogs that use your settings generics (IE clearchannel, getdocs) have the following issues:
[p]help <settingscommand> <subcommand> has what appears to be the argument type included in the message, but as a raw repr of the class.
Clicking the Configure button in [p]<settingscommand> modalconfig raises the following error:
Traceback (most recent call last):
File "datapath\cogs\Downloader\lib\AAA3A_utils\views.py", line 217, in interaction_check
self.function_result = await self.function(self, interaction, **self.function_kwargs)
File "datapath\cogs\Downloader\lib\AAA3A_utils\settings.py", line 1014, in on_button
await interaction.response.send_modal(view_modal)
discord.errors.InteractionResponded: This interaction has already been responded to before
You don’t have any per-setting bounds configured to restrict config settings to sane values, meaning that for some converters like int you can enter values that break the cogs (IE negative ints).
You’re using the config internal _get_base_group rather than the public all_guilds method. I assume the reason for this is because you need to use a context manager, however I would advise avoiding privates where possible.
You might want to go through your listeners and make sure that allowed_by_whitelist_blacklist and cog_disabled_in_guild are used appropriately in all of them.
Because your utils version the cog expects is not baked into the info.json, and is checked later at cog load, you cannot use [p]cog installversion with your repo without manually finding the commit of the utils with the version the cog expects and pip installing that as well. Trying to install a cog this way always updates the lib to the latest version, regardless of what version the cog would otherwise want to have.
Running the command [p]calc in a channel without embed_links permissions for the bot raises this error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\calculator\calculator.py", line 186, in _calculate
await CalculatorView(cog=self).start(ctx)
File "datapath\cogs\CogManager\cogs\calculator\view.py", line 180, in start
self._message: discord.Message = await self.ctx.send(
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
Clicking the button for “ln” and passing “5” with it raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\calculator\view.py", line 217, in _callback
self._result = f"{await self.cog.calculate(self._expression)}"
File "datapath\cogs\CogManager\cogs\calculator\calculator.py", line 95, in calculate
result = evaluate(expression)
File "datapath\cogs\Downloader\lib\expr\core.py", line 25, in evaluate
return state.evaluate(expr, cls=cls)
File "datapath\cogs\Downloader\lib\expr\parser.py", line 269, in evaluate
result = parser.parse(lexer.lex(expr), state=self)
File "datapath\cogs\Downloader\lib\rply\parser.py", line 22, in parse
current_state = self._reduce_production(
File "datapath\cogs\Downloader\lib\rply\parser.py", line 82, in _reduce_production
value = p.func(state, targ)
File "datapath\cogs\Downloader\lib\expr\parser.py", line 225, in function
return Mul(self.getvar(p[0]), p[2]) # Probably this instead
File "datapath\cogs\Downloader\lib\expr\parser.py", line 233, in getvar
name = p[0].getstr()
TypeError: 'Token' object is not subscriptable
Visually, the calculator treats this as In (capital i) rather than ln (lowercase L). I assume this is related.
I can reproduce this error by passing any input following the structure <capital letter>(<number>) IE A(5), so it seems like this might be a bug with that lib with certain input.
The same error also happens when using the |x| option.
Your own output produces numbers formatted with commas for numbers larger than 1,000. I understand why you are replacing commas with periods when processing, but simply copy pasting a result of a calc to use in a future calc makes it interpret it incorrectly as a result. Reconsider either how you display the results or choosing to replace commas with periods.
Consider using the variables kwarg to add custom variables to your expression, rather than replacing in the input string manually.
Blanket replacing numbers after ^ characters with the unicode power equivalent makes it so powers greater than 9 require explicit parentheses to enter (for example, 2^10 must be entered as 2^(10))
ClearChannel
If you want help text to not be included in short, but be included in description (so the extended description part does not get bolded), you need to have two newlines after the first line. [p]clearchannel looks like that may have been the intention.
CmdChannel
The docstrings imply that the channel/user are optional, but they are actually required.
CodeSnippets
Because of a combination of your Cog.after_invoke always calling ctx.tick, your Context.tick falling back to sending Done. if no messages are sent, and [p]codesnippets creating a task to send its message rather than awaiting it, that command sends Done. in addition to the expected code on success.
CommandsButtons
Consider adding a custom usage kwarg to your command deco for [p]commandsbuttons add to make the style arg more readable.
Currently [p]commandsbuttons add requires an emoji, and has an optional label. Strictly speaking, an emoji or a label is required, and you may want to make your command support just either.
[p]commandsbuttons add can be used on messages with views that were not previously managed by this cog, overriding them.
[p]commandsbuttons add complains about missing message perms no matter what permission is actually missing, including add_reactions.
Consider using the check decorators instead of a check built into the command.
[p]commandsbuttons remove raises the following exception after removing the last button from a message:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\commandsbuttons\commandsbuttons.py", line 317, in remove
self.cogsutils.views.append(view)
UnboundLocalError: local variable 'view' referenced before assignment
CtxVar
Using [p]ctxvar ctx in a channel without embed_links permissions raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\ctxvar\ctxvar.py", line 106, in ctx
await Menu(pages=embeds).start(ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 127, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
ctxvar.py L38, invoke_without_subcommand=True does nothing. The proper kwarg is invoke_without_command, however this particular group command has no implementation on its own, so using this kwarg would make no sense and just make the command harder to use.
Having Invite so high in the convert order for [p]ctxvar whatis makes it basically impossible to use the following converters with string input. For example, entering Flame just converts to an invite to the guild with that vanity invite.
The cog errors if used in a channel where the bot does not have embed_links permissions:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\dictionary\dictionary.py", line 86, in dictionary
await DictionaryView(cog=self, word=word).start(ctx)
File "datapath\cogs\CogManager\cogs\dictionary\view.py", line 38, in start
self._message: discord.Message = await self.ctx.send(embed=embed, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 146, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
The Phonetics button silently errors if used in a channel where the bot does not have attach_files permissions:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\dictionary\view.py", line 119, in show_phonetics
await Menu(pages=[{"embed": embed, "files": files}]).start(self.ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 132, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 146, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
DiscordEdit
EditAutoMod is not included in the subclassed bases despite being advertised as a feature, not sure if this was forgotten or if there’s a real reason why it’s not there.
Running a command that requires a confirmation in a channel without embed_links permissions causes the message to be sent as just the mention, which makes the confirmation very confusing.
Similarly, listing commands raise the following error when the bot does not have embed_links permissions:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\discordedit\editrole.py", line 126, in editrole_list
await Menu(pages=embeds).start(ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 127, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
editguild and editrole have explicit bot permissions checks, but the other 3 do not. These permission errors are caught by the cog itself, so it’s not an issue to do it either way, but it should probably be consistent across the cog.
[p]edittextchannel permissions can currently be used to perform multiple kinds of permissions escalations. It should require manage_roles, the permission that is actually needed to modify the permission overrides in a channel, rather than manage_channel. Additionally, the client only allows you to modify the overrides for permissions that you yourself have, but the command allows any permissions to be edited without explicit checks for each permission.
This also applies to [p]editvoicechannel
DiscordModals
The permissions checks when sending the modal results fail due to the check_permissions_for bug listed above.
The word “argument” is missing an “r” in many of your converter fail messages.
[p]discordmodals add can be used on messages with views that were not previously managed by this cog, overriding them.
Passing invalid data to the emoji field doesn’t have a custom error message
DiscordSearch
Running [p]discordsearch in a channel where the bot does not have embed_links permissions raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\discordsearch\discordsearch.py", line 218, in discordsearch
await Menu(pages=embeds).start(ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 127, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
You need to add logic to prevent catastrophic backtracking in your regex handling. Currently, the regex (A+A+)+B applied on a single message that is 2000 As entirely crashes the bot attempting to match. Since this command can be used by users other than the bot owner, this is a stability problem. I suggest looking at how Trusty safely handles user-inputted regex in retrigger.
The limit argument acts a bit strangely, it limits the number of messages searched, rather than the number returned.
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\draw\__init__.py", line 37, in <module>
from .draw import Draw
File "datapath\cogs\CogManager\cogs\draw\draw.py", line 150
background: typing.Literal[*MAIN_COLORS] = MAIN_COLORS[-1],
^
SyntaxError: invalid syntax
I assume this is due to the use of typehint syntax such as typing.Literal[*MAIN_COLORS], and the fact that my bot is on Python 3.9.6. This cog either needs to lock its python version to ones which support this syntax, or to rework its type hints to not cause this issue on lower python versions.
DropdownsTexts
Consider expanding the docstring of [p]dropdownstexts add to explain what the arguments mean. The difference between what “message”, “label”, and “text” refer to is not initially clear.
Currently [p]dropdownstexts add requires an emoji, and has an optional label. Strictly speaking, an emoji or a label is required, and you may want to make your command support just either.
[p]dropdownstexts add can be used on messages with views that were not previously managed by this cog, overriding them.
[p]dropdownstexts add complains about missing message perms no matter what permission is actually missing, including add_reactions.
Consider using the check decorators instead of a check built into the command.
Using a different emoji but the same label as an existing dropdown raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\dropdownstexts\dropdownstexts.py", line 201, in add
await message.edit(view=view)
discord.errors.HTTPException: 400 Bad Request (error code: 50035): Invalid Form Body
In components.0.components.0.options.1: The specified option value is already used
EditFile
There need to be stronger warnings in the instal_msg (and possibly also command docstrings) that this cog (though locked to the bot owner) is very dangerous, since it allows direct read/write/delete of files on the bot’s machine. Considering the fact that reading the wrong file can expose sensitive information like tokens and deleting the wrong file can corrupt the bot or the system entirely, the potential danger of using this cog needs to be stated more clearly.
The listed reasons for why a datapath may not exist both have potential inaccuracies. “data directory removed” happens if the cog has a Config object stored under the attribute name config, however a Config object could exist under a different name, commonly conf. “does not store any data” happens otherwise, however the cog may just not have currently stored any data, and may do so in the future. I think both of these cases should probably just be reduced to always saying “This cog has not yet stored any persistent data in its data folder”, to avoid making incorrect assumptions.
[p]editfile rename says the file was deleted, rather than renamed, on success.
ExportChannel
The docstrings of the subcommands aren’t very descriptive.
The docstring of the group command is grammatically incorrect, export should be exporting.
The messages subcommand seems unnecessary, considering that other subcommands like bot contain a limit argument, all could probably be given a limit argument.
The cog errors if the channel argument provided is a channel the bot does not have read_messages in:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\exportchannel\exportchannel.py", line 137, in all
count_messages, messages, file = await self.export_messages(ctx, channel=channel)
File "datapath\cogs\CogManager\cogs\exportchannel\exportchannel.py", line 76, in export_messages
count_messages, messages = await self.get_messages(ctx, channel=channel, **kwargs)
File "datapath\cogs\CogManager\cogs\exportchannel\exportchannel.py", line 57, in get_messages
async for message in channel.history(
discord.errors.Forbidden: 403 Forbidden (error code: 50001): Missing Access
This is due to the bug in check_permissions_for.
The cog errors if the channel the command is used in is a channel the bot does not have attach_files in:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\exportchannel\exportchannel.py", line 138, in all
message = await ctx.send(
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
It might be nice to be able to specify datetimes instead of just actually existing messages in before, after, and between.
GetDocs
Loading this cog brings my CPU usage from ~10% to ~40%, and makes the bot incredibly laggy. I would consider this unreasonably high. Minimum, this needs to be disclosed in the install_msg, however if possible it should not be using this amount of resources.
[p]getdoc lists discord.py as the default, but I believe the default is actually whatever is configured in [p]setgetdocs defaultsource, that value defaulting to discord.py. I think this should be made more clear in the docstring.
The query parameter of [p]getdoc has special meaning for random, however this makes searching all methods from the random stdlib of python harder to search.
When clicking one of the buttons of [p]getdoc to change the display, the same button can be clicked again to return to the original view. However, the button that needs to be clicked still is labeled “Show x”. It might make sense to swap the label to “Hide x” or “Return” when in one of these submenus.
I’m not sure how [p]retfm is currently being sorted, but it doesn’t seem to make much sense. Searching a stdlib like asyncio or random doesn’t group the results nicely by those actually in the same library vs various other results that fuzzymatch.
Using the commands from this cog in a channel without embed_links permission raises an error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\getdocs\getdocs.py", line 347, in rtfm
await Menu(pages=embeds).start(ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 127, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
You should not be using _cogsutils_add_hybrid_commands to modify the arguments of the app command version of your hybrid commands. You should be using the @app_commands decorators to configure the things being configured by that function.
GetLoc
N/A
GistsHandler
Running [p]gist then clicking the “View File Content” button raises the following error in console:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\gistshandler\view.py", line 317, in view_content
if self.file.name.split(".")[-1] != "md":
AttributeError: 'NoneType' object has no attribute 'name'
Running [p]gist in a channel where the bot does not have embed_links permission raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\gistshandler\gistshandler.py", line 68, in gist
await GistsHandlerView(cog=self, gist=gist, file=file).start(ctx)
File "datapath\cogs\CogManager\cogs\gistshandler\view.py", line 217, in start
await self._update()
File "datapath\cogs\CogManager\cogs\gistshandler\view.py", line 263, in _update
self._message: discord.Message = await self.ctx.send(embed=self._embed, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
IP
The code of [p]ip website implies that there is a configuration setting to set the port that gets included in the command output, however no such command exists.
Medicat
This is hidden, so it’s fine, but I’d suggest removing it from your main repo if you don’t intend for it to be installed by users.
MemberPrefix
The cog reacts with an X when returning to the default prefix, which makes it seem like something went wrong at a glance.
The cog overrides the ability for members to use existing prefixes, but also has no good way to modify what prefixes other cogs expect would be contextually usable with code such as await bot.get_prefix(message). There is also no bot owner reset options for individual users. As such, it seems like it may be impossible for a user who set a member prefix then forgot it to figure out what their prefix is without an eval or checking the settings.json file directly.
Additionally, since [p]set serverprefix does not override the --mentionable flag, that exists as one method of getting out of a similar situation for server prefixes. However, your cog does override this flag, making the mention prefix no longer work if a member prefix is set.
There is a hidden command to reset, but it affects an entire guild and is hidden. Something that could target individual members and that is not hidden would be helpful.
Only by reading the code, I can see that mentioning the bot should work only for the memberprefix command, however in practice it does not appear that this is working, nor is it listed as an option anywhere in the cog.
If a user has a member prefix configured, and attempts to use a default prefix to use a command from one of your cogs, the bot enters an infinite typing loop due to the cog superclass making every single command trigger typing.
Your before_invoke method seems to want the author to be a Member, but it assumes access to an object with a .id. It might make more sense to just use member_from_ids instead for your config call, and not worry about getting a member object exactly.
If [p]memberprefix is set to an existing prefix for the bot, the [p]memberprefix command duplicates invocations.
There is no reason to make an entire custom string converter just to be able to Greedy it, rather you can just do the same thing core does and use *prefixes: str.
MemoryGame
Running [p]memorygameleaderboard in a channel where the bot does not have embed_links perms raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\memorygame\memorygame.py", line 204, in memorygameleaderboard
await Menu(pages=embeds).start(ctx)
File "datapath\cogs\Downloader\lib\AAA3A_utils\menus.py", line 127, in start
self._message = await ctx.send(**kwargs, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
Running [p]memorygame in a channel where the bot does not have embed_links perms raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\memorygame\memorygame.py", line 156, in memorygame
await MemoryGameView(
File "datapath\cogs\CogManager\cogs\memorygame\view.py", line 69, in start
self._message: discord.Message = await self.ctx.send(embed=embed, view=self)
File "datapath\cogs\Downloader\lib\AAA3A_utils\\context.py", line 147, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
It doesn’t say how many credits you get, or what determined the amount you got, after a game. It might be nice to output some amount of confirmation that credits were given.
You might want to use "\u200c" instead of a literal zero width space in your code, to make it more clear when looking at your code on an editor that doesn’t explicitly mark that character (namely, github).
Using [p]minecraft getplayerskin/getserver in a channel where the bot does not have attach_files permission raises the following error:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\minecraft\minecraft.py", line 258, in getplayerskin
await ctx.send(embed=embed, files=files)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 146, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
The docstring of [p]minecraft checkplayers is inaccurate, it is a copy paste of [p]minecraft removeserver.
ReactToCommand
The docstring for [p]rtc add could be improved by adding an extra newline after the first line. If two newlines are put after a line, the rest goes into the full help and only the first line is part of the short help. Currently, the entire docstring for this command is in the short help, which looks worse.
The cog doesn’t actually allow you to run commands with the reactions, due to a syntax error in the listener:
When adding a reaction command to a message, the bot doesn’t add the reaction to the message, which makes it less clear that using a reaction on the message will do something. It also means that to get a reaction to be initially there, the person who is using the cog needs to either remember to react before running the command, or accept that the react command will be executed once when adding it.
Since the bot’s reaction being removed from the message causes the command to be unregistered, and [p]rtc remove removes the reaction, it is even stranger that it does not initially react.
[p]rtc add only checks that the bot has add_reaction perms in the message channel, not in the ctx channel where the reaction actually gets added to test if it exists. This causes the default “an error has occurred” response, which isn’t clear about what the actual problem is.
[p]reminder expires docstring is copied from [p]reminders text.
The reason for why a reminder fails to be created is masked by a lot of other failures that are not applicable. I get 6 bullet points of information when only the last (reminder time must be within x minutes from now) is actually blocking the reminder. This is really confusing.
[p]reminder timetips errors when used in a channel where the bot does not have embed_links permissions:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\reminders\reminders.py", line 787, in timetips
await ctx.send(embed=embed)
File "datapath\cogs\Downloader\lib\AAA3A_utils\context.py", line 146, in send
return await self.original_context.send(content=content, **kwargs)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
The command [p]remind no_add_reactions <@470684878107705344> in 1 minute wawa was interpreted as a reminder with content no_add_reactions <@> in wawa and time 18 hours and 55 minutes. It seems like it pulled from both the user id and the in 1 minute literal text when considering the time. Not sure why it pulled the id, or why it pulled time text from a non-continuous block, but I assume that this will be overly greedy in other cases.
If the bot does not have embed_links perms in a channel a [p]remind is sent to, it does not error but it does not include any of the reminder text in the message.
Requiring in for all relative times is annoying. It’s relatively clear whether something will be in or on, based on whether an exact time is given or a relative time is given. Currently however, times like 5 minutes fail to validate, despite obviously being relative.
It’s possible to break the buttons for managing reminders by spawning a submenu, changing the state of the reminder, then attempting to use the previously spawned submenu. These submenus should make no assumptions about the kind of data the reminder will have when they are pressed. In this example, the Remove Repeat Rule button expects there to be at least one rule that can be removed:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\reminders\views.py", line 471, in remove_repeat_rule
await interaction.response.send_modal(RemoveRepeatRuleModal(self))
discord.errors.HTTPException: 400 Bad Request (error code: 50035): Invalid Form Body
In data.components.0.components.0.max_length: int value should be greater than or equal to 1.
RolesButton
The docstring for [p]rolesbuttons add lists names for the colors of the buttons, but only the number version is allowed.
[p]rolesbuttons add only checks that the bot has add_reaction perms in the message channel, not in the ctx channel where the reaction actually gets added to test if it exists. This causes the default “invalid emoji” response, which isn’t clear about what the actual problem is.
The message for adding vs removing the role is inconsistent, only removing shows the role id.
Currently [p]rolesbuttons add requires an emoji, and has an optional label. Strictly speaking, an emoji or a label is required, and you may want to make your command support just either.
[p]rolesbuttons add complains about missing message perms no matter what permission is actually missing, including add_reactions.
Consider using the check decorators instead of a check built into the command.
RunCode
The docstring for [p]help RunCode is inaccurate, it lists the command [p]setrundocs listlanguages instead of [p]setruncode listlanguages.
java does not work with [p]runcode, because a class Mainis automatically defined outside of a file calledMain.java`.
Seen
The cog explicitly filters out command messages from Seen when detecting if a user has been seen recently, which seems counterintuitive to the point of the cog.
[p]seen ignoreme isn’t clear as to what state it is in as it always responds with a check, regardless of if it is starting or stopping tracking.
red_delete_data_for_user does not delete data currently in the cache, only data that has been stored to config.
The [p]sanction docstring is beyond the limit, and gets cut off.
Having the [p]sanction subcommand names be numbers as their main command name seems needlessly unintuitive.
I’m not really sure why some mod actions are using command invokes? warn I understand, but ban really can just be a guild.ban call directly instead.
Sudo
It should be made more clear that this cog makes bot owners unable to be perceived as bot owners in commands while the cog is loaded unless the [p]sudo command is used.
The extra commands from this cog are shoved into the help text of a different command, instead of being listed in the cog help, which makes the function they serve unclear.
TicketTool
You should probably organize the commands in [p]settickettool better, IE put all of the profile related command under a subcommand. Currently, getting 3 pages (with default help settings) of output from one command is quite intimidating.
[p]ticket list is missing a docstring.
[p]settickettool rename uses the same docstring as clone.
The warning message when a profile has not been configured properly says to use [p]ticketset to configure, however this is not a valid alias.
It isn’t clear that the profile needs to be named main in order to allow [p]ticket create to be run without passing an explicit profile name every time (or making an alias to do that).
The cog seems to hit channel PATCH ratelimits very easily. I suspect this has to do with editing the channel’s description, though I am not certain. Creating a ticket, claiming it, and trying to close it over the span of about 1 minute already hits a major rate limit:
[WARNING] discord.http: We are being rate limited. PATCH https://discord.com/api/v10/channels/1117173406262120529 responded with 429. Retrying in 559.16 seconds.
The cog doesn’t properly handle reasons above ~1000 characters in length, which is a fairly reasonably length for a ticket that contains all relevant details:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\tickettool\tickettool.py", line 734, in command_create
await ticket.create()
File "datapath\cogs\CogManager\cogs\tickettool\ticket.py", line 287, in create
await self.channel.edit(topic=topic)
discord.errors.HTTPException: 400 Bad Request (error code: 50035): Invalid Form Body
In topic: Must be 1024 or fewer in length.
The default value for dynamicchannelname uses ticket.id, however this is invalid structure. It doesn’t cause an issue, because the logic for picking the name catches the error that gets raised and uses the same default, but it is technically incorrect.
Due to this handling, the cog silently ignores this setting if the user inputs an incorrect value. It might make sense to make it fail to create the ticket instead, similar to how other misconfigured settings work.
Be very careful with using .format on a user provided string. Currently, all formatted values are builtin types, but if discord.py objects are passed in instead that can cause a permissions escalation.
There’s no option to have no limit to the number of tickets, besides setting the value to a very large number.
It is not clear that additional [p]ticket subcommands will appear inside of a ticket channel.
TransferChannel
The bullet formatting of [p]transferchannel is messed up by the new markdown syntax. You should remove the leading spaces before the dashes.
The docstrings of the subcommands aren’t very descriptive.
The messages subcommand seems unnecessary, considering that other subcommands like bot contain a limit argument, all could probably be given a limit argument.
The cog errors if the channel argument provided is a channel the bot does not have read_messages in:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 204, in all
count_messages, messages = await self.transfer_messages(
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 133, in transfer_messages
count_messages, messages = await self.get_messages(ctx, channel=source, **kwargs)
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 109, in get_messages
async for message in channel.history(
discord.errors.Forbidden: 403 Forbidden (error code: 50001): Missing Access
This is due to the bug in check_permissions_for.
The cog errors if the destination channel does not have embed_links perms for the bot and the messages are formatted as embeds:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 204, in all
count_messages, messages = await self.transfer_messages(
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 139, in transfer_messages
await destination.send(embed=embed)
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
This is due to the fact that check_channels expects way to be embed, when it is actually embeds.
The cog errors if the destination channel does not have attach_files perms for the bot, the messages contain attachments, and the messages are formatted as messages:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 204, in all
count_messages, messages = await self.transfer_messages(
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 160, in transfer_messages
await destination.send(
discord.errors.Forbidden: 403 Forbidden (error code: 50013): Missing Permissions
It might be nice to be able to specify datetimes instead of just actually existing messages in before, after, and between.
URLButtons
Currently [p]urlbuttons add requires an emoji, and has an optional label. Strictly speaking, an emoji or a label is required, and you may want to make your command support just either.
[p]urlbuttons add can be used on messages with views that were not previously managed by this cog, overriding them.
[p]urlbuttons add complains about the emoji being invalid if add_reactions perms are missing.
Thank you very much for such a precise and detailed review!
I’ve corrected and pushed all of these points, required as optional, apart from some optional points that I couldn’t do.
The commit content of all these modifications is: 52bbeef736614a2e796c92864b76d1deb5e0bdd4
Most of the review comments have been addressed, however there are still a few outstanding issues. Most cogs are omitted from the list, as they require no further changes.
Commit hash: 8026426383592545ab0383a9d7bcc754eedb7939 Red v3.5.2 Consider only bullets prefixed with to be required.
General
I would suggest against putting default overrides for red_delete_data_for_user and red_get_data_for_user in your cog superclass, as that misses the point of needing to explicitly override the methods to declare that the cog does not store data. The core provided default is how it is for a reason. You are risking a cog that does not store data forgetting to implement these methods, and falling back to your default of claiming the cog does not store data. Requiring that all cogs explicitly write the method to mark themselves as not storing data is a safety check you are somewhat bypassing.
The DM requests to the owner to enable sentry don’t actually run, so the repo will always require manually running the command to send errors to sentry. This is the correct default, but I’m not sure if the logic to enable auto-reporting is broken or if it is purposefully disabled.
Calculator
abs(5) returns Error!, rather than a value.
DiscordModals
The config migration seems to have broken existing buttons:
[ERROR] red.AAA3A-cogs.DiscordModals: The Button View could not be added correctly for the 590640587301191711-590640587867291653-1109622987256254506 message.
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\discordmodals\discordmodals.py", line 281, in load_buttons
button["style"] = discord.ButtonStyle(button["style"])
KeyError: 'style'
Draw
Cog still does not load due to invalid typing syntax on older versions of python:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\draw\__init__.py", line 37, in <module>
from .draw import Draw
File "datapath\cogs\CogManager\cogs\draw\draw.py", line 156
background: Literal[*MAIN_COLORS] = MAIN_COLORS[-1],
^
SyntaxError: invalid syntax
GetDocs
The cog does not load due to invalid typing syntax:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\getdocs\__init__.py", line 37, in <module>
from .getdocs import GetDocs
File "datapath\cogs\CogManager\cogs\getdocs\getdocs.py", line 30, in <module>
from .types import Attribute, Attributes, Documentation, Examples, Parameters, SearchResults
File "datapath\cogs\CogManager\cogs\getdocs\types.py", line 231, in <module>
class Documentation:
File "datapath\cogs\CogManager\cogs\getdocs\types.py", line 236, in Documentation
parameters: typing.Union[Parameters[str, str], str]
TypeError: <class 'getdocs.types.Parameters'> is not a generic class
I swapped to 3.8 for the second part of this review, so this may be a 3.8 error that does not happen on 3.9. If that’s the case, the cog still needs to work on all supported versions of the bot, so either min_bot_version needs to be set, or this needs to be changed to 3.8 compatible syntax.
GistsHandler
This cog fails to load if an API token is configured under github,token that is invalid
Traceback (most recent call last):
File "C:\Users\Flame\Desktop\Random\TestingRedbot\qa\BOT\cogs\CogManager\cogs\gistshandler\__init__.py", line 44, in setup
await cog.cogsutils.add_cog(bot)
File "C:\Users\Flame\Desktop\Random\TestingRedbot\qa\BOT\cogs\Downloader\lib\AAA3A_utils\cogsutils.py", line 103, in add_cog
await value
File "c:\users\flame\venv\live\lib\site-packages\redbot\core\bot.py", line 1907, in add_cog
await super().add_cog(cog, guild=guild, guilds=guilds)
File "C:\Users\Flame\Desktop\Random\TestingRedbot\qa\BOT\cogs\CogManager\cogs\gistshandler\gistshandler.py", line 48, in cog_load
await self.gists_client.authorize(token)
gists.exceptions.AuthorizationFailure: Invalid personal access token has been passed.
It should either silently ignore the token if it is not required, or have a user-friendly message if it is required.
Reminders
The background loop had an error:
Traceback (most recent call last):
File "datapath\cogs\Downloader\lib\AAA3A_utils\loop.py", line 125, in loop
self.last_result = await self.function(**self.function_kwargs)
File "datapath\cogs\CogManager\cogs\reminders\reminders.py", line 234, in reminders_loop
await reminder.process(utc_now=utc_now)
File "datapath\cogs\CogManager\cogs\reminders\types.py", line 602, in process
content=humanize_list([target["mention"] for target in self.targets]) if self.targets is not None else None,
File "datapath\cogs\CogManager\cogs\reminders\types.py", line 602, in <listcomp>
content=humanize_list([target["mention"] for target in self.targets]) if self.targets is not None else None,
TypeError: string indices must be integers
TransferChannel
The cog still errors if the channel argument provided is a channel the bot does not have read_messages in:
Traceback (most recent call last):
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 217, in all
count_messages, __ = await self.transfer_messages(
File "datapath\cogs\CogManager\cogs\transferchannel\transferchannel.py", line 173, in transfer_messages
await destination.send(
discord.errors.Forbidden: 403 Forbidden (error code: 50001): Missing Access
to be required.