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
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
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.sendwhen it should bereaction.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_logfunction 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_logaltogether.
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
onandoffinstead 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 truenot[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)
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.
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.
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.
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.
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.
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.
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.
L55: This method of pluralization is fundamentally incompatible with i18n. Drop it or the i18n.
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.
L421-430, L836-847, (more in various locations):
Comparing values to translated results is not safe here.
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
L909-910: Don’t isinstance each channel in a guild. That’s expensive, and guild.text_channels exists.
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.
L132: Class should at a minimum have a pass (linter would catch this)
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.)
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.
L795-802:
Consider changing this extended chaining of ternary statements to a lookup.
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)
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)
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.sendwhen it should bereaction.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
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.
bot_version needs to be a list of integers.
cog.py
In your
_set_logfunction 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_logaltogether.
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
L257: This is overly complicated for use with language translation and could be better with
onandoffinstead 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.
L696: Should be
[p]warnset showmod truenot[p]warnset reinvite true
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 
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.
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
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 
1 Like
Oops, it was my command for testing Sentry errors 
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)
Awesome, marking this as approved!