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

app/services/attachments/claimable_ids_from_text.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module Attachments
+  module ClaimableIdsFromText
+    REFERENCE_REGEX = %r{/attachments/(\d+)/content}
+
+    module_function
+
+    def call(text, user:, container: nil)
+      ids = text.to_s.scan(REFERENCE_REGEX).flatten.map(&:to_i).uniq
+      return [] if ids.empty?
+
+      claimable_scope(container).where(id: ids, author: user).pluck(:id)
+    end
+
+    def claimable_scope(container)
+      uncontainered = Attachment.where(container: nil)
+      return uncontainered if container.nil? || container.new_record?
+
+      uncontainered.or(Attachment.where(container:))
+    end
+  end
+end

app/services/work_packages/activities_tab/comment_attachments_claims/set_attributes_service.rb

@@ -34,49 +34,12 @@ module CommentAttachmentsClaims
       class SetAttributesService < ::BaseServices::SetAttributes
         include ::Attachments::SetReplacements
 
-        ATTACHMENT_CSS_SELECTOR = "img.op-uc-image"
-
         def perform
-          ids_from_notes = collect_attachment_ids_from_notes
-          claimable_ids = filter_claimable_attachment_ids(ids_from_notes)
+          claimable_ids = Attachments::ClaimableIdsFromText.call(model.notes, user: User.current, container: model)
 
           self.params = params.reverse_merge(attachment_ids: claimable_ids)
           super
         end
-
-        private
-
-        def collect_attachment_ids_from_notes
-          return [] if model.notes.blank?
-
-          parser.css(ATTACHMENT_CSS_SELECTOR).filter_map do |img|
-            src = img["src"]
-            next if src.blank?
-
-            # Extract the attachment ID from the src URL
-            # Example: "/api/v3/attachments/30381/content" -> "30381"
-            match = src.match(%r{/attachments/(\d+)/content})
-            match[1] if match
-          end
-        end
-
-        def filter_claimable_attachment_ids(ids)
-          return [] if ids.blank?
-
-          # Only claim attachments that are actually claimable. We must not try to
-          # reassign attachments that are already attached to another container
-          # (e.g., the work package, or another comment), and we must only claim unattached files of
-          # the current user to satisfy validation rules.
-          Attachment
-            .where(container: nil)
-            .or(Attachment.where(container: model))
-            .where(id: ids, author: User.current)
-            .pluck(:id)
-        end
-
-        def parser
-          @parser ||= Nokogiri::HTML.fragment(model.notes)
-        end
       end
     end
   end

app/services/work_packages/set_attributes_service.rb

@@ -44,6 +44,7 @@ def set_attributes(attributes)
     validate_custom_fields = attributes.delete(:validate_custom_fields)
 
     set_attachments_attributes(attributes)
+    claim_attachments_referenced_in_description(attributes)
     set_versions_attributes(attributes)
     set_static_attributes(attributes)
 
@@ -65,6 +66,16 @@ def set_custom_values_to_validate(attributes, validate_custom_fields = nil)
     end
   end
 
+  def claim_attachments_referenced_in_description(attributes)
+    return unless model.new_record? && attributes.key?(:description)
+
+    claimable_ids = Attachments::ClaimableIdsFromText.call(attributes[:description], user:)
+    return if claimable_ids.empty?
+
+    explicit_ids = model.attachments_replacements&.ids || []
+    model.attachments_replacements = Attachment.where(id: explicit_ids | claimable_ids)
+  end
+
   def set_versions_attributes(attributes)
     target_ids = attributes.delete(:target_version_ids)
     observed_in_ids = attributes.delete(:observed_in_version_ids)

frontend/src/app/core/state/attachments/attachments.service.spec.ts

@@ -27,28 +27,86 @@
 //++
 
 import { TestBed } from '@angular/core/testing';
-import { provideHttpClient, withInterceptorsFromDi, withXhr } from '@angular/common/http';
+import {
+  HttpResponse,
+  provideHttpClient,
+  withInterceptorsFromDi,
+  withXhr,
+} from '@angular/common/http';
 import { provideHttpClientTesting } from '@angular/common/http/testing';
+import { firstValueFrom, of } from 'rxjs';
 import { States } from 'core-app/core/states/states.service';
 import { ConfigurationService } from 'core-app/core/config/configuration.service';
+import { I18nService } from 'core-app/core/i18n/i18n.service';
 import { OpUploadService } from 'core-app/core/upload/upload.service';
