fix!: Consistent card/cardBefore on CardUpdatedEvent

lib/Service/AssignmentService.php

@@ -134,7 +134,12 @@ public function assignUser(int $cardId, string $userId, int $type = Assignment::
 		$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;
 	}
@@ -161,8 +166,12 @@ public function unassignUser(int $cardId, string $userId, int $type = 0): Assign
 				}
 				$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;
 			}

lib/Service/CardService.php

@@ -400,11 +400,11 @@ public function rename(int $id, string $title): Card {
 		$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;
 	}
 
 	/**
@@ -485,7 +485,7 @@ public function archive(int $id): Card {
 		$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;
 	}
@@ -512,7 +512,7 @@ public function unarchive(int $id): Card {
 		$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;
 	}
@@ -537,7 +537,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;
 	}
@@ -563,7 +563,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;
 	}
@@ -592,12 +592,13 @@ public function assignLabel(int $cardId, int $labelId): Card {
 		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));
-		return $card;
+		$newCard = $this->cardMapper->find($cardId);
+		$this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $card));
 	}
 
 	/**
@@ -621,12 +622,13 @@ public function removeLabel(int $cardId, int $labelId): Card {
 			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));
-		return $card;
+		$newCard = $this->cardMapper->find($cardId);
+		$this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($newCard, $card));
 	}
 
 	public function getCardUrl(int $cardId): string {

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);
 	}

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())
@@ -259,9 +277,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')
@@ -340,6 +358,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());
@@ -367,6 +386,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());
 	}
@@ -396,6 +416,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();
@@ -433,6 +454,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() {
@@ -443,6 +465,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());
 	}
 
@@ -452,14 +475,15 @@ 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')
 			->willReturn(1);
 		$this->labelMapper->expects($this->once())
 			->method('find')
 			->willReturn($label);
+		$this->expectCardUpdatedEvent();
 		$this->cardService->assignLabel(123, 999);
 	}
 
@@ -478,11 +502,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);
 	}