feat: support user_facing_error_messages for report result for task#450
feat: support user_facing_error_messages for report result for task#450lukasdotcom wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesTask result reporting
Estimated code review effort: 2 (Simple) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Signed-off-by: Lukas Schaefer <lukas@lschaefer.xyz>
2291990 to
fd84de5
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nc_py_api/ex_app/providers/task_processing.py`:
- Around line 172-179: The public synchronous and asynchronous result-reporting
methods need to document the new user_facing_error_message parameter. Update the
docstrings for both report_result methods to explain its purpose, how it relates
to error_message, and when it is shown to users; leave the existing typing and
forwarding behavior unchanged.
🪄 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: 5cea4338-bc09-4229-b947-be0b868ff4ec
📒 Files selected for processing (1)
nc_py_api/ex_app/providers/task_processing.py
| user_facing_error_message: str | None = None, | ||
| ) -> dict[str, typing.Any]: | ||
| """Report result of the task processing to Nextcloud.""" | ||
| with contextlib.suppress(NextcloudException): | ||
| if r := self._session.ocs( | ||
| "POST", | ||
| f"/ocs/v2.php/taskprocessing/tasks_provider/{task_id}/result", | ||
| json={"taskId": task_id, "output": output, "errorMessage": error_message}, | ||
| json={"taskId": task_id, "output": output, "errorMessage": error_message, "userFacingErrorMessage": user_facing_error_message}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document user_facing_error_message in both public methods.
The new public parameter is correctly typed and forwarded, but neither report_result docstring explains its purpose or relationship to error_message. Add parameter documentation to both synchronous and asynchronous methods so generated API docs accurately describe when this user-facing message is shown.
As per path instructions, public methods in nc_py_api/ex_app/** require attention to docstring quality.
Also applies to: 253-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nc_py_api/ex_app/providers/task_processing.py` around lines 172 - 179, The
public synchronous and asynchronous result-reporting methods need to document
the new user_facing_error_message parameter. Update the docstrings for both
report_result methods to explain its purpose, how it relates to error_message,
and when it is shown to users; leave the existing typing and forwarding behavior
unchanged.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nc_py_api/ex_app/providers/task_processing.py`:
- Around line 189-201: Extend the sync and async task-result API tests for their
respective result-reporting methods to pass user_facing_error_message and assert
the request payload contains it serialized as userFacingErrorMessage. Keep the
existing error_message assertions and cover both APIs with focused payload
checks.
🪄 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: f56318ba-9eda-4586-89d0-bf60afd940ed
📒 Files selected for processing (1)
nc_py_api/ex_app/providers/task_processing.py
| user_facing_error_message: str | None = None, | ||
| ) -> dict[str, typing.Any]: | ||
| """Report result of the task processing to Nextcloud.""" | ||
| with contextlib.suppress(NextcloudException): | ||
| if r := self._session.ocs( | ||
| "POST", | ||
| f"/ocs/v2.php/taskprocessing/tasks_provider/{task_id}/result", | ||
| json={"taskId": task_id, "output": output, "errorMessage": error_message}, | ||
| json={ | ||
| "taskId": task_id, | ||
| "output": output, | ||
| "errorMessage": error_message, | ||
| "userFacingErrorMessage": user_facing_error_message, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Add coverage for the new payload field in both APIs.
The supplied sync and async tests only pass error_message; they do not verify that user_facing_error_message is serialized as userFacingErrorMessage. Add focused tests for both methods.
Also applies to: 292-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nc_py_api/ex_app/providers/task_processing.py` around lines 189 - 201, Extend
the sync and async task-result API tests for their respective result-reporting
methods to pass user_facing_error_message and assert the request payload
contains it serialized as userFacingErrorMessage. Keep the existing
error_message assertions and cover both APIs with focused payload checks.
Changes proposed in this pull request:
Summary by CodeRabbit