fix(deck): address PR #759 review feedback

- Validate description_max_length is positive (raises ValueError on 0
  or negative); the prior code would have wiped descriptions to a
  single ellipsis character on description_max_length=0.
- Extract filter logic into testable module-level helpers
  (_apply_board_filters, _apply_stack_filters, _apply_card_filters)
  and replace the dense `continue`-based loop in deck_get_stacks with
  the reviewer's elif form.
- Document the truncation length quirk in the helper docstring (result
  is description_max_length + 1 chars when truncation fires).
- Add 13 new unit tests covering the include/exclude flags on board,
  stacks, and flat card lists, plus the new validation paths and an
  explicit "description fits within limit, no ellipsis" case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/server/deck.py

@@ -34,14 +34,81 @@ logger = logging.getLogger(__name__)
 def _truncate_card_descriptions(
     cards: list[DeckCard], description_max_length: int | None
 ) -> None:
-    """Truncate each card's description in-place when it exceeds the limit."""
+    """Truncate each card's description in-place when it strictly exceeds the
+    limit.
+
+    Descriptions whose length is less than or equal to ``description_max_length``
+    are left untouched (no ellipsis is appended). Descriptions longer than the
+    limit are truncated to ``description_max_length`` characters and an
+    ellipsis ("…") is appended, so the resulting string is
+    ``description_max_length + 1`` characters total.
+
+    Args:
+        cards: Cards to mutate in place.
+        description_max_length: Positive truncation threshold, or ``None`` to
+            skip truncation entirely.
+
+    Raises:
+        ValueError: If ``description_max_length`` is not positive.
+    """
     if description_max_length is None:
         return
+    if description_max_length <= 0:
+        raise ValueError(
+            f"description_max_length must be positive, got {description_max_length}"
+        )
     for card in cards:
         if card.description and len(card.description) > description_max_length:
             card.description = card.description[:description_max_length] + "…"
 
 
+def _apply_board_filters(
+    board: DeckBoard,
+    *,
+    include_acl: bool,
+    include_users: bool,
+    include_labels: bool,
+) -> DeckBoard:
+    """Drop board sub-fields the caller didn't request (in-place)."""
+    if not include_acl:
+        board.acl = []
+    if not include_users:
+        board.users = []
+    if not include_labels:
+        board.labels = []
+    return board
+
+
+def _apply_stack_filters(
+    stack: DeckStack,
+    *,
+    include_cards: bool,
+    include_archived_cards: bool,
+    description_max_length: int | None,
+) -> DeckStack:
+    """Apply card-shaping filters to a single stack (in-place)."""
+    if not include_cards:
+        stack.cards = None
+    elif stack.cards:
+        if not include_archived_cards:
+            stack.cards = [c for c in stack.cards if not c.archived]
+        _truncate_card_descriptions(stack.cards, description_max_length)
+    return stack
+
+
+def _apply_card_filters(
+    cards: list[DeckCard],
+    *,
+    include_archived: bool,
+    description_max_length: int | None,
+) -> list[DeckCard]:
+    """Apply filters to a flat list of cards. Returns a (possibly new) list."""
+    if not include_archived:
+        cards = [c for c in cards if not c.archived]
+    _truncate_card_descriptions(cards, description_max_length)
+    return cards
+
+
 def configure_deck_tools(mcp: FastMCP):
     """Configure Nextcloud Deck tools and resources for the MCP server."""
 
