Split Card Update in CardApiController as it was gigantic and broke codacy complexity rules by a lot. Also moved validation checks into respective services.

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

lib/Controller/CardApiController.php

@@ -68,26 +68,7 @@ class CardApiController extends ApiController {
 	 * Get a specific card.
 	 */
 	public function get() {		
-		$boardError = $this->apiHelper->entityHasError($this->request->params['boardId'], 'board', $this->boardService);
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
-		$stackError = $this->apiHelper->entityHasError($this->request->params['stackId'], 'stack', $this->stackService);		
-		if ($stackError) {
-			return new DataResponse($stackError['message'], $stackError['status']);
-		}
-
-		if (is_numeric($this->request->params['cardId']) === false) {
-			return new DataResponse('card id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
 		$card = $this->cardService->find($this->request->params['cardId']);		
-
-		if ($card === false || $card === null) {
-			return new DataResponse('Card not found', HTTP::STATUS_NOT_FOUND);
-		}
-
 		return new DataResponse($card, HTTP::STATUS_OK);
 	}
 
@@ -103,31 +84,7 @@ class CardApiController extends ApiController {
 	 * Get a specific card.
 	 */
 	public function create($title, $type = 'plain', $order = 999) {				
-
-		$boardError = $this->apiHelper->entityHasError($this->request->params['boardId'], 'board', $this->boardService);
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
-		$stackError = $this->apiHelper->entityHasError($this->request->params['stackId'], 'stack', $this->stackService);		
-		if ($stackError) {
-			return new DataResponse($stackError['message'], $stackError['status']);
-		}
-
-		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);
-		}
-
-		try {
 		$card = $this->cardService->create($title, $this->request->params['stackId'], $type, $order, $this->userId);
-		} catch (Exception $e) {
-			return new DataResponse($e->getMessage(), HTTP::STATUS_INTERNAL_SERVER_ERROR);
-		}
-		
 		return new DataResponse($card, HTTP::STATUS_OK);
 	}
 
@@ -136,78 +93,23 @@ class CardApiController extends ApiController {
 	 * @CORS
 	 * @NoCSRFRequired
 	 *	 
-	 * @params $title
-	 * @params $type
-	 * @params $order
-	 * @params $description
-	 * @params $duedate
-	 * @params $archive
-	 * @params $assignedUserId
 	 * 
-	 * Get a specific card.
+	 * Update a card
 	 */
-	public function update($title, $type, $order, $description = null, $duedate = null, $archive = false, $assignedUserId = 0) {
+	public function update($cardId, $title, $stackId, $type, $order = 0, $description = '', $owner, $duedate = null) {
-
+		$card = $this->cardService->update($this->request->getParam('cardId'), $title, $this->request->getParam('stackId'), $type, $order, $description, $owner, $duedate);
-		$boardError = $this->apiHelper->entityHasError($this->request->params['boardId'], 'board', $this->boardService);
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
-		$stackError = $this->apiHelper->entityHasError($this->request->params['stackId'], 'stack', $this->stackService);		
-		if ($stackError) {
-			return new DataResponse($stackError['message'], $stackError['status']);
-		}
-
-		if (is_numeric($this->request->params['cardId']) === false) {
-			return new DataResponse('card id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}		
-
-		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);
-		}
-
-		if (is_bool($order) === false) {
-			return new DataResponse('archive must be a boolean', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		if (is_numeric($assignedUserId) === false) {
-			return new DataResponse('user id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}
-
-		try {
-			$card = $this->cardService->update(
-				$this->request->params['cardId'],
-				$title,
-				$this->request->params['stackId'],
-				$type,
-				$order,
-				$description,
-				$this->userId,
-				$duedate);
-
-			if ($archive) {
-				$card = $this->cardService->archive($this->request->params['cardId']);
-			} else {
-				$card = $this->cardService->unarchive($this->request->params['cardId']);
-			}
-
-			if ($assignedUserId > 0) {
-				$card = $this->cardService->assignUser($this->request->params['cardId'], $assignedUserId);
-			} else {
-				$card = $this->cardService->assignUser($this->request->params['cardId'], $assignedUserId);
-			}
-
-		} catch(Exception $e) {
-			return new DataResponse($e->getMessage(), HTTP::STATUS_INTERNAL_SERVER_ERROR);
-		}
-
 		return new DataResponse($card, HTTP::STATUS_OK);
 	}
 
+	
+	public function assignUser() {
+		throw new Exception('Not Implemented');
+	}
+
+	public function unassignUser() {
+		throw new Exception('Not Implemented');
+	}
+
 	/**
 	 * @NoAdminRequired
 	 * @CORS
@@ -216,27 +118,7 @@ class CardApiController extends ApiController {
 	 * Delete a specific card.
 	 */
 	public function delete() {
-
-		$boardError = $this->apiHelper->entityHasError($this->request->params['boardId'], 'board', $this->boardService);
-		if ($boardError) {
-			return new DataResponse($boardError['message'], $boardError['status']);
-		}
-
-		$stackError = $this->apiHelper->entityHasError($this->request->params['stackId'], 'stack', $this->stackService);		
-		if ($stackError) {
-			return new DataResponse($stackError['message'], $stackError['status']);
-		}
-
-		if (is_numeric($this->request->params['cardId']) === false) {
-			return new DataResponse('card id must be a number', HTTP::STATUS_BAD_REQUEST);
-		}		
-
-		try {
 		$card = $this->cardService->delete($this->request->params['cardId']);
-		} catch (Exception $e) {
-			return new DataResponse($e.getMessage(), HTTP::STATUS_INTERNAL_SERVER_ERROR);
-		}		
-
 		return new DataResponse($card, HTTP::STATUS_OK);
 	}
 }

lib/Service/CardService.php

@@ -34,7 +34,7 @@ use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\LabelMapper;
 use OCA\Deck\NotFoundException;
 use OCA\Deck\StatusException;
-
+use OCA\Deck\BadRequestException;
 
 class CardService {
 
@@ -117,8 +117,30 @@ class CardService {
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadrequestException
 	 */
 	public function create($title, $stackId, $type, $order, $owner) {
+
+		if ($title === 'false' || $title === null) {
+			throw new BadRequestException('title must be provided');
+		}
+
+		if (is_numeric($stackId) === false) {
+			throw new BadRequestException('stack id must be a number');
+		}
+
+		if ($type === 'false' || $type === null) {
+			throw new BadRequestException('type must be provided');
+		}
+
+		if (is_numeric($order) === false) {
+			throw new BadRequestException('order must be a number');
+		}
+
+		if ($owner === false || $owner === null) {
+			throw new BadRequestException('owner must be provided');
+		}
+
 		$this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT);
 		if ($this->boardService->isArchived($this->stackMapper, $stackId)) {
 			throw new StatusException('Operation not allowed. This board is archived.');
@@ -130,7 +152,6 @@ class CardService {
 		$card->setOrder($order);
 		$card->setOwner($owner);
 		return $this->cardMapper->insert($card);
-
 	}
 
 	/**
@@ -140,8 +161,14 @@ class CardService {
 	 * @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('card id must be a number');
+		}
+
 		$this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT);
 		if ($this->boardService->isArchived($this->cardMapper, $id)) {
 			throw new StatusException('Operation not allowed. This board is archived.');
@@ -166,8 +193,30 @@ class CardService {
 	 * @throws \OCA\Deck\NoPermissionException
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
+	 * @throws BadRequestException
 	 */
-	public function update($id, $title, $stackId, $type, $order, $description, $owner, $duedate, $deletedAt) {
+	public function update($id, $title, $stackId, $type, $order = 0, $description = '', $owner, $duedate = null, $deletedAt) {
+
+		if (is_numeric($id) === false) {
+			throw new BadRequestException('card id must be a number');			
+		}
+
+		if ($title === false || $title === null) {
+			throw new BadRequestException('title must be provided');
+		}
+
+		if (is_numeric($stackId) === false) {
+			throw new BadRequestException('stack id must be a number $$$');
+		}
+
+		if ($type === false || $type === null) {
+			throw new BadRequestException('type must be provided');
+		}
+
+		if ($owner === false || $owner === null) {
+			throw new BadRequestException('owner must be provided');
+		}
+
 		$this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT);
 		if ($this->boardService->isArchived($this->cardMapper, $id)) {
 			throw new StatusException('Operation not allowed. This board is archived.');