Skip to content

fix(client): request expand=true when fetching a single ticket - #321

Open
jovamateus26 wants to merge 1 commit into
basher83:mainfrom
jovamateus26:fix/get-ticket-expand
Open

fix(client): request expand=true when fetching a single ticket#321
jovamateus26 wants to merge 1 commit into
basher83:mainfrom
jovamateus26:fix/get-ticket-expand

Conversation

@jovamateus26

@jovamateus26 jovamateus26 commented Aug 22, 2026

Copy link
Copy Markdown

Fixes #319

Problem

zammad_get_ticket — and the zammad://ticket/{id} resource, which shares the same client method — renders State, Priority, Group, Owner and Customer as Unknown for every ticket.

get_ticket fetched the ticket with api.ticket.find(ticket_id), and zammad_py's Resource.find() accepts no filters, so expand=true never reached Zammad. Without it the API returns only *_id fields, the optional Ticket fields stay None, and _brief_field falls back to "Unknown".

search_tickets was unaffected because it passes filters={..., "expand": "true"} — this is the one call site the earlier expand work (tests/test_expand_param_fix.py) could not cover, since find() has no filters parameter at all.

Change

Fetch the ticket through zammad_py's session with params={"expand": "true"}, in a small _find_ticket_expanded helper. This follows the pattern list_tags already uses for the tag_list endpoint, and keeps the lowercase "true" that the existing tests care about (Zammad is case-sensitive here and requests serializes bool True as "True").

On failure it raises requests.HTTPError(response.text) — same shape zammad_py produces — so the server's TicketIdGuidanceError mapping, which looks for "couldn't find" in the message, still fires for a bad ticket id.

No model changes were needed: Ticket already declares group: GroupBrief | str | None and friends, with a comment noting the string form arrives when expand=true.

Tests

  • Updated the two existing get_ticket tests to mock the session response instead of ticket.find.
  • test_get_ticket_requests_expanded_fields — asserts the exact URL and that expand is the literal "true", and that the expanded names survive.
  • test_get_ticket_not_found_raises_with_zammad_message — asserts Zammad's error body survives, so the ticket-id guidance keeps working.

224 passed, coverage 88.47% (gate 86%). ruff check, ruff format --check and mypy mcp_zammad all clean on the touched files.

Verification against a real instance

Zammad 6.5, ticket #27003, before and after:

**State**: Unknown        ->  **State**: open
**Priority**: Unknown     ->  **Priority**: 2 normal
**Group**: Unknown        ->  **Group**: CGT
**Owner**: Unknown        ->  **Owner**: -
**Customer**: Unknown     ->  **Customer**: jovanir

Note

Out of scope here, but flagged in the issue: _brief_field only checks isinstance(value, StateBrief | PriorityBrief | UserBrief), so a GroupBrief or OrganizationBrief object would still yield "Unknown". It does not bite with expand=true (Zammad sends plain strings), so I left it alone — happy to fold in a one-liner if you want it.

Summary by CodeRabbit

  • Bug Fixes
    • Ticket details now include expanded fields when retrieved.
    • Ticket lookup errors provide the service’s response message for clearer troubleshooting.
    • Existing article pagination behavior is preserved.

get_ticket called api.ticket.find(), and zammad_py's Resource.find()
accepts no filters, so expand=true was never sent. Zammad then returns
only the *_id fields, and _format_ticket_detail_markdown / the
zammad://ticket/{id} resource render State, Priority, Group, Owner and
Customer as "Unknown".

Fetch the ticket through zammad_py's session instead, following the same
pattern already used by list_tags, so the lowercase "true" expand flag
gets through. The Ticket model already accepts the expanded string form.

Verified against a live Zammad 6 instance: State/Priority/Group/Owner/
Customer now render real values instead of "Unknown".
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added type:bug Something is not working correctly area:mcp-tools area:ci-cd Continuous integration and deployment pipelines area:python Python development and tooling labels Aug 22, 2026
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a09faa80-887f-4318-bd68-a46ae54988da

📥 Commits

Reviewing files that changed from the base of the PR and between 8873b2e and 5b529fd.

📒 Files selected for processing (2)
  • mcp_zammad/client.py
  • tests/test_client_methods.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

get_ticket now requests expanded ticket data through the authenticated session. It preserves article pagination, returns expanded fields, and includes Zammad’s response body in HTTP errors. Tests cover request parameters, expanded fields, article handling, and failed lookups.

Changes

Expanded ticket retrieval

Layer / File(s) Summary
Expanded ticket fetch
mcp_zammad/client.py
get_ticket now uses a direct session request with expand=true. Failed responses raise requests.HTTPError with the response body.
Expanded ticket validation
tests/test_client_methods.py
Tests verify expanded fields, case-sensitive query parameters, article behavior, and Zammad error messages.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 5b529

The ticket-fetching path now requests expanded fields so State, Priority, Group, Owner, and Customer render correctly. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses conventional commit format and clearly describes the expanded ticket request fix.
Linked Issues check ✅ Passed The changes satisfy issue #319 by requesting expand=true, preserving expanded fields, and retaining server error text.
Out of Scope Changes check ✅ Passed The changes are limited to the ticket-fetching fix and its regression tests; no unrelated scope is present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 7 minor

Alerts:
⚠ 7 issues (≤ 0 issues of at least minor severity)

Results:
7 new issues

Category Results
Documentation 7 minor

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@jovamateus26

Copy link
Copy Markdown
Author

Heads-up on the two red/pending checks, neither of which I think is a defect in this patch — but both are your call.

1. The GitHub Actions workflows are waiting on you. Tests and Coverage, Security Scan, Codacy Security Scan and Build and Publish Docker Image are all sitting at action_required — the first-time-contributor gate on fork PRs. Nothing ran. Locally, on this branch: 224 passed, coverage 88.47% (gate 86%), and ruff check / ruff format --check / mypy mcp_zammad clean.

2. Codacy's 7 "new issues" are all Info-level pydocstyle findings that contradict this repo's own docstring convention. The full list:

Rule Where Message
D213 ×4 client.py:194, client.py:212, test_client_methods.py:371, test_client_methods.py:407 Multi-line docstring summary should start at the second line
D406 client.py:194 Section name should end with a newline (Raises, not Raises:)
D407 client.py:194 Missing dashed underline after section (Raises)
D413 client.py:194 Missing blank line after last section (Raises)

D213 fires on every multi-line docstring whose summary sits on the first line — which is the entire codebase. D406/D407 ask for numpy-style section headers (Raises + dashed underline) where the repo consistently uses Google style (Raises:).

The clean proof is in the sibling PR #322, which mostly deletes code: Codacy reports 12 issues "Fixed", and they are the same D213/D406/D407/D413 patterns — in code that was already on main. So Prospector is enforcing a convention this project doesn't follow anywhere, and the gate only counts it against lines a PR happens to touch.

I'd rather not adopt a foreign docstring style in one file just to clear the gate, but I'm not attached to that. Options, your pick:

  • Leave as is and treat the Info-level Documentation findings as noise (my suggestion).
  • I reformat this PR's docstrings to single-line summaries with the detail moved into # comments, which gets it to 0 added issues — at the cost of looking different from the rest of the file.
  • Tune the config so Prospector/pydocstyle matches the Google convention the repo actually uses (convention: google, which drops D213/D406/D407 and their friends). Happy to send that as a separate PR if you want it, but it's your quality gate, so I didn't touch it uninvited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci-cd Continuous integration and deployment pipelines area:python Python development and tooling type:bug Something is not working correctly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zammad_get_ticket returns "Unknown" for State, Priority, Group, Owner and Customer

1 participant