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

Add handling short mentions tags to ExpensiMark #824

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
module.exports = {
extends: ['expensify', 'prettier'],
parser: '@typescript-eslint/parser',
env: {
jest: true,
},
overrides: [
{
files: ['*.js', '*.jsx'],
Expand Down
36 changes: 28 additions & 8 deletions __tests__/ExpensiMark-HTML-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* eslint-disable max-len */
/* eslint-disable max-len,no-useless-concat */
import ExpensiMark from '../lib/ExpensiMark';

const parser = new ExpensiMark();
Expand Down Expand Up @@ -693,9 +693,9 @@ test('Test url replacements', () => {
'<a href="http://example.com/foo/*/bar/*/test.txt" target="_blank" rel="noreferrer noopener">http://example.com/foo/*/bar/*/test.txt</a> ' +
'test-.com ' +
'-<a href="https://test.com" target="_blank" rel="noreferrer noopener">test.com</a> ' +
'@test.com ' +
'@test.com <a href="https://test.com" target="_blank" rel="noreferrer noopener">test.com</a> ' +
'@test.com @test.com ';
'<mention-short>@test.com</mention-short> ' +
'<mention-short>@test.com</mention-short> <a href="https://test.com" target="_blank" rel="noreferrer noopener">test.com</a> ' +
'<mention-short>@test.com</mention-short> <mention-short>@test.com</mention-short> ';

expect(parser.replace(urlTestStartString)).toBe(urlTestReplacedString);
});
Expand Down Expand Up @@ -876,7 +876,7 @@ test('Test urls autolinks correctly', () => {
{
testString: 'expensify.com -expensify.com @expensify.com',
resultString:
'<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> -<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> @expensify.com',
'<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> -<a href="https://expensify.com" target="_blank" rel="noreferrer noopener">expensify.com</a> <mention-short>@expensify.com</mention-short>',
},
{
testString: 'https//www.expensify.com',
Expand Down Expand Up @@ -1376,7 +1376,7 @@ test('Test for user mention without leading whitespace', () => {

test('Test for user mention with @username@expensify', () => {
const testString = '@username@expensify';
const resultString = '@username@expensify';
const resultString = '<mention-short>@username</mention-short><mention-short>@expensify</mention-short>';
expect(parser.replace(testString)).toBe(resultString);
});

Expand Down Expand Up @@ -1452,6 +1452,26 @@ test('Test for @here mention with inlineCodeBlock style', () => {
expect(parser.replace(testString)).toBe(resultString);
});

describe('Tests for short mentions', () => {
test('short mentions should work for @username', () => {
const testString = '@johnny';
const resultString = '<mention-short>@johnny</mention-short>';
expect(parser.replace(testString)).toBe(resultString);
});

test('short mentions should work for @firstname.lastname', () => {
const testString = '@john.doe';
const resultString = '<mention-short>@john.doe</mention-short>';
expect(parser.replace(testString)).toBe(resultString);
});

test('short mentions should work and not break @here after mention', () => {
const testString = '@john.doe@here';
const resultString = '<mention-short>@john.doe</mention-short><mention-here>@here</mention-here>';
expect(parser.replace(testString)).toBe(resultString);
});
});

// Examples that should match for here mentions:
test('Test for here mention with @here', () => {
const testString = '@here';
Expand Down Expand Up @@ -1650,13 +1670,13 @@ test('Skip rendering invalid markdown', () => {

test('Test for email with test+1@gmail.com@gmail.com', () => {
const testString = 'test+1@gmail.com@gmail.com';
const resultString = '<a href="mailto:test+1@gmail.com">test+1@gmail.com</a>@gmail.com';
const resultString = '<a href="mailto:test+1@gmail.com">test+1@gmail.com</a><mention-short>@gmail.com</mention-short>';
expect(parser.replace(testString)).toBe(resultString);
});

test('Test for email with test@gmail.com@gmail.com', () => {
const testString = 'test@gmail.com@gmail.com';
const resultString = '<a href="mailto:test@gmail.com">test@gmail.com</a>@gmail.com';
const resultString = '<a href="mailto:test@gmail.com">test@gmail.com</a><mention-short>@gmail.com</mention-short>';
expect(parser.replace(testString)).toBe(resultString);
});

Comment on lines 1671 to 1681

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah indeed test+1@gmail.com@gmail.com looks like invalid email.
But I thought the reasoning was that the parser should treat it as a separate email (1st part) and then just text @gmail.com which now becomes a short mention.

Do you want me to change anything related to this?

Choose a reason for hiding this comment

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

No.

Expand Down
1 change: 0 additions & 1 deletion __tests__/ExpensiMark-test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* eslint-disable max-len */
import ExpensiMark from '../lib/ExpensiMark';
import * as Utils from '../lib/utils';
import {any, string} from "prop-types";

const parser = new ExpensiMark();

Expand Down
5 changes: 5 additions & 0 deletions lib/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,11 @@ const CONST = {
*/
EMOJI_RULE:
/[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu,

/**
* Regex to match a piece of text or @here, needed for both shortMention and userMention
*/
PRE_MENTION_TEXT_PART: '(@here|[a-zA-Z0-9.!$%&+=?^\\`{|}-]?)',
},

REPORT: {
Expand Down
37 changes: 36 additions & 1 deletion lib/ExpensiMark.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ export default class ExpensiMark {
{
name: 'userMentions',
regex: new RegExp(
`(@here|[a-zA-Z0-9.!$%&+=?^\`{|}-]?)(@${Constants.CONST.REG_EXP.EMAIL_PART}|@${Constants.CONST.REG_EXP.PHONE_PART})(?!((?:(?!<a).)+)?<\\/a>|[^<]*(<\\/pre>|<\\/code>))`,
`${Constants.CONST.REG_EXP.PRE_MENTION_TEXT_PART}(@${Constants.CONST.REG_EXP.EMAIL_PART}|@${Constants.CONST.REG_EXP.PHONE_PART})(?!((?:(?!<a).)+)?<\\/a>|[^<]*(<\\/pre>|<\\/code>))`,
'gim',
),
replacement: (_extras, match, g1, g2) => {
Expand Down Expand Up @@ -484,6 +484,41 @@ export default class ExpensiMark {
rawInputReplacement: '$1<a href="mailto:$2" data-raw-href="$2" data-link-variant="auto">$2</a>',
},

/**
* This regex matches a short user mention in a string.
* A short-mention is a string that starts with the '@' symbol and is followed by a valid user's primary login without the email domain part
* Ex: @john.doe, @user12345, but NOT @user@email.com
*
* Notes:
* Phone is not a valid short mention.
* In reality these "short-mentions" are just possible candidates, because the parser has no way of verifying if there exists a user named ex: @john.examplename.
* The actual verification whether these mentions are pointing to real users is done in specific projects using ExpensiMark.
* Nevertheless, "@john.examplename" is a correct possible short-mention, and so would be parsed.
* This behaviour is similar to treating every user@something as valid user login.
*
* This regex will correctly preserve any @here mentions, the same way as "userMention" rule.
*/
{
name: 'shortMentions',

regex: new RegExp(
`${Constants.CONST.REG_EXP.PRE_MENTION_TEXT_PART}(@(?=((?=[\\w]+[\\w'#%+-]+(?:\\.[\\w'#%+-]+)*)[\\w\\.'#%+-]{1,64}(?= |_|\\b))(?!([:\\/\\\\]))(?<end>.*))(?!here)\\S{3,254}(?=\\k<end>$))(?!((?:(?!<a).)+)?<\\/a>|[^<]*(<\\/pre>|<\\/code>|<\\/mention-user>|<\\/mention-here>))`,
'gim',
),
replacement: (_extras, match, g1, g2) => {
if (!Str.isValidMention(match)) {
return match;
}
return `${g1}<mention-short>${g2}</mention-short>`;
},
},

{
name: 'hereMentionAfterShortMentions',
regex: /(<\/mention-short>)(@here)(?=\b)/gm,
replacement: '$1<mention-here>$2</mention-here>',
},
Comment on lines +516 to +520

Choose a reason for hiding this comment

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

@Kicu Is this still needed? Since patch does not have this code and it works fine for me.

Copy link
Member Author

@Kicu Kicu Feb 21, 2025

Choose a reason for hiding this comment

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

yes I verified this is still needed.
Without it such string don't work: @mateusz.titz@here

It's a mistake I didn't add it to the patch, sorry.

Without this:
Screenshot 2025-02-21 at 14 31 37
With this:
Screenshot 2025-02-21 at 14 31 52

Copy link
Member Author

Choose a reason for hiding this comment

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

you should be able to test by yourself, you can just copy-paste this block and add it to the same file that patch modifies

Choose a reason for hiding this comment

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

@Kicu One improvement note: Add some border radius so that we know these are separate mentions in RN Live Markdown.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding rounded borders is quite a complicated feature on which we are already working, and for now we cannot implement it for native, only for web.
Please refer to this issue: Expensify/App#56560 which adds it only for web.
And this an even older issue related to rounded corners: Expensify/App#19299

In short: for now we can have it only on web, and a colleague is working on this. So I won't be touching styles in this PR


{
// Use \B in this case because \b doesn't match * or ~.
// \B will match everything that \b doesn't, so it works
Expand Down
Loading