[SILO-700] chore: improve tests for module and cycles feature check #10
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughThis pull request introduces three new API resources—Initiatives, Stickies, and Teamspaces—each with full CRUD operations and nested sub-resources. It extends workspace and project feature management APIs, enhances BaseResource to support optional DELETE payloads, updates package exports, and includes comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as PlaneClient
participant Init as Initiatives
participant Labels as InitiativeLabels
participant API as BaseResource<br/>(HTTP)
Note over Client,API: Initialization
Client->>Init: __init__(config)
Init->>Labels: __init__(config)
Note over Client,API: Create Initiative
Client->>Init: create(workspace_slug, data)
Init->>API: _post(endpoint, data)
API-->>Init: response
Init->>Init: model_validate(response)
Init-->>Client: Initiative
Note over Client,API: Manage Initiative Labels
Client->>Labels: add_labels(workspace_slug, initiative_id, label_ids)
Labels->>API: _post(endpoint, payload)
API-->>Labels: response_list
Labels->>Labels: model_validate each
Labels-->>Client: Iterable[InitiativeLabel]
Note over Client,API: Cleanup
Client->>Init: delete(workspace_slug, initiative_id)
Init->>API: _delete(endpoint)
API-->>Init: None
sequenceDiagram
participant PlaneClient
participant Stickies
participant Teamspaces
participant Resources as (Projects,<br/>Members)
Note over PlaneClient,Resources: PlaneClient Resource Integration
PlaneClient->>PlaneClient: __init__()
PlaneClient->>Stickies: instantiate
PlaneClient->>Teamspaces: instantiate
Teamspaces->>Resources: instantiate sub-resources
Note over PlaneClient,Resources: Access Flow
PlaneClient->>Stickies: list(workspace_slug)
PlaneClient->>Teamspaces: list(workspace_slug)
Teamspaces->>Resources: access members/projects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (11)
tests/unit/test_projects.py (1)
99-127: Consider testing additional feature toggle scenarios.The current tests verify enabling a feature (
cycles=True), but consider adding test cases for:
- Disabling features (setting to
False)- Enabling multiple features simultaneously
- Testing idempotency (updating with the same values)
These additional scenarios would strengthen test coverage for edge cases.
tests/unit/test_workspaces.py (1)
29-39: Consider restoring original feature state after test.The test modifies workspace features but doesn't restore the original state. If other tests depend on specific feature configurations, this could cause test order dependencies.
Consider storing and restoring the original state:
def test_update_features(self, client: PlaneClient, workspace_slug: str) -> None: """Test updating workspace features.""" # Get current features first features = client.workspaces.get_features(workspace_slug) + original_initiatives = features.initiatives # Update features features.initiatives = True updated = client.workspaces.update_features(workspace_slug, features) assert updated is not None assert hasattr(updated, "initiatives") assert updated.initiatives is True + + # Restore original state + features.initiatives = original_initiatives + client.workspaces.update_features(workspace_slug, features)tests/unit/test_stickies.py (1)
55-58: Consider logging cleanup failures instead of silencing them.The bare exception handler silently suppresses all cleanup errors, which could hide legitimate issues like network failures or authentication problems that might affect other tests.
Apply this pattern for better observability:
yield sticky try: client.stickies.delete(workspace_slug, sticky.id) - except Exception: - pass + except Exception as e: + # Log but don't fail the test if cleanup fails + print(f"Warning: Failed to cleanup sticky {sticky.id}: {e}")This applies to similar cleanup blocks at lines 70-73 as well.
tests/unit/test_initiatives.py (1)
59-62: Consider logging cleanup failures instead of silencing them.Multiple fixtures throughout this file use bare exception handlers that silently suppress cleanup errors. This pattern appears at lines 61-62, 79-80, 119-120, 145-146, 165-166, 186-187, 224-225, 247-248, and 291-292.
Consider this pattern for better observability:
yield initiative try: client.initiatives.delete(workspace_slug, initiative.id) - except Exception: - pass + except Exception as e: + # Log but don't fail the test if cleanup fails + print(f"Warning: Failed to cleanup initiative {initiative.id}: {e}")This would help identify issues like authentication failures, network problems, or API errors that might affect test reliability.
plane/api/workspaces.py (1)
31-31: Remove trailing whitespace.Line 31 appears to have trailing whitespace after the docstring closing quotes.
plane/api/initiatives/projects.py (1)
8-62: Consider materializingproject_idsfor JSON payloads and tightening return typeThe overall shape of
InitiativeProjects(endpoints, query params, andmodel_validateusage) matches the rest of the SDK and looks good.One small robustness/type improvement:
- The methods accept
project_ids: Iterable[str]but pass that iterable straight into the JSON body. If a caller passes a generator instead of a list/tuple, JSON serialization can misbehave. Either:
- Narrow the parameter type to
list[str](orSequence[str]) to reflect the expectation, or- Materialize the iterable before sending:
- response = self._post( - f"{workspace_slug}/initiatives/{initiative_id}/projects", - {"project_ids": project_ids}, - ) + response = self._post( + f"{workspace_slug}/initiatives/{initiative_id}/projects", + {"project_ids": list(project_ids)}, + ) ... - return self._delete( - f"{workspace_slug}/initiatives/{initiative_id}/projects", - {"project_ids": project_ids}, - ) + return self._delete( + f"{workspace_slug}/initiatives/{initiative_id}/projects", + {"project_ids": list(project_ids)}, + )
- Similarly, since you always return a list comprehension in
add, you could annotate the return type aslist[Project]for clarity.plane/models/stickies.py (1)
27-54: Consider consolidating CreateSticky and UpdateSticky.Both models have identical field definitions. While this pattern is common in CRUD operations, you could reduce duplication by having UpdateSticky inherit from CreateSticky or by extracting shared fields into a base class.
Example consolidation:
class CreateSticky(BaseModel): """Request model for creating a sticky.""" model_config = ConfigDict(extra="ignore", populate_by_name=True) name: str | None = None description: dict | str | None = None description_html: str | None = None description_stripped: str | None = None description_binary: bytes | None = None logo_props: dict | None = None color: str | None = None background_color: str | None = None -class UpdateSticky(BaseModel): +class UpdateSticky(CreateSticky): """Request model for updating a sticky.""" - - model_config = ConfigDict(extra="ignore", populate_by_name=True) - - name: str | None = None - description: dict | str | None = None - description_html: str | None = None - description_stripped: str | None = None - description_binary: bytes | None = None - logo_props: dict | None = None - color: str | None = None - background_color: str | None = None + passplane/api/initiatives/labels.py (2)
67-74: Remove unnecessary return statement.The
_deletemethod returnsNone, so the explicitreturnstatement is unnecessary.Apply this diff:
def delete(self, workspace_slug: str, label_id: str) -> None: """Delete an initiative label by ID. Args: workspace_slug: The workspace slug identifier label_id: UUID of the initiative label """ - return self._delete(f"{workspace_slug}/initiatives/labels/{label_id}") + self._delete(f"{workspace_slug}/initiatives/labels/{label_id}")
126-139: Remove unnecessary return statement.The
_deletemethod returnsNone, so the explicitreturnstatement is unnecessary.Apply this diff:
def remove_labels( self, workspace_slug: str, initiative_id: str, label_ids: Iterable[str] ) -> None: """Remove labels from an initiative. Args: workspace_slug: The workspace slug identifier initiative_id: UUID of the initiative label_ids: List of label UUIDs to remove """ - return self._delete( + self._delete( f"{workspace_slug}/initiatives/{initiative_id}/labels", {"label_ids": label_ids}, )tests/unit/test_teamspaces.py (2)
56-59: Log exceptions in cleanup code.Silent exception handling makes debugging failures difficult. Consider logging the exception or using pytest's built-in cleanup mechanisms.
Apply this pattern to all cleanup blocks:
yield teamspace try: client.teamspaces.delete(workspace_slug, teamspace.id) - except Exception: - pass + except Exception as e: + print(f"Cleanup failed: {e}")Alternatively, use
pytest.fixturewithfinalizerfor more robust cleanup:@pytest.fixture def teamspace(self, client: PlaneClient, workspace_slug: str, teamspace_data: CreateTeamspace): """Create a test teamspace and yield it, then delete it.""" teamspace = client.teamspaces.create(workspace_slug, teamspace_data) yield teamspace def cleanup(): try: client.teamspaces.delete(workspace_slug, teamspace.id) except Exception as e: print(f"Cleanup failed: {e}") request.addfinalizer(cleanup)
95-103: Rename duplicate test method.There's already a
test_list_teamspaces_with_paramsmethod inTestTeamspacesAPI(line 23). This duplicate name is confusing, even though they're in different classes. Consider renaming this one to reflect its more detailed validation.-def test_list_teamspaces_with_params( +def test_list_teamspaces_response_fields( self, client: PlaneClient, workspace_slug: str ) -> None: - """Test listing teamspaces with query parameters.""" + """Test listing teamspaces and validate response field structure.""" params = {"per_page": 5} response = client.teamspaces.list(workspace_slug, params=params)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.gitignore(1 hunks)README.md(1 hunks)plane/__init__.py(2 hunks)plane/api/__init__.py(2 hunks)plane/api/base_resource.py(1 hunks)plane/api/initiatives/__init__.py(1 hunks)plane/api/initiatives/base.py(1 hunks)plane/api/initiatives/epics.py(1 hunks)plane/api/initiatives/labels.py(1 hunks)plane/api/initiatives/projects.py(1 hunks)plane/api/projects.py(2 hunks)plane/api/stickies.py(1 hunks)plane/api/teamspaces/__init__.py(1 hunks)plane/api/teamspaces/base.py(1 hunks)plane/api/teamspaces/members.py(1 hunks)plane/api/teamspaces/projects.py(1 hunks)plane/api/workspaces.py(2 hunks)plane/client/plane_client.py(2 hunks)plane/models/enums.py(1 hunks)plane/models/initiatives.py(1 hunks)plane/models/projects.py(1 hunks)plane/models/stickies.py(1 hunks)plane/models/teamspaces.py(1 hunks)plane/models/users.py(2 hunks)plane/models/workspaces.py(1 hunks)tests/unit/test_cycles.py(3 hunks)tests/unit/test_initiatives.py(1 hunks)tests/unit/test_intake.py(1 hunks)tests/unit/test_modules.py(3 hunks)tests/unit/test_projects.py(2 hunks)tests/unit/test_stickies.py(1 hunks)tests/unit/test_teamspaces.py(1 hunks)tests/unit/test_workspaces.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
plane/api/teamspaces/__init__.py (1)
plane/api/teamspaces/base.py (1)
Teamspaces(15-95)
tests/unit/test_intake.py (1)
plane/models/projects.py (2)
Project(9-57)ProjectFeature(138-149)
plane/api/initiatives/__init__.py (1)
plane/api/initiatives/base.py (1)
Initiatives(16-97)
plane/api/base_resource.py (5)
plane/api/initiatives/base.py (1)
delete(75-82)plane/api/initiatives/labels.py (1)
delete(67-74)plane/api/projects.py (1)
delete(54-61)plane/api/stickies.py (1)
delete(60-67)plane/api/teamspaces/base.py (1)
delete(73-80)
tests/unit/test_initiatives.py (7)
plane/client/plane_client.py (1)
PlaneClient(22-61)plane/models/initiatives.py (4)
CreateInitiative(28-39)CreateInitiativeLabel(80-88)UpdateInitiative(42-53)UpdateInitiativeLabel(91-99)plane/models/projects.py (1)
Project(9-57)plane/api/initiatives/base.py (4)
create(27-41)delete(75-82)retrieve(43-54)update(56-73)plane/api/initiatives/labels.py (7)
create(19-33)delete(67-74)retrieve(35-46)update(48-65)list_labels(91-105)add_labels(107-124)remove_labels(126-139)plane/api/initiatives/epics.py (2)
add(30-47)remove(49-60)plane/api/initiatives/projects.py (2)
add(32-49)remove(51-62)
plane/api/initiatives/base.py (5)
plane/models/initiatives.py (4)
CreateInitiative(28-39)Initiative(6-25)PaginatedInitiativeResponse(56-61)UpdateInitiative(42-53)plane/api/base_resource.py (5)
BaseResource(12-98)_post(39-44)_get(32-37)_patch(53-58)_delete(60-65)plane/api/initiatives/epics.py (1)
InitiativeEpics(8-60)plane/api/initiatives/labels.py (5)
InitiativeLabels(13-139)create(19-33)retrieve(35-46)update(48-65)delete(67-74)plane/api/initiatives/projects.py (1)
InitiativeProjects(8-62)
plane/api/teamspaces/members.py (2)
plane/models/users.py (2)
PaginatedUserLiteResponse(21-26)UserLite(7-18)plane/api/base_resource.py (4)
BaseResource(12-98)_get(32-37)_post(39-44)_delete(60-65)
plane/api/teamspaces/projects.py (4)
plane/models/projects.py (2)
PaginatedProjectResponse(131-136)Project(9-57)plane/api/base_resource.py (4)
BaseResource(12-98)_get(32-37)_post(39-44)_delete(60-65)plane/api/initiatives/projects.py (2)
add(32-49)remove(51-62)plane/api/teamspaces/members.py (2)
add(32-49)remove(51-62)
plane/api/initiatives/epics.py (2)
plane/api/base_resource.py (4)
BaseResource(12-98)_get(32-37)_post(39-44)_delete(60-65)plane/api/initiatives/projects.py (2)
add(32-49)remove(51-62)
plane/client/plane_client.py (3)
plane/api/initiatives/base.py (1)
Initiatives(16-97)plane/api/stickies.py (1)
Stickies(8-82)plane/api/teamspaces/base.py (1)
Teamspaces(15-95)
plane/api/workspaces.py (3)
plane/models/workspaces.py (1)
WorkspaceFeature(3-13)plane/api/projects.py (2)
get_features(99-107)update_features(109-122)plane/api/base_resource.py (2)
_get(32-37)_patch(53-58)
tests/unit/test_cycles.py (2)
plane/models/projects.py (2)
Project(9-57)ProjectFeature(138-149)tests/unit/test_projects.py (1)
project(46-58)
plane/api/initiatives/projects.py (2)
plane/models/projects.py (2)
PaginatedProjectResponse(131-136)Project(9-57)plane/api/base_resource.py (4)
BaseResource(12-98)_get(32-37)_post(39-44)_delete(60-65)
plane/api/stickies.py (2)
plane/models/stickies.py (4)
CreateSticky(27-39)PaginatedStickyResponse(57-62)Sticky(6-24)UpdateSticky(42-54)plane/api/base_resource.py (5)
BaseResource(12-98)_post(39-44)_get(32-37)_patch(53-58)_delete(60-65)
plane/api/teamspaces/base.py (4)
plane/models/teamspaces.py (4)
CreateTeamspace(23-31)PaginatedTeamspaceResponse(45-50)Teamspace(4-20)UpdateTeamspace(34-42)plane/api/base_resource.py (5)
BaseResource(12-98)_post(39-44)_get(32-37)_patch(53-58)_delete(60-65)plane/api/teamspaces/members.py (1)
TeamspaceMembers(8-62)plane/api/teamspaces/projects.py (1)
TeamspaceProjects(8-62)
tests/unit/test_stickies.py (3)
plane/client/plane_client.py (1)
PlaneClient(22-61)plane/models/stickies.py (2)
CreateSticky(27-39)UpdateSticky(42-54)plane/api/stickies.py (4)
create(14-28)delete(60-67)retrieve(30-41)update(43-58)
tests/unit/test_teamspaces.py (6)
plane/client/plane_client.py (1)
PlaneClient(22-61)plane/models/projects.py (1)
Project(9-57)plane/models/teamspaces.py (2)
CreateTeamspace(23-31)UpdateTeamspace(34-42)plane/api/teamspaces/base.py (4)
create(25-39)delete(73-80)retrieve(41-52)update(54-71)plane/api/teamspaces/members.py (2)
add(32-49)remove(51-62)plane/api/teamspaces/projects.py (2)
add(32-49)remove(51-62)
plane/models/initiatives.py (1)
plane/models/enums.py (1)
InitiativeState(84-91)
plane/api/initiatives/labels.py (2)
plane/models/initiatives.py (4)
CreateInitiativeLabel(80-88)InitiativeLabel(64-77)PaginatedInitiativeLabelResponse(102-107)UpdateInitiativeLabel(91-99)plane/api/base_resource.py (5)
BaseResource(12-98)_post(39-44)_get(32-37)_patch(53-58)_delete(60-65)
plane/api/projects.py (3)
plane/models/projects.py (1)
ProjectFeature(138-149)plane/api/workspaces.py (2)
get_features(23-30)update_features(32-40)plane/api/base_resource.py (2)
_get(32-37)_patch(53-58)
plane/api/__init__.py (3)
plane/api/initiatives/base.py (1)
Initiatives(16-97)plane/api/stickies.py (1)
Stickies(8-82)plane/api/teamspaces/base.py (1)
Teamspaces(15-95)
plane/__init__.py (4)
plane/api/initiatives/base.py (1)
Initiatives(16-97)plane/api/stickies.py (1)
Stickies(8-82)plane/api/teamspaces/base.py (1)
Teamspaces(15-95)plane/api/workspaces.py (1)
Workspaces(8-40)
tests/unit/test_workspaces.py (2)
plane/client/plane_client.py (1)
PlaneClient(22-61)plane/api/workspaces.py (2)
get_features(23-30)update_features(32-40)
tests/unit/test_modules.py (2)
plane/models/projects.py (2)
Project(9-57)ProjectFeature(138-149)tests/unit/test_projects.py (1)
project(46-58)
tests/unit/test_projects.py (2)
tests/unit/test_workspaces.py (2)
test_get_features(18-27)test_update_features(29-39)plane/api/projects.py (3)
get_members(86-97)get_features(99-107)update_features(109-122)
🪛 Ruff (0.14.5)
tests/unit/test_initiatives.py
61-62: try-except-pass detected, consider logging the exception
(S110)
61-61: Do not catch blind exception: Exception
(BLE001)
79-80: try-except-pass detected, consider logging the exception
(S110)
79-79: Do not catch blind exception: Exception
(BLE001)
119-120: try-except-pass detected, consider logging the exception
(S110)
119-119: Do not catch blind exception: Exception
(BLE001)
145-146: try-except-pass detected, consider logging the exception
(S110)
145-145: Do not catch blind exception: Exception
(BLE001)
165-166: try-except-pass detected, consider logging the exception
(S110)
165-165: Do not catch blind exception: Exception
(BLE001)
186-187: try-except-pass detected, consider logging the exception
(S110)
186-186: Do not catch blind exception: Exception
(BLE001)
224-225: try-except-pass detected, consider logging the exception
(S110)
224-224: Do not catch blind exception: Exception
(BLE001)
247-248: try-except-pass detected, consider logging the exception
(S110)
247-247: Do not catch blind exception: Exception
(BLE001)
291-292: try-except-pass detected, consider logging the exception
(S110)
291-291: Do not catch blind exception: Exception
(BLE001)
tests/unit/test_stickies.py
57-58: try-except-pass detected, consider logging the exception
(S110)
57-57: Do not catch blind exception: Exception
(BLE001)
72-73: try-except-pass detected, consider logging the exception
(S110)
72-72: Do not catch blind exception: Exception
(BLE001)
tests/unit/test_teamspaces.py
58-59: try-except-pass detected, consider logging the exception
(S110)
58-58: Do not catch blind exception: Exception
(BLE001)
73-74: try-except-pass detected, consider logging the exception
(S110)
73-73: Do not catch blind exception: Exception
(BLE001)
133-134: try-except-pass detected, consider logging the exception
(S110)
133-133: Do not catch blind exception: Exception
(BLE001)
179-180: try-except-pass detected, consider logging the exception
(S110)
179-179: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (23)
.gitignore (1)
49-49: LGTM!Adding
.envto.gitignoreis standard practice to prevent sensitive environment variables from being committed.plane/api/initiatives/epics.py (2)
30-32: Inconsistent parameter naming with sibling resources.The parameter is named
epic_idsbut the corresponding methods inInitiativeProjects(plane/api/initiatives/projects.py line 32) useproject_ids: Iterable[str]. Consider using a consistent type annotation:- def add( - self, workspace_slug: str, initiative_id: str, epic_ids: Iterable[str] - ) -> Iterable[Epic]: + def add( + self, workspace_slug: str, initiative_id: str, epic_ids: Iterable[str] + ) -> Iterable[Epic]:Actually, the type is already
Iterable[str], which is correct. The naming convention is consistent with the pattern.
8-60: LGTM!The
InitiativeEpicsimplementation follows established patterns from similar resources (InitiativeProjects,InitiativeLabels). Methods are properly documented, use model validation, and handle payloads correctly.README.md (1)
237-239: LGTM!Documentation correctly reflects the new API resources exposed via
PlaneClient.plane/models/enums.py (1)
84-92: LGTM!The
InitiativeStateenum follows established patterns in the codebase with proper docstring and consistent value formatting.tests/unit/test_cycles.py (2)
63-63: LGTM!Enabling the cycles feature before creating a cycle in the fixture ensures tests run against the correct project state. This aligns with the PR's goal to improve feature check validation.
79-79: LGTM!Feature toggle correctly enables cycles before the create operation, validating that the feature check works as expected.
plane/client/plane_client.py (1)
4-4: LGTM!New API clients (Initiatives, Stickies, Teamspaces) are properly imported and instantiated following the established pattern for other resources.
Also applies to: 11-12, 59-61
plane/__init__.py (1)
2-2: LGTM! New API resources properly exported.The imports and
__all__entries for Initiatives, Stickies, Teamspaces, and Workspaces are correctly added, following the existing pattern in the file.Also applies to: 8-9, 14-14, 37-39, 44-44
plane/api/teamspaces/__init__.py (1)
1-4: LGTM! Standard package initialization.The module initialization follows the standard pattern used across the codebase for exporting the main API resource class.
tests/unit/test_modules.py (1)
10-10: LGTM! Feature enablement improves test reliability.Adding
ProjectFeature(modules=True)before module operations ensures the modules feature is enabled, which aligns with the PR objective to improve test coverage for feature checks. This is a good practice for testing feature-gated functionality.Also applies to: 64-64, 80-80
plane/api/initiatives/__init__.py (1)
1-4: LGTM! Standard package initialization.The module initialization follows the standard pattern used across the codebase for exporting the main API resource class.
plane/api/__init__.py (1)
3-5: LGTM! New API resources properly exported.The imports and
__all__entries for Initiatives, Stickies, and Teamspaces are correctly added, following the existing pattern in the file.Also applies to: 14-16
plane/api/base_resource.py (1)
60-65: Verify DELETE with body aligns with API specification.The
_deletemethod now accepts an optionaldatapayload sent as JSON in the request body. While HTTP allows DELETE requests with bodies, this is uncommon and not all HTTP clients/servers handle it consistently.Please confirm:
- The upstream API specification requires or supports DELETE with a body for specific endpoints
- The use cases where this is needed (e.g., bulk deletes, providing deletion context)
If this is needed, consider documenting when to use the
dataparameter in the method docstring.plane/models/users.py (1)
4-4: LGTM! Standard paginated response model.The
PaginatedUserLiteResponseclass follows the established pattern for paginated responses in the codebase. The model configuration and structure are consistent with similar models.Also applies to: 21-27
plane/api/projects.py (1)
99-122: LGTM!The project features API methods follow the established patterns in the codebase, with clear documentation and consistent implementation.
plane/api/workspaces.py (1)
23-40: LGTM!The workspace features API methods are well-implemented and consistent with the established patterns in the codebase.
plane/api/teamspaces/projects.py (1)
8-62: LGTM!The TeamspaceProjects API client is well-structured and follows the established patterns in the codebase. The implementation is consistent with similar sub-resource APIs like initiatives projects.
plane/api/teamspaces/members.py (1)
8-62: LGTM!The TeamspaceMembers API client follows the same clean pattern as the TeamspaceProjects client, with consistent implementation and clear documentation.
plane/api/stickies.py (1)
8-82: Stickies client follows existing resource patterns and looks correctEndpoints, payload serialization (
model_dump(exclude_none=True)), and response parsing viamodel_validateall align with the existingBaseResourceusage (e.g., initiatives labels/epics). Paths under/workspaces/{workspace_slug}/stickiesare consistently built, and the type hints/docstrings match behavior. I don't see any functional or API-surface issues here.plane/models/teamspaces.py (1)
4-50: Teamspace models are consistent with the rest of the SDKThe four models (
Teamspace,CreateTeamspace,UpdateTeamspace,PaginatedTeamspaceResponse) follow the same conventions as other Pydantic v2 models in the project (use ofConfigDict, optional fields for updates, and aresults: list[Teamspace]wrapper). Field naming and types line up with how the API clients will consume them, so this looks good as-is.plane/api/initiatives/base.py (1)
16-97: Initiatives client and sub-resources are wired cleanly and align with existing patternsThe CRUD methods hit the expected initiative endpoints, use
model_dump(exclude_none=True)for create/update, and parse responses withmodel_validate, matching how other resources are implemented. Initializinglabels,projects, andepicswith the same config under__init__provides a straightforward, discoverable surface for nested operations. I don't see any functional or typing issues here.plane/api/teamspaces/base.py (1)
15-95: Teamspaces client mirrors existing resource design and looks solid
Teamspacesfollows the same pattern as other clients: correct workspace-scoped paths, clear docstrings, create/update usingmodel_dump(exclude_none=True), andmodel_validatefor responses. Sub-resources forprojectsandmembersare initialized in__init__, which keeps the API cohesive. No issues stand out.
| description_binary: bytes | None = None | ||
| start_date: str | None = None | ||
| end_date: str | None = None | ||
| logo_props: dict |
There was a problem hiding this comment.
🧩 Analysis chain
Verify logo_props optionality across models.
The Initiative response model declares logo_props: dict as required (line 19), but CreateInitiative (line 37) and UpdateInitiative (line 51) make it optional (dict | None = None). This inconsistency suggests either:
- The API provides a default value when
logo_propsis omitted during creation, or - The
Initiativemodel should allowNoneto match the optional nature in create/update.
Ensure the model definitions align with the actual API contract.
🏁 Script executed:
#!/bin/bash
# Search for how logo_props is used in initiative-related API code and tests
rg -nC3 --type=py 'logo_props' -g '*initiative*'Length of output: 1297
🏁 Script executed:
#!/bin/bash
# Find API view/endpoint files that use Initiative model
# Look for imports and usage of Initiative in view/serializer files
rg -nC2 --type=py 'Initiative' | grep -E '(views|serializers|api|endpoints)' -i | head -30Length of output: 1882
🏁 Script executed:
#!/bin/bash
# Search for where Initiative objects are created or instantiated
rg -nC3 --type=py 'Initiative\(' | head -40Length of output: 2865
🏁 Script executed:
#!/bin/bash
# Look for logo_props being set or defaulted anywhere
rg -nC3 --type=py 'logo_props.*=' | head -40Length of output: 2099
Fix Initiative.logo_props to match input models and codebase patterns.
The Initiative response model declares logo_props: dict as required, but CreateInitiative and UpdateInitiative make it optional with dict | None = None. This inconsistency contradicts:
- The pattern across similar models (Cycle, Page, WorkItemType all use
Any | None = Nonefor logo_props) - Actual usage in tests, which never provide
logo_propswhen creating initiatives - The input model contract, where it's optional with a default of
None
Change line 19 to logo_props: dict | None = None to align with input models and the codebase pattern.
🤖 Prompt for AI Agents
In plane/models/initiatives.py around line 19, the Initiative response model
currently declares logo_props: dict as required but the input models and other
similar models make logo_props optional; change the declaration to logo_props:
dict | None = None so it matches CreateInitiative/UpdateInitiative, the codebase
pattern (Any | None = None), and tests that omit logo_props.
| class ProjectFeature(BaseModel): | ||
| """Project feature model.""" | ||
|
|
||
| model_config = ConfigDict(extra="allow", populate_by_name=True) | ||
|
|
||
| epics: bool | None = None | ||
| modules: bool | None = None | ||
| cycles: bool | None = None | ||
| views: bool | None = None | ||
| pages: bool | None = None | ||
| intakes: bool | None = None | ||
| work_item_types: bool | None = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix indentation to follow PEP 8 standard.
The class body uses 2-space indentation instead of Python's standard 4-space indentation. This is inconsistent with PEP 8 and the rest of the codebase.
Apply this diff to fix the indentation:
class ProjectFeature(BaseModel):
- """Project feature model."""
+ """Project feature model."""
- model_config = ConfigDict(extra="allow", populate_by_name=True)
+ model_config = ConfigDict(extra="allow", populate_by_name=True)
- epics: bool | None = None
- modules: bool | None = None
- cycles: bool | None = None
- views: bool | None = None
- pages: bool | None = None
- intakes: bool | None = None
- work_item_types: bool | None = None
+ epics: bool | None = None
+ modules: bool | None = None
+ cycles: bool | None = None
+ views: bool | None = None
+ pages: bool | None = None
+ intakes: bool | None = None
+ work_item_types: bool | None = None📝 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.
| class ProjectFeature(BaseModel): | |
| """Project feature model.""" | |
| model_config = ConfigDict(extra="allow", populate_by_name=True) | |
| epics: bool | None = None | |
| modules: bool | None = None | |
| cycles: bool | None = None | |
| views: bool | None = None | |
| pages: bool | None = None | |
| intakes: bool | None = None | |
| work_item_types: bool | None = None | |
| class ProjectFeature(BaseModel): | |
| """Project feature model.""" | |
| model_config = ConfigDict(extra="allow", populate_by_name=True) | |
| epics: bool | None = None | |
| modules: bool | None = None | |
| cycles: bool | None = None | |
| views: bool | None = None | |
| pages: bool | None = None | |
| intakes: bool | None = None | |
| work_item_types: bool | None = None |
🤖 Prompt for AI Agents
In plane/models/projects.py around lines 138 to 149, the class ProjectFeature
uses 2-space indentation for its body which violates PEP 8 and project style;
update the indentation to 4 spaces for the docstring, model_config, and all
field declarations (epics, modules, cycles, views, pages, intakes,
work_item_types) so the entire class body uses consistent 4-space indentation
matching the rest of the codebase.
| project_grouping: bool | ||
| initiatives: bool | ||
| teams: bool | ||
| customers: bool | ||
| wiki: bool | ||
| pi: bool No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider making feature flags optional.
All feature fields are typed as required bool, but API responses may not always include all fields. Consider making them optional to handle partial responses gracefully:
- project_grouping: bool
- initiatives: bool
- teams: bool
- customers: bool
- wiki: bool
- pi: bool
+ project_grouping: bool | None = None
+ initiatives: bool | None = None
+ teams: bool | None = None
+ customers: bool | None = None
+ wiki: bool | None = None
+ pi: bool | None = NoneThis aligns with the pattern used in ProjectFeature (plane/models/projects.py lines 137-148) where all feature flags are optional.
📝 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.
| project_grouping: bool | |
| initiatives: bool | |
| teams: bool | |
| customers: bool | |
| wiki: bool | |
| pi: bool | |
| project_grouping: bool | None = None | |
| initiatives: bool | None = None | |
| teams: bool | None = None | |
| customers: bool | None = None | |
| wiki: bool | None = None | |
| pi: bool | None = None |
🤖 Prompt for AI Agents
In plane/models/workspaces.py around lines 8 to 13, the feature flag fields are
plain bools but should be optional to match the ProjectFeature pattern and
handle partial API responses; update each field to use Optional[bool] (e.g.,
Optional[bool] = None) and add the typing import if missing so the model accepts
missing keys without raising type errors.
| from plane.client import PlaneClient | ||
| from plane.models.intake import CreateIntakeWorkItem | ||
| from plane.models.projects import Project | ||
| from plane.models.projects import Project, ProjectFeature |
There was a problem hiding this comment.
Unused import: ProjectFeature is imported but never used.
ProjectFeature is added to the import statement but is not referenced anywhere in this test file. Consider either:
- Removing the unused import if it's not needed
- Adding feature enablement calls if intake features should be toggled before tests (similar to the pattern in
tests/unit/test_cycles.pylines 63 and 79)
If intake features need to be enabled, you might add:
client.projects.update_features(
workspace_slug,
project.id,
ProjectFeature(intakes=True)
)before creating intake work items.
🤖 Prompt for AI Agents
In tests/unit/test_intake.py around line 7, ProjectFeature is imported but never
used; either remove ProjectFeature from the import list to eliminate the unused
import, or if intake features must be enabled for these tests, add a
feature-enable call before creating intake work items (mirroring
tests/unit/test_cycles.py lines ~63 and ~79) by calling the project's
update_features with a ProjectFeature enabling intakes for the test
workspace/project.
| assert hasattr(response.results[0], "id") | ||
| assert hasattr(response.results[0], "name") | ||
| assert hasattr(response.results[0], "description_html") | ||
| assert hasattr(response.results[0], "description_stripped") | ||
| assert hasattr(response.results[0], "description_binary") | ||
| assert hasattr(response.results[0], "logo_props") | ||
| assert hasattr(response.results[0], "lead") | ||
| assert hasattr(response.results[0], "workspace") |
There was a problem hiding this comment.
Guard against empty results list.
Accessing response.results[0] without checking if the list is empty could raise an IndexError if no teamspaces exist.
Apply this diff:
response = client.teamspaces.list(workspace_slug, params=params)
assert response is not None
assert hasattr(response, "results")
assert len(response.results) <= 5
- assert hasattr(response.results[0], "id")
- assert hasattr(response.results[0], "name")
- assert hasattr(response.results[0], "description_html")
- assert hasattr(response.results[0], "description_stripped")
- assert hasattr(response.results[0], "description_binary")
- assert hasattr(response.results[0], "logo_props")
- assert hasattr(response.results[0], "lead")
- assert hasattr(response.results[0], "workspace")
+ if response.results:
+ assert hasattr(response.results[0], "id")
+ assert hasattr(response.results[0], "name")
+ assert hasattr(response.results[0], "description_html")
+ assert hasattr(response.results[0], "description_stripped")
+ assert hasattr(response.results[0], "description_binary")
+ assert hasattr(response.results[0], "logo_props")
+ assert hasattr(response.results[0], "lead")
+ assert hasattr(response.results[0], "workspace")📝 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.
| assert hasattr(response.results[0], "id") | |
| assert hasattr(response.results[0], "name") | |
| assert hasattr(response.results[0], "description_html") | |
| assert hasattr(response.results[0], "description_stripped") | |
| assert hasattr(response.results[0], "description_binary") | |
| assert hasattr(response.results[0], "logo_props") | |
| assert hasattr(response.results[0], "lead") | |
| assert hasattr(response.results[0], "workspace") | |
| if response.results: | |
| assert hasattr(response.results[0], "id") | |
| assert hasattr(response.results[0], "name") | |
| assert hasattr(response.results[0], "description_html") | |
| assert hasattr(response.results[0], "description_stripped") | |
| assert hasattr(response.results[0], "description_binary") | |
| assert hasattr(response.results[0], "logo_props") | |
| assert hasattr(response.results[0], "lead") | |
| assert hasattr(response.results[0], "workspace") |
🤖 Prompt for AI Agents
In tests/unit/test_teamspaces.py around lines 104 to 111, the test accesses
response.results[0] directly which can raise IndexError if the results list is
empty; add a guard asserting the list is non-empty (e.g., assert
response.results and/or assert len(response.results) > 0 with a clear message)
before the attribute checks, then either perform the existing hasattr checks on
response.results[0] or iterate over response.results to validate attributes for
each entry.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.