From bbe44108b5661d0c3935fa76e27abbc433fd458e Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 25 Oct 2018 23:41:12 +0200 Subject: [PATCH 1/2] only write when the displayname differs, but then announce it refs #5212 and fixes #9112 Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 11 +++++++++-- apps/user_ldap/tests/User/UserTest.php | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 02764a72ec..4afc8032f7 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -414,14 +414,21 @@ class User { * * @param string $displayName * @param string $displayName2 - * @returns string the effective display name + * @return string the effective display name */ public function composeAndStoreDisplayName($displayName, $displayName2 = '') { $displayName2 = (string)$displayName2; if($displayName2 !== '') { $displayName .= ' (' . $displayName2 . ')'; } - $this->store('displayName', $displayName); + $oldName = $this->config->getUserValue($this->uid, 'user_ldap', 'displayName', ''); + if ($oldName !== $displayName) { + $this->store('displayName', $displayName); + $user = $this->userManager->get($this->getUsername()); + if ($user instanceof \OC\User\User) { + $user->triggerChange('displayName', $displayName); + } + } return $displayName; } diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index 837c72a3a3..63c90c6ac7 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -1010,11 +1010,36 @@ class UserTest extends \Test\TestCase { public function testComposeAndStoreDisplayName($part1, $part2, $expected) { $this->config->expects($this->once()) ->method('setUserValue'); + $this->config->expects($this->once()) + ->method('getUserValue'); + + $ncUserObj = $this->createMock(\OC\User\User::class); + $ncUserObj->expects($this->once()) + ->method('triggerChange') + ->with('displayName', $expected); + $this->userManager->expects($this->once()) + ->method('get') + ->willReturn($ncUserObj); $displayName = $this->user->composeAndStoreDisplayName($part1, $part2); $this->assertSame($expected, $displayName); } + public function testComposeAndStoreDisplayNameNoOverwrite() { + $displayName = 'Randall Flagg'; + $this->config->expects($this->never()) + ->method('setUserValue'); + $this->config->expects($this->once()) + ->method('getUserValue') + ->willReturn($displayName); + + $this->userManager->expects($this->never()) + ->method('get'); // Implicit: no triggerChange can be called + + $composedDisplayName = $this->user->composeAndStoreDisplayName($displayName); + $this->assertSame($composedDisplayName, $displayName); + } + public function testHandlePasswordExpiryWarningDefaultPolicy() { $this->connection->expects($this->any()) ->method('__get') From 49456e42f958bec9b0fc9d07c5ec37ff51cc9351 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 26 Oct 2018 17:38:42 +0200 Subject: [PATCH 2/2] do not run into UniqueConstraintViolationException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … when an unmapped user logs in for the first time when background job mode is ajax and no memcache was configured. Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/User/User.php | 8 +++++--- apps/user_ldap/tests/User/UserTest.php | 26 ++++++++++++++++++-------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 4afc8032f7..f4be19a7ad 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -421,11 +421,13 @@ class User { if($displayName2 !== '') { $displayName .= ' (' . $displayName2 . ')'; } - $oldName = $this->config->getUserValue($this->uid, 'user_ldap', 'displayName', ''); - if ($oldName !== $displayName) { + $oldName = $this->config->getUserValue($this->uid, 'user_ldap', 'displayName', null); + if ($oldName !== $displayName) { $this->store('displayName', $displayName); $user = $this->userManager->get($this->getUsername()); - if ($user instanceof \OC\User\User) { + if (!empty($oldName) && $user instanceof \OC\User\User) { + // if it was empty, it would be a new record, not a change emitting the trigger could + // potentially cause a UniqueConstraintViolationException, depending on some factors. $user->triggerChange('displayName', $displayName); } } diff --git a/apps/user_ldap/tests/User/UserTest.php b/apps/user_ldap/tests/User/UserTest.php index 63c90c6ac7..6ff9defe47 100644 --- a/apps/user_ldap/tests/User/UserTest.php +++ b/apps/user_ldap/tests/User/UserTest.php @@ -998,25 +998,35 @@ class UserTest extends \Test\TestCase { public function displayNameProvider() { return [ - ['Roland Deschain', '', 'Roland Deschain'], - ['Roland Deschain', null, 'Roland Deschain'], - ['Roland Deschain', 'gunslinger@darktower.com', 'Roland Deschain (gunslinger@darktower.com)'], + ['Roland Deschain', '', 'Roland Deschain', false], + ['Roland Deschain', '', 'Roland Deschain', true], + ['Roland Deschain', null, 'Roland Deschain', false], + ['Roland Deschain', 'gunslinger@darktower.com', 'Roland Deschain (gunslinger@darktower.com)', false], + ['Roland Deschain', 'gunslinger@darktower.com', 'Roland Deschain (gunslinger@darktower.com)', true], ]; } /** * @dataProvider displayNameProvider */ - public function testComposeAndStoreDisplayName($part1, $part2, $expected) { + public function testComposeAndStoreDisplayName($part1, $part2, $expected, $expectTriggerChange) { $this->config->expects($this->once()) ->method('setUserValue'); + $oldName = $expectTriggerChange ? 'xxGunslingerxx' : null; $this->config->expects($this->once()) - ->method('getUserValue'); + ->method('getUserValue') + ->with($this->user->getUsername(), 'user_ldap', 'displayName', null) + ->willReturn($oldName); $ncUserObj = $this->createMock(\OC\User\User::class); - $ncUserObj->expects($this->once()) - ->method('triggerChange') - ->with('displayName', $expected); + if ($expectTriggerChange) { + $ncUserObj->expects($this->once()) + ->method('triggerChange') + ->with('displayName', $expected); + } else { + $ncUserObj->expects($this->never()) + ->method('triggerChange'); + } $this->userManager->expects($this->once()) ->method('get') ->willReturn($ncUserObj);