feat: add group_by/sub_group_by support to list_workspace work items - #47
Closed
sangeethailango wants to merge 1 commit into
Closed
feat: add group_by/sub_group_by support to list_workspace work items#47sangeethailango wants to merge 1 commit into
sangeethailango wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Introduce WorkspaceWorkItemQueryParams with group_by, sub_group_by, and sub_issue fields. Add GroupedPaginatedWorkItemResponse and SubGroupedPaginatedWorkItemResponse models; list_workspace now dispatches to the correct response type based on grouped_by/sub_grouped_by keys returned by the server. Also removes the duplicate list_workspace method. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
WalkthroughThe workspace-level work item listing API now supports optional grouping and sub-grouping of results. New response models handle single and two-level grouped pagination shapes, query parameters define grouping inputs with validation, and the list_workspace method conditionally deserializes responses based on the presence of grouping dimensions. ChangesGrouped workspace listing
Sequence DiagramsequenceDiagram
participant Client
participant ListWorkspace
participant APIResponse
participant ResponseValidator
Client->>ListWorkspace: call with WorkspaceWorkItemQueryParams
ListWorkspace->>APIResponse: GET with group_by/sub_group_by
APIResponse-->>ListWorkspace: JSON response
ListWorkspace->>ResponseValidator: check grouped_by key in response
alt sub_grouped_by present
ResponseValidator->>ResponseValidator: validate as SubGroupedPaginatedWorkItemResponse
else grouped_by present
ResponseValidator->>ResponseValidator: validate as GroupedPaginatedWorkItemResponse
else flat response
ResponseValidator->>ResponseValidator: validate as PaginatedWorkItemResponse
end
ResponseValidator-->>Client: typed response model instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 📝 Generate docstrings
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)plane/api/work_items/base.py (1)🤖 Prompt for all review comments with AI agents287-293: 💤 Low value
Consider documenting the response shape detection mechanism.
The conditional deserialization logic inspects grouped_by and sub_grouped_by keys in the raw response to determine which model to validate into. This is a runtime dispatch based on response structure.
While the logic is correct, it creates an implicit contract with the server: the presence of these keys determines the shape. Consider adding a brief inline comment explaining this dispatch logic for future maintainers.
📝 Suggested inline comment🤖 Prompt for AI Agents+ # Dispatch to the appropriate response model based on grouping keys + # present in the server response (grouped_by, sub_grouped_by) grouped_by = response.get("grouped_by") sub_grouped_by = response.get("sub_grouped_by") if grouped_by and sub_grouped_by: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/work_items/base.py` around lines 287 - 293, Add a brief inline comment above the runtime dispatch that explains the implicit contract with the server: that presence of the "grouped_by" and "sub_grouped_by" keys in the raw response is used to choose which Pydantic model to deserialize into. Annotate the block containing grouped_by = response.get("grouped_by"), sub_grouped_by = response.get("sub_grouped_by") and the three return paths using SubGroupedPaginatedWorkItemResponse.model_validate, GroupedPaginatedWorkItemResponse.model_validate, and PaginatedWorkItemResponse.model_validate to clarify the detection logic and expected response shapes for future maintainers.
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/work_items/base.py`:
- Around line 283-286: The API call to self._get building the work-items
endpoint is missing the required trailing slash; update the URL string passed to
self._get from f"{workspace_slug}/work-items" to include a trailing slash (e.g.,
f"{workspace_slug}/work-items/") so all endpoints in this module conform to the
trailing-slash guideline; ensure the call that uses
prepare_work_item_params(params) and assigns to response remains unchanged
except for the URL string.
In `@plane/models/query_params.py`:
- Around line 106-149: Add a Pydantic model-level validator to
WorkspaceWorkItemQueryParams to enforce the docstring rules: if sub_group_by is
set then group_by must be set, and group_by and sub_group_by must not be equal.
Import model_validator from pydantic, then add a `@model_validator`(mode="after")
method on WorkspaceWorkItemQueryParams (e.g., validate_grouping) that checks
self.sub_group_by and self.group_by and raises a ValueError with a clear message
when the constraints are violated.
---
Nitpick comments:
In `@plane/api/work_items/base.py`:
- Around line 287-293: Add a brief inline comment above the runtime dispatch
that explains the implicit contract with the server: that presence of the
"grouped_by" and "sub_grouped_by" keys in the raw response is used to choose
which Pydantic model to deserialize into. Annotate the block containing
grouped_by = response.get("grouped_by"), sub_grouped_by =
response.get("sub_grouped_by") and the three return paths using
SubGroupedPaginatedWorkItemResponse.model_validate,
GroupedPaginatedWorkItemResponse.model_validate, and
PaginatedWorkItemResponse.model_validate to clarify the detection logic and
expected response shapes for future maintainers.
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: cc28d799-200e-4ca4-91f8-86f8b5aafec3
📥 CommitsReviewing files that changed from the base of the PR and between a105871 and 403d4ff.
📒 Files selected for processing (4)- plane/api/work_items/base.py
- plane/models/__init__.py
- plane/models/query_params.py
- plane/models/work_items.py
Sorry, something went wrong.
Comment thread
plane/api/work_items/base.py
Comment on lines
283
to
286
| response = self._get( | ||
| f"{workspace_slug}/work-items", params=query_params | ||
| f"{workspace_slug}/work-items", | ||
| params=prepare_work_item_params(params), | ||
| ) |
Copy link
Copy Markdown
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Add trailing slash to API endpoint.
The endpoint on line 284 is missing a trailing /. As per coding guidelines, all API endpoints in plane/api/**/*.py must end with a trailing slash.
🔧 Proposed fix response = self._get(
- f"{workspace_slug}/work-items",
+ f"{workspace_slug}/work-items/",
params=prepare_work_item_params(params),
)‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = self._get( | |
| f"{workspace_slug}/work-items", params=query_params | |
| f"{workspace_slug}/work-items", | |
| params=prepare_work_item_params(params), | |
| ) | |
| response = self._get( | |
| f"{workspace_slug}/work-items/", | |
| params=prepare_work_item_params(params), | |
| ) |
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/work_items/base.py` around lines 283 - 286, The API call to
self._get building the work-items endpoint is missing the required trailing
slash; update the URL string passed to self._get from
f"{workspace_slug}/work-items" to include a trailing slash (e.g.,
f"{workspace_slug}/work-items/") so all endpoints in this module conform to the
trailing-slash guideline; ensure the call that uses
prepare_work_item_params(params) and assigns to response remains unchanged
except for the URL string.
Sorry, something went wrong.
Comment thread
plane/models/query_params.py
Comment on lines
+106
to
+149
| class WorkspaceWorkItemQueryParams(WorkItemQueryParams): | ||
| """Query parameters for the workspace-scoped work item list endpoint. | ||
|
|
||
| Extends :class:`WorkItemQueryParams` with grouping and sub-issue controls | ||
| that are specific to ``GET /workspaces/{slug}/work-items``. | ||
|
|
||
| Grouping fields (``group_by``, ``sub_group_by``) change the shape of the | ||
| ``results`` envelope: | ||
|
|
||
| - Neither set → ``results`` is ``list[WorkItem]`` | ||
| - ``group_by`` only → ``results`` is ``dict[str, WorkItemGroupBucket]`` | ||
| - Both set → ``results`` is ``dict[str, dict[str, WorkItemGroupBucket]]`` | ||
|
|
||
| ``group_by`` and ``sub_group_by`` must differ; the server returns HTTP 400 | ||
| if they are the same. | ||
| """ | ||
|
|
||
| model_config = ConfigDict(extra="ignore", populate_by_name=True) | ||
|
|
||
| group_by: str | None = Field( | ||
| None, | ||
| description=( | ||
| "Field to group results by. When set the paginator returns a dict of " | ||
| "group buckets instead of a flat list. Valid values: " | ||
| + ", ".join(f"``{f}``" for f in GROUP_BY_FIELDS) | ||
| ), | ||
| ) | ||
| sub_group_by: str | None = Field( | ||
| None, | ||
| description=( | ||
| "Field to nest a second grouping within each top-level group. " | ||
| "Requires ``group_by`` to be set and must differ from it. " | ||
| "Same valid values as ``group_by``." | ||
| ), | ||
| ) | ||
| sub_issue: bool | None = Field( | ||
| None, | ||
| description=( | ||
| "When ``False``, only top-level items and direct children of epics " | ||
| "are returned (sub-issues are excluded). Omit or pass ``True`` to " | ||
| "include all items regardless of parent." | ||
| ), | ||
| ) | ||
|
|
Copy link
Copy Markdown
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add client-side validation for group_by and sub_group_by constraint.
The docstring states that group_by and sub_group_by must differ (lines 119-120), but there's no Pydantic validator to enforce this client-side. Users will only discover the constraint when the server returns HTTP 400, which degrades the developer experience.
✨ Proposed validator to enforce the constraint sub_issue: bool | None = Field(
None,
description=(
"When ``False``, only top-level items and direct children of epics "
"are returned (sub-issues are excluded). Omit or pass ``True`` to "
"include all items regardless of parent."
),
)
+
+ `@model_validator`(mode="after")
+ def _validate_grouping_fields_differ(self) -> "WorkspaceWorkItemQueryParams":
+ """Ensure group_by and sub_group_by are distinct when both are set."""
+ if (
+ self.group_by is not None
+ and self.sub_group_by is not None
+ and self.group_by == self.sub_group_by
+ ):
+ raise ValueError("group_by and sub_group_by must differ")
+ return selfYou'll also need to import model_validator:
-from pydantic import BaseModel, ConfigDict, Field
+from pydantic import BaseModel, ConfigDict, Field, model_validatorVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plane/models/query_params.py` around lines 106 - 149, Add a Pydantic model-level validator to WorkspaceWorkItemQueryParams to enforce the docstring rules: if sub_group_by is set then group_by must be set, and group_by and sub_group_by must not be equal. Import model_validator from pydantic, then add a `@model_validator`(mode="after") method on WorkspaceWorkItemQueryParams (e.g., validate_grouping) that checks self.sub_group_by and self.group_by and raises a ValueError with a clear message when the constraints are violated.
Sorry, something went wrong.
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
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.
Introduce WorkspaceWorkItemQueryParams with group_by, sub_group_by, and sub_issue fields. Add GroupedPaginatedWorkItemResponse and SubGroupedPaginatedWorkItemResponse models; list_workspace now dispatches to the correct response type based on grouped_by/sub_grouped_by keys returned by the server. Also removes the duplicate list_workspace method.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes