Notifications for simple @-mentioning in comments #1449

blizzz · 2016-09-19T16:47:56Z

Contributes to #753

The PR implements notification for @-mentioning against User IDs (according to step 1 in #753).

In a shared file, write a comment and type the user id of one of the sharees:

A notification will appear…

…and look like this

Upon click the user is brought to the Files app, in the dir of the file or folder commented on. the file or folder is also highlighted.

When a comment containing a mention is deleted, the notification is also removed, if applicable.

It does not include or implement a subscription system.

Please test and review @nickvergessen (most code you have seen at owncloud/core#24495 already) @schiessle @jancborchardt

mention-bot · 2016-09-19T16:47:59Z

@blizzz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nickvergessen to be a potential reviewer

nickvergessen · 2016-09-19T16:51:21Z

apps/comments/lib/AppInfo/Application.php

+
+/**
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *


missing @license

done (this is for me, Github keeps showing me the dealt-with comments despite code changes, making it hard to track whats left…)

nickvergessen · 2016-09-19T16:53:05Z

apps/comments/appinfo/app.php

+$notificationManager = \OC::$server->getNotificationManager();
+$notificationManager->registerNotifier(
+	function() {
+		return new \OCA\Comments\Notification\Notifier(


Use $application->getContainer()->query('OCA\Comments\Notification\Notifier') for automatic DI?
Or even better $application->getContainer()->query(\OCA\Comments\Notification\Notifier::class)

nickvergessen · 2016-09-19T16:53:27Z

apps/comments/appinfo/app.php

+$notificationListener = function(\OCP\Comments\CommentsEvent $event) {
+	$application = new \OCP\AppFramework\App('comments');
+	/** @var \OCA\Comments\Notification\Listener $listener */
+	$listener = $application->getContainer()->query('OCA\Comments\Notification\Listener');


Use \OCA\Comments\Notification\Listener::class instead of magic string?

+1 for better maintainability.

nickvergessen · 2016-09-19T16:54:26Z

apps/comments/appinfo/routes.php

+
+$application = new Application();
+$application->registerRoutes($this, ['routes' => [
+	['name' => 'Notifications#view', 'url' => '/notifications/view/{id}', 'verb' => 'GET'],


please include the type in the url and dispatch an event on the endpoint.
Comments are already used by other apps, which would like to redirect to other parts then

The ID is a unique comment ID, not coupled to Actor or Object type. And the URL is still within apps/comments/. Still an issue?

Hmm should be fine then I guess

nickvergessen · 2016-09-19T16:55:12Z

apps/comments/lib/AppInfo/Application.php

+		parent::__construct('comments', $urlParams);
+		$container = $this->getContainer();
+
+		$container->registerService('NotificationsController', function(IContainer $c) {


Use magic instead:
$container->registerAlias('NotificationsController', \OCA\Comments\Controller\Notifications::class);

nickvergessen · 2016-09-19T16:56:13Z

apps/comments/lib/Controller/Notifications.php

+		try {
+			$comment = $this->commentsManager->get($id);
+			if($comment->getObjectType() !== 'files') {
+				return new NotFoundResponse();


as mentioned above, dispatch an event, so announcements can redirect to the given announcement

Now this should not be a problem anymore, because the code path is a result from a registered EventHandler. So, if a different app wants to work with Notifications, it will react upon any event, create the notification pointing to a controller of its own. Thus, to be considered done.

nickvergessen · 2016-09-19T16:57:23Z

apps/comments/lib/Controller/Notifications.php

+		$notification = $this->notificationManager->createNotification();
+		$notification->setApp('comments')
+			->setObject('comment', $comment->getId())
+			->setSubject('mention', [$comment->getObjectType(), $comment->getObjectId()] )


I would remove this, object+user should be fine already, but I guess we will never change that data

It is possible to change the data in theory. Currently it does not happen in files and it would not be reasonable either (unless you introduce moving comments from one file to the other…). The notification data itself does not change. If setting the subject with 'mention' alone works, it would be the safer approach indeed.

nickvergessen · 2016-09-19T16:57:45Z

apps/comments/lib/Notification/Listener.php

+		$comment = $event->getComment();
+
+		if($comment->getObjectType() !== 'files') {
+			// we only support notifications for files at this point


Now, this is also dependent on the registered EventHandler, custom types/apps would need to register their own. → Done.

nickvergessen · 2016-09-19T16:58:43Z

apps/comments/lib/Notification/Listener.php

+		$notification
+			->setApp('comments')
+			->setObject('comment', $comment->getId())
+			->setSubject('mention', [ $comment->getObjectType(), $comment->getObjectId() ])


Should listen to delete files hook, and then delete the notifications? But could also be a step 2

Nope, that already works 😎

When files are deleted, the related comments are deleted, too. And this triggers deletion of notifications. Tested :)

Btw, even works for Unshared files.

nickvergessen · 2016-09-19T16:59:36Z

apps/comments/lib/Notification/Notifier.php

+				$displayName = $commenter->getDisplayName();
+			}
+		} else if($comment->getActorType() === ICommentsManager::DELETED_USER) {
+			$displayName = $l->t('a now deleted user');


This will only work in english. I'd to a switch with the full message.

You're right, missing context for translators :(

blizzz · 2016-09-20T10:24:26Z

As discussed with @nickvergessen:

To be user to be integrated with other apps, we extend the API to register a CommentEntity. This will be notified on events. Therefore, we can remove the usage of the EventDispatcher and specific/coupled implementations. (So far we have one client using Comments infrastructure except of Files which is the Announcement Center – this is were improvement suggestions come from).

Create ~~CommentEntity~~ CommentsEventHandler Interface. Event notifications will be piped thereto.
Extend CommentsManager API with a ~~CommentEntitiy~~ event handler registration method.
Implement this in core implementation
~~Create FileCommentsEntity within comments app, implementing the CommentEntity interface~~ Replace CommentEntityEvent with a registration-bases solution within CommentsManager. To be done in a seperate PR, out of scope. → Comments: Replace CommentEntityEvent with a registration-bases solution within CommentsManager. #1539
Add a Release Note and inform developers on discourse → Comments: Replace CommentEntityEvent with a registration-bases solution within CommentsManager. #1539

Updates inline

blizzz · 2016-09-27T12:50:48Z

@nickvergessen reevaluate your review, please. 🎆

nickvergessen · 2016-09-28T09:05:32Z

apps/comments/lib/Controller/Notifications.php

+
+			$dir = $this->folder->getRelativePath($files[0]->getParent()->getPath());
+			$url = $this->urlGenerator->linkToRouteAbsolute(
+				'files.view.index',


I'd use

$link = $this->urlGenerator->linkToRouteAbsolute( 'files.viewcontroller.showFile', ['fileId' => $comment->getObjectId()] );

this automatically highlights the row in the file list and will also in the future open the sidebar.
Maybe we can also add a #comments-tab or something, so it can automatically open the comments tab in this case.

Have it locally, push later today.

in the future open the sidebar.
Maybe we can also add a #comments-tab or something, so it can automatically open the comments tab in this case.

Are there already issues or PRs on this?

No issues/PRs as far as I know

nickvergessen · 2016-09-28T09:20:49Z

Other then that works well, will also try to get this working for the announcements later today.

Todos for follow up PRs

Allow autocomplete of names when user enumeration is allowed
Open sidebar comments tab (maybe even highlight the comment that has the mention)
Highlight the actual mention (maybe with avatar?) in the comment text

nickvergessen · 2016-09-28T09:27:31Z

apps/comments/lib/Notification/Listener.php

+			if($event->getEvent() === CommentsEvent::EVENT_DELETE) {
+				$this->notificationManager->markProcessed($notification);
+			} else {
+				$this->notificationManager->notify($notification);


It's a bit odd, but if I see this correctly, when you modify a comment so someone is not mentioned anymore, the notification is not removed, right?

That's right. (As comparison, Github does not behave differently, which does not mean it is correct however).

Ok, so the issue is that we have garbage in the DB.

nickvergessen · 2016-09-28T09:37:07Z

apps/comments/lib/Notification/Listener.php

+
+			$notification->setUser($user);
+			if($event->getEvent() === CommentsEvent::EVENT_DELETE) {
+				$this->notificationManager->markProcessed($notification);


Also this is not reached, when deleting a comment, because your EventHandler does only handle add?

Interesting point. I believe to remember that it was needed. However, deleting a comment indeed does not end up here. And that's probably a bug. While the notification is not shown to the user, it still is present in the database.

Added the deletion handler. We could also hook in an activity thing and log about deleted activities if it makes sense. i tried this a bit, but could not get my head around formatting 🙊

MorrisJobke · 2016-10-05T22:30:26Z

@blizzz could you check out the comments? Thanks :)

jancborchardt · 2016-10-06T15:10:31Z

Regarding the UI of the notification, it needs to look a bit nicer like this:

😎 Joas mentioned you in a comment on 🖼 Berlin Weißensee at night.jpg

Smiley is a placeholder for the avatar, and image for the filetype. Remember that usernames need to always have avatars next to them. And ideally filenames should always have a connection to their filetype icons or previews!
If the bolding or the avatar is not possible with the notification app or API yet, we should take steps to make it so.

As the first step, the wording should be fixed to the above. :) (That way there is important info directly at the beginning of the message: Someone mentioned you…)

blizzz · 2016-10-06T19:54:13Z

@jancborchardt thanks for having a look!

Regarding the UI of the notification, it needs to look a bit nicer like this:

😎 Joas mentioned you in a comment on 🖼️ Berlin Weißensee at night.jpg

Smiley is a placeholder for the avatar, and image for the filetype. Remember that usernames need to always have avatars next to them. And ideally filenames should always have a connection to their filetype icons or previews!
If the bolding or the avatar is not possible with the notification app or API yet, we should take steps to make it so.

IIRC notifications accept plain text only at the moment. Thus, it's out of scope here. It makes sense however, I agree!

@jancborchardt might be an addition to #753

As the first step, the wording should be fixed to the above. :) (That way there is important info directly at the beginning of the message: Someone mentioned you…)

Good point.

Imprve notification wording to the above

@license

(WIP) notify user when mentioned in comments Fix doc, and create absolute URL for as notification link. PSR-4 compatibility changes also move notification creation to comments app Do not notify yourself unit test for controller and application smaller fixes - translatable app name - remove doubles in mention array - micro perf optimization - display name: special label for deleted users, keep user id for users that could not be fetched from userManager Comment Notification-Listener Unit Test fix email adresses remove notification when triggering comment was deleted add and adjust tests add missing @license tags simplify NotificationsController registration appinfo simplification, php docs make string easier to translate adjust test replace dispatcher-based listeners with a registration method and interface safer to not pass optional data parameter to setSubject for marking as processed. ID and mention suffices Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de> update comment Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

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

blizzz · 2016-10-07T15:30:55Z

Allow autocomplete of names when user enumeration is allowed

See 3. from #753

Open sidebar comments tab (maybe even highlight the comment that has the mention)
Highlight the actual mention (maybe with avatar?) in the comment text

I'll mention it there as well.

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

blizzz · 2016-10-07T23:24:02Z

@jancborchardt here you go

(the user id in brackets is custom ldap setting, by default you see the normal display name, so don't be afraid)

* notifications can be cleaned up, no polluted DB * updating comments will re-notify users or remove notifications, depending on the message Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz · 2016-10-12T16:09:58Z

Added a pre-update event so, so that there will be no orphaned notifications in the DB. Also users are getting either re-notified, notified, and un-notifies, depending on whether they are still mentioned, newly mentioned or removed.

@nickvergessen how's your satisfaction grade now? :)

codecov-io · 2016-10-12T16:39:04Z

Current coverage is 57.43% (diff: 95.85%)

Merging #1449 into master will increase coverage by 1.19%

@@             master      #1449   diff @@
==========================================
  Files          1070       1081     +11   
  Lines         60505      62974   +2469   
  Methods        6829       7186    +357   
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits          34026      36168   +2142   
- Misses        26479      26806    +327   
  Partials          0          0

Diff Coverage	File Path
0%	new apps/comments/appinfo/routes.php
•• 23%	apps/comments/appinfo/app.php
•••••••• 80%	new apps/comments/lib/Notification/Notifier.php
•••••••• 89%	new apps/comments/tests/Unit/EventHandlerTest.php
••••••••• 93%	new apps/comments/lib/Controller/Notifications.php
••••••••• 95%	lib/private/Comments/Manager.php
••••••••• 97%	new ...ps/comments/tests/Unit/Notification/ListenerTest.php
•••••••••• 100%	lib/public/Comments/CommentsEvent.php
•••••••••• 100%	...ate/AppFramework/DependencyInjection/DIContainer.php
•••••••••• 100%	new apps/comments/lib/AppInfo/Application.php

Review all 16 files changed

Powered by Codecov. Last update 9e7824f...70c7781

blizzz · 2016-10-13T20:21:52Z

apps/comments/lib/Notification/Listener.php

+	 * @param string $message
+	 * @return string[] containing the mentions, e.g. ['@alice', '@bob']
+	 */
+	public function extractMentions($message) {


Ok, I am currently looking into 2. and 4. from #753 and extractMentions() can be well used in other code parts. Listeners is the wrong place to be, thus.

It could be a direct method of a IComment and I think it makes most sense. However this extends the API with a method. Except we add an additional interface, IMentionable within the Comments namespace. It would be an optional ability and we remain backwards compatibility. Downside: for now it would be just one method (more might come with further implementing #753, but it is unlikely). A future change would cause the same dilemma. Now the time is not bad since I have not heard about a single 3rd party Comments implementation.

Other places would be the Comments Manager (similar issue, and makes less sense to me) or we introduce a public Utility class (might get too close to our implementation, limiting flexibility). Or it would be an interface as well and we gain too much additional overhead…

=> I work on it on the subsequent PR, does not need to be dealt with here. For now I'll extend IComment, but I am not fixed on this, can be discussed and easily moved around.

nickvergessen · 2016-10-14T08:32:02Z

apps/comments/lib/EventHandler.php

+
+		$eventType = $event->getEvent();
+		if( $eventType === CommentsEvent::EVENT_ADD
+			&& $event instanceof CommentsEvent


this is not necessary on all the ifs because you typehint it in the methods signature ;)

Legay from when i had the EntityEvent in mind… thanks for noticing 🙇‍♂️

nickvergessen · 2016-10-14T08:33:39Z

apps/comments/lib/EventHandler.php

+	 * @param CommentsEvent $event
+	 */
+	private function activityHandler(CommentsEvent $event) {
+		$c = $this->app->getContainer();


this is hacky, why not inject the actual objects?

yes, why? 🤔 🔕 🖊️

nickvergessen · 2016-10-14T08:35:45Z

apps/comments/lib/Notification/Listener.php

+	public function evaluate(CommentsEvent $event) {
+		$comment = $event->getComment();
+
+		if($comment->getObjectType() !== 'files') {


This has already been taken care of

nickvergessen · 2016-10-14T10:18:04Z

apps/comments/lib/Notification/Notifier.php

+						[ $displayName, $fileName ]
+					);
+				}
+				$notification->setParsedSubject($subject);


Could do:

$notification->setParsedMessage($comment->getMessage());

Then the notification also contains the text of the comment. I think that makes actually sense?

It might reveal sensitive information, when popping up on a big screen? It might be also too much text for a notification, so should be trimmed down.

Facebook shows the first few words at the end on their notification, Diaspora a bit similar, but then this is how they tell apart different posts, there is no name or title. Imho the filename is sufficient and we should not put more information here. At least in the first iteration. If it proves to suck we still can adjust, my feeling is less is more however.

trimming is something the notification API listeners take care of ;)

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

blizzz · 2016-10-14T14:45:13Z

@nickvergessen should be fine now.

nickvergessen · 2016-10-14T15:01:37Z

👍

blizzz · 2016-10-14T23:21:49Z

Another reviewer? @jancborchardt @rullzer @LukasReschke perhaps?

MorrisJobke · 2016-10-17T07:30:27Z

Tested and works 👍

blizzz added enhancement 3. to review Waiting for reviews feature: comments labels Sep 19, 2016

blizzz added this to the Nextcloud 11.0 milestone Sep 19, 2016

blizzz mentioned this pull request Sep 19, 2016

user mentions in comments #753

Closed

7 tasks

blizzz changed the title ~~Notificacations for simple @-mentioning in comments~~ Notifications for simple @-mentioning in comments Sep 19, 2016

nickvergessen requested changes Sep 19, 2016

View reviewed changes

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

blizzz force-pushed the comments-user-mention branch 2 times, most recently from e0b5bcc to a8f41c8 Compare September 26, 2016 22:54

blizzz mentioned this pull request Sep 27, 2016

Comments: Replace CommentEntityEvent with a registration-bases solution within CommentsManager. #1539

Closed

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

blizzz force-pushed the comments-user-mention branch from 41076b8 to 1317bbe Compare September 27, 2016 12:31

nickvergessen reviewed Sep 28, 2016

View reviewed changes

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

jancborchardt mentioned this pull request Oct 7, 2016

Richer notification messages for comment mentions (and other notifications) nextcloud/notifications#27

Closed

blizzz and others added 2 commits October 7, 2016 17:11

update autoloading map

522b053

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

use different files route/controller

006da9a

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

blizzz added 2 commits October 8, 2016 01:10

remove notifications of deleted comments from the DB

a35d4e7

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

adjust wording of the notification

a9671a4

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

blizzz force-pushed the comments-user-mention branch from 4fc4f02 to a9671a4 Compare October 7, 2016 23:19

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

emit pre-update event for comments

1bcd2ca

* notifications can be cleaned up, no polluted DB * updating comments will re-notify users or remove notifications, depending on the message Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

blizzz commented Oct 13, 2016

View reviewed changes

blizzz mentioned this pull request Oct 13, 2016

comment mentions: show displayname not uid #1738

Merged

2 tasks

nickvergessen reviewed Oct 14, 2016

View reviewed changes

apply some polish

70c7781

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

nickvergessen approved these changes Oct 14, 2016

View reviewed changes

MorrisJobke merged commit 96f8f20 into master Oct 17, 2016

jancborchardt deleted the comments-user-mention branch October 17, 2016 16:45

BFallert mentioned this pull request Oct 14, 2021

Contacts with umlaut cannot be selected via search #29242

Closed

Uh oh!

Notifications for simple @-mentioning in comments #1449

Notifications for simple @-mentioning in comments #1449

Uh oh!

Conversation

blizzz commented Sep 19, 2016

Uh oh!

mention-bot commented Sep 19, 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!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

blizzz commented Sep 27, 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!

nickvergessen commented Sep 28, 2016

Todos for follow up PRs

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

blizzz commented Sep 20, 2016 •

edited

Loading

jancborchardt commented Oct 6, 2016 •

edited

Loading

blizzz commented Oct 6, 2016 •

edited

Loading

blizzz commented Oct 12, 2016 •

edited

Loading

codecov-io commented Oct 12, 2016 •

edited

Loading