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.

Sign up for GitHub

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

Jump to bottom

Mention sub name users #5041

Merged
merged 3 commits into from
Jan 29, 2021
Merged

Mention sub name users #5041

merged 3 commits into from
Jan 29, 2021

Conversation

nickvergessen
Copy link
Member

Steps

  1. Post message Mention to @sub and @subname
  2. Post message Mention to @subname and @sub
Before After
Bildschirmfoto von 2021-01-28 16-03-05 Bildschirmfoto von 2021-01-28 16-02-52

@nickvergessen nickvergessen added this to the 💚 Next Alpha (21) milestone Jan 28, 2021
@nickvergessen nickvergessen force-pushed the bugfix/noid/mention-sub-name-users branch from f26714c to 98a4270 Compare January 28, 2021 15:22
@nickvergessen
Copy link
Member Author

/backport to stable20.1

@nickvergessen
Copy link
Member Author

/backport to stable20

blizzz
blizzz approved these changes Jan 28, 2021
View reviewed changes
lib/Chat/Parser/UserMention.php Outdated Show resolved Hide resolved
danxuliu
danxuliu requested changes Jan 28, 2021
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Using \b (word boundary)) does not cover all the valid characters in a user name. I have added unit tests that show the issue.

@nickvergessen
Copy link
Member Author

Then I guess we should indeed sort by length first, because Hi @sub. is more likely than 2 users sub and sub.name and will get quite complexe to match otherwise.

@nickvergessen nickvergessen mentioned this pull request Jan 29, 2021
Merged
danxuliu
danxuliu approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Tests pass and code looks good.

Allowing quotes also for user names without spaces should be fine, so 👍

nickvergessen and others added 3 commits January 29, 2021 11:15
@nickvergessen
Fix mentioning users with subnames
6f05d93 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Add an integration test for sub and subname mentions
b699ff6 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@danxuliu @nickvergessen
Add unit tests for subname mentions
e65fa8d 
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/mention-sub-name-users branch from a08d0f9 to e65fa8d Compare January 29, 2021 10:15
@nickvergessen
Copy link
Member Author

Fixing up

@nickvergessen nickvergessen merged commit 6c229f7 into master Jan 29, 2021
@nickvergessen nickvergessen deleted the bugfix/noid/mention-sub-name-users branch January 29, 2021 12:13
This was referenced Jan 29, 2021
Merged
Merged
@nickvergessen nickvergessen mentioned this pull request Feb 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blizzz blizzz blizzz approved these changes

@danxuliu danxuliu danxuliu approved these changes

Assignees
No one assigned
Labels
4. to release bug feature: chat 💬 Chat and system messages
Projects
None yet
Milestone
v11.0.0-rc.1
Development

Successfully merging this pull request may close these issues.
3 participants
@nickvergessen @blizzz @danxuliu