[APPROVED] reediculous456/RedBot-Cogs

Applications
#1

Discord Name: reediculous456

GitHub Repository (Must be V3): GitHub - reediculous456/RedBot-Cogs

Description: A cog to verify a user and grant them a role when they join a server. The intent here is to prevent the user from accessing anything in the server until verified

#2
#3

RedBot-Cogs

  • Repository Review: reediculous456/RedBot-Cogs
  • Commit hash at time of review: d25230d752046efea948ee5e32c66f69e2d85ff8

Hi reediculous456, thank you for your patience whilst I reviewed your repository. If you have any queries about this review, please send me a message on Discord.

General Comments

EUD Compliance

Both of your cogs are missing elements required to be fully EUD compliant. Take a look at the given examples below and implement them into your cogs.

Inside the cog classes of verifier/verifier.py and quote_otd/quote_otd.py

This will handle logic for deleting user data when requested. As you currently don’t store user data, you can just pass through it.

    async def red_delete_data_for_user(self, **kwargs):
        """Nothing to delete."""
        return

Inside verifier/info.json and quote_otd/info.json

Along with the other keys, this key should disclose whether, how and/or why, data is stored about users.

{
    "end_user_data_statement": "This cog does not persistently store data or metadata about users.",
}

Inside verifier/__init__.py and quote_otd/__init__.py

This will retrieve the statement from info.json and store it inside the __red_end_user_data_statement__ identifier.

from redbot.core.utils import get_end_user_data_statement

__red_end_user_data_statement__ = get_end_user_data_statement(__file__)

Review Comments

quote_otd/quote_otd.py

  • L11 - We actually use a subclass of commands.Bot as we have added various methods exclusive to Red. You should type hint this using the Red class, imported from redbot.core.bot.
  • L13 - You should pass the force_registration=True kwarg to Config.get_conf.
  • L28 - This method here is somewhat redundent as similar logic is already provided through prefix/clean_prefix properties of Context. You can use ctx.prefix/ctx.clean_prefix right away.
  • L60 - The docstring should inform users that they may upload a file to add quotes.
  • L82 - I recommend using some form of menu here or paginated messages, as this will improve user experience and make your codewriting cleaner (optional).
  • L98 - You should define the embed colour as await ctx.embed_colour() to allow for uniformality for the bots using your cog.
  • L111 - It is a common misconception, but int converters can also correspond to negative integers, which you won’t want in this case. You can use the commands.positive_int converter here. Alternatively, you could call abs(hour) and abs(minute) to return their absolute positive values.
  • L115 - Please relate back to my previous comment about L28.
  • L119 - This try/except clause is overly generalized which is not great to practice. Instead, catch specific exceptions if you’re expecting them, and provide more helpful exception detail.
  • L142 - This conditional statement is redundent, as the enabled parameter has no default (it is always required and will never be None).

verifier/verifier.py

  • The review comments from quote_otd.py for lines 11, 13 and 28 also apply to this cog.

  • L40 - Please relate back to my previous comment about L28.

  • L41 - It is not optimal to make repeated calls to Config like this. Instead, scopes provide an all() method which will return a dictionary of the registered values for that scope.
    Instead of calling config multiple times, you can write like this:

    guild_config = await self.config.guild(guild).all()
questions = guild_config["questions"]
role = get(guild.roles, id=guild_config["role_id"])
kick_on_fail = guild_config["kick_on_fail"]
num_questions_to_ask = guild_config["num_questions_to_ask"]

  • L85 - You should check that the bot has sufficient permissions before trying to kick the user. Currently, if the bot does not have permission, the discord.Forbidden will be caught but will send unrelated information (L92).

  • L128 - You could consider renaming this command to verifiedrole for better UX, the command currently reads as verifyset setverifiedrole which is unnecessarily long (optional).

  • L130 - This conditional statement is redundent, as the role parameter has no default (it is always required and will never be None).

  • L137 - Again question cannot be None here as question is a required parameter.

  • L147 - This conditional statement is redundent, as the index parameter has no default (it is always required and will never be None).

  • L162 - index and question cannot be None here as index and question are required parameters.

  • L184 - See comment about L128, but for this command.

  • L186 - This conditional statement is redundent, as the kick_on_fail parameter has no default (it is always required and will never be None)

  • L196 - This conditional statement is redundent, as the verification_enabled parameter has no default (it is always required and will never be None)

  • L204 - See comment about L128, but for this command.

  • L214 - See comment about L128, but for this command.

requirements.txt

  • This file is not required (optional).

Next Steps

  • Make the necessary changes in response to the review criteria.
  • Contact me once you are done, or if you have any questions along the way!
#4

Discussed through Discord and changes have been made in response to the review. Marking this as approved🎉

#5