feat(time-accounting): add time_unit support to update_ticket and add_article#211
Open
matiasiglesias wants to merge 1 commit intobasher83:mainfrom
Open
feat(time-accounting): add time_unit support to update_ticket and add_article#211matiasiglesias wants to merge 1 commit intobasher83:mainfrom
matiasiglesias wants to merge 1 commit intobasher83:mainfrom
Conversation
…_article - Add time_unit parameter to TicketUpdateParams, TicketUpdate models - Add time_unit parameter to client.update_ticket() method - Add time_unit parameter to ArticleCreate model and client.add_article() - Update server docstring for zammad_update_ticket tool - Add comprehensive tests for time_unit in both tools
Contributor
WalkthroughThe pull request adds optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_zammad/client.py (1)
220-236:⚠️ Potential issue | 🟠 MajorValidate
time_unitin client methods before sending API requests
update_ticket()andadd_article()accepttime_unitbut do not enforce> 0locally. Direct client users can pass0/negative values and only fail downstream.🔧 Proposed fix
def update_ticket( self, ticket_id: int, title: str | None = None, state: str | None = None, priority: str | None = None, owner: str | None = None, group: str | None = None, time_unit: float | None = None, ) -> dict[str, Any]: """Update an existing ticket.""" + if time_unit is not None and time_unit <= 0: + raise ValueError("time_unit must be greater than 0") + update_data = {} if title is not None: update_data["title"] = title @@ def add_article( self, ticket_id: int, body: str, 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. @@ """ + if time_unit is not None and time_unit <= 0: + raise ValueError("time_unit must be greater than 0") + article_data = { "ticket_id": ticket_id, "body": body,As per coding guidelines, "Always validate input with Pydantic models" and "Validate all user inputs".
Also applies to: 246-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp_zammad/client.py` around lines 220 - 236, The update_ticket and add_article methods accept a time_unit parameter but do not validate it locally, allowing 0 or negative values to be sent to the API; before adding time_unit to update_data (or the article payload) check that time_unit is not None and > 0 and otherwise raise a ValueError (or use the existing Pydantic request model validation) so invalid values are rejected client-side—update the checks around the time_unit parameter in update_ticket and add_article (and any similar blocks that build payloads from time_unit) to perform this validation before including time_unit in the request payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_server.py`:
- Around line 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).
---
Outside diff comments:
In `@mcp_zammad/client.py`:
- Around line 220-236: The update_ticket and add_article methods accept a
time_unit parameter but do not validate it locally, allowing 0 or negative
values to be sent to the API; before adding time_unit to update_data (or the
article payload) check that time_unit is not None and > 0 and otherwise raise a
ValueError (or use the existing Pydantic request model validation) so invalid
values are rejected client-side—update the checks around the time_unit parameter
in update_ticket and add_article (and any similar blocks that build payloads
from time_unit) to perform this validation before including time_unit in the
request payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 020c0df1-95b3-48be-b050-311ce0521270
📒 Files selected for processing (6)
mcp_zammad/client.pymcp_zammad/models.pymcp_zammad/server.pytests/test_client.pytests/test_client_methods.pytests/test_server.py
Comment on lines
+785
to
+818
| 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") | ||
|
|
Contributor
There was a problem hiding this comment.
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
time_unitparameter tozammad_update_ticketandzammad_add_articleMCP tools for Zammad time accounting supporttime_unitfield toTicketUpdateParams,TicketUpdate, andArticleCreatePydantic models with validation (must be > 0)time_unitparameter toclient.update_ticket()andclient.add_article()methods, only included in API payload when not NoneTest plan
update_ticketwithtime_unit(client and server level)add_articlewithtime_unit(client and server level)time_unit=0andtime_unit=-5time_unitnot sent in payload whenNoneSummary by CodeRabbit