Skip to content

Commit 12c44d2

Browse files
fix: read receipts not turning blue when all active users have read (#39246)
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Diego Sampaio <chinello@gmail.com>
1 parent 7d19209 commit 12c44d2

14 files changed

Lines changed: 350 additions & 36 deletions

File tree

.changeset/hot-lies-divide.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
'@rocket.chat/model-typings': patch
3+
'@rocket.chat/models': patch
4+
'@rocket.chat/meteor': patch
5+
---
6+
7+
Fixes an issue where messages appeared as unread even when all active users had read them. Read receipts now correctly ignore deactivated users.

apps/meteor/app/lib/server/functions/setUserActiveStatus.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export async function setUserActiveStatus(
113113
}
114114

115115
if (user.username) {
116-
const { modifiedCount } = await Subscriptions.setArchivedByUsername(user.username, !active);
116+
const { modifiedCount } = await Subscriptions.setArchivedForDMsWithUsername(user.username, !active);
117117
if (modifiedCount) {
118118
void notifyOnSubscriptionChangedByNameAndRoomType({ t: 'd', name: user.username });
119119
}

apps/meteor/app/lib/server/functions/unarchiveRoom.ts

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,40 @@
11
import { Message } from '@rocket.chat/core-services';
22
import type { IMessage } from '@rocket.chat/core-typings';
3-
import { Rooms, Subscriptions } from '@rocket.chat/models';
3+
import { Rooms, Subscriptions, Users } from '@rocket.chat/models';
44

55
import { notifyOnRoomChangedById, notifyOnSubscriptionChangedByRoomId } from '../lib/notifyListener';
66

7+
const BATCH_SIZE = 100_000;
8+
9+
async function getActiveUserIds(userIds: string[]): Promise<Set<string>> {
10+
const activeUserIds = new Set<string>();
11+
12+
for (let i = 0; i < userIds.length; i += BATCH_SIZE) {
13+
const batch = await Users.findActiveByIds(userIds.slice(i, i + BATCH_SIZE), { projection: { _id: 1 } }).toArray();
14+
for (const u of batch) {
15+
activeUserIds.add(u._id);
16+
}
17+
}
18+
19+
return activeUserIds;
20+
}
21+
22+
async function unarchiveSubscriptionsByIds(ids: string[]): Promise<void> {
23+
for (let i = 0; i < ids.length; i += BATCH_SIZE) {
24+
await Subscriptions.unarchiveByIds(ids.slice(i, i + BATCH_SIZE));
25+
}
26+
}
27+
728
export const unarchiveRoom = async function (rid: string, user: IMessage['u']): Promise<void> {
829
await Rooms.unarchiveById(rid);
930

10-
const unarchiveResponse = await Subscriptions.unarchiveByRoomId(rid);
11-
if (unarchiveResponse.modifiedCount) {
31+
const archivedSubs = await Subscriptions.findArchivedByRoomId(rid, { projection: { 'u._id': 1 } }).toArray();
32+
33+
if (archivedSubs.length > 0) {
34+
const activeUserIds = await getActiveUserIds(archivedSubs.map((s) => s.u._id));
35+
const idsToUnarchive = archivedSubs.filter((s) => activeUserIds.has(s.u._id)).map((s) => s._id);
36+
37+
await unarchiveSubscriptionsByIds(idsToUnarchive);
1238
void notifyOnSubscriptionChangedByRoomId(rid);
1339
}
1440

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { Rooms, Subscriptions } from '@rocket.chat/models';
2+
3+
const BATCH_SIZE = 100_000;
4+
5+
async function getArchivedRoomIds(rids: string[]): Promise<Set<string>> {
6+
const archivedRoomIds = new Set<string>();
7+
8+
for (let i = 0; i < rids.length; i += BATCH_SIZE) {
9+
const batch = await Rooms.findManyArchivedByRoomIds(rids.slice(i, i + BATCH_SIZE), { projection: { _id: 1 } }).toArray();
10+
for (const r of batch) {
11+
archivedRoomIds.add(r._id);
12+
}
13+
}
14+
15+
return archivedRoomIds;
16+
}
17+
18+
async function unarchiveSubscriptionsByIds(ids: string[]): Promise<void> {
19+
for (let i = 0; i < ids.length; i += BATCH_SIZE) {
20+
await Subscriptions.unarchiveByIds(ids.slice(i, i + BATCH_SIZE));
21+
}
22+
}
23+
24+
export const unarchiveUserSubscriptions = async (userId: string): Promise<boolean> => {
25+
const archivedSubs = await Subscriptions.findArchivedByUserId(userId, { projection: { rid: 1 } }).toArray();
26+
27+
if (!archivedSubs.length) {
28+
return false;
29+
}
30+
31+
const archivedRoomIds = await getArchivedRoomIds(archivedSubs.map((s) => s.rid));
32+
const idsToUnarchive = archivedSubs.filter((s) => !archivedRoomIds.has(s.rid)).map((s) => s._id);
33+
34+
if (!idsToUnarchive.length) {
35+
return false;
36+
}
37+
38+
await unarchiveSubscriptionsByIds(idsToUnarchive);
39+
return true;
40+
};
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import type { IUser } from '@rocket.chat/core-typings';
2+
import { Subscriptions } from '@rocket.chat/models';
3+
4+
import { notifyOnSubscriptionChangedByUserId } from './notifyListener';
5+
import { callbacks } from '../../../../server/lib/callbacks';
6+
import { unarchiveUserSubscriptions } from '../functions/unarchiveUserSubscriptions';
7+
8+
const handleDeactivateUser = async (user: IUser): Promise<void> => {
9+
const { modifiedCount } = await Subscriptions.setArchivedByUserId(user._id, true);
10+
if (modifiedCount) {
11+
void notifyOnSubscriptionChangedByUserId(user._id);
12+
}
13+
};
14+
15+
const handleActivateUser = async (user: IUser): Promise<void> => {
16+
const unarchived = await unarchiveUserSubscriptions(user._id);
17+
if (unarchived) {
18+
void notifyOnSubscriptionChangedByUserId(user._id);
19+
}
20+
};
21+
22+
callbacks.add('afterDeactivateUser', handleDeactivateUser, callbacks.priority.LOW, 'subscription-archive-on-deactivate');
23+
24+
callbacks.add('afterActivateUser', handleActivateUser, callbacks.priority.LOW, 'subscription-unarchive-on-activate');

apps/meteor/app/lib/server/lib/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
for the *server* pieces of code which does include the shared
66
library files.
77
*/
8+
import './afterUserActions';
89
import './notifyUsersOnMessage';
910

1011
export { sendNotification } from './sendNotificationsOnMessage';

apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class ReadReceiptClass {
7272
}
7373

7474
// mark message as read if the sender is the only one in the room
75-
const isUserAlone = (await Subscriptions.countByRoomIdAndNotUserId(roomId, userId)) === 0;
75+
const isUserAlone = (await Subscriptions.countUnarchivedByRoomIdAndNotUserId(roomId, userId)) === 0;
7676
if (isUserAlone) {
7777
const result = await Messages.setAsReadById(message._id);
7878
if (result.modifiedCount > 0) {
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import type { Page } from '@playwright/test';
2+
3+
import { IS_EE } from './config/constants';
4+
import { createAuxContext } from './fixtures/createAuxContext';
5+
import type { IUserState } from './fixtures/userStates';
6+
import { Users } from './fixtures/userStates';
7+
import { HomeChannel } from './page-objects';
8+
import { createTargetChannel, deleteChannel, setSettingValueById } from './utils';
9+
import { expect, test } from './utils/test';
10+
import type { ITestUser } from './utils/user-helpers';
11+
import { createTestUser, loginTestUser } from './utils/user-helpers';
12+
13+
test.use({ storageState: Users.admin.state });
14+
15+
test.describe.serial('read-receipts-deactivated-users', () => {
16+
let poHomeChannel: HomeChannel;
17+
let targetChannel: string;
18+
let user1Context: { page: Page; poHomeChannel: HomeChannel } | undefined;
19+
let user2Context: { page: Page; poHomeChannel: HomeChannel } | undefined;
20+
let testUser1: ITestUser;
21+
let testUser2: ITestUser;
22+
let testUser1State: IUserState;
23+
let testUser2State: IUserState;
24+
25+
test.skip(!IS_EE, 'Enterprise Only');
26+
27+
test.beforeAll(async ({ api }) => {
28+
[testUser1, testUser2] = await Promise.all([createTestUser(api), createTestUser(api)]);
29+
30+
[testUser1State, testUser2State] = await Promise.all([loginTestUser(api, testUser1), loginTestUser(api, testUser2)]);
31+
32+
targetChannel = await createTargetChannel(api, { members: [testUser1.data.username, testUser2.data.username] });
33+
await Promise.all([
34+
setSettingValueById(api, 'Message_Read_Receipt_Enabled', true),
35+
setSettingValueById(api, 'Message_Read_Receipt_Store_Users', true),
36+
]);
37+
});
38+
39+
test.afterAll(async ({ api }) => {
40+
await Promise.all([
41+
setSettingValueById(api, 'Message_Read_Receipt_Enabled', false),
42+
setSettingValueById(api, 'Message_Read_Receipt_Store_Users', false),
43+
]);
44+
45+
await deleteChannel(api, targetChannel);
46+
await Promise.all([testUser1?.delete(), testUser2?.delete()]);
47+
});
48+
49+
test.beforeEach(async ({ page }) => {
50+
poHomeChannel = new HomeChannel(page);
51+
await page.goto('/home');
52+
});
53+
54+
test.afterEach(async () => {
55+
await Promise.all([user1Context?.page.close(), user2Context?.page.close()]);
56+
user1Context = undefined;
57+
user2Context = undefined;
58+
});
59+
60+
test('should correctly handle read receipts as users are deactivated', async ({ browser, api, page }) => {
61+
const { page: page1 } = await createAuxContext(browser, testUser1State);
62+
const user1Ctx = { page: page1, poHomeChannel: new HomeChannel(page1) };
63+
user1Context = user1Ctx;
64+
65+
const { page: page2 } = await createAuxContext(browser, testUser2State);
66+
const user2Ctx = { page: page2, poHomeChannel: new HomeChannel(page2) };
67+
user2Context = user2Ctx;
68+
69+
await Promise.all([
70+
poHomeChannel.navbar.openChat(targetChannel),
71+
user1Ctx.poHomeChannel.navbar.openChat(targetChannel),
72+
user2Ctx.poHomeChannel.navbar.openChat(targetChannel),
73+
]);
74+
75+
await test.step('when all users are active', async () => {
76+
await poHomeChannel.content.sendMessage('Message 1: All three users active');
77+
78+
await Promise.all([
79+
expect(user1Ctx.poHomeChannel.content.lastUserMessage).toBeVisible(),
80+
expect(user2Ctx.poHomeChannel.content.lastUserMessage).toBeVisible(),
81+
]);
82+
83+
await expect(poHomeChannel.content.lastUserMessage.getByRole('status', { name: 'Message viewed' })).toBeVisible();
84+
85+
await poHomeChannel.content.openLastMessageMenu();
86+
await page.locator('role=menuitem[name="Read receipts"]').click();
87+
await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(3);
88+
await page.getByRole('button', { name: 'Close' }).click();
89+
});
90+
91+
await test.step('when some users are deactivated', async () => {
92+
await api.post('/users.setActiveStatus', { userId: testUser1.data._id, activeStatus: false });
93+
94+
await poHomeChannel.content.sendMessage('Message 2: User1 deactivated, two active users');
95+
96+
await expect(user2Ctx.poHomeChannel.content.lastUserMessage).toBeVisible();
97+
98+
await expect(poHomeChannel.content.lastUserMessage.getByRole('status', { name: 'Message viewed' })).toBeVisible();
99+
100+
await poHomeChannel.content.openLastMessageMenu();
101+
await page.locator('role=menuitem[name="Read receipts"]').click();
102+
await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(2);
103+
await page.getByRole('button', { name: 'Close' }).click();
104+
});
105+
106+
await test.step('when only one user remains active (user alone in room)', async () => {
107+
await api.post('/users.setActiveStatus', { userId: testUser2.data._id, activeStatus: false });
108+
109+
await poHomeChannel.content.sendMessage('Message 3: Only admin active');
110+
111+
await expect(poHomeChannel.content.lastUserMessage.getByRole('status', { name: 'Message viewed' })).toBeVisible();
112+
113+
await poHomeChannel.content.openLastMessageMenu();
114+
await page.locator('role=menuitem[name="Read receipts"]').click();
115+
await expect(page.getByRole('dialog').getByRole('listitem')).toHaveCount(1);
116+
await page.getByRole('button', { name: 'Close' }).click();
117+
});
118+
});
119+
});

apps/meteor/tests/e2e/read-receipts.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ test.describe.serial('read-receipts', () => {
3030
await poHomeChannel.navbar.openChat(targetChannel);
3131
await poHomeChannel.content.sendMessage('hello world');
3232
await poHomeChannel.content.openLastMessageMenu();
33-
expect(page.locator('role=menuitem[name="Read receipts"]')).not.toBeVisible;
33+
await expect(page.locator('role=menuitem[name="Read receipts"]')).not.toBeVisible();
3434
});
3535
});
3636

apps/meteor/tests/e2e/utils/user-helpers.ts

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@ import type { APIResponse } from '@playwright/test';
33
import type { IUser } from '@rocket.chat/core-typings';
44

55
import type { BaseTest } from './test';
6-
import { DEFAULT_USER_CREDENTIALS } from '../config/constants';
6+
import { BASE_URL, DEFAULT_USER_CREDENTIALS } from '../config/constants';
7+
import type { IUserState } from '../fixtures/userStates';
78

89
export interface ICreateUserOptions {
910
username?: string;
@@ -62,6 +63,63 @@ export async function createTestUser(api: BaseTest['api'], options: ICreateUserO
6263
};
6364
}
6465

66+
/**
67+
* Logs in a test user via the REST API and returns an IUserState
68+
* suitable for use with createAuxContext.
69+
*
70+
* Use this instead of the pre-baked Users.userN fixtures whenever the test
71+
* will deactivate (or otherwise invalidate the session of) the user, so that
72+
* shared fixture tokens are never corrupted.
73+
*/
74+
export async function loginTestUser(api: BaseTest['api'], user: ITestUser): Promise<IUserState> {
75+
const response = await api.post('/login', {
76+
username: user.data.username,
77+
password: DEFAULT_USER_CREDENTIALS.password,
78+
});
79+
const {
80+
data: { userId, authToken },
81+
} = await response.json();
82+
83+
const expires = new Date();
84+
expires.setFullYear(expires.getFullYear() + 1);
85+
86+
return {
87+
data: {
88+
_id: userId,
89+
username: user.data.username,
90+
loginToken: authToken,
91+
loginExpire: expires,
92+
hashedToken: '',
93+
},
94+
state: {
95+
cookies: [
96+
{ sameSite: 'Lax', name: 'rc_uid', value: userId, domain: 'localhost', path: '/', expires: -1, httpOnly: false, secure: false },
97+
{
98+
sameSite: 'Lax',
99+
name: 'rc_token',
100+
value: authToken,
101+
domain: 'localhost',
102+
path: '/',
103+
expires: -1,
104+
httpOnly: false,
105+
secure: false,
106+
},
107+
],
108+
origins: [
109+
{
110+
origin: BASE_URL,
111+
localStorage: [
112+
{ name: 'userLanguage', value: 'en-US' },
113+
{ name: 'Meteor.loginToken', value: authToken },
114+
{ name: 'Meteor.loginTokenExpires', value: expires.toISOString() },
115+
{ name: 'Meteor.userId', value: userId },
116+
],
117+
},
118+
],
119+
},
120+
};
121+
}
122+
65123
/**
66124
* Creates multiple test users at once
67125
*/

0 commit comments

Comments
 (0)