Moved BadRequestException checks to middleware, removed uneeded unit tests in BoardApiController

Signed-off-by: Ryan Fletcher <ryan.fletcher@codepassion.ca>

lib/BadRequestException.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Ryan Fletcher <ryan.fletcher@codepassion.ca>
+ *
+ * @author Ryan Fletcher <ryan.fletcher@codepassion.ca>
+ *
+ * @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\Deck;
+
+use OCP\AppFramework\Http;
+class BadRequestException extends \Exception {
+
+	public function __construct($message) {
+		parent::__construct($message);
+	}
+
+	public function getStatus() {
+		return HTTP::STATUS_BAD_REQUEST;
+	}
+}

lib/Controller/BoardApiController.php

@@ -63,7 +63,6 @@ class BoardApiController extends ApiController {
 	 */
 	public function index() {
 		$boards = $this->service->findAll();
-
 		return new DataResponse($boards, HTTP::STATUS_OK);
 	}
 
@@ -75,18 +74,8 @@ class BoardApiController extends ApiController {
 	 *
 	 * Return the board specified by $this->request->getParam('boardId').
 	 */
-	public function get() {
+	public function get() {		
-
-		if (is_numeric($this->request->getParam('boardId')) === false) {
-			return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
 		$board = $this->service->find($this->request->getParam('boardId'));
-
-		if ($board === false || $board === null) {
-			return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($board, HTTP::STATUS_OK);
 	}
 
@@ -100,22 +89,8 @@ class BoardApiController extends ApiController {
 	 *
 	 * Create a board with the specified title and color.
 	 */
-	public function create($title, $color) {
+	public function create($title, $color) {		
-
+		$board = $this->service->create($title, $this->userId, $color);		
-		if ($title === false || $title === null) {
-			return new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if ($color === false || $color === null) {
-			return new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		$board = $this->service->create($title, $this->userId, $color);
-
-		if ($board === false || $board === null) {
-			return new DataResponse('Internal Server Error', HTTP::STATUS_INTERNAL_SERVER_ERROR);
-		}
-
 		return new DataResponse($board, HTTP::STATUS_OK);
 	}
 
@@ -131,29 +106,7 @@ class BoardApiController extends ApiController {
 	 * Update a board with the specified boardId, title and color, and archived state.
 	 */
 	public function update($title, $color, $archived = false) {		
-
-		if (is_numeric($this->request->getParam('boardId')) === false) {
-			return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if (is_bool($archived) === false) {
-			return new DataResponse('archived must be a boolean', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if ($title === false || $title === null) {
-			return new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if ($color === false || $color === null) {
-			return new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST);
-		}
-
 		$board = $this->service->update($this->request->getParam('boardId'), $title, $color, $archived);
-
-		if ($board === false || $board === null) {
-			return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($board, HTTP::STATUS_OK);
 	}
 
@@ -166,17 +119,7 @@ class BoardApiController extends ApiController {
 	 * Delete the board specified by $boardId.  Return the board that was deleted.
 	 */
 	public function delete() {
-
+		$board = $this->service->delete($this->request->getParam('boardId'));
-		if (is_numeric($this->request->getParam('boardId')) === false) {
-			return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}		
-
-		$board = $this->service->delete($this->request->getParam('boardId'));		
-
-		if ($board === false || $board === null) {
-			return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($board, HTTP::STATUS_OK);
 	}
 
@@ -188,18 +131,8 @@ class BoardApiController extends ApiController {
 	 *
 	 * Undo the deletion of the board specified by $boardId.
 	 */
-	public function undoDelete() {
+	public function undoDelete() {		
-
+		$board = $this->service->deleteUndo($this->request->getParam('boardId'));		
-		if (is_numeric($this->request->getParam('boardId')) === false) {
-			return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}		
-
-		$board = $this->service->deleteUndo($this->request->getParam('boardId'));
-
-		if ($board === false || $board === null) {
-			return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($board, HTTP::STATUS_OK);
 	}
 

lib/Middleware/SharingMiddleware.php

@@ -24,6 +24,7 @@
 namespace OCA\Deck\Middleware;
 
 use OCA\Deck\StatusException;
+use OCA\Deck\BadRequestException;
 use OCP\AppFramework\Middleware;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\ILogger;
@@ -59,7 +60,7 @@ class SharingMiddleware extends Middleware {
 	 * @throws \Exception
 	 */
 	public function afterException($controller, $methodName, \Exception $exception) {
-		if ($exception instanceof StatusException) {
+		if ($exception instanceof StatusException || $exception instanceof BadRequestException) {
 			if ($this->config->getSystemValue('loglevel', Util::WARN) === Util::DEBUG) {
 				$this->logger->logException($exception);
 			}

lib/Service/BoardService.php

@@ -36,6 +36,7 @@ use OCA\Deck\Db\Board;
 use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\LabelMapper;
 use OCP\IUserManager;
+use OCA\Deck\BadRequestException;
 
 
 class BoardService {
@@ -111,8 +112,14 @@ class BoardService {
 	 * @throws DoesNotExistException
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function find($boardId) {
+
+		if ( is_numeric($boardId) === false ) {
+			throw new BadRequestException('board id must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ);
 		/** @var Board $board */
 		$board = $this->boardMapper->find($boardId, true, true);
@@ -201,13 +208,27 @@ class BoardService {
 	 * @param $userId
 	 * @param $color
 	 * @return \OCP\AppFramework\Db\Entity
+	 * @throws BadRequestException
 	 */
 	public function create($title, $userId, $color) {
+
+		if ($title === false || $title === null) {
+			throw new BadRequestException('title must be provided');
+		}
+
+		if ($userId === false || $userId === null) {
+			throw new BadRequestException('userId must be provided');
+		}
+
+		if ($color === false || $color === null) {
+			throw new BadRequestException('color must be provided');
+		}
+
 		$board = new Board();
 		$board->setTitle($title);
 		$board->setOwner($userId);
 		$board->setColor($color);
-		$new_board = $this->boardMapper->insert($board);
+		$new_board = $this->boardMapper->insert($board);		
 
 		// create new labels
 		$default_labels = [
@@ -243,8 +264,14 @@ class BoardService {
 	 * @throws DoesNotExistException
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function delete($id) {
+
+		if (is_numeric($id) === false) {
+			throw new BadRequestException('board id must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ);
 		$board = $this->find($id);
 		$board->setDeletedAt(time());
@@ -260,6 +287,11 @@ class BoardService {
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
 	 */
 	public function deleteUndo($id) {
+
+		if (is_numeric($id) === false) {
+			throw new BadRequestException('board id must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ);
 		$board = $this->find($id);
 		$board->setDeletedAt(0);
@@ -288,8 +320,26 @@ class BoardService {
 	 * @throws DoesNotExistException
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function update($id, $title, $color, $archived) {
+
+		if (is_numeric($id) === false) {
+			throw new BadRequestException('board id must be a number');
+		}
+
+		if ($title === false || $title === null) {
+			throw new BadRequestException('color must be provided');
+		}
+
+		if ($color === false || $color === null) {
+			throw new BadRequestException('color must be provided');
+		}
+
+		if ( is_bool($archived) === false ) {
+			throw new BadRequestException('archived must be a boolean');
+		}
+
 		$this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE);
 		$board = $this->find($id);
 		$board->setTitle($title);

tests/unit/controller/BoardApiControllerTest.php

@@ -92,31 +92,6 @@ class BoardApiControllerTest extends \Test\TestCase {
 
 		$expected = new DataResponse($board, HTTP::STATUS_OK);
 		$actual = $this->controller->get();
-		$this->assertEquals($expected, $actual);
-	}	
-
-	public function testGetBadRequest() {
-
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('hello'));
-
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->get();
-
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testGetNotFound() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('999'));
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->get();
-
 		$this->assertEquals($expected, $actual);
 	}
 	
@@ -132,20 +107,8 @@ class BoardApiControllerTest extends \Test\TestCase {
 		$expected = new DataResponse($board, HTTP::STATUS_OK);
 		$actual = $this->controller->create($this->exampleBoard['title'], $this->exampleBoard['color']);
 		$this->assertEquals($expected, $actual);
-	}
+	}	
 
-	public function testCreateBadTitle() {		
-		$expected = new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->create(null, $this->exampleBoard['color']);
-		$this->assertEquals($expected, $actual);
-	}
-	
-	public function testCreateBadColor() {		
-		$expected = new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->create($this->exampleBoard['title'], null);
-		$this->assertEquals($expected, $actual);
-	}
-	
 	public function testUpdate() {
 		$board = new Board();
 		$board->setId($this->exampleBoard['id']);
@@ -163,61 +126,9 @@ class BoardApiControllerTest extends \Test\TestCase {
 		$expected = new DataResponse($board, HTTP::STATUS_OK);
 		$actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']);
 		$this->assertEquals($expected, $actual);		
-	}
+	}					
-
+	
-	public function testUpdateBadId() {		
+	public function testDelete() {
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']);
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testUpdateBadArchived() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue($this->exampleBoard['id']));			
-
-		$expected = new DataResponse('archived must be a boolean', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color'], 'Not a boolean value');
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testUpdateBadTitle() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue($this->exampleBoard['id']));			
-
-		$expected = new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->update(null, $this->exampleBoard['color']);
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testUpdateBadColor() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue($this->exampleBoard['id']));			
-
-		$expected = new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->update($this->exampleBoard['title'], null);
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testUpdateBoardNotFound() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue($this->exampleBoard['id']));			
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']);
-		$this->assertEquals($expected, $actual);
-	}
-
-	// TODO: Write testDelete()
-	public function testDelete() {		
-
 		$board = new Board();
 		$board->setId($this->exampleBoard['id']);
 		$board->setTitle($this->exampleBoard['title']);
@@ -235,33 +146,7 @@ class BoardApiControllerTest extends \Test\TestCase {
 		$actual = $this->controller->delete();
 
 		$this->assertEquals($expected, $actual);
-	}
+	}		
-
-	public function testDeleteBadId() {
-
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('bad id'));
-
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->delete();
-
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testDeleteNotFound() {
-		
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('85'));
-		
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->delete();
-
-		$this->assertEquals($expected, $actual);
-	}	
 	
 	public function testUndoDelete() {
 		$board = new board();
@@ -281,28 +166,4 @@ class BoardApiControllerTest extends \Test\TestCase {
 		$actual = $this->controller->undoDelete();
 		$this->assertEquals($expected, $actual);
 	}
-
-	public function testUndoDeleteBadId() {		
-
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('bad id'));
-
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->undoDelete();
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testUndoDeleteNotFound() {
-
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue(189));
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->undoDelete();
-		$this->assertEquals($expected, $actual);
-	}
 }