Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions mcp_zammad/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ def update_ticket(
priority: str | None = None,
owner: str | None = None,
group: str | None = None,
time_unit: float | None = None,
) -> dict[str, Any]:
"""Update an existing ticket."""
update_data = {}
Expand All @@ -230,6 +231,8 @@ def update_ticket(
update_data["owner"] = owner
if group is not None:
update_data["group"] = group
if time_unit is not None:
update_data["time_unit"] = time_unit

return dict(self.api.ticket.update(ticket_id, update_data))

Expand All @@ -240,6 +243,7 @@ def add_article(
article_type: str = "note",
internal: bool = False,
sender: str = "Agent",
time_unit: float | None = None,
attachments: list[dict[str, str]] | None = None,
) -> dict[str, Any]:
"""Add an article (comment/note) to a ticket with optional attachments.
Expand All @@ -250,6 +254,7 @@ def add_article(
article_type: Article type (note, email, phone)
internal: Whether the article is internal
sender: Sender type (Agent, Customer, System)
time_unit: Time spent for time accounting (unit defined in Zammad admin settings)
attachments: Optional list of attachments with keys:
- filename: str
- data: str (base64-encoded content)
Expand All @@ -266,6 +271,9 @@ def add_article(
"sender": sender,
}

if time_unit is not None:
article_data["time_unit"] = time_unit

if attachments:
article_data["attachments"] = attachments

Expand Down
9 changes: 9 additions & 0 deletions mcp_zammad/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,9 @@ class TicketUpdate(StrictBaseModel):
priority: str | None = Field(None, description="New priority name", max_length=100)
owner: str | None = Field(None, description="New owner login/email", max_length=255)
group: str | None = Field(None, description="New group name", max_length=100)
time_unit: float | None = Field(
None, description="Time spent for time accounting (unit defined in Zammad admin settings)", gt=0
)

@field_validator("title")
@classmethod
Expand Down Expand Up @@ -322,6 +325,9 @@ class ArticleCreate(StrictBaseModel):
article_type: ArticleType = Field(default=ArticleType.NOTE, alias="type", description="Article type")
internal: bool = Field(default=False, description="Whether the article is internal")
sender: ArticleSender = Field(default=ArticleSender.AGENT, description="Sender type")
time_unit: float | None = Field(
default=None, description="Time spent for time accounting (unit defined in Zammad admin settings)", gt=0
)
attachments: list[AttachmentUpload] | None = Field(
default=None, description="Optional attachments to include", max_length=10
)
Expand Down Expand Up @@ -354,6 +360,9 @@ class TicketUpdateParams(StrictBaseModel):
priority: str | None = Field(None, description="New priority name", max_length=100)
owner: str | None = Field(None, description="New owner login/email", max_length=255)
group: str | None = Field(None, description="New group name", max_length=100)
time_unit: float | None = Field(
None, description="Time spent for time accounting (unit defined in Zammad admin settings)", gt=0
)

@field_validator("title")
@classmethod
Expand Down
1 change: 1 addition & 0 deletions mcp_zammad/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ def zammad_update_ticket(params: TicketUpdateParams) -> Ticket:
- group (str | None): New group name
- owner (str | None): New owner email/login
- customer (str | None): New customer email/login
- time_unit (float | None): Time spent for time accounting

Returns:
Ticket: The updated ticket object with schema:
Expand Down
34 changes: 34 additions & 0 deletions tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,40 @@ def test_add_article_without_attachments_backward_compat(mock_api: MagicMock) ->
assert "attachments" not in call_args # Should not include empty attachments


@patch("mcp_zammad.client.ZammadAPI")
def test_add_article_with_time_unit(mock_api: MagicMock) -> None:
"""Test adding article with time_unit for time accounting."""
mock_instance = mock_api.return_value
mock_instance.ticket_article.create.return_value = {"id": 789, "ticket_id": 123, "body": "Worked on this"}

with patch.dict(
os.environ, {"ZAMMAD_URL": "https://test.zammad.com/api/v1", "ZAMMAD_HTTP_TOKEN": "token"}, clear=True
):
client = ZammadClient()
result = client.add_article(ticket_id=123, body="Worked on this", time_unit=45.5)

assert result["id"] == 789
call_args = mock_instance.ticket_article.create.call_args[0][0]
assert call_args["time_unit"] == 45.5


@patch("mcp_zammad.client.ZammadAPI")
def test_add_article_without_time_unit_excludes_field(mock_api: MagicMock) -> None:
"""Test that time_unit is not included in payload when None."""
mock_instance = mock_api.return_value
mock_instance.ticket_article.create.return_value = {"id": 789, "ticket_id": 123, "body": "Simple comment"}

with patch.dict(
os.environ, {"ZAMMAD_URL": "https://test.zammad.com/api/v1", "ZAMMAD_HTTP_TOKEN": "token"}, clear=True
):
client = ZammadClient()
result = client.add_article(ticket_id=123, body="Simple comment")

assert result["id"] == 789
call_args = mock_instance.ticket_article.create.call_args[0][0]
assert "time_unit" not in call_args


@patch("mcp_zammad.client.ZammadAPI")
def test_delete_attachment_success(mock_api: MagicMock) -> None:
"""Test successful attachment deletion."""
Expand Down
55 changes: 55 additions & 0 deletions tests/test_client_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,32 @@ def test_update_ticket(self, mock_zammad_api: Mock) -> None:
assert result["title"] == "Updated Title"
mock_instance.ticket.update.assert_called_once_with(1, {"title": "Updated Title", "state": "open"})

def test_update_ticket_with_time_unit(self, mock_zammad_api: Mock) -> None:
"""Test update_ticket method with time_unit for time accounting."""
mock_instance = Mock()
mock_instance.ticket.update.return_value = {"id": 1, "title": "Test", "state": "open"}
mock_zammad_api.return_value = mock_instance

client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token")

result = client.update_ticket(1, title="Test", time_unit=2.5)

assert result["id"] == 1
mock_instance.ticket.update.assert_called_once_with(1, {"title": "Test", "time_unit": 2.5})

def test_update_ticket_without_time_unit_excludes_field(self, mock_zammad_api: Mock) -> None:
"""Test that time_unit is not included in payload when None."""
mock_instance = Mock()
mock_instance.ticket.update.return_value = {"id": 1, "title": "Test"}
mock_zammad_api.return_value = mock_instance

client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token")

client.update_ticket(1, title="Test")

call_args = mock_instance.ticket.update.call_args[0][1]
assert "time_unit" not in call_args

def test_get_groups(self, mock_zammad_api: Mock) -> None:
"""Test get_groups method."""
mock_instance = Mock()
Expand Down Expand Up @@ -366,6 +392,35 @@ def test_add_article(self, mock_zammad_api: Mock) -> None:
{"ticket_id": 1, "body": "Response", "type": "email", "internal": True, "sender": "Customer"}
)

def test_add_article_with_time_unit(self, mock_zammad_api: Mock) -> None:
"""Test add_article method with time_unit."""
mock_instance = Mock()
mock_instance.ticket_article.create.return_value = {"id": 2, "body": "Worked on it", "type": "note"}
mock_zammad_api.return_value = mock_instance

client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token")

result = client.add_article(ticket_id=1, body="Worked on it", time_unit=15.0)

assert result["id"] == 2
mock_instance.ticket_article.create.assert_called_once_with(
{"ticket_id": 1, "body": "Worked on it", "type": "note", "internal": False, "sender": "Agent", "time_unit": 15.0}
)

def test_add_article_without_time_unit(self, mock_zammad_api: Mock) -> None:
"""Test add_article method without time_unit excludes it from payload."""
mock_instance = Mock()
mock_instance.ticket_article.create.return_value = {"id": 3, "body": "Simple note", "type": "note"}
mock_zammad_api.return_value = mock_instance

client = ZammadClient(url="https://test.zammad.com/api/v1", http_token="test-token")

result = client.add_article(ticket_id=1, body="Simple note")

assert result["id"] == 3
call_args = mock_instance.ticket_article.create.call_args[0][0]
assert "time_unit" not in call_args

def test_get_user(self, mock_zammad_api: Mock) -> None:
"""Test get_user method."""
mock_instance = Mock()
Expand Down
119 changes: 118 additions & 1 deletion tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
TicketPriority,
TicketSearchParams,
TicketState,
TicketUpdateParams,
User,
UserBrief,
UserCreate,
Expand Down Expand Up @@ -517,10 +518,73 @@ def test_add_article_tool(mock_zammad_client, sample_article_data, decorator_cap

# Verify the client was called with correct params (including attachments=None for backward compat)
mock_instance.add_article.assert_called_once_with(
ticket_id=1, article_type="note", attachments=None, body="New comment", internal=False, sender="Agent"
ticket_id=1,
article_type="note",
attachments=None,
body="New comment",
internal=False,
sender="Agent",
time_unit=None,
)


def test_add_article_with_time_unit_tool(mock_zammad_client, sample_article_data, decorator_capturer):
"""Test zammad_add_article tool with time_unit for time accounting."""
mock_instance, _ = mock_zammad_client

mock_instance.add_article.return_value = sample_article_data

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_tools()

params = ArticleCreate(ticket_id=1, body="Worked on this issue", time_unit=30.5)
result = test_tools["zammad_add_article"](params)

assert result.body == "Test article"

mock_instance.add_article.assert_called_once()
call_kwargs = mock_instance.add_article.call_args[1]
assert call_kwargs["time_unit"] == 30.5


def test_add_article_without_time_unit_tool(mock_zammad_client, sample_article_data, decorator_capturer):
"""Test zammad_add_article tool without time_unit passes None."""
mock_instance, _ = mock_zammad_client

mock_instance.add_article.return_value = sample_article_data

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_tools()

params = ArticleCreate(ticket_id=1, body="Simple comment")
result = test_tools["zammad_add_article"](params)

assert result.body == "Test article"

mock_instance.add_article.assert_called_once()
call_kwargs = mock_instance.add_article.call_args[1]
assert call_kwargs["time_unit"] is None


def test_add_article_invalid_time_unit():
"""Test that ArticleCreate rejects invalid time_unit values."""
with pytest.raises(ValidationError, match="time_unit"):
ArticleCreate(ticket_id=1, body="test", time_unit=0)

with pytest.raises(ValidationError, match="time_unit"):
ArticleCreate(ticket_id=1, body="test", time_unit=-5)


def test_add_article_invalid_type():
"""Test that ArticleCreate rejects invalid article types."""
# Test invalid article type
Expand Down Expand Up @@ -718,6 +782,59 @@ def test_update_ticket_tool(mock_zammad_client, sample_ticket_data):
)


def test_update_ticket_with_time_unit_tool(mock_zammad_client, sample_ticket_data):
"""Test update ticket tool with time_unit for time accounting."""
mock_instance, _ = mock_zammad_client

updated_ticket = sample_ticket_data.copy()
updated_ticket["title"] = "Updated Title"

mock_instance.update_ticket.return_value = updated_ticket

server_inst = ZammadMCPServer()
server_inst.client = mock_instance
client = server_inst.get_client()

ticket_data = client.update_ticket(1, title="Updated Title", time_unit=2.5)
result = Ticket(**ticket_data)

assert result.id == 1
mock_instance.update_ticket.assert_called_once_with(1, title="Updated Title", time_unit=2.5)


def test_update_ticket_without_time_unit_tool(mock_zammad_client, sample_ticket_data):
"""Test update ticket tool without time_unit does not pass it."""
mock_instance, _ = mock_zammad_client

mock_instance.update_ticket.return_value = sample_ticket_data

server_inst = ZammadMCPServer()
server_inst.client = mock_instance
client = server_inst.get_client()

client.update_ticket(1, title="Updated Title")

mock_instance.update_ticket.assert_called_once_with(1, title="Updated Title")

Comment on lines +785 to +818
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

update_ticket time-unit tests currently assert mock behavior, not tool behavior

At Line 798 and Line 815 the test calls a mocked client directly, so it does not verify zammad_update_ticket parameter forwarding at the server/tool layer.

🔧 Proposed fix (exercise the actual tool handler)
-def test_update_ticket_with_time_unit_tool(mock_zammad_client, sample_ticket_data):
+def test_update_ticket_with_time_unit_tool(mock_zammad_client, sample_ticket_data, decorator_capturer):
@@
-    client = server_inst.get_client()
-
-    ticket_data = client.update_ticket(1, title="Updated Title", time_unit=2.5)
-    result = Ticket(**ticket_data)
+    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_tools()
+    result = test_tools["zammad_update_ticket"](TicketUpdateParams(ticket_id=1, title="Updated Title", time_unit=2.5))
@@
-    mock_instance.update_ticket.assert_called_once_with(1, title="Updated Title", time_unit=2.5)
+    mock_instance.update_ticket.assert_called_once_with(ticket_id=1, title="Updated Title", time_unit=2.5)

-def test_update_ticket_without_time_unit_tool(mock_zammad_client, sample_ticket_data):
+def test_update_ticket_without_time_unit_tool(mock_zammad_client, sample_ticket_data, decorator_capturer):
@@
-    client = server_inst.get_client()
-
-    client.update_ticket(1, title="Updated Title")
-
-    mock_instance.update_ticket.assert_called_once_with(1, title="Updated Title")
+    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_tools()
+    test_tools["zammad_update_ticket"](TicketUpdateParams(ticket_id=1, title="Updated Title"))
+
+    mock_instance.update_ticket.assert_called_once_with(ticket_id=1, title="Updated Title")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_server.py` around lines 785 - 818, The tests call the mocked
client's update_ticket directly instead of exercising the server/tool handler,
so they don't verify zammad_update_ticket's parameter forwarding; change both
test_update_ticket_with_time_unit_tool and
test_update_ticket_without_time_unit_tool to invoke the server's tool handler
(zammad_update_ticket) on the ZammadMCPServer instance (e.g., call
server_inst.zammad_update_ticket or the server's tool dispatch method that runs
the zammad_update_ticket handler) with the same args used now, then assert
mock_instance.update_ticket was called with the expected parameters (including
time_unit when provided and omitted when not).


def test_update_ticket_invalid_time_unit():
"""Test that TicketUpdateParams rejects invalid time_unit values."""
with pytest.raises(ValidationError, match="time_unit"):
TicketUpdateParams(ticket_id=1, time_unit=0)

with pytest.raises(ValidationError, match="time_unit"):
TicketUpdateParams(ticket_id=1, time_unit=-5)


def test_update_ticket_valid_time_unit():
"""Test that TicketUpdateParams accepts valid time_unit values."""
params = TicketUpdateParams(ticket_id=1, time_unit=1.5)
assert params.time_unit == 1.5

params_none = TicketUpdateParams(ticket_id=1)
assert params_none.time_unit is None


def test_get_organization_tool(mock_zammad_client, sample_organization_data):
"""Test get organization tool."""
mock_instance, _ = mock_zammad_client
Expand Down