Skip to content

Bug/COMMS-890: Claim description image uploads on new work packages when the attachments list is hidden - #25118

Open
akabiru wants to merge 4 commits into
devfrom
bug/comms-890-cleanup-job-can-delete-attachments-uploaded-to-a-new-work-package-when-attachments-drag-and-drop-field-is-missing-during-the-upload
Open

Bug/COMMS-890: Claim description image uploads on new work packages when the attachments list is hidden#25118
akabiru wants to merge 4 commits into
devfrom
bug/comms-890-cleanup-job-can-delete-attachments-uploaded-to-a-new-work-package-when-attachments-drag-and-drop-field-is-missing-during-the-upload

Conversation

@akabiru

@akabiru akabiru commented Sep 3, 2026

Copy link
Copy Markdown
Member

https://community.openproject.org/wp/COMMS-890

With "Show attachments in the work packages files tab" switched off, the new work package form never renders the attachments list, and that component was the only place syncing CKEditor uploads into the create payload. The request went out with an empty attachment list, so the uploaded image stayed uncontainered until the cleanup job deleted it.

The frontend now mirrors uploads into the new resource whether or not the list is rendered, and the backend additionally claims any of the author's uncontainered attachments referenced from the description on create, so the same upload also survives when it arrives from the Primer create dialog or from an API client that sends an empty list.

Setting off, image uploaded in the description of a new work package, no attachments list rendered

02-new-wp-ckeditor-image-no-attachment-list

Saved work package with the image; the attachment is claimed

03-saved-wp-with-image

A new work package created with `_links.attachments: []` but a description
embedding an uploaded image left that attachment uncontainered, so the
cleanup job deleted it. Uncontainered attachments of the current user that
the description references are now claimed alongside any explicit list.
… list

The store-to-resource sync lived only in op-attachments, so with the
attachments list hidden a CKEditor upload on a new work package never
reached the create payload.
Both work package descriptions and comments now resolve claimable
attachments through Attachments::ClaimableIdsFromText, which also picks up
markdown image references the CSS selector missed.
Feature spec for the ticket flow: attachments list deactivated, image
uploaded through CKEditor on a new work package, attachment claimed on save.
@akabiru akabiru self-assigned this Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]
🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25118, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`

Treat this as a standalone task, unrelated to PR #25118. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25118 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @akabiru to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @akabiru, and request a review from @akabiru.
On every commit, set @akabiru as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are targeted, enforce correct claiming constraints (author + uncontainered/own-container), and are well-covered by service, request, and feature specs including the reported regression path.

Pull request overview

Fixes a regression where image uploads performed in the work package description on new work packages were not claimed (and later cleaned up) when the attachments list UI was not rendered. The change makes claiming robust across UI variants and also covers API clients that send an empty attachments list.

Changes:

  • Frontend: after uploading attachments for a new HAL resource, mirror the uploaded attachment links back into the resource payload even if the attachments list component is not present.
  • Backend: on create, additionally claim any uncontainered attachments referenced from the description text (while still enforcing “only author’s uncontainered attachments”).
  • Tests: add unit/request/feature specs covering description-referenced attachment claiming and the “attachments list hidden” regression.
File summaries
File Description
spec/services/work_packages/set_attributes_service_description_attachment_claims_spec.rb Covers claiming behavior in WorkPackages::SetAttributesService for new vs persisted work packages and explicit vs description-referenced attachments.
spec/services/attachments/claimable_ids_from_text_spec.rb Unit tests for extracting and filtering claimable attachment IDs from text with/without container context.
spec/requests/api/v3/work_packages/create_resource_spec.rb Regression coverage for API create when _links.attachments is empty but description references an upload.
spec/features/work_packages/attachments/attachment_upload_spec.rb End-to-end regression coverage for CKEditor upload when attachments list is hidden.
frontend/src/app/core/state/attachments/attachments.service.ts Mirrors uploads into the “new resource” attachments links after upload completes.
frontend/src/app/core/state/attachments/attachments.service.spec.ts Verifies attachFiles mirrors new-resource attachments and leaves persisted-resource links unchanged.
app/services/work_packages/set_attributes_service.rb Claims attachments referenced in description during create by unioning with explicit attachment_ids.
app/services/work_packages/activities_tab/comment_attachments_claims/set_attributes_service.rb Reuses the shared text-scanning logic for comment attachment claiming.
app/services/attachments/claimable_ids_from_text.rb Adds shared implementation for extracting referenced attachment IDs and filtering to claimable ones.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@akabiru akabiru added the bugfix label Sep 3, 2026
@akabiru akabiru added this to the 17.9.x milestone Sep 3, 2026
@akabiru
akabiru marked this pull request as ready for review September 3, 2026 16:30
@akabiru
akabiru requested a review from a team September 3, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants