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

feat: Limit of members that can be added using the autojoin feature in a team's channel to the value of the API_Users_Limit setting #31859

Merged
merged 28 commits into from
Jun 12, 2024

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented Feb 29, 2024

Proposed changes (including videos or screenshots)

There existed an implicit limit of 50 members that would be included whenever a a channel with auto join was created inside a team, this task ties the limit to the API_Users_Limit setting, so it is configurable.

Issue(s)

Steps to test or reproduce

This task requires a team with over 1000 members, so all of these members should be created and associated with the team

  1. Create a channel from the team
  2. Open the kebab-menu and mark the auto join box
    All the members should've been added correctly
  3. Add one more user to the team
  4. Create a new channel in the team
  5. Mark the auto join box
    Now the new user should not have been added since there is a hard limit of 1000 members

Further comments

SUP-34

This PR is a copy of #31423
I created this one, because the other one was created from a fork which was causing a few problems such as weird conflicts

Copy link

changeset-bot bot commented Feb 29, 2024

🦋 Changeset detected

Latest commit: 4cbf36b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 34 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/i18n Minor
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Major
@rocket.chat/web-ui-registration Major
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-client Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-video-conf Major
@rocket.chat/uikit-playground Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/model-typings Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/models Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Gustrb
Copy link
Contributor Author

Gustrb commented Feb 29, 2024

This PR is a copy of #31423
I did this because the other PR was made from a fork and was causing a few issues since the develop branch was messed up

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 10 lines in your changes missing coverage. Please review.

Project coverage is 56.41%. Comparing base (972ba42) to head (4cbf36b).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #31859      +/-   ##
===========================================
- Coverage    56.42%   56.41%   -0.02%     
===========================================
  Files         2445     2450       +5     
  Lines        53932    54017      +85     
  Branches     11118    11134      +16     
===========================================
+ Hits         30432    30472      +40     
- Misses       20849    20885      +36     
- Partials      2651     2660       +9     
Flag Coverage Δ
e2e 56.17% <28.57%> (-0.02%) ⬇️
unit 71.84% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@Gustrb Gustrb marked this pull request as ready for review February 29, 2024 13:13
@Gustrb Gustrb requested review from a team as code owners February 29, 2024 13:13
Copy link
Member

@MarcosSpessatto MarcosSpessatto left a comment

Choose a reason for hiding this comment

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

Is it possible to add tests to cover this feature?

We can use a small number for the setting to avoid creating too many users.

The ideal would be to test every case, with more and less members than the accepted, etc.

We can also add some UI tests to make sure the error is being displayed if the limit was exceeded.

apps/meteor/server/services/team/service.ts Show resolved Hide resolved
apps/meteor/server/services/team/service.ts Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
@scuciatto scuciatto added this to the 7.0 milestone Mar 5, 2024
@scuciatto scuciatto removed this from the 6.8 milestone Apr 17, 2024
@scuciatto scuciatto added this to the 6.10 milestone May 29, 2024
MarcosSpessatto
MarcosSpessatto previously approved these changes Jun 6, 2024
.changeset/clean-moose-cover.md Outdated Show resolved Hide resolved
Co-authored-by: Aleksander Nicacio da Silva <[email protected]>
@Gustrb Gustrb added stat: QA assured Means it has been tested and approved by a company insider and removed stat: QA assured Means it has been tested and approved by a company insider labels Jun 12, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 12, 2024
@kodiakhq kodiakhq bot merged commit 16b67aa into develop Jun 12, 2024
50 checks passed
@kodiakhq kodiakhq bot deleted the feat/increase-the-number-of-auto-join-members branch June 12, 2024 14:10
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants