Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow get_top_role to be optionally hoisted via config. #3093

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Allow get_top_role to be optionally hoisted via config. #3093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7922a97
Allow top role's hoisted check to be configurable.
scragly Sep 7, 2021
655e063
Add use_hoisted_top_role config help details.
scragly Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cogs/modmail.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,7 @@ async def anonadduser(self, ctx, *users_arg: Union[discord.Member, discord.Role,

tag = self.bot.config["mod_tag"]
if tag is None:
tag = str(get_top_hoisted_role(ctx.author))
tag = str(get_top_role(ctx.author, self.bot.config["use_hoisted_top_role"]))
name = self.bot.config["anon_username"]
if name is None:
name = tag
Expand Down Expand Up @@ -1003,7 +1003,7 @@ async def anonremoveuser(self, ctx, *users_arg: Union[discord.Member, discord.Ro

tag = self.bot.config["mod_tag"]
if tag is None:
tag = str(get_top_hoisted_role(ctx.author))
tag = str(get_top_role(ctx.author, self.bot.config["use_hoisted_top_role"]))
name = self.bot.config["anon_username"]
if name is None:
name = tag
Expand Down
2 changes: 2 additions & 0 deletions core/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ class ConfigManager:
"confirm_thread_creation_deny": "\N{NO ENTRY SIGN}",
# regex
"use_regex_autotrigger": False,
"use_hoisted_top_role": True,
}

private_keys = {
Expand Down Expand Up @@ -209,6 +210,7 @@ class ConfigManager:
"thread_show_roles",
"thread_show_account_age",
"thread_show_join_age",
"use_hoisted_top_role",
}

enums = {
Expand Down
11 changes: 11 additions & 0 deletions core/config_help.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,5 +1121,16 @@
"notes": [
"This configuration can only to be set through `.env` file or environment (config) variables."
]
},
"use_hoisted_top_role": {
"default": "Yes",
"description": "Controls if only hoisted roles are evaluated when finding top role.",
"examples": [
],
"notes": [
"Top role is displayed in embeds when replying or adding/removing users to a thread in the case mod_tag and anon_username are not set.",
"If this configuration is enabled, only roles that are hoisted (displayed seperately in member list) will be used. If a user has no hoisted roles, it will return 'None'.",
"If you would like to display the top role of a user regardless of if it's hoisted or not, disable `use_hoisted_top_role`."
]
}
}
6 changes: 3 additions & 3 deletions core/thread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
match_user_id,
match_other_recipients,
truncate,
get_top_hoisted_role,
get_top_role,
create_thread_channel,
get_joint_id,
)
Expand Down Expand Up @@ -938,7 +938,7 @@ async def send(
# Anonymously sending to the user.
tag = self.bot.config["mod_tag"]
if tag is None:
tag = str(get_top_hoisted_role(author))
tag = str(get_top_role(author, self.bot.config["use_hoisted_top_role"]))
name = self.bot.config["anon_username"]
if name is None:
name = tag
Expand Down Expand Up @@ -1055,7 +1055,7 @@ async def send(
elif not anonymous:
mod_tag = self.bot.config["mod_tag"]
if mod_tag is None:
mod_tag = str(get_top_hoisted_role(message.author))
mod_tag = str(get_top_role(message.author, self.bot.config["use_hoisted_top_role"]))
embed.set_footer(text=mod_tag) # Normal messages
else:
embed.set_footer(text=self.bot.config["anon_tag"])
Expand Down
6 changes: 4 additions & 2 deletions core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"trigger_typing",
"escape_code_block",
"tryint",
"get_top_hoisted_role",
"get_top_role",
"get_joint_id",
]

Expand Down Expand Up @@ -369,9 +369,11 @@ def tryint(x):
return x


def get_top_hoisted_role(member: discord.Member):
def get_top_role(member: discord.Member, hoisted=True):
roles = sorted(member.roles, key=lambda r: r.position, reverse=True)
for role in roles:
if not hoisted:
return role
Comment on lines 374 to +376
Copy link
Contributor

@Jerrie-Aries Jerrie-Aries Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can just do:

    if not hoisted:
        return member.top_role

before the for loop. Hmm, but the output still the same tho. 😅

And also, additionally, I'm suggesting if hoisted is True and no hoisted role is found after the for loop, it should fallback to member.top_role. Just to make sure this function returns an appropriate role object. 🤔

Copy link
Contributor Author

@scragly scragly Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can just do:

Thanks for the recommendation. It's a bit of a nitpick perhaps, as in discord.py, it's also iterating over the roles anyway in a similar manner. I'm inclined to leave it as is.

I'm suggesting if hoisted is True and no hoisted role is found after the for loop, it should fallback to member.top_role. Just to make sure this function returns an appropriate role object.

Honestly, this should probably have been the default behavior back when the original PR change occurred, however I didn't want to assume there wasn't considerations behind why it wasn't. Instead, my intention is to not alter the existing behavior as was written by @fourjr originally, and just make it configurable.

I don't want to creep the scope of this PR beyond this intention. If I did, I'd probably end up re-writing the entire project eventually just because I found a whole bunch of things I think should be different.

If the behavior you state is something you want, feel free to create a PR after this is merged.

if role.hoist:
return role

Expand Down