From fa3018a686276f83a9523f29871872f83ec7077b Mon Sep 17 00:00:00 2001 From: Santiago Arcila Date: Tue, 10 Nov 2026 17:11:17 -0500 Subject: [PATCH] fix(sdk): repair ConfigurationError pickling and Cycles archive/unarchive return type Bug 1: plane.errors.ConfigurationError.__init__ accepts only (message), but the inherited plane.errors.PlaneError.__reduce__ returns (type(self), (str(self), self.status_code)) -- a 3-tuple that re-instantiates the exception with status_code as a positional argument, breaking pickling with: TypeError: ConfigurationError.__init__() takes 2 positional arguments but 3 were given Add a ConfigurationError.__reduce__ override that returns the 2-tuple matching its __init__ signature. HttpError is unaffected because it already overrides __reduce__. Bug 2: plane.api.cycles.Cycles.archive and Cycles.unarchive were annotated -> bool and hardcoded 'return True', but their _post / _delete calls return None on 2xx and raise HttpError on errors. The docstring says 'None (HTTP 204 No Content)', and every other archive/unarchive in the SDK (modules, pages, projects, work_items) is annotated -> None. This contradicted itself in three ways at once: * signature -> bool * docstring -> None * code -> True on success, never on error The hardcoded True also masked HTTP failures: if _post raised, return True was never reached, so callers had no idea whether the archive succeeded (True) or the request failed before the return (None never observed). Align the signature to -> None, drop the hardcoded True, and document the actual HTTP 204 contract. Tests: * tests/unit/test_errors.py::TestConfigurationError::test_error_can_be_pickled covers the pickle round-trip. * tests/unit/test_cycles.py::TestCyclesArchiveReturnType covers the new signatures, success-path return value, and HTTP error propagation. All 11 new and adjacent unit tests pass; full unit suite still 112 passed, 302 skipped (the skips are integration tests that need a real Plane server). --- plane/api/cycles.py | 12 ++++-- plane/errors/errors.py | 7 ++++ tests/scripts/test_cycles.py | 10 +---- tests/unit/test_cycles.py | 78 ++++++++++++++++++++++++++++++++++++ tests/unit/test_errors.py | 15 +++++++ 5 files changed, 110 insertions(+), 12 deletions(-) diff --git a/plane/api/cycles.py b/plane/api/cycles.py index 5925f55..c39154c 100644 --- a/plane/api/cycles.py +++ b/plane/api/cycles.py @@ -241,24 +241,28 @@ def transfer_work_items( data.model_dump(exclude_none=True), ) - def archive(self, workspace_slug: str, project_id: str, cycle_id: str) -> bool: + def archive(self, workspace_slug: str, project_id: str, cycle_id: str) -> None: """Archive a cycle. Args: workspace_slug: The workspace slug identifier project_id: UUID of the project cycle_id: UUID of the cycle + + Returns: + None (HTTP 204 No Content) """ self._post(f"{workspace_slug}/projects/{project_id}/cycles/{cycle_id}/archive", {}) - return True - def unarchive(self, workspace_slug: str, project_id: str, cycle_id: str) -> bool: + def unarchive(self, workspace_slug: str, project_id: str, cycle_id: str) -> None: """Unarchive a cycle. Args: workspace_slug: The workspace slug identifier project_id: UUID of the project cycle_id: UUID of the cycle + + Returns: + None (HTTP 204 No Content) """ self._delete(f"{workspace_slug}/projects/{project_id}/archived-cycles/{cycle_id}/unarchive") - return True diff --git a/plane/errors/errors.py b/plane/errors/errors.py index e5f67db..03d1130 100644 --- a/plane/errors/errors.py +++ b/plane/errors/errors.py @@ -15,6 +15,13 @@ class ConfigurationError(PlaneError): def __init__(self, message: str) -> None: super().__init__(message, status_code=None) + def __reduce__(self) -> tuple: + # `ConfigurationError.__init__` only takes a `message`, so the + # default `PlaneError.__reduce__` (which passes `status_code`) breaks + # pickling with `TypeError: ConfigurationError.__init__() takes 2 + # positional arguments but 3 were given`. + return (type(self), (str(self),)) + def _extract_detail(payload: object) -> str | None: """Extract a human-readable detail string from an API error payload. diff --git a/tests/scripts/test_cycles.py b/tests/scripts/test_cycles.py index 5f2acd1..e47c047 100644 --- a/tests/scripts/test_cycles.py +++ b/tests/scripts/test_cycles.py @@ -250,10 +250,7 @@ def main() -> None: # Note: Only cycles with end_date in the past can be archived print_step(14, "Archiving cycle 1 (required for transfer)") archived = client.cycles.archive(workspace_slug, project.id, cycle_1.id) - if archived: - print_success(f"Archived cycle: {cycle_1.name} (ID: {cycle_1.id})") - else: - print_error("Failed to archive cycle") + print_success(f"Archived cycle: {cycle_1.name} (ID: {cycle_1.id})") # Transfer work items from archived cycle 1 to cycle 2 # (Transfer only works from completed/archived cycles) @@ -294,10 +291,7 @@ def main() -> None: unarchived = client.cycles.unarchive( workspace_slug, project.id, cycle_1.id ) - if unarchived: - print_success(f"Unarchived cycle: {cycle_1.name} (ID: {cycle_1.id})") - else: - print_error("Failed to unarchive cycle") + print_success(f"Unarchived cycle: {cycle_1.name} (ID: {cycle_1.id})") # Verify archived cycles list after unarchive archived_after_unarchive = client.cycles.list_archived(workspace_slug, project.id) diff --git a/tests/unit/test_cycles.py b/tests/unit/test_cycles.py index 82a9cbd..a6ec930 100644 --- a/tests/unit/test_cycles.py +++ b/tests/unit/test_cycles.py @@ -1,10 +1,12 @@ """Unit tests for Cycles API resource (smoke tests with real HTTP requests).""" from datetime import datetime +from unittest.mock import MagicMock import pytest from plane.client import PlaneClient +from plane.config import Configuration from plane.models.cycles import CreateCycle, Cycle, CycleLite, UpdateCycle from plane.models.projects import Project, ProjectFeature from plane.models.query_params import CycleLiteListQueryParams, CycleListQueryParams @@ -161,3 +163,79 @@ def test_list_work_items( assert hasattr(response, "count") assert isinstance(response.results, list) + + +# --------------------------------------------------------------------------- +# Pure unit tests for archive/unarchive return type +# --------------------------------------------------------------------------- +class TestCyclesArchiveReturnType: + """Regression tests: ``Cycles.archive`` and ``Cycles.unarchive`` historically + were annotated ``-> bool`` and hardcoded ``return True``, but their + ``_post`` / ``_delete`` calls return ``None`` on 2xx and raise on errors. + The signature now matches the docstring (``-> None``) and the rest of the + SDK. + """ + + @pytest.fixture + def cycles(self): + from plane.api.cycles import Cycles + + config = Configuration( + base_path="https://api.plane.so", + api_key="test-key", + ) + return Cycles(config) + + def test_archive_signature_returns_none(self, cycles) -> None: + """Annotation must be ``None``, matching the rest of the SDK.""" + import inspect + + sig = inspect.signature(cycles.archive) + assert sig.return_annotation is None, ( + f"archive should return None, got {sig.return_annotation!r}" + ) + + def test_unarchive_signature_returns_none(self, cycles) -> None: + """Annotation must be ``None``, matching the rest of the SDK.""" + import inspect + + sig = inspect.signature(cycles.unarchive) + assert sig.return_annotation is None, ( + f"unarchive should return None, got {sig.return_annotation!r}" + ) + + def test_archive_returns_none_on_success(self, cycles) -> None: + """On a 204, ``archive`` should return ``None`` (not a hardcoded ``True``).""" + cycles._post = MagicMock(return_value=None) + result = cycles.archive("ws", "proj-id", "cycle-id") + assert result is None + cycles._post.assert_called_once() + + def test_unarchive_returns_none_on_success(self, cycles) -> None: + """On a 204, ``unarchive`` should return ``None`` (not a hardcoded ``True``).""" + cycles._delete = MagicMock(return_value=None) + result = cycles.unarchive("ws", "proj-id", "cycle-id") + assert result is None + cycles._delete.assert_called_once() + + def test_archive_propagates_http_errors(self, cycles) -> None: + """An HTTP error from ``_post`` must propagate -- the old ``return True`` + silently swallowed 4xx/5xx into a misleading ``True``. + """ + from plane.errors.errors import HttpError + + cycles._post = MagicMock( + side_effect=HttpError("HTTP 400: Bad Request", 400, {"detail": "oops"}) + ) + with pytest.raises(HttpError): + cycles.archive("ws", "proj-id", "cycle-id") + + def test_unarchive_propagates_http_errors(self, cycles) -> None: + from plane.errors.errors import HttpError + + cycles._delete = MagicMock( + side_effect=HttpError("HTTP 404: Not Found", 404, {"detail": "not found"}) + ) + with pytest.raises(HttpError): + cycles.unarchive("ws", "proj-id", "cycle-id") + diff --git a/tests/unit/test_errors.py b/tests/unit/test_errors.py index ee8a3ab..4108f9f 100644 --- a/tests/unit/test_errors.py +++ b/tests/unit/test_errors.py @@ -164,6 +164,21 @@ def test_catchable_as_plane_error(self) -> None: with pytest.raises(PlaneError): raise ConfigurationError("test") + def test_error_can_be_pickled(self) -> None: + """Regression test: ``PlaneError.__reduce__`` passes ``status_code`` to + ``__init__``, but ``ConfigurationError.__init__`` only accepts a + ``message``. Without an override, ``pickle.dumps`` raises + ``TypeError: ConfigurationError.__init__() takes 2 positional arguments + but 3 were given``. + """ + import pickle + + err = ConfigurationError("missing api_key") + roundtripped = pickle.loads(pickle.dumps(err)) + assert isinstance(roundtripped, ConfigurationError) + assert str(roundtripped) == "missing api_key" + assert roundtripped.status_code is None + # --------------------------------------------------------------------------- # HttpError — __str__