Skip to content

Commit 89761fc

Browse files
Lightshadow02claude
Lightshadow02
and
claude
committed
fix(cards): do not clear card color when update omits the field
Fixes #8131 Signed-off-by: HUGO LOUREIRO <149155946+Lightshadow02@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 524c579 commit 89761fc

5 files changed

Lines changed: 115 additions & 6 deletions

File tree

‎lib/Controller/CardApiController.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,9 @@ public function create($title, $type = 'plain', $order = 999, $description = '',
8888
#[NoAdminRequired]
8989
#[CORS]
9090
#[NoCSRFRequired]
91-
public function update(string $title, $type, string $owner, string $description = '', int $order = 0, $duedate = null, $startdate = null, $archived = null, ?string $color = null): DataResponse {
91+
public function update(string $title, $type, string $owner, string $description = '', int $order = 0, $duedate = null, $startdate = null, $archived = null): DataResponse {
9292
$done = array_key_exists('done', $this->request->getParams()) ? new OptionalNullableValue($this->request->getParam('done', null)) : null;
93+
$color = array_key_exists('color', $this->request->getParams()) ? new OptionalNullableValue($this->request->getParam('color', null)) : null;
9394
$card = $this->cardService->update($this->request->getParam('cardId'), $title, $this->request->getParam('stackId'), $type, $owner, $description, $order, $duedate, 0, $archived, $done, $startdate, $color);
9495
return new DataResponse($card, HTTP::STATUS_OK);
9596
}

‎lib/Controller/CardController.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,13 @@ public function create(string $title, int $stackId, string $type = 'plain', int
6464
* @param $duedate
6565
*/
6666
#[NoAdminRequired]
67-
public function update(int $id, string $title, int $stackId, string $type, int $order, string $description, $duedate, $deletedAt, $archived = null, $startdate = null, ?string $color = null): Card {
67+
public function update(int $id, string $title, int $stackId, string $type, int $order, string $description, $duedate, $deletedAt, $archived = null, $startdate = null): Card {
6868
$done = array_key_exists('done', $this->request->getParams())
6969
? new OptionalNullableValue($this->request->getParam('done', null))
7070
: null;
71+
$color = array_key_exists('color', $this->request->getParams())
72+
? new OptionalNullableValue($this->request->getParam('color', null))
73+
: null;
7174
return $this->cardService->update($id, $title, $stackId, $type, $this->userId, $description, $order, $duedate, $deletedAt, $archived, $done, $startdate, $color);
7275
}
7376

‎lib/Controller/CardOcsController.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,13 @@ public function removeLabel(?int $boardId, int $cardId, int $labelId): DataRespo
113113

114114
#[NoAdminRequired]
115115
#[PublicPage]
116-
public function update(int $id, string $title, int $stackId, string $type, int $order, string $description, $duedate, $deletedAt, int $boardId, array|string|null $owner = null, $archived = null, $startdate = null, ?string $color = null): DataResponse {
116+
public function update(int $id, string $title, int $stackId, string $type, int $order, string $description, $duedate, $deletedAt, int $boardId, array|string|null $owner = null, $archived = null, $startdate = null): DataResponse {
117117
$done = array_key_exists('done', $this->request->getParams())
118118
? new OptionalNullableValue($this->request->getParam('done', null))
119119
: null;
120+
$color = array_key_exists('color', $this->request->getParams())
121+
? new OptionalNullableValue($this->request->getParam('color', null))
122+
: null;
120123
if (!$owner) {
121124
$owner = $this->userId;
122125
} else {

‎lib/Service/CardService.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ public function delete(int $id): Card {
246246
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
247247
* @throws BadRequestException
248248
*/
249-
public function update(int $id, string $title, int $stackId, string $type, string $owner, string $description = '', int $order = 0, ?string $duedate = null, ?int $deletedAt = null, ?bool $archived = null, ?OptionalNullableValue $done = null, ?string $startdate = null, ?string $color = null): Card {
249+
public function update(int $id, string $title, int $stackId, string $type, string $owner, string $description = '', int $order = 0, ?string $duedate = null, ?int $deletedAt = null, ?bool $archived = null, ?OptionalNullableValue $done = null, ?string $startdate = null, ?OptionalNullableValue $color = null): Card {
250250
$this->cardServiceValidator->check(compact('id', 'title', 'stackId', 'type', 'owner', 'order'));
251251

252252
$this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT, allowDeletedCard: true);
@@ -289,7 +289,10 @@ public function update(int $id, string $title, int $stackId, string $type, strin
289289
$card->setOrder($order);
290290
$card->setDuedate($duedate ? new \DateTime($duedate) : null);
291291
$card->setStartdate($startdate ? new \DateTime($startdate) : null);
292-
$card->setColor($color);
292+
if ($color !== null) {
293+
$colorValue = $color->getValue();
294+
$card->setColor(is_string($colorValue) && $colorValue !== '' ? $colorValue : null);
295+
}
293296
$resetDuedateNotification = false;
294297
if (
295298
$card->getDuedate() === null

‎tests/unit/Service/CardServiceTest.php‎

Lines changed: 100 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use OCA\Deck\Db\Stack;
3838
use OCA\Deck\Db\StackMapper;
3939
use OCA\Deck\Model\CardDetails;
40+
use OCA\Deck\Model\OptionalNullableValue;
4041
use OCA\Deck\Notification\NotificationHelper;
4142
use OCA\Deck\StatusException;
4243
use OCA\Deck\Validators\CardServiceValidator;
@@ -368,7 +369,7 @@ public function testUpdate() {
368369
->method('find')
369370
->with(234)
370371
->willReturn($stack);
371-
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, 'ffffff');
372+
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, new OptionalNullableValue('ffffff'));
372373
$this->assertEquals('newtitle', $actual->getTitle());
373374
$this->assertEquals(234, $actual->getStackId());
374375
$this->assertEquals('text', $actual->getType());
@@ -378,6 +379,104 @@ public function testUpdate() {
378379
$this->assertEquals('ffffff', $actual->getColor());
379380
}
380381

382+
public function testUpdateKeepsColorWhenOmitted() {
383+
$card = Card::fromParams([
384+
'title' => 'Card title',
385+
'archived' => 'false',
386+
'stackId' => 234,
387+
'color' => '00ff00',
388+
]);
389+
$card->setColor('ff0000');
390+
$stack = Stack::fromParams([
391+
'id' => 234,
392+
'boardId' => 1337,
393+
]);
394+
$this->cardMapper->expects($this->once())->method('find')->willReturn($card);
395+
$this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) {
396+
$c->setId(1);
397+
return $c;
398+
});
399+
$this->stackMapper->expects($this->once())
400+
->method('find')
401+
->with(234)
402+
->willReturn($stack);
403+
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, null);
404+
$this->assertSame('ff0000', $actual->getColor());
405+
}
406+
407+
public function testUpdateClearsColorWhenNullProvided() {
408+
$card = Card::fromParams([
409+
'title' => 'Card title',
410+
'archived' => 'false',
411+
'stackId' => 234,
412+
'color' => '00ff00',
413+
]);
414+
$card->setColor('ff0000');
415+
$stack = Stack::fromParams([
416+
'id' => 234,
417+
'boardId' => 1337,
418+
]);
419+
$this->cardMapper->expects($this->once())->method('find')->willReturn($card);
420+
$this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) {
421+
$c->setId(1);
422+
return $c;
423+
});
424+
$this->stackMapper->expects($this->once())
425+
->method('find')
426+
->with(234)
427+
->willReturn($stack);
428+
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, new OptionalNullableValue(null));
429+
$this->assertNull($actual->getColor());
430+
}
431+
432+
public function testUpdateClearsColorWhenEmptyStringProvided() {
433+
$card = Card::fromParams([
434+
'title' => 'Card title',
435+
'archived' => 'false',
436+
'stackId' => 234,
437+
'color' => '00ff00',
438+
]);
439+
$card->setColor('ff0000');
440+
$stack = Stack::fromParams([
441+
'id' => 234,
442+
'boardId' => 1337,
443+
]);
444+
$this->cardMapper->expects($this->once())->method('find')->willReturn($card);
445+
$this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) {
446+
$c->setId(1);
447+
return $c;
448+
});
449+
$this->stackMapper->expects($this->once())
450+
->method('find')
451+
->with(234)
452+
->willReturn($stack);
453+
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, new OptionalNullableValue(''));
454+
$this->assertNull($actual->getColor());
455+
}
456+
457+
public function testUpdateSetsColor() {
458+
$card = Card::fromParams([
459+
'title' => 'Card title',
460+
'archived' => 'false',
461+
'stackId' => 234,
462+
]);
463+
$stack = Stack::fromParams([
464+
'id' => 234,
465+
'boardId' => 1337,
466+
]);
467+
$this->cardMapper->expects($this->once())->method('find')->willReturn($card);
468+
$this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) {
469+
$c->setId(1);
470+
return $c;
471+
});
472+
$this->stackMapper->expects($this->once())
473+
->method('find')
474+
->with(234)
475+
->willReturn($stack);
476+
$actual = $this->cardService->update(123, 'newtitle', 234, 'text', 'admin', 'foo', 999, '2017-01-01 00:00:00', null, null, null, null, new OptionalNullableValue('00ff00'));
477+
$this->assertSame('00ff00', $actual->getColor());
478+
}
479+
381480
public function testUpdateWithStartdate() {
382481
$card = Card::fromParams([
383482
'title' => 'Card title',

0 commit comments

Comments
 (0)