UQComputingSociety / uqcsbot-discord

:mortar_board: UQCSbot: Our friendly little Discord bot
https://discord.uqcs.org
MIT License
20 stars 20 forks source link

Added managecogs command #131

Closed 49Indium closed 1 year ago

49Indium commented 1 year ago

Reloading the bot takes a significant amount of time when debugging, so I wrote a command that allows users to load, unload and reload cogs. Could also be used to quickly shut off bot features or to restart a cog that is stuck without restarting the entire bot.

This command should only be accessible by committee (maybe the infra team as well).

emadkhan713 commented 1 year ago

shouldn't there be a "@commands.check" to compare the user's roles before the command is run? e.g.

async def is_privileged():
  def predicate(interaction: discord.Interaction):
    return len(
      [role for role in interaction.user.roles if role.name == "Committee"]
    ) == 0
  return app_commands.check(predicate)

# followed by
@is_privileged()

this is what I'm going for

49Indium commented 1 year ago

I agree that there should be some sort of check; I'm not sure how though. I was planning to do this by allocating the command to certain users, however, I have referred to starboard and believe that @app_commands.default_permissions(manage_guild=True) might be at least more consistent, if not better way to do it. If checks have some other benefit, I'm happy to switch.

I'm not certain who should be able to access the command, so I'll leave it at this for now, until committee makes a decision. I'm not familiar with permissions, so anyone feel free to suggest a better way.

JamesDearlove commented 1 year ago

As I recall with previous conversations within infra + committee on this, the bot's settings in the server settings should now act as the main point of call of permissions granting. It saves having to change the code to assign someone permissions to be able to use a command.

49Indium commented 1 year ago

Does this mean that starboard should change, or that @app_commands.default_permissions(manage_guild=True) is just an additional check?

JamesDearlove commented 1 year ago

I think currently it's an additional check. @andrewj-brown also brought up a similar conversation about this in pr #119.

Side note/FYI to maintainers: ongoing discussion of this is happening in Discord

andrewj-brown commented 1 year ago

Does this mean that starboard should change, or that @app_commands.default_permissions(manage_guild=True) is just an additional check?

The default_permissions decorator just hints to discord what the default permission for using the command should be; it defers to whatever gets set in the actual server settings. Commands with perms should always have that decorator attached, which can then be overridden by server perms.

Direct perm-checks in code (e.g. what @emadkhan713 suggests) should also be avoided, for the reasons Jimmy gave - there are a couple scattered around right now, but that's an unfortunate consequence of locking specific arguments behind permissions (e.g. membercount --force), because Discord only handles permissions on a per-command basis.

Standardising perms is on my to-do-list, linked to the ongoing "make getting channels consistent" project that I've started in the typing PR (cc bot.py and the new self.bot.GENERAL_CNAME rather than redefining it in every file). For now, just the decorator should be enough and we'll look at it all later.

(While I'm waffling on, also, we want to avoid role-name-based checks, because that locks us into specific names. Better to use the explicit discord permission objects, which are much more granular.)

andrewj-brown commented 1 year ago

I trust that whatever testing I did 2 weeks ago was sufficient, merging