Fix comments (and systemtags) when involving users with numerical ids

apps/comments/lib/Activity/Listener.php

@@ -31,7 +31,6 @@
 use OCP\Files\Node;
 use OCP\IUser;
 use OCP\IUserSession;
-use OCP\Share;
 use OCP\Share\IShareHelper;
 
 class Listener {
@@ -99,7 +98,7 @@ public function commentEvent(CommentsEvent $event) {
 				/** @var Node $node */
 				$node = array_shift($nodes);
 				$al = $this->shareHelper->getPathsForAccessList($node);
-				$users = array_merge($users, $al['users']);
+				$users += $al['users'];
 			}
 		}
 
@@ -120,7 +119,9 @@ public function commentEvent(CommentsEvent $event) {
 			]);
 
 		foreach ($users as $user => $path) {
-			$activity->setAffectedUser($user);
+			// numerical user ids end up as integers from array keys, but string
+			// is required
+			$activity->setAffectedUser((string)$user);
 
 			$activity->setSubject('add_comment_subject', [
 				'actor' => $actor,

apps/comments/tests/Unit/Activity/ListenerTest.php

@@ -0,0 +1,187 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\Comments\Tests\Unit\Activity;
+
+use OCA\Comments\Activity\Listener;
+use OCP\Activity\IEvent;
+use OCP\Activity\IManager;
+use OCP\App\IAppManager;
+use OCP\Comments\CommentsEvent;
+use OCP\Comments\IComment;
+use OCP\Files\Config\ICachedMountFileInfo;
+use OCP\Files\Config\IMountProviderCollection;
+use OCP\Files\Config\IUserMountCache;
+use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
+use OCP\Files\Node;
+use OCP\IUser;
+use OCP\IUserSession;
+use OCP\Share\IShareHelper;
+use Test\TestCase;
+
+class ListenerTest extends TestCase {
+
+	/** @var Listener */
+	protected $listener;
+
+	/** @var IManager|\PHPUnit_Framework_MockObject_MockObject */
+	protected $activityManager;
+
+	/** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */
+	protected $session;
+
+	/** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */
+	protected $appManager;
+
+	/** @var IMountProviderCollection|\PHPUnit_Framework_MockObject_MockObject */
+	protected $mountProviderCollection;
+
+	/** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */
+	protected $rootFolder;
+
+	/** @var IShareHelper|\PHPUnit_Framework_MockObject_MockObject */
+	protected $shareHelper;
+
+	protected function setUp() {
+		parent::setUp();
+
+		$this->activityManager = $this->createMock(IManager::class);
+		$this->session = $this->createMock(IUserSession::class);
+		$this->appManager = $this->createMock(IAppManager::class);
+		$this->mountProviderCollection = $this->createMock(IMountProviderCollection::class);
+		$this->rootFolder = $this->createMock(IRootFolder::class);
+		$this->shareHelper = $this->createMock(IShareHelper::class);
+
+		$this->listener = new Listener(
+			$this->activityManager,
+			$this->session,
+			$this->appManager,
+			$this->mountProviderCollection,
+			$this->rootFolder,
+			$this->shareHelper
+		);
+	}
+
+	public function testCommentEvent() {
+		$this->appManager->expects($this->any())
+			->method('isInstalled')
+			->with('activity')
+			->willReturn(true);
+
+		$comment = $this->createMock(IComment::class);
+		$comment->expects($this->any())
+			->method('getObjectType')
+			->willReturn('files');
+
+		/** @var CommentsEvent|\PHPUnit_Framework_MockObject_MockObject $event */
+		$event = $this->createMock(CommentsEvent::class);
+		$event->expects($this->any())
+			->method('getComment')
+			->willReturn($comment);
+		$event->expects($this->any())
+			->method('getEvent')
+			->willReturn(CommentsEvent::EVENT_ADD);
+
+		/** @var IUser|\PHPUnit_Framework_MockObject_MockObject $ownerUser */
+		$ownerUser = $this->createMock(IUser::class);
+		$ownerUser->expects($this->any())
+			->method('getUID')
+			->willReturn('937393');
+
+		/** @var \PHPUnit_Framework_MockObject_MockObject $mount */
+		$mount = $this->createMock(ICachedMountFileInfo::class);
+		$mount->expects($this->any())
+			->method('getUser')
+			->willReturn($ownerUser); // perhaps not the right user, but does not matter in this scenario
+
+		$mounts = [ $mount, $mount ]; // to make sure duplicates are dealt with
+
+		$userMountCache = $this->createMock(IUserMountCache::class);
+		$userMountCache->expects($this->any())
+			->method('getMountsForFileId')
+			->willReturn($mounts);
+
+		$this->mountProviderCollection->expects($this->any())
+			->method('getMountCache')
+			->willReturn($userMountCache);
+
+		$node = $this->createMock(Node::class);
+		$nodes = [ $node ];
+
+		$ownerFolder = $this->createMock(Folder::class);
+		$ownerFolder->expects($this->any())
+			->method('getById')
+			->willReturn($nodes);
+
+		$this->rootFolder->expects($this->any())
+			->method('getUserFolder')
+			->willReturn($ownerFolder);
+
+		$al = [ 'users' => [
+			'873304' => 'i/got/it/here',
+			'254342' => 'there/i/have/it',
+			'sandra' => 'and/here/i/placed/it'
+		]];
+		$this->shareHelper->expects($this->any())
+			->method('getPathsForAccessList')
+			->willReturn($al);
+
+		$this->session->expects($this->any())
+			->method('getUser')
+			->willReturn($ownerUser);
+
+		/** @var \PHPUnit_Framework_MockObject_MockObject $activity */
+		$activity = $this->createMock(IEvent::class);
+		$activity->expects($this->exactly(count($al['users'])))
+			->method('setAffectedUser');
+		$activity->expects($this->once())
+			->method('setApp')
+			->with('comments')
+			->willReturnSelf();
+		$activity->expects($this->once())
+			->method('setType')
+			->with('comments')
+			->willReturnSelf();
+		$activity->expects($this->once())
+			->method('setAuthor')
+			->with($ownerUser->getUID())
+			->willReturnSelf();
+		$activity->expects($this->once())
+			->method('setObject')
+			->with('files', $this->anything())
+			->willReturnSelf();
+		$activity->expects($this->once())
+			->method('setMessage')
+			->with('add_comment_message', $this->anything())
+			->willReturnSelf();
+
+		$this->activityManager->expects($this->once())
+			->method('generateEvent')
+			->willReturn($activity);
+		$this->activityManager->expects($this->exactly(count($al['users'])))
+			->method('publish');
+
+		$this->listener->commentEvent($event);
+	}
+}

apps/systemtags/lib/Activity/Listener.php

@@ -185,7 +185,7 @@ public function mapperEvent(MapperEvent $event) {
 				/** @var Node $node */
 				$node = array_shift($nodes);
 				$al = $this->shareHelper->getPathsForAccessList($node);
-				$users = array_merge($users, $al['users']);
+				$users += $al['users'];
 			}
 		}
 
@@ -203,6 +203,7 @@ public function mapperEvent(MapperEvent $event) {
 			->setObject($event->getObjectType(), (int) $event->getObjectId());
 
 		foreach ($users as $user => $path) {
+			$user = (string)$user; // numerical ids could be ints which are not accepted everywhere
 			$activity->setAffectedUser($user);
 
 			foreach ($tags as $tag) {

build/integration/features/comments.feature

@@ -16,21 +16,21 @@ Feature: comments
 
   Scenario: Creating a comment on a shared file belonging to another user
     Given user "user0" exists
-    Given user "user1" exists
+    Given user "12345" exists
     Given User "user0" uploads file "data/textfile.txt" to "/myFileToComment.txt"
     Given As "user0" sending "POST" to "/apps/files_sharing/api/v1/shares" with
       | path | myFileToComment.txt |
-      | shareWith | user1 |
+      | shareWith | 12345 |
       | shareType | 0 |
-    When "user1" posts a comment with content "A comment from another user" on the file named "/myFileToComment.txt" it should return "201"
-    Then As "user1" load all the comments of the file named "/myFileToComment.txt" it should return "207"
+    When "12345" posts a comment with content "A comment from another user" on the file named "/myFileToComment.txt" it should return "201"
+    Then As "12345" load all the comments of the file named "/myFileToComment.txt" it should return "207"
     And the response should contain a property "oc:parentId" with value "0"
     And the response should contain a property "oc:childrenCount" with value "0"
     And the response should contain a property "oc:verb" with value "comment"
     And the response should contain a property "oc:actorType" with value "users"
     And the response should contain a property "oc:objectType" with value "files"
     And the response should contain a property "oc:message" with value "A comment from another user"
-    And the response should contain a property "oc:actorDisplayName" with value "user1"
+    And the response should contain a property "oc:actorDisplayName" with value "12345"
     And the response should contain only "1" comments
 
   Scenario: Creating a comment on a non-shared file belonging to another user
@@ -206,4 +206,4 @@ Feature: comments
     And the response should contain a property "oc:message" with value "My first comment"
     And the response should contain a property "oc:actorDisplayName" with value "user1"
     And the response should contain only "1" comments
-    Then As "user0" edit the last created comment and set text to "My edited comment" it should return "403"
+    Then As "user0" edit the last created comment and set text to "My edited comment" it should return "403"

build/integration/features/tags.feature

@@ -114,14 +114,14 @@ Feature: tags
 
   Scenario: Assigning a normal tag to a file shared by someone else as regular user should work
     Given user "user0" exists
-    Given user "user1" exists
+    Given user "12345" exists
     Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName"
     Given user "user0" uploads file "data/textfile.txt" to "/myFileToTag.txt"
     Given As "user0" sending "POST" to "/apps/files_sharing/api/v1/shares" with
       | path | myFileToTag.txt |
-      | shareWith | user1 |
+      | shareWith | 12345 |
       | shareType | 0 |
-    When "user1" adds the tag "MySuperAwesomeTagName" to "/myFileToTag.txt" shared by "user0"
+    When "12345" adds the tag "MySuperAwesomeTagName" to "/myFileToTag.txt" shared by "user0"
     Then The response should have a status code "201"
     And "/myFileToTag.txt" shared by "user0" has the following tags
       |MySuperAwesomeTagName|