From 1b1a3669d317c55afc7cb1dad79a3698e677bf51 Mon Sep 17 00:00:00 2001 From: Viktor Diezel Date: Tue, 5 Aug 2025 16:19:33 +0200 Subject: [PATCH 1/2] fix!: always sets the cardBefore property on the CardUpdatedEvent Signed-off-by: Viktor Diezel --- lib/Service/AssignmentService.php | 13 +++++++++++-- lib/Service/CardService.php | 22 +++++++++++++--------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/lib/Service/AssignmentService.php b/lib/Service/AssignmentService.php index e6dc0b9c28..7e398f672a 100644 --- a/lib/Service/AssignmentService.php +++ b/lib/Service/AssignmentService.php @@ -138,7 +138,12 @@ public function assignUser($cardId, $userId, int $type = Assignment::TYPE_USER) $this->changeHelper->cardChanged($cardId); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_CARD_USER_ASSIGN, ['assigneduser' => $userId]); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $cardBefore = clone $card; + $cardBefore->setAssignedUsers($assignments); + + $newAssignments = $this->assignedUsersMapper->findAll($cardId); + $card->setAssignedUsers($newAssignments); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $cardBefore)); return $assignment; } @@ -168,8 +173,12 @@ public function unassignUser($cardId, $userId, $type = 0) { } $this->changeHelper->cardChanged($cardId); + $cardBefore = clone $card; + $cardBefore->setAssignedUsers($assignments); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $newAssignments = $this->assignedUsersMapper->findAll($cardId); + $card->setAssignedUsers($newAssignments); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $cardBefore)); return $assignment; } diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 8fad26447d..d1709ce7ab 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -421,11 +421,11 @@ public function rename($id, $title) { $changes = new ChangeSet($card); $card->setTitle($title); $this->changeHelper->cardChanged($card->getId(), false); - $update = $this->cardMapper->update($card); + $newCard = $this->cardMapper->update($card); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $changes->getBefore())); - return $update; + return $newCard; } /** @@ -511,7 +511,7 @@ public function archive($id) { $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $newCard, ActivityManager::SUBJECT_CARD_UPDATE_ARCHIVE); $this->changeHelper->cardChanged($id, false); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $changes->getBefore())); return $newCard; } @@ -540,7 +540,7 @@ public function unarchive($id) { $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $newCard, ActivityManager::SUBJECT_CARD_UPDATE_UNARCHIVE); $this->changeHelper->cardChanged($id, false); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $changes->getBefore())); return $newCard; } @@ -567,7 +567,7 @@ public function done(int $id): Card { $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $newCard, ActivityManager::SUBJECT_CARD_UPDATE_DONE); $this->changeHelper->cardChanged($id, false); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $changes->getBefore())); return $newCard; } @@ -593,7 +593,7 @@ public function undone(int $id): Card { $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $newCard, ActivityManager::SUBJECT_CARD_UPDATE_UNDONE); $this->changeHelper->cardChanged($id, false); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $changes->getBefore())); return $newCard; } @@ -624,11 +624,13 @@ public function assignLabel($cardId, $labelId) { if ($label->getBoardId() !== $this->cardMapper->findBoardId($card->getId())) { throw new StatusException('Operation not allowed. Label does not exist.'); } + $this->cardMapper->assignLabel($cardId, $labelId); $this->changeHelper->cardChanged($cardId); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_LABEL_ASSIGN, ['label' => $label]); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $newCard = $this->cardMapper->find($cardId); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $card)); } /** @@ -655,11 +657,13 @@ public function removeLabel($cardId, $labelId) { throw new StatusException('Operation not allowed. This card is archived.'); } $label = $this->labelMapper->find($labelId); + $this->cardMapper->removeLabel($cardId, $labelId); $this->changeHelper->cardChanged($cardId); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_LABEL_UNASSING, ['label' => $label]); - $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $newCard = $this->cardMapper->find($cardId); + $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $card)); } public function getCardUrl(int $cardId): string { From 86d19c4f0b9df9cd2bf9a78a5abea0ad82922fcc Mon Sep 17 00:00:00 2001 From: Viktor Diezel Date: Tue, 5 Aug 2025 18:15:51 +0200 Subject: [PATCH 2/2] test: add test to assert that CardUpdatedEvent is called Signed-off-by: Viktor Diezel --- tests/unit/Service/AssignmentServiceTest.php | 31 ++++++++++++++---- tests/unit/Service/CardServiceTest.php | 33 +++++++++++++++++--- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/tests/unit/Service/AssignmentServiceTest.php b/tests/unit/Service/AssignmentServiceTest.php index bef8fa60d8..71b999a08e 100644 --- a/tests/unit/Service/AssignmentServiceTest.php +++ b/tests/unit/Service/AssignmentServiceTest.php @@ -31,6 +31,7 @@ use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\ChangeHelper; +use OCA\Deck\Event\CardUpdatedEvent; use OCA\Deck\NotFoundException; use OCA\Deck\Notification\NotificationHelper; use OCA\Deck\Validators\AssignmentServiceValidator; @@ -123,14 +124,14 @@ public function mockActivity($type, $object, $subject) { public function testAssignUser() { $assignments = []; - $this->assignedUsersMapper->expects($this->once()) - ->method('findAll') - ->with(123) - ->willReturn($assignments); $assignment = new Assignment(); $assignment->setCardId(123); $assignment->setParticipant('admin'); $assignment->setType(Assignment::TYPE_USER); + $this->assignedUsersMapper->expects($this->exactly(2)) + ->method('findAll') + ->with(123) + ->willReturn([], [$assignment]); $this->cardMapper->expects($this->once()) ->method('findBoardId') ->willReturn(1); @@ -145,6 +146,15 @@ public function testAssignUser() { ->method('insert') ->with($assignment) ->willReturn($assignment); + $this->eventDispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function ($event) use ($assignment) { + $this->assertInstanceOf(CardUpdatedEvent::class, $event); + $this->assertNotNull($event->getCard()); + $this->assertNotNull($event->getCardBefore()); + return true; + })); $actual = $this->assignmentService->assignUser(123, 'admin'); $this->assertEquals($assignment, $actual); } @@ -200,14 +210,23 @@ public function testUnassignUserExisting() { $assignments = [ $assignment ]; - $this->assignedUsersMapper->expects($this->once()) + $this->assignedUsersMapper->expects($this->exactly(2)) ->method('findAll') ->with(123) - ->willReturn($assignments); + ->willReturn($assignments, []); $this->assignedUsersMapper->expects($this->once()) ->method('delete') ->with($assignment) ->willReturn($assignment); + $this->eventDispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function ($event) { + $this->assertInstanceOf(CardUpdatedEvent::class, $event); + $this->assertNotNull($event->getCard()); + $this->assertNotNull($event->getCardBefore()); + return true; + })); $actual = $this->assignmentService->unassignUser(123, 'admin'); $this->assertEquals($assignment, $actual); } diff --git a/tests/unit/Service/CardServiceTest.php b/tests/unit/Service/CardServiceTest.php index 0f1e1445e5..46c7305844 100644 --- a/tests/unit/Service/CardServiceTest.php +++ b/tests/unit/Service/CardServiceTest.php @@ -36,6 +36,7 @@ use OCA\Deck\Db\LabelMapper; use OCA\Deck\Db\Stack; use OCA\Deck\Db\StackMapper; +use OCA\Deck\Event\CardUpdatedEvent; use OCA\Deck\Model\CardDetails; use OCA\Deck\Notification\NotificationHelper; use OCA\Deck\StatusException; @@ -164,6 +165,23 @@ public function mockActivity($type, $object, $subject) { ->with($event); } + private function expectCardUpdatedEvent(?callable $assertions = null): void { + $this->eventDispatcher + ->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function ($event) use ($assertions) { + $this->assertInstanceOf(CardUpdatedEvent::class, $event); + $this->assertNotNull($event->getCard()); + $this->assertNotNull($event->getCardBefore()); + + if ($assertions !== null) { + $assertions($event); + } + + return true; + })); + } + public function testFind() { $user = $this->createMock(IUser::class); $this->userManager->expects($this->once()) @@ -254,9 +272,9 @@ public function testClone() { $this->cardMapper->expects($this->once()) ->method('update')->willReturn($clonedCard); - $this->cardMapper->expects($this->exactly(2)) + $this->cardMapper->expects($this->exactly(3)) ->method('find') - ->willReturn($card, $clonedCard); + ->willReturn($card, $clonedCard, $clonedCard); $this->cardMapper->expects($this->any()) ->method('findBoardId') @@ -335,6 +353,7 @@ public function testUpdate() { ->method('find') ->with(234) ->willReturn($stack); + $this->expectCardUpdatedEvent(); $actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null); $this->assertEquals('newtitle', $actual->getTitle()); $this->assertEquals(234, $actual->getStackId()); @@ -362,6 +381,7 @@ public function testRename() { $this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) { return $c; }); + $this->expectCardUpdatedEvent(); $actual = $this->cardService->rename(123, 'newtitle'); $this->assertEquals('newtitle', $actual->getTitle()); } @@ -391,6 +411,7 @@ public function testReorder($cardId, $newPosition, $order) { $card = new Card(); $card->setStackId(123); $this->cardMapper->expects($this->once())->method('find')->willReturn($card); + $this->expectCardUpdatedEvent(); $result = $this->cardService->reorder($cardId, 123, $newPosition); foreach ($result as $card) { $actual[$card->getOrder()] = $card->getId(); @@ -428,6 +449,7 @@ public function testArchive() { $this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) { return $c; }); + $this->expectCardUpdatedEvent(); $this->assertTrue($this->cardService->archive(123)->getArchived()); } public function testUnarchive() { @@ -438,6 +460,7 @@ public function testUnarchive() { $this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) { return $c; }); + $this->expectCardUpdatedEvent(); $this->assertFalse($this->cardService->unarchive(123)->getArchived()); } @@ -447,7 +470,7 @@ public function testAssignLabel() { $card->setId(123); $label = new Label(); $label->setBoardId(1); - $this->cardMapper->expects($this->once())->method('find')->willReturn($card); + $this->cardMapper->expects($this->exactly(2))->method('find')->willReturn($card, $card); $this->cardMapper->expects($this->once())->method('assignLabel'); $this->cardMapper->expects($this->once()) ->method('findBoardId') @@ -455,6 +478,7 @@ public function testAssignLabel() { $this->labelMapper->expects($this->once()) ->method('find') ->willReturn($label); + $this->expectCardUpdatedEvent(); $this->cardService->assignLabel(123, 999); } @@ -473,11 +497,12 @@ public function testRemoveLabel() { $card->setId(123); $label = new Label(); $label->setBoardId(1); - $this->cardMapper->expects($this->once())->method('find')->willReturn($card); + $this->cardMapper->expects($this->exactly(2))->method('find')->willReturn($card, $card); $this->cardMapper->expects($this->once())->method('removeLabel'); $this->labelMapper->expects($this->once()) ->method('find') ->willReturn($label); + $this->expectCardUpdatedEvent(); $this->cardService->removeLabel(123, 999); }