diff --git a/mcp_zammad/server.py b/mcp_zammad/server.py index 4a5f5443..6c353f36 100644 --- a/mcp_zammad/server.py +++ b/mcp_zammad/server.py @@ -102,14 +102,6 @@ def model_dump(self) -> dict[str, Any]: ... # codacy: ignore E704 CHARACTER_LIMIT = 25000 # Maximum response size per MCP best practices ARTICLE_BODY_TRUNCATE_LENGTH = 500 # Maximum length for article body in markdown formatting -# Zammad state type IDs (from Zammad API) -STATE_TYPE_NEW = 1 -STATE_TYPE_OPEN = 2 -STATE_TYPE_CLOSED = 3 -STATE_TYPE_PENDING_REMINDER = 4 -STATE_TYPE_PENDING_CLOSE = 5 - - # Tool annotation constants def _read_only_annotations(title: str) -> ToolAnnotations: """Create read-only tool annotations with title.""" @@ -1848,8 +1840,6 @@ def clear_caches(self) -> None: del self._states_cache if hasattr(self, "_priorities_cache"): del self._priorities_cache - if hasattr(self, "_state_type_mapping"): - del self._state_type_mapping @staticmethod def _extract_state_name(ticket: dict[str, Any]) -> str: @@ -1884,16 +1874,21 @@ def _is_ticket_escalated(ticket: dict[str, Any]) -> bool: or ticket.get("update_escalation_at") ) - def _get_state_type_mapping(self) -> dict[str, int]: - """Get mapping of state names to state_type_id. - - Returns: - Dictionary mapping state name to state_type_id - """ - if not hasattr(self, "_state_type_mapping"): - states = self._get_cached_states() - self._state_type_mapping = {state.name: state.state_type_id for state in states} - return self._state_type_mapping + # Semantic ticket state names that map to the open/closed/pending buckets. + # Categorization is keyed on the state *name*, not the numeric state_type_id: + # state_type_id is assigned per instance and is not stable across Zammad + # versions or installations, so matching on it produced incorrect counts on + # instances whose state set differs from the defaults (e.g. a renumbered + # "closed" state or an extra custom state such as "merged"). + _STATE_NAME_OPEN = frozenset({"new", "open"}) + _STATE_NAME_CLOSED = frozenset({"closed"}) + _STATE_NAME_PENDING = frozenset({"pending", "pending reminder", "pending close"}) + # Fallback prefix for custom states that Zammad's own UI treats as pending + # (e.g. a user-defined "pending refund"). A space-terminated word match is + # deliberately narrower than a bare prefix or substring: it catches + # "pending anything" without misfiring on near-miss names such as + # "pendingly" or "pending-approval", which are not reliably pending states. + _STATE_NAME_PENDING_PREFIX = "pending " def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: """Categorize a ticket state into open/closed/pending counters. @@ -1905,20 +1900,21 @@ def _categorize_ticket_state(self, state_name: str) -> tuple[int, int, int]: Tuple of (open_increment, closed_increment, pending_increment) Note: - Uses state_type_id from Zammad instead of string matching: - - 1 (new), 2 (open) -> open - - 3 (closed) -> closed - - 4 (pending reminder), 5 (pending close) -> pending + Categorizes by the semantic state name (case-insensitive) rather + than the numeric state_type_id, which is per-instance and unstable: + - "new", "open" -> open + - "closed" -> closed + - "pending", "pending reminder", "pending close", or any custom + state starting with "pending " (space-terminated) -> pending + Any other state (e.g. "merged") is counted in the total but not in + any bucket. """ - state_type_mapping = self._get_state_type_mapping() - state_type_id = state_type_mapping.get(state_name, 0) - - # Categorize based on state_type_id - if state_type_id in [STATE_TYPE_NEW, STATE_TYPE_OPEN]: + name = state_name.strip().casefold() + if name in self._STATE_NAME_OPEN: return (1, 0, 0) - if state_type_id == STATE_TYPE_CLOSED: + if name in self._STATE_NAME_CLOSED: return (0, 1, 0) - if state_type_id in [STATE_TYPE_PENDING_REMINDER, STATE_TYPE_PENDING_CLOSE]: + if name in self._STATE_NAME_PENDING or name.startswith(self._STATE_NAME_PENDING_PREFIX): return (0, 0, 1) return (0, 0, 0) @@ -2084,7 +2080,8 @@ def zammad_get_ticket_stats(params: GetTicketStatsParams) -> TicketStats: Note: Uses pagination to scan tickets without loading all into memory. May take several seconds for large ticket databases (>10k tickets). - State categorization uses state_type_id: new/open=open, closed=closed, pending=pending. + State categorization is by semantic state name: new/open=open, + closed=closed, pending reminder/pending close=pending. Date filtering (start_date, end_date) not yet implemented - shows warning if provided. Processes up to 100,000 tickets (1000 pages x 100 per page). """ @@ -2222,7 +2219,8 @@ def zammad_list_ticket_states(params: ListParams) -> str: Results are cached in memory for performance (cleared on server restart). All states are returned in a single response (no pagination needed). Use state 'name' field when creating/updating tickets, not ID. - State types: 1=new, 2=open, 3=closed, 4=pending reminder, 5=pending close. + The state_type_id values shown are per-instance and are not + meaningful across different Zammad installations. """ states = self._get_cached_states() diff --git a/tests/test_server.py b/tests/test_server.py index e5e7e7be..8579c42a 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -1398,6 +1398,89 @@ def test_get_ticket_stats_tool(mock_zammad_client, decorator_capturer): mock_logger.warning.assert_called_with("Date filtering not yet implemented - ignoring date parameters") +def test_categorize_ticket_state_uses_name_not_type_id(): + """Categorize by state name, not numeric state_type_id (per-instance and unstable).""" + # Regression guard: on instances with renumbered built-in states or custom + # states, the numeric state_type_id does not line up with the defaults, so + # only the semantic name can be trusted. + server_inst = ZammadMCPServer() + + assert server_inst._categorize_ticket_state("new") == (1, 0, 0) + assert server_inst._categorize_ticket_state("open") == (1, 0, 0) + assert server_inst._categorize_ticket_state("closed") == (0, 1, 0) + assert server_inst._categorize_ticket_state("pending reminder") == (0, 0, 1) + assert server_inst._categorize_ticket_state("pending close") == (0, 0, 1) + # The bare "pending" state (exact match) is pending. + assert server_inst._categorize_ticket_state("pending") == (0, 0, 1) + # Custom "pending " states fall into pending via the space-terminated + # word fallback, so user-defined states (e.g. "pending refund") are not + # silently dropped. + assert server_inst._categorize_ticket_state("pending refund") == (0, 0, 1) + # The fallback is deliberately narrow: near-miss names are NOT auto- + # pending (a bare prefix or substring would miscount these). + assert server_inst._categorize_ticket_state("pendingly") == (0, 0, 0) + assert server_inst._categorize_ticket_state("pending-approval") == (0, 0, 0) + assert server_inst._categorize_ticket_state("PENDING") == (0, 0, 1) + assert server_inst._categorize_ticket_state("PENDING APPROVAL") == (0, 0, 1) + # A state that merely *contains* "pending" elsewhere is NOT auto-pending. + assert server_inst._categorize_ticket_state("reviewed pending approval") == (0, 0, 0) + # Other custom states: counted in the total, excluded from every bucket. + assert server_inst._categorize_ticket_state("merged") == (0, 0, 0) + # Case-insensitive, tolerant of surrounding whitespace. + assert server_inst._categorize_ticket_state(" Closed ") == (0, 1, 0) + + +def test_get_ticket_stats_uses_state_name_not_state_type_id(mock_zammad_client, decorator_capturer): + """Ticket stats stay correct when state_type_id values are non-default.""" + # Replicates a real Zammad instance where the built-in states are renumbered + # (closed=5, pending reminder=3) and a custom "merged" state (id 6) exists. + # The old implementation compared state_type_id against hardcoded defaults + # (1-5), which scrambled the counts on such an instance. + mock_instance, _ = mock_zammad_client + + tickets = [ + {"id": 1, "state": {"id": 1, "name": "new", "state_type_id": 1}}, + {"id": 2, "state": {"id": 2, "name": "open", "state_type_id": 2}}, + # Non-default: "pending reminder" is state_type_id 3 here (not 4). + {"id": 3, "state": {"id": 3, "name": "pending reminder", "state_type_id": 3}}, + # Non-default: "closed" is state_type_id 5 here (not 3). + {"id": 4, "state": {"id": 5, "name": "closed", "state_type_id": 5}}, + {"id": 5, "state": {"id": 4, "name": "pending close", "state_type_id": 4}}, + # Custom state: counted in total, excluded from every bucket. + {"id": 6, "state": {"id": 6, "name": "merged", "state_type_id": 6}}, + ] + mock_instance.search_tickets.side_effect = [tickets, []] + mock_instance.get_ticket_states.return_value = [ + {"id": 1, "name": "new", "state_type_id": 1, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 2, "name": "open", "state_type_id": 2, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + { + "id": 3, + "name": "pending reminder", + "state_type_id": 3, + "created_at": "2024-01-01", + "updated_at": "2024-01-01", + }, + {"id": 5, "name": "closed", "state_type_id": 5, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 4, "name": "pending close", "state_type_id": 4, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + {"id": 6, "name": "merged", "state_type_id": 6, "created_at": "2024-01-01", "updated_at": "2024-01-01"}, + ] + + server_inst = ZammadMCPServer() + server_inst.client = mock_instance + test_tools, capture_tool = decorator_capturer(server_inst.mcp.tool) + server_inst.mcp.tool = capture_tool # type: ignore[method-assign, assignment] + server_inst.get_client = lambda: server_inst.client # type: ignore[method-assign, assignment, return-value] + server_inst._setup_system_tools() + + stats = test_tools["zammad_get_ticket_stats"](GetTicketStatsParams()) + + assert stats.total_count == 6 + assert stats.open_count == 2 # new + open + assert stats.closed_count == 1 # closed (state_type_id 5, not 3) + assert stats.pending_count == 2 # pending reminder (id 3) + pending close (id 4) + # "merged" is in the total but in no bucket: 2 + 1 + 2 = 5, + 1 custom = 6. + + def test_resource_handlers(decorator_capturer): """Test resource handler registration and execution.""" server = ZammadMCPServer()