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

Issue-26016 make usermention case insensitive #591

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

nikhildewoolkar
Copy link
Contributor

Fixed Issues

$ GH_LINK
Expensify/App#26016

Tests

Open the app
Open any report
Write '@' to trigger mentions
Select any one mention, make any letter of mention as capital and send
Hover on mention to observe that tooltip doesn't display the right display name and avatar
For same mention as step 4 or select different mention and capitalize all letters and send
Observe that now it is not displayed as mention

@neil-marcellini
Copy link
Contributor

@sobitneupane would you please comment here so I can assign you for review? @nikhildewoolkar please link the fixed issue properly next time so that auto assignment runs.

__tests__/ExpensiMark-HTML-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-HTMLToText-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-HTMLToText-test.js Outdated Show resolved Hide resolved
Comment on lines 695 to 717
let testString = '@user@domain.com';
expect(parser.replace(testString)).toBe('<mention-user>@user@domain.com</mention-user>');

testString = '@USER@DOMAIN.COM';
expect(parser.replace(testString)).toBe('<mention-user>@user@domain.com</mention-user>');

testString = '@USER@domain.com';
expect(parser.replace(testString)).toBe('<mention-user>@user@domain.com</mention-user>');

testString = '@user@DOMAIN.com';
expect(parser.replace(testString)).toBe('<mention-user>@user@domain.com</mention-user>');

testString = '@user@domain.com';
expect(parser.replace('<mention-user>@user@domain.com</mention-user>')).toBe(testString);

testString = '@USER@DOMAIN.COM';
expect(parser.replace('<mention-user>@USER@DOMAIN.COM</mention-user>')).toBe(testString);

testString = '@USER@domain.com';
expect(parser.replace('<mention-user>@USER@domain.com</mention-user>')).toBe(testString);

testString = '@user@DOMAIN.com';
expect(parser.replace('<mention-user>@user@DOMAIN.com</mention-user>')).toBe(testString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only test htmlToMarkdown with that method

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

@nikhildewoolkar
We do have test for user mention in "ExpensiMark-HTML-test.js"

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

You can add other similar tests with capital letters in username and domain in testString.

We do not have tests for html to text for <mention-user> as of now. You can add those tests in "/ExpensiMark-HTMLToText-test.js" file. Please go through existing tests in the file and add new tests accordingly.

@nikhildewoolkar
Copy link
Contributor Author

Kindly review PR. I have made requested changes.

Copy link
Contributor

@neil-marcellini neil-marcellini left a comment

Choose a reason for hiding this comment

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

Woohoo 🎉

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

Changes look good and tests well.

cc: @neil-marcellini

@neil-marcellini neil-marcellini merged commit f6a88d1 into Expensify:main Nov 3, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants