diff --git a/nextcloud_mcp_server/client/deck.py b/nextcloud_mcp_server/client/deck.py index c17bbe79..b2a23611 100644 --- a/nextcloud_mcp_server/client/deck.py +++ b/nextcloud_mcp_server/client/deck.py @@ -1,5 +1,8 @@ +import logging from typing import Any, Dict, List, Optional +from httpx import HTTPStatusError, RequestError + from nextcloud_mcp_server.client.base import BaseNextcloudClient from nextcloud_mcp_server.models.deck import ( DeckACL, @@ -13,6 +16,8 @@ from nextcloud_mcp_server.models.deck import ( DeckStack, ) +logger = logging.getLogger(__name__) + class DeckClient(BaseNextcloudClient): """Client for Nextcloud Deck app operations.""" @@ -443,6 +448,11 @@ class DeckClient(BaseNextcloudClient): 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). + + The done re-mark is best-effort: the move itself has already committed + by then, so if that follow-up fails the card is left on the target + board without its done state (logged as a warning) rather than raising + and implying the move failed. """ # 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 @@ -467,7 +477,9 @@ class DeckClient(BaseNextcloudClient): "stackId": target_stack_id, "order": order, "description": current.description or "", - # ISO string preserves the due date; None leaves it cleared + # Sent explicitly as None when absent (update_card omits the key); + # both are equivalent here — the route reads duedate and there's + # nothing to clear on a card that never had one. "duedate": current.duedate.isoformat() if current.duedate else None, # 0 keeps the card live (a positive value would soft-delete it) "deletedAt": current.deletedAt or 0, @@ -482,12 +494,24 @@ class DeckClient(BaseNextcloudClient): 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). + # (the timestamp is refreshed to now — see the docstring note). The move + # has already committed, so this is best-effort: on failure, warn with + # the card's new location rather than raising as if the move failed. 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) + try: + 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) + except (HTTPStatusError, RequestError) as e: + logger.warning( + "Card %s moved to board %s stack %s but restoring done " + "state failed: %s", + card_id, + target_board_id, + target_stack_id, + e, + ) return moved diff --git a/tests/client/deck/test_deck_move_card_api.py b/tests/client/deck/test_deck_move_card_api.py index 32c2cb73..56da76ba 100644 --- a/tests/client/deck/test_deck_move_card_api.py +++ b/tests/client/deck/test_deck_move_card_api.py @@ -138,6 +138,43 @@ async def test_move_card_to_board_restores_done_state(mocker): assert result.done is not None +async def test_move_card_to_board_done_restore_failure_is_swallowed(mocker): + """If the post-move done PUT fails, the move still succeeds (best-effort).""" + 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), # move + httpx.HTTPStatusError( # done PUT fails + "500 Server Error", + request=httpx.Request("PUT", "http://test.local"), + response=create_mock_response(status_code=500, json_data={}), + ), + ], + ) + + client = DeckClient(mocker.AsyncMock(spec=httpx.AsyncClient), "testuser") + # Does not raise — the move already committed + result = 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, + ) + + # Returns the moved card from the (successful) move PUT, done unrestored + assert result.stackId == 99 + assert result.done is None + # The done restore was attempted (and failed), with no re-fetch after it + assert mock_make_request.call_count == 4 + assert mock_make_request.call_args_list[3].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( diff --git a/tests/integration/test_deck_move_card_to_board.py b/tests/integration/test_deck_move_card_to_board.py index 6ea524eb..334b3e57 100644 --- a/tests/integration/test_deck_move_card_to_board.py +++ b/tests/integration/test_deck_move_card_to_board.py @@ -212,6 +212,46 @@ async def test_move_card_to_board_preserves_archived_status( assert after.archived is True, "archived state was lost during the move" +async def test_move_card_to_board_preserves_done_and_archived( + nc_client: NextcloudClient, two_boards_with_stacks: tuple +): + """A done *and* archived card keeps both states across a cross-board move. + + This exercises the done-restore path (post-move done PUT + re-fetch) on an + archived card, confirming get_card on the destination handles it. + """ + 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+archived {suffix}" + ) + await nc_client.deck.update_card( + board_id=source_board_id, + stack_id=source_stack_id, + card_id=card.id, + done="2030-01-02T03:04:05+00:00", + ) + await nc_client.deck.archive_card(source_board_id, source_stack_id, card.id) + + moved = 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, + ) + assert moved.done is not None + assert moved.archived is True + + after = await nc_client.deck.get_card(target_board_id, target_stack_id, card.id) + assert after.stackId == target_stack_id + assert after.done is not None, "done state was lost during the move" + assert after.archived is True, "archived state was lost during the move" + + async def test_move_card_to_board_rejects_stack_not_on_target_board( nc_client: NextcloudClient, two_boards_with_stacks: tuple ):