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

feat: add rules to recognize user and @here mentions #524

Closed
wants to merge 51 commits into from

Conversation

getusha
Copy link
Contributor

@getusha getusha commented Apr 25, 2023

@mananjadhav @puneetlath

Fixed Issues

$ Expensify/App#17882

Tests

  1. Open any chat
  2. Send valid mention e.g. @jhon, @david
  3. Send @here mention
  4. The user mention should be replaced as <mention-user>mention</mention-user> and the @here should be <mention-here>@here</mention-here>

QA

Same as tests

@getusha getusha requested a review from a team as a code owner April 25, 2023 15:38
@github-actions
Copy link

github-actions bot commented Apr 25, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from hayata-suenaga and removed request for a team April 25, 2023 15:39
@puneetlath
Copy link
Contributor

Thanks for the quick work here @getusha. Let's also add a bunch of tests so that we are confident it is working as expected in a bunch of different scenarios.

@puneetlath
Copy link
Contributor

Another couple of things @getusha.

  1. You need to sign the CLA in this repo.
  2. Let's make sure that the mention is the inside most html tag. For example if someone types *@here* we want the html to be <strong><mention-here>@here</mention-here></strong> not <mention-here><strong>@here</strong></mention-here>. That might already be the case, but I just wanted to be explicit.

@getusha
Copy link
Contributor Author

getusha commented Apr 25, 2023

I have read the CLA Document and I hereby sign the CLA

@getusha
Copy link
Contributor Author

getusha commented Apr 25, 2023

recheck

@getusha
Copy link
Contributor Author

getusha commented Apr 25, 2023

Let's make sure that the mention is the inside most html tag. For example if someone types @here we want the html to be @here not @here. That might already be the case, but I just wanted to be explicit.

@puneetlath yes I am considering the stylings too,
i will also include cover for it within the tests

@puneetlath puneetlath self-requested a review April 25, 2023 18:20
@puneetlath
Copy link
Contributor

@mananjadhav looks like you need to comment so that we can assign you.

@mananjadhav
Copy link
Contributor

@getusha Can we please add tests for the rules? Considering all the scenarios like styling, codefence, email, mentions not rendered as hyperlink, etc.

looks like you need to comment so that we can assign you.

@puneetlath I think this comment should help?

@puneetlath
Copy link
Contributor

Weird, it doesn't let me assign you.

image

Maybe you can assign yourself?

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
@mananjadhav
Copy link
Contributor

It didn't let me assign, but the moment I added review comments it added me as a reviewer.

lib/ExpensiMark.js Outdated Show resolved Hide resolved
@getusha
Copy link
Contributor Author

getusha commented Apr 25, 2023

@mananjadhav @puneetlath
I have just pushed the changes and included the test coverage.

@getusha
Copy link
Contributor Author

getusha commented May 9, 2023

@puneetlath
I am not sure if it worked, but did run the command and fixed conflicts by merging to main. can you please check?

@puneetlath
Copy link
Contributor

It looks like it worked for commit signing, but it also looks like you lost some of your changes. For example, it seems like all your Expensimark-test changes are gone.

@puneetlath
Copy link
Contributor

An easier method might be to take your changes as of afb1d85 and move it to a new branch and open a new PR.

@getusha
Copy link
Contributor Author

getusha commented May 9, 2023

An easier method might be to take your changes as of afb1d85 and move it to a new branch and open a new PR.

Yes, that would be easier. lets check it one more time

Copy link
Contributor

@puneetlath puneetlath left a comment

Choose a reason for hiding this comment

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

Ok it does look like we have the same changes as before now, so approving.

@puneetlath
Copy link
Contributor

Ok, I approved @getusha but we're still getting the signed commits error.
image

@getusha
Copy link
Contributor Author

getusha commented May 9, 2023

Alright, i will raise a new PR

@getusha
Copy link
Contributor Author

getusha commented May 9, 2023

Opened a new PR here, going to close this one.

*/
{
name: 'userMentions',
regex: new RegExp(`((&#x60;|<code>|<pre>)\\s*?)?[\`.a-zA-Z]?@+${CONST.REG_EXP.EMAIL_PART}`, 'gm'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@getusha Why do we need a @+ can't it just be @ as we need just only one @. As this seems like causing a regression for more info check this Expensify/App#28946 (comment) Let us know if are we missing something.

cc: @mananjadhav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Santhosh-Sellavel I am not sure if i remember that correctly, but if all tests are passing i think its safe to remove it.

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.