Skip to content
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

Merged
merged 13 commits into from
Jun 3, 2024

Conversation

dbattersby
Copy link
Contributor

@dbattersby dbattersby commented May 14, 2024

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:

group-dm-max-limit-1

Now we only show the users being added within the member count (not the current user):

group-dm-max-limit-2

@github-actions github-actions bot added chat PRs which include a change to Chat plugin i18n PRs which update English locale files or i18n related code labels May 14, 2024
@github-actions github-actions bot removed the i18n PRs which update English locale files or i18n related code label Jun 3, 2024
@dbattersby dbattersby changed the title FIX: update group chat member to include current user FIX: chat direct message group user limit is off by 1 Jun 3, 2024
@@ -25,10 +25,11 @@ class AddUsersToChannel
contract
model :channel
policy :can_add_users_to_channel
model :users, optional: true
model :target_users, optional: true
Copy link
Contributor Author

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);
Copy link
Contributor Author

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])
Copy link
Contributor Author

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.

@dbattersby dbattersby marked this pull request as ready for review June 3, 2024 06:05
Copy link
Member

@ZogStriP ZogStriP left a comment

Choose a reason for hiding this comment

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

Nice 👍 LGTM

@dbattersby dbattersby merged commit 4e80c9e into main Jun 3, 2024
16 checks passed
@dbattersby dbattersby deleted the fix-group-chat-member-count branch June 3, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin
2 participants