fix: keep consistency with backend email validation #566

Antasel · 2023-08-16T19:09:55Z

Fixed Issues

$ Expensify/App#17387
Proposal: Expensify/App#17387 (comment), Expensify/App#17387 (comment)

Tests

Go to a chat and send following valid emails and verify that all are parsed as email

Valid emails

        // Domain length (63 chars in each label)
        test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com
        abc@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890.km
        abc@co.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890.km

        // Address length (64 chars)
        sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@test.com

        // Overall length (254 chars)
        averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com

        // Domain with lots of dashes
        sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asj-j-s-sjdjdjdjd-jdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdke.com.ab.net.aa.bb.cc.dd.ee
        abc@g---m--ai-l.com

        // Domain with repeated labels of 63 chars
        test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.com

        // TLD >=2 chars
        abc@gmail.co
        a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijk

        // Very short address
        a@example.com

        // xn-- style domain name
        test@xn--diseolatinoamericano-76b.com

        // Unusual but valid prefixes
        -test@example.com
        _test@example.com
        #test@example.com
        test.+123@example.com
        -test-@example.com

send following invalid emails and verify that the emails are not parsed as email, but only valid parts are extracted as email

Invalid emails

        $test@gmail.com
        !test@gmail.com
        "test"@gmail.com
        ~abc@gmail.com~
        abc@gmail.com~
        test@example_123site.com
        test{@example.com
        test..new@example.com
        test@example-.a.com
        test@example......a.com

        // Invalid period location
        .test@example.com
        .test.new@example.com
        test.@example.com

        // Domain too long (>63 chars in each label)
        test@averylongdomainpartoftheemailthatwillgooverthelimitasitismorethan63chars.com
        abc@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890a.km
        abc@co.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890a.km

        // Address too long (>64 chars)
        averylongaddresspartoftheemailthatwillgovoerthelimitasitismorethan64chars@example.com

        // Incorrect domains start/end
        test@example-.com
        test@-example-.com

        // TLD too short
        test@example.a

        // TLD too long
        a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijkl

Navigate to Settings > Profile > Contact Method > New Contact Method, try an valid / invalid email

Demo Video:

test-keep-consistency-with-email-validation.mp4

Autotest cases:

Str-test.js new tests

describe('Str.isValidEmail', () => {
    it('Correctly identifies valid emails', () => {
        expect(Str.isValidEmail('abc@gmail.com')).toBeTruthy();

        // Domain length (63 chars in each label)
        expect(Str.isValidEmail('test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com')).toBeTruthy();
        expect(Str.isValidEmail('abc@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890.km')).toBeTruthy();
        expect(Str.isValidEmail('abc@co.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890.km')).toBeTruthy();

        // Address length (64 chars)
        expect(Str.isValidEmail('sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@test.com')).toBeTruthy();

        // Overall length (254 chars)
        expect(Str.isValidEmail('averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com')).toBeTruthy();

        // Domain with lots of dashes
        expect(Str.isValidEmail('sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asj-j-s-sjdjdjdjd-jdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdke.com.ab.net.aa.bb.cc.dd.ee')).toBeTruthy();
        expect(Str.isValidEmail('abc@g---m--ai-l.com')).toBeTruthy();

        // Domain with repeated labels of 63 chars
        expect(Str.isValidEmail('test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.com')).toBeTruthy();

        // TLD >=2 chars
        expect(Str.isValidEmail('abc@gmail.co')).toBeTruthy();
        expect(Str.isValidEmail('a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijk')).toBeTruthy();

        // Very short address
        expect(Str.isValidEmail('a@example.com')).toBeTruthy();

        // xn-- style domain name
        expect(Str.isValidEmail('test@xn--diseolatinoamericano-76b.com')).toBeTruthy();

        // Unusual but valid prefixes
        expect(Str.isValidEmail('-test@example.com')).toBeTruthy();
        expect(Str.isValidEmail('_test@example.com')).toBeTruthy();
        expect(Str.isValidEmail('#test@example.com')).toBeTruthy();
        expect(Str.isValidEmail('test.+123@example.com')).toBeTruthy();
        expect(Str.isValidEmail('-test-@example.com')).toBeTruthy();

        // Invalid chars
        expect(Str.isValidEmail('$test@gmail.com')).toBeFalsy();
        expect(Str.isValidEmail('!test@gmail.com')).toBeFalsy();
        expect(Str.isValidEmail('"test"@gmail.com')).toBeFalsy();
        expect(Str.isValidEmail('~abc@gmail.com~')).toBeFalsy();
        expect(Str.isValidEmail('abc@gmail.com~')).toBeFalsy();
        expect(Str.isValidEmail('test@example_123site.com')).toBeFalsy();
        expect(Str.isValidEmail('test{@example.com')).toBeFalsy();
        expect(Str.isValidEmail('test..new@example.com')).toBeFalsy();
        expect(Str.isValidEmail('test@example-.a.com')).toBeFalsy();
        expect(Str.isValidEmail('test@example......a.com')).toBeFalsy();

        // Invalid period location
        expect(Str.isValidEmail('.test@example.com')).toBeFalsy();
        expect(Str.isValidEmail('.test.new@example.com')).toBeFalsy();
        expect(Str.isValidEmail('test.@example.com')).toBeFalsy();

        // Domain too long (>63 chars in each label)
        expect(Str.isValidEmail('test@averylongdomainpartoftheemailthatwillgooverthelimitasitismorethan63chars.com')).toBeFalsy();
        expect(Str.isValidEmail('abc@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890a.km')).toBeFalsy();
        expect(Str.isValidEmail('abc@co.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890a.km')).toBeFalsy();

        // Address too long (>64 chars)
        expect(Str.isValidEmail('averylongaddresspartoftheemailthatwillgovoerthelimitasitismorethan64chars@example.com')).toBeFalsy();

        // Incorrect domains start/end
        expect(Str.isValidEmail('test@example-.com')).toBeFalsy();
        expect(Str.isValidEmail('test@-example-.com')).toBeFalsy();

        // TLD too short
        expect(Str.isValidEmail('test@example.a')).toBeFalsy();

        // TLD too long
        expect(Str.isValidEmail('a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijkl')).toBeFalsy();
    });
});

ExpensiMark-HTML-test.js new tests

// Check emails within other markdown
test('Test emails within other markdown', () => {
    const testString = '> test@example.com\n'
    + '```test@example.com```\n'
    + '`test@example.com`\n'
    + '_test@example.com_ '
    + '__test@example.com_';
    const result = '<blockquote><a href="mailto:test@example.com">test@example.com</a></blockquote>'
    + '<pre>test@example.com</pre>'
    + '<code>test@example.com</code><br />'
    + '<em><a href="mailto:test@example.com">test@example.com</a></em> '
    + '<em><a href="mailto:_test@example.com">_test@example.com</a></em>';
    expect(parser.replace(testString)).toBe(result);
});

// Check email regex's validity at various limits
test('Test markdown replacement for valid emails', () => {
    const testString = 'A simple email: abc@gmail.com, '
    + 'or a very short one a@example.com '
    + 'hitting the maximum domain length (63 chars) test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com '
    + 'or the maximum address length (64 chars) sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@test.com '
    + 'overall length of 254 averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com '
    + 'domain with many dashes sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asj-j-s-sjdjdjdjd-jdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdke.com.ab.net.aa.bb.cc.dd.ee '
    + ' how about a domain with repeated labels of 63 chars test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.com '
    + 'max length email with italics '
    + '_averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com_ '
    + ' xn-- style domain test@xn--diseolatinoamericano-76b.com '
    + 'or a more complex case where we need to determine where to apply italics markdown '
    + '_email@test.com\n_email2@test.com\n\nemail3@test.com_ '
    + 'some unusual, but valid prefixes -test@example.com '
    + ' and _test@example.com '
    + 'a max length email enclosed in brackets '
    + '(averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com) '
    + 'max length email with ellipsis ending '
    + 'averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com... '
    + 'try a markdown link with a valid max-length email '
    + '[text](sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfff)';
    const result = 'A simple email: <a href="mailto:abc@gmail.com">abc@gmail.com</a>, '
    + 'or a very short one <a href="mailto:a@example.com">a@example.com</a> '
    + 'hitting the maximum domain length (63 chars) <a href="mailto:test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com">test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com</a> '
    + 'or the maximum address length (64 chars) <a href="mailto:sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@test.com">sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@test.com</a> '
    + 'overall length of 254 <a href="mailto:averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com">averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com</a> '
    + 'domain with many dashes <a href="mailto:sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asj-j-s-sjdjdjdjd-jdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdke.com.ab.net.aa.bb.cc.dd.ee">sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asj-j-s-sjdjdjdjd-jdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdke.com.ab.net.aa.bb.cc.dd.ee</a> '
    + ' how about a domain with repeated labels of 63 chars <a href="mailto:test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.com">test@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekasgasgasgasgashfnfn.com</a> '
    + 'max length email with italics '
    + '<em><a href="mailto:averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com">averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com</a></em> '
    + ' xn-- style domain <a href="mailto:test@xn--diseolatinoamericano-76b.com">test@xn--diseolatinoamericano-76b.com</a> '
    + 'or a more complex case where we need to determine where to apply italics markdown '
    + '<em><a href="mailto:email@test.com">email@test.com</a><br />'
    + '<a href="mailto:_email2@test.com">_email2@test.com</a><br /><br />'
    + '<a href="mailto:email3@test.com">email3@test.com</a></em> '
    + 'some unusual, but valid prefixes <a href="mailto:-test@example.com">-test@example.com</a> '
    + ' and <a href="mailto:_test@example.com">_test@example.com</a> '
    + 'a max length email enclosed in brackets '
    + '(<a href="mailto:averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com">averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com</a>) '
    + 'max length email with ellipsis ending '
    + '<a href="mailto:averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com">averylongaddresspartthatalmostwillreachthelimitofcharsperaddress@nowwejustneedaverylongdomainpartthatwill.reachthetotallengthlimitforthewholeemailaddress.whichis254charsaccordingtothePHPvalidate-email-filter.extendingthetestlongeruntilwereachtheright.com</a>... '
    + 'try a markdown link with a valid max-length email '
    + '<a href="mailto:sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfff">text</a>';
    expect(parser.replace(testString)).toBe(result);
});

test('Test markdown replacement for invalid emails', () => {
    const testString = 'Replace the valid email part within a string '
    + '.test@example.com '
    + '$test@gmail.com '
    + 'test..new@example.com '
    + 'Some chars that are not allowed within emails '
    + 'test{@example.com '
    + 'domain length over limit test@averylongdomainpartoftheemailthatwillgooverthelimitasitismorethan63chars.com '
    + 'address over limit averylongaddresspartoftheemailthatwillgovoerthelimitasitismorethan64chars@example.com '
    + 'overall length too long '
    + 'sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfffa '
    + 'invalid domain start/end '
    + 'test@example-.com '
    + 'test@-example-.com '
    + 'test@example.a '
    + '[text](sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfffa)';
    const result = 'Replace the valid email part within a string '
    + '.<a href="mailto:test@example.com">test@example.com</a> '
    + '$<a href="mailto:test@gmail.com">test@gmail.com</a> '
    + 'test..<a href="mailto:new@example.com">new@example.com</a> '
    + 'Some chars that are not allowed within emails '
    + 'test{@example.com '
    + 'domain length over limit test@averylongdomainpartoftheemailthatwillgooverthelimitasitismorethan63chars.com '
    + 'address over limit averylongaddresspartoftheemailthatwillgovoerthelimitasitismorethan64chars@example.com '
    + 'overall length too long '
    + 'sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfffa '
    + 'invalid domain start/end '
    + 'test@example-.com '
    + 'test@-example-.com '
    + 'test@example.a '
    + '[text](sjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjab@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.com.a.aa.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcj.asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjasfffa)';
    expect(parser.replace(testString)).toBe(result);
});

aldo-expensify · 2023-08-16T19:14:53Z

Failing to add @aimane-chnaif as reviewer 🤷

aimane-chnaif · 2023-08-16T20:16:21Z

@jjcoffee please take first round of review and test

aimane-chnaif

@Antasel please fix lint. Also, as agreed, we can skip the case of _ prefixed email inside italic markdown. This will make code much simpler.

Antasel · 2023-08-17T04:02:53Z

@aimane-chnaif
I've fixed lint

as agreed, we can skip the case of _ prefixed email inside italic markdown. This will make code much simpler.

Also restored autoemail & quote rule's order and some tweaks on Italic rule
according to the change, autotest cases are updated

jjcoffee · 2023-08-17T09:05:35Z

@aimane-chnaif Can you clarify what you mean by the case of "_ prefixed email inside italic markdown"? Do you mean:

Email with leading underscore (_test@example.com) wrapped in italic markdown: __test@example.com_
Normal email wrapped in italic markdown: _test@example.com_

jjcoffee · 2023-08-17T09:06:37Z

__tests__/ExpensiMark-HTML-test.js

@@ -102,10 +102,10 @@ test('Test markdown replacement for emails wrapped in bold/strikethrough/italic
    expect(parser.replace(testInput)).toBe('<strong><a href="mailto:abc@gmail.com">abc@gmail.com</a></strong>');

    testInput = '_abc@gmail.com_';
-    expect(parser.replace(testInput)).toBe('<em><a href="mailto:abc@gmail.com">abc@gmail.com</a></em>');
+    expect(parser.replace(testInput)).toBe('<a href="mailto:_abc@gmail.com">_abc@gmail.com</a>_');


I don't think this would be expected behaviour and would result in a regression. If you wrap a normal email (abc@gmail.com) in italic markdown it should just be italicised and linked. cc @aimane-chnaif

Agreed. If we just change parsing order of autoEmail and italic, should this be fixed?

@Antasel Let's add all of these in test case. Github markdown seems to have different logic

abc@gmail.com -> abc@gmail.com

_abc@gmail.com -> _abc@gmail.com

__abc@gmail.com -> __abc@gmail.com

abc@gmail.com_ -> abc@gmail.com_

abc@gmail.com__ -> abc@gmail.com__

_abc@gmail.com_ -> abc@gmail.com

__abc@gmail.com_ -> _abc@gmail.com

_abc@gmail.com__ -> abc@gmail.com_

__abc@gmail.com__ -> abc@gmail.com

@Antasel Let's add all of these in test case. Github markdown seems to have different logic

abc@gmail.com -> abc@gmail.com

_abc@gmail.com -> _abc@gmail.com

__abc@gmail.com -> __abc@gmail.com

abc@gmail.com_ -> abc@gmail.com_

abc@gmail.com__ -> abc@gmail.com__

_abc@gmail.com_ -> abc@gmail.com

__abc@gmail.com_ -> _abc@gmail.com

_abc@gmail.com__ -> abc@gmail.com_

__abc@gmail.com__ -> abc@gmail.com

those cases are already covered in Expensify/App#17387 (comment)
so there is no need to restore the changed order of autoemail and italic rule, just leave it as it is.
I will revert last commit.
How you think ? @jjcoffee

@Antasel Yes I think that's the only way to get the behaviour we want here. I don't think it's possible to isolate a fix for _abc@gmail.com_ from __abc@gmail.com_, which is what @aimane-chnaif was initially talking about.

@aimane-chnaif you meant "the case of _ prefixed email inside italic markdown can be skipped" is following case ?

__abc@gmail.com_ -> _abc@gmail.com
after parsed, underscore is not contained in email
if so, it can be done by only moving autoemail rule after italic rule. no need extra tweak in italic rule
cc: @aimane-chnaif

I am fine with it. @jjcoffee you too?

@Antasel Sorry I'm not clear, what would the result be for __abc@gmail.com_?

That expected to be parsed as _abc@gmail.com but _abc@gmail.com actually, @Antasel you meant correct?

@aimane-chnaif I thought you wanted _<em><a href="mailto:abc@gmail.com">abc@gmail.com</a></em> after parsed.
currently with Expensify/App#17387 (comment), it is parsed as <em><a href="mailto:_abc@gmail.com">_abc@gmail.com</a></em>

That's what I don't care too much. Either result is fine to me

jjcoffee · 2023-08-17T09:07:06Z

__tests__/ExpensiMark-HTML-test.js


    testInput = '~*_abc@gmail.com_*~';
-    expect(parser.replace(testInput)).toBe('<del><strong><em><a href="mailto:abc@gmail.com">abc@gmail.com</a></em></strong></del>');
+    expect(parser.replace(testInput)).toBe('<del><strong><a href="mailto:_abc@gmail.com">_abc@gmail.com</a>_</strong></del>');


Same here, I don't think this is the expected behaviour.

jjcoffee · 2023-08-17T10:13:58Z

@Antasel Let's add some of the markdown tests from ExpensiMark-HTML-test.js into the Tests section of the PR so it's easier to visualise how it will work for markdown cases. For example _abc@gmail.com_ would be useful, maybe some of the multiline ones too?

jjcoffee · 2023-08-17T13:02:27Z

@Antasel Please can you fix the lint issues again.

aimane-chnaif · 2023-08-17T13:04:21Z

@Antasel I just noticed that your commit signature is unverified.

jjcoffee · 2023-08-17T13:13:30Z

@Antasel See here at point 10. It might be easiest to close this PR and open a new one where you make new signed commits, then you'll get a "Verified" badge here:

Antasel · 2023-08-17T13:20:42Z

thanks, I will do it now,
but I remember I followed it before

jjcoffee · 2023-08-17T13:27:11Z

The result is not correct for .test.new@example.com:

Also doesn't work with it on its own. Can you check @Antasel?

aimane-chnaif · 2023-08-17T13:34:11Z

The result is not correct for .test.new@example.com:

Also doesn't work with it on its own. Can you check @Antasel?

This is more like a bug in Expensify/App#16762.
If it's not quick fix, can be out of scope.
As it is parsed as email in current app which is wrong, I won't say this is regression.

jjcoffee · 2023-08-17T13:41:43Z

@aimane-chnaif Are you saying it's incorrect to parse test.new@example.com as an email, or just the one with the leading dot? The former is correct as far as I know! Current production:

Antasel · 2023-08-17T13:46:25Z

@jjcoffee I am getting correctly

Antasel · 2023-08-17T13:51:21Z

@aimane-chnaif commit signature has been verified

aimane-chnaif · 2023-08-17T13:53:18Z

@Antasel is it now ready for review?

Antasel · 2023-08-17T13:57:46Z

we need to check why @jjcoffee is getting incorrectly

Antasel · 2023-08-17T13:59:02Z

@jjcoffee which platform did you check it on ?

jjcoffee · 2023-08-17T13:59:36Z

@Antasel I think all your commits need to be signed - you've only signed the new one. I think that's correct right @aimane-chnaif?

I will retest soon - I might have some cached node_modules messing things up 😄

aimane-chnaif · 2023-08-17T14:00:49Z

@Antasel there's still unsigned commit. All commits should be signed

jjcoffee · 2023-08-17T14:03:16Z

@Antasel It fails when I add it as a test, so it must be an issue with the regex:

aimane-chnaif · 2023-08-23T12:05:16Z

'Maximum Regex stack depth reached'

This was reported before and now marked as fixed - Expensify/App#21266

jjcoffee · 2023-08-23T12:07:29Z

@Antasel Can you try merging main in case the issue is resolved by the PR @aimane-chnaif mentions?

Antasel · 2023-08-23T12:11:32Z

noted, let me try

Antasel · 2023-08-23T12:17:42Z

@jjcoffee
my test was on the main where the PR had been merged

Antasel · 2023-08-23T12:23:29Z

@Antasel I'm confused, are you saying that autolink is trying to parse the domain part of this email:

yes, sure
if we disable autolink rule, no error is happened

Antasel · 2023-08-23T12:30:37Z

@jjcoffee @aimane-chnaif
you can try at your side.
on latest version of expensify-common, please limit domain level length to 10 on MARKDOWN_EMAIL regex, then autolink rule is not parsing even simple invalid email T@aaaaaaaaaaaa.aaaaaaaaaaaa.aaaaa , causing the error.
maybe #559 's contributor should check it again

Antasel · 2023-08-23T12:42:55Z

I am getting this warning, but ios app gets stuck for 10 seconds

Antasel · 2023-08-24T08:18:12Z

@aimane-chnaif @jjcoffee
I've posted more reasonable bug report (Expensify/App#21266 (comment)) on #21266 .
The issue is out of current PR's scope.
I think we can move forward.

jjcoffee · 2023-08-24T08:46:22Z

@Antasel Agree it looks like a separate issue. From what I understand, any invalid email (like a super long email that's over the length limits) will not be parsed by the email rule so the autolinker tries to parse it and fails because of issues with the autolink rule. I think it's worth reporting this as a separate bug on Slack.

I think next steps here are to add some additional manual tests:

Try to login with an invalid email + an email at the limit (e.g. just below length limit)
Navigate to Settings > Profile > Contact Method > New Contact Method, try an invalid email

I will run some platform tests today, but otherwise it's ready for review. We're running close to the 9 day limit for merging, so let's try and get this merged today! cc @aimane-chnaif

Antasel · 2023-08-24T09:11:09Z

@jjcoffee noted
Could you please make video of iOS, mobile safari, desktop Platform to save time, whilst testing ?

jjcoffee · 2023-08-24T13:27:41Z

@Antasel Actually we can ignore tests for sign-in as if FE validates, BE will still return that it's an invalid email (the error message is just slightly different). Also, there's now(?) a length limit on all email input fields, so you can't enter an email >254 chars anyway 😄

Can you add Navigate to Settings > Profile > Contact Method > New Contact Method, try an invalid email as a test to the description here please? Maybe specifically for a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijkl

Antasel · 2023-08-24T15:00:01Z

Can you add Navigate to Settings > Profile > Contact Method > New Contact Method, try an invalid email as a test to the description here please? Maybe specifically for a@a.abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijkl

I've added it and updated demo video.
now Could we move forward ?
@aimane-chnaif @jjcoffee

jjcoffee · 2023-08-24T15:20:23Z

@Antasel Thanks! Please also remove the // Overall length too long item in the invalid emails list since this will always fail the test on iOS.

jjcoffee · 2023-08-24T15:41:53Z

Tests well, just waiting for the Android build. I think this is ready for review @aimane-chnaif - please prioritise as we're getting close to the 9 day limit. Thanks!

Screenshots/Videos

Web

New Contact method

email-regex-chrome-desktop-contact-method-2023-08-24_15.32.58.mp4

Markdown

email-regex-chrome-desktop-markdown-2023-08-24_15.34.03.mp4

Mobile Web - Chrome

New Contact method

android-chrome-contact-methods.mp4

Markdown - valid

android-chrome-markdown.mp4

Markdown - invalid

Uploading android-chrome-markdown-invalid.mp4…

Mobile Web - Safari

email-regex-ios-safari-2023-08-24_17.08.50.mp4

Desktop

email-regex-mac-desktop-2023-08-24_17.15.02.mp4

iOS

New Contact method

email-regex-ios-native-contact-method-2023-08-24_17.26.22.mp4

Markdown

email-regex-ios-native-markdown-2023-08-24_16.55.48.mp4

Android

email-regex-android-2023-08-25_14.35.53.mp4

Antasel · 2023-08-24T21:16:05Z

@Antasel Thanks! Please also remove the // Overall length too long item in the invalid emails list since this will always fail the test on iOS.

It's removed

jjcoffee · 2023-08-25T12:38:39Z

Android test added! @aimane-chnaif Do you think you can review today?

aimane-chnaif · 2023-08-25T12:40:51Z

This is not expected. Italicize should not happen when there's non-blank character before starting _ and after ending _

jjcoffee · 2023-08-25T12:49:06Z

I guess this is to do with putting the autoEmail rule after the italic rule, since a~*_b@gmail.com would otherwise be parsed by autoEmail as this being the email part: _b@gmail.com. cc @Antasel

jjcoffee · 2023-08-25T12:51:34Z

@aimane-chnaif Actually this is current behaviour on main, so unrelated to this PR:

aimane-chnaif · 2023-08-25T12:52:24Z

Nvm. No matter this is bug or not, I think this can be out of scope as already happens on staging:

aimane-chnaif · 2023-08-25T13:47:06Z

Screenshots/Videos

Web

valid.emails.mov

invalid.emails.mov

contact.method.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

aimane-chnaif

LGTM!
@NikkiWines all yours

NikkiWines

Amazing 🤩 one very minor question but otherwise looks good.

NikkiWines · 2023-08-26T00:31:00Z

__tests__/Str-test.js

-        expect(Str.isValidEmail('test@gmail')).toBeFalsy();
-        expect(Str.isValidEmail('@gmail.com')).toBeFalsy();
-        expect(Str.isValidEmail('usernamelongerthan64charactersshouldnotworkaccordingtorfc822whichisusedbyphp@gmail.com')).toBeFalsy();


All of these should still be invalid, right? Why not retain these tests?

Though looks like at least the last one is already covered by the test on 184

Right. They are not intentionally removed but maybe overwritten since these are recently added in #562 and #563 after @Antasel prepared these cases for testing.
@Antasel let's add these back. I just tested these cases and still work fine.
And also pull main

@NikkiWines @aimane-chnaif
The cases have been restored.

s77rt · 2024-01-18T14:37:00Z

lib/ExpensiMark.js

+            {
+                /**
+                 * Use \b in this case because it will match on words, letters,
+                 * and _: https://www.rexegg.com/regex-boundaries.html#wordboundary
+                 * The !_blank is to prevent the `target="_blank">` section of the
+                 * link replacement from being captured Additionally, something like
+                 * `\b\_([^<>]*?)\_\b` doesn't work because it won't replace
+                 * `_https://www.test.com_`
+                 * Use [\s\S]* instead of .* to match newline
+                 */
+                name: 'italic',
+                regex: /(\b_+|\b)(?!_blank")_((?![\s_])[\s\S]*?[^\s_])_(?![^\W_])(?![^<]*(<\/pre>|<\/code>|<\/a>|<\/mention-user>|_blank))/g,
+
+                // We want to add extraLeadingUnderscores back before the <em> tag unless textWithinUnderscores starts with valid email
+                replacement: (match, extraLeadingUnderscores, textWithinUnderscores) => {
+                    if (textWithinUnderscores.includes('<pre>') || this.containsNonPairTag(textWithinUnderscores)) {
+                        return match;
+                    }
+                    if (String(textWithinUnderscores).match(`^${CONST.REG_EXP.MARKDOWN_EMAIL}`)) {
+                        return `<em>${extraLeadingUnderscores}${textWithinUnderscores}</em>`;
+                    }
+                    return `${extraLeadingUnderscores}<em>${textWithinUnderscores}</em>`;
+                },
+            },


We used to have the quote rule run before the italic rule. Changing that order caused an invalid markdown rendering for cases like:

_Msg then a > quote_

Coming from Expensify/App#26941

Antasel requested a review from a team as a code owner August 16, 2023 19:09

melvin-bot bot requested review from aldo-expensify and removed request for a team August 16, 2023 19:10

Antasel mentioned this pull request Aug 16, 2023

[HOLD for payment 2023-09-07] [$4000] Inconsistent front-end email validation across the product Expensify/App#17387

Closed

6 tasks

aldo-expensify requested a review from NikkiWines August 16, 2023 19:13

aimane-chnaif reviewed Aug 16, 2023

View reviewed changes

jjcoffee reviewed Aug 17, 2023

View reviewed changes

Antasel force-pushed the fix-#17387 branch from 3a3dc25 to 19328fa Compare August 17, 2023 12:45

Antasel force-pushed the fix-#17387 branch from 9ce4d11 to 19328fa Compare August 17, 2023 14:12

jjcoffee approved these changes Aug 24, 2023

View reviewed changes

aimane-chnaif approved these changes Aug 25, 2023

View reviewed changes

NikkiWines reviewed Aug 26, 2023

View reviewed changes

Antasel and others added 2 commits August 28, 2023 11:54

Merge branch 'Expensify:main' into fix-#17387

b98cffb

restore a few invalid emails test cases

e5f7917

NikkiWines approved these changes Aug 29, 2023

View reviewed changes

NikkiWines merged commit e63d06e into Expensify:main Aug 29, 2023
3 checks passed

s77rt reviewed Jan 18, 2024

View reviewed changes

s77rt mentioned this pull request Jan 18, 2024

[HOLD for payment 2024-01-24] Mweb-Compose-(*&~) not applied for multiline-quote message&edit compose of (_),displays extra lines Expensify/App#26941

Closed

6 tasks

fix: keep consistency with backend email validation #566

fix: keep consistency with backend email validation #566

Conversation

Antasel commented Aug 16, 2023 • edited Loading

Fixed Issues

Tests

Autotest cases:

aldo-expensify commented Aug 16, 2023

aimane-chnaif commented Aug 16, 2023

aimane-chnaif left a comment

Choose a reason for hiding this comment

Antasel commented Aug 17, 2023 • edited Loading

jjcoffee commented Aug 17, 2023

Choose a reason for hiding this comment

aimane-chnaif Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Antasel Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcoffee Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcoffee commented Aug 17, 2023

jjcoffee commented Aug 17, 2023

aimane-chnaif commented Aug 17, 2023

jjcoffee commented Aug 17, 2023

Antasel commented Aug 17, 2023

jjcoffee commented Aug 17, 2023

aimane-chnaif commented Aug 17, 2023

jjcoffee commented Aug 17, 2023 • edited Loading

Antasel commented Aug 17, 2023

Antasel commented Aug 17, 2023

aimane-chnaif commented Aug 17, 2023

Antasel commented Aug 17, 2023

Antasel commented Aug 17, 2023

jjcoffee commented Aug 17, 2023

aimane-chnaif commented Aug 17, 2023

jjcoffee commented Aug 17, 2023

aimane-chnaif commented Aug 23, 2023

jjcoffee commented Aug 23, 2023

Antasel commented Aug 23, 2023

Antasel commented Aug 23, 2023 • edited Loading

Antasel commented Aug 23, 2023

Antasel commented Aug 23, 2023 • edited Loading

Antasel commented Aug 23, 2023

Antasel commented Aug 24, 2023

jjcoffee commented Aug 24, 2023

Antasel commented Aug 24, 2023

jjcoffee commented Aug 24, 2023 • edited Loading

Antasel commented Aug 24, 2023

jjcoffee commented Aug 24, 2023

jjcoffee commented Aug 24, 2023 • edited Loading

Screenshots/Videos

Antasel commented Aug 24, 2023

jjcoffee commented Aug 25, 2023

aimane-chnaif commented Aug 25, 2023

jjcoffee commented Aug 25, 2023 • edited Loading

jjcoffee commented Aug 25, 2023

aimane-chnaif commented Aug 25, 2023

aimane-chnaif commented Aug 25, 2023

Screenshots/Videos

aimane-chnaif left a comment

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antasel commented Aug 16, 2023 •

edited

Loading

Antasel commented Aug 17, 2023 •

edited

Loading

aimane-chnaif Aug 17, 2023 •

edited

Loading

Antasel Aug 17, 2023 •

edited

Loading

jjcoffee Aug 17, 2023 •

edited

Loading

jjcoffee commented Aug 17, 2023 •

edited

Loading

Antasel commented Aug 23, 2023 •

edited

Loading

Antasel commented Aug 23, 2023 •

edited

Loading

jjcoffee commented Aug 24, 2023 •

edited

Loading

jjcoffee commented Aug 24, 2023 •

edited

Loading

jjcoffee commented Aug 25, 2023 •

edited

Loading