From 798a00d89dccd2ef1db298dcdec81061b60363f2 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 10 Jun 2026 23:41:07 +0200 Subject: [PATCH] fix(deck): preserve done/archived and validate target board on move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-1 review on PR #885: - Preserve `done` across a cross-board move. The internal card-update route (the only one that works cross-board — the board/stack-scoped route 404s for a card not already on that board) does not accept a done value, so a "done" card is re-marked done after the move. Deck stamps the current time there, so the original timestamp isn't preserved — documented as a route limitation. (`archived` is already preserved: CardService only mutates it when sent.) - Validate that target_stack_id is on target_board_id before moving, so the parameter is load-bearing and a mismatch fails loudly instead of misreporting. - Skip the same-board guard's get_stacks round-trip on a same-stack reorder. - Add unit coverage (done-restore call, destination validation, same-stack skip) and integration coverage (done preservation, target-board mismatch). Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/client/deck.py | 72 +++++--- nextcloud_mcp_server/server/deck.py | 15 +- tests/client/deck/test_deck_move_card_api.py | 155 +++++++++++++----- .../test_deck_move_card_to_board.py | 68 +++++++- 4 files changed, 243 insertions(+), 67 deletions(-) diff --git a/nextcloud_mcp_server/client/deck.py b/nextcloud_mcp_server/client/deck.py index 6d096af6..c17bbe79 100644 --- a/nextcloud_mcp_server/client/deck.py +++ b/nextcloud_mcp_server/client/deck.py @@ -392,13 +392,15 @@ class DeckClient(BaseNextcloudClient): # board silently orphans the card's labels (they keep the old boardId). # Cross-board moves must go through move_card_to_board(), which uses the # card-update route (CardService::update) and remaps labels by title. - board_stack_ids = {stack.id for stack in await self.get_stacks(board_id)} - if target_stack_id not in board_stack_ids: - raise ValueError( - f"target_stack_id {target_stack_id} is not a stack on board " - f"{board_id}; reorder_card only moves cards within a board. " - "Use move_card_to_board() to move a card to another board." - ) + # A same-stack reorder can't cross a board boundary, so skip the lookup. + if target_stack_id != stack_id: + board_stack_ids = {stack.id for stack in await self.get_stacks(board_id)} + if target_stack_id not in board_stack_ids: + raise ValueError( + f"target_stack_id {target_stack_id} is not a stack on board " + f"{board_id}; reorder_card only moves cards within a board. " + "Use move_card_to_board() to move a card to another board." + ) # Use the non-API route /cards/{cardId}/reorder which correctly reads # stackId from the body. The API route /api/.../stacks/{stackId}/cards/... @@ -418,31 +420,45 @@ class DeckClient(BaseNextcloudClient): source_board_id: int, source_stack_id: int, card_id: int, + target_board_id: int, target_stack_id: int, order: int = 0, ) -> DeckCard: """Move a card to a stack on a different (or the same) board. Unlike :meth:`reorder_card`, this goes through the card-update route - (``CardService::update``, which honours ``stackId`` in the body). When - the destination stack is on a different board, Deck remaps the card's - board-scoped labels by title — assigning the same-titled label on the - destination board, or cloning it there when the user has board-manage - permission — instead of leaving orphaned labels behind. This mirrors - Deck's native "Move/copy card" action. Card identity (id, comments, - attachments) is preserved. + (``CardService::update``). When the destination stack is on a different + board, Deck remaps the card's board-scoped labels by title — assigning + the same-titled label on the destination board, or cloning it there + when the user has board-manage permission — instead of leaving orphaned + labels behind. This mirrors Deck's native "Move/copy card" action. Card + identity (id, comments, attachments), ``archived`` state and the due + date are preserved. - The card-update route is a full replacement, so the current card is - fetched first to preserve fields that are not being changed. The - internal ``/apps/deck/cards/{cardId}`` route is used (rather than the - board/stack-scoped API route) because the latter reads ``stackId`` from - the URL, which would override the target stack in the body — the same - parameter conflict that affects reorder (issue #469). The internal - route derives ``owner`` from the session user server-side. + The internal ``/apps/deck/cards/{cardId}`` route is used: it reads the + target ``stackId`` from the body (the board/stack-scoped API route + instead binds ``stackId`` from the URL — issue #469 — and 404s for a + card that isn't already on that board). The controller derives ``owner`` + from the session user and does not accept a ``done`` value, so a "done" + card is re-marked done after the move; Deck stamps the current time + there, so the original done timestamp is not preserved (a limitation of + what this route exposes). """ + # Validate the destination so target_board_id is load-bearing: the move + # itself is driven by stackId, so without this a stack on another board + # would relocate the card while the reported board is wrong. + target_stack_ids = { + stack.id for stack in await self.get_stacks(target_board_id) + } + if target_stack_id not in target_stack_ids: + raise ValueError( + f"target_stack_id {target_stack_id} is not a stack on target " + f"board {target_board_id}." + ) + current = await self.get_card(source_board_id, source_stack_id, card_id) - json_data: Dict[str, Any] = { + json_data: dict[str, Any] = { # The route placeholder is {cardId} but the controller reads the # card id from the body, so it must be sent explicitly. "id": card_id, @@ -463,7 +479,17 @@ class DeckClient(BaseNextcloudClient): json=json_data, headers=headers, ) - return DeckCard(**response.json()) + moved = DeckCard(**response.json()) + + # This route clears `done`; restore the done *state* if the card had it + # (the timestamp is refreshed to now — see the docstring note). + if current.done is not None: + await self._make_request( + "PUT", f"/apps/deck/cards/{card_id}/done", headers=headers + ) + moved = await self.get_card(target_board_id, target_stack_id, card_id) + + return moved # Labels async def get_label(self, board_id: int, label_id: int) -> DeckLabel: diff --git a/nextcloud_mcp_server/server/deck.py b/nextcloud_mcp_server/server/deck.py index daaad483..9455cdcf 100644 --- a/nextcloud_mcp_server/server/deck.py +++ b/nextcloud_mcp_server/server/deck.py @@ -1274,20 +1274,25 @@ def configure_deck_tools(mcp: FastMCP): board-manage permission. Use deck_reorder_card for moves within a single board. - The destination is determined by target_stack_id; target_board_id must - be the board that owns it (it is used to report the resulting location). + target_stack_id must be a stack on target_board_id; the move is + rejected otherwise. Args: source_board_id: The ID of the board the card currently lives on source_stack_id: The ID of the stack the card currently lives in card_id: The ID of the card to move - target_board_id: The ID of the destination board (must own target_stack_id) - target_stack_id: The ID of the destination stack + target_board_id: The ID of the destination board + target_stack_id: The ID of the destination stack (must be on target_board_id) order: Position within the destination stack (default 0 = top) """ client = await get_client(ctx) await client.deck.move_card_to_board( - source_board_id, source_stack_id, card_id, target_stack_id, order + source_board_id, + source_stack_id, + card_id, + target_board_id, + target_stack_id, + order, ) return CardOperationResponse( success=True, diff --git a/tests/client/deck/test_deck_move_card_api.py b/tests/client/deck/test_deck_move_card_api.py index 79f609eb..172047ec 100644 --- a/tests/client/deck/test_deck_move_card_api.py +++ b/tests/client/deck/test_deck_move_card_api.py @@ -1,11 +1,12 @@ """Unit tests for DeckClient.move_card_to_board and the reorder same-board guard. These mock the HTTP layer to assert request construction without a live server: -- move_card_to_board must send the card ``id`` and target ``stackId`` in the - body (the route placeholder is ``{cardId}`` but the controller reads ``id`` - from the body), and preserve due/deleted state. +- move_card_to_board must validate the destination board, then PUT to the + internal card route with the card id and target stackId in the body (a + cross-board move the board/stack-scoped route can't do), restoring done state + afterwards since that route clears it. - reorder_card must reject a target stack that is not on the given board before - issuing the reorder request, steering callers to move_card_to_board. + issuing the reorder request, while skipping the lookup for same-stack reorders. """ import httpx @@ -21,24 +22,31 @@ from tests.client.conftest import ( pytestmark = pytest.mark.unit -def _stacks_list_response(stacks: list[dict]) -> httpx.Response: +def _stacks_list_response(stack_ids: list[int]) -> httpx.Response: """Mock response for get_stacks (a JSON array of stack objects).""" - return create_mock_response(status_code=200, json_data=stacks) + return create_mock_response( + status_code=200, + json_data=[ + {"id": sid, "title": f"S{sid}", "boardId": 1, "order": i, "deletedAt": 0} + for i, sid in enumerate(stack_ids) + ], + ) async def test_move_card_to_board_sends_id_and_target_stack(mocker): - """The PUT body carries the card id and the destination stack id.""" - get_card_response = create_mock_deck_card_response( - card_id=42, title="Movable", stack_id=10, description="keep me" - ) - put_response = create_mock_deck_card_response( - card_id=42, title="Movable", stack_id=99, description="keep me" - ) - + """The PUT lands on the internal card route with id + target stack in body.""" mock_make_request = mocker.patch.object( DeckClient, "_make_request", - side_effect=[get_card_response, put_response], + side_effect=[ + _stacks_list_response([99]), # destination validation + create_mock_deck_card_response( # get_card (source) + card_id=42, title="Movable", stack_id=10, description="keep me" + ), + create_mock_deck_card_response( # move PUT + card_id=42, title="Movable", stack_id=99, description="keep me" + ), + ], ) client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") @@ -46,14 +54,14 @@ async def test_move_card_to_board_sends_id_and_target_stack(mocker): source_board_id=1, source_stack_id=10, card_id=42, + target_board_id=2, target_stack_id=99, ) assert isinstance(moved, DeckCard) assert moved.stackId == 99 - # Second call is the update PUT to the internal card route - put_call = mock_make_request.call_args_list[1] + put_call = mock_make_request.call_args_list[2] method, url = put_call.args[0], put_call.args[1] body = put_call.kwargs["json"] assert method == "PUT" @@ -62,22 +70,24 @@ async def test_move_card_to_board_sends_id_and_target_stack(mocker): assert body["stackId"] == 99 assert body["title"] == "Movable" assert body["description"] == "keep me" - # No live due/deleted state on the source card assert body["duedate"] is None assert body["deletedAt"] == 0 + # Not-done card: no follow-up done call + assert mock_make_request.call_count == 3 async def test_move_card_to_board_preserves_duedate(mocker): """A due date on the source card is forwarded as an ISO-8601 string.""" - get_card_response = create_mock_deck_card_response( - card_id=7, stack_id=10, duedate="2030-01-02T03:04:05+00:00" - ) - put_response = create_mock_deck_card_response(card_id=7, stack_id=99) - mock_make_request = mocker.patch.object( DeckClient, "_make_request", - side_effect=[get_card_response, put_response], + side_effect=[ + _stacks_list_response([99]), + create_mock_deck_card_response( + card_id=7, stack_id=10, duedate="2030-01-02T03:04:05+00:00" + ), + create_mock_deck_card_response(card_id=7, stack_id=99), + ], ) client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") @@ -85,25 +95,75 @@ async def test_move_card_to_board_preserves_duedate(mocker): source_board_id=1, source_stack_id=10, card_id=7, + target_board_id=2, target_stack_id=99, ) - body = mock_make_request.call_args_list[1].kwargs["json"] + body = mock_make_request.call_args_list[2].kwargs["json"] assert body["duedate"] == "2030-01-02T03:04:05+00:00" +async def test_move_card_to_board_restores_done_state(mocker): + """A done card triggers a follow-up PUT to the done endpoint after the move.""" + mock_make_request = mocker.patch.object( + DeckClient, + "_make_request", + side_effect=[ + _stacks_list_response([99]), + create_mock_deck_card_response( # source card is done + card_id=8, stack_id=10, done="2029-12-31T23:59:00+00:00" + ), + create_mock_deck_card_response(card_id=8, stack_id=99, done=None), + create_mock_deck_card_response(card_id=8, stack_id=99), # done PUT + create_mock_deck_card_response( # re-fetch after restore + card_id=8, stack_id=99, done="2031-01-01T00:00:00+00:00" + ), + ], + ) + + client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.move_card_to_board( + source_board_id=1, + source_stack_id=10, + card_id=8, + target_board_id=2, + target_stack_id=99, + ) + + # 4th call restores done on the moved card + done_call = mock_make_request.call_args_list[3] + assert done_call.args[0] == "PUT" + assert done_call.args[1] == "/apps/deck/cards/8/done" + + +async def test_move_card_to_board_rejects_stack_not_on_target_board(mocker): + """A target stack absent from the target board is rejected before any move.""" + mock_make_request = mocker.patch.object( + DeckClient, + "_make_request", + return_value=_stacks_list_response([50, 51]), # 99 not present + ) + + client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + with pytest.raises(ValueError, match="not a stack on target board"): + await client.move_card_to_board( + source_board_id=1, + source_stack_id=10, + card_id=42, + target_board_id=2, + target_stack_id=99, + ) + + # Only the destination validation ran; no get_card / move PUT + mock_make_request.assert_called_once() + + async def test_reorder_card_rejects_cross_board_target(mocker): """A target stack absent from the board is rejected before any reorder PUT.""" - # get_stacks returns stacks 10 and 11 — target 99 is on another board mock_make_request = mocker.patch.object( DeckClient, "_make_request", - return_value=_stacks_list_response( - [ - {"id": 10, "title": "A", "boardId": 1, "order": 1, "deletedAt": 0}, - {"id": 11, "title": "B", "boardId": 1, "order": 2, "deletedAt": 0}, - ] - ), + return_value=_stacks_list_response([10, 11]), # target 99 on another board ) client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") @@ -121,18 +181,37 @@ async def test_reorder_card_rejects_cross_board_target(mocker): assert "/stacks" in mock_make_request.call_args.args[1] +async def test_reorder_card_same_stack_skips_lookup(mocker): + """A same-stack reorder issues only the PUT — no get_stacks round-trip.""" + mock_make_request = mocker.patch.object( + DeckClient, + "_make_request", + return_value=create_mock_response(status_code=200, json_data={}), + ) + + client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + await client.reorder_card( + board_id=1, + stack_id=10, + card_id=42, + order=3, + target_stack_id=10, # same stack — pure reorder + ) + + mock_make_request.assert_called_once() + call = mock_make_request.call_args + assert call.args[0] == "PUT" + assert call.args[1] == "/apps/deck/cards/42/reorder" + assert call.kwargs["json"] == {"order": 3, "stackId": 10} + + async def test_reorder_card_allows_same_board_target(mocker): """A target stack on the same board passes the guard and issues the PUT.""" mock_make_request = mocker.patch.object( DeckClient, "_make_request", side_effect=[ - _stacks_list_response( - [ - {"id": 10, "title": "A", "boardId": 1, "order": 1, "deletedAt": 0}, - {"id": 11, "title": "B", "boardId": 1, "order": 2, "deletedAt": 0}, - ] - ), + _stacks_list_response([10, 11]), create_mock_response(status_code=200, json_data={}), ], ) diff --git a/tests/integration/test_deck_move_card_to_board.py b/tests/integration/test_deck_move_card_to_board.py index 8469a0ea..a2e85c55 100644 --- a/tests/integration/test_deck_move_card_to_board.py +++ b/tests/integration/test_deck_move_card_to_board.py @@ -75,6 +75,7 @@ async def test_move_card_to_board_preserves_identity( source_board_id=source_board_id, source_stack_id=source_stack_id, card_id=card.id, + target_board_id=target_board_id, target_stack_id=target_stack_id, ) @@ -105,7 +106,8 @@ async def test_move_card_to_board_remaps_labels( two_boards_with_stacks ) - # Pick a default label that exists on both boards by title + # "Finished" is one of the four labels Deck auto-creates on every new + # board, so it is reliably present on both the source and target boards. source_board = await nc_client.deck.get_board(source_board_id) target_board = await nc_client.deck.get_board(target_board_id) source_label = next( @@ -131,6 +133,7 @@ async def test_move_card_to_board_remaps_labels( source_board_id=source_board_id, source_stack_id=source_stack_id, card_id=card.id, + target_board_id=target_board_id, target_stack_id=target_stack_id, ) @@ -148,6 +151,69 @@ async def test_move_card_to_board_remaps_labels( assert all(label.id != source_label.id for label in finished) +async def test_move_card_to_board_preserves_done_status( + nc_client: NextcloudClient, two_boards_with_stacks: tuple +): + """A card marked done keeps its done timestamp across a cross-board move.""" + source_board_id, source_stack_id, target_board_id, target_stack_id = ( + two_boards_with_stacks + ) + + suffix = uuid.uuid4().hex[:8] + card = await nc_client.deck.create_card( + source_board_id, source_stack_id, f"Done card {suffix}" + ) + done_ts = "2030-01-02T03:04:05+00:00" + await nc_client.deck.update_card( + board_id=source_board_id, + stack_id=source_stack_id, + card_id=card.id, + done=done_ts, + ) + before = await nc_client.deck.get_card(source_board_id, source_stack_id, card.id) + assert before.done is not None, "Card should be done before the move" + + await nc_client.deck.move_card_to_board( + source_board_id=source_board_id, + source_stack_id=source_stack_id, + card_id=card.id, + target_board_id=target_board_id, + target_stack_id=target_stack_id, + ) + + after = await nc_client.deck.get_card(target_board_id, target_stack_id, card.id) + assert after.done is not None, "done status was cleared during the move" + + +async def test_move_card_to_board_rejects_stack_not_on_target_board( + nc_client: NextcloudClient, two_boards_with_stacks: tuple +): + """The move is rejected when target_stack_id is not on target_board_id.""" + source_board_id, source_stack_id, target_board_id, _target_stack_id = ( + two_boards_with_stacks + ) + + suffix = uuid.uuid4().hex[:8] + card = await nc_client.deck.create_card( + source_board_id, source_stack_id, f"Mismatch {suffix}" + ) + + with pytest.raises(ValueError, match="not a stack on target board"): + await nc_client.deck.move_card_to_board( + source_board_id=source_board_id, + source_stack_id=source_stack_id, + card_id=card.id, + target_board_id=target_board_id, + target_stack_id=source_stack_id, # on the source board, not target + ) + + # The card stayed put + still_there = await nc_client.deck.get_card( + source_board_id, source_stack_id, card.id + ) + assert still_there.stackId == source_stack_id + + async def test_reorder_card_rejects_cross_board_target( nc_client: NextcloudClient, two_boards_with_stacks: tuple ):