@@ -175,13 +242,12 @@ def configure_deck_tools(mcp: FastMCP):
         """
         client = await get_client(ctx)
         board = await client.deck.get_board(board_id)
-        if not include_acl:
+        return _apply_board_filters(
-            board.acl = []
+            board,
-        if not include_users:
+            include_acl=include_acl,
-            board.users = []
+            include_users=include_users,
-        if not include_labels:
+            include_labels=include_labels,
-            board.labels = []
+        )
-        return board
 
     @mcp.tool(
         title="List Deck Stacks",
@@ -212,13 +278,12 @@ def configure_deck_tools(mcp: FastMCP):
         client = await get_client(ctx)
         stacks = await client.deck.get_stacks(board_id)
         for stack in stacks:
-            if not include_cards:
+            _apply_stack_filters(
-                stack.cards = None
+                stack,
-                continue
+                include_cards=include_cards,
-            if stack.cards:
+                include_archived_cards=include_archived_cards,
-                if not include_archived_cards:
+                description_max_length=description_max_length,
-                    stack.cards = [c for c in stack.cards if not c.archived]
+            )
-                _truncate_card_descriptions(stack.cards, description_max_length)
         return ListStacksResponse(stacks=stacks, total=len(stacks))
 
     @mcp.tool(
@@ -248,13 +313,12 @@ def configure_deck_tools(mcp: FastMCP):
         """
         client = await get_client(ctx)
         stack = await client.deck.get_stack(board_id, stack_id)
