fix: DeckClient.update_card partial update bugs

[cbcoutinho]

Summary

Fixes partial update bugs in DeckClient.update_card where:

1. Updating without explicit title caused 400 errors
2. Updating without explicit description cleared it

Fixes: #452

Root Cause

The Deck PUT API is a full replacement, not a partial update. Fields omitted from the payload are either rejected (required fields) or cleared (optional fields).

Solution

Always include title, type, owner, and description in the payload, using current card values when not explicitly provided. This matches the existing pattern that was already used for type and owner.

Bonus: Simplified owner extraction logic since DeckCard.validate_owner already ensures it's always a string.

Test Results

All 8 integration tests pass:

tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_title_only_preserves_description PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_description_only PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_title_and_description PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_duedate_only PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_archived_only PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_order_only PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_preserves_type PASSED
tests/client/deck/test_deck_update_card_api.py::TestDeckClientUpdateCard::test_update_preserves_owner PASSED

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

PR Review: DeckClient.update_card Integration Tests

Thank you for this comprehensive PR documenting the bugs in DeckClient.update_card. The investigation and test coverage are excellent. Here's my detailed feedback:

---

🎯 Strengths

Excellent Documentation

• Clear problem statement: The PR description and test docstrings clearly explain the root cause (PUT is full replacement, not partial update)
• Comprehensive test matrix: All edge cases are covered with expected vs actual behavior documented
• Related issues linked: Good context with deck#3127 and deck#4106 references

High-Quality Test Suite

• Follows repository conventions: Uses integration test fixtures (nc_client, deck_test_card) per CLAUDE.md guidelines
• Proper cleanup: Fixture ensures board deletion even on test failure
• Clear test names: Each test clearly states the scenario and expected behavior
• Good use of pytest.raises: Properly validates 400 errors with status code assertions

Strong Investigation

• The investigation comment in issue DeckClient.update_card always sets owner/type for card update – Deck API rejects this (400 Bad Request) #452 shows thorough API testing and corrects the original issue report based on empirical evidence

---

🐛 Issues & Recommendations

1. Critical: Tests Document Bugs But Don't Verify Fixes

Problem: This PR adds tests that verify current buggy behavior, but doesn't include the fix itself. The tests will pass in their current form, which doesn't help prevent regression.

Recommendation:

• Either mark these tests with @pytest.mark.xfail(reason="Bug #452: ...", strict=True) to document that they currently fail
• Or include the fix in this PR and update tests to verify correct behavior

Example:

@pytest.mark.xfail(reason="Bug #452: description cleared instead of preserved", strict=True)
async def test_update_title_only_clears_description(self, nc_client, deck_test_card):
    # ...
    assert updated.description == "Original description"  # This should pass after fix

2. Test Assertion Issue

In test_update_title_only_clears_description (line 79):

# BUG: Description was cleared instead of preserved
assert updated.description == ""  # Should be "Original description"

Problem: This assertion verifies the bug, not the desired behavior. When the bug is fixed, this test will fail.

Recommendation: Either:

• Use @pytest.mark.xfail as mentioned above, OR
• Change assertion to: assert updated.description == "Original description" and expect it to fail until fixed

3. Missing Test Case: Update Description with Title

While you test "title only" and "title + description", consider adding:

• Test: Update description while preserving original title (provide both in call)
• Purpose: Verify that explicitly providing title prevents the 400 error

async def test_update_description_with_title(self, nc_client, deck_test_card):
    """Updating description when title is also provided should work."""
    await nc_client.deck.update_card(
        board_id=deck_test_card["board_id"],
        stack_id=deck_test_card["stack_id"],
        card_id=deck_test_card["card_id"],
        title="Original Title",  # Explicitly preserve
        description="New description",
    )
    
    updated = await nc_client.deck.get_card(...)
    assert updated.title == "Original Title"
    assert updated.description == "New description"

4. Proposed Fix Approach

