MPT-18374 Add Helpdesk Forms API services and tests#245
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (9)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a Helpdesk Form resource: new 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.
🧹 Nitpick comments (2)
tests/unit/resources/helpdesk/test_forms.py (1)
18-27: Consider simplifying assertion pattern.The intermediate
resultvariable for boolean comparisons adds unnecessary indirection. Direct assertions would be more idiomatic:✨ Proposed simplification
def test_endpoint(forms_service): - result = forms_service.path == "/public/v1/helpdesk/forms" - - assert result is True + assert forms_service.path == "/public/v1/helpdesk/forms" def test_async_endpoint(async_forms_service): - result = async_forms_service.path == "/public/v1/helpdesk/forms" - - assert result is True + assert async_forms_service.path == "/public/v1/helpdesk/forms"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/helpdesk/test_forms.py` around lines 18 - 27, The tests test_endpoint and test_async_endpoint create an unnecessary intermediate boolean variable `result`; remove `result` and assert the path directly by replacing the two blocks with direct assertions (e.g. use `assert forms_service.path == "/public/v1/helpdesk/forms"` and `assert async_forms_service.path == "/public/v1/helpdesk/forms"`), eliminating the redundant `result` assignment and `assert result is True`.mpt_api_client/resources/helpdesk/forms.py (1)
11-12: Consider whetherFormmodel needs additional typed fields.The
Formmodel currently only inherits fromModelwithout defining any Form-specific attributes. While this works (attributes are dynamically populated from API responses), adding type hints for common fields likename,description,statuswould improve IDE support and documentation.This is optional and can be deferred if the API schema is still evolving.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/resources/helpdesk/forms.py` around lines 11 - 12, The Form model currently just subclasses Model with no typed attributes; add explicit typed fields (e.g., name: str, description: Optional[str], status: Optional[str] or an Enum, id: Optional[int/str>, created_at: Optional[datetime]) to the Form class to improve IDE/type-checker support and documentation while keeping flexibility for evolving API schemas; update the Form class definition (class Form(Model):) to declare these attributes with appropriate typing and defaults/Optionals and import any needed types (Optional, datetime, Enum) so IDEs and static analysis pick up common form fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mpt_api_client/resources/helpdesk/forms.py`:
- Around line 11-12: The Form model currently just subclasses Model with no
typed attributes; add explicit typed fields (e.g., name: str, description:
Optional[str], status: Optional[str] or an Enum, id: Optional[int/str>,
created_at: Optional[datetime]) to the Form class to improve IDE/type-checker
support and documentation while keeping flexibility for evolving API schemas;
update the Form class definition (class Form(Model):) to declare these
attributes with appropriate typing and defaults/Optionals and import any needed
types (Optional, datetime, Enum) so IDEs and static analysis pick up common form
fields.
In `@tests/unit/resources/helpdesk/test_forms.py`:
- Around line 18-27: The tests test_endpoint and test_async_endpoint create an
unnecessary intermediate boolean variable `result`; remove `result` and assert
the path directly by replacing the two blocks with direct assertions (e.g. use
`assert forms_service.path == "/public/v1/helpdesk/forms"` and `assert
async_forms_service.path == "/public/v1/helpdesk/forms"`), eliminating the
redundant `result` assignment and `assert result is True`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4bef60f5-0553-41ea-b59b-66a9197de78e
📒 Files selected for processing (8)
mpt_api_client/resources/helpdesk/forms.pympt_api_client/resources/helpdesk/helpdesk.pytests/e2e/helpdesk/forms/__init__.pytests/e2e/helpdesk/forms/conftest.pytests/e2e/helpdesk/forms/test_async_forms.pytests/e2e/helpdesk/forms/test_sync_forms.pytests/unit/resources/helpdesk/test_forms.pytests/unit/resources/helpdesk/test_helpdesk.py
ab8ec41 to
ea01cda
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/helpdesk/forms/test_async_forms.py (1)
53-56: Strengthen delete test with a post-delete check.This currently passes if delete returns without raising, even if the record still exists. Add a follow-up read assertion to validate effect.
Proposed test hardening
`@pytest.mark.skip`(reason="Unskip after MPT-19124 completed") async def test_delete_form(async_mpt_ops, async_created_form): - await async_mpt_ops.helpdesk.forms.delete(async_created_form.id) # act + await async_mpt_ops.helpdesk.forms.delete(async_created_form.id) + with pytest.raises(MPTAPIError): + await async_mpt_ops.helpdesk.forms.get(async_created_form.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpdesk/forms/test_async_forms.py` around lines 53 - 56, The delete test (test_delete_form) only calls async_mpt_ops.helpdesk.forms.delete(async_created_form.id) and must be hardened by adding a post-delete verification: after calling async_mpt_ops.helpdesk.forms.delete, attempt to fetch the same form (e.g., async_mpt_ops.helpdesk.forms.read or get with async_created_form.id) and assert the expected failure (raise of a NotFound/404 exception or a None/absent result) to confirm the record was actually removed; update the test to await that read call and assert the proper absent/exception behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/helpdesk/forms/test_async_forms.py`:
- Around line 53-56: The delete test (test_delete_form) only calls
async_mpt_ops.helpdesk.forms.delete(async_created_form.id) and must be hardened
by adding a post-delete verification: after calling
async_mpt_ops.helpdesk.forms.delete, attempt to fetch the same form (e.g.,
async_mpt_ops.helpdesk.forms.read or get with async_created_form.id) and assert
the expected failure (raise of a NotFound/404 exception or a None/absent result)
to confirm the record was actually removed; update the test to await that read
call and assert the proper absent/exception behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2469cf58-e696-43b3-802c-5fab3eea8b17
📒 Files selected for processing (8)
mpt_api_client/resources/helpdesk/forms.pympt_api_client/resources/helpdesk/helpdesk.pytests/e2e/helpdesk/forms/__init__.pytests/e2e/helpdesk/forms/conftest.pytests/e2e/helpdesk/forms/test_async_forms.pytests/e2e/helpdesk/forms/test_sync_forms.pytests/unit/resources/helpdesk/test_forms.pytests/unit/resources/helpdesk/test_helpdesk.py
✅ Files skipped from review due to trivial changes (5)
- tests/unit/resources/helpdesk/test_forms.py
- tests/e2e/helpdesk/forms/test_sync_forms.py
- tests/e2e/helpdesk/forms/init.py
- tests/e2e/helpdesk/forms/conftest.py
- mpt_api_client/resources/helpdesk/forms.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/resources/helpdesk/test_helpdesk.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/resources/helpdesk/helpdesk.py`:
- Around line 48-52: The Helpdesk and AsyncHelpdesk classes now exceed the
WPS214 method limit due to the new forms property; to fix CI, add a per-class
suppression for the wemake rule by annotating the Helpdesk and AsyncHelpdesk
class definitions with a noqa for WPS214 (e.g., append "# noqa: WPS214" to each
class line) or alternatively refactor by consolidating service accessors (e.g.,
create a single services container or a single getter method that returns
FormsService and the other services) so the number of explicit
methods/properties drops below the WPS214 threshold; update the class
declarations or refactor the accessors accordingly referencing the
Helpdesk/AsyncHelpdesk classes and the forms property.
🪄 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: 428f686f-6d76-4299-9745-10659db201a4
📒 Files selected for processing (8)
mpt_api_client/resources/helpdesk/forms.pympt_api_client/resources/helpdesk/helpdesk.pytests/e2e/helpdesk/forms/__init__.pytests/e2e/helpdesk/forms/conftest.pytests/e2e/helpdesk/forms/test_async_forms.pytests/e2e/helpdesk/forms/test_sync_forms.pytests/unit/resources/helpdesk/test_forms.pytests/unit/resources/helpdesk/test_helpdesk.py
✅ Files skipped from review due to trivial changes (3)
- tests/e2e/helpdesk/forms/init.py
- tests/e2e/helpdesk/forms/conftest.py
- mpt_api_client/resources/helpdesk/forms.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/resources/helpdesk/test_helpdesk.py
- tests/e2e/helpdesk/forms/test_sync_forms.py
- tests/e2e/helpdesk/forms/test_async_forms.py
|
d3rky
approved these changes
Mar 25, 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
/public/v1/helpdesk/formspublishandunpublishresource actions on formshelpdesk.forms,async_helpdesk.forms)Testing
uv run pytest tests/unit/resources/helpdesk/test_forms.py tests/unit/resources/helpdesk/test_helpdesk.pymake check-alluv run pytest tests/unit/resources/helpdesk/test_forms.pyCloses MPT-18374