fix: WorkItemAttachments.create() returns wrapper response

[volodchenkov]

Fixes #33.

Summary

The Plane API returns a wrapper around the created attachment record,
including an S3 multipart-POST policy in upload_data for the file
upload. Previously WorkItemAttachments.create() validated this wrapper
as a flat WorkItemAttachment, raising ValidationError on the missing
asset field (the actual attachment is nested inside response["attachment"]).

This PR introduces WorkItemAttachmentCreateResponse capturing the full
wrapper shape:

• attachment: WorkItemAttachment — the created record
• upload_data: dict — S3 multipart-POST policy
• asset_id: str | None
• asset_url: str | None

WorkItemAttachments.create() is updated to return this new model.
Callers access the attachment via result.attachment and the upload
data via result.upload_data.

Test plan

• Added tests/unit/test_work_item_attachments.py covering:
  • list returns empty for new work item
  • create returns wrapper with both attachment record and upload_data
  • full lifecycle: create → list → retrieve → update is_uploaded → delete
• ruff format clean on changed files
• ruff check clean on changed files

Breaking change note

Return type of WorkItemAttachments.create() changes from
WorkItemAttachment to WorkItemAttachmentCreateResponse. Since the
previous return value crashed model_validate against any real Plane
response, no caller could have been depending on the old shape working.

Summary by CodeRabbit

• New Features

  • Attachment creation now returns the attachment plus multipart upload data (S3 policy/fields) so uploads can be performed immediately.

• Improvements

  • Attachment update flow now confirms and returns the latest attachment state after marking an upload as ready.

• Tests

  • Added end-to-end tests covering listing, creation (with upload data), update lifecycle, and deletion of attachments.

[coderabbitai]

Warning

Rate limit exceeded

@volodchenkov has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 13 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 052acf07-7593-4cfc-80db-1574e59567f3

📥 Commits

Reviewing files that changed from the base of the PR and between 6df22d9 and c3c8fa9.

📒 Files selected for processing (3)

• plane/api/work_items/attachments.py
• plane/models/work_items.py
• tests/unit/test_work_item_attachments.py

📝 Walkthrough

Walkthrough

Create now returns a wrapper model that includes the created attachment plus S3 multipart upload data; update performs a PATCH then re-lists attachments to return the updated record (raising if not found). Tests exercising list/create/update/delete were added.

Changes

WorkItem Attachments API & Model

Layer / File(s)	Summary
Data Shape plane/models/work_items.py	Adds WorkItemAttachmentCreateResponse with fields attachment: WorkItemAttachment, upload_data: dict[str, Any], optional asset_id, and asset_url.
Core Implementation plane/api/work_items/attachments.py	WorkItemAttachments.create() now validates/returns WorkItemAttachmentCreateResponse and documents the two-step flow (POST returns attachment + upload_data for S3 multipart). WorkItemAttachments.update() issues a PATCH (no response payload expected), then re-lists attachments and returns the matching attachment or raises ValueError if not found.
Tests tests/unit/test_work_item_attachments.py	New smoke tests: empty list for new work item, create returns wrapper with upload_data containing url and fields, and a full lifecycle test (create → list → get → update is_uploaded=True → delete).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client as Client\nrgba(66,133,244,0.5)
    participant API as Plane API\nrgba(52,168,83,0.5)
    participant S3 as S3 Multipart\nrgba(234,67,53,0.5)

    Client->>API: POST /work_items/{id}/attachments (create)
    API-->>Client: 201 { attachment, upload_data{url, fields}, asset_id?, asset_url? }
    Client->>S3: multipart upload using upload_data.url & fields + parts
    S3-->>Client: 200 OK (upload complete)
    Client->>API: PATCH /work_items/{id}/attachments/{attachment_id} (is_uploaded=True)
    API-->>Client: 204 No Content
    Client->>API: GET /work_items/{id}/attachments (list) -> find attachment by id
    API-->>Client: 200 [ ... attachment (is_uploaded=True) ... ]

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A tiny wrapper snug and bright,
Holds attachment and upload light,
I hop with fields and S3 cheer,
Two-step dance — the file is near,
Tests stamp paws and say: "All clear!"

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing WorkItemAttachments.create() to return a wrapper response instead of a flat attachment object.
Linked Issues check	✅ Passed	The PR fully addresses issue #33 by introducing WorkItemAttachmentCreateResponse model and updating WorkItemAttachments.create() to return the wrapper response with nested attachment and upload_data.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the validation error in WorkItemAttachments.create(). The new model, updated method, and tests all address the issue with no out-of-scope additions.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/unit/test_work_item_attachments.py (1)

31-34: 💤 Low value

Bare except Exception: pass in three cleanup blocks — flagged by Ruff S110/BLE001.

All three teardown paths silently swallow any exception, making failure diagnosis difficult. A lightweight fix is to emit a warning instead of passing silently.

♻️ Proposed fix (shown for the fixture; apply the same pattern to the other two blocks)

