From 8304f5f5088077074472d24fe244c82600d2b1a8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Mar 2015 13:12:14 +0100 Subject: [PATCH 1/4] Fix getting the avatar of a non-existing user --- core/avatar/avatarcontroller.php | 12 ++++++---- tests/core/avatar/avatarcontrollertest.php | 27 ++++++++++++++++++---- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/core/avatar/avatarcontroller.php b/core/avatar/avatarcontroller.php index dd6e8fdc71..f63e02b776 100644 --- a/core/avatar/avatarcontroller.php +++ b/core/avatar/avatarcontroller.php @@ -101,11 +101,13 @@ class AvatarController extends Controller { ['Content-Type' => $image->mimeType()]); $resp->setETag(crc32($image->data())); } else { - $resp = new DataResponse( - ['data' => [ - 'displayname' => $this->userManager->get($userId)->getDisplayName() - ] - ]); + $user = $this->userManager->get($userId); + $userName = $user ? $user->getDisplayName() : ''; + $resp = new DataResponse([ + 'data' => [ + 'displayname' => $userName, + ], + ]); } $resp->addHeader('Pragma', 'public'); diff --git a/tests/core/avatar/avatarcontrollertest.php b/tests/core/avatar/avatarcontrollertest.php index f43cd7fedd..cc9ff62807 100644 --- a/tests/core/avatar/avatarcontrollertest.php +++ b/tests/core/avatar/avatarcontrollertest.php @@ -74,7 +74,6 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['Request'] = $this->getMockBuilder('OCP\IRequest') ->disableOriginalConstructor()->getMock(); - $this->avatarMock = $this->getMockBuilder('OCP\IAvatar') ->disableOriginalConstructor()->getMock(); $this->userMock = $this->getMockBuilder('OCP\IUser') @@ -82,11 +81,11 @@ class AvatarControllerTest extends \Test\TestCase { $this->avatarController = $this->container['AvatarController']; - // Store current User + // Store current User $this->oldUser = \OC_User::getUser(); // Create a dummy user - $this->user = \OC::$server->getSecureRandom()->getLowStrengthGenerator()->generate(12, ISecureRandom::CHAR_LOWER); + $this->user = $this->getUniqueID('user'); OC::$server->getUserManager()->createUser($this->user, $this->user); \OC_Util::tearDownFS(); @@ -102,7 +101,8 @@ class AvatarControllerTest extends \Test\TestCase { // Configure userMock $this->userMock->method('getDisplayName')->willReturn($this->user); $this->userMock->method('getUID')->willReturn($this->user); - $this->container['UserManager']->method('get')->willReturn($this->userMock); + $this->container['UserManager']->method('get') + ->willReturnMap([[$this->user, $this->userMock]]); $this->container['UserSession']->method('getUser')->willReturn($this->userMock); } @@ -134,7 +134,7 @@ class AvatarControllerTest extends \Test\TestCase { } /** - * Fetch the users avatar + * Fetch the user's avatar */ public function testGetAvatar() { $image = new Image(OC::$SERVERROOT.'/tests/data/testimage.jpg'); @@ -150,6 +150,23 @@ class AvatarControllerTest extends \Test\TestCase { $this->assertEquals(crc32($response->getData()), $response->getEtag()); } + /** + * Fetch the avatar of a non-existing user + */ + public function testGetAvatarNoUser() { + $image = new Image(OC::$SERVERROOT.'/tests/data/testimage.jpg'); + $this->avatarMock->method('get')->willReturn($image); + $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); + + $response = $this->avatarController->getAvatar($this->user . 'doesnotexist', 32); + + $this->assertEquals($response->getStatus(), Http::STATUS_OK); + + $image2 = new Image($response->getData()); + $this->assertEquals($image->mimeType(), $image2->mimeType()); + $this->assertEquals(crc32($response->getData()), $response->getEtag()); + } + /** * Make sure we get the correct size */ From 61ec37431ac98d812a0ef95cc4bb86d6c2d558f5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Mar 2015 15:14:01 +0100 Subject: [PATCH 2/4] Fix order of expected and actual on assertEquals() calls --- tests/core/avatar/avatarcontrollertest.php | 38 +++++++++++----------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/core/avatar/avatarcontrollertest.php b/tests/core/avatar/avatarcontrollertest.php index cc9ff62807..3fa0ef2cee 100644 --- a/tests/core/avatar/avatarcontrollertest.php +++ b/tests/core/avatar/avatarcontrollertest.php @@ -128,9 +128,9 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->getAvatar($this->user, 32); - //Comment out unitl JS is fixed - //$this->assertEquals($response->getStatus(), Http::STATUS_NOT_FOUND); - $this->assertEquals($response->getData()['data']['displayname'], $this->user); + //Comment out until JS is fixed + //$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); + $this->assertEquals($this->user, $response->getData()['data']['displayname']); } /** @@ -143,7 +143,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->getAvatar($this->user, 32); - $this->assertEquals($response->getStatus(), Http::STATUS_OK); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); $image2 = new Image($response->getData()); $this->assertEquals($image->mimeType(), $image2->mimeType()); @@ -160,7 +160,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->getAvatar($this->user . 'doesnotexist', 32); - $this->assertEquals($response->getStatus(), Http::STATUS_OK); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); $image2 = new Image($response->getData()); $this->assertEquals($image->mimeType(), $image2->mimeType()); @@ -213,7 +213,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->deleteAvatar(); - $this->assertEquals($response->getStatus(), Http::STATUS_OK); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); } /** @@ -224,7 +224,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->deleteAvatar(); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -232,7 +232,7 @@ class AvatarControllerTest extends \Test\TestCase { */ public function testTmpAvatarNoTmp() { $response = $this->avatarController->getTmpAvatar(); - $this->assertEquals($response->getStatus(), Http::STATUS_NOT_FOUND); + $this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); } /** @@ -242,7 +242,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['Cache']->method('get')->willReturn(file_get_contents(OC::$SERVERROOT.'/tests/data/testimage.jpg')); $response = $this->avatarController->getTmpAvatar(); - $this->assertEquals($response->getStatus(), Http::STATUS_OK); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); } @@ -252,7 +252,7 @@ class AvatarControllerTest extends \Test\TestCase { public function testPostAvatarNoPathOrImage() { $response = $this->avatarController->postAvatar(null); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -275,7 +275,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->postAvatar(null); //On correct upload always respond with the notsquare message - $this->assertEquals($response->getData()['data'], 'notsquare'); + $this->assertEquals('notsquare', $response->getData()['data']); //File should be deleted $this->assertFalse(file_exists($fileName)); @@ -291,7 +291,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->postAvatar(null); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -313,7 +313,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->postAvatar(null); - $this->assertEquals($response->getData()['data']['message'], 'Unknown filetype'); + $this->assertEquals('Unknown filetype', $response->getData()['data']['message']); //File should be deleted $this->assertFalse(file_exists($fileName)); @@ -331,7 +331,7 @@ class AvatarControllerTest extends \Test\TestCase { $response = $this->avatarController->postAvatar('avatar.jpg'); //On correct upload always respond with the notsquare message - $this->assertEquals($response->getData()['data'], 'notsquare'); + $this->assertEquals('notsquare', $response->getData()['data']); } /** @@ -340,7 +340,7 @@ class AvatarControllerTest extends \Test\TestCase { public function testPostCroppedAvatarInvalidCrop() { $response = $this->avatarController->postCroppedAvatar([]); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -349,7 +349,7 @@ class AvatarControllerTest extends \Test\TestCase { public function testPostCroppedAvatarNoTmpAvatar() { $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 10]); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -362,7 +362,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 11]); - $this->assertEquals($response->getStatus(), Http::STATUS_BAD_REQUEST); + $this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus()); } /** @@ -373,8 +373,8 @@ class AvatarControllerTest extends \Test\TestCase { $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 10]); - $this->assertEquals($response->getStatus(), Http::STATUS_OK); - $this->assertEquals($response->getData()['status'], 'success'); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); + $this->assertEquals('success', $response->getData()['status']); } } From 5d71eb5670acdc0c94c01fbce945ad6c062f2e25 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Mar 2015 13:30:26 +0100 Subject: [PATCH 3/4] Color avatars of non-existing users gray and display X instead --- core/js/jquery.avatar.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/js/jquery.avatar.js b/core/js/jquery.avatar.js index 7c19cb321f..74acaac792 100644 --- a/core/js/jquery.avatar.js +++ b/core/js/jquery.avatar.js @@ -85,7 +85,9 @@ if (result.data && result.data.displayname) { $div.imageplaceholder(user, result.data.displayname); } else { - $div.imageplaceholder(user); + // User does not exist + $div.imageplaceholder(user, 'X'); + $div.css('background-color', '#b9b9b9'); } } else { $div.hide(); From 30357aaac0fc58c592ecfab4bd8271c674128316 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 13 Mar 2015 17:35:34 +0100 Subject: [PATCH 4/4] No user no avatar easy as that --- tests/core/avatar/avatarcontrollertest.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/core/avatar/avatarcontrollertest.php b/tests/core/avatar/avatarcontrollertest.php index 3fa0ef2cee..a5456eb678 100644 --- a/tests/core/avatar/avatarcontrollertest.php +++ b/tests/core/avatar/avatarcontrollertest.php @@ -130,6 +130,7 @@ class AvatarControllerTest extends \Test\TestCase { //Comment out until JS is fixed //$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); + $this->assertEquals(Http::STATUS_OK, $response->getStatus()); $this->assertEquals($this->user, $response->getData()['data']['displayname']); } @@ -154,17 +155,15 @@ class AvatarControllerTest extends \Test\TestCase { * Fetch the avatar of a non-existing user */ public function testGetAvatarNoUser() { - $image = new Image(OC::$SERVERROOT.'/tests/data/testimage.jpg'); - $this->avatarMock->method('get')->willReturn($image); + $this->avatarMock->method('get')->willReturn(null); $this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock); $response = $this->avatarController->getAvatar($this->user . 'doesnotexist', 32); + //Comment out until JS is fixed + //$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus()); $this->assertEquals(Http::STATUS_OK, $response->getStatus()); - - $image2 = new Image($response->getData()); - $this->assertEquals($image->mimeType(), $image2->mimeType()); - $this->assertEquals(crc32($response->getData()), $response->getEtag()); + $this->assertEquals('', $response->getData()['data']['displayname']); } /**