[APPROVED] Laggron's Dumb Cogs

#1

Discord Name: El Laggron#0260

GitHub Repository (Must be V3): https://github.com/retke/Laggrons-Dumb-Cogs

Description: Utility cogs for the servers.

1 Like
assigned TrustyJAID #3
#4

Hi @El_Laggron I’ll be reviewing your re-application over the next little while. Please note it may take some time due to the scope of some of your cogs.

1 Like
#5

Hi @El_Laggron, here’s my review. First I’m going to list the items that have not changed since your first two attempts.

  • Not properly disclosing that you are grabbing arbitrary messages on an error with sentry which are not guaranteed to be relevent to the issue You need to clarify in your enable asking exactly what user data you collect, and fix grabbing irrelevant messages.

Not fixed after initial review. This is really straight forward, when you first setup your cogs you must explain that your cogs have the option to collect x, y, and z to help you solve issues faster. To add onto this you’ve expanded this function to your other cogs since the initial reviews without extra declaration. Your logging also collects things that you do not care about, most of the data is benign from my testing but it must be explicitly stated what data you do collect through sentry.

RoleInvite

errors.py

  • each error here should inherit from Exception.

This has not been fixed since the first review.

roleinvite.py

  • On line 191, you check that the role is assignable by the bot due to hierarchy, but there needs to also be a check that the user should be able to give that role away. Right now, this is a privilege escalation issue. Someone could give themselves a higher role than they have by setting up an invite with this, quitting the server, and then using that invite.

Not fixed since first review.

Instantcmd

docs

  • [REJECTED (2) ] Laggron's Dumb Cogs The issue described in the docs here have not been completely fixed. Your example command says messag.send when it should be reaction.message.send. Furthermore your example for reaction add has no checks if the emoji is the emoji you want and replies to every emoji added including the one the bot added. You should provide a good example of how to use listeners that doesn’t lead to people running poor code when testing things out.

WarnSystem

warnsystem.py

  • Do not hook on_command_error when you are only interested in your own cog’s errors.

Not fixed since initial review. You should be using async def __error(self, ctx, error) to only collect errors within the cog.

These should have all been fixed before you attempted to re-apply and I’m disappointed that you have not fixed even the simple issues here.

===================================

Now onto other things that should be changed.

All Cogs

info.json

  • author is a list of strings now.
  • bot_version needs to be a list of integers.
    Reference.

cog.py

  • In your _set_log function you declare log as a global variable. This is unnecessary as your log will already be initialized when the cog is imported if you declare it in the header next to the imports. You can also then pass sentry to the cogs init alongside bot eliminating the need to have _set_log altogether.

Say

say.py

  • L38: self.translator is not used anywhere
  • L40: self.cache is not used anywhere
  • L344-346: Cleanup cache is unnecessary with using Tunnel
  • L257: This is overly complicated for use with language translation and could be better with on and off instead of enable/enabled and disable/disabled. (Optional)

WarnSystem

info.json

  • You should declare in your install_msg that this cog conflicts with core warnings cog by using the warn command.

warnsystem.py

  • L696: Should be [p]warnset showmod true not [p]warnset reinvite true

  • L669-680: Issue was brought up in DM’s after I was assigned to review this. Fixed in commit 34c1c83569f871404970b05bd418dc9d1f928b73

  • L1088-1095:
    You may consider using a lookup here instead of lambda as you have elsewhere in your cogs. (Optional)

  • L1260: See issue with L257 of say.py.

  • Re-invite on unban will only work if the user being banned shares another server with the bot and enabled DM’s from members on that shared server. It might be better to offer them an invite before banning which will only work once the ban is over.(Optional)

  • You format a timestamp in the embeds yourself, while they look nicer than discords will cause confusion when the timestamps don’t match up because the bot is running in another timezone. This can be solved by passing a timezone naive UTC datetime object to the embed.timestamp attribute.(Optional)

#6

Thanks for your review, I’d just like to bring and/or get a clarification on some things.

Sentry

For the Sentry logging stuff, I don’t really get what I actually need to tell to the user. Should I write a long document explaining everything in details? I was inspired by what was done on core Red, and the message for Sentry is similar to mine. Also what’s logged is already handled by the raven library, I’m adding other things such as the command object or the owner info, is this what I need to tell, or should I explain in details what raven logs?

Your logging also collects things that you do not care about

Once again, it’s handled by the library and I don’t know how to filter that (filter can be done on the sentry ui, currently used for token or IP, but it can be removed at any time). The only solution I can find for this would be writing my own API for error logging.

RoleInvite

Forgive me for RoleInvite, I fixed those issues but I was also working on a rewrite on a different branch and the merge cancelled the changes, see commits on december 27, my bad for not paying attention, I’m going to fix this.

WarnSystem

For the on_command_error thing, I wasn’t aware of __error, so I used this instead:
if not ctx.command.cog_name == self.__class__.__name__: return
Same result, but I’m still going to change this to your method since it doesn’t require the check.

InstantCommands

No excuse for this, sorry, I’m gonna rewrite it.

#7

To answer your questions about Sentry, you might consider looking at a completely different solution than raven. Raven and other sentry mechanics are being removed from Red in 3.1 and raven itself is deprecated. There are filtering tools inside sentry-python which you may consider moving to instead of raven.

As of right now I noticed that your logger will collect arbitrary data from d.py including user ID’s, presence updates, and other data not relevant to the error itself. This is likely due to hooking into on_command_error and declaring global log variables although I’m not 100% sure. I saw no serious things being transferred. That said you must declare what type of data you are collecting, you can figure this out by viewing the results from the sentry you have setup and determining what data is being recorded. Consider the extensive privacy policy pages many applications have to enforce when collecting data on users even if it’s their own software. I’m not saying you need to have a full privacy policy to link to that users must accept. But having a general “error logging” when your sentry collects more than just errors is not acceptable.

Given everything here I’m putting 2 weeks to complete the changes and if they are not complete within that time you may not re-apply until all non-optional changes are complete and no less than 1 month after the 2 weeks are up.

#8

Alright, I’m pretty sure everything is fixed now, referring to all 3 reviews. I listed all the issues and associated the commit that fixes it, so we can be sure that, this time, I’m not letting issues unresolved…

First review

InstantCommands

instantcmd.py

L6: you aren’t using inspect. If this is for future stuff, comment it out.

Fixed on time

L59-66: Appears to be dead code, though seems to have a purpose towards a future feature hinted at in your docs. Consider removing this or commenting it out if you intend to use it later.

Fixed on time

L240-245: The issue you linked to in discord.py, Danny actually responded in how you could handle this. Should stop pointing to that issue and fix this. Users should not need to restart their bot to remove the listeners.

Fixed on time

RoleInvite

errors.py

each error here should inherit from Exception

Fixed on time, overwritten by another branch and fixed back

roleinvite.py

On line 191, you check that the role is assignable by the bot due to hierarchy, but there needs to also be a check that the user should be able to give that role away. Right now, this is a privilege escalation issue. Someone could give themselves a higher role than they have by setting up an invite with this, quitting the server, and then using that invite.

Fixed on time, overwritten by another branch and fixed back

Say

say.py

L79: File download function:

I would highly advise not doing it in this fashion. The files do not need to be stored on disk at all, and adds an unneeded wget dependency.

There’s already a nice tool for this in Red.

from redbot.core.utils import tunnel
files = await tunnel.files_from_attach(some_message)

This is also designed specifically with the bot upload limit in mind, so you can safely use this out of the box with it.

Fixed on time

WarnSystem

The first review was closed after the WarnSystem review, so all the changes were done after.

api.py

L46: No. Absolutely do not do this. This is not an appropriate use of a global.
L47: Importing here is not the solution to your documentation problem.

Fixed after first review

L55: This method of pluralization is fundamentally incompatible with i18n. Drop it or the i18n.

Fixed after first review

L66-75:
The cascading divmods don’t need to be conditional,
and the dictionary access in this manner is an uneccessary performance hit.
You are translating the same strings over and over using the results as dictionary keys here.

Fixed after first review

L421-430, L836-847, (more in various locations):
Comparing values to translated results is not safe here.

Fixed after first review

