-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
message: Fix code checking group mention permission to handle anonymous user groups. #30130
Conversation
This commits fixes the code which checks group mention permission to handle anonymous user groups correctly. Basically we were not checking whether the UserGroup is linked to a NamedUserGroup and directly accessing named_user_group which results in an error. We also update the error messages to include the group name which has permission to mention the groups since now there might be a comnbination of groups and users who has permission to mention the group. This commit also adds tests to check sending and editing messages when can_mention_group is set to a anonymous user group.
This commit updates the code to not call is_user_in_group function if can_mention_group setting is set to "role:everyone" group.
1aae7f6
to
eb82d52
Compare
"You are not allowed to mention user group '{user_group_name}'. You must be a member of '{can_mention_group_name}' to mention this group." | ||
).format(user_group_name=group.name, can_mention_group_name=can_mention_group_name) | ||
_("You are not allowed to mention user group '{user_group_name}'.").format( | ||
user_group_name=group.name |
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.
Now that can_mention_group
can be a combination of users and groups, we might also want to handle the system bot separately like what should we do if the setting is set to {direct_subgroups: [everyone_group.id], direct_members: [admin.id]}
. Obviously including everyone group as subgroup and then having other fields, does not make sense but we do not restrict anyone from doing this, so this might be the case.
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.
I don't think we need special code for that case; it's a useless configuration, but not a problematic one.... and might even be a useful intermediate state to keep track of some individual permissions when temporarily changing the current permission value to everyone
with intent to restrict it again later.
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.
Ok, so do we want to allow system bots to mention the group in case the setting is set to {direct_subgroups: [everyone_group.id], direct_members: [admin.id]}
. We do not allow currently.
user_group.direct_members.set([othello]) | ||
user_group.direct_subgroups.set([moderators_system_group]) | ||
leadership.can_mention_group = user_group | ||
leadership.save() |
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.
I think we should aim to use higher-level functions than this in tests wherever possible -- there's no reason we shouldn't be able to just call the do_
with the object we want in about the same number of lines of code.
Merged, with one comment about test setup patterns that might be best addressed in a follow-up PR anyway; I'm not sure whether you'll find it makes sense to add a new |
UserGroup
object is linked to aNamedUserGroup
object.Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: