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: user highlights matching just part of words #33776

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clean-flies-collect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Fix user highlights not matching only whole words
19 changes: 0 additions & 19 deletions apps/meteor/app/lib/server/functions/notifications/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,22 +47,3 @@ export function replaceMentionedUsernamesWithFullNames(message: string, mentions
});
return message;
}

/**
* Checks if a message contains a user highlight
*
* @param {string} message
* @param {array|undefined} highlights
*
* @returns {boolean}
*/
export function messageContainsHighlight(message: Pick<IMessage, 'msg'>, highlights: string[] | undefined): boolean {
if (!highlights || highlights.length === 0) {
return false;
}

return highlights.some((highlight: string) => {
const regexp = new RegExp(escapeRegExp(highlight), 'i');
return regexp.test(message.msg);
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import type { IMessage } from '@rocket.chat/core-typings';
import { escapeRegExp } from '@rocket.chat/string-helpers';

/**
* Checks if a message contains a user highlight
*
* @param {string} message
* @param {array|undefined} highlights
*
* @returns {boolean}
*/
export function messageContainsHighlight(message: Pick<IMessage, 'msg'>, highlights: string[] | undefined): boolean {
if (!highlights || highlights.length === 0) {
return false;
}

return highlights.some((highlight: string) => {
const regexp = new RegExp(`\\b${escapeRegExp(highlight)}\\b`, 'i');
Copy link
Member

@matheusbsilva137 matheusbsilva137 Oct 25, 2024

Choose a reason for hiding this comment

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

This will still make us highlight emojis (:highlight:), won't it? 🤔 I wonder if we should take the chance to fix this too (wdyt, do you consider them words too? I've already seem some complaints about this)

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm.. good point.. let me create a few unit tests

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to not match emojis.. not the prettiest regex but that's the one I could come up with

return regexp.test(message.msg);
});
}
11 changes: 1 addition & 10 deletions apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,17 @@ import type { IMessage, IRoom, IUser, RoomType } from '@rocket.chat/core-typings
import { isEditedMessage } from '@rocket.chat/core-typings';
import type { Updater } from '@rocket.chat/models';
import { Subscriptions, Rooms } from '@rocket.chat/models';
import { escapeRegExp } from '@rocket.chat/string-helpers';
import moment from 'moment';

import { callbacks } from '../../../../lib/callbacks';
import { settings } from '../../../settings/server';
import { messageContainsHighlight } from '../functions/notifications/messageContainsHighlight';
import {
notifyOnSubscriptionChanged,
notifyOnSubscriptionChangedByRoomIdAndUserId,
notifyOnSubscriptionChangedByRoomIdAndUserIds,
} from './notifyListener';

function messageContainsHighlight(message: IMessage, highlights: string[]): boolean {
if (!highlights || highlights.length === 0) return false;

return highlights.some((highlight: string) => {
const regexp = new RegExp(escapeRegExp(highlight), 'i');
return regexp.test(message.msg);
});
}

export async function getMentions(message: IMessage): Promise<{ toAll: boolean; toHere: boolean; mentionIds: string[] }> {
const {
mentions,
Expand Down
3 changes: 2 additions & 1 deletion apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ import { roomCoordinator } from '../../../../server/lib/rooms/roomCoordinator';
import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
import { Notification } from '../../../notification-queue/server/NotificationQueue';
import { settings } from '../../../settings/server';
import { messageContainsHighlight, parseMessageTextPerUser, replaceMentionedUsernamesWithFullNames } from '../functions/notifications';
import { parseMessageTextPerUser, replaceMentionedUsernamesWithFullNames } from '../functions/notifications';
import { notifyDesktopUser, shouldNotifyDesktop } from '../functions/notifications/desktop';
import { getEmailData, shouldNotifyEmail } from '../functions/notifications/email';
import { messageContainsHighlight } from '../functions/notifications/messageContainsHighlight';
import { getPushData, shouldNotifyMobile } from '../functions/notifications/mobile';
import { getMentions } from './notifyUsersOnMessage';

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';

import { messageContainsHighlight } from '../../../../../../../app/lib/server/functions/notifications/messageContainsHighlight';

describe('messageContainsHighlight', () => {
it('should return false for no highlights', async () => {
const message = {
msg: 'regular message',
};
expect(messageContainsHighlight(message, [])).to.be.false;
});

it('should return true when find a highlight in the beggining of the message', async () => {
const message = {
msg: 'highlighted regular message',
};
expect(messageContainsHighlight(message, ['highlighted'])).to.be.true;
});

it('should return true when find a highlight in the end of the message', async () => {
const message = {
msg: 'highlighted regular message',
};
expect(messageContainsHighlight(message, ['message'])).to.be.true;
});

it('should return false if the highlight is just part of the word', async () => {
const message = {
msg: 'highlighted regular message',
};
expect(messageContainsHighlight(message, ['light'])).to.be.false;
});

it('should return true if find one of the multiple highlights', async () => {
const message = {
msg: 'highlighted regular message',
};
expect(messageContainsHighlight(message, ['high', 'ssage', 'regular', 'light'])).to.be.true;
});
});
Loading