fix: handle categories, recurrence_rule, attendees, and reminder_minutes in update_event

[cbcoutinho]

Summary

• Fix _merge_ical_properties() silently ignoring categories, recurrence_rule, attendees, and reminder_minutes during event updates
• Add support for setting and clearing all four fields, matching patterns already used by _create_ical_event() and _merge_ical_todo_properties()
• Add client-level and MCP-level integration tests verifying both set and clear operations

Closes #544

Test plan

• uv run ruff check — passes
• uv run ruff format --check — passes
• uv run ty check -- nextcloud_mcp_server — passes
• uv run pytest tests/client/calendar/test_calendar_operations.py -v -k "update_event" — 2 tests pass
• uv run pytest tests/server/test_calendar_events_mcp.py -v — 1 test passes

🤖 Generated with Claude Code

---

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

[claude]

Code Review

Great work on this fix! This PR correctly addresses issue #544 by implementing the missing field handlers in _merge_ical_properties(). The implementation is well-structured and follows established patterns. Here's my detailed review:

---

✅ Strengths

1. Correct Pattern Matching: The implementation correctly mirrors the patterns from _create_ical_event() (lines 651-693) and _merge_ical_todo_properties() (lines 1086-1092), ensuring consistency across the codebase.

2. Proper Clear Operation Handling: Each field correctly handles both setting and clearing:

   • Categories: Empty string → delete CATEGORIES property
   • Recurrence: Empty string → delete RRULE property
   • Attendees: Empty string → remove all ATTENDEE properties
   • Reminder: 0 minutes → remove all VALARM components

3. Attendee Handling: The attendee logic correctly removes all existing attendees before adding new ones (lines 835-840), preventing duplicates.

4. Comprehensive Test Coverage:

   • Client-level test (test_update_event_extended_fields) verifies both set and clear operations
   • MCP-level test (test_mcp_update_event_extended_fields) validates end-to-end behavior
   • Both tests use two-phase approach (set → verify → clear → verify)

5. Proper Fixture Usage: Tests correctly use temporary_calendar fixture and include proper cleanup in finally blocks.

---

🔍 Minor Observations

1. Categories Whitespace Handling (calendar.py:819)

The current implementation:

component["CATEGORIES"] = categories_str.split(",")

Consideration: Should we strip whitespace from category names? For example, "work, meeting" would create a category with a leading space: " meeting".

Comparison:

• _create_ical_event (line 654): Also uses split(",") without stripping
• _merge_ical_todo_properties (line 1090): Same behavior

Verdict: ✅ Consistent with existing code. If whitespace handling is desired, it should be addressed separately across all methods.

2. Recurrence Rule Validation (calendar.py:827)

component["RRULE"] = vRecur.from_ical(rrule_str)

Question: Should we wrap this in a try-except to handle malformed recurrence rules gracefully?

Analysis: _create_ical_event (line 677) doesn't validate either, and the outer try-except at line 794 will catch any parsing errors. Current behavior is consistent.

3. Test Assertion Specificity (test_calendar_events_mcp.py:66-84)

The assertions check for string containment ("work" in categories) rather than exact matches. This is appropriate for categories (which might have additional items), but could potentially miss edge cases.

Example: If categories were set to "networking", the test assert "work" in categories would pass.

Verdict: ✅ Acceptable for integration tests. The two-phase test (set → clear) would catch most issues.

---

🎯 Code Quality Assessment

Criterion	Rating	Notes
Correctness	✅ Excellent	Fixes the reported bug completely
Consistency	✅ Excellent	Matches patterns in _create_ical_event and _merge_ical_todo_properties
Type Safety	✅ Good	PR author confirmed ty check passes
Test Coverage	✅ Excellent	Both client-level and MCP-level tests with set/clear verification
Documentation	✅ Good	Clear PR description with test plan
Error Handling	✅ Good	Relies on existing try-except at method level

---

🔒 Security & Performance

• No security concerns: All inputs are properly handled (email addresses prefixed with mailto:, strings split safely)
• No performance issues: Operations are efficient (linear scans for deletion are appropriate for small lists)
• No OWASP concerns: No injection risks, XSS, or other vulnerabilities

---

📋 Compliance with CLAUDE.md