-        if not include_cards:
+        return _apply_stack_filters(
-            stack.cards = None
+            stack,
-        elif stack.cards:
+            include_cards=include_cards,
-            if not include_archived_cards:
+            include_archived_cards=include_archived_cards,
-                stack.cards = [c for c in stack.cards if not c.archived]
+            description_max_length=description_max_length,
-            _truncate_card_descriptions(stack.cards, description_max_length)
+        )
-        return stack
 
     @mcp.tool(
         title="List Archived Deck Stacks",
@@ -312,10 +376,11 @@ def configure_deck_tools(mcp: FastMCP):
         """
         client = await get_client(ctx)
         stack = await client.deck.get_stack(board_id, stack_id)
-        cards = stack.cards or []
+        cards = _apply_card_filters(
-        if not include_archived:
+            stack.cards or [],
-            cards = [c for c in cards if not c.archived]
+            include_archived=include_archived,
-        _truncate_card_descriptions(cards, description_max_length)
+            description_max_length=description_max_length,
+        )
         return ListCardsResponse(cards=cards, total=len(cards))
 
     @mcp.tool(

tests/unit/test_deck_server.py

@@ -1,24 +1,94 @@
 import pytest
 
-from nextcloud_mcp_server.models.deck import DeckCard
+from nextcloud_mcp_server.models.deck import (
-from nextcloud_mcp_server.server.deck import _truncate_card_descriptions
+    DeckACL,
+    DeckBoard,
+    DeckCard,
+    DeckLabel,
+    DeckPermissions,
+    DeckStack,
+    DeckUser,
+)
+from nextcloud_mcp_server.server.deck import (
+    _apply_board_filters,
+    _apply_card_filters,
+    _apply_stack_filters,
+    _truncate_card_descriptions,
+)
 
 pytestmark = pytest.mark.unit
 
 
-def _make_card(card_id: int, description: str | None) -> DeckCard:
+# Fixtures ------------------------------------------------------------------
+
+
+def _make_card(
+    card_id: int,
+    description: str | None = "desc",
+    archived: bool = False,
+) -> DeckCard:
     return DeckCard(
         id=card_id,
         title=f"Card {card_id}",
         stackId=1,
         type="plain",
         order=card_id,
-        archived=False,
+        archived=archived,
         owner="testuser",
         description=description,
     )
 
 
+def _make_user(uid: str = "testuser") -> DeckUser:
+    return DeckUser(primaryKey=uid, uid=uid, displayname=uid)
+
+
+def _make_board(
+    board_id: int = 1,
+    *,
+    labels: list[DeckLabel] | None = None,
+    acl: list[DeckACL] | None = None,
+    users: list[DeckUser] | None = None,
+) -> DeckBoard:
+    return DeckBoard(
+        id=board_id,
+        title=f"Board {board_id}",
+        owner=_make_user(),
+        color="FF0000",
+        archived=False,
+        labels=labels
+        if labels is not None
+        else [DeckLabel(id=1, title="L1", color="00FF00")],
+        acl=acl if acl is not None else [],
+        permissions=DeckPermissions(
+            PERMISSION_READ=True,
+            PERMISSION_EDIT=True,
+            PERMISSION_MANAGE=True,
+            PERMISSION_SHARE=True,
+        ),
+        users=users if users is not None else [_make_user("alice"), _make_user("bob")],
+        deletedAt=0,
+    )
+
+
+def _make_stack(
+    stack_id: int = 1,
+    *,
+    cards: list[DeckCard] | None = None,
+) -> DeckStack:
+    return DeckStack(
+        id=stack_id,
+        title=f"Stack {stack_id}",
+        boardId=1,
+        order=stack_id,
+        deletedAt=0,
+        cards=cards,
+    )
+
+
+# _truncate_card_descriptions ----------------------------------------------
+
+
 def test_truncate_card_descriptions_no_op_when_limit_is_none():
     """When description_max_length is None, descriptions are left untouched."""
     cards = [_make_card(1, "x" * 5000)]
@@ -49,3 +119,206 @@ def test_truncate_card_descriptions_at_exact_boundary():
     cards = [_make_card(1, "x" * 100)]
     _truncate_card_descriptions(cards, 100)
     assert cards[0].description == "x" * 100
+
+
+def test_truncate_card_descriptions_shorter_than_limit_no_ellipsis():
+    """A description shorter than the limit must not have an ellipsis appended."""
+    cards = [_make_card(1, "hello")]
+    _truncate_card_descriptions(cards, 1000)
+    assert cards[0].description == "hello"
+
+
+def test_truncate_card_descriptions_rejects_zero():
+    """A zero limit is invalid (would wipe descriptions to a single ellipsis)."""
+    cards = [_make_card(1, "anything")]
+    with pytest.raises(ValueError, match="must be positive"):
+        _truncate_card_descriptions(cards, 0)
+
+
+def test_truncate_card_descriptions_rejects_negative():
+    """Negative limits are invalid."""
+    cards = [_make_card(1, "anything")]
+    with pytest.raises(ValueError, match="must be positive"):
+        _truncate_card_descriptions(cards, -10)
+
+
+# _apply_board_filters ------------------------------------------------------
+
+
+def test_apply_board_filters_defaults_preserve_fields():
+    """With all include_* flags True, no fields are cleared."""
+    board = _make_board()
+    result = _apply_board_filters(
+        board, include_acl=True, include_users=True, include_labels=True
+    )
+    assert len(result.labels) == 1
+    assert len(result.users) == 2
+
+
+def test_apply_board_filters_excludes_acl():
+    """include_acl=False clears the acl list."""
+    board = _make_board(
+        acl=[
+            DeckACL(
+                id=1,
+                participant=_make_user("alice"),
+                type=0,
+                boardId=1,
+                permissionEdit=True,
+                permissionShare=True,
+                permissionManage=False,
+                owner=False,
+            )
+        ]
+    )
+    result = _apply_board_filters(
+        board, include_acl=False, include_users=True, include_labels=True
+    )
+    assert result.acl == []
+
+
+def test_apply_board_filters_excludes_users():
+    """include_users=False clears the users list."""
+    board = _make_board()
+    result = _apply_board_filters(
+        board, include_acl=True, include_users=False, include_labels=True
+    )
+    assert result.users == []
+
+
+def test_apply_board_filters_excludes_labels():
+    """include_labels=False clears the labels list."""
+    board = _make_board()
+    result = _apply_board_filters(
+        board, include_acl=True, include_users=True, include_labels=False
+    )
+    assert result.labels == []
+
+
+def test_apply_board_filters_excludes_all():
+    """All include_* flags False clears every filterable list."""
+    board = _make_board()
+    result = _apply_board_filters(
+        board, include_acl=False, include_users=False, include_labels=False
+    )
+    assert result.acl == []
+    assert result.users == []
+    assert result.labels == []
+
+
+# _apply_stack_filters ------------------------------------------------------
+
+
+def test_apply_stack_filters_include_cards_false_strips_cards():
+    """include_cards=False sets cards to None regardless of other flags."""
+    stack = _make_stack(cards=[_make_card(1), _make_card(2, archived=True)])
+    result = _apply_stack_filters(
+        stack,
+        include_cards=False,
+        include_archived_cards=True,
+        description_max_length=None,
+    )
+    assert result.cards is None
+
+
+def test_apply_stack_filters_excludes_archived_by_default():
+    """include_archived_cards=False filters out archived cards."""
+    stack = _make_stack(
+        cards=[_make_card(1, archived=False), _make_card(2, archived=True)]
+    )
+    result = _apply_stack_filters(
+        stack,
+        include_cards=True,
+        include_archived_cards=False,
+        description_max_length=None,
+    )
+    assert result.cards is not None
+    assert [c.id for c in result.cards] == [1]
+
+
+def test_apply_stack_filters_keeps_archived_when_requested():
+    """include_archived_cards=True retains archived cards."""
+    stack = _make_stack(
+        cards=[_make_card(1, archived=False), _make_card(2, archived=True)]
+    )
+    result = _apply_stack_filters(
+        stack,
+        include_cards=True,
+        include_archived_cards=True,
+        description_max_length=None,
+    )
+    assert result.cards is not None
+    assert [c.id for c in result.cards] == [1, 2]
+
+
+def test_apply_stack_filters_truncates_descriptions_after_archive_filter():
+    """Truncation runs on the post-archive-filter card set."""
+    stack = _make_stack(
+        cards=[
+            _make_card(1, description="x" * 50, archived=False),
+            _make_card(2, description="y" * 50, archived=True),
+        ]
+    )
+    result = _apply_stack_filters(
+        stack,
+        include_cards=True,
+        include_archived_cards=False,
+        description_max_length=10,
+    )
+    assert result.cards is not None
+    assert len(result.cards) == 1
+    assert result.cards[0].description is not None
+    assert result.cards[0].description.endswith("…")
+
+
+def test_apply_stack_filters_handles_none_cards():
+    """A stack with no cards (cards=None) is left untouched."""
+    stack = _make_stack(cards=None)
+    result = _apply_stack_filters(
+        stack,
+        include_cards=True,
+        include_archived_cards=False,
+        description_max_length=10,
+    )
+    assert result.cards is None
+
+
+# _apply_card_filters -------------------------------------------------------
+
+
+def test_apply_card_filters_excludes_archived_by_default():
+    """include_archived=False filters archived cards out of the flat list."""
+    cards = [
+        _make_card(1, archived=False),
+        _make_card(2, archived=True),
+        _make_card(3, archived=False),
+    ]
+    result = _apply_card_filters(
+        cards, include_archived=False, description_max_length=None
+    )
+    assert [c.id for c in result] == [1, 3]
+
+
+def test_apply_card_filters_keeps_archived_when_requested():
+    """include_archived=True retains archived cards."""
+    cards = [_make_card(1, archived=False), _make_card(2, archived=True)]
+    result = _apply_card_filters(
+        cards, include_archived=True, description_max_length=None
+    )
+    assert [c.id for c in result] == [1, 2]
+
+
+def test_apply_card_filters_truncates_descriptions():
+    """description_max_length is honored on the returned cards."""
+    cards = [_make_card(1, description="x" * 50)]
+    result = _apply_card_filters(
+        cards, include_archived=True, description_max_length=10
+    )
+    assert result[0].description is not None
+    assert result[0].description.endswith("…")
+
+
+def test_apply_card_filters_empty_list_is_noop():
+    """An empty input returns an empty output."""
+    result = _apply_card_filters([], include_archived=False, description_max_length=10)
+    assert result == []