+import warnings
 ...
         try:
             client.work_items.delete(workspace_slug, project.id, work_item.id)
-        except Exception:
-            pass
+        except Exception as exc:
+            warnings.warn(f"Cleanup failed: {exc}", stacklevel=2)

Also applies to: 61-66, 99-104

🤖 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_work_item_attachments.py` around lines 31 - 34, The cleanup
blocks use bare "except Exception: pass" (e.g., around
client.work_items.delete(workspace_slug, project.id, work_item.id)); replace
each bare except with an explicit except Exception as e: that emits a warning or
log containing the exception details (for example via warnings.warn or process
logger) so failures during teardown are visible; also ensure warnings is
imported and apply the same change to the other two cleanup blocks that
currently swallow exceptions.

🤖 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 `@tests/unit/test_work_item_attachments.py`:
- Line 57: The test incorrectly asserts identity against False so it will fail
when WorkItemAttachment.is_uploaded is None; update the assertion in the test to
accept both False and None by replacing "assert result.attachment.is_uploaded is
False" with a check that allows either value (e.g., use "assert not
result.attachment.is_uploaded" or "assert result.attachment.is_uploaded in
(False, None)") so that result.attachment.is_uploaded (the
WorkItemAttachment.is_uploaded field) passes when the API returns null or false.

---

Nitpick comments:
In `@tests/unit/test_work_item_attachments.py`:
- Around line 31-34: The cleanup blocks use bare "except Exception: pass" (e.g.,
around client.work_items.delete(workspace_slug, project.id, work_item.id));
replace each bare except with an explicit except Exception as e: that emits a
warning or log containing the exception details (for example via warnings.warn
or process logger) so failures during teardown are visible; also ensure warnings
is imported and apply the same change to the other two cleanup blocks that
currently swallow exceptions.

🪄 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: fdd1533d-8dc3-4592-b0c3-0cc738021c49

📥 Commits

Reviewing files that changed from the base of the PR and between 6e5389f and d1457b7.

📒 Files selected for processing (3)

• plane/api/work_items/attachments.py
• plane/models/work_items.py
• tests/unit/test_work_item_attachments.py

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

tests/unit/test_work_item_attachments.py (1)

31-34: ⚡ Quick win

Blind except Exception: pass in teardown violates Ruff B (BLE001) — same pattern repeats at lines 65–66 and 103–104.

Use contextlib.suppress(Exception) instead, which is idiomatic, readable, and silences the Ruff warning without changing behavior.

♻️ Proposed fix (apply the same pattern to lines 65–66 and 103–104)

+from contextlib import suppress
 ...
     yield work_item.id
-    try:
+    with suppress(Exception):
         client.work_items.delete(workspace_slug, project.id, work_item.id)
-    except Exception:
-        pass

As per coding guidelines: "Apply Ruff rules: … B (bugbear)" — BLE001 is part of the bugbear ruleset.

🤖 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_work_item_attachments.py` around lines 31 - 34, Replace the
blind try/except blocks that call client.work_items.delete(...) in the teardown
of tests/unit/test_work_item_attachments.py with contextlib.suppress(Exception);
specifically, remove the try: ... except Exception: pass patterns around the
client.work_items.delete(workspace_slug, project.id, work_item.id) calls (and
the two other identical occurrences) and wrap each delete call in a
contextlib.suppress(Exception) context manager so Ruff BLE001 is satisfied while
preserving behavior.

plane/api/work_items/attachments.py (1)

119-129: ⚖️ Poor tradeoff

update() should call retrieve() after PATCH, not list() + filter.

The docstring itself claims that the list API only returns is_uploaded=True records — yet test_full_lifecycle calls retrieve() before setting is_uploaded=True and expects it to return the attachment (lines 85–88 of the test file). If retrieve() works post-PATCH (which the test implies), using it here is strictly simpler, avoids loading all attachments, and removes the fragile assumption about is_uploaded filtering in list().

♻️ Proposed refactor

-        self._patch(
-            f"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}",
-            data.model_dump(exclude_none=True),
-        )
-        for attachment in self.list(workspace_slug, project_id, work_item_id):
-            if attachment.id == attachment_id:
-                return attachment
-        raise ValueError(
-            f"Attachment {attachment_id} not found after update; "
-            "Plane only lists attachments with is_uploaded=True."
-        )
+        self._patch(
+            f"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}",
+            data.model_dump(exclude_none=True),
+        )
+        return self.retrieve(workspace_slug, project_id, work_item_id, attachment_id)

Please verify whether GET /attachments/{id}/ on the live Plane API returns JSON attachment metadata or performs an S3 redirect. The update docstring claims the latter, but retrieve() exists and the lifecycle test calls it successfully, suggesting the former.