✅ async/await: Not applicable (synchronous iCal manipulation)
✅ Type hints: Uses Dict[str, Any] consistently
✅ Code quality: Follows existing patterns
✅ Testing patterns: Uses temporary_calendar fixture, proper cleanup
✅ Architectural patterns: Maintains separation between client methods and iCal manipulation
✅ Error handling: Consistent with existing code

---

🎬 Recommendation

✅ APPROVE - This PR is ready to merge.

The implementation correctly fixes issue #544 by adding support for updating categories, recurrence_rule, attendees, and reminder_minutes fields. The code follows established patterns, includes comprehensive tests, and passes all quality checks.

Optional follow-up (not blocking):

• Consider adding whitespace trimming for categories in a future PR (would require updating all three methods: _create_ical_event, _merge_ical_properties, _merge_ical_todo_properties)

---

Great work! 🚀

[claude]

Code Review

Thank you for this PR! This is a solid fix for issue #544. The implementation correctly addresses the missing field handling in _merge_ical_properties(). Here's my detailed review:

✅ Strengths

1. Excellent test coverage: Both client-level (test_update_event_extended_fields) and MCP-level (test_mcp_update_event_extended_fields) integration tests with two-phase verification (set + clear operations)

2. Consistent implementation: The new code matches patterns from _create_ical_event() and _merge_ical_todo_properties(), maintaining codebase consistency

3. Proper field clearing: All four fields support both setting and clearing (empty string/0 values properly remove fields)

4. Good whitespace handling: The .strip() addition for categories in all three methods (commit ec8eab9) prevents issues like "work, meeting" creating " meeting"

5. Follows CLAUDE.md conventions: Uses async/await, proper type hints, and error handling patterns

🔍 Observations & Suggestions

1. Reminder handling correctness (nextcloud_mcp_server/client/calendar.py:845-857)

The implementation correctly handles VALARM clearing by filtering subcomponents before conditionally adding a new alarm. This matches the pattern in _create_ical_event() and properly supports reminder_minutes: 0 to clear reminders.

Question: Should negative reminder_minutes values be explicitly rejected or normalized? Currently minutes > 0 silently ignores negatives, but validation at the API boundary might be clearer.

2. RRULE validation (nextcloud_mcp_server/client/calendar.py:826-831)

The code uses vRecur.from_ical(rrule_str) which will raise ValueError for malformed recurrence rules. This is correct behavior, but consider:

• Adding a docstring note about expected RRULE format
• Potentially wrapping in a try-except with a more descriptive error message

3. Attendee email validation (nextcloud_mcp_server/client/calendar.py:834-842)

Current validation: if email.strip(): only checks for non-empty strings. Consider:

• Basic email format validation (regex or simple @ check)
• Handling invalid mailto: URIs more gracefully
• Currently, inputs like "alice@example.com, , bob@example.com" work correctly (empty entries are skipped), which is good

4. Test assertion strength (tests/client/calendar/test_calendar_operations.py:308)

This assertion passes if categories is "work,meeting" or "working". Consider exact matching with split and strip for more precise validation. Similar consideration for attendees assertions.

5. Missing edge case tests

Consider adding tests for:

• Invalid RRULE strings (should fail gracefully)
• Malformed attendee emails
• Very large reminder values (e.g., 10080 minutes = 1 week)
• Unicode in categories (e.g., Chinese or emoji characters)

6. Documentation

Consider updating:

• docs/calendar.md (if it exists) with update behavior for these fields
• Docstring for update_event() method to clarify which fields support clearing

🔒 Security

No security concerns identified:

• Input sanitization via .strip() prevents injection
• CalDAV library handles iCalendar escaping
• No SQL injection or XSS vectors

🎯 Performance

No performance concerns:

• List comprehensions are efficient
• Subcomponent filtering for VALARM is O(n) but n is typically small
• Attendee deletion loop is acceptable (ATTENDEE count rarely exceeds 20-30)

📋 Checklist Status

All checklist items completed and passing

🎯 Recommendation

LGTM with minor suggestions. The core fix is correct and well-tested. The suggestions above are for future enhancements, not blockers. This PR successfully resolves issue #544.

Suggested merge decision: Approve and merge after considering whether to add stricter validation (suggestion #1-2) or merge as-is with follow-up issues for edge cases.

Great work on the thorough testing approach! 🎉