Open
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
📝 WalkthroughWalkthroughThe pull request expands the Epics API from read-only operations to full CRUD lifecycle management, adding methods for creating, updating, and deleting epics. It introduces support for managing work items associated with epics via new Changes
Sequence DiagramssequenceDiagram
participant Client
participant API as Epics API
participant Server as Backend Server
participant DB as Database
Client->>API: create(workspace, project, CreateEpic)
API->>Server: POST /epics
Server->>DB: Insert epic record
DB-->>Server: Epic created
Server-->>API: Epic response
API-->>Client: Epic object
Client->>API: update(workspace, project, epic_id, UpdateEpic)
API->>Server: PATCH /epics/{epic_id}
Server->>DB: Update epic fields
DB-->>Server: Epic updated
Server-->>API: Epic response
API-->>Client: Updated Epic object
Client->>API: delete(workspace, project, epic_id)
API->>Server: DELETE /epics/{epic_id}
Server->>DB: Delete epic record
DB-->>Server: Deletion confirmed
Server-->>API: Success response
API-->>Client: None
sequenceDiagram
participant Client
participant API as Epics API
participant Server as Backend Server
participant DB as Database
Client->>API: list_issues(workspace, project, epic_id, params)
API->>Server: GET /epics/{epic_id}/issues
Server->>DB: Query issues by epic
DB-->>Server: Paginated issue results
Server-->>API: PaginatedEpicIssueResponse
API-->>Client: list[EpicIssue]
Client->>API: add_issues(workspace, project, epic_id, AddEpicWorkItems)
API->>Server: POST /epics/{epic_id}/issues
Server->>DB: Associate work items with epic
DB-->>Server: Associated items data
Server-->>API: EpicIssue list
API-->>Client: list[EpicIssue]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
plane/models/epics.py (2)
138-143: Consider renamingPaginatedEpicIssueResponsetoPaginatedEpicWorkItemResponse.For terminology consistency with "Work Item" naming convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane/models/epics.py` around lines 138 - 143, Rename the class PaginatedEpicIssueResponse to PaginatedEpicWorkItemResponse to align with "Work Item" terminology; update the class declaration (leave model_config and results: list[EpicIssue] unchanged) and then replace all references/usages/imports of PaginatedEpicIssueResponse across the codebase (including any serializers, type hints, and exports) to the new PaginatedEpicWorkItemResponse to maintain compatibility with PaginatedResponse and EpicIssue.
92-128: Consider renamingEpicIssuetoEpicWorkItemfor terminology consistency.While the coding guideline specifically targets endpoint and parameter names, renaming to
EpicWorkItemwould maintain consistent terminology throughout the SDK.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane/models/epics.py` around lines 92 - 128, Rename the response model class EpicIssue to EpicWorkItem and update all references: change the class name declaration from EpicIssue to EpicWorkItem, update any type hints, imports, factory/parse calls, serializers/deserializers, and tests that reference EpicIssue (including usages in functions or other models that expect EpicIssue) so they now reference EpicWorkItem; keep the same attributes and model_config, and ensure any runtime string names (e.g., in JSON schema generation or registry lookups) are updated to the new identifier to avoid deserialization breaks.tests/unit/test_epics.py (2)
111-115: Same bare exception pattern as fixture cleanup.Consistent with the fixture cleanup approach. Consider logging for debugging purposes as noted above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_epics.py` around lines 111 - 115, The test cleanup currently swallows all exceptions with a bare except in the finally block around client.work_items.delete(workspace_slug, project.id, work_item.id); change this to catch Exception as e (or a more specific exception type if known) and log the error so failures during cleanup are visible — e.g. replace the bare except with "except Exception as e:" and call the test logger or pytest.fail/logging (referencing client.work_items.delete, workspace_slug, project.id, work_item.id, and the surrounding finally block) to record the exception details rather than silently passing.
50-53: Consider using a more specific exception type or logging.The bare
except Exception: passsilently swallows all errors during cleanup. While this is acceptable for test teardown to prevent masking test failures, it may hide important issues.♻️ Optional: Log exceptions during cleanup
yield epic try: client.epics.delete(workspace_slug, project.id, epic.id) - except Exception: - pass + except Exception as e: + # Cleanup failure is non-fatal for tests + print(f"Epic cleanup failed: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_epics.py` around lines 50 - 53, The cleanup currently swallows all errors with a bare "except Exception: pass" around client.epics.delete(workspace_slug, project.id, epic.id); replace this with either a narrow exception type that the client raises (e.g., ApiError/HTTPError from your API client) in the except clause, or if the exact type is unclear catch Exception as e and log the failure (using your test logger or logging.warning/exception) including workspace_slug, project.id and epic.id so cleanup failures are visible without failing tests. Ensure you update the handler around client.epics.delete to reference the chosen exception class or the logged variable name.plane/api/epics.py (2)
102-122: Consider renaming method fromlist_issuestolist_work_items.Per coding guidelines, "Issue" terminology should be avoided in favor of "Work Item". The docstring already uses "work items" correctly. Consider renaming for consistency.
♻️ Proposed rename
- def list_issues( + def list_work_items( self, workspace_slug: str, project_id: str, epic_id: str, params: PaginatedQueryParams | None = None, ) -> PaginatedEpicIssueResponse:As per coding guidelines: "Never use 'Issue' in endpoint or parameter names — always use 'Work Item' instead"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane/api/epics.py` around lines 102 - 122, Rename the method list_issues to list_work_items in the Epic API client: update the method definition name, its docstring header/summary (ensure it still describes "work items"), and every internal reference/callsite (tests, imports, any place calling EpicClient.list_issues) to use list_work_items; keep the same parameters and return type (PaginatedEpicIssueResponse) unless there is an existing renamed response type, and update any public API export lists or type hints that expose the old name so there are no broken imports. Ensure method-level docstring and any API route string remain unchanged.
124-143: Consider renaming method fromadd_issuestoadd_work_itemsand add trailing slash.Per coding guidelines, "Issue" terminology should be avoided in favor of "Work Item". The docstring already uses the correct terminology.
♻️ Proposed changes
- def add_issues( + def add_work_items( self, workspace_slug: str, project_id: str, epic_id: str, data: AddEpicWorkItems, ) -> list[EpicIssue]: """Add work items as sub-issues under an epic. Args: workspace_slug: The workspace slug identifier project_id: UUID of the project epic_id: UUID of the epic data: Work item IDs to add """ response = self._post( - f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues", + f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues/", data.model_dump(exclude_none=True), ) return [EpicIssue.model_validate(item) for item in response]As per coding guidelines: "Never use 'Issue' in endpoint or parameter names — always use 'Work Item' instead" and "All API endpoints should end with a trailing
/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plane/api/epics.py` around lines 124 - 143, Rename the method add_issues to add_work_items, update its docstring to use "work item" terminology, and change the endpoint path in the _post call from f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues" to f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/work-items/" (note the "work-items" segment and the trailing slash). Also update the return typing and model conversion to use the work-item model (replace EpicIssue with EpicWorkItem and EpicIssue.model_validate(...) with EpicWorkItem.model_validate(...)), and update any callers, imports, and tests that reference add_issues or EpicIssue to the new names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plane/api/epics.py`:
- Around line 118-121: The API call constructing the endpoint in the expression
using self._get (the f-string that builds
"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues") must include a
trailing slash; update the f-string used when calling self._get so the endpoint
ends with "/". Ensure the change is applied in the same call site (the response
= self._get(...) invocation) so all requests conform to the guideline that
endpoints end with a trailing slash.
- Around line 30-35: Remove the orphaned comment "enable epics feature flag" and
ensure the API URL passed to self._post ends with a trailing slash: update the
call that currently uses f"{workspace_slug}/projects/{project_id}/epics" to
f"{workspace_slug}/projects/{project_id}/epics/" (the block that returns
Epic.model_validate(response) should remain unchanged).
- Around line 52-56: The endpoint URL strings for epic retrieve/update/delete
are missing the required trailing slash; update the f-strings like
f"{workspace_slug}/projects/{project_id}/epics/{epic_id}" (and the analogous
strings used in the update and delete calls) to include a trailing "/" at the
end (e.g. f".../epics/{epic_id}/") so all requests issued by methods that call
self._get, self._patch, and self._delete use endpoints that end with a slash.
In `@tests/unit/test_epics.py`:
- Around line 55-65: The test_create_epic test creates an epic via
client.epics.create but never deletes it, leaving an orphan resource; update the
test to clean up after itself by either using the existing epic fixture or
deleting the created epic in a teardown path (e.g., call
client.epics.delete(workspace_slug, epic.id) in a finally block or register it
for cleanup), and ensure the code checks epic is not None before attempting
deletion; reference test_create_epic, client.epics.create, and
client.epics.delete (or the epic fixture) when making the change.
---
Nitpick comments:
In `@plane/api/epics.py`:
- Around line 102-122: Rename the method list_issues to list_work_items in the
Epic API client: update the method definition name, its docstring header/summary
(ensure it still describes "work items"), and every internal reference/callsite
(tests, imports, any place calling EpicClient.list_issues) to use
list_work_items; keep the same parameters and return type
(PaginatedEpicIssueResponse) unless there is an existing renamed response type,
and update any public API export lists or type hints that expose the old name so
there are no broken imports. Ensure method-level docstring and any API route
string remain unchanged.
- Around line 124-143: Rename the method add_issues to add_work_items, update
its docstring to use "work item" terminology, and change the endpoint path in
the _post call from
f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues" to
f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/work-items/" (note the
"work-items" segment and the trailing slash). Also update the return typing and
model conversion to use the work-item model (replace EpicIssue with EpicWorkItem
and EpicIssue.model_validate(...) with EpicWorkItem.model_validate(...)), and
update any callers, imports, and tests that reference add_issues or EpicIssue to
the new names.
In `@plane/models/epics.py`:
- Around line 138-143: Rename the class PaginatedEpicIssueResponse to
PaginatedEpicWorkItemResponse to align with "Work Item" terminology; update the
class declaration (leave model_config and results: list[EpicIssue] unchanged)
and then replace all references/usages/imports of PaginatedEpicIssueResponse
across the codebase (including any serializers, type hints, and exports) to the
new PaginatedEpicWorkItemResponse to maintain compatibility with
PaginatedResponse and EpicIssue.
- Around line 92-128: Rename the response model class EpicIssue to EpicWorkItem
and update all references: change the class name declaration from EpicIssue to
EpicWorkItem, update any type hints, imports, factory/parse calls,
serializers/deserializers, and tests that reference EpicIssue (including usages
in functions or other models that expect EpicIssue) so they now reference
EpicWorkItem; keep the same attributes and model_config, and ensure any runtime
string names (e.g., in JSON schema generation or registry lookups) are updated
to the new identifier to avoid deserialization breaks.
In `@tests/unit/test_epics.py`:
- Around line 111-115: The test cleanup currently swallows all exceptions with a
bare except in the finally block around client.work_items.delete(workspace_slug,
project.id, work_item.id); change this to catch Exception as e (or a more
specific exception type if known) and log the error so failures during cleanup
are visible — e.g. replace the bare except with "except Exception as e:" and
call the test logger or pytest.fail/logging (referencing
client.work_items.delete, workspace_slug, project.id, work_item.id, and the
surrounding finally block) to record the exception details rather than silently
passing.
- Around line 50-53: The cleanup currently swallows all errors with a bare
"except Exception: pass" around client.epics.delete(workspace_slug, project.id,
epic.id); replace this with either a narrow exception type that the client
raises (e.g., ApiError/HTTPError from your API client) in the except clause, or
if the exact type is unclear catch Exception as e and log the failure (using
your test logger or logging.warning/exception) including workspace_slug,
project.id and epic.id so cleanup failures are visible without failing tests.
Ensure you update the handler around client.epics.delete to reference the chosen
exception class or the logged variable name.
🪄 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: 188f1ee0-dba1-4b97-8411-53a16ea6ab43
📒 Files selected for processing (4)
plane/api/epics.pyplane/models/epics.pypyproject.tomltests/unit/test_epics.py
Comment on lines
+30
to
+35
| # enable epics feature flag | ||
| response = self._post( | ||
| f"{workspace_slug}/projects/{project_id}/epics", | ||
| data.model_dump(exclude_none=True), | ||
| ) | ||
| return Epic.model_validate(response) |
There was a problem hiding this comment.
Remove orphaned comment and add trailing slash to endpoint.
The comment "enable epics feature flag" has no corresponding code. Additionally, the endpoint URL should end with a trailing / per coding guidelines.
🔧 Proposed fix
- # enable epics feature flag
response = self._post(
- f"{workspace_slug}/projects/{project_id}/epics",
+ f"{workspace_slug}/projects/{project_id}/epics/",
data.model_dump(exclude_none=True),
)As per coding guidelines: "All API endpoints should end with a trailing /"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane/api/epics.py` around lines 30 - 35, Remove the orphaned comment "enable
epics feature flag" and ensure the API URL passed to self._post ends with a
trailing slash: update the call that currently uses
f"{workspace_slug}/projects/{project_id}/epics" to
f"{workspace_slug}/projects/{project_id}/epics/" (the block that returns
Epic.model_validate(response) should remain unchanged).
Comment on lines
+52
to
+56
| query_params = params.model_dump(exclude_none=True) if params else None | ||
| response = self._get( | ||
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}", params=query_params | ||
| ) | ||
| return Epic.model_validate(response) |
There was a problem hiding this comment.
Add trailing slash to endpoint URLs.
The retrieve, update, and delete endpoints are missing trailing slashes.
🔧 Proposed fixes for lines 54, 70, 83
response = self._get(
- f"{workspace_slug}/projects/{project_id}/epics/{epic_id}", params=query_params
+ f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/", params=query_params
) response = self._patch(
- f"{workspace_slug}/projects/{project_id}/epics/{epic_id}",
+ f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/",
data.model_dump(exclude_none=True),
)- self._delete(f"{workspace_slug}/projects/{project_id}/epics/{epic_id}")
+ self._delete(f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/")As per coding guidelines: "All API endpoints should end with a trailing /"
📝 Committable suggestion
‼️ 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
| query_params = params.model_dump(exclude_none=True) if params else None | |
| response = self._get( | |
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}", params=query_params | |
| ) | |
| return Epic.model_validate(response) | |
| query_params = params.model_dump(exclude_none=True) if params else None | |
| response = self._get( | |
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/", params=query_params | |
| ) | |
| return Epic.model_validate(response) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane/api/epics.py` around lines 52 - 56, The endpoint URL strings for epic
retrieve/update/delete are missing the required trailing slash; update the
f-strings like f"{workspace_slug}/projects/{project_id}/epics/{epic_id}" (and
the analogous strings used in the update and delete calls) to include a trailing
"/" at the end (e.g. f".../epics/{epic_id}/") so all requests issued by methods
that call self._get, self._patch, and self._delete use endpoints that end with a
slash.
Comment on lines
118
to
121
| response = self._get( | ||
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}", params=query_params | ||
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues", | ||
| params=query_params, | ||
| ) |
There was a problem hiding this comment.
Add trailing slash to endpoint URL.
🔧 Proposed fix
response = self._get(
- f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues",
+ f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues/",
params=query_params,
)As per coding guidelines: "All API endpoints should end with a trailing /"
📝 Committable suggestion
‼️ 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}/projects/{project_id}/epics/{epic_id}", params=query_params | |
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues", | |
| params=query_params, | |
| ) | |
| response = self._get( | |
| f"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues/", | |
| params=query_params, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plane/api/epics.py` around lines 118 - 121, The API call constructing the
endpoint in the expression using self._get (the f-string that builds
"{workspace_slug}/projects/{project_id}/epics/{epic_id}/issues") must include a
trailing slash; update the f-string used when calling self._get so the endpoint
ends with "/". Ensure the change is applied in the same call site (the response
= self._get(...) invocation) so all requests conform to the guideline that
endpoints end with a trailing slash.
Comment on lines
+55
to
+65
| def test_create_epic( | ||
| self, client: PlaneClient, workspace_slug: str, project: Project | ||
| ) -> None: | ||
| """Test creating an epic.""" | ||
| timestamp = datetime.now().strftime("%Y%m%d_%H%M%S_%f") | ||
| data = CreateEpic(name=f"Test Epic {timestamp}") | ||
| client.projects.update_features(workspace_slug, project.id, ProjectFeature(epics=True)) | ||
| epic = client.epics.create(workspace_slug, project.id, data) | ||
| assert epic is not None | ||
| assert epic.id is not None | ||
| assert epic.name == data.name |
There was a problem hiding this comment.
Missing cleanup for created epic.
This test creates an epic directly without using the epic fixture, leaving an orphan resource after test completion. Consider adding cleanup or restructuring to use the fixture.
🧹 Proposed fix to add cleanup
def test_create_epic(
self, client: PlaneClient, workspace_slug: str, project: Project
) -> None:
"""Test creating an epic."""
timestamp = datetime.now().strftime("%Y%m%d_%H%M%S_%f")
data = CreateEpic(name=f"Test Epic {timestamp}")
client.projects.update_features(workspace_slug, project.id, ProjectFeature(epics=True))
epic = client.epics.create(workspace_slug, project.id, data)
- assert epic is not None
- assert epic.id is not None
- assert epic.name == data.name
+ try:
+ assert epic is not None
+ assert epic.id is not None
+ assert epic.name == data.name
+ finally:
+ try:
+ client.epics.delete(workspace_slug, project.id, epic.id)
+ except Exception:
+ pass📝 Committable suggestion
‼️ 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
| def test_create_epic( | |
| self, client: PlaneClient, workspace_slug: str, project: Project | |
| ) -> None: | |
| """Test creating an epic.""" | |
| timestamp = datetime.now().strftime("%Y%m%d_%H%M%S_%f") | |
| data = CreateEpic(name=f"Test Epic {timestamp}") | |
| client.projects.update_features(workspace_slug, project.id, ProjectFeature(epics=True)) | |
| epic = client.epics.create(workspace_slug, project.id, data) | |
| assert epic is not None | |
| assert epic.id is not None | |
| assert epic.name == data.name | |
| def test_create_epic( | |
| self, client: PlaneClient, workspace_slug: str, project: Project | |
| ) -> None: | |
| """Test creating an epic.""" | |
| timestamp = datetime.now().strftime("%Y%m%d_%H%M%S_%f") | |
| data = CreateEpic(name=f"Test Epic {timestamp}") | |
| client.projects.update_features(workspace_slug, project.id, ProjectFeature(epics=True)) | |
| epic = client.epics.create(workspace_slug, project.id, data) | |
| try: | |
| assert epic is not None | |
| assert epic.id is not None | |
| assert epic.name == data.name | |
| finally: | |
| try: | |
| client.epics.delete(workspace_slug, project.id, epic.id) | |
| except Exception: | |
| pass |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_epics.py` around lines 55 - 65, The test_create_epic test
creates an epic via client.epics.create but never deletes it, leaving an orphan
resource; update the test to clean up after itself by either using the existing
epic fixture or deleting the created epic in a teardown path (e.g., call
client.epics.delete(workspace_slug, epic.id) in a finally block or register it
for cleanup), and ensure the code checks epic is not None before attempting
deletion; reference test_create_epic, client.epics.create, and
client.epics.delete (or the epic fixture) when making the change.
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
1 participant
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
Type of Change
Test Scenarios
Summary by CodeRabbit
Release Notes
New Features
Version Update