Moved Data Response checks into stack serivice, cleaned up related unit tests.

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

lib/Controller/StackApiController.php

@@ -65,14 +65,7 @@ class StackApiController extends ApiController {
 	 * Return all of the stacks in the specified board.
 	 */
 	public function index() {				
-		$boardError = $this->apiHelper->entityHasError($this->request->getParam('boardId'), 'board', $this->boardService);
-
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
 		$stacks = $this->stackService->findAll($this->request->getParam('boardId'));
-
 		return new DataResponse($stacks, HTTP::STATUS_OK);
 	}
 
@@ -84,22 +77,7 @@ class StackApiController extends ApiController {
 	 * Return all of the stacks in the specified board.
 	 */
 	public function get() {
-		if (is_numeric($this->request->getParam('stackId')) === false) {
-			return new DataResponse('stack id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-		
-		$boardError = $this->apiHelper->entityHasError($this->request->getParam('boardId'), 'board', $this->boardService);
-
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}		
-
 		$stack = $this->stackService->find($this->request->getParam('stackId'));
-
-		if ($stack === false || $stack === null) {
-			return new DataResponse('stack not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($stack, HTTP::STATUS_OK);
 	}
 	
@@ -114,23 +92,7 @@ class StackApiController extends ApiController {
 	 * Create a stack with the specified title and order.
 	 */
 	public function create($title, $order) {
-
-		if ($title === false || $title === null) {
-			return new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if (is_numeric($order) === false) {
-			return new DataResponse('order must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		$boardError = $this->apiHelper->entityHasError( $this->request->getParam('boardId'), 'board', $this->boardService );
-
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}		
-		
 		$stack = $this->stackService->create($title, $this->request->getParam('boardId'), $order);
-		
 		return new DataResponse($stack, HTTP::STATUS_OK);
 	}
 
@@ -145,32 +107,8 @@ class StackApiController extends ApiController {
 	 * Update a stack by the specified stackId and boardId with the values that were put.
 	 */
 	public function update($title, $order) {		
-		
-		$boardError = $this->apiHelper->entityHasError( $this->request->getParam('boardId'), 'board', $this->boardService );
-
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-		
-		if (is_numeric($this->request->getParam('stackId')) === false) {
-			return new DataResponse('stack id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if (is_numeric($order) === false) {
-			return new DataResponse('order must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		try {
 		$stack = $this->stackService->update($this->request->getParam('stackId'), $title, $this->request->getParam('boardId'), $order);
-
-			if ($stack === false || $stack === null) {
-				return new DataResponse('stack not found', HTTP::STATUS_NOT_FOUND);
-			}
-	
 		return new DataResponse($stack, HTTP::STATUS_OK);
-		} catch (StatusException $e) {			
-			return new DataResponse($e->getMessage(), HTTP::STATUS_INTERNAL_SERVER_ERROR);
-		}
 	}
 
 	/**
@@ -181,23 +119,7 @@ class StackApiController extends ApiController {
 	 * Delete the stack specified by $this->request->getParam('stackId').
 	 */
 	public function delete() {				
-
-		$boardError = $this->apiHelper->entityHasError( $this->request->getParam('boardId'), 'board', $this->boardService );
-
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
-		if (is_numeric($this->request->getParam('stackId')) === false) {
-			return new DataResponse('stack id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
 		$stack = $this->stackService->delete($this->request->getParam('stackId'));
-		
-		if ($stack == false || $stack == null) {
-			return new DataResponse('Stack Not Found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($stack, HTTP::STATUS_OK);
 	}
 }

lib/Service/StackService.php

@@ -31,6 +31,7 @@ use OCA\Deck\Db\AssignedUsersMapper;
 use OCA\Deck\Db\Stack;
 use OCA\Deck\Db\StackMapper;
 use OCA\Deck\StatusException;
+use OCA\Deck\BadRequestException;
 use OCP\ICache;
 use OCP\ICacheFactory;
 
@@ -94,8 +95,13 @@ class StackService {
 	 * @return \OCP\AppFramework\Db\Entity
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function find($stackId) {
+		if (is_numeric($stackId) === false) {
+			throw new BadRequestException('stack id must be a number');
+		}
+
 		$stack = $this->stackMapper->find($stackId);
 		$cards = $this->cardMapper->findAll($stackId);
 		foreach ($cards as $cardIndex => $card) {
@@ -111,8 +117,13 @@ class StackService {
 	 * @param $boardId
 	 * @return array
 	 * @throws \OCA\Deck\NoPermissionException
+	 * @throws BadRequestException
 	 */
 	public function findAll($boardId) {
+		if (is_numeric($boardId) === false) {
+			throw new BadRequestException('boardId must be a number');
+		}
+
 		$this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_READ);
 		$stacks = $this->stackMapper->findAll($boardId);
 		$this->enrichStacksWithCards($stacks);
@@ -156,8 +167,22 @@ class StackService {
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function create($title, $boardId, $order) {
+
+		if ($title === false || $title === null) {
+			throw new BadRequestException('title must be provided');
+		}
+
+		if (is_numeric($order) === false) {
+			throw new BadRequestException('order must be a number');
+		}
+
+		if (is_numeric($boardId) === false) {
+			throw new BadRequestException('board id must be a number');
+		}
+
 		$this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_MANAGE);
 		if ($this->boardService->isArchived(null, $boardId)) {
 			throw new StatusException('Operation not allowed. This board is archived.');
@@ -176,8 +201,14 @@ class StackService {
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function delete($id) {
+
+		if ( is_numeric($id) === false ) {
+			throw new BadRequestException('stack id must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->stackMapper, $id, Acl::PERMISSION_MANAGE);
 
 		$stack = $this->stackMapper->find($id);
@@ -200,8 +231,26 @@ class StackService {
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
 	public function update($id, $title, $boardId, $order, $deletedAt) {
+
+		if (is_numeric($id) === false) {
+			throw new BadRequestException('stack id must be a number');
+		}
+
+		if ($title === false || $title === null) {
+			throw new BadRequestException('title must be provided');
+		}
+
+		if (is_numeric($boardId) === false) {
+			throw new BadRequestException('board id must be a number');
+		}
+
+		if (is_numeric($order) === false) {
+			throw new BadRequestException('order must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->stackMapper, $id, Acl::PERMISSION_MANAGE);
 		if ($this->boardService->isArchived($this->stackMapper, $id)) {
 			throw new StatusException('Operation not allowed. This board is archived.');

tests/unit/controller/StackApiControllerTest.php

@@ -69,12 +69,6 @@ class StackApiControllerTest extends \Test\TestCase {
 		$stack->setOrder($this->exampleStack['order']);
 		$stacks = [$stack];		
 
-		$board = new Board();
-		$board->setId($this->exampleBoard['boardId']);
-		$this->boardService->expects($this->once())
-			->method('find')
-			->willReturn($board);
-
 		$this->stackService->expects($this->once())
 			->method('findAll')
 			->willReturn($stacks);
@@ -89,139 +83,28 @@ class StackApiControllerTest extends \Test\TestCase {
 		$this->assertEquals($expected, $actual);
 	}
 
-	public function testIndexBadBoardId() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue('bad board id'));
-
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->index();
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testIndexBoardNotFound() {
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('boardId')
-			->will($this->returnValue(689));
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->index();
-		$this->assertEquals($expected, $actual);
-	}
-
 	public function testGet() {
 		$stack = new Stack();
 		$stack->setId($this->exampleStack['id']);
 		$stack->setBoardId($this->exampleStack['boardId']);
 		$stack->setOrder($this->exampleStack['order']);		
 
-		$board = new Board();
-		$board->setId($this->exampleBoard['boardId']);
-		$this->boardService->expects($this->once())
-			->method('find')
-			->willReturn($board);
-
 		$this->stackService->expects($this->once())
 			->method('find')
 			->willReturn($stack);
 
-		$this->request->expects($this->exactly(3))
+		$this->request->expects($this->once())
 			->method('getParam')
-			->withConsecutive(				
+			->with('stackId')
-				['stackId'],
+			->willReturn($this->exampleStack['id']);	
-				['boardId'],
-				['stackId']
-			)
-			->willReturnOnConsecutiveCalls(				
-				$this->returnValue($this->exampleStack['id']),
-				$this->returnValue($this->exampleBoard['boardId']),
-				$this->returnValue($this->exampleStack['id']));		
 
 		$expected = new DataResponse($stack, HTTP::STATUS_OK);
 		$actual = $this->controller->get();
 		$this->assertEquals($expected, $actual);
 	}				
 
-	public function testGetBadBoardId() {
-
-		$this->request->expects($this->exactly(2))
-			->method('getParam')
-			->withConsecutive(
-				['stackId'],
-				['boardId']				
-			)
-			->willReturnOnConsecutiveCalls(				
-				$this->returnValue(5),
-				$this->returnValue('not a board id'));
-
-		$expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->get();
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testGetBoardNotFound() {		
-		$this->request->expects($this->exactly(2))
-			->method('getParam')
-			->withConsecutive(
-				['stackId'],
-				['boardId']				
-			)
-			->willReturnOnConsecutiveCalls(				
-				$this->returnValue(5),
-				$this->returnValue($this->exampleBoard['boardId']));				
-
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->get();
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testGetStackBadStackId() {		
-
-		$this->request->expects($this->any())
-			->method('getParam')
-			->with('stackId')
-			->will($this->returnValue('not a stack id'));		
-
-		$expected = new DataResponse('stack id must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->get();
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testGetStackNotFound() {
-
-		$board = new Board();
-		$board->setId($this->exampleBoard['boardId']);
-		$this->boardService->expects($this->once())
-			->method('find')
-			->willReturn($board);		
-
-		$this->request->expects($this->exactly(3))
-			->method('getParam')
-			->withConsecutive(
-				['stackId'],
-				['boardId'],
-				['stackId']
-			)
-			->willReturnOnConsecutiveCalls(				
-				$this->returnValue(5),
-				$this->returnValue($this->exampleBoard['boardId']),
-				$this->returnValue(5));
-
-		$expected = new DataResponse('stack not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->get();
-		$this->assertEquals($expected, $actual);
-	}
-
 	public function testCreate() {
 
-		$board = new Board();
-		$board->setId($this->exampleBoard['boardId']);
-		$this->boardService->expects($this->once())
-			->method('find')
-			->willReturn($board);
 		$this->request->expects($this->any())
 			->method('getParam')
 			->with('boardId')
@@ -241,28 +124,4 @@ class StackApiControllerTest extends \Test\TestCase {
 		$actual = $this->controller->create($this->exampleStack['title'], $this->exampleStack['order']);
 		$this->assertEquals($expected, $actual);
 	}			
-
-	public function testCreateBadOrder() {
-		$expected = new DataResponse('order must be a number', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->create($this->exampleStack['title'], 'not an order number');
-		$this->assertEquals($expected, $actual);
-	}
-	
-	public function testCreateBadTitle() {
-		$expected = new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST);
-		$actual = $this->controller->create(null, 999);
-		$this->assertEquals($expected, $actual);
-	}
-
-	public function testCreateBoardNotFound() {
-
-		$this->request->expects($this->once())
-			->method('getParam')
-			->will($this->returnValue(453));
-
-		$expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND);
-		$actual = $this->controller->create($this->exampleStack['title'], $this->exampleStack['order']);
-		$this->assertEquals($expected, $actual);
-	}
-
 }