L421-456 (Not repeating myself for each instance of this you do.):
Again, this is fundamentally incorrect for i18n.
Languages do not all follow this pattern, you cannot assume this can work.
You need positional, or ideally named parameters for format, with the entire relevent string present.
See L:227-229 of warnsystem.py for an example of where you did this correctly

Fixed after first review

L909-910: Don’t isinstance each channel in a guild. That’s expensive, and guild.text_channels exists.

Fixed after first review

errors.py

(each)
Logging at exception creation, instead of at exception handling is poor form, especially since you are using exceptions as part of control flow.

Fixed after first review

L132: Class should at a minimum have a pass (linter would catch this)

Fixed after first review

warnsystem.py

L49-70: Missing attribution of borrowed code.

That was actually already on line 31, but I added it a second time after first review

L92, various uses of this config value below:
Don’t recreate a setting which exists in the bot already. Use the bot settings.
Creating an override for the cog is fine here (this is not how you are using it),
but users should not have to set immunities, and heirarchy respecting per cog

We had a discussion in DM, this setting is not included on Red for now.
sigh, yeah go ahead and ignore that one for now, but just keep in mind that may become a bot global setting like the automod immunities later (it was supposed to have, but it’s too late to happen for 3.0.0 now, and I didn’t notice that change hadn’t happened.”

L225: Missing required parameter to function (linter would catch this.)

Fixed after first review

L323-341, (additional possible locations):
Sending embeds without this being tied to the purpose of your cog, without respecting the setting for this

Still don’t get what was meant here.

L426: (interacts with api.py:L520)
Something here needs to change. You should not be using isinstance on the result of a function of yours to determine there was an issue.
It’s an expensive function call and should be used sparingly.

Fixed after first review

L795-802:
Consider changing this extended chaining of ternary statements to a lookup.

Fixed after first review

L1291:
Do not hook on_command_error when you are only interested in your own cog’s errors.

This actually prevents logging other errors:

if not ctx.command.cog_name == self.__class__.__name__:
    return

L1306:
construct this f-string better to avoid the replacement operation.
It’s an uneeded performance hit per message

Fixed after first review but Sentry is now removed from all cogs.

Miscelaneous

Never closing sentry session

Sentry is removed.

You are using a lot of lambda statements where a ternary operator would be better and more efficient (example: warnsystem L1235)

Fixed after first review

f-strings in logging is technically acceptable, but it would be better for
these to not be formatted at all and pass parameters for formatting since
a large portion of the time, many of these statements will never need to be evaluated
(debug level logging)

Still makes it easier for me, isn’t translated and doesn’t reduce performances…

occasional f-string use without any interpolation (see L345 of warnsystem for an example of this)

Fixed after first review

Not properly disclosing that you are grabbing arbitrary messages on an error
with sentry which are not guaranteed to be relevent to the issue
You need to clarify in your enable asking exactly what user data you collect,
and fix grabbing irrelevant messages

Sentry completly removed.

Second review

Two issues were mentionned in that review:

  • the outdated docs after the breaking changes, fixed
  • the listeners being added multiple times upon cog reloads, fixed

Third review

Before referring all commits fixing the problems, just one thing to tell: I completly removed Sentry since it’s causing more problems than it solves, especially with sentry-sdk which is way more complicated for a cog.

Things that were not fixed

Sentry

Not properly disclosing that you are grabbing arbitrary messages on an error with sentry which are not guaranteed to be relevent to the issue You need to clarify in your enable asking exactly what user data you collect, and fix grabbing irrelevant messages.

Sentry is removed.

RoleInvite

each error here should inherit from Exception

As I said above, fixed on time, overwritten by another branch and fixed back.

On line 191, you check that the role is assignable by the bot due to hierarchy, but there needs to also be a check that the user should be able to give that role away. Right now, this is a privilege escalation issue. Someone could give themselves a higher role than they have by setting up an invite with this, quitting the server, and then using that invite.

Same here, fixed on time, overwritten by another branch and fixed back.

InstantCmd

The issue described in the docs here have not been completely fixed. Your example command says messag.send when it should be reaction.message.send. Furthermore your example for reaction add has no checks if the emoji is the emoji you want and replies to every emoji added including the one the bot added. You should provide a good example of how to use listeners that doesn’t lead to people running poor code when testing things out.

Fixed

WarnSystem

Do not hook on_command_error when you are only interested in your own cog’s errors.

This has never been a problem since I use this inside the function, filtering errors that are not mine. Also, converting this to __error somehow caused a lot of issues, so I’m keeping this block of code, this only log errors from the current cog.

if not ctx.command.cog_name == self.__class__.__name__:
    return

New issues

info.json

author is a list of strings now.

Fixed

bot_version needs to be a list of integers.

Fixed

cog.py

In your _set_log function you declare log as a global variable. This is unnecessary as your log will already be initialized when the cog is imported if you declare it in the header next to the imports. You can also then pass sentry to the cogs init alongside bot eliminating the need to have _set_log altogether.

Sentry removed.

Say

  • L38: self.translator is not used anywhere
  • L40: self.cache is not used anywhere
  • L344-346: Cleanup cache is unnecessary with using Tunnel

Fixed

L257: This is overly complicated for use with language translation and could be better with on and off instead of enable/enabled and disable/disabled. (Optional)

Sentry removed, this includes this part.

WarnSystem

You should declare in your install_msg that this cog conflicts with core warnings cog by using the warn command.

Fixed

L696: Should be [p]warnset showmod true not [p]warnset reinvite true

Fixed

L669-680: Issue was brought up in DM’s after I was assigned to review this. Fixed in commit 34c1c83569f871404970b05bd418dc9d1f928b73

You do the work for me :smiley:

L1088-1095:
You may consider using a lookup here instead of lambda as you have elsewhere in your cogs. (Optional)

Technically it’s already a lookup, it uses dict.get.

L1260: See issue with L257 of say.py.

Sentry removed.

Re-invite on unban will only work if the user being banned shares another server with the bot and enabled DM’s from members on that shared server. It might be better to offer them an invite before banning which will only work once the ban is over. (Optional)

It’s a good idea, I’m adding this on my todo list for the next version, since this is not required right now.

You format a timestamp in the embeds yourself, while they look nicer than discords will cause confusion when the timestamps don’t match up because the bot is running in another timezone. This can be solved by passing a timezone naive UTC datetime object to the embed.timestamp attribute.(Optional)

Same here.

#9

i personnaly checked all of that (before it was posted), everything is really fixed, then unfixed, and fixed again …
I think the fact that (mostly WarnSystem) your cogs are really complicated, makes everything really hard to deal with.
Oh and maybe i have to say how stressed laggron was yesterday before posting that message (that i checked case by case, everything is true lol), honestly was like “god dammit i’m not gonna make it again, won’t be able to apply again ever after this one, …” and just finished writing this at like 1 in the morning, was so stressed he was still on discord at 2 lol
You’re gonna make it laggron ! just take your time, like you took your time to write the last post lol

1 Like
#10

Hi @El_Laggron, thanks for taking your time to complete these fixes. I’ve looked through and your current latest version does in fact fix all the issues from my review.

I will clarify that the intent was not to have you remove sentry logging entirely but just declare what information you were actually collecting from it. In my tests I swapped your sentry url with my own and saw a lot more data than I wanted to see combine with some things said on the main server that gave me initial concerns. Testing showed no issues but declaring what you’re collecting is a must. I also wanted to provide that the methods you were using are depreciated and in many cases not supported. The new method I linked showed ways to filter certain events client side and is the recommended sentry logging system for python in general.

There’s still some leftover testing commands I you need to clean up like L1163 in warnsystem.py that will appear in cog help and has no doc strings and just raises a RuntimeError. Clean that up and everything will be good to go :smile:

1 Like
#11

Oops, it was my command for testing Sentry errors :sweat_smile: I just removed it.

And to clarify about why I removed Sentry: I wanted to rewrite most of how the logger works since the library changes, but it has some problems (especially the user and extra context which cannot be unset, and doesn’t take the client as argument, so it may take another cog’s client since it’s the same thread).
I may add Sentry back, but this was taking me too much time, not only for the review but in general, and it doesn’t help me that much currently since I don’t get a lot of events.

Thanks a lot for your time invested, I hope it was not a pain (especially warnsystem which is long af) :smiley:

#12

Awesome, marking this as approved!

closed #13