diff --git a/mcp_zammad/client.py b/mcp_zammad/client.py index ce9da7d4..89743ab6 100644 --- a/mcp_zammad/client.py +++ b/mcp_zammad/client.py @@ -5,6 +5,7 @@ from typing import Any from urllib.parse import urlparse +import requests # type: ignore[import-untyped] from zammad_py import ZammadAPI from zammad_py.exceptions import ConfigException @@ -189,11 +190,34 @@ def search_tickets( return list(result) + def _find_ticket_expanded(self, ticket_id: int) -> dict[str, Any]: + """Fetch a single ticket with ``expand=true``. + + The expand value must be the lowercase string ``"true"`` — Zammad is + case-sensitive here and ``requests`` serializes the bool ``True`` as + ``"True"``, which Zammad ignores. + + Raises: + requests.HTTPError: If the API request fails, carrying Zammad's + response body so callers can detect "Couldn't find Ticket ..." + """ + response = self.api.session.get(f"{self.url}/tickets/{ticket_id}", params={"expand": "true"}) + if not response.ok: + raise requests.HTTPError(response.text) + return dict(response.json()) + def get_ticket( self, ticket_id: int, include_articles: bool = True, article_limit: int = 10, article_offset: int = 0 ) -> dict[str, Any]: - """Get a single ticket by ID with optional article pagination.""" - ticket = self.api.ticket.find(ticket_id) + """Get a single ticket by ID with optional article pagination. + + Uses a direct HTTP call via zammad_py's internal session because + ``Resource.find()`` accepts no filters, so there is no way to pass + ``expand=true`` through it. Without expansion Zammad only returns the + ``*_id`` fields and the state/priority/group/owner/customer names all + render as "Unknown". + """ + ticket = self._find_ticket_expanded(ticket_id) if include_articles: articles = self.api.ticket.articles(ticket_id) diff --git a/tests/test_client_methods.py b/tests/test_client_methods.py index 1bf84876..d8112bc3 100644 --- a/tests/test_client_methods.py +++ b/tests/test_client_methods.py @@ -320,10 +320,20 @@ def test_search_tickets_no_query(self, mock_zammad_api: Mock) -> None: assert len(result) == 1 mock_instance.ticket.all.assert_called_once_with(filters={"page": 1, "per_page": 25, "expand": "true"}) + @staticmethod + def _mock_ticket_response(mock_instance: Mock, payload: dict, *, ok: bool = True, text: str = "") -> Mock: + """Point the client's session at a canned GET /tickets/{id} response.""" + response = Mock() + response.ok = ok + response.text = text + response.json.return_value = payload + mock_instance.session.get.return_value = response + return response + def test_get_ticket_with_articles(self, mock_zammad_api: Mock) -> None: """Test get_ticket with article pagination.""" mock_instance = Mock() - mock_instance.ticket.find.return_value = {"id": 1, "title": "Test Ticket"} + self._mock_ticket_response(mock_instance, {"id": 1, "title": "Test Ticket"}) mock_instance.ticket.articles.return_value = [ {"id": 1, "body": "Article 1"}, {"id": 2, "body": "Article 2"}, @@ -346,7 +356,7 @@ def test_get_ticket_with_articles(self, mock_zammad_api: Mock) -> None: def test_get_ticket_all_articles(self, mock_zammad_api: Mock) -> None: """Test get_ticket with all articles.""" mock_instance = Mock() - mock_instance.ticket.find.return_value = {"id": 1, "title": "Test Ticket"} + self._mock_ticket_response(mock_instance, {"id": 1, "title": "Test Ticket"}) mock_instance.ticket.articles.return_value = [{"id": 1, "body": "Article 1"}, {"id": 2, "body": "Article 2"}] mock_zammad_api.return_value = mock_instance @@ -357,6 +367,62 @@ def test_get_ticket_all_articles(self, mock_zammad_api: Mock) -> None: assert len(result["articles"]) == 2 + def test_get_ticket_requests_expanded_fields(self, mock_zammad_api: Mock) -> None: + """get_ticket must ask for expand=true, otherwise names come back as None. + + Regression test: without the flag Zammad only returns ``*_id`` fields and + the server renders State/Priority/Group/Owner/Customer as "Unknown". + """ + mock_instance = Mock() + self._mock_ticket_response( + mock_instance, + { + "id": 1, + "title": "Test Ticket", + "state_id": 2, + "state": "open", + "priority_id": 3, + "priority": "3 high", + "group": "Support", + }, + ) + mock_zammad_api.return_value = mock_instance + + client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token") + + result = client.get_ticket(1, include_articles=False) + + mock_instance.session.get.assert_called_once_with( + "https://test.zammad.com/api/v1/tickets/1", params={"expand": "true"} + ) + # The literal string "true" matters: requests serializes bool True as + # "True", which Zammad ignores because the parameter is case-sensitive. + _, kwargs = mock_instance.session.get.call_args + assert kwargs["params"]["expand"] == "true" + assert result["state"] == "open" + assert result["priority"] == "3 high" + assert result["group"] == "Support" + + def test_get_ticket_not_found_raises_with_zammad_message(self, mock_zammad_api: Mock) -> None: + """A failed lookup must surface Zammad's body so the server can map it. + + The server turns "Couldn't find Ticket ..." into TicketIdGuidanceError, + so the response text has to survive. + """ + mock_instance = Mock() + self._mock_ticket_response( + mock_instance, + {}, + ok=False, + text='{"error":"Couldn\'t find Ticket with \'id\'=999"}', + ) + mock_zammad_api.return_value = mock_instance + + client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token") + + with pytest.raises(requests.HTTPError, match="Couldn't find Ticket"): + client.get_ticket(999) + def test_create_ticket(self, mock_zammad_api: Mock) -> None: """Test create_ticket method.""" mock_instance = Mock()