diff --git a/app/services/attachments/claimable_ids_from_text.rb b/app/services/attachments/claimable_ids_from_text.rb new file mode 100644 index 000000000000..acc5ad2f10b2 --- /dev/null +++ b/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 diff --git a/app/services/work_packages/activities_tab/comment_attachments_claims/set_attributes_service.rb b/app/services/work_packages/activities_tab/comment_attachments_claims/set_attributes_service.rb index f4a59ff12dbd..8bf7db74a47b 100644 --- a/app/services/work_packages/activities_tab/comment_attachments_claims/set_attributes_service.rb +++ b/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 diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index faecb1066926..74ecbcba7dd8 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/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) diff --git a/frontend/src/app/core/state/attachments/attachments.service.spec.ts b/frontend/src/app/core/state/attachments/attachments.service.spec.ts index 3418a80d7e5b..14f2b5f95dca 100644 --- a/frontend/src/app/core/state/attachments/attachments.service.spec.ts +++ b/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); + }); }); }); diff --git a/frontend/src/app/core/state/attachments/attachments.service.ts b/frontend/src/app/core/state/attachments/attachments.service.ts index b4102adc69fa..86516f85d6b8 100644 --- a/frontend/src/app/core/state/attachments/attachments.service.ts +++ b/frontend/src/app/core/state/attachments/attachments.service.ts @@ -104,6 +104,13 @@ export class AttachmentsResourceService extends ResourceStoreService { + if (isNewResource(resource)) { + this.syncNewResourceAttachments(resource); + } + }), ); } @@ -150,6 +157,15 @@ export class AttachmentsResourceService extends ResourceStoreService 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 { const observables = this.uploadService.upload(href, files); const uploads = files.map((f, i):[File, Observable>] => [f.file, observables[i]]); diff --git a/spec/features/work_packages/attachments/attachment_upload_spec.rb b/spec/features/work_packages/attachments/attachment_upload_spec.rb index 9bb1984c13e9..cc27dec317fd 100644 --- a/spec/features/work_packages/attachments/attachment_upload_spec.rb +++ b/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 diff --git a/spec/requests/api/v3/work_packages/create_resource_spec.rb b/spec/requests/api/v3/work_packages/create_resource_spec.rb index b0eea3735b7b..6acd44e9587c 100644 --- a/spec/requests/api/v3/work_packages/create_resource_spec.rb +++ b/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: %() + }, + _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: %() + }, + _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:) } diff --git a/spec/services/attachments/claimable_ids_from_text_spec.rb b/spec/services/attachments/claimable_ids_from_text_spec.rb new file mode 100644 index 000000000000..fa023af058f5 --- /dev/null +++ b/spec/services/attachments/claimable_ids_from_text_spec.rb @@ -0,0 +1,148 @@ +# 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. +#++ + +require "spec_helper" + +RSpec.describe Attachments::ClaimableIdsFromText do + shared_let(:user) { create(:user) } + shared_let(:other_user) { create(:user) } + shared_let(:own_uncontainered) { create(:attachment, author: user, container: nil) } + + subject(:claimable_ids) { described_class.call(text, user:) } + + context "when the text is blank" do + let(:text) { "" } + + it "returns an empty array without querying" do + allow(Attachment).to receive(:where) + + expect(claimable_ids).to eq([]) + expect(Attachment).not_to have_received(:where) + end + end + + context "when the text is nil" do + let(:text) { nil } + + it "returns an empty array without querying" do + allow(Attachment).to receive(:where) + + expect(claimable_ids).to eq([]) + expect(Attachment).not_to have_received(:where) + end + end + + context "when the text references an inline image attachment" do + let(:text) { %() } + + it { is_expected.to contain_exactly(own_uncontainered.id) } + end + + context "when the text references an attachment via markdown" do + let(:text) { "![](/api/v3/attachments/#{own_uncontainered.id}/content)" } + + it { is_expected.to contain_exactly(own_uncontainered.id) } + end + + context "when the same attachment is referenced multiple times" do + let(:text) do + "#{own_uncontainered.id} " \ + "![](/api/v3/attachments/#{own_uncontainered.id}/content) " \ + "" + end + + it { is_expected.to contain_exactly(own_uncontainered.id) } + end + + context "when the attachment belongs to another user" do + shared_let(:other_users_attachment) { create(:attachment, author: other_user, container: nil) } + + let(:text) { "![](/api/v3/attachments/#{other_users_attachment.id}/content)" } + + it { is_expected.to eq([]) } + end + + context "when the attachment is already containered elsewhere" do + shared_let(:containered_attachment) { create(:attachment, author: user, container: create(:work_package)) } + + let(:text) { "![](/api/v3/attachments/#{containered_attachment.id}/content)" } + + it { is_expected.to eq([]) } + end + + context "when a container is given" do + subject(:claimable_ids) { described_class.call(text, user:, container:) } + + shared_let(:work_package) { create(:work_package) } + shared_let(:containered_in_given_container) { create(:attachment, author: user, container: work_package) } + shared_let(:containered_elsewhere) { create(:attachment, author: user, container: create(:work_package)) } + + let(:container) { work_package } + let(:text) do + "![](/api/v3/attachments/#{own_uncontainered.id}/content) " \ + "![](/api/v3/attachments/#{containered_in_given_container.id}/content) " \ + "![](/api/v3/attachments/#{containered_elsewhere.id}/content)" + end + + it "includes attachments already in that container alongside the still-uncontainered ones" do + expect(claimable_ids).to contain_exactly(own_uncontainered.id, containered_in_given_container.id) + end + + context "when the text also references another user's uncontainered attachment" do + shared_let(:other_users_attachment) { create(:attachment, author: other_user, container: nil) } + + let(:text) do + "![](/api/v3/attachments/#{own_uncontainered.id}/content) " \ + "![](/api/v3/attachments/#{containered_in_given_container.id}/content) " \ + "![](/api/v3/attachments/#{other_users_attachment.id}/content)" + end + + it "excludes the other user's attachment" do + expect(claimable_ids).to contain_exactly(own_uncontainered.id, containered_in_given_container.id) + end + end + + context "when the container is a new record" do + let(:container) { WorkPackage.new } + + it "behaves like no container was given" do + expect(claimable_ids).to contain_exactly(own_uncontainered.id) + end + end + + context "when the container is nil" do + let(:container) { nil } + + it "behaves like the container kwarg was omitted" do + expect(claimable_ids).to contain_exactly(own_uncontainered.id) + end + end + end +end diff --git a/spec/services/work_packages/set_attributes_service_description_attachment_claims_spec.rb b/spec/services/work_packages/set_attributes_service_description_attachment_claims_spec.rb new file mode 100644 index 000000000000..732eec379b30 --- /dev/null +++ b/spec/services/work_packages/set_attributes_service_description_attachment_claims_spec.rb @@ -0,0 +1,131 @@ +# 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. +#++ + +require "spec_helper" + +RSpec.describe WorkPackages::SetAttributesService do + shared_let(:user) { create(:user) } + shared_let(:other_user) { create(:user) } + + subject(:service_result) do + described_class + .new(user:, model: work_package, contract_class: EmptyContract) + .call(params) + end + + context "when the work package is new" do + let(:work_package) { WorkPackage.new } + + context "and the description references an own uncontainered attachment" do + shared_let(:attachment) { create(:attachment, author: user, container: nil) } + + let(:params) do + { description: %() } + end + + it "claims the attachment" do + expect(service_result.result.attachments_replacements).to contain_exactly(attachment) + end + end + + context "and attachment_ids is explicitly empty alongside a description reference" do + shared_let(:attachment) { create(:attachment, author: user, container: nil) } + + let(:params) do + { + attachment_ids: [], + description: "![](/api/v3/attachments/#{attachment.id}/content)" + } + end + + it "claims the attachment referenced in the description" do + expect(service_result.result.attachments_replacements).to contain_exactly(attachment) + end + end + + context "and attachment_ids already names another attachment alongside a description reference" do + shared_let(:explicit_attachment) { create(:attachment, author: user, container: nil) } + shared_let(:described_attachment) { create(:attachment, author: user, container: nil) } + + let(:params) do + { + attachment_ids: [explicit_attachment.id], + description: "![](/api/v3/attachments/#{described_attachment.id}/content)" + } + end + + it "unions the explicit and description-referenced attachments" do + expect(service_result.result.attachments_replacements) + .to contain_exactly(explicit_attachment, described_attachment) + end + end + + context "and the params carry no description key" do + let(:params) { { subject: "no description here" } } + + it "leaves attachments_replacements untouched" do + expect(service_result.result.attachments_replacements).to be_nil + end + end + + context "and the description references nothing" do + let(:params) { { description: "Lorem ipsum dolor sit amet" } } + + it "leaves attachments_replacements untouched" do + expect(service_result.result.attachments_replacements).to be_nil + end + end + + context "and the description references another user's uncontainered attachment" do + shared_let(:other_users_attachment) { create(:attachment, author: other_user, container: nil) } + + let(:params) do + { description: "![](/api/v3/attachments/#{other_users_attachment.id}/content)" } + end + + it "does not claim it" do + expect(service_result.result.attachments_replacements).to be_nil + end + end + end + + context "when the work package is persisted" do + shared_let(:work_package) { create(:work_package, author: user) } + shared_let(:attachment) { create(:attachment, author: user, container: nil) } + + let(:params) do + { description: "![](/api/v3/attachments/#{attachment.id}/content)" } + end + + it "does not claim attachments referenced in the description" do + expect(service_result.result.attachments_replacements).to be_nil + end + end +end