Skip to content

Commit

Permalink
Remove unneeded cardId parameters
Browse files Browse the repository at this point in the history
Signed-off-by: Julius Härtl <jus@bitgrid.net>
  • Loading branch information
juliusknorr committed Jul 11, 2020
1 parent cd0b3b2 commit 6114c28
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 88 deletions.
9 changes: 4 additions & 5 deletions lib/Controller/AttachmentApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ public function getAll() {
*
*/
public function display() {
$attachment = $this->attachmentService->display($this->request->getParam('cardId'), $this->request->getParam('attachmentId'));
return $attachment;
return $this->attachmentService->display($this->request->getParam('attachmentId'));
}

/**
Expand All @@ -76,7 +75,7 @@ public function create($type, $data) {
*
*/
public function update($data) {
$attachment = $this->attachmentService->update($this->request->getParam('cardId'), $this->request->getParam('attachmentId'), $data);
$attachment = $this->attachmentService->update($this->request->getParam('attachmentId'), $data);
return new DataResponse($attachment, HTTP::STATUS_OK);
}

Expand All @@ -87,7 +86,7 @@ public function update($data) {
*
*/
public function delete() {
$attachment = $this->attachmentService->delete($this->request->getParam('cardId'), $this->request->getParam('attachmentId'));
$attachment = $this->attachmentService->delete($this->request->getParam('attachmentId'));
return new DataResponse($attachment, HTTP::STATUS_OK);
}

Expand All @@ -98,7 +97,7 @@ public function delete() {
*
*/
public function restore() {
$attachment = $this->attachmentService->restore($this->request->getParam('cardId'), $this->request->getParam('attachmentId'));
$attachment = $this->attachmentService->restore($this->request->getParam('attachmentId'));
return new DataResponse($attachment, HTTP::STATUS_OK);
}
}
16 changes: 8 additions & 8 deletions lib/Controller/AttachmentController.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ public function getAll($cardId) {
* @return \OCP\AppFramework\Http\Response
* @throws \OCA\Deck\NotFoundException
*/
public function display($cardId, $attachmentId) {
return $this->attachmentService->display($cardId, $attachmentId);
public function display($attachmentId) {
return $this->attachmentService->display($attachmentId);
}

/**
Expand All @@ -70,21 +70,21 @@ public function create($cardId) {
/**
* @NoAdminRequired
*/
public function update($cardId, $attachmentId) {
return $this->attachmentService->update($cardId, $attachmentId, $this->request->getParam('data'));
public function update($attachmentId) {
return $this->attachmentService->update($attachmentId, $this->request->getParam('data'));
}

/**
* @NoAdminRequired
*/
public function delete($cardId, $attachmentId) {
return $this->attachmentService->delete($cardId, $attachmentId);
public function delete($attachmentId) {
return $this->attachmentService->delete($attachmentId);
}

/**
* @NoAdminRequired
*/
public function restore($cardId, $attachmentId) {
return $this->attachmentService->restore($cardId, $attachmentId);
public function restore($attachmentId) {
return $this->attachmentService->restore($attachmentId);
}
}
33 changes: 7 additions & 26 deletions lib/Service/AttachmentService.php
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ public function create($cardId, $type, $data) {
/**
* Display the attachment
*
* @param $cardId
* @param $attachmentId
* @return Response
* @throws BadRequestException
Expand All @@ -222,11 +221,7 @@ public function create($cardId, $type, $data) {
* @throws \OCP\AppFramework\Db\DoesNotExistException
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
*/
public function display($cardId, $attachmentId) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

public function display($attachmentId) {
if (is_numeric($attachmentId) === false) {
throw new BadRequestException('attachment id must be a number');
}
Expand All @@ -249,7 +244,6 @@ public function display($cardId, $attachmentId) {
/**
* Update an attachment with custom data
*
* @param $cardId
* @param $attachmentId
* @param $request
* @return mixed
Expand All @@ -258,11 +252,7 @@ public function display($cardId, $attachmentId) {
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
* @throws BadRequestException
*/
public function update($cardId, $attachmentId, $data) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

public function update($attachmentId, $data) {
if (is_numeric($attachmentId) === false) {
throw new BadRequestException('attachment id must be a number');
}
Expand All @@ -277,7 +267,7 @@ public function update($cardId, $attachmentId, $data) {
}

$this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
$this->cache->clear('card-' . $cardId);
$this->cache->clear('card-' . $attachment->getCardId());

$attachment->setData($data);
try {
Expand All @@ -304,19 +294,14 @@ public function update($cardId, $attachmentId, $data) {
* Either mark an attachment as deleted for later removal or just remove it depending
* on the IAttachmentService implementation
*
* @param $cardId
* @param $attachmentId
* @return \OCP\AppFramework\Db\Entity
* @throws \OCA\Deck\NoPermissionException
* @throws \OCP\AppFramework\Db\DoesNotExistException
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
* @throws BadRequestException
*/
public function delete($cardId, $attachmentId) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

public function delete($attachmentId) {
if (is_numeric($attachmentId) === false) {
throw new BadRequestException('attachment id must be a number');
}
Expand All @@ -328,7 +313,7 @@ public function delete($cardId, $attachmentId) {
}

$this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
$this->cache->clear('card-' . $cardId);
$this->cache->clear('card-' . $attachment->getCardId());

try {
$service = $this->getService($attachment->getType());
Expand All @@ -347,11 +332,7 @@ public function delete($cardId, $attachmentId) {
return $attachment;
}

public function restore($cardId, $attachmentId) {
if (is_numeric($cardId) === false) {
throw new BadRequestException('card id must be a number');
}

public function restore($attachmentId) {
if (is_numeric($attachmentId) === false) {
throw new BadRequestException('attachment id must be a number');
}
Expand All @@ -363,7 +344,7 @@ public function restore($cardId, $attachmentId) {
}

$this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
$this->cache->clear('card-' . $cardId);
$this->cache->clear('card-' . $attachment->getCardId());

try {
$service = $this->getService($attachment->getType());
Expand Down
2 changes: 1 addition & 1 deletion lib/Service/PermissionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public function matchPermissions(Board $board) {
*/
public function checkPermission($mapper, $id, $permission, $userId = null) {
$boardId = $id;
if ($mapper instanceof IPermissionMapper) {
if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) {
$boardId = $mapper->findBoardId($id);
}
if ($boardId === null) {
Expand Down
4 changes: 1 addition & 3 deletions lib/Service/StackService.php
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,7 @@ public function findAll($boardId, $since = -1) {
}

public function fetchDeleted($boardId) {
$this->permissionService->checkPermission(
$this->boardMapper, $boardId, Acl::PERMISSION_READ
);
$this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ);
$stacks = $this->stackMapper->findDeleted($boardId);
$this->enrichStacksWithCards($stacks);

Expand Down
15 changes: 8 additions & 7 deletions tests/unit/Service/AttachmentServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ private function createAttachment($type, $data) {
$attachment = new Attachment();
$attachment->setType($type);
$attachment->setData($data);
$attachment->setCardId(123);
return $attachment;
}

Expand Down Expand Up @@ -255,7 +256,7 @@ public function testDisplay() {
->method('display')
->with($attachment)
->willReturn($response);
$actual = $this->attachmentService->display(123, 1);
$actual = $this->attachmentService->display(1);
$this->assertEquals($response, $actual);
}

Expand All @@ -272,7 +273,7 @@ public function testDisplayInvalid() {
->method('display')
->with($attachment)
->will($this->throwException(new InvalidAttachmentType('deck_file')));
$this->attachmentService->display(123, 1);
$this->attachmentService->display(1);
}
public function testUpdate() {
$attachment = $this->createAttachment('deck_file', 'file_name.jpg');
Expand All @@ -294,7 +295,7 @@ public function testUpdate() {
$a->setExtendedData(['mime' => 'image/jpeg']);
});

$actual = $this->attachmentService->update(123, 1, 'file_name.jpg');
$actual = $this->attachmentService->update(1, 'file_name.jpg');

$expected->setExtendedData(['mime' => 'image/jpeg']);
$expected->setLastModified($attachment->getLastModified());
Expand All @@ -318,7 +319,7 @@ public function testDelete() {
$this->attachmentMapper->expects($this->once())
->method('delete')
->willReturn($attachment);
$actual = $this->attachmentService->delete(123, 1);
$actual = $this->attachmentService->delete(1);
$this->assertEquals($expected, $actual);
}

Expand All @@ -343,7 +344,7 @@ public function testDeleteWithUndo() {
->method('update')
->willReturn($attachment);
$expected->setDeletedAt(23);
$actual = $this->attachmentService->delete(123, 1);
$actual = $this->attachmentService->delete(1);
$this->assertEquals($expected, $actual);
}

Expand All @@ -363,7 +364,7 @@ public function testRestore() {
->method('update')
->willReturn($attachment);
$expected->setDeletedAt(0);
$actual = $this->attachmentService->restore(123, 1);
$actual = $this->attachmentService->restore(1);
$this->assertEquals($expected, $actual);
}

Expand All @@ -380,6 +381,6 @@ public function testRestoreNotAllowed() {
$this->attachmentServiceImpl->expects($this->once())
->method('allowUndo')
->willReturn(false);
$actual = $this->attachmentService->restore(123, 1);
$actual = $this->attachmentService->restore(1);
}
}
36 changes: 8 additions & 28 deletions tests/unit/controller/AttachmentApiControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,9 @@ public function testDisplay() {
->method('display')
->willReturn($this->attachmentExample);

$this->request->expects($this->exactly(2))
$this->request->expects($this->once())
->method('getParam')
->withConsecutive(
['cardId'],
['attachmentId']
)->willReturnonConsecutiveCalls(
$this->cardId,
$this->attachmentExample->getId());
->willReturn($this->attachmentExample->getId());

$expected = $this->attachmentExample;
$actual = $this->controller->display();
Expand Down Expand Up @@ -114,14 +109,9 @@ public function testUpdate() {
->method('update')
->willReturn($this->attachmentExample);

$this->request->expects($this->exactly(2))
$this->request->expects($this->once())
->method('getParam')
->withConsecutive(
['cardId'],
['attachmentId']
)->willReturnonConsecutiveCalls(
$this->cardId,
$this->attachmentExample->getId());
->willReturn($this->attachmentExample->getId());

$expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK);
$actual = $this->controller->update($data);
Expand All @@ -133,14 +123,9 @@ public function testDelete() {
->method('delete')
->willReturn($this->attachmentExample);

$this->request->expects($this->exactly(2))
$this->request->expects($this->once())
->method('getParam')
->withConsecutive(
['cardId'],
['attachmentId']
)->willReturnonConsecutiveCalls(
$this->cardId,
$this->attachmentExample->getId());
->willReturn($this->attachmentExample->getId());

$expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK);
$actual = $this->controller->delete();
Expand All @@ -152,14 +137,9 @@ public function testRestore() {
->method('restore')
->willReturn($this->attachmentExample);

$this->request->expects($this->exactly(2))
$this->request->expects($this->once())
->method('getParam')
->withConsecutive(
['cardId'],
['attachmentId']
)->willReturnonConsecutiveCalls(
$this->cardId,
$this->attachmentExample->getId());
->willReturn($this->attachmentExample->getId());

$expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK);
$actual = $this->controller->restore();
Expand Down
19 changes: 9 additions & 10 deletions tests/unit/controller/AttachmentControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ public function setUp(): void {
$this->controller = new AttachmentController(
'deck',
$this->request,
$this->attachmentService,
$this->userId
$this->attachmentService
);
}

Expand All @@ -55,8 +54,8 @@ public function testGetAll() {
}

public function testDisplay() {
$this->attachmentService->expects($this->once())->method('display')->with(1, 2);
$this->controller->display(1, 2);
$this->attachmentService->expects($this->once())->method('display')->with(2);
$this->controller->display(2);
}

public function testCreate() {
Expand All @@ -76,25 +75,25 @@ public function testUpdate() {
->will($this->onConsecutiveCalls('data'));
$this->attachmentService->expects($this->once())
->method('update')
->with(1, 2, 'data')
->with(2, 'data')
->willReturn(1);
$this->assertEquals(1, $this->controller->update(1, 2));
$this->assertEquals(1, $this->controller->update(2));
}


public function testDelete() {
$this->attachmentService->expects($this->once())
->method('delete')
->with(123, 234)
->with(234)
->willReturn(1);
$this->assertEquals(1, $this->controller->delete(123, 234));
$this->assertEquals(1, $this->controller->delete(234));
}

public function testRestore() {
$this->attachmentService->expects($this->once())
->method('restore')
->with(123, 234)
->with(234)
->willReturn(1);
$this->assertEquals(1, $this->controller->restore(123, 234));
$this->assertEquals(1, $this->controller->restore(234));
}
}

0 comments on commit 6114c28

Please sign in to comment.