🤖 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/work_items/attachments.py` around lines 119 - 129, The update()
implementation currently calls self._patch(...) then iterates self.list(...) to
find the updated attachment which is inefficient and relies on is_uploaded
filtering; instead, after self._patch(...) call self.retrieve(workspace_slug,
project_id, work_item_id, attachment_id) and return its result (handle/propagate
any exceptions), removing the list/filter loop; also verify whether GET
/attachments/{id}/ returns JSON metadata or an S3 redirect and adjust retrieve()
behavior/docs accordingly if it follows a redirect flow.

🤖 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/work_items/attachments.py`:
- Line 120: The PATCH endpoint URL string in attachments.py currently lacks the
required trailing slash; update the URL used when constructing the PATCH call
(the f-string containing
"{workspace_slug}/projects/{project_id}/work-items/{work_item_id}/attachments/{attachment_id}")
to include a trailing "/" at the end so the endpoint becomes
".../attachments/{attachment_id}/" to comply with the API endpoint guideline.

In `@tests/unit/test_work_item_attachments.py`:
- Around line 82-83: The test currently asserts the new attachment appears in
client.work_items.attachments.list(...) immediately, but according to the
update() docstring list() only includes attachments with is_uploaded=True and
the newly created attachment is is_uploaded=False; update the test to reflect
that by removing or moving the assertion: first assert the attachment is NOT
present in the list (or skip the presence check) before calling
client.work_items.attachments.update(..., is_uploaded=True), then after update()
assert any(a.id == attachment_id for a in listed) to confirm it appears;
reference the methods client.work_items.attachments.list and
client.work_items.attachments.update and the attachment_id variable when
adjusting the assertions.

---

Nitpick comments:
In `@plane/api/work_items/attachments.py`:
- Around line 119-129: The update() implementation currently calls
self._patch(...) then iterates self.list(...) to find the updated attachment
which is inefficient and relies on is_uploaded filtering; instead, after
self._patch(...) call self.retrieve(workspace_slug, project_id, work_item_id,
attachment_id) and return its result (handle/propagate any exceptions), removing
the list/filter loop; also verify whether GET /attachments/{id}/ returns JSON
metadata or an S3 redirect and adjust retrieve() behavior/docs accordingly if it
follows a redirect flow.

In `@tests/unit/test_work_item_attachments.py`:
- Around line 31-34: Replace the blind try/except blocks that call
client.work_items.delete(...) in the teardown of
tests/unit/test_work_item_attachments.py with contextlib.suppress(Exception);
specifically, remove the try: ... except Exception: pass patterns around the
client.work_items.delete(workspace_slug, project.id, work_item.id) calls (and
the two other identical occurrences) and wrap each delete call in a
contextlib.suppress(Exception) context manager so Ruff BLE001 is satisfied while
preserving behavior.

🪄 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: 8d2a6414-ede1-4895-a14a-7af07b1dfd85

📥 Commits

Reviewing files that changed from the base of the PR and between d1457b7 and 6df22d9.

📒 Files selected for processing (3)

• plane/api/work_items/attachments.py
• plane/models/work_items.py
• tests/unit/test_work_item_attachments.py

🚧 Files skipped from review as they are similar to previous changes (1)

• plane/models/work_items.py

[volodchenkov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[volodchenkov]

Re: the suggestion to use retrieve() instead of list + filter in update() — declining this one.

GET /api/v1/workspaces/{ws}/projects/{p}/work-items/{wi}/attachments/{aid}/ does not return JSON metadata. It returns a 302 redirect to an S3 presigned download URL — the endpoint serves the file, not the record. Verified empirically against a live self-hosted Plane (status 302, location: https://storage.yandexcloud.net/plane-data-private/...).

So retrieve() itself is broken for metadata fetch (separate concern; will file an issue). The list + filter workaround in update() is the only approach that works against Plane's actual API today.

Updated the test for the previous reorder feedback (mark uploaded → list → delete) so the list+filter path is exercised end-to-end.

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@volodchenkov Thanks for the detailed empirical clarification — a 302 redirect to a presigned S3 URL is a completely valid reason to avoid retrieve() for metadata fetches, and the list + filter workaround in update() makes total sense given that constraint. I'll make sure not to flag this pattern in future reviews. I'm also noting the retrieve() breakage for separate tracking.

Kicking off the re-review now.

(⊙_⊙)？

[review]

---

✏️ Learnings added

Learnt from: volodchenkov
URL: https://github.com/makeplane/plane-python-sdk/pull/34

Timestamp: 2026-05-06T06:58:53.770Z
Learning: In the Plane API (plane-python-sdk), `GET /api/v1/workspaces/{ws}/projects/{p}/work-items/{wi}/attachments/{aid}/` does NOT return JSON metadata. It returns a 302 redirect to an S3 presigned download URL (verified empirically against a live self-hosted Plane instance). Therefore, `WorkItemAttachments.retrieve()` is broken for metadata fetches, and the `list + filter` workaround used in `WorkItemAttachments.update()` (list all attachments, then filter by `attachment_id`) is the only correct approach to obtain attachment metadata after an update.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.