make usermention case insensitive #584
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
melvin-bot
bot
requested review from
cristipaval
and removed request for
a team
@nikhildewoolkar Could you please sign the CLA? |
|
@nikhildewoolkar Bump on this |
|
The current code change looks good though 👍 |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
Thanks @nikhildewoolkar! Could you now take care of this comment from @neil-marcellini ? |
|
updated pr with testcases |
|
@neil-marcellini Could you please review this one again? |
|
Yes I will review it today |
nikhildewoolkar
dismissed stale reviews from neil-marcellini and cristipaval
via
2174a6f
|
updated the pr with suggested changes.kindly review |
lib/ExpensiMark.js
Outdated
| replacement: (match) => { | ||
| if (!Str.isValidMention(match)) { | ||
| return match; | ||
| } | ||
| return `<mention-user>${match}</mention-user>`; | ||
| return `<mention-user>${match.toLowerCase()}</mention-user>`; |
There was a problem hiding this comment.
@nikhildewoolkar Won't this convert the input to lowercase? As discussed in the issue, we don't want to convert the input to lower case. We want to leave it as it is.
Context: Expensify/App#26016 (comment)
Expensify/App#26016 (comment)
There was a problem hiding this comment.
Ok yes good catch, thank you. I agree that we don't want to make it lowercase here.
That also makes me think that we should have tests for converting a mention in html back to markdown, with uppercase, lowercase, mixed, etc.
lib/ExpensiMark.js
Outdated
| replacement: (match) => { | ||
| if (!Str.isValidMention(match)) { | ||
| return match; | ||
| } | ||
| return `<mention-user>${match}</mention-user>`; | ||
| return `<mention-user>${match.toLowerCase()}</mention-user>`; |
There was a problem hiding this comment.
Ok yes good catch, thank you. I agree that we don't want to make it lowercase here.
That also makes me think that we should have tests for converting a mention in html back to markdown, with uppercase, lowercase, mixed, etc.
|
updated PR. please review. |
There was a problem hiding this comment.
LGTM!
@sobitneupane @neil-marcellini btw, what is the process to update the version in App?
|
PR Updated |
__tests__/ExpensiMark-HTML-test.js
Outdated
|
|
||
| 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); |
There was a problem hiding this comment.
Looks good, but I think we should add these tests to __tests__/ExpensiMark-HTMLToText-test.js and __tests__/ExpensiMark-Markdown-test.js.
There was a problem hiding this comment.
Sorry that this is dragging on for a while. Please DM me when you make the updates and we can have faster iterations and get this merged today.
|
By mistake, I closed this PR. So made new PR with the changes suggested in earlier chats #591. Kindly Review. |
Fixed Issues
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