feat(status-roles): add cog to assign roles based on user status #789
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces a new cog, Sequence diagram for status role assignmentsequenceDiagram
participant User
participant DiscordClient
participant StatusRolesCog
participant DiscordAPI
User->>DiscordClient: Updates status
DiscordClient->>StatusRolesCog: on_presence_update(before, after)
StatusRolesCog->>StatusRolesCog: get_custom_status(member)
StatusRolesCog->>StatusRolesCog: has_activity_changed(before, after)
StatusRolesCog->>StatusRolesCog: check_and_update_roles(member)
StatusRolesCog->>StatusRolesCog: get_custom_status(member)
StatusRolesCog->>StatusRolesCog: for each config in STATUS_ROLES
StatusRolesCog->>StatusRolesCog: re.search(pattern, status_text)
alt matches and not has_role
StatusRolesCog->>DiscordAPI: member.add_roles(role)
DiscordAPI-->>StatusRolesCog: Role added
else not matches and has_role
StatusRolesCog->>DiscordAPI: member.remove_roles(role)
DiscordAPI-->>StatusRolesCog: Role removed
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @electron271 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a more specific exception handler for
discord.Forbiddento differentiate between missing permissions to add vs remove roles. - It might be helpful to include the server ID in the log messages for easier debugging.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+66
to
+67
| # Status Roles | ||
| STATUS_ROLES: Final[list[dict[str, int]]] = config["STATUS_ROLES"] |
There was a problem hiding this comment.
suggestion: Consider revising the type annotation for STATUS_ROLES.
The current type annotation suggests that all dictionary values are ints, yet later the code retrieves a regex pattern string from the config. It may be more precise to revise the annotation (or use a TypedDict) to reflect that some values are strings.
Suggested implementation:
from typing import Union
STATUS_ROLES: Final[list[dict[str, Union[int, str]]]] = config["STATUS_ROLES"]Ensure that the import for "Union" is added only once at the top of the file if there are multiple typing imports.
|
|
||
| return None | ||
|
|
||
| async def check_and_update_roles(self, member: discord.Member): |
There was a problem hiding this comment.
issue (complexity): Consider extracting helper functions to reduce nesting and improve the readability and testability of the check_and_update_roles function by separating the per-config processing, regex matching, and role update logic into their own methods .
Consider extracting helper functions to break down the nested logic in check_and_update_roles. For example, you can separate the per-config processing, regex matching, and role update into their own methods. This keeps each function focused and easier to test. For instance:
def status_matches(self, status_text: str, pattern: str) -> bool:
"""Check if the status text matches the provided regex pattern."""
return bool(re.search(pattern, status_text, re.IGNORECASE))Then refactor the role update logic:
async def update_member_role(self, member: discord.Member, role: discord.Role, should_have: bool, pattern: str, status_text: str):
if should_have and role not in member.roles:
logger.info(f"Adding role {role.name} to {member.display_name} (status: '{status_text}' matched '{pattern}')")
await member.add_roles(role, reason="Status role automation")
elif not should_have and role in member.roles:
logger.info(f"Removing role {role.name} from {member.display_name} (status no longer matches)")
await member.remove_roles(role, reason="Status role automation")Finally, simplify check_and_update_roles by looping through configs and delegating per-config processing:
async def check_and_update_roles(self, member: discord.Member):
if member.bot:
return
status_text = self.get_custom_status(member) or ""
for config in self.status_roles:
if int(config.get("server_id", 0)) != member.guild.id:
continue
role_id = int(config.get("role_id", 0))
pattern = str(config.get("status_regex", ".*"))
role = member.guild.get_role(role_id)
if not role:
logger.warning(f"Role {role_id} configured in STATUS_ROLES not found in guild {member.guild.name}")
continue
try:
matches = self.status_matches(status_text, pattern)
await self.update_member_role(member, role, matches, pattern, status_text)
except re.error:
logger.exception(f"Invalid regex pattern '{pattern}' in STATUS_ROLES config")
except discord.Forbidden:
logger.exception(f"Bot lacks permission to modify roles for {member.display_name} in {member.guild.name}")
except Exception:
logger.exception(f"Error updating roles for {member.display_name}")These changes maintain all functionality while reducing nesting and improving readability and testability.
Comment on lines
+53
to
+57
| for activity in member.activities: | ||
| if isinstance(activity, discord.CustomActivity) and activity.name: | ||
| return activity.name | ||
|
|
||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Use the built-in function next instead of a for-loop (use-next)
Suggested change
| for activity in member.activities: | |
| if isinstance(activity, discord.CustomActivity) and activity.name: | |
| return activity.name | |
| return None | |
| return next( | |
| ( | |
| activity.name | |
| for activity in member.activities | |
| if isinstance(activity, discord.CustomActivity) and activity.name | |
| ), | |
| None, | |
| ) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
add automatically giving roles based on status
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
ran it with regex "test" and changed status
Summary by Sourcery
Adds a cog that automatically assigns roles to users based on their status, using regular expressions to match against the user's custom status. The cog checks and updates roles on startup and when a user's presence changes.
New Features: