-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat: allow to override perms on commands in bulk. #3417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Changes from all commits
79a577f
fef2b99
75ec6b6
45e6f78
30360de
5723b54
20a2b89
3b704e1
e59365e
fba8c2e
436efd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1408,7 +1408,7 @@ def _parse_level(name): | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| @permissions.command(name="override") | ||||||||||||||||||||||
| @checks.has_permissions(PermissionLevel.OWNER) | ||||||||||||||||||||||
| async def permissions_override(self, ctx, command_name: str.lower, *, level_name: str): | ||||||||||||||||||||||
| async def permissions_override(self, ctx, command_name: str.lower, *, level_name: str = None): | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Change a permission level for a specific command. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -1422,8 +1422,16 @@ async def permissions_override(self, ctx, command_name: str.lower, *, level_name | |||||||||||||||||||||
| - `{prefix}perms remove override reply` | ||||||||||||||||||||||
| - `{prefix}perms remove override plugin enabled` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| You can also override multiple commands at once using: | ||||||||||||||||||||||
| - `{prefix}perms override bulk` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| You can retrieve a single or all command level override(s), see`{prefix}help permissions get`. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| if command_name == "bulk": | ||||||||||||||||||||||
|
Comment on lines
+1426
to
+1430
|
||||||||||||||||||||||
| - `{prefix}perms override bulk` | |
| You can retrieve a single or all command level override(s), see`{prefix}help permissions get`. | |
| """ | |
| if command_name == "bulk": | |
| - `{prefix}perms override @@bulk` | |
| You can retrieve a single or all command level override(s), see`{prefix}help permissions get`. | |
| """ | |
| if command_name == "@@bulk": |
lorenzo132 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help text doesn't mention that commas can be used as separators, but the code handles them (line 1494 replaces commas with spaces). Update the description to mention that commas, spaces, and newlines can all be used as separators.
| "You can list multiple commands separated by spaces or newlines.\n" | |
| "You can list multiple commands separated by commas, spaces, or newlines.\n" |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter condition c.strip() checks if the string is non-empty after stripping, but doesn't actually strip the whitespace from the strings that are kept. This means command names with leading or trailing whitespace will be preserved, which could cause command lookups to fail. Consider changing to raw_commands = [c.strip() for c in raw_commands if c.strip()] to actually strip whitespace from the command names.
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefixes list includes self.bot.prefix which could be None or an empty string, but the prefix stripping only executes if self.bot.prefix is truthy. If self.bot.prefix is falsy, the mention prefixes (which are always valid) will not be stripped. The condition should check if the prefixes list has valid entries, or the list should only include non-empty prefixes.
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter condition c.strip() checks if the string is non-empty after stripping, but doesn't actually strip the whitespace from the strings that are kept. This means command names with leading or trailing whitespace (from prefix stripping) will be preserved, which could cause command lookups to fail. Consider changing to raw_commands = [c.strip() for c in raw_commands if c.strip()] to actually strip whitespace from the command names.
lorenzo132 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The View instance needs to initialize a value attribute before it's used. The AcceptButton and DenyButton callbacks set self.view.value, but this View doesn't have a value attribute initialized. This will cause an AttributeError when a button is clicked. Initialize the view's value attribute by either creating a custom View class (like ConfirmThreadCreationView) or by adding view.value = None after creating the View instance.
| view = discord.ui.View() | |
| view = discord.ui.View() | |
| view.value = None |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View instances don't have an explicit timeout set. Discord.py's default View timeout is 180 seconds, but for consistency with the message wait timeout (120 seconds on line 1486) and better user experience, consider setting an explicit timeout for the Views. For example: discord.ui.View(timeout=120.0) to match the initial message wait timeout.
| view = discord.ui.View() | |
| view = discord.ui.View(timeout=120.0) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interaction response is deferred after stopping the view. The order should be reversed: defer the interaction response before stopping the view to ensure the interaction is properly acknowledged. Swap lines 1594 and 1595.
| self.view.stop() | |
| await interaction.response.defer() | |
| await interaction.response.defer() | |
| self.view.stop() |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The View instance needs to initialize a value attribute before it's used. The LevelSelect callback sets self.view.value, but this View doesn't have a value attribute initialized. This will cause an AttributeError when the select menu is used. Initialize the view's value attribute by either creating a custom View class (like ConfirmThreadCreationView) or by adding view.value = None after creating the View instance.
| view = discord.ui.View() | |
| view = discord.ui.View() | |
| view.value = None |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View instances don't have an explicit timeout set. Discord.py's default View timeout is 180 seconds, but for consistency with the message wait timeout (120 seconds on line 1486) and better user experience, consider setting an explicit timeout for the Views. For example: discord.ui.View(timeout=120.0) to match the initial message wait timeout.
| view = discord.ui.View() | |
| view = discord.ui.View(timeout=120.0) |
lorenzo132 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The View instance needs to initialize a value attribute before it's used. The AcceptButton and DenyButton callbacks set self.view.value, but this View doesn't have a value attribute initialized. This will cause an AttributeError when a button is clicked. Initialize the view's value attribute by either creating a custom View class (like ConfirmThreadCreationView) or by adding view.value = None after creating the View instance.
| view = discord.ui.View() | |
| view = discord.ui.View() | |
| view.value = None |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View instances don't have an explicit timeout set. Discord.py's default View timeout is 180 seconds, but for consistency with the message wait timeout (120 seconds on line 1486) and better user experience, consider setting an explicit timeout for the Views. For example: discord.ui.View(timeout=120.0) to match the initial message wait timeout.
| view = discord.ui.View() | |
| view = discord.ui.View(timeout=120.0) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing logging for bulk override operations. The single command override (line 1453-1457) logs changes with logger.info, but the bulk override silently applies changes without logging. Consider adding logging to track what commands were updated and to which permission level, similar to the single override pattern.
| logger.info( | |
| "Bulk override: set permission level %s for %d commands: %s", | |
| level.name, | |
| len(final_commands), | |
| ", ".join(cmd.qualified_name for cmd in final_commands), | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space before
{prefix}. The text should read "see{prefix}help permissions get" instead of "see{prefix}help permissions get".