Merge pull request #33614 from nextcloud/perf/noid/user-displayname-c…

…ache-for-comment-mentions Use user displayname cache for comment mentions
nextcloud · Aug 19, 2022 · 7d7f7ab · 7d7f7ab
2 parents fdec866 + ae67eba
commit 7d7f7ab
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 65 deletions.
diff --git a/apps/comments/lib/Notification/Notifier.php b/apps/comments/lib/Notification/Notifier.php
@@ -100,9 +100,9 @@ public function prepare(INotification $notification, string $languageCode): INot
 		$displayName = $comment->getActorId();
 		$isDeletedActor = $comment->getActorType() === ICommentsManager::DELETED_USER;
 		if ($comment->getActorType() === 'users') {
-			$commenter = $this->userManager->get($comment->getActorId());
-			if ($commenter instanceof IUser) {
-				$displayName = $commenter->getDisplayName();
+			$commenter = $this->userManager->getDisplayName($comment->getActorId());
+			if ($commenter !== null) {
+				$displayName = $commenter;
 			}
 		}
 
@@ -171,8 +171,8 @@ public function commentToRichMessage(IComment $comment): array {
 		$mentions = $comment->getMentions();
 		foreach ($mentions as $mention) {
 			if ($mention['type'] === 'user') {
-				$user = $this->userManager->get($mention['id']);
-				if (!$user instanceof IUser) {
+				$userDisplayName = $this->userManager->getDisplayName($mention['id']);
+				if ($userDisplayName === null) {
 					continue;
 				}
 			}

diff --git a/apps/comments/tests/Unit/Notification/NotifierTest.php b/apps/comments/tests/Unit/Notification/NotifierTest.php
@@ -39,27 +39,28 @@
 use OCP\IUserManager;
 use OCP\L10N\IFactory;
 use OCP\Notification\INotification;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class NotifierTest extends TestCase {
 
 	/** @var Notifier */
 	protected $notifier;
-	/** @var IFactory|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IFactory|MockObject */
 	protected $l10nFactory;
-	/** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IL10N|MockObject */
 	protected $l;
-	/** @var IRootFolder|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IRootFolder|MockObject */
 	protected $folder;
-	/** @var ICommentsManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var ICommentsManager|MockObject */
 	protected $commentsManager;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IURLGenerator|MockObject */
 	protected $url;
-	/** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IUserManager|MockObject */
 	protected $userManager;
-	/** @var INotification|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var INotification|MockObject */
 	protected $notification;
-	/** @var IComment|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IComment|MockObject */
 	protected $comment;
 	/** @var string */
 	protected $lc = 'tlh_KX';
@@ -97,15 +98,7 @@ public function testPrepareSuccess() {
 		$displayName = 'Huraga';
 		$message = '@Huraga mentioned you in a comment on "Gre\'thor.odp"';
 
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $user */
-		$user = $this->createMock(IUser::class);
-		$user->expects($this->once())
-			->method('getDisplayName')
-			->willReturn($displayName);
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $you */
-		$you = $this->createMock(IUser::class);
-
-		/** @var Node|\PHPUnit\Framework\MockObject\MockObject $node */
+		/** @var Node|MockObject $node */
 		$node = $this->createMock(Node::class);
 		$node
 			->expects($this->atLeastOnce())
@@ -213,10 +206,10 @@ public function testPrepareSuccess() {
 
 		$this->userManager
 			->expects($this->exactly(2))
-			->method('get')
+			->method('getDisplayName')
 			->willReturnMap([
-				['huraga', $user],
-				['you', $you],
+				['huraga', $displayName],
+				['you', 'You'],
 			]);
 
 		$this->notifier->prepare($this->notification, $this->lc);
@@ -226,10 +219,7 @@ public function testPrepareSuccessDeletedUser() {
 		$fileName = 'Gre\'thor.odp';
 		$message = 'You were mentioned on "Gre\'thor.odp", in a comment by a user that has since been deleted';
 
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $you */
-		$you = $this->createMock(IUser::class);
-
-		/** @var Node|\PHPUnit\Framework\MockObject\MockObject $node */
+		/** @var Node|MockObject $node */
 		$node = $this->createMock(Node::class);
 		$node
 			->expects($this->atLeastOnce())
@@ -334,9 +324,11 @@ public function testPrepareSuccessDeletedUser() {
 
 		$this->userManager
 			->expects($this->once())
-			->method('get')
-			->with('you')
-			->willReturn($you);
+			->method('getDisplayName')
+			->willReturnMap([
+				['huraga', null],
+				['you', 'You'],
+			]);
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}
@@ -373,7 +365,7 @@ public function testPrepareDifferentApp() {
 
 		$this->userManager
 			->expects($this->never())
-			->method('get');
+			->method('getDisplayName');
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}
@@ -411,7 +403,7 @@ public function testPrepareNotFound() {
 
 		$this->userManager
 			->expects($this->never())
-			->method('get');
+			->method('getDisplayName');
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}
@@ -422,12 +414,6 @@ public function testPrepareDifferentSubject() {
 
 		$displayName = 'Huraga';
 
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $user */
-		$user = $this->createMock(IUser::class);
-		$user->expects($this->once())
-			->method('getDisplayName')
-			->willReturn($displayName);
-
 		$this->folder
 			->expects($this->never())
 			->method('getById');
@@ -472,9 +458,9 @@ public function testPrepareDifferentSubject() {
 
 		$this->userManager
 			->expects($this->once())
-			->method('get')
+			->method('getDisplayName')
 			->with('huraga')
-			->willReturn($user);
+			->willReturn($displayName);
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}
@@ -485,12 +471,6 @@ public function testPrepareNotFiles() {
 
 		$displayName = 'Huraga';
 
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $user */
-		$user = $this->createMock(IUser::class);
-		$user->expects($this->once())
-			->method('getDisplayName')
-			->willReturn($displayName);
-
 		$this->folder
 			->expects($this->never())
 			->method('getById');
@@ -536,9 +516,9 @@ public function testPrepareNotFiles() {
 
 		$this->userManager
 			->expects($this->once())
-			->method('get')
+			->method('getDisplayName')
 			->with('huraga')
-			->willReturn($user);
+			->willReturn($displayName);
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}
@@ -549,12 +529,6 @@ public function testPrepareUnresolvableFileID() {
 
 		$displayName = 'Huraga';
 
-		/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $user */
-		$user = $this->createMock(IUser::class);
-		$user->expects($this->once())
-			->method('getDisplayName')
-			->willReturn($displayName);
-
 		$userFolder = $this->createMock(Folder::class);
 		$this->folder->expects($this->once())
 			->method('getUserFolder')
@@ -609,9 +583,9 @@ public function testPrepareUnresolvableFileID() {
 
 		$this->userManager
 			->expects($this->once())
-			->method('get')
+			->method('getDisplayName')
 			->with('huraga')
-			->willReturn($user);
+			->willReturn($displayName);
 
 		$this->notifier->prepare($this->notification, $this->lc);
 	}

diff --git a/lib/private/Server.php b/lib/private/Server.php
@@ -1186,14 +1186,12 @@ public function __construct($webRoot, \OC\Config $config) {
 
 			$manager->registerDisplayNameResolver('user', function ($id) use ($c) {
 				$manager = $c->get(IUserManager::class);
-				$user = $manager->get($id);
-				if (is_null($user)) {
-					$l = $c->getL10N('core');
-					$displayName = $l->t('Unknown user');
-				} else {
-					$displayName = $user->getDisplayName();
+				$userDisplayName = $manager->getDisplayName($id);
+				if ($userDisplayName === null) {
+					$l = $c->get(IFactory::class)->get('core');
+					return $l->t('Unknown user');
 				}
-				return $displayName;
+				return $userDisplayName;
 			});
 
 			return $manager;