MPT-18370 Add helpdesk queues API services with unit and e2e test coverage#243
Merged
MPT-18370 Add helpdesk queues API services with unit and e2e test coverage#243
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR introduces a new Helpdesk Queues resource to the API client, adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/resources/helpdesk/test_queues.py (2)
44-64: Add sync “with payload” action tests for parity with async coverage.Right now only async actions assert request-body propagation; adding the same assertion path for sync actions will make regression detection stronger for both service variants.
💡 Suggested additional test
+@pytest.mark.parametrize( + ("action", "input_data"), + [ + ("activate", {"id": "HQU-1234-5678", "status": "Active"}), + ("disable", {"id": "HQU-1234-5678", "status": "Disabled"}), + ], +) +def test_custom_resource_actions_with_data(queues_service, action, input_data): + response_expected_data = {"id": "HQU-1234-5678", "status": "Updated"} + with respx.mock: + mock_route = respx.post( + f"https://api.example.com/public/v1/helpdesk/queues/HQU-1234-5678/{action}" + ).mock( + return_value=httpx.Response( + status_code=httpx.codes.OK, + headers={"content-type": "application/json"}, + json=response_expected_data, + ) + ) + + result = getattr(queues_service, action)("HQU-1234-5678", input_data) + + assert mock_route.call_count == 1 + request = mock_route.calls[0].request + assert request.content == b'{"id":"HQU-1234-5678","status":"Active"}' if action == "activate" else b'{"id":"HQU-1234-5678","status":"Disabled"}' + assert result.to_dict() == response_expected_data + assert isinstance(result, Queue)Also applies to: 67-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/helpdesk/test_queues.py` around lines 44 - 64, The sync tests (test_custom_resource_actions_no_data / tests/unit/resources/helpdesk/test_queues.py) only cover no-body calls; add matching "with payload" sync tests that mirror the async coverage by calling getattr(queues_service, action) with a payload dict, mocking the POST to the same URL and asserting mock_route.call_count == 1, that request.content equals the JSON-encoded payload bytes (not b""), and that the returned result.to_dict() matches the mocked response and is an instance of Queue; repeat for both actions ("activate", "disable") to achieve parity with the existing async payload assertions.
8-11: Avoid brittle raw-byte JSON assertions in async payload checks.Comparing
request.contentto hardcoded bytes can fail on semantically equivalent serialization differences (key order/encoding). Prefer asserting parsed JSON content.💡 Suggested change
+import json ... -def _request_content(action: str) -> bytes: - if action == "activate": - return b'{"id":"HQU-1234-5678","status":"Active"}' - return b'{"id":"HQU-1234-5678","status":"Disabled"}' - ... - request_expected_content = _request_content(action) ... - assert request.content == request_expected_content + assert json.loads(request.content.decode()) == input_dataAlso applies to: 75-76, 91-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/helpdesk/test_queues.py` around lines 8 - 11, The tests use brittle byte-string equality by comparing request.content to hardcoded bytes in the helper _request_content and in the test assertions; replace those raw-byte comparisons with JSON parsing and structural assertions: decode/parse request.content using json.loads (or json.loads(request.content.decode())) and compare dictionaries or specific fields (e.g., ["id"], ["status"]) instead of exact byte sequences; update the helper _request_content usages and the two other assertions that compare request.content so they assert on parsed JSON objects or keys rather than hardcoded bytes.
🤖 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/e2e/helpdesk/queues/conftest.py`:
- Around line 17-22: The teardown in the created_queue fixture currently calls
mpt_ops.helpdesk.queues.delete(queue.id) unconditionally which can double-delete
if the test already removed the queue; change the teardown to be best-effort by
wrapping the delete in a try/except that catches and swallows not-found/HTTP
errors (or check existence via mpt_ops.helpdesk.queues.get before deleting) so
teardown never raises on already-deleted resources; apply the same pattern to
the other fixture(s) that call mpt_ops.helpdesk.queues.delete in teardown.
---
Nitpick comments:
In `@tests/unit/resources/helpdesk/test_queues.py`:
- Around line 44-64: The sync tests (test_custom_resource_actions_no_data /
tests/unit/resources/helpdesk/test_queues.py) only cover no-body calls; add
matching "with payload" sync tests that mirror the async coverage by calling
getattr(queues_service, action) with a payload dict, mocking the POST to the
same URL and asserting mock_route.call_count == 1, that request.content equals
the JSON-encoded payload bytes (not b""), and that the returned result.to_dict()
matches the mocked response and is an instance of Queue; repeat for both actions
("activate", "disable") to achieve parity with the existing async payload
assertions.
- Around line 8-11: The tests use brittle byte-string equality by comparing
request.content to hardcoded bytes in the helper _request_content and in the
test assertions; replace those raw-byte comparisons with JSON parsing and
structural assertions: decode/parse request.content using json.loads (or
json.loads(request.content.decode())) and compare dictionaries or specific
fields (e.g., ["id"], ["status"]) instead of exact byte sequences; update the
helper _request_content usages and the two other assertions that compare
request.content so they assert on parsed JSON objects or keys rather than
hardcoded bytes.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e599e889-9fec-4196-a025-e261cf60733f
📒 Files selected for processing (7)
mpt_api_client/resources/helpdesk/helpdesk.pympt_api_client/resources/helpdesk/queues.pytests/e2e/helpdesk/queues/conftest.pytests/e2e/helpdesk/queues/test_async_queues.pytests/e2e/helpdesk/queues/test_sync_queues.pytests/unit/resources/helpdesk/test_helpdesk.pytests/unit/resources/helpdesk/test_queues.py
|
robcsegal
approved these changes
Mar 23, 2026
This was referenced Mar 24, 2026
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
2 participants
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
Queueresource with sync and asyncQueuesServiceimplementations under Helpdeskactivateanddisablequeuesaccessors into sync and async Helpdesk module entrypointsTesting
uv run pytest tests/unit/resources/helpdesk/test_queues.py tests/unit/resources/helpdesk/test_helpdesk.pymake check-allCloses MPT-18370
Changes
Queuemodel andQueuesService(synchronous) andAsyncQueuesService(asynchronous) implementations under the Helpdesk module/public/v1/helpdesk/queues) with collection keydataactivate()anddisable()methods for both sync and async services, supporting optional resource data payloadqueuesproperty accessors to bothHelpdeskandAsyncHelpdeskentry points for accessing the queue services