Based on your investigation and the current implementation in deck.py:274-324, here's the recommended fix:

Current behavior (lines 292-293):

if title is not None:
    json_data["title"] = title

Fixed behavior:

# Title is required by Deck API, always include it
json_data["title"] = title if title is not None else current_card.title

# Description should be preserved when not explicitly provided
json_data["description"] = description if description is not None else (
    current_card.description or ""
)

Rationale:

1. Matches existing pattern for type and owner (lines 297-309)
2. Preserves partial update semantics from user's perspective
3. Handles PUT full replacement requirement at implementation layer
4. description or "" handles None from API gracefully

5. Code Quality

Type Hints: Following CLAUDE.md conventions:

• Line 1: Should use Python 3.10+ syntax: from typing import Any is unnecessary for basic types
• Consider: dict[str, Any] instead of Dict[str, Any] (though not used in this PR)

Async Best Practices: ✅ All async patterns follow repository conventions (anyio compatible)

6. Performance Consideration

update_card already calls get_card (line 289 in deck.py) for type and owner. After the fix, this will also provide title and description, so no additional API calls needed. 👍

---

🔒 Security

No security concerns identified. Standard CRUD operations with proper authentication handled by BaseNextcloudClient.

---

📊 Test Coverage

Current: Tests cover all parameter combinations (title, description, duedate, archived, order, type, owner)

Suggested additions:

1. Test updating multiple fields simultaneously (e.g., title + duedate + archived)
2. Test that None vs not provided behaves identically after fix
3. Edge case: Empty string "" vs None for description

---

📝 Next Steps

Option A: Keep as Documentation PR (Current State)

1. Add @pytest.mark.xfail decorators to failing tests
2. Update assertions to verify correct behavior
3. Create separate PR for the fix
4. Remove xfail when fix merges

Option B: Convert to Full Fix PR (Recommended)

1. Implement fix in nextcloud_mcp_server/client/deck.py
2. Update test assertions to verify correct behavior
3. Run full test suite: uv run pytest tests/client/deck/ -v
4. Add unit test in test_deck_api.py verifying payload construction
5. Update PR title/description to reflect fix included

---

✅ Approval Recommendation

Conditional approval pending:

• Decision on Option A vs Option B approach
• Fix test assertions or add @pytest.mark.xfail
• Consider adding suggested test cases

The investigation quality and test coverage are excellent. Once the test assertion strategy is clarified, this will be a solid contribution! 🎉

---

References

• CLAUDE.md testing conventions: ✅ Followed
• Integration test patterns: ✅ Properly used fixtures
• Async patterns: ✅ Uses anyio-compatible async/await
• Related upstream issues: deck#3127, deck#4106
• Repository issue: DeckClient.update_card always sets owner/type for card update – Deck API rejects this (400 Bad Request) #452

Great work documenting this API quirk! The Deck API's undocumented full-replacement behavior is now well-tested.

[claude]

Code Review: DeckClient.update_card Integration Tests

Thank you for this PR! The integration tests are well-structured and clearly document the expected behavior. Here's my detailed feedback:

✅ Strengths

1. Excellent test documentation: The docstrings and module header clearly explain the expected behavior and link to relevant issues
2. Comprehensive coverage: Tests cover all major partial update scenarios (title, description, duedate, archived, order, type, owner)
3. Good fixture design: The deck_test_card fixture follows the repository's patterns with proper cleanup
4. Correct test markers: Uses pytest.mark.integration appropriately
5. Clear expected behavior: Tests document what SHOULD happen (partial updates preserve other fields)

🔍 Issues Identified

1. Critical Bug in Current Implementation

Looking at nextcloud_mcp_server/client/deck.py:274-324, the current update_card implementation has TWO bugs:

Bug #1: Lines 292-293 conditionally include title only if provided:

if title is not None:
    json_data["title"] = title

