feat/collections resource#64
Open
sunder-ch wants to merge 5 commits into
Open
Conversation
Adds a full Collections resource (create/list/retrieve/update/delete collections, add/list/search/move/remove pages within a collection, and collection member CRUD) matching the new external Collections API, plus `collection_id`/`parent_id` on CreatePage so pages can be created directly inside a collection or as a sub-page.
…ion listing returns root-branch pages only
📝 WalkthroughWalkthroughThe SDK adds collection models, collection CRUD, page and membership sub-resources, client wiring, collection-related page creation fields and query parameters, smoke tests, and a version bump to 0.2.21. ChangesCollections API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlaneClient
participant Collections
participant CollectionPages
participant HTTPAPI
PlaneClient->>Collections: invoke collection operation
Collections->>CollectionPages: invoke page operation
CollectionPages->>HTTPAPI: send request with serialized data
HTTPAPI-->>CollectionPages: return response
CollectionPages-->>PlaneClient: return validated model
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
| model_config = ConfigDict(extra="ignore", populate_by_name=True) | ||
|
|
||
| name: str | ||
| access: int | None = None |
Contributor
There was a problem hiding this comment.
Can we add enums for these
class CollectionAccessEnum(IntEnum):
PUBLIC = 0
PRIVATE = 1
| id: str | None = None | ||
| collection: str | None = None | ||
| member: str | None = None | ||
| access: int | None = None |
Contributor
There was a problem hiding this comment.
Can we add enums for these
class CollectionMemberAccessEnum(IntEnum):
VIEW = 0
COMMENT = 1
EDIT = 2
| from .base_resource import BaseResource | ||
|
|
||
|
|
||
| class Collections(BaseResource): |
Contributor
There was a problem hiding this comment.
Lets restructure this to match the current convention?
plane/api/collections/
base.py → Collections: list / retrieve / create / update / delete
pages.py → CollectionPages: list / add / search / update / remove
members.py → CollectionMembers: list / add / update / remove
Usage becomes client.collections.list(ws), client.collections.pages.list(ws, collection_id), client.collections.members.update(ws, collection_id, member_id, data)
| except Exception: | ||
| pass | ||
|
|
||
| def test_create_page_in_collection_and_sub_page( |
Contributor
There was a problem hiding this comment.
Can you please verify this test is passing on your end?
![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
🤖 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 `@plane/api/collections/base.py`:
- Around line 29-84: Append a trailing slash to every collection endpoint URL
used by the collection API methods in plane/api/collections/base.py lines 29-84,
every member endpoint in plane/api/collections/members.py lines 24-72, and every
page endpoint in plane/api/collections/pages.py lines 33-104. Update the URL
construction in the relevant list, create, retrieve, update, delete, and related
methods while preserving their existing parameters and behavior.
In `@plane/api/collections/members.py`:
- Around line 27-72: Rename the public CRUD methods consistently: in
plane/api/collections/members.py lines 27-41, rename add to create, and lines
64-72, rename remove to delete; in plane/api/collections/pages.py lines 39-53,
rename add to create, and lines 94-104, rename remove to delete. Preserve each
method’s existing behavior and signatures while applying the required CRUD
naming convention.
In `@plane/api/collections/pages.py`:
- Around line 55-70: The search method currently accepts a raw string and
manually builds query parameters. Add a dedicated Pydantic search-query DTO,
update search to accept that DTO, serialize it using
model_dump(exclude_none=True), and retain
CollectionPageSearchResult.model_validate() for response validation.
In `@tests/unit/test_collections.py`:
- Around line 111-119: In tests/unit/test_collections.py at lines 111-119,
165-175, and 206-210, narrow the exception handling around page and collection
deletion so unexpected remote cleanup failures propagate; suppress only the
documented idempotent “not found” response, and remove blanket Exception/pass
handling at each affected cleanup site.
- Around line 65-77: Move the cleanup guard in this test to begin immediately
after creating the source resource. Initialize target and page variables to
None, wrap their creation in try, and update the cleanup logic to conditionally
delete each resource that was successfully created, including source.
🪄 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 Plus
Run ID: c1ccf89f-1d0b-48ea-a542-bd0b632d8caa
📒 Files selected for processing (10)
plane/api/collections/__init__.pyplane/api/collections/base.pyplane/api/collections/members.pyplane/api/collections/pages.pyplane/client/plane_client.pyplane/models/collections.pyplane/models/pages.pyplane/models/query_params.pypyproject.tomltests/unit/test_collections.py
Comment on lines
+29
to
+84
| response = self._get(f"{workspace_slug}/collections") | ||
| return [Collection.model_validate(item) for item in response] | ||
|
|
||
| def create(self, workspace_slug: str, data: CreateCollection) -> Collection: | ||
| """Create a new collection in a workspace. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| data: Collection data | ||
| """ | ||
| response = self._post(f"{workspace_slug}/collections", data.model_dump(exclude_none=True)) | ||
| return Collection.model_validate(response) | ||
|
|
||
| def retrieve(self, workspace_slug: str, collection_id: str) -> Collection: | ||
| """Retrieve a collection by ID. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| """ | ||
| response = self._get(f"{workspace_slug}/collections/{collection_id}") | ||
| return Collection.model_validate(response) | ||
|
|
||
| def update(self, workspace_slug: str, collection_id: str, data: UpdateCollection) -> Collection: | ||
| """Update a collection's name, logo, or sort order. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| data: Fields to update (access cannot be changed after creation) | ||
| """ | ||
| response = self._patch( | ||
| f"{workspace_slug}/collections/{collection_id}", | ||
| data.model_dump(exclude_none=True), | ||
| ) | ||
| return Collection.model_validate(response) | ||
|
|
||
| def delete( | ||
| self, | ||
| workspace_slug: str, | ||
| collection_id: str, | ||
| archive_pages: bool | None = None, | ||
| ) -> None: | ||
| """Delete a collection. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| archive_pages: Whether to archive the collection's pages instead of | ||
| leaving them unfiled. Omit to use the server's default (True). | ||
| Private collections always archive their pages regardless. | ||
| """ | ||
| params = None | ||
| if archive_pages is not None: | ||
| params = {"archive_pages": "true" if archive_pages else "false"} | ||
| return self._delete(f"{workspace_slug}/collections/{collection_id}", params=params) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Append trailing slashes to every collection endpoint.
These paths violate the SDK URL convention and can fail against strict routing or redirect POST/PATCH/DELETE requests.
plane/api/collections/base.py#L29-L84: make each collection URL end in/.plane/api/collections/members.py#L24-L72: make each member URL end in/.plane/api/collections/pages.py#L33-L104: make each page URL end in/.
As per coding guidelines, “All API endpoints should end with a trailing / and follow URL convention: {base_path}/api/v1{resource_base_path}/{endpoint}/.”
📍 Affects 3 files
plane/api/collections/base.py#L29-L84(this comment)plane/api/collections/members.py#L24-L72plane/api/collections/pages.py#L33-L104
🤖 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 `@plane/api/collections/base.py` around lines 29 - 84, Append a trailing slash
to every collection endpoint URL used by the collection API methods in
plane/api/collections/base.py lines 29-84, every member endpoint in
plane/api/collections/members.py lines 24-72, and every page endpoint in
plane/api/collections/pages.py lines 33-104. Update the URL construction in the
relevant list, create, retrieve, update, delete, and related methods while
preserving their existing parameters and behavior.
Source: Coding guidelines
Comment on lines
+27
to
+72
| def add( | ||
| self, workspace_slug: str, collection_id: str, data: CreateCollectionMember | ||
| ) -> CollectionMember: | ||
| """Add a member to a collection. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| data: Member user id and access level | ||
| """ | ||
| response = self._post( | ||
| f"{workspace_slug}/collections/{collection_id}/members", | ||
| data.model_dump(exclude_none=True), | ||
| ) | ||
| return CollectionMember.model_validate(response) | ||
|
|
||
| def update( | ||
| self, | ||
| workspace_slug: str, | ||
| collection_id: str, | ||
| member_id: str, | ||
| data: UpdateCollectionMember, | ||
| ) -> CollectionMember: | ||
| """Update a collection member's access level. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| member_id: UUID of the CollectionMember row (not the user id) | ||
| data: New access level | ||
| """ | ||
| response = self._patch( | ||
| f"{workspace_slug}/collections/{collection_id}/members/{member_id}", | ||
| data.model_dump(exclude_none=True), | ||
| ) | ||
| return CollectionMember.model_validate(response) | ||
|
|
||
| def remove(self, workspace_slug: str, collection_id: str, member_id: str) -> None: | ||
| """Remove a member from a collection. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| member_id: UUID of the CollectionMember row (not the user id) | ||
| """ | ||
| return self._delete(f"{workspace_slug}/collections/{collection_id}/members/{member_id}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required CRUD method names.
The new public API exposes add/remove; rename them to create/delete consistently.
plane/api/collections/members.py#L27-L41: renameaddtocreate.plane/api/collections/members.py#L64-L72: renameremovetodelete.plane/api/collections/pages.py#L39-L53: renameaddtocreate.plane/api/collections/pages.py#L94-L104: renameremovetodelete.
As per coding guidelines, “All resource methods follow CRUD verbs: create, retrieve, update, delete, list.”
📍 Affects 2 files
plane/api/collections/members.py#L27-L72(this comment)plane/api/collections/pages.py#L39-L104
🤖 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 `@plane/api/collections/members.py` around lines 27 - 72, Rename the public
CRUD methods consistently: in plane/api/collections/members.py lines 27-41,
rename add to create, and lines 64-72, rename remove to delete; in
plane/api/collections/pages.py lines 39-53, rename add to create, and lines
94-104, rename remove to delete. Preserve each method’s existing behavior and
signatures while applying the required CRUD naming convention.
Source: Coding guidelines
Comment on lines
+55
to
+70
| def search( | ||
| self, workspace_slug: str, collection_id: str, search: str | None = None | ||
| ) -> list[CollectionPageSearchResult]: | ||
| """Search pages that are not yet in a collection, to add them. | ||
|
|
||
| Args: | ||
| workspace_slug: The workspace slug identifier | ||
| collection_id: UUID of the collection | ||
| search: Optional case-insensitive substring filter on page name | ||
| """ | ||
| query_params = {"search": search} if search else None | ||
| response = self._get( | ||
| f"{workspace_slug}/collections/{collection_id}/pages-search", | ||
| params=query_params, | ||
| ) | ||
| return [CollectionPageSearchResult.model_validate(item) for item in response] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Accept a query DTO for page search.
search takes a raw string and hand-builds query parameters. Add a dedicated Pydantic search-query DTO and serialize it with model_dump(exclude_none=True).
As per coding guidelines, “Resource methods must accept Pydantic DTOs, serialize with model_dump(exclude_none=True), and validate responses with Model.model_validate().”
🤖 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 `@plane/api/collections/pages.py` around lines 55 - 70, The search method
currently accepts a raw string and manually builds query parameters. Add a
dedicated Pydantic search-query DTO, update search to accept that DTO, serialize
it using model_dump(exclude_none=True), and retain
CollectionPageSearchResult.model_validate() for response validation.
Source: Coding guidelines
Comment on lines
+65
to
+77
| source = client.collections.create( | ||
| workspace_slug, CreateCollection(name=_collection_name("Source Collection")) | ||
| ) | ||
| target = client.collections.create( | ||
| workspace_slug, CreateCollection(name=_collection_name("Target Collection")) | ||
| ) | ||
| page = client.pages.create_workspace_page( | ||
| workspace_slug, | ||
| CreatePage( | ||
| name=_collection_name("Collection Page"), | ||
| description_html="<p>collection sdk test</p>", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enter cleanup protection before creating dependent resources.
If target or page creation fails, source is never deleted because execution has not reached try. Initialize dependent resources to None, enter try immediately after creating source, and conditionally clean up whichever resources were created.
Proposed structure
source = client.collections.create(
workspace_slug, CreateCollection(name=_collection_name("Source Collection"))
)
- target = client.collections.create(...)
- page = client.pages.create_workspace_page(...)
+ target = None
+ page = None
try:
+ target = client.collections.create(...)
+ page = client.pages.create_workspace_page(...)
...🤖 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 `@tests/unit/test_collections.py` around lines 65 - 77, Move the cleanup guard
in this test to begin immediately after creating the source resource. Initialize
target and page variables to None, wrap their creation in try, and update the
cleanup logic to conditionally delete each resource that was successfully
created, including source.
Comment on lines
+111
to
+119
| try: | ||
| client.pages.delete_workspace_page(workspace_slug, page.id) | ||
| except Exception: | ||
| pass | ||
| for collection in (source, target): | ||
| try: | ||
| client.collections.delete(workspace_slug, collection.id, archive_pages=False) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not silently ignore remote cleanup failures. Catching every Exception and using pass can leave collections/pages in the shared workspace while the test still passes. Allow unexpected failures to surface; only suppress a documented idempotent case such as a confirmed “not found” response.
tests/unit/test_collections.py#L111-L119: narrow page and collection deletion exception handling.tests/unit/test_collections.py#L165-L175: narrow page and collection deletion exception handling.tests/unit/test_collections.py#L206-L210: narrow collection deletion exception handling.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 113-114: try-except-pass detected, consider logging the exception
(S110)
[warning] 113-113: Do not catch blind exception: Exception
(BLE001)
[error] 118-119: try-except-pass detected, consider logging the exception
(S110)
[warning] 118-118: Do not catch blind exception: Exception
(BLE001)
📍 Affects 1 file
tests/unit/test_collections.py#L111-L119(this comment)tests/unit/test_collections.py#L165-L175tests/unit/test_collections.py#L206-L210
🤖 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 `@tests/unit/test_collections.py` around lines 111 - 119, In
tests/unit/test_collections.py at lines 111-119, 165-175, and 206-210, narrow
the exception handling around page and collection deletion so unexpected remote
cleanup failures propagate; suppress only the documented idempotent “not found”
response, and remove blanket Exception/pass handling at each affected cleanup
site.
Source: Linters/SAST tools
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.
Description
Summary by CodeRabbit