Skip to content

Commit

Permalink
fix: use setImmediate to handle username update (#30500)
Browse files Browse the repository at this point in the history
  • Loading branch information
tapiarafael authored Oct 11, 2023
1 parent 38e1216 commit 4a59798
Show file tree
Hide file tree
Showing 5 changed files with 258 additions and 59 deletions.
5 changes: 5 additions & 0 deletions .changeset/wicked-jars-double.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@rocket.chat/meteor": patch
---

Handle the username update in the background
1 change: 1 addition & 0 deletions apps/meteor/app/lib/server/functions/saveUser.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ export const saveUser = async function (userId, userData) {
_id: userData._id,
username: userData.username,
name: userData.name,
updateUsernameInBackground: true,
}))
) {
throw new Meteor.Error('error-could-not-save-identity', 'Could not save user identity', {
Expand Down
129 changes: 93 additions & 36 deletions apps/meteor/app/lib/server/functions/saveUserIdentity.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { IUser } from '@rocket.chat/core-typings';
import { Messages, VideoConference, LivechatDepartmentAgents, Rooms, Subscriptions, Users } from '@rocket.chat/models';

import { SystemLogger } from '../../../../server/lib/logger/system';
import { FileUpload } from '../../../file-upload/server';
import { _setRealName } from './setRealName';
import { _setUsername } from './setUsername';
Expand All @@ -11,7 +13,17 @@ import { validateName } from './validateName';
* @param {object} changes changes to the user
*/

export async function saveUserIdentity({ _id, name: rawName, username: rawUsername }: { _id: string; name?: string; username?: string }) {
export async function saveUserIdentity({
_id,
name: rawName,
username: rawUsername,
updateUsernameInBackground = false,
}: {
_id: string;
name?: string;
username?: string;
updateUsernameInBackground?: boolean; // TODO: remove this
}) {
if (!_id) {
return false;
}
Expand Down Expand Up @@ -48,46 +60,91 @@ export async function saveUserIdentity({ _id, name: rawName, username: rawUserna

// if coming from old username, update all references
if (previousUsername) {
if (usernameChanged && typeof rawUsername !== 'undefined') {
const fileStore = FileUpload.getStore('Avatars');
const previousFile = await fileStore.model.findOneByName(previousUsername);
const file = await fileStore.model.findOneByName(username);
if (file) {
await fileStore.model.deleteFile(file._id);
}
if (previousFile) {
await fileStore.model.updateFileNameById(previousFile._id, username);
}

await Messages.updateAllUsernamesByUserId(user._id, username);
await Messages.updateUsernameOfEditByUserId(user._id, username);

const cursor = Messages.findByMention(previousUsername);
for await (const msg of cursor) {
const updatedMsg = msg.msg.replace(new RegExp(`@${previousUsername}`, 'ig'), `@${username}`);
await Messages.updateUsernameAndMessageOfMentionByIdAndOldUsername(msg._id, previousUsername, username, updatedMsg);
}

await Rooms.replaceUsername(previousUsername, username);
await Rooms.replaceMutedUsername(previousUsername, username);
await Rooms.replaceUsernameOfUserByUserId(user._id, username);
await Subscriptions.setUserUsernameByUserId(user._id, username);

await LivechatDepartmentAgents.replaceUsernameOfAgentByUserId(user._id, username);
const handleUpdateParams = {
username,
previousUsername,
rawUsername,
usernameChanged,
user,
name,
previousName,
rawName,
nameChanged,
};
if (updateUsernameInBackground) {
setImmediate(async () => {
try {
await updateUsernameReferences(handleUpdateParams);
} catch (err) {
SystemLogger.error(err);
}
});
} else {
await updateUsernameReferences(handleUpdateParams);
}
}

return true;
}

// update other references if either the name or username has changed
if (usernameChanged || nameChanged) {
// update name and fname of 1-on-1 direct messages
await Subscriptions.updateDirectNameAndFnameByName(previousUsername, rawUsername && username, rawName && name);
async function updateUsernameReferences({
username,
previousUsername,
rawUsername,
usernameChanged,
user,
name,
previousName,
rawName,
nameChanged,
}: {
username: string;
previousUsername: string;
rawUsername?: string;
usernameChanged: boolean;
user: IUser;
name: string;
previousName: string | undefined;
rawName?: string;
nameChanged: boolean;
}): Promise<void> {
if (usernameChanged && typeof rawUsername !== 'undefined') {
const fileStore = FileUpload.getStore('Avatars');
const previousFile = await fileStore.model.findOneByName(previousUsername);
const file = await fileStore.model.findOneByName(username);
if (file) {
await fileStore.model.deleteFile(file._id);
}
if (previousFile) {
await fileStore.model.updateFileNameById(previousFile._id, username);
}

// update name and fname of group direct messages
await updateGroupDMsName(user);
await Messages.updateAllUsernamesByUserId(user._id, username);
await Messages.updateUsernameOfEditByUserId(user._id, username);

// update name and username of users on video conferences
await VideoConference.updateUserReferences(user._id, username || previousUsername, name || previousName);
const cursor = Messages.findByMention(previousUsername);
for await (const msg of cursor) {
const updatedMsg = msg.msg.replace(new RegExp(`@${previousUsername}`, 'ig'), `@${username}`);
await Messages.updateUsernameAndMessageOfMentionByIdAndOldUsername(msg._id, previousUsername, username, updatedMsg);
}

await Rooms.replaceUsername(previousUsername, username);
await Rooms.replaceMutedUsername(previousUsername, username);
await Rooms.replaceUsernameOfUserByUserId(user._id, username);
await Subscriptions.setUserUsernameByUserId(user._id, username);

await LivechatDepartmentAgents.replaceUsernameOfAgentByUserId(user._id, username);
}

return true;
// update other references if either the name or username has changed
if (usernameChanged || nameChanged) {
// update name and fname of 1-on-1 direct messages
await Subscriptions.updateDirectNameAndFnameByName(previousUsername, rawUsername && username, rawName && name);

// update name and fname of group direct messages
await updateGroupDMsName(user);

// update name and username of users on video conferences
await VideoConference.updateUserReferences(user._id, username || previousUsername, name || previousName);
}
}
48 changes: 25 additions & 23 deletions apps/meteor/tests/end-to-end/api/09-rooms.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import path from 'path';
import { expect } from 'chai';
import { after, afterEach, before, beforeEach, describe, it } from 'mocha';

import { sleep } from '../../../lib/utils/sleep';
import { getCredentials, api, request, credentials } from '../../data/api-data.js';
import { sendSimpleMessage, deleteMessage } from '../../data/chat.helper';
import { imgURL } from '../../data/interactions.js';
Expand Down Expand Up @@ -1543,29 +1544,30 @@ describe('[Rooms]', function () {
roomId = result.body.room.rid;
});

it('should update group name if user changes username', (done) => {
updateSetting('UI_Use_Real_Name', false).then(() => {
request
.post(api('users.update'))
.set(credentials)
.send({
userId: testUser._id,
data: {
username: `changed.username.${testUser.username}`,
},
})
.end(() => {
request
.get(api('subscriptions.getOne'))
.set(credentials)
.query({ roomId })
.end((err, res) => {
const { subscription } = res.body;
expect(subscription.name).to.equal(`rocket.cat,changed.username.${testUser.username}`);
done();
});
});
});
it('should update group name if user changes username', async () => {
await updateSetting('UI_Use_Real_Name', false);
await request
.post(api('users.update'))
.set(credentials)
.send({
userId: testUser._id,
data: {
username: `changed.username.${testUser.username}`,
},
});

// need to wait for the username update finish
await sleep(300);

await request
.get(api('subscriptions.getOne'))
.set(credentials)
.query({ roomId })
.send()
.expect((res) => {
const { subscription } = res.body;
expect(subscription.name).to.equal(`rocket.cat,changed.username.${testUser.username}`);
});
});

it('should update group name if user changes name', (done) => {
Expand Down
134 changes: 134 additions & 0 deletions apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import { expect } from 'chai';
import proxyquire from 'proxyquire';
import sinon from 'sinon';

// Create stubs for dependencies
const stubs = {
findOneUserById: sinon.stub(),
updateUsernameAndMessageOfMentionByIdAndOldUsername: sinon.stub(),
updateUsernameOfEditByUserId: sinon.stub(),
updateAllUsernamesByUserId: sinon.stub(),
updateDirectNameAndFnameByName: sinon.stub(),
updateUserReferences: sinon.stub(),
setUsername: sinon.stub(),
setRealName: sinon.stub(),
validateName: sinon.stub(),
FileUpload: sinon.stub(),
};

const { saveUserIdentity } = proxyquire.noCallThru().load('../../../../app/lib/server/functions/saveUserIdentity', {
'@rocket.chat/models': {
Users: {
findOneById: stubs.findOneUserById,
},
Messages: {
updateUsernameAndMessageOfMentionByIdAndOldUsername: stubs.updateUsernameAndMessageOfMentionByIdAndOldUsername,
updateUsernameOfEditByUserId: stubs.updateUsernameOfEditByUserId,
updateAllUsernamesByUserId: stubs.updateAllUsernamesByUserId,
},
Subscriptions: {
updateDirectNameAndFnameByName: stubs.updateDirectNameAndFnameByName,
},
VideoConference: {
updateUserReferences: stubs.updateUserReferences,
},
},
'meteor/meteor': {
'Meteor': sinon.stub(),
'@global': true,
},
'../../../../app/file-upload/server': {
FileUpload: stubs.FileUpload,
},
'../../../../app/lib/server/functions/setRealName': {
_setRealName: stubs.setRealName,
},
'../../../../app/lib/server/functions/setUsername': {
_setUsername: stubs.setUsername,
},
'../../../../app/lib/server/functions/updateGroupDMsName': {
updateGroupDMsName: sinon.stub(),
},
'../../../../app/lib/server/functions/validateName': {
validateName: stubs.validateName,
},
});

describe('Users - saveUserIdentity', () => {
beforeEach(() => {
// Reset stubs before each test
Object.values(stubs).forEach((stub) => stub.reset());
});

it('should return false if _id is not provided', async () => {
const result = await saveUserIdentity({ _id: undefined });

expect(stubs.findOneUserById.called).to.be.false;
expect(result).to.be.false;
});

it('should return false if user does not exist', async () => {
stubs.findOneUserById.returns(undefined);
const result = await saveUserIdentity({ _id: 'valid_id' });

expect(stubs.findOneUserById.calledWith('valid_id')).to.be.true;
expect(result).to.be.false;
});

it('should return false if username is not allowed', async () => {
stubs.findOneUserById.returns({ username: 'oldUsername' });
stubs.validateName.returns(false);
const result = await saveUserIdentity({ _id: 'valid_id', username: 'admin' });

expect(stubs.validateName.calledWith('admin')).to.be.true;
expect(result).to.be.false;
});

it('should return false if username is invalid or unavailable', async () => {
stubs.findOneUserById.returns({ username: 'oldUsername' });
stubs.validateName.returns(true);
stubs.setUsername.returns(false);
const result = await saveUserIdentity({ _id: 'valid_id', username: 'invalidUsername' });

expect(stubs.validateName.calledWith('invalidUsername')).to.be.true;
expect(stubs.setUsername.calledWith('valid_id', 'invalidUsername', { username: 'oldUsername' })).to.be.true;
expect(result).to.be.false;
});

it("should not update the username if it's not changed", async () => {
stubs.findOneUserById.returns({ username: 'oldUsername', name: 'oldName' });
stubs.validateName.returns(true);
stubs.setUsername.returns(true);
await saveUserIdentity({ _id: 'valid_id', username: 'oldUsername', name: 'oldName' });

expect(stubs.validateName.called).to.be.false;
expect(stubs.setUsername.called).to.be.false;
expect(stubs.updateUsernameOfEditByUserId.called).to.be.false;
expect(stubs.updateAllUsernamesByUserId.called).to.be.false;
expect(stubs.updateUsernameAndMessageOfMentionByIdAndOldUsername.called).to.be.false;
expect(stubs.updateDirectNameAndFnameByName.called).to.be.false;
expect(stubs.updateUserReferences.called).to.be.false;
});

it('should return false if _setName fails', async () => {
stubs.findOneUserById.returns({ name: 'oldName' });
stubs.setRealName.returns(false);
const result = await saveUserIdentity({ _id: 'valid_id', name: 'invalidName' });

expect(stubs.setRealName.calledWith('valid_id', 'invalidName', { name: 'oldName' })).to.be.true;
expect(result).to.be.false;
});

it('should update Subscriptions and VideoConference if name changes', async () => {
stubs.findOneUserById.returns({ name: 'oldName', username: 'oldUsername' });
stubs.setRealName.returns(true);
const result = await saveUserIdentity({ _id: 'valid_id', name: 'name', username: 'oldUsername' });

expect(stubs.setUsername.called).to.be.false;
expect(stubs.setRealName.called).to.be.true;
expect(stubs.updateUsernameOfEditByUserId.called).to.be.false;
expect(stubs.updateDirectNameAndFnameByName.called).to.be.true;
expect(stubs.updateUserReferences.called).to.be.true;
expect(result).to.be.true;
});
});

0 comments on commit 4a59798

Please sign in to comment.