+import { ToastService } from 'core-app/shared/components/toaster/toast.service';
+import { HalResource } from 'core-app/features/hal/resources/hal-resource';
+import { IAttachment } from 'core-app/core/state/attachments/attachment.model';
 import { AttachmentsResourceService } from './attachments.service';
 
 describe('AttachmentsResourceService', () => {
+  let service:AttachmentsResourceService;
+
+  const attachment = {
+    id: '42',
+    fileName: 'a.png',
+    _links: {
+      self: { href: '/api/v3/attachments/42' },
+      delete: { href: '/api/v3/attachments/42' },
+    },
+  } as unknown as IAttachment;
+
   beforeEach(() => {
     TestBed.configureTestingModule({
       providers: [
         AttachmentsResourceService,
         { provide: States, useValue: new States() },
         { provide: ConfigurationService, useValue: {} },
-        { provide: OpUploadService, useValue: {} },
+        { provide: I18nService, useValue: { t: () => '' } },
+        { provide: ToastService, useValue: { addUpload: vi.fn() } },
+        {
+          provide: OpUploadService,
+          useValue: { upload: vi.fn(() => [of(new HttpResponse({ body: attachment }))]) },
+        },
         provideHttpClient(withXhr(), withInterceptorsFromDi()),
         provideHttpClientTesting(),
       ],
     });
+
+    service = TestBed.inject(AttachmentsResourceService);
   });
 
   it('initialises via dependency injection', () => {
-    expect(TestBed.inject(AttachmentsResourceService)).toBeTruthy();
+    expect(service).toBeTruthy();
+  });
+
+  describe('attachFiles', () => {
+    it('mirrors uploaded attachments into a new resource', async () => {
+      const resource = {
+        $source: { id: 'new' },
+        id: 'new',
+        $links: {},
+        attachments: { elements: [] },
+      } as unknown as HalResource;
+
+      await firstValueFrom(service.attachFiles(resource, [new File([''], 'a.png')]));
+
+      expect(resource.attachments).toEqual({ elements: [{ href: '/api/v3/attachments/42' }] });
+    });
+
+    it('leaves the attachments link of a persisted resource untouched', async () => {
+      const attachments = { href: '/api/v3/work_packages/5/attachments' };
+      const resource = {
+        $source: { id: '5' },
+        id: '5',
+        $links: {},
+        attachments,
+        addAttachment: { href: '/api/v3/work_packages/5/attachments' },
+      } as unknown as HalResource;
+
+      await firstValueFrom(service.attachFiles(resource, [new File([''], 'a.png')]));
+
+      expect(resource.attachments).toBe(attachments);
+    });
   });
 });

frontend/src/app/core/state/attachments/attachments.service.ts

@@ -104,6 +104,13 @@ export class AttachmentsResourceService extends ResourceStoreService<IAttachment
         identifier,
         href,
         uploadFiles,
+      )
+      .pipe(
+        tap(() => {
+          if (isNewResource(resource)) {
+            this.syncNewResourceAttachments(resource);
+          }
+        }),
       );
   }
 
@@ -150,6 +157,15 @@ export class AttachmentsResourceService extends ResourceStoreService<IAttachment
     return attachments?.href || null;
   }
 
+  private syncNewResourceAttachments(resource:HalResource):void {
+    const ids = this.query.getValue().collections[HAL_NEW_RESOURCE_ID]?.ids ?? [];
+    const attachments = ids
+      .map((id) => this.query.getEntity(id))
+      .filter((attachment):attachment is IAttachment => !!attachment);
+
+    resource.attachments = { elements: attachments.map((attachment) => attachment._links.self) };
+  }
+
   private uploadAttachments(href:string, files:IUploadFile[]):Observable<IAttachment[]> {
     const observables = this.uploadService.upload<IAttachment>(href, files);
     const uploads = files.map((f, i):[File, Observable<HttpEvent<unknown>>] => [f.file, observables[i]]);

spec/features/work_packages/attachments/attachment_upload_spec.rb

@@ -141,6 +141,49 @@
         wp.reload
         expect(wp.attachments.count).to eq(1)
       end
+
+      context "with the attachments list hidden" do
+        let!(:project) do
+          create(:project, types: [type], deactivate_work_package_attachments: true)
+        end
+
+        it "claims the image uploaded in the description (Regression COMMS-890)" do
+          table.visit!
+          new_page = table.create_wp_by_button type
+          subject = new_page.edit_field :subject
+          subject.set_value "My subject"
+
+          expect(page).to have_no_css("op-attachments")
+
+          target = find(".ck-content")
+          attachments.drag_and_drop_file(target, image_fixture.path)
+
+          sleep 2 unless using_cuprite? # rubocop:disable OpenProject/NoSleepInFeatureSpecs
+          editor.wait_until_upload_progress_toaster_cleared
+
+          editor.in_editor do |_container, editable|
+            expect(editable).to have_css('img[src*="/api/v3/attachments/"]', wait: 20)
+            expect(editable).to have_no_css(".ck-upload-placeholder-loader")
+          end
+
+          sleep 2 unless using_cuprite? # rubocop:disable OpenProject/NoSleepInFeatureSpecs
+
+          scroll_to_and_click find_by_id("work-packages--edit-actions-save")
+
+          new_page.expect_and_dismiss_toaster(
+            message: "Successful creation."
+          )
+
+          split_view = Pages::SplitWorkPackage.new(WorkPackage.last)
+
+          field = split_view.edit_field :description
+          expect(field.display_element).to have_css("img")
+
+          wp = WorkPackage.last
+          expect(wp.attachments.count).to eq(1)
+          expect(wp.attachments.first.container).to eq(wp)
+        end
+      end
     end
 
     context "when on a new page" do

spec/requests/api/v3/work_packages/create_resource_spec.rb

@@ -528,6 +528,90 @@
       end
     end
 
+    context "when attachments are referenced in the description" do
+      let(:attachment) { create(:attachment, container: nil, author: current_user) }
+      let(:parameters) do
+        {
+          subject: "subject",
+          description: {
+            raw: %(<img class="op-uc-image" src="/api/v3/attachments/#{attachment.id}/content">)
+          },
+          _links: {
+            type: {
+              href: api_v3_paths.type(project.enabled_types.first.id)
+            },
+            project: {
+              href: api_v3_paths.project(project.id)
+            },
+            attachments: []
+          }
+        }
+      end
+
+      it "creates the work package, claims the attachment and journals it" do
+        expect(last_response).to have_http_status(:created)
+
+        work_package = WorkPackage.last
+        expect(work_package.attachments).to match_array(attachment)
+        expect(attachment.reload.container).to eq(work_package)
+        expect(work_package.journals.first.attachable_journals.map(&:attachment_id))
+          .to contain_exactly(attachment.id)
+      end
+
+      context "and the referenced attachment belongs to another user" do
+        let(:attachment) { create(:attachment, container: nil, author: create(:user)) }
+
+        it "creates the work package without claiming the attachment" do
+          expect(last_response).to have_http_status(:created)
+
+          expect(WorkPackage.last.attachments).to be_empty
+          expect(attachment.reload.container).to be_nil
+        end
+      end
+
+      context "and the referenced attachment is already containered in another work package" do
+        let(:attachment) do
+          create(:attachment, container: create(:work_package, project:), author: current_user)
+        end
+
+        it "creates the work package without claiming the attachment" do
+          expect(last_response).to have_http_status(:created)
+
+          expect(WorkPackage.last.attachments).to be_empty
+          expect(attachment.reload.container).not_to eq(WorkPackage.last)
+        end
+      end
+
+      context "and attachment_ids explicitly names another attachment" do
+        let(:explicitly_claimed_attachment) { create(:attachment, container: nil, author: current_user) }
+        let(:parameters) do
+          {
+            subject: "subject",
+            description: {
+              raw: %(<img class="op-uc-image" src="/api/v3/attachments/#{attachment.id}/content">)
+            },
+            _links: {
+              type: {
+                href: api_v3_paths.type(project.enabled_types.first.id)
+              },
+              project: {
+                href: api_v3_paths.project(project.id)
+              },
+              attachments: [
+                { href: api_v3_paths.attachment(explicitly_claimed_attachment.id) }
+              ]
+            }
+          }
+        end
+
+        it "claims both the explicit and the description-referenced attachments" do
+          expect(last_response).to have_http_status(:created)
+
+          expect(WorkPackage.last.attachments).to contain_exactly(attachment, explicitly_claimed_attachment)
+        end
+      end
+    end
+
     context "when file links are being claimed" do
       let(:storage) { create(:nextcloud_storage) }
       let(:project_storage) { create(:project_storage, project:, storage:) }