But the Deck API requires title for PUT requests. This causes 400 errors when updating other fields without providing title.

Bug #2: Lines 294-295 conditionally include description only if provided:

if description is not None:
    json_data["description"] = description

Since PUT is a full replacement, omitting description causes it to be cleared by the API.

The current implementation already fetches current_card (line 289) and preserves type and owner (lines 297-309), but NOT title or description.

2. Inconsistency with Issue #452

Issue #452 claims that type and owner should NOT be sent to the API. However, the current code (lines 297-309) sends them unconditionally:

json_data["type"] = type if type is not None else current_card.type
json_data["owner"] = ...  # Complex logic to extract owner

Question: Does the Deck API actually reject type/owner in PUT requests? The tests assume they should be preserved, which requires sending them. This needs clarification from API behavior testing.

3. Missing title and description Preservation

To make these tests pass, the fix should:

async def update_card(
    self,
    board_id: int,
    stack_id: int,
    card_id: int,
    title: str | None = None,
    description: str | None = None,
    type: str | None = None,
    owner: str | None = None,
    order: int | None = None,
    duedate: str | None = None,
    archived: bool | None = None,
    done: str | None = None,
) -> None:
    # Get current card to preserve unspecified fields
    current_card = await self.get_card(board_id, stack_id, card_id)

    json_data = {
        # Always include required fields, use current values if not provided
        "title": title if title is not None else current_card.title,
        "description": description if description is not None else (current_card.description or ""),
        "type": type if type is not None else current_card.type,
        "owner": owner if owner is not None else _extract_owner(current_card.owner),
    }
    
    # Include optional fields only if explicitly provided
    if order is not None:
        json_data["order"] = order
    if duedate is not None:
        json_data["duedate"] = duedate
    if archived is not None:
        json_data["archived"] = archived
    if done is not None:
        json_data["done"] = done
    
    # ... rest of implementation

📋 Recommendations

High Priority

