Fix: move autoEmail to before mentions #550
Conversation
cead22
changed the title
Can you explain precisely what this means for each of the cases? For instance, is this one below correct? 
What about this one? Should 
|
__tests__/ExpensiMark-HTML-test.js
Outdated
| @@ -896,7 +896,7 @@ test('Test for user mention without space or supported styling character', () => | |||
|
|
|||
| test('Test for user mention with user email includes underscores', () => { | |||
| const testString = '@[email protected]'; | |||
| const resultString = '<mention-user>@_concierge_@expensify.com</mention-user>'; | |||
| const resultString = '@_<a href=\"mailto:[email protected]\">concierge_@expensify.com</a>'; | |||
There was a problem hiding this comment.
Is it possible the first _ should be part of the email?
There was a problem hiding this comment.
@cead22 I agree that this should be changed. We can resolve it simply by addding a _ character in the beginning part of the regular expression for MARKDOWN_EMAIL here
- MARKDOWN_EMAIL: "([a-zA-Z0-9.!#$%&'+/=?^`{|}-][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*@[a-zA-Z0-9-]+?(\\.[a-zA-Z]+)+)",
+ MARKDOWN_EMAIL: "([a-zA-Z0-9.!#$%&'+/=?^_`{|}-][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*@[a-zA-Z0-9-]+?(\\.[a-zA-Z]+)+)",However, I think this is not related to the current issue that is being resolved (which is dealing with the inconsistency rather than the correctness of the email validation). Furthermore, the current regular expression for email is also capturing many invalid emails, for example [email protected] (emails cannot start with dot character) and [email protected] (emails cannot have consecutive dot characters). IMO, we should resolve this in a seperated issue, something like Update email regular expression to remove matches of invalid emails.
Thoughts?
Sorry for the confusion. The difference you are seeing is the result before I apply the changes in this PR. The expected behavior is that those texts should be rendered as email like what was shown when the texts are typed and sent. I have updated the new video recordings in the PR description with a brand new account to avoid this confusion. |
|
@eVoloshchak Thanks for pointing that out. After some more digging, I'm having these 2 solutions for this, please let me know what you think.
Change the - (?![^<]*>|[^<>]*<\\/)${CONST.REG_EXP.MARKDOWN_EMAIL}(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))
+ (?![^<]*>|[^<>]*<\\/)([a-zA-Z0-9.!#$%&'*+/=?^_\`{|}~-]*?@?)(${CONST.REG_EXP.MARKDOWN_EMAIL})(?![^<]*(<\\/pre>|<\\/code>|<\\/a>))Change the - replacement: '<a href="mailto:$1">$1</a>',
+ replacement: (match, g1, g2) => {
+ if (match.startsWith('@')) {
+ return match;
+ }
+ return `${g1}<a href="mailto:${g2}">${g2}</a>`;
+ },
IMO, solution 2 should be better, since it provide the least change which will fix the mention issue, which wouldn't affect other cases that we may not yet thought about. Here is the recording of the cases that both solution will solve. |
Shouldn't it be the other way around? I.e. "replace the match with mention only if it starts with |
Yes, that's what the current logic is doing, and we are not changing that. My suggestion is that we change the mention regex so that it also cover other characters. Currently, with the email The main point is that the current regex (intentionally matching text before |
|
@eVoloshchak friendly bump. What do you think should be our next step here? Thanks. |
|
Sorry for the delay, for some reason there's no review request showing up for me for this PR. |
I don't think so. You can notice that regex does not contain some invalid email username characters i.e |
…character prefix so it can be filtered later
…the email rule, remove special markdown character in mention regex
|
Ok, so here's what I have done
For the email starting with |
|
Looks good except one test case 
|
|
@eVoloshchak I think the current email regex is not totally correct. Should we fix it in other PR? |
|
Yeah, I'll just report tt=hat on Slack, it's not caused by this PR |
There was a problem hiding this comment.
This looks good. @tienifr can you add more manual tests to the PR description to test (and QA)
- Some cases of mentions to make sure those are still working
- Tapping on a mention and making sure it works
- Tapping on one of those emails you're sending, and make sure the behavior is correct
If there are more edge cases that aren't covered in the automated tests that you can think of, those would be good to add as well
|
Also, I noticed the iOS videos are showing an app that looks broken, without a header. I know that's not related to these changes but if you can update those so the app doesn't look broken (since we're trying to show in these videos that the app works well), and if you know how to reproduce that, can you comment here? |
|
@cead22 Tests and IOS videos are updated, please have a look! Also, I would like to bring attention to this edge case. It's appearing in both staging and this PR, and although I think we should resolve this in another PR, it's worth mentioning and the expected behavior should be agreed upon:
I think it probably would be confusing for the users when they try to mention another user that has email named cc @cead22 @eVoloshchak for thoughts. |
Weird enough, at the moment it's not reproducible on my side 😬 |
My understanding is, both @[email protected] and @[email protected] are not valid emails, so @here part being displayed as a mention is correct I think, but curious what others think UPD: Notice how GitHub also doesn't render them as emails |
Why not resolve here? Is it happening in production? That seems very related to this PR
I don't know if I missed something, but I think both these statements are wrong. Those are valid emails and GH markdown renders them as such |
|
Because it's more about the correctness of the regex, instead of the inconsistency issue. If we all agree that it's a bug and should be fixed in this PR then we can fix it by modifying the user mention regex so that it exclude the @here mention when it follows with a domain. - regex: /[a-zA-Z0-9.!$%&+/=?^`{|}-]?@here(?=_\b|\b)(?!((?:(?!<a).)+)?<\/a>|[^<]*(<\/pre>|<\/code>))/gm,
+ regex: /[a-zA-Z0-9.!$%&+/=?^`{|}-]?@here(?=_\b|\b)(?!(@[a-zA-Z0-9-]+?(.[a-zA-Z]+)+)|((?:(?!<a).)+)?<\/a>|[^<]*(<\/pre>|<\/code>))/gm,What do you think about this change? |
Please correct me if i'm wrong, the expected behavior for tapping on an email is that the app would open the mailbox? Since we are using
I will update it once we agree on the expected behavior of the email/mention |
Ah, my bad, I missed that they're supposed to be rendered as mentions instead of as emails
This looks like the correct behavior to me |
|
@tienifr whatever the behavior is today, it should be the same with this change. The reason I think the test is relevant is because we're modifying some code that's involved in rendering emails, so imo we should test that this isn't breaking what we have |
|
Okay, so I added a few other edge cases that worth testing. Those test cases require some additional modification. Here are the latest changes:
Can you please check that out? |
|
Also @cead22 , can you please clarify what you mean by Screen.Recording.2023-07-01.at.11.49.13.mov |
|
@cead22 Friendly bump. Can you take a look? |
|
The updates in this comment make sense to me 👍
This should be the same behavior we have today. We're just testing that we're not breaking the existing behavior since we're changing some code related to how we render emails
That's fine, we don't need this in the recordings, but we should test and QA the behavior didn't break |
Looks like this was missed again. I still think it'd be good to show this in a video, but at least it's in the QA steps |
lib/str.js
Outdated
| // A valid mention starts with a space, *, _, #, ', ", or @ (with no preceding characters). | ||
| return /[\s*~_#'"@]/g.test(mention.charAt(0)); | ||
| // A valid mention starts with a space or @, or | ||
| // starts and ends with a *, _, #, ', or " (with no preceding characters). |
There was a problem hiding this comment.
Is # supposed to be here? I thought # was used for headings like so # heading 1, ie, with a # before the text but without a # after the text
There was a problem hiding this comment.
A valid mention starts with a space or @ or starts and ends with a *, _, #, ', or " (with no preceding characters)
Is this correct?
I think I'm missing something, can mention start and end with *, _, #, ', or "?
There was a problem hiding this comment.
@eVoloshchak it can "start" with * in the sense that the mention can be within two *, so it'll still technically start with an @, but this is saying it can start with * becasue it can be like let's ask *@[email protected]*.
This is why I asked about #, cause that one is only a leading character unlike *, _, ', ", and ~.
Incidentally @tienifr, the ~ is missing from the comment starts and ends with a *, _, #, ', or "...
There was a problem hiding this comment.
Incidentally @tienifr, the ~ is missing from the comment starts and ends with a *, _, #, ', or "...
I missed that. Thanks for pointing it out.
This is why I asked about #, cause that one is only a leading character unlike *, _, ', ", and ~.
I agree that # is redundant here since it is only a leading character, and we can get rid of it. It also does not affect the current behavior of either this PR or the current staging version
|
@eVoloshchak since there were code changes after you approved, can you please re-review? Thanks! |
@cead22 This has been updated in the Web video. Please have a look! |
@eVoloshchak that seems like a good idea, can you start a convo in #expensify-open-source? |
|
@cead22 I've resolved that. |
Fixed Issues
$ Expensify/App#20085
Tests
QA
Screenshots/Videos
Web
550-web-03.mp4
Mobile Web - Chrome
550-mweb-chrome.mov
Mobile Web - Safari
550-mweb-safari-02.mov
Desktop
Screen.Recording.2023-06-16.at.12.27.44.mov
iOS
550-ios-02.1.mov
Android
550-android.1.mov