-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
FIX: chat direct message group user limit is off by 1 #27014
Conversation
@@ -25,10 +25,11 @@ class AddUsersToChannel | |||
contract | |||
model :channel | |||
policy :can_add_users_to_channel | |||
model :users, optional: true | |||
model :target_users, optional: true |
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.
Updated this to target_users
to work with the MaxUsersExcessPolicy policy.
@@ -26,7 +26,11 @@ export default class NewGroup extends Component { | |||
} else { | |||
return acc + 1; | |||
} | |||
}, 1); | |||
}, 0); |
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.
This is important as we want to set the default count to 0/N, previously it was 1/N to account for current user.
@@ -111,15 +111,16 @@ | |||
user_1 = Fabricate(:user) | |||
user_2 = Fabricate(:user) | |||
user_3 = Fabricate(:user) | |||
group = Fabricate(:public_group, users: [user_1, user_2]) | |||
user_4 = Fabricate(:user) | |||
group = Fabricate(:public_group, users: [user_1, user_2, user_3]) |
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.
Since max dm users is set to 3 in the before block, we should expect adding 4 users to be restricted.
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.
Nice 👍 LGTM
This change allows the correct number of members to be added when creating a group direct message, based on the site setting
chat_max_direct_message_users
.Previously we counted the current user within the max user limit. Therefore the count was off by 1:
Now we only show the users being added within the member count (not the current user):