1. Verify API requirements: Test against actual Deck API to confirm:

   • Which fields are required vs optional for PUT /cards/{cardId}
   • Whether type/owner are accepted/rejected (conflicts between issue DeckClient.update_card always sets owner/type for card update – Deck API rejects this (400 Bad Request) #452 and current implementation)
   • How the API handles missing description (empty string vs null vs omitted)

2. Add test for empty description: Test updating a card with description="" to ensure it's preserved:

   async def test_update_preserves_empty_description(self, nc_client, deck_test_card):
       """Empty description should be preserved when not explicitly changed."""
       # First clear description
       await nc_client.deck.update_card(..., description="")
       # Then update something else
       await nc_client.deck.update_card(..., title="New Title")
       updated = await nc_client.deck.get_card(...)
       assert updated.description == ""

3. Fix implementation: Update deck.py:274-324 to preserve title and description using current card values when not provided

Medium Priority

4. Type hints: Update function signature to use Python 3.10+ syntax per CLAUDE.md:

   title: str | None = None  # instead of Optional[str]

5. Test edge cases: Consider adding tests for:

   • Updating a card with duedate=None to clear duedate
   • Updating with archived=False to unarchive
   • Concurrent updates (though this may be out of scope)

6. Documentation: Add a comment in the implementation explaining why we fetch current_card first (Deck PUT is full replacement, not PATCH)

🎯 Next Steps

Before implementing the fix:

1. Run the tests to confirm they fail as expected: uv run pytest tests/client/deck/test_deck_update_card_api.py -v
2. Verify API behavior by testing against a real Nextcloud Deck instance
3. Decide on the fix strategy based on API requirements (Option 1 from PR description seems correct)
4. Update the implementation in deck.py
5. Run tests again to ensure they pass
6. Add unit tests in test_deck_api.py using mocked responses to complement integration tests

📊 Summary

Aspect	Rating	Notes
Test Quality	⭐⭐⭐⭐⭐	Comprehensive, well-documented
Test Coverage	⭐⭐⭐⭐	Covers main scenarios, could add edge cases
Follows Conventions	⭐⭐⭐⭐⭐	Matches CLAUDE.md patterns perfectly
Implementation Fix	⏳ Pending	Needs implementation to match tests
API Verification	⏳ Required	Must confirm API requirements

Overall: Excellent test design that clearly documents the bug and expected behavior. Once API requirements are verified and the implementation is fixed, this will be a valuable contribution! 🚀

[claude]

Code Review for PR #453

Thank you for this comprehensive fix to the DeckClient.update_card partial update issue! This PR demonstrates excellent software engineering practices. Here's my detailed review:

---

✅ Strengths

1. Excellent Test-Driven Approach

The PR follows TDD principles beautifully:

• Commit 1: Documents current buggy behavior with failing tests
• Commit 2: Refactors tests to define expected behavior (specification)
• Commit 3: Implements the fix to make tests pass

This progression is pedagogically valuable and makes the problem/solution crystal clear.

2. Comprehensive Test Coverage

The test suite in test_deck_update_card_api.py is thorough:

• ✅ Tests all partial update scenarios (title-only, description-only, combined, etc.)
• ✅ Tests preservation of fields not being updated
• ✅ Tests special fields (duedate, archived, order)
• ✅ Uses proper fixtures with cleanup
• ✅ Clear docstrings explaining expected behavior

3. Code Quality Improvements

The implementation fix is clean and well-documented:

# Before: Complex conditional logic with nested ternaries
json_data["owner"] = (
    owner if owner is not None 
    else (current_card.owner if isinstance(current_card.owner, str) 
          else current_card.owner.uid if hasattr(current_card.owner, "uid") 
          else current_card.owner.primaryKey)
)

# After: Simple, leveraging existing model validator
json_data = {
    "owner": owner if owner is not None else current_card.owner,
    # ...
}

The simplified owner handling is possible because DeckCard.validate_owner already ensures owner is always a string (see models/deck.py:102-108).

4. Proper Understanding of PUT Semantics

The fix correctly recognizes that the Deck API's PUT endpoint is a full replacement, not a partial update. The solution (fetch-then-merge) is the standard pattern for implementing partial updates over PUT APIs.

---

🔍 Observations & Suggestions

1. Performance Consideration: Extra GET Request

Current behavior: Every update_card call makes 2 HTTP requests (GET + PUT).

# Line 290
current_card = await self.get_card(board_id, stack_id, card_id)

Trade-off analysis:

• ✅ Correctness: Guarantees fields are preserved
• ⚠️ Performance: Doubles network latency for updates
• ⚠️ Race conditions: If card changes between GET and PUT, concurrent updates could be lost

Alternative approaches (for consideration in future work):

1. Optimistic approach: Make caller responsible for providing all required fields
   • Pros: Single request, explicit contract
   • Cons: Breaking API change, shifts burden to callers
2. ETag-based optimistic locking: Use ETag from GET in PUT to detect conflicts
   • Pros: Prevents lost updates
   • Cons: More complex error handling
3. Upstream fix: Request PATCH support in Deck API (see deck #3127, #4106 mentioned in tests)

Recommendation: Current approach is acceptable given:

• Updates are typically infrequent user-driven actions (not high-volume)
• Correctness > performance for card updates
• Changing to require all fields would break existing code

Consider documenting this trade-off in the method docstring.

2. Potential Bug: Empty Description Handling

Line 303:

"description": description if description is not None else (current_card.description or ""),

Question: Should description=None preserve the current description, or should users be able to explicitly clear it?

Current behavior:

• update_card(description=None) → preserves existing description ✅
• No way to explicitly clear description to empty string

Test gap: No test for "clear description" scenario.

Suggestion: Consider whether callers should be able to clear the description:

# Option 1: Allow explicit empty string to clear
await update_card(description="")  # Clears to empty

# Option 2: Use sentinel value for clarity
from typing import Literal
CLEAR = object()
await update_card(description=CLEAR)  # Explicitly clears

If current behavior is intentional (no way to clear description), document it. Otherwise, add test case:

async def test_update_clears_description(nc_client, deck_test_card):
    """Passing empty string should clear the description."""
    await nc_client.deck.update_card(
        ...,
        description="",  # Explicitly clear
    )
    updated = await nc_client.deck.get_card(...)
    assert updated.description == ""

3. Type Hints: Missing Return Type Annotation

The update_card method returns None, but this could be more explicit:

async def update_card(
    self,
    board_id: int,
    stack_id: int,
    card_id: int,
    # ... parameters
) -> None:  # ✅ Already correct

Actually, reviewing again - this is already correct! Good job following CLAUDE.md conventions.

4. Test Fixture: Consider Parameterization

Many tests follow the same pattern (update field, verify preservation). Consider using pytest.mark.parametrize for DRY-er tests:

@pytest.mark.parametrize("field,value,getter", [
    ("title", "New Title", lambda c: c.title),
    ("description", "New Desc", lambda c: c.description),
    ("order", 99, lambda c: c.order),
    ("archived", True, lambda c: c.archived),
])
async def test_update_single_field_preserves_others(
    nc_client, deck_test_card, field, value, getter
):
    await nc_client.deck.update_card(
        deck_test_card["board_id"],
        deck_test_card["stack_id"],
        deck_test_card["card_id"],
        **{field: value}
    )
    updated = await nc_client.deck.get_card(...)
    assert getter(updated) == value
    assert updated.title == "Original Title"  # etc.

However, the current verbose approach is also valid for:

• Better error messages (know exactly which scenario failed)
• Easier debugging
• Clear documentation of each case

Your call on this one!

---

🔒 Security Review

✅ No security concerns identified:

• No user input sanitization issues (IDs are integers, strings passed to API)
• No SQL injection risk (using HTTP API, not direct DB)
• No XSS vectors (server-side only)
• Proper use of async/await prevents blocking

---

📋 Checklist Against CLAUDE.md

Requirement	Status	Notes
async/await with anyio	✅	Uses standard async/await
Type hints (Python 3.10+ syntax)	✅	str | None syntax, lowercase generics
Pydantic models for responses	✅	DeckCard model used
Integration test fixtures	✅	Uses nc_client, proper cleanup
Test markers	✅	pytest.mark.integration
Docstrings	✅	Clear test and method documentation
Comments only where needed	✅	Inline comments explain non-obvious behavior
No over-engineering	✅	Minimal, focused fix

---

🧪 Pre-Merge Checklist

Before merging, please verify:

1. Tests pass:

   # Rebuild container after code changes (per CLAUDE.md)
   docker-compose up --build -d mcp
   
   # Run the new integration tests
   uv run pytest tests/client/deck/test_deck_update_card_api.py -v
   
   # Run existing Deck tests to ensure no regressions
   uv run pytest tests/client/deck/ -v

2. Code quality:

   uv run ruff check
   uv run ruff format
   uv run ty check -- nextcloud_mcp_server

3. Integration test (if you have access to a Deck-enabled Nextcloud instance):

   uv run pytest -m "integration and not oauth" -v

---

🎯 Recommendation

APPROVE with minor suggestions

This is high-quality work that:

• Fixes a real bug (DeckClient.update_card always sets owner/type for card update – Deck API rejects this (400 Bad Request) #452)
• Adds comprehensive test coverage
• Improves code readability
• Follows project conventions

Suggested follow-up (not blocking):

1. Add docstring to update_card method documenting the fetch-then-merge behavior
2. Consider adding test for clearing description (if intended to be supported)
3. Monitor performance in production; optimize if update latency becomes an issue

---

Great work on this fix! The TDD approach and thorough testing are exemplary. 🎉