[Approved] Palmtree5-cogs

Applications
#1

Discord Name: Palm__#0873

GitHub Repository (Must be V3): https://github.com/palmtree5/palmtree5-cogs/tree/redv3-rewrites

Description: A collection of cogs written by me for various purposes, including moderation and interfacing with certain games and websites

Special Notes:

#2
#3

:wave: Sorry about the delay with getting to the app here.

Commit hash at time of review: c5fafc719b20a5fc076a4413198f674d3b650e39

I’ll be keeping the review separated as a message per cog to hopefully keep this easy to go through, I know it may look like a lot, but most of what I’ve seen here is fine, with just a few things needing addressing.

banrole/banrole.py

Most of what I have here are small changes which result in a significant performance boost. There’s also a potential for abuse here which should be addressed before this makes it into widespread use.

L16:
banrole command:
should use @commands.bot_has_permissions(ban_members=True) or inline it for custom error handling of this.

L27:

        for member in role.members:
            try:
                await ctx.guild.ban(member)
            except discord.Forbidden:
                failure_list.append("{0.name}#{0.discriminator} (id {0.id})".format(member))
            else:
                banned_list.append(member.id)

This needs a few safety checks. One of them will keep the bot from unnecessarily consuming part of the guild’s ban rate-limit, plus global limits the other prevents a small abuse case.

        for member in role.members:
            try:
                # prevent known condition for a discord.Forbidden
                assert ctx.guild.me.top_role > member.top_role and ctx.guild.owner != member
                # prevent admins from banning (or trying to ban) other people higher than them
                assert ctx.author.top_role > member.top_role or ctx.author == ctx.guild.owner
                await ctx.guild.ban(member)
            except (discord.HTTPException, AssertionError):
                failure_list.append("{0.name}#{0.discriminator} (id {0.id})".format(member))
            else:
                banned_list.append(member.id)

The second hierarchy check can be removed in the case of checking that the bot is already set not to respect hierarchy. (This setting can be pulled from mod’s config currently).

L40: unban role command should use @commands.bot_has_permissions(ban_members=True) or inline it for custom error handling of this.

Additionally, there should probably be an inline check that the person is above the role they are attempting to un-ban.

L47:

        for uid in banned_list:
            user = ctx.bot.get_user(uid)
            if user is None:
                try:
                    user = await ctx.bot.get_user_info(uid)
                except discord.NotFound:
                    banned_list.remove(uid)
                    return
            try:
                await ctx.guild.unban(user)
            except discord.Forbidden:
                pass
            except discord.NotFound:
                banned_list.remove(uid)
            else:
                banned_list.remove(uid)

This runs into an unfortunate issue with rate limits exceptionally fast.

The bucket for getting arbitrary users is much less forgiving than the guild ban/unban bucket and has global effects faster. This also makes fewer API calls overall in both best and worst case.

Changing to the below resolves this, while also providing some user feedback if the command fails.

        for uid in banned_list:
            try:
                await ctx.guild.unban(discord.Object(id=uid))
            except discord.Forbidden:
            # even checking for permissions, this is still possible with 2FA failures
                return await ctx.send("I can't do that")  # if you can't unban one user, you can't unban any.
            except discord.NotFound :
                banned_list.remove(uid)
            else:
                banned_list.remove(uid)
#4

eventmaker

This one is working great (I’m already using it on a server :eyes:)

There isn’t much to comment on here. It’s a solid cog.

Only things I noticed out of place :

  • You have an unused variable on Line 142 of eventmaker.py
  • You aren’t using Red’s menu util (not neccessary to change)

I’d suggest moving over to red’s menu for this at some point for consistency with core bot’s menuing, but this isn’t a requirement here.

I’ll finish my review of the remaining cogs soon. Feel free to start with what’s here or wait for the remaining cogs to be reviewed to start.

And if there are any questions about any of this, don’t hesitate to ask.

#5

hpapi:

This one took me a little longer to review. I don’t own minecraft, so I can’t grab an API key for the hypixel server to just verify basic behavior.

There aren’t any problematic parts in how it interacts with discord or other bot functions, and the interaction with hypixel’s API seem to be fine looking at the lib used and expected return values from them.

lockdown:

L108-109: change this to pass to use the auto help.

L55-56 & L95-86:

    for channel in guild.channels:
            if isinstance(channel, (discord.VoiceChannel, discord.TextChannel)):

This can be changed to:

    for channel in (*guild.text_channels, *guild.voice_channels):
        # and an unindent

to avoid the overhead of calling isinstance on each of the channels (generator creation is cheaper than even one isinstance call.)

L67, related L97:

early termination here on partial missing perms is problematic. It has the potential to partially lock down the server without recording that it did so, making it impossible to revert with the corresponding command to end the lockdown.

I recommend changing the logic here to check that you have the perms required to do this in each channel prior to even starting.

Keep in mind when designing such a check that the API does not let you edit a channels overwrites in a way which would remove access from yourself.

The other option would be to allow it to continue past channels where it could not perform the change, and inform the person invoking which channels could not be locked down.

mcsvr:

again, dont own minecraft. also again, no major issues. There may be an issue with how this interacts with the API, but this appears to be mostly fine. At the very least, it’s not going to break on the discord side of things.

L: 162-163: replace with pass to use auto help

messagepinner:

Outright broken, wont load.

__init__
L5: you’re passing bot to a cog not expecting it in it’s init
L6: redundant listener addition, this one should be handled automaticallly

messagepinner:

missing cog18n decorator (yes, I know it’s being removed soon, current version still needs it)

reddit:

:ok_hand:

Suggest to eventually migrate to red’s menu util for consistency with main both behavior, but not requied.

slowmode:

missing an import (discord) being used for error handling

ssrecords:

L72-73: swap these for just pass for auto help use.

potentially use red’s menu util (not required)

tweets:

menus.py : consider red’s menu util for future (optional)

L124: place autohelp=False in the group decorator to retain your custom handling without double sending the help message.

That wraps up my review (until changes are made) Again, if there are any questions, feel free to ask anything here or DM on discord.

#6

I’m gonna go ahead and try to reply all in one message here:

fixed

fixed

Fixed both of these

Fixed

Fixed

So I took the first option on this one and now have it flat denying locking/unlocking the server if the bot doesn’t have necessary perms in all target channels

fixed the autohelp

fixed the double sending of help

Fixed both of those

Fixed both

I believe I got everything in the list, though I didn’t touch switching over to the bot’s provided menu system yet as I can do that at a later point in time. If I managed to miss something though, let me know

#7

:+1:

Mostly good. I noticed two additional things with lockdown:

lockdown can actually (potentially) give a user back perms if they were restricted beyond the lockdown role for some reason. This is sufficiently enough of an edge case that fixing this should probably still be done, but I can understand it being a low priority, and would not consider it worth delaying the application for.

Additionally, the cog does not clear previously empty overwrites. (you have to explicitly send None as an overwrite to clear it, an empty dict is treated differently) This one is more likely to be encountered, but lower impact as well (UI clutter). Should probably be fixed when you get a chance, but given the lack of negative impact, handle it when you have the time to do so.

Separately:
image
You’ve already seen and acknowledged the issue in your channel. Seeing the response you had for it, and the impact: this one’s more impactful on end user experience. If you can’t get around to fixing this in the short term, adding a notice on cog install of potential breakage with API changes, and catching the exception and informing the user of the issue without throwing a traceback at them for it would be sufficient for ensuring a good end user experience with this for now.

#8

Acknowledging I’ve seen this, will get back to this on Monday when I have more time

#9

Fixed the current issues with Hpapi but also went ahead and added a note about possible api breakage to the info.json. Not completely clear on the second of the additional things with lockdown. I’ll look into the first one at some point though (I think I have an idea of how to do that one)

#10

Hey, thanks for getting back with the changes. This is good to go now (approving).

#11
#12