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] Active users count on @all and @here #25957

Merged
merged 12 commits into from
Aug 25, 2022
Merged

Conversation

LucianoPierdona
Copy link
Contributor

@LucianoPierdona LucianoPierdona commented Jun 22, 2022

Proposed changes (including videos or screenshots)

this PR updates the old roomMembersCount to count active users instead of everyone

Issue(s)

Steps to test or reproduce

Open rocket.chat
Create a channel/room with all the members
Go to Administration > Settings > General > Notifications.
update Max Room Members Before Disabling All Message Notifications to 2 (if it matches the above requirements, if not, please use the number of active users you have in the room).
send a message with one of the active users and check if you've received a notification with the other one.

Expected Result:

Active users should've received the notification (except the one that send it).

Further comments

@LucianoPierdona LucianoPierdona self-assigned this Jun 22, 2022
@LucianoPierdona LucianoPierdona marked this pull request as ready for review June 22, 2022 16:42
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jul 11, 2022
@alvaropmello alvaropmello added this to the 5.0.1 milestone Jul 21, 2022
@alvaropmello alvaropmello modified the milestones: 5.0.1, 5.1.0 Jul 28, 2022
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #25957 (6257d93) into develop (0ba7bbc) will increase coverage by 0.04%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #25957      +/-   ##
===========================================
+ Coverage    38.65%   38.70%   +0.04%     
===========================================
  Files          792      792              
  Lines        18892    18894       +2     
  Branches      1937     1937              
===========================================
+ Hits          7303     7313      +10     
+ Misses       11300    11294       -6     
+ Partials       289      287       -2     
Flag Coverage Δ
e2e 38.70% <75.00%> (+0.04%) ⬆️

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

gabriellsh
gabriellsh previously approved these changes Aug 18, 2022
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

using a $lookup here is not ideal.. large rooms will have issues performing it..

to illustrate this, I ran both codes (the old and the one) against the top 10 rooms in one of our servers and this is the result:

the code:

rooms.forEach(function(rid) {
	const now = Date.now();
	db.rocketchat_subscription.find({ rid }).count();
	print(rid + ": " + (Date.now() - now));
});

rooms.forEach(function(rid) {
	const now = Date.now();
	db.rocketchat_subscription.aggregate([
		{ $match: { rid } },
		{
			$lookup: {
				from: 'users',
				localField: 'u._id',
				foreignField: '_id',
				as: 'receiver',
			},
		},
		{ $match: { 'receiver.active': true } },
		{ $count: 'count' },
	]);
	print(rid + ": " + (Date.now() - now));
});

the results:

id members .count() in ms .aggregate() in ms
room1 38072 21 4191
room2 36770 16 3833
room3 10200 4 1102
room4 3843 2 401
room5 2225 2 245
room6 1629 1 179
room7 1454 1 160
room8 1148 1 125
room9 899 0 99
room10 810 1 88

I also found out we already "archive" subscriptions of deactivated users, but for some reason we do that only for DMs (look at Subscriptions.setArchivedByUsername).. if we change that to all subscription types, we can filter for not archives.

@sampaiodiego
Copy link
Member

sampaiodiego commented Aug 22, 2022

so, I remembered we have a property called __rooms in IUser, this fields stores every room ID the user is joined, except DMs.. since this field is indexed, it can be used to achieve the same results without the need to use $lookup from Subscriptions..

btw, my previous comment is still valid, this is just another solution that is easier to implement.. I think we should get rid of the __rooms field in the future.. in the meantime, we can use it for this.

@LucianoPierdona
Copy link
Contributor Author

LucianoPierdona commented Aug 22, 2022

so, I remembered we have a property called __rooms in IUser, this fields stores every room ID the user is joined, except DMs.. since this field is indexed, it can be used to achieve the same results without the need to use $lookup from Subscriptions..

btw, my previous comment is still valid, this is just another solution that is easier to implement.. I think we should get rid of the __rooms field in the future.. in the meantime, we can use it for this.

Query updated!

@kodiakhq kodiakhq bot merged commit 063de7d into develop Aug 25, 2022
@kodiakhq kodiakhq bot deleted the fix/count-active-users branch August 25, 2022 04:34
gabriellsh added a commit that referenced this pull request Aug 26, 2022
…hreads

* 'develop' of github.com:RocketChat/Rocket.Chat: (93 commits)
  Chore: Upgrade dependencies (#26694)
  Chore: More Omnichannel tests (#26691)
  Regression: Banner - Room not found - Omnichannel room (#26693)
  [NEW] Capability to search visitors by custom fields (#26312)
  Chore: Create tests for Omnichannel admin add a custom fields (#26609)
  [FIX] Avatars of other chats disappear when they located near chat with broken avatar (#26689)
  [IMPROVE] Added identification on calls to/from existing contacts (#26334)
  Regression: invalid statistics format  (#26684)
  Regression: "Cache size is not a function" error when booting (#26683)
  [FIX] Correct IMAP configuration for email inbox (#25789)
  [FIX] Active users count on `@all` and `@here`  (#25957)
  [FIX] Autotranslate method should respect setting (#26549)
  Chore: Remove italic/bold font-style from system messages (#26655)
  Chore: Convert AppSetting to tsx (#26625)
  Chore: Remove & Test old closeChat templates (#26631)
  [IMPROVE] General federation improvements (#26150)
  [NEW] Warn admins about running multiple instances of the monolith (#26667)
  Regression: Prevent message from being temp forever (#26668)
  Regression: Add alsoSendThreadToChannel to user settings api (#26663)
  [IMPROVE] Spotlight search user results (#26599)
  ...
csuadev pushed a commit that referenced this pull request Aug 26, 2022
Co-authored-by: Matheus Barbosa Silva <[email protected]>
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants