Add Forms tools — full OCS v3 coverage (25 tools)#47
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new Nextcloud Forms MCP module was added and registered, the Nextcloud client gained JSON-capable PATCH/PUT helpers, integration tests and CI were updated to include Forms, and documentation/progress files were updated to list the new Forms tools and counts. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MCP as MCP Server
participant Forms as Forms Tools
participant Client as NextcloudClient
participant API as Nextcloud OCS API
User->>MCP: Invoke Forms tool
MCP->>Forms: Route request to Forms handler
Forms->>Forms: Validate input & permissions
alt Read
Forms->>Client: ocs_get / ocs_list
else Create/Update
Forms->>Client: ocs_post_json / ocs_patch_json / ocs_put_json
else Delete
Forms->>Client: ocs_delete
end
Client->>API: HTTP request to /ocs/v3/apps/forms/*
API-->>Client: JSON response
Client->>Client: Validate & parse ocs.status/ocs.data
Client-->>Forms: Return parsed data
Forms-->>MCP: Tool result JSON
MCP-->>User: Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 96.29% 95.74% -0.55%
==========================================
Files 28 29 +1
Lines 2859 3105 +246
==========================================
+ Hits 2753 2973 +220
- Misses 106 132 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
(AI) CI green on both NC 32 and NC 33 after installing the Forms app in the workflow. Marked ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/nc_mcp_server/tools/forms.py (2)
326-335: Refetch-after-PATCH is intentional but O(form) per option update — worth a brief note.The pattern here (PATCH returns only the modified id, so we GET the parent question and linearly scan its options) is consistent with the PR's stated design. Just flagging that for a form with many options this pulls the full question payload on every
update_optioncall. Fine for typical form sizes; if you later add bulk edits, consider a single batched refetch.Same pattern in
update_form_share(lines 408-416) refetching the whole form — an individual share endpoint GET would be cheaper if one exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/forms.py` around lines 326 - 335, Add a brief explanatory comment in update_option (around the call to client.ocs_patch_json and the subsequent client.ocs_get/option scan) and similarly in update_form_share where the full form is refetched, stating that the PATCH returns only an id so we intentionally GET the parent (O(number_of_options) / O(form) cost), and note that this is acceptable for typical sizes but recommend future optimizations (e.g., returning the updated option/share in the PATCH response, adding a dedicated GET endpoint for a single option/share, or batching updates) so maintainers understand the tradeoff.
223-239: Remove thesubtypeparameter—it's not supported by the Forms API.The Nextcloud Forms OCS v3 API documentation (
docs/API_v3.md) does not includesubtypeas a parameter for POST/api/v3/forms/{formId}/questions. Grid questions are configured viaoptions(rows) andextraSettings(columns), not asubtypefield. The current validation and the proposed refactor both assumesubtypeis an API-supported parameter, but it is not. Sending unsupported fields to the API risks silent failures or unexpected behavior.Either remove the
subtypeparameter entirely, or (if cell-type selection is intended as a feature) implement it locally by translatingsubtypeinto the appropriate grid configuration after question creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/forms.py` around lines 223 - 239, The code in the question-creation flow incorrectly accepts and sends a non-supported "subtype" field to the Forms API: remove the subtype parameter usage and validation from the function that builds the request (remove checks against GRID_SUBTYPES and the branch that sets body["subtype"]), stop raising/expecting subtype for question_type == "grid", and ensure the request body only contains supported fields (e.g., "type", "text", "fromId", and grid configuration should be supplied via "options" / "extraSettings" if needed); if you need to map a user-provided subtype to grid configuration, implement that translation locally (e.g., convert subtype into appropriate options/extraSettings) before calling client.ocs_post_json in the code that prepares the POST to apps/forms/api/v3/forms/{form_id}/questions.tests/integration/test_forms.py (3)
273-281: Minor:_build_submittable_formdoesn't useself.It's defined as an instance method but has no dependency on
self; promoting it to@staticmethod(or moving it to a module-level helper next to_make_form) avoids the implicit receiver and makes reuse from other classes straightforward if needed later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_forms.py` around lines 273 - 281, The method _build_submittable_form does not use self, so mark it as a `@staticmethod` (or move it to a module-level helper next to _make_form); update its definition to include the `@staticmethod` decorator and ensure any internal references remain unchanged (function name: _build_submittable_form, helper: _make_form, parameter: nc_mcp) so callers use ClassName._build_submittable_form(...) or import the moved helper as needed.
34-38: Stale test name after parameter rename.
test_list_without_type_returns_liststill references the oldtypeparameter name; per the PR summary this was renamed toownership. Considertest_list_without_ownership_returns_listfor consistency with the new public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_forms.py` around lines 34 - 38, The test function name test_list_without_type_returns_list is stale after the parameter rename; rename the test to test_list_without_ownership_returns_list and update any references (e.g., in test discovery or CI filters) so it matches the new public API; keep the body using _make_form(nc_mcp, "mcp-test-list-all") and the nc_mcp.call("list_forms") assertion unchanged.
303-315: Ordering assumption in single-submission delete test.The test picks
listing["submissions"][0]["id"]without asserting what orderlist_submissionsreturns. It's fine today because the assertion is only on the resulting count, but if you later extend this test to check which submission survived, the ordering of the Forms OCS v3 response will matter. Worth a short comment here, or an explicit sort byid/timestamp, to guard against future flakiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_forms.py` around lines 303 - 315, In test_delete_single_submission, avoid assuming list_submissions returns a stable order: before selecting first_id from listing["submissions"][0]["id"], either sort listing["submissions"] by a deterministic key (e.g., submission "id" or "createdAt"/timestamp) or add an explicit assertion about the returned order; update the code in test_delete_single_submission to call list_submissions, then sort the resulting list by the chosen key and use the first element's "id" for delete_submission (or add a clarifying comment if you prefer to assert ordering), referencing the test function name and the list_submissions/delete_submission calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 371: The README entry for `list_forms` is incorrect: the tool
`list_forms` (see docstring in src/nc_mcp_server/tools/forms.py and the function
handling ownership) does not accept "all" — omitting the ownership argument
returns the merged view; update the README table row from "`owned / shared /
all`" to something like "`owned / shared (omit for both)`" to match the
`list_forms` behavior, and if you prefer the README to accept "all" instead,
modify the `list_forms` ownership handling to map `"all"` to no `?type=` query
before forwarding to the server so `type=all` is never sent.
In `@src/nc_mcp_server/tools/forms.py`:
- Around line 30-45: The doc/behaviour mismatch: list_forms currently treats any
ownership string (e.g., "all") as a raw type sent to the API; update list_forms
to accept "all" as an explicit alias for the merged result by mapping ownership
== "all" to None before building params so we don't forward type=all to the API,
and update the list_forms docstring to mention that "all" is accepted as an
alias for omitting the ownership filter; locate the handling in the list_forms
function (client = get_client(); params = ...) and adjust the params logic
accordingly.
- Around line 440-444: The return logic currently treats empty dicts/lists as
"no data" because it uses a truthy check; update the conditional in the
submission flow that calls client.ocs_post_json (the block that POSTS to
"apps/forms/api/v3/forms/{form_id}/submissions") to test specifically for None
(e.g., "if data is None") so that non-null empty payloads ({} or []) are
returned via json.dumps(data) and only a true None falls back to '{"status":
"submitted"}'; keep the call to client.ocs_post_json and the json.dumps(data)
behavior but change the condition to an explicit None check.
In `@tests/integration/test_forms.py`:
- Around line 43-47: The test test_create_produces_form_with_id currently
asserts a hardcoded ownerId "admin" which is fragile; update the assertion to
validate ownership more robustly by either (a) asserting created["ownerId"] is a
non-empty string (e.g. isinstance(created["ownerId"], str) and
created["ownerId"]) or (b) comparing against the expected user sourced from the
test helper (use the caller nc_mcp’s configured user or method that returns the
expected test account) so the check uses nc_mcp’s configuration rather than a
hardcoded "admin".
- Around line 340-348: Replace the hardcoded form_id=1 in
test_read_only_blocks_delete and test_write_blocks_delete by creating a real
form first via an admin-scoped helper and using its returned id so the tests
only exercise permission checks; e.g., call the admin client
(nc_mcp_admin.call("create_form", ...)) or a shared helper (create_form(...)) to
create a form, capture the response id, and pass that id into
nc_mcp_read_only.call("delete_form", form_id=<created_id>) and
nc_mcp_write.call("delete_form", form_id=<created_id>) respectively so the
expected ToolError with r"[Pp]ermission" is the only failure path.
---
Nitpick comments:
In `@src/nc_mcp_server/tools/forms.py`:
- Around line 326-335: Add a brief explanatory comment in update_option (around
the call to client.ocs_patch_json and the subsequent client.ocs_get/option scan)
and similarly in update_form_share where the full form is refetched, stating
that the PATCH returns only an id so we intentionally GET the parent
(O(number_of_options) / O(form) cost), and note that this is acceptable for
typical sizes but recommend future optimizations (e.g., returning the updated
option/share in the PATCH response, adding a dedicated GET endpoint for a single
option/share, or batching updates) so maintainers understand the tradeoff.
- Around line 223-239: The code in the question-creation flow incorrectly
accepts and sends a non-supported "subtype" field to the Forms API: remove the
subtype parameter usage and validation from the function that builds the request
(remove checks against GRID_SUBTYPES and the branch that sets body["subtype"]),
stop raising/expecting subtype for question_type == "grid", and ensure the
request body only contains supported fields (e.g., "type", "text", "fromId", and
grid configuration should be supplied via "options" / "extraSettings" if
needed); if you need to map a user-provided subtype to grid configuration,
implement that translation locally (e.g., convert subtype into appropriate
options/extraSettings) before calling client.ocs_post_json in the code that
prepares the POST to apps/forms/api/v3/forms/{form_id}/questions.
In `@tests/integration/test_forms.py`:
- Around line 273-281: The method _build_submittable_form does not use self, so
mark it as a `@staticmethod` (or move it to a module-level helper next to
_make_form); update its definition to include the `@staticmethod` decorator and
ensure any internal references remain unchanged (function name:
_build_submittable_form, helper: _make_form, parameter: nc_mcp) so callers use
ClassName._build_submittable_form(...) or import the moved helper as needed.
- Around line 34-38: The test function name test_list_without_type_returns_list
is stale after the parameter rename; rename the test to
test_list_without_ownership_returns_list and update any references (e.g., in
test discovery or CI filters) so it matches the new public API; keep the body
using _make_form(nc_mcp, "mcp-test-list-all") and the nc_mcp.call("list_forms")
assertion unchanged.
- Around line 303-315: In test_delete_single_submission, avoid assuming
list_submissions returns a stable order: before selecting first_id from
listing["submissions"][0]["id"], either sort listing["submissions"] by a
deterministic key (e.g., submission "id" or "createdAt"/timestamp) or add an
explicit assertion about the returned order; update the code in
test_delete_single_submission to call list_submissions, then sort the resulting
list by the chosen key and use the first element's "id" for delete_submission
(or add a clarifying comment if you prefer to assert ordering), referencing the
test function name and the list_submissions/delete_submission calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc6d6bba-4f93-40ee-bc7c-e939141fe830
📒 Files selected for processing (9)
.github/workflows/tests-integration.ymlPROGRESS.mdREADME.mdsrc/nc_mcp_server/client.pysrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/forms.pytests/integration/conftest.pytests/integration/test_forms.pytests/integration/test_server.py
| async def list_forms(ownership: str | None = None) -> str: | ||
| """List forms visible to the current user. | ||
|
|
||
| Args: | ||
| ownership: Filter by ownership. One of: "owned" (forms I created), | ||
| "shared" (forms shared with me). Omit to get both merged. | ||
|
|
||
| Returns: | ||
| JSON array of form summaries. Each entry includes id, hash, title, | ||
| state (0=active, 1=closed, 2=archived), permissions, and metadata. | ||
| Call get_form(id) for full details including questions. | ||
| """ | ||
| client = get_client() | ||
| params = {"type": ownership} if ownership else None | ||
| data = await client.ocs_get("apps/forms/api/v3/forms", params=params) | ||
| return json.dumps(data) |
There was a problem hiding this comment.
Minor doc mismatch with README: list_forms accepts "owned"/"shared" (omit for both), not "all".
README (line 371) advertises owned / shared / all, but this docstring correctly says omit-to-merge. Either document "all" as an explicit alias (and map it to None), or update the README so users aren't confused into passing ownership="all" (which this tool will forward verbatim to the API as type=all).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nc_mcp_server/tools/forms.py` around lines 30 - 45, The doc/behaviour
mismatch: list_forms currently treats any ownership string (e.g., "all") as a
raw type sent to the API; update list_forms to accept "all" as an explicit alias
for the merged result by mapping ownership == "all" to None before building
params so we don't forward type=all to the API, and update the list_forms
docstring to mention that "all" is accepted as an alias for omitting the
ownership filter; locate the handling in the list_forms function (client =
get_client(); params = ...) and adjust the params logic accordingly.
| async def test_create_produces_form_with_id(self, nc_mcp: McpTestHelper) -> None: | ||
| created = json.loads(await nc_mcp.call("create_form")) | ||
| assert isinstance(created["id"], int) | ||
| assert created["ownerId"] == "admin" | ||
| assert created["hash"] |
There was a problem hiding this comment.
Fragile assertion on hardcoded ownerId == "admin".
Line 46 assumes the integration test user is admin. If the test harness ever runs under a different user (e.g., a dedicated test account or in a shared CI setup), this assertion will fail even though the tool is behaving correctly. Consider asserting isinstance(created["ownerId"], str) and created["ownerId"] or reading the expected user from the same config nc_mcp uses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_forms.py` around lines 43 - 47, The test
test_create_produces_form_with_id currently asserts a hardcoded ownerId "admin"
which is fragile; update the assertion to validate ownership more robustly by
either (a) asserting created["ownerId"] is a non-empty string (e.g.
isinstance(created["ownerId"], str) and created["ownerId"]) or (b) comparing
against the expected user sourced from the test helper (use the caller nc_mcp’s
configured user or method that returns the expected test account) so the check
uses nc_mcp’s configuration rather than a hardcoded "admin".
| @pytest.mark.asyncio | ||
| async def test_read_only_blocks_delete(self, nc_mcp_read_only: McpTestHelper) -> None: | ||
| with pytest.raises(ToolError, match=r"[Pp]ermission"): | ||
| await nc_mcp_read_only.call("delete_form", form_id=1) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_write_blocks_delete(self, nc_mcp_write: McpTestHelper) -> None: | ||
| with pytest.raises(ToolError, match=r"[Pp]ermission"): | ||
| await nc_mcp_write.call("delete_form", form_id=1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that the delete_form tool enforces permissions before any form lookup/fetch,
# so form_id=1 never needs to exist for the permission tests to pass.
fd -t f 'forms.py' src/nc_mcp_server/tools | xargs -I{} sh -c 'echo "=== {} ==="; cat {}' | sed -n '1,400p'
rg -nP -C5 '\bdelete_form\b' src/nc_mcp_server/tools/forms.py || true
rg -nP -C3 'permission|require_write|require_delete|@\w*permission' src/nc_mcp_server/tools/forms.py || trueRepository: cloud-py-api/nc_mcp_server
Length of output: 25495
Use a real form ID instead of hardcoded form_id=1 to make permission tests resilient to future changes.
test_read_only_blocks_delete and test_write_blocks_delete currently rely on form_id=1 not existing, so the permission check fires first. If future refactoring fetches the form before checking permissions, a NotFound error would match differently than the expected r"[Pp]ermission" pattern, masking the test intent.
Create a real form using an admin-scoped helper and pass its id so the only failure path is the permission layer, making the test robust to code reordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_forms.py` around lines 340 - 348, Replace the
hardcoded form_id=1 in test_read_only_blocks_delete and test_write_blocks_delete
by creating a real form first via an admin-scoped helper and using its returned
id so the tests only exercise permission checks; e.g., call the admin client
(nc_mcp_admin.call("create_form", ...)) or a shared helper (create_form(...)) to
create a form, capture the response id, and pass that id into
nc_mcp_read_only.call("delete_form", form_id=<created_id>) and
nc_mcp_write.call("delete_form", form_id=<created_id>) respectively so the
expected ToolError with r"[Pp]ermission" is the only failure path.
…submit_form fallback
|
(AI) Thanks CodeRabbit. Addressed the two real findings in f12839f:
Declined:
|
- Remove 'datetime' from QUESTION_TYPES (Nextcloud now rejects it) - Add 'color' and 'linearscale' question types that were missing from the catalog - list_forms with no ownership now fans out to type=owned and type=shared and merges the results (NC's endpoint defaults to 'owned' only, so omitting the filter previously gave half of what the docstring promised) - Soften create_options docstring: the server accepts options on non-choice questions (no-op), so it's not a pre-condition
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 `@tests/integration/test_forms.py`:
- Around line 37-39: The local variable assignment forms = json.loads(await
nc_mcp.call("list_forms")) causes pyright reportUnknownArgumentType because
json.loads returns Any; explicitly annotate the local to restore typing (e.g.
change to forms: list[dict[str, Any]] = json.loads(...)) and add the necessary
typing import (Any) if not already present; apply the same explicit annotation
pattern to other similar locals (questions, other forms assignments) in the test
to keep consistency and silence the pyright warning, referencing the variable
name forms and the json.loads(...) call to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d677f558-40f0-434c-8465-8d5b0573115c
📒 Files selected for processing (4)
PROGRESS.mdREADME.mdsrc/nc_mcp_server/tools/forms.pytests/integration/test_forms.py
✅ Files skipped from review due to trivial changes (1)
- PROGRESS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nc_mcp_server/tools/forms.py
1 participant
Summary
Adds 25 tools covering the Nextcloud Forms app (surveys, polls, questionnaires) using the OCS v3 API. Full feature parity with the Forms web UI — create forms, add questions and options, share with users/groups/links, collect and export submissions.
Why Forms over Tables: analysis showed Tables' OCS v2 API is incomplete (row/column/view CRUD requires the non-OCS v1 REST API), which breaks the "OCS-only" invariant we've maintained since skipping Deck. Forms is 100% OCS v3 — all 12 endpoints under
/ocs/v2.php/apps/forms/api/v3/.Tools
list_forms,get_form,list_questions,get_question,list_submissions,get_submissioncreate_form,update_form,create_question,update_question,reorder_questions,create_options,update_option,reorder_options,create_form_share,update_form_share,submit_form,update_submission,export_submissionsdelete_form,delete_question,delete_option,delete_form_share,delete_submission,delete_all_submissionsDesign notes
PATCHendpoints return just the modified id (int). Update tools refetch viaGETso AI callers receive the full updated object — one round-trip more, but the response is actionable without a follow-up call.ocs_patch_jsonandocs_put_jsonto the HTTP client (Forms bodies useapplication/json; existingocs_patch/ocs_putsend form-encoded).typeparameter onlist_forms→ownershipand oncreate_question→question_typeto avoid shadowing a Python builtin (A002). Docs and docstrings updated accordingly.mcp-test-title prefix.Total tool count: 101 → 126.
Test plan
test_server.py::test_all_tools_registeredsees all 25 new toolsSummary by CodeRabbit
New Features
Tests
Chores
Documentation