comment mentions: show displayname not uid by blizzz · Pull Request #1738 · nextcloud/server

blizzz · 2016-10-13T22:21:59Z

requires Notifications for simple @-mentioning in comments #1449

contributes to #753, items 2 and 4.

comment flow

Write a comment with a mentions

After saving, the rendered comment will show the displayname

Oops! You forgot to mention someone else! Editing the comment will show the uids

When the edit is done, the comment is rendered with display names again

Tech background

In the back, the DAV endpoint will send mention information with the raw message.

The client (web ui here) replaces the uid with the displayname for presentation.

The next step (follow-up PR) will introduce autocomplete. After typing @ a search within the recipients of this file is supposed to happen. The client would make sure that the raw user id stays in the message, but its editing/composing abilities would hide this from the end user. That's the idea at least.

Todos left

evaluate JS tests (adjust, add, or neither?)

mention-bot · 2016-10-13T22:22:00Z

@blizzz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @nickvergessen and @LukasReschke to be potential reviewers.

nickvergessen · 2016-10-14T10:48:28Z

This is on "rendering" the comment?

blizzz · 2016-10-16T12:59:14Z

This is on "rendering" the comment?

Yes, essentially 2. and 4. from #753. 2 alone without 4 is too cumbersome as it would introduce too much client business logic that then would be worthless when doing 4.

blizzz · 2016-10-16T13:12:17Z

lib/private/Comments/Comment.php

+			// exclude author, no self-mentioning
+			if($uid === '@' . $this->getActorId()) {
+				continue;
+			}


So far we plan only to support mentioning users. In the back of the head, mentioning groups in future might be reasonable. Thus we add a "type" key, however I am uncertain how we would implement this. Then there is a question whether a custom app would want to notify other, custom things. Is this legitimate? We could do this by extending the pattern and perhaps prepend a type between @ and the identifier (and defaulting to users). Just as food for thought.

blizzz · 2016-10-17T12:21:25Z

Unsure about JS tests yet, thus still developing, otherwise I am happy about feedback – what do you think? @nickvergessen @rullzer @MorrisJobke (and anyone else is welcome, too :) )

nickvergessen · 2016-10-17T12:23:41Z

apps/comments/js/commentmodel.js

 			'objectId': '{' + NS_OWNCLOUD + '}objectId',
-			'isUnread': '{' + NS_OWNCLOUD + '}isUnread'
+			'isUnread': '{' + NS_OWNCLOUD + '}isUnread',
+			'mentions': '{' + NS_OWNCLOUD + '}mentions'


should we start our namespace?

I think we pointed out once we stay with the current namespaces.

nickvergessen · 2016-10-17T12:24:53Z

apps/comments/js/commentstabview.js

+			for(var i in mentions) {
+				var mention = '@' + mentions[i].mentionId;
+				mention = mention.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+				var displayName = '<b>'+ _.escape(mentions[i].mentionDisplayName)+'</b>';


strong instead of b?

add avatar?

@nextcloud/designers wishes how it shoud look like?

That is what I should use in the activity app and I guess it's the same everywhere: https://github.com/nextcloud/activity/blob/master/js/formatter.js#L145-L148

implemented

nickvergessen · 2016-10-17T12:25:25Z

apps/comments/js/commentstabview.js

+
+			for(var i in mentions) {
+				var mention = '@' + mentions[i].mentionId;
+				mention = mention.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');


If you mention a user test and testname this breaks?

Almost there… but it is impossible to parse everything properly.

Example:
"Let's talk about it today, @foobar."

Since we allow dots, @foobar and @foobar. are possible scenarios here. If you have both? Would be silly to insert a space between the username and the fullstop.

But even better it is that we allow white spaces in usernames now:

"@alice Bernadette wants to talk"

Username can be '@Alice Bernadette``or'@Alice``(or both).

For now, I improve the Regexp to have the 98% solution, butt when going with autocomplete I am afraid we need to wrap the mention into brackets to be able to support all these funny usernames.

98% sounds good enough

nickvergessen · 2016-10-17T12:27:10Z

apps/dav/lib/Comments/CommentNode.php

+	const PROPERTY_NAME_MENTION = '{http://owncloud.org/ns}mention';
+	const PROPERTY_NAME_MENTION_TYPE = '{http://owncloud.org/ns}mentionType';
+	const PROPERTY_NAME_MENTION_ID = '{http://owncloud.org/ns}mentionId';
+	const PROPERTY_NAME_MENTION_DISPLAYNAME = '{http://owncloud.org/ns}mentionDisplayName';


nextcloud? or too confusing?

As in #1738 (comment) and yes, it would increase confusion.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

nickvergessen · 2016-10-19T07:41:43Z

While posting the avatars are not loaded. Switching the sidebar tab and causing a reload fixes it. I assume there is just a JS call missing.

nickvergessen · 2016-10-19T07:43:25Z

Clicking on the edit button breaks things heavily:

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

…ents Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2016-10-25T16:38:10Z

Review time 🎉

codecov-io · 2016-10-25T19:06:10Z

Current coverage is 57.91% (diff: 71.23%)

Diff Coverage	File Path
0%	apps/comments/appinfo/app.php
•••• 45%	apps/dav/lib/Comments/CommentNode.php
•••••••••• 100%	apps/comments/lib/Notification/Listener.php
•••••••••• 100%	lib/private/Comments/Manager.php
•••••••••• 100%	...ps/comments/tests/Unit/Notification/ListenerTest.php
•••••••••• 100%	lib/private/Comments/Comment.php

No coverage report found for master at 097a9ec.

Powered by Codecov. Last update 097a9ec...b12b52b

nickvergessen · 2016-10-26T08:54:31Z

Works now, I will fix activities to also show the user names and avatars in a second PR

👍

MorrisJobke · 2016-10-26T12:02:46Z

Tested and works 👍

blizzz added enhancement 2. developing Work in progress feature: comments labels Oct 13, 2016

blizzz added this to the Nextcloud 11.0 milestone Oct 13, 2016

blizzz commented Oct 16, 2016

View reviewed changes

blizzz force-pushed the comments-provide-displaynames-with-mentions branch 2 times, most recently from 95fa09c to 9d97c7f Compare October 17, 2016 12:08

blizzz changed the base branch from comments-user-mention to master October 17, 2016 12:08

blizzz changed the title ~~[WIP] comment mentions: show displayname not uid~~ comment mentions: show displayname not uid Oct 17, 2016

blizzz mentioned this pull request Oct 17, 2016

user mentions in comments #753

Closed

7 tasks

nickvergessen reviewed Oct 17, 2016

View reviewed changes

blizzz added 6 commits October 19, 2016 00:33

move mention extraction to (I)Comment and report mentions via DAV

fea3e20

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

resolve displayname via manager and registerable resolvers

5d98ab8

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

show displayname not uid in commit mentions

0e926ef

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

improve regex, fixes replacements of usernames with same trunk

b21ce6e

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

fix regex

70bd6ca

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

display avatar (if enabled) alongside displaynames

1854339

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz force-pushed the comments-provide-displaynames-with-mentions branch from d3b891d to 1854339 Compare October 18, 2016 22:34

blizzz added 5 commits October 19, 2016 12:47

fix avatar displaying after submit (either edit or new)

bc18a29

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

consolidate error handling

763a99e

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

add missing methods to test fake manager

e5ce61b

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

avoid that all avatars are copied to the author row when editing comm…

ab39547

…ents Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

fix JS test and introduce tests for rendering mentions to displaynames

b12b52b

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz force-pushed the comments-provide-displaynames-with-mentions branch from 7d07922 to b12b52b Compare October 25, 2016 16:34

blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 25, 2016

MorrisJobke merged commit cde7f53 into master Oct 26, 2016

MorrisJobke deleted the comments-provide-displaynames-with-mentions branch October 26, 2016 12:02

Uh oh!

Conversation

blizzz commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

comment flow

Tech background

Todos left

Uh oh!

mention-bot commented Oct 13, 2016

Uh oh!

nickvergessen commented Oct 14, 2016

Uh oh!

blizzz commented Oct 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Oct 19, 2016

Uh oh!

nickvergessen commented Oct 19, 2016

Uh oh!

blizzz commented Oct 25, 2016

Uh oh!

codecov-io commented Oct 25, 2016

Current coverage is 57.91% (diff: 71.23%)

Uh oh!

nickvergessen commented Oct 26, 2016

Uh oh!

MorrisJobke commented Oct 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blizzz commented Oct 13, 2016 •

edited

Loading