diff --git a/app/contracts/labels/validate_replacements.rb b/app/contracts/labels/validate_replacements.rb new file mode 100644 index 000000000000..b4b54585151a --- /dev/null +++ b/app/contracts/labels/validate_replacements.rb @@ -0,0 +1,77 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Labels + module ValidateReplacements + extend ActiveSupport::Concern + + included do + validate :validate_label_name_replacements + end + + private + + def validate_label_name_replacements + return if model.label_name_replacements.blank? + return unless writable?(:labels) + + if model.label_name_replacements.size > Labelable::MAX_LABEL_NAMES_PER_SAVE + errors.add(:labels, :too_many, count: Labelable::MAX_LABEL_NAMES_PER_SAVE) + return + end + + label_name_error_details.each { errors.add(:labels, it[:error], **it.except(:error)) } + end + + def label_name_error_details + names_to_create = label_names_to_create + return [] if names_to_create.empty? + + authorization = label_creation_authorization_errors(names_to_create.first) + (authorization + names_to_create.flat_map { label_name_errors(it) }).uniq + end + + def label_names_to_create + existing_names = Label.named_any(model.label_name_replacements).pluck(:name).to_set(&:downcase) + model.label_name_replacements.reject { existing_names.include?(it.downcase) } + end + + def label_creation_authorization_errors(name) + contract = Labels::CreateContract.new(Label.new(name:), user) + contract.validate + contract.errors.details[:base] + end + + def label_name_errors(name) + label = Label.new(name:) + label.valid? + label.errors.details[:name] + end + end +end diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 7bbc65e80410..e33684fd7e1f 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -30,6 +30,7 @@ module WorkPackages class BaseContract < ::ModelContract include ::Attachments::ValidateReplacements + include ::Labels::ValidateReplacements include AssignableValuesContract include WorkPackages::SetAttributesService::ProgressValuesCalculations diff --git a/app/models/concerns/labelable.rb b/app/models/concerns/labelable.rb index 9ecd958b9f67..a7e729ed16cd 100644 --- a/app/models/concerns/labelable.rb +++ b/app/models/concerns/labelable.rb @@ -30,56 +30,89 @@ module Labelable extend ActiveSupport::Concern + MAX_LABEL_NAMES_PER_SAVE = 50 + included do has_many :labelings, as: :labelable, dependent: :delete_all has_many :labels, -> { order(:id) }, through: :labelings scope :labeled_with, ->(label) { joins(:labelings).where(labelings: { label_id: label }) } + before_save :abort_save_with_label_names after_save :persist_label_associations validate :validate_label_id_replacements end - attr_reader :label_id_replacements + attr_reader :label_id_replacements, :label_name_replacements def label_id_replacements=(new_labels) @label_id_replacements = new_labels.nil? ? nil : normalize_label_ids(new_labels) end - def override_labels? = !label_id_replacements.nil? + def label_name_replacements=(names) + @label_name_replacements = names.nil? ? nil : normalize_label_names(names) + end + + def replace_label_names_with_ids(ids) + self.label_id_replacements = label_ids_to_assign | ids + self.label_name_replacements = nil + end + + def override_labels? = !label_id_replacements.nil? || !label_name_replacements.nil? def effective_labels return labels unless override_labels? - Label.where(id: label_id_replacements).order(:id) + Label.where(id: label_ids_to_assign).order(:id).to_a + labels_for_names_to_assign end def label_changes return {} unless override_labels? - return {} if label_id_replacements.sort == label_ids.sort + return {} if label_ids_to_assign.sort == label_ids.sort && label_names_to_assign.empty? - { "labels" => [label_ids, label_id_replacements] } + { "labels" => [label_ids, label_ids_to_assign + label_names_to_assign] } end private + def label_ids_to_assign = Array(label_id_replacements) + + def label_names_to_assign = Array(label_name_replacements) + def normalize_label_ids(new_labels) Array.wrap(new_labels) .filter_map { |label| label.respond_to?(:id) ? label.id : label.to_s.presence&.to_i } .uniq end + def normalize_label_names(names) + Array.wrap(names).map { it.to_s.squish }.compact_blank.uniq(&:downcase) + end + + def labels_for_names_to_assign + existing = Label.named_any(label_names_to_assign).index_by { it.name.downcase } + + label_names_to_assign.map { |name| existing[name.downcase] || Label.new(name:) } + end + def validate_label_id_replacements return unless override_labels? - missing = label_id_replacements - Label.where(id: label_id_replacements).pluck(:id) + missing = label_ids_to_assign - Label.where(id: label_ids_to_assign).pluck(:id) errors.add(:labels, :does_not_exist) if missing.any? end + def abort_save_with_label_names + return if label_names_to_assign.empty? + + errors.add(:labels, :invalid) + throw :abort + end + def persist_label_associations - replace_labels(label_id_replacements) if override_labels? + replace_labels(label_ids_to_assign) if override_labels? self.label_id_replacements = nil end diff --git a/app/models/label.rb b/app/models/label.rb index 6ad8f287f83a..b241ba3e8df1 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -58,6 +58,13 @@ class Label < ApplicationRecord where("LOWER(labels.name) = LOWER(?)", normalize_value_for(:name, name)) } + scope :named_any, ->(names) { + next none if names.blank? + + where("LOWER(labels.name) IN (SELECT LOWER(n) FROM unnest(ARRAY[?]::text[]) AS n)", + names.map { normalize_value_for(:name, it) }) + } + normalizes :name, with: -> { it.squish } validates :name, diff --git a/app/services/work_packages/create_service.rb b/app/services/work_packages/create_service.rb index 0acc001e1b3e..44582ee662ec 100644 --- a/app/services/work_packages/create_service.rb +++ b/app/services/work_packages/create_service.rb @@ -29,6 +29,7 @@ class WorkPackages::CreateService < BaseServices::BaseCallable include ::WorkPackages::Shared::UpdateAncestors + include ::WorkPackages::Shared::FindOrCreateLabels include ::Shared::ServiceContext include Types::ApplyPatterns @@ -59,20 +60,31 @@ def create(attributes, work_package) # Set attributes service passed, meaning the contract is fulfilled. # Avoid running validations again as we might be in a project copy scenario. work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements - work_package.save(validate: false) + return save_failure(result, work_package) if find_or_create_labels(work_package).failure? + return save_failure(result, work_package) unless work_package.save(validate: false) - apply_patterns(work_package) + after_create(result, work_package) + end - # update ancestors before rescheduling, as the parent might switch to automatic mode - multi_update_ancestors(result.all_results).each do |ancestor_result| - result.merge!(ancestor_result) - end + result + end - result.merge!(reschedule_related(work_package)) + def after_create(result, work_package) + apply_patterns(work_package) - set_user_as_watcher(work_package) + # update ancestors before rescheduling, as the parent might switch to automatic mode + multi_update_ancestors(result.all_results).each do |ancestor_result| + result.merge!(ancestor_result) end + result.merge!(reschedule_related(work_package)) + + set_user_as_watcher(work_package) + end + + def save_failure(result, work_package) + result.success = false + result.errors = work_package.errors result end diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 86d9fad37f32..7759957adfe3 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -86,8 +86,11 @@ def set_versions_attributes(attributes) def set_labels_attributes(attributes) label_ids = attributes.delete(:label_ids) + label_names = attributes.delete(:label_names) model.label_id_replacements = label_ids if label_ids + + model.label_name_replacements = label_names if label_names end def set_static_attributes(attributes) diff --git a/app/services/work_packages/shared/find_or_create_labels.rb b/app/services/work_packages/shared/find_or_create_labels.rb new file mode 100644 index 000000000000..360413ce88af --- /dev/null +++ b/app/services/work_packages/shared/find_or_create_labels.rb @@ -0,0 +1,71 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module WorkPackages + module Shared + module FindOrCreateLabels + def find_or_create_labels(work_package) + return ServiceResult.success(result: work_package) if work_package.label_name_replacements.blank? + + call = find_or_create_label_ids(work_package.label_name_replacements) + + if call.success? + work_package.replace_label_names_with_ids(call.result) + ServiceResult.success(result: work_package) + else + add_label_error(work_package, call.errors) + ServiceResult.failure(result: work_package, errors: work_package.errors) + end + end + + private + + def add_label_error(work_package, label_errors) + detail = label_errors.details[:name].first || { error: :invalid } + work_package.errors.add(:labels, detail[:error], **detail.except(:error)) + end + + def find_or_create_label_ids(names) + service = Labels::FindOrCreateService.new(user:) + ids = [] + + # A stable order keeps concurrent saves with overlapping new names from + # deadlocking on the unique name index. + names.sort_by(&:downcase).each do |name| + call = service.call(name:) + return call if call.failure? + + ids << call.result.id + end + + ServiceResult.success(result: ids) + end + end + end +end diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index 3b33630b1320..305a3ebb702b 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -29,6 +29,7 @@ class WorkPackages::UpdateService < BaseServices::Update include ::WorkPackages::Shared::UpdateAncestors + include ::WorkPackages::Shared::FindOrCreateLabels include Attachments::ReplaceAttachments include Types::ApplyPatterns @@ -41,6 +42,15 @@ def initialize(user:, model:, contract_class: nil, contract_options: {}, cause_o private + def after_validate(call) + labels_call = find_or_create_labels(call.result) + return super if labels_call.success? + + call.success = false + call.errors = labels_call.errors + call + end + def after_perform(service_call) # TODO: code smell here: saving the automatically generated subject depends # on running the UpdateAncestorsService right after. The subject gets saved diff --git a/config/locales/en.yml b/config/locales/en.yml index 08a11a28319f..070b028e4d51 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1047,6 +1047,8 @@ en: identifier: must_not_be_changed: "must not be changed manually. Work package identifiers are allocated automatically and resolved through the alias table, so changing them by hand breaks links and identifier history. If this change is deliberate, save with `save(context: :%{context})` and keep the work_package_semantic_aliases table consistent." semantic_identifier_incomplete: "and sequence_number must both be set at the same time." + labels: + too_many: "can't include more than %{count} new labels." observed_in_versions: error_readonly: "cannot be set because you are not allowed to assign versions in this project." parent: diff --git a/docs/api/apiv3/components/schemas/work_package_write_model.yml b/docs/api/apiv3/components/schemas/work_package_write_model.yml index 09dd6c8b885b..5a0037f91bb6 100644 --- a/docs/api/apiv3/components/schemas/work_package_write_model.yml +++ b/docs/api/apiv3/components/schemas/work_package_write_model.yml @@ -141,6 +141,13 @@ properties: description: |- List of labels attached to the work package + An entry whose `href` is `null` and whose `title` is a non-blank name creates that label when the work package + is saved, or reuses an existing label with the same name (case-insensitive). The form endpoint echoes such + entries back without creating anything. Creating a label requires the user to be allowed to edit work + packages in at least one project, as for `POST /api/v3/labels`. Names are limited to 255 characters, and a + single request may contain at most 50 entries without an `href`. A `title` is ignored on entries that have + an `href`. + **Resource**: Collection of Label parent: allOf: diff --git a/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.spec.ts b/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.spec.ts new file mode 100644 index 000000000000..a2f2f56d6baa --- /dev/null +++ b/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.spec.ts @@ -0,0 +1,153 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { TestBed } from '@angular/core/testing'; +import { Injector } from '@angular/core'; +import { of } from 'rxjs'; +import { provideHttpClient, withInterceptorsFromDi, withXhr } from '@angular/common/http'; +import { HalResourceService } from 'core-app/features/hal/services/hal-resource.service'; +import { SchemaCacheService } from 'core-app/core/schemas/schema-cache.service'; +import { SchemaResource } from 'core-app/features/hal/resources/schema-resource'; +import { SchemaProxy } from 'core-app/features/hal/schemas/schema-proxy'; +import { WorkPackageResource } from 'core-app/features/hal/resources/work-package-resource'; +import { WorkPackageChangeset } from 'core-app/features/work-packages/components/wp-edit/work-package-changeset'; +import { States } from 'core-app/core/states/states.service'; +import { I18nService } from 'core-app/core/i18n/i18n.service'; +import { ConfigurationService } from 'core-app/core/config/configuration.service'; +import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; +import { TimezoneService } from 'core-app/core/datetime/timezone.service'; +import { WeekdayService } from 'core-app/core/days/weekday.service'; +import { HalResourceNotificationService } from 'core-app/features/hal/services/hal-resource-notification.service'; +import { WorkPackageNotificationService } from 'core-app/features/work-packages/services/notifications/work-package-notification.service'; +import { WorkPackageCreateService } from 'core-app/features/work-packages/components/wp-new/wp-create.service'; +import { WorkPackagesActivityService } from 'core-app/features/work-packages/components/wp-single-view-tabs/activity-panel/wp-activity.service'; + +describe('WorkPackageChangeset labels', () => { + let halResourceService:HalResourceService; + let changeset:WorkPackageChangeset; + + const buildPayload = () => (changeset as unknown as { buildPayloadFromChanges:() => { _links:Record } }) + .buildPayloadFromChanges(); + + beforeEach(() => { + void TestBed.configureTestingModule({ + providers: [ + I18nService, + States, + TimezoneService, + PathHelperService, + ConfigurationService, + HalResourceService, + SchemaCacheService, + WorkPackagesActivityService, + { provide: WeekdayService, useValue: { loadWeekdays: () => of(true) } }, + { provide: HalResourceNotificationService, useValue: { handleRawError: () => false } }, + { provide: WorkPackageNotificationService, useValue: {} }, + { provide: WorkPackageCreateService, useValue: {} }, + provideHttpClient(withXhr(), withInterceptorsFromDi()), + ], + }).compileComponents(); + + halResourceService = TestBed.inject(Injector).get(HalResourceService); + + const workPackage = halResourceService.createHalResourceOfClass(WorkPackageResource, { + _type: 'WorkPackage', + id: '1', + lockVersion: 3, + _links: { + self: { href: '/api/v3/work_packages/1' }, + update: { href: '/api/v3/work_packages/1/form', method: 'post' }, + labels: [{ href: '/api/v3/labels/1', title: 'Bug' }], + }, + }) as unknown as WorkPackageResource; + + const schema = halResourceService.createHalResourceOfClass(SchemaResource, { + _type: 'Schema', + labels: { name: 'Labels', type: '[]Label', writable: true }, + }); + + vi.spyOn(SchemaCacheService.prototype, 'of').mockReturnValue(SchemaProxy.create(schema, workPackage)); + + changeset = new WorkPackageChangeset(workPackage); + }); + + afterEach(() => vi.restoreAllMocks()); + + const existing = () => halResourceService.createHalResourceOfType('Label', { + _links: { self: { href: '/api/v3/labels/1', title: 'Bug' } }, + }); + + it('sends new record labels as title-only links next to existing ones', () => { + const newRecord = halResourceService.createHalResourceOfType('Label', { id: null, name: 'Needs triage', href: null }); + changeset.setValue('labels', [existing(), newRecord]); + + expect(buildPayload()._links.labels).toEqual([ + { href: '/api/v3/labels/1' }, + { href: null, title: 'Needs triage' }, + ]); + }); + + it('handles new record labels echoed back by the form', () => { + const echoed = halResourceService.createHalResourceOfType('Label', { + _links: { self: { href: null, title: 'Needs triage' } }, + }); + changeset.setValue('labels', [existing(), echoed]); + + expect(buildPayload()._links.labels).toEqual([ + { href: '/api/v3/labels/1' }, + { href: null, title: 'Needs triage' }, + ]); + }); + + it('leaves labels out of the payload when they were not changed', () => { + expect(buildPayload()._links.labels).toBeUndefined(); + }); + + it('keeps an emptied label list', () => { + changeset.setValue('labels', []); + + expect(buildPayload()._links.labels).toEqual([]); + }); + + it('sends new record labels even when the reference resource has no labels link', () => { + const bare = halResourceService.createHalResourceOfClass(WorkPackageResource, { + _type: 'WorkPackage', + id: '2', + lockVersion: 1, + _links: { self: { href: '/api/v3/work_packages/2' } }, + }) as unknown as WorkPackageResource; + changeset = new WorkPackageChangeset(bare); + + const newRecord = halResourceService.createHalResourceOfType('Label', { id: null, name: 'Needs triage', href: null }); + changeset.setValue('labels', [newRecord]); + + const payload = buildPayload(); + + expect(payload._links.labels).toEqual([{ href: null, title: 'Needs triage' }]); + expect(payload).not.toHaveProperty('labels'); + }); +}); diff --git a/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.ts b/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.ts index bebebb1f3a3f..1ea6ffb6baac 100644 --- a/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.ts +++ b/frontend/src/app/features/work-packages/components/wp-edit/work-package-changeset.ts @@ -29,6 +29,7 @@ import { WorkPackageResource } from 'core-app/features/hal/resources/work-packag import { ChangesetPayload, ResourceChangeset } from 'core-app/shared/components/fields/changeset/resource-changeset'; import { ISchemaProxy } from 'core-app/features/hal/schemas/schema-proxy'; import { WorkPackageSchemaProxy } from 'core-app/features/hal/schemas/work-package-schema-proxy'; +import { HalResource } from 'core-app/features/hal/resources/hal-resource'; import isNewResource from 'core-app/features/hal/helpers/is-new-resource'; export class WorkPackageChangeset extends ResourceChangeset { @@ -74,7 +75,22 @@ export class WorkPackageChangeset extends ResourceChangeset } } - return super.applyChanges(payload); + return this.applyLabelLinks(super.applyChanges(payload)); + } + + private applyLabelLinks(payload:ChangesetPayload):ChangesetPayload { + const labels = this.changeset.getValue('labels'); + + if (!Array.isArray(labels) || !this.schema.isAttributeEditable('labels')) { + return payload; + } + + delete payload.labels; + payload._links.labels = (labels as HalResource[]) + .filter((label) => Boolean(label.href) || Boolean(label.name)) + .map((label) => (label.href ? { href: label.href } : { href: null, title: label.name })); + + return payload; } protected setNewDefaultFor(key:string, val:unknown) { diff --git a/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.spec.ts b/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.spec.ts index b28f87c09bc4..8a2401b36dc9 100644 --- a/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.spec.ts +++ b/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.spec.ts @@ -28,25 +28,21 @@ import { NO_ERRORS_SCHEMA } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { provideHttpClient, withInterceptorsFromDi, withXhr } from '@angular/common/http'; -import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; +import { provideHttpClientTesting } from '@angular/common/http/testing'; import { NgSelectModule } from '@ng-select/ng-select'; import { of } from 'rxjs'; import { States } from 'core-app/core/states/states.service'; import { I18nService } from 'core-app/core/i18n/i18n.service'; -import { HalResourceNotificationService } from 'core-app/features/hal/services/hal-resource-notification.service'; import { CurrentUserService } from 'core-app/core/current-user/current-user.service'; import { LabelsAutocompleterComponent } from './labels-autocompleter.component'; describe('LabelsAutocompleterComponent', () => { let component:LabelsAutocompleterComponent; - let httpMock:HttpTestingController; - let halNotification:{ handleRawError:ReturnType }; let currentUser:{ hasCapabilities$:ReturnType }; const i18nStub = { t: (key:string) => key }; beforeEach(async () => { - halNotification = { handleRawError: vi.fn() }; currentUser = { hasCapabilities$: vi.fn(() => of(true)) }; await TestBed.configureTestingModule({ @@ -58,68 +54,19 @@ describe('LabelsAutocompleterComponent', () => { provideHttpClient(withXhr(), withInterceptorsFromDi()), provideHttpClientTesting(), { provide: I18nService, useValue: i18nStub }, - { provide: HalResourceNotificationService, useValue: halNotification }, { provide: CurrentUserService, useValue: currentUser }, ], }).compileComponents(); component = TestBed.createComponent(LabelsAutocompleterComponent).componentInstance; - httpMock = TestBed.inject(HttpTestingController); }); - afterEach(() => { - httpMock.verify(); - }); - - function pendingRequest() { - return httpMock.expectOne((req) => req.method === 'POST' && req.url.endsWith('/labels')); - } - it('does not offer to create a blank term', () => { expect(component.createLabel(' ')).toBeUndefined(); }); - it('resolves with the created label on success', async () => { - const promise = component.createLabel('Urgent'); - - pendingRequest().flush({ id: 5, name: 'Urgent', _links: { self: { href: '/api/v3/labels/5' } } }); - - await expect(promise).resolves.toEqual({ id: 5, name: 'Urgent', href: '/api/v3/labels/5' }); - }); - - it('delegates errors to the generic handler and rejects', async () => { - const promise = component.createLabel('Urgent'); - - pendingRequest().flush({ _type: 'Error' }, { status: 422, statusText: 'Unprocessable Entity' }); - - await expect(promise).rejects.toBeTruthy(); - expect(halNotification.handleRawError).toHaveBeenCalled(); - }); - - it('ignores a second call while a request is in flight, then allows a retry', async () => { - const first = component.createLabel('Urgent'); - const second = component.createLabel('Urgent'); - - expect(second).toBeUndefined(); - - pendingRequest().flush({ id: 5, name: 'Urgent', _links: { self: { href: '/api/v3/labels/5' } } }); - await first; - - const retry = component.createLabel('Urgent'); - expect(retry).toBeDefined(); - pendingRequest().flush({ id: 5, name: 'Urgent', _links: { self: { href: '/api/v3/labels/5' } } }); - await retry; - }); - - it('resets the in-flight flag after an error so a retry can be sent', async () => { - const first = component.createLabel('Urgent'); - pendingRequest().flush({ _type: 'Error' }, { status: 500, statusText: 'Internal Server Error' }); - await expect(first).rejects.toBeTruthy(); - - const retry = component.createLabel('Urgent'); - expect(retry).toBeDefined(); - pendingRequest().flush({ id: 5, name: 'Urgent', _links: { self: { href: '/api/v3/labels/5' } } }); - await retry; + it('returns a label with no href for a trimmed term', () => { + expect(component.createLabel(' Needs triage ')).toEqual({ id: null, name: 'Needs triage', href: null }); }); it('enables addTag once the project grants work_packages/update, without throwing change-detection errors', () => { @@ -148,4 +95,13 @@ describe('LabelsAutocompleterComponent', () => { expect(fixture.componentInstance.addTag).toBe(false); expect(currentUser.hasCapabilities$).not.toHaveBeenCalled(); }); + + it('treats a label with no href and a persisted label with the same name as the same label, ignoring case', () => { + const unsaved = { id: null, name: 'Needs Triage', href: null }; + const persisted = { id: 9, name: 'needs triage', href: '/api/v3/labels/9' }; + const other = { id: 10, name: 'Blocked', href: '/api/v3/labels/10' }; + + expect(component.compareWith?.(unsaved, persisted)).toBe(true); + expect(component.compareWith?.(unsaved, other)).toBe(false); + }); }); diff --git a/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.ts b/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.ts index d22b2e0b834f..a3739d3fb9ca 100644 --- a/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.ts +++ b/frontend/src/app/shared/components/autocompleter/labels-autocompleter/labels-autocompleter.component.ts @@ -27,16 +27,14 @@ import { ChangeDetectionStrategy, Component, Input, OnInit, inject } from '@angular/core'; import { keyBy } from 'lodash-es'; -import { BehaviorSubject, firstValueFrom, Observable, of, throwError } from 'rxjs'; -import { catchError, finalize, map, switchMap } from 'rxjs/operators'; +import { BehaviorSubject, Observable, of } from 'rxjs'; +import { map, switchMap } from 'rxjs/operators'; import { OpAutocompleterComponent, } from 'core-app/shared/components/autocompleter/op-autocompleter/op-autocompleter.component'; import { ApiV3FilterBuilder } from 'core-app/shared/helpers/api-v3/api-v3-filter-builder'; import { addFiltersToPath } from 'core-app/core/apiv3/helpers/add-filters-to-path'; import { IHALCollection } from 'core-app/core/apiv3/types/hal-collection.type'; -import { compareByAttribute } from 'core-app/shared/helpers/angular/tracking-functions'; -import { HalResourceNotificationService } from 'core-app/features/hal/services/hal-resource-notification.service'; import { CurrentUserService } from 'core-app/core/current-user/current-user.service'; import { LabelsAutocompleterTemplateComponent, @@ -45,7 +43,7 @@ import { export const labelsAutocompleterSelector = 'op-labels-autocompleter'; export interface ILabelAutocompleteItem { - id:string|number; + id:string|number|null; name:string; href:string|null; } @@ -67,12 +65,8 @@ export class LabelsAutocompleterComponent extends OpAutocompleterComponent(undefined); @Input() @@ -118,32 +112,26 @@ export class LabelsAutocompleterComponent extends OpAutocompleterComponent|undefined { + public createLabel(searchTerm:string):ILabelAutocompleteItem|undefined { const name = searchTerm.trim(); - if (!name || this.creatingLabel) { + if (!name) { return undefined; } - this.creatingLabel = true; - - return firstValueFrom( - this - .http - .post(this.apiV3Service.labels.toString(), { name }) - .pipe( - map((label) => ({ id: label.id, name: label.name, href: label._links.self.href })), - catchError((error:unknown) => { - this.halNotification.handleRawError(error); - - return throwError(() => error); - }), - finalize(() => { this.creatingLabel = false; }), - ), - ); + return { id: null, name, href: null }; } - protected defaultCompareWithFunction():(a:unknown, b:unknown) => boolean { - return compareByAttribute('href', 'name'); + protected defaultCompareWithFunction():(option:unknown, selected:unknown) => boolean { + return (option, selected) => { + const optionLabel = option as ILabelAutocompleteItem|null|undefined; + const selectedLabel = selected as ILabelAutocompleteItem|null|undefined; + + if (!optionLabel || !selectedLabel) { + return !optionLabel && !selectedLabel; + } + + return optionLabel.name.toLowerCase() === selectedLabel.name.toLowerCase(); + }; } protected defaultTrackByFunction():(item:{ href:unknown, name:unknown }) => unknown { diff --git a/frontend/src/app/shared/components/autocompleter/op-autocompleter/op-autocompleter.component.ts b/frontend/src/app/shared/components/autocompleter/op-autocompleter/op-autocompleter.component.ts index aef96e942c52..7116aac328a0 100644 --- a/frontend/src/app/shared/components/autocompleter/op-autocompleter/op-autocompleter.component.ts +++ b/frontend/src/app/shared/components/autocompleter/op-autocompleter/op-autocompleter.component.ts @@ -64,7 +64,7 @@ import { UserResource } from 'core-app/features/hal/resources/user-resource'; import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; export interface IAutocompleteItem { - id:ID; + id:ID|null; href:string|null; } diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index 1b58566f3a41..ab0adcb13a60 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -636,7 +636,7 @@ def self_v3_path(*) getter: ->(*) { next unless embed_link?(:labels) - represented.effective_labels.map do |label| + represented.effective_labels.reject(&:new_record?).map do |label| ::API::V3::Labels::LabelRepresenter.create(label, current_user:) end }, @@ -652,7 +652,16 @@ def self_v3_path(*) title_attribute: :name) .to_hash end - } + }, + setter: ->(fragment:, **) do + next unless OpenProject::FeatureDecisions.work_package_labels_active? + + with_href, without_href = fragment.partition { |link| link["href"].present? } + represented.label_ids = parse_link_ids_from_fragment(with_href, :label) + + names = without_href.filter_map { |link| link["title"].presence if link["title"].is_a?(String) } + represented.label_names = names if names.any? + end associated_resource :parent, v3_path: :work_package, diff --git a/spec/contracts/work_packages/create_contract_spec.rb b/spec/contracts/work_packages/create_contract_spec.rb index a5ebfc741306..dd1bb1b463d6 100644 --- a/spec/contracts/work_packages/create_contract_spec.rb +++ b/spec/contracts/work_packages/create_contract_spec.rb @@ -124,6 +124,22 @@ def resolve_written_value(value) it_behaves_like "can write", :author_id, -> { user.id + 1 } end + context "with a pending label name for a user who may not create labels" do + before do + work_package.label_name_replacements = ["Brand new"] + end + + after do + work_package.label_name_replacements = nil + end + + it "is invalid with error_unauthorized on labels" do + contract.validate + + expect(contract.errors.symbols_for(:labels)).to include(:error_unauthorized) + end + end + context "when disabled for admin", with_settings: { apiv3_write_readonly_attributes: false } do let(:user) { build_stubbed(:admin) } diff --git a/spec/contracts/work_packages/shared_contract_examples.rb b/spec/contracts/work_packages/shared_contract_examples.rb index 71bd9b6ce6e7..5324576451ac 100644 --- a/spec/contracts/work_packages/shared_contract_examples.rb +++ b/spec/contracts/work_packages/shared_contract_examples.rb @@ -265,6 +265,7 @@ after do work_package.label_id_replacements = nil + work_package.label_name_replacements = nil end before do @@ -272,6 +273,69 @@ end it_behaves_like "contract is valid" + + context "with a new label name the user may create" do + let(:permissions) { super() + %i[edit_work_packages] } + + before do + work_package.label_name_replacements = ["Brand new"] + end + + it_behaves_like "contract is valid" + end + + context "with a new label name that is too long" do + let(:permissions) { super() + %i[edit_work_packages] } + + before do + work_package.label_name_replacements = ["x" * 300] + end + + it_behaves_like "contract is invalid", labels: :too_long + + it "renders the length limit in the message" do + contract.validate + + expect(contract.errors.full_messages_for(:labels)) + .to contain_exactly("Labels is too long (maximum is 255 characters).") + end + end + + context "with more label names than allowed" do + let(:permissions) { super() + %i[edit_work_packages] } + + before do + work_package.label_name_replacements = Array.new(Labelable::MAX_LABEL_NAMES_PER_SAVE + 1) { "Label #{it}" } + end + + it_behaves_like "contract is invalid", labels: :too_many + end + + context "with several new label names the user may create" do + let(:permissions) { super() + %i[edit_work_packages] } + + before do + work_package.label_name_replacements = ["First", "Second", "Third"] + allow(Labels::CreateContract).to receive(:new).and_call_original + end + + it "checks the permission to create labels once" do + contract.validate + + expect(Labels::CreateContract).to have_received(:new).once + end + end + + context "with a label name that already exists in different casing" do + let(:permissions) { super() + %i[edit_work_packages] } + + before do + create(:label, name: "Existing") + work_package.label_name_replacements = ["EXISTING"] + end + + it_behaves_like "contract is valid" + end end describe "ignore_non_working_days" do diff --git a/spec/features/work_packages/details/inplace_editor/labels_editor_spec.rb b/spec/features/work_packages/details/inplace_editor/labels_editor_spec.rb index ee608019ec55..1f59983cdccf 100644 --- a/spec/features/work_packages/details/inplace_editor/labels_editor_spec.rb +++ b/spec/features/work_packages/details/inplace_editor/labels_editor_spec.rb @@ -5,6 +5,7 @@ require "features/work_packages/shared_contexts" require "support/edit_fields/edit_field" require "features/work_packages/work_packages_page" +require "features/page_objects/notification" RSpec.describe "labels inplace editor", :js, with_flag: { work_package_labels: true } do let(:project) { create(:project) } @@ -15,6 +16,7 @@ create(:user, member_with_permissions: { project => %i[view_work_packages edit_work_packages] }) end + let(:too_long_name) { "x" * 256 } before do login_as(user) @@ -26,6 +28,18 @@ def create_option_text(name) /#{Regexp.escape(prefix)}\s*"#{Regexp.escape(name)}"/ end + def pick_new_label(name) + field.autocomplete(name, select: false) + field.autocomplete_selector.send_keys(:return) + end + + def expect_too_long_label_rejected + PageObjects::Notifications.new(page) + .expect_error("Labels #{I18n.t('activerecord.errors.messages.too_long', count: 255)}") + field.expect_error + expect(Label.named(too_long_name)).to be_empty + end + context "in the full view" do let(:work_package_page) { Pages::FullWorkPackage.new(work_package) } let(:field) { work_package_page.edit_field(:labels) } @@ -61,13 +75,31 @@ def create_option_text(name) field.autocomplete_selector.send_keys(:return) field.expect_selected_values(label.name, new_label_name) + expect(Label.named(new_label_name)).to be_empty + field.submit_by_dashboard field.expect_state_text(new_label_name) - expect(Label.named(new_label_name)).to be_present + expect(Label.named(new_label_name).count).to eq(1) expect(work_package.reload.labels.map(&:name)).to contain_exactly(label.name, new_label_name) end + it "does not create a label when the edit with a new label is cancelled" do + new_label_name = "Urgent" + + field.activate! + field.autocomplete(new_label_name, select: false) + field.autocomplete_selector.send_keys(:return) + field.expect_selected_values(label.name, new_label_name) + + field.cancel_by_escape + + field.expect_inactive! + field.expect_state_text(label.name) + expect(Label.named(new_label_name)).to be_empty + expect(work_package.reload.labels).to contain_exactly(label) + end + it "offers no create option for an existing name in a different case" do field.activate! @@ -117,6 +149,16 @@ def create_option_text(name) field.activate! field.expect_selected_values(label.name) end + + it "shows the validation error when a new label name is too long" do + field.activate! + pick_new_label(too_long_name) + field.expect_selected_values(label.name, too_long_name) + + field.submit_by_dashboard + + expect_too_long_label_rejected + end end context "in the create form" do @@ -159,14 +201,50 @@ def create_option_text(name) field.autocomplete_selector.send_keys(:return) field.expect_selected_values(new_label_name) + expect(Label.named(new_label_name)).to be_empty work_package_page.set_attributes({ subject: "Work package with a new label" }) work_package_page.save! work_package_page.expect_and_dismiss_toaster(message: "Successful creation.") - expect(Label.named(new_label_name)).to be_present + expect(Label.named(new_label_name).count).to eq(1) expect(WorkPackage.last.labels.map(&:name)).to contain_exactly(new_label_name) end + + it "shows the validation error when a new label name is too long" do + pick_new_label(too_long_name) + field.expect_selected_values(too_long_name) + + work_package_page.set_attributes({ subject: "Work package with a too long label" }) + work_package_page.save! + + expect_too_long_label_rejected + expect(WorkPackage.where(subject: "Work package with a too long label")).to be_empty + end + end + end + + context "in the work package table" do + let(:wp_table) { Pages::WorkPackagesTable.new(project) } + let(:field) { wp_table.edit_field(work_package, :labels) } + let!(:query) do + create(:query, user:, project:, column_names: %w[id subject labels]) + end + + before do + create(:labeling, label:, labelable: work_package) + wp_table.visit_query query + wp_table.expect_work_package_listed work_package + end + + it "shows the validation error when a new label name is too long" do + field.activate! + pick_new_label(too_long_name) + field.expect_selected_values(label.name, too_long_name) + + field.submit_by_dashboard + + expect_too_long_label_rejected end end end diff --git a/spec/lib/api/v3/work_packages/work_package_payload_representer_parsing_spec.rb b/spec/lib/api/v3/work_packages/work_package_payload_representer_parsing_spec.rb index b6b178c6d013..47bcf1b1726a 100644 --- a/spec/lib/api/v3/work_packages/work_package_payload_representer_parsing_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_payload_representer_parsing_spec.rb @@ -155,4 +155,63 @@ expect(subject.observed_in_version_ids).to eq([version.id.to_s, other_version.id.to_s]) end end + + describe "labels", with_flag: { work_package_labels: true } do + let(:label) { build_stubbed(:label) } + let(:other_label) { build_stubbed(:label) } + let(:labels_links) { [] } + let(:hash) { { _links: { "labels" => labels_links } } } + + context "with existing labels and a pending one" do + let(:labels_links) do + [ + { "href" => api_v3_paths.label(label.id) }, + { "href" => api_v3_paths.label(other_label.id) }, + { "href" => nil, "title" => "Needs triage" } + ] + end + + it "writes the ids to label_ids and the pending names to label_names" do + expect(subject.label_ids).to eq([label.id.to_s, other_label.id.to_s]) + expect(subject.label_names).to eq(["Needs triage"]) + end + end + + context "with an entry lacking both href and title" do + let(:labels_links) { [{ "href" => nil }] } + + it "clears the ids and sets no names" do + expect(subject.label_ids).to eq([]) + expect(subject.to_h).not_to have_key(:label_names) + end + end + + context "with a title that is not a string" do + let(:labels_links) { [{ "href" => nil, "title" => { "a" => 1 } }] } + + it "sets no names" do + expect(subject.to_h).not_to have_key(:label_names) + end + end + + context "with an empty array" do + it "clears the ids" do + expect(subject.label_ids).to eq([]) + end + end + + context "with the feature flag off", with_flag: { work_package_labels: false } do + let(:labels_links) do + [ + { "href" => api_v3_paths.label(label.id) }, + { "href" => nil, "title" => "Needs triage" } + ] + end + + it "ignores the labels links" do + expect(subject.label_ids).to be_nil + expect(subject.label_names).to be_nil + end + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 0a93f90002f7..3be96dcf1cca 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -104,6 +104,22 @@ def label_work_packages(label, project, count:) end end + describe ".named_any" do + it "finds labels regardless of casing and surrounding whitespace" do + alpha = create(:label, name: "Alpha") + bravo = create(:label, name: "Bravo") + create(:label, name: "Charlie") + + expect(described_class.named_any([" alpha ", "BRAVO", "Missing"])).to contain_exactly(alpha, bravo) + end + + it "finds nothing for empty input" do + create(:label, name: "Alpha") + + expect(described_class.named_any([])).to be_empty + end + end + describe ".page_of" do let!(:alpha) { create(:label, name: "Alpha") } let!(:bravo) { create(:label, name: "bravo") } diff --git a/spec/models/work_package/work_package_labels_spec.rb b/spec/models/work_package/work_package_labels_spec.rb index 277c494f5a94..762a3ab80795 100644 --- a/spec/models/work_package/work_package_labels_spec.rb +++ b/spec/models/work_package/work_package_labels_spec.rb @@ -197,4 +197,81 @@ def persisted_label_ids = Labeling.where(labelable: work_package).pluck(:label_i end end end + + describe "#label_name_replacements=" do + before { work_package.reload } + + def persisted_label_ids = Labeling.where(labelable: work_package).pluck(:label_id) + + it "squishes names and drops blanks" do + work_package.label_name_replacements = [" New name ", "", nil, " "] + + expect(work_package.label_name_replacements).to eq(["New name"]) + end + + it "drops case-insensitive duplicates, keeping the first spelling" do + work_package.label_name_replacements = ["Urgent", "urgent", "URGENT", "Other"] + + expect(work_package.label_name_replacements).to eq(%w[Urgent Other]) + end + + it "stays nil when assigned nil" do + work_package.label_name_replacements = nil + + expect(work_package.label_name_replacements).to be_nil + expect(work_package).not_to be_override_labels + end + + context "with names only" do + before { work_package.label_name_replacements = ["Brand new", "APPLE"] } + + it "overrides the labels" do + expect(work_package).to be_override_labels + end + + it "matches existing names case-insensitively and builds unsaved labels for new ones" do + labels = work_package.effective_labels + + expect(labels).to include(lower_label) + new_label = labels.find { it.name == "Brand new" } + expect(new_label).to be_new_record + expect(Label.where(name: "Brand new")).not_to exist + end + + it "reports the pending names as a change" do + expect(work_package.label_changes) + .to eq("labels" => [[lower_label.id, higher_label.id], ["Brand new", "APPLE"]]) + end + + it "is valid" do + expect(work_package).to be_valid + end + end + + describe "saving" do + it "refuses to save while label names have no ids and creates no label" do + work_package.label_name_replacements = ["Brand new"] + + expect { expect(work_package.save).to be(false) }.not_to change(Label, :count) + expect(work_package.errors.symbols_for(:labels)).to contain_exactly(:invalid) + expect(persisted_label_ids).to contain_exactly(lower_label.id, higher_label.id) + end + + it "saves once the label names are replaced with ids" do + work_package.label_name_replacements = ["Brand new"] + work_package.replace_label_names_with_ids([lower_label.id]) + work_package.save! + + expect(persisted_label_ids).to contain_exactly(lower_label.id) + expect(work_package.label_name_replacements).to be_nil + end + + it "records the labels in the journal" do + work_package.replace_label_names_with_ids([lower_label.id]) + work_package.save! + + expect(work_package.journals.last.label_journals.pluck(:label_id)).to contain_exactly(lower_label.id) + end + end + end end diff --git a/spec/requests/api/v3/work_packages/create_form_resource_spec.rb b/spec/requests/api/v3/work_packages/create_form_resource_spec.rb index 25abd042cd67..d3e32d2e5407 100644 --- a/spec/requests/api/v3/work_packages/create_form_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/create_form_resource_spec.rb @@ -149,6 +149,45 @@ end end + describe "labels", with_flag: :work_package_labels do + let(:parameters) do + { + _links: { + project: { href: api_v3_paths.project(project.id) }, + labels: [{ href: nil, title: "Needs triage" }] + }, + subject: "lorem ipsum" + } + end + + it "has 0 validation errors" do + expect(subject.body).to have_json_size(0).at_path("_embedded/validationErrors") + end + + it "echoes the pending name without creating the label" do + expect(subject.body) + .to be_json_eql({ href: nil, title: "Needs triage" }.to_json) + .at_path("_embedded/payload/_links/labels/0") + expect(Label.where(name: "Needs triage")).not_to exist + end + + context "for a user who may add work packages but not create labels" do + shared_let(:user) do + create(:user, member_with_permissions: { project => %i[view_project view_work_packages add_work_packages] }) + end + + it "has a validation error on labels" do + expect(subject.body).to have_json_path("_embedded/validationErrors/labels") + end + + it "reports the unauthorized message" do + expect(subject.body) + .to be_json_eql("Labels #{I18n.t('activerecord.errors.messages.error_unauthorized')}".to_json) + .at_path("_embedded/validationErrors/labels/message") + end + end + end + describe "with empty version links and no project (as sent back by the global creation form)" do let(:parameters) 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 7a34b41eee91..5a251f245ab1 100644 --- a/spec/requests/api/v3/work_packages/create_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/create_resource_spec.rb @@ -296,6 +296,45 @@ end end + describe "labels", with_flag: :work_package_labels do + let(:extra_permissions) { %i[edit_work_packages] } + let(:parameters) do + super().deep_merge(_links: { labels: [{ href: nil, title: "Needs triage" }] }) + end + let(:created_work_package) { WorkPackage.find_by(subject: "new work packages") } + + context "with a title for a label that does not exist yet" do + it "returns Created(201)" do + expect(last_response).to have_http_status(:created) + end + + it "creates the label and assigns it" do + label = Label.find_by!(name: "Needs triage") + + expect(created_work_package.labels).to contain_exactly(label) + end + end + + context "for a user who may add work packages but not create labels" do + let(:extra_permissions) { [] } + + it "returns 422" do + expect(last_response).to have_http_status(:unprocessable_entity) + end + + it "reports the labels as unauthorized" do + expect(last_response.body) + .to be_json_eql("Labels #{I18n.t('activerecord.errors.messages.error_unauthorized')}".to_json) + .at_path("message") + end + + it "creates neither the work package nor the label" do + expect(created_work_package).to be_nil + expect(Label.count).to eq(0) + end + end + end + describe "targetVersions" do let(:target_version) { create(:version, project:) } let(:extra_permissions) { %i[assign_versions] } diff --git a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb index d830cd44a5a0..c693e0eccfae 100644 --- a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb @@ -217,6 +217,66 @@ it "does not persist them" do expect(Labeling.where(labelable: work_package)).not_to exist end + + context "with a title for a label that does not exist yet" do + let(:params) do + valid_params.merge(_links: { labels: [{ href: nil, title: "Needs triage" }] }) + end + + it_behaves_like "having no errors" + + it "echoes the pending name back without a href" do + expect(last_response.body) + .to be_json_eql({ href: nil, title: "Needs triage" }.to_json) + .at_path("_embedded/payload/_links/labels/0") + end + + it "creates neither the label nor the labeling" do + expect(Label.where(name: "Needs triage")).not_to exist + expect(Labeling.where(labelable: work_package)).not_to exist + end + end + + context "with a title for a label that does not exist yet by a user who may not create labels" do + let(:current_user) do + create(:user).tap do |user| + create(:work_package_member, + principal: user, + entity: work_package, + roles: [create(:edit_work_package_role)]) + end + end + let(:params) do + valid_params.merge(_links: { labels: [{ href: nil, title: "Needs triage" }] }) + end + + it "reports the labels as unauthorized" do + expect(last_response.body) + .to be_json_eql("Labels #{I18n.t('activerecord.errors.messages.error_unauthorized')}".to_json) + .at_path("_embedded/validationErrors/labels/message") + end + + it "creates no label" do + expect(Label.count).to eq(0) + end + end + + context "with a title matching an existing label in different casing" do + let(:label) { create(:label, name: "Needs triage") } + let(:params) do + valid_params.merge(_links: { labels: [{ href: nil, title: label.name.upcase }] }) + end + + it "echoes the existing label's href" do + expect(last_response.body) + .to be_json_eql(api_v3_paths.label(label.id).to_json) + .at_path("_embedded/payload/_links/labels/0/href") + end + + it "does not create another label" do + expect(Label.count).to eq(1) + end + end end context "for invalid content" do diff --git a/spec/requests/api/v3/work_packages/update_resource_spec.rb b/spec/requests/api/v3/work_packages/update_resource_spec.rb index be390b75c400..1fd31d0d7899 100644 --- a/spec/requests/api/v3/work_packages/update_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/update_resource_spec.rb @@ -804,6 +804,32 @@ end end + context "with a new title from a user who may edit the work package but not create labels" do + let(:label_links) { [{ href: nil, title: "Needs triage" }] } + let(:current_user) do + create(:user).tap do |user| + create(:work_package_member, + principal: user, + entity: work_package, + roles: [create(:edit_work_package_role)]) + end + end + + include_context "patch request" + + it { expect(response).to have_http_status(:unprocessable_entity) } + + it "reports the labels as unauthorized" do + expect(response.body) + .to be_json_eql("Labels #{I18n.t('activerecord.errors.messages.error_unauthorized')}".to_json) + .at_path("message") + end + + it "creates no label" do + expect(Label.count).to eq(0) + end + end + context "when another attribute of the same request is invalid" do let(:params) { valid_params.merge(subject: "", _links: { labels: label_links }) } @@ -816,6 +842,77 @@ end end + context "with a title for a label that does not exist yet" do + let(:label_links) { [{ href: nil, title: "Needs triage" }] } + let(:created_label) { Label.find_by!(name: "Needs triage") } + + include_context "patch request" + + it { expect(response).to have_http_status(:ok) } + + it "creates the label as the current user and assigns it" do + expect(created_label.author).to eq(current_user) + expect(work_package.reload.labels).to contain_exactly(created_label) + end + + it "responds with the href of the created label" do + expect(response.body) + .to be_json_eql(api_v3_paths.label(created_label.id).to_json) + .at_path("_links/labels/0/href") + end + end + + context "with a title matching an existing label in different casing" do + let!(:label) { create(:label, name: "Needs triage") } + let(:label_links) { [{ href: nil, title: "NEEDS TRIAGE" }] } + + include_context "patch request" + + it { expect(response).to have_http_status(:ok) } + + it "reuses the existing label" do + expect(Label.count).to eq(1) + expect(work_package.reload.labels).to contain_exactly(label) + end + end + + context "with a title and another attribute of the same request being invalid" do + let(:params) { valid_params.merge(subject: "", _links: { labels: [{ href: nil, title: "Needs triage" }] }) } + + include_context "patch request" + + it { expect(response).to have_http_status(:unprocessable_entity) } + + it "does not create the label" do + expect(Label.where(name: "Needs triage")).not_to exist + end + end + + context "with the feature flag off", with_flag: { work_package_labels: false } do + let(:label_links) { [{ href: nil, title: "Needs triage" }] } + + include_context "patch request" + + it { expect(response).to have_http_status(:ok) } + + it "creates no label" do + expect(Label.count).to eq(0) + end + end + + context "with a null href and no title" do + let(:label_links) { [{ href: nil }] } + + include_context "patch request" + + it { expect(response).to have_http_status(:ok) } + + it "creates no label and assigns none" do + expect(Label.count).to eq(0) + expect(work_package.reload.labels).to be_empty + end + end + context "for a user having assign_versions but lacking edit_work_packages permission" do let(:permissions) { %i[view_work_packages assign_versions] } @@ -832,6 +929,22 @@ it "does not assign the label" do expect(work_package.reload.labels).to be_empty end + + context "with a title for a label that does not exist yet" do + let(:label_links) { [{ href: nil, title: "Needs triage" }] } + + it { expect(response).to have_http_status(:unprocessable_entity) } + + it "has a readonly error" do + expect(response.body) + .to be_json_eql("urn:openproject-org:api:v3:errors:PropertyIsReadOnly".to_json) + .at_path("errorIdentifier") + end + + it "does not create the label" do + expect(Label.where(name: "Needs triage")).not_to exist + end + end end end diff --git a/spec/services/work_packages/create_service_integration_spec.rb b/spec/services/work_packages/create_service_integration_spec.rb index 4ae1506586a7..473ade8e0bbc 100644 --- a/spec/services/work_packages/create_service_integration_spec.rb +++ b/spec/services/work_packages/create_service_integration_spec.rb @@ -303,6 +303,62 @@ end end + describe "creating labels typed into the work package", with_flag: { work_package_labels: true } do + let(:role) do + create(:project_role, + permissions: %i[view_work_packages add_work_packages edit_work_packages]) + end + let(:attributes) { { subject: "Labelled", project:, label_names: %w[charlie Alpha bravo] } } + let(:saved_work_package) { WorkPackage.find_by!(subject: "Labelled") } + + def stub_first_label_lookup_miss + calls = 0 + allow(Label).to receive(:named).and_wrap_original do |original, *args| + (calls += 1) == 1 ? Label.none : original.call(*args) + end + end + + it "creates the labels in case-insensitive name order" do + expect(service_result).to be_success + + created = Label.where(name: %w[bravo Alpha charlie]).order(:id).pluck(:name) + expect(created).to eq(%w[Alpha bravo charlie]) + end + + it "reuses a label another save created after validation" do + other = create(:label, name: "bravo") + stub_first_label_lookup_miss + + expect(service_result).to be_success + expect(Label.where("LOWER(name) = 'bravo'").count).to eq(1) + expect(saved_work_package.labels).to include(other) + end + + it "rolls the created labels back when the work package cannot be saved" do + allow_any_instance_of(WorkPackage).to receive(:save).and_return(false) # rubocop:disable RSpec/AnyInstance + + expect { service_result }.not_to change(Label, :count) + expect(service_result).to be_failure + end + + context "when a label cannot be created" do + let(:failure) do + ServiceResult.failure(result: nil).tap { it.errors.add(:name, :blank) } + end + + before do + service = instance_double(Labels::FindOrCreateService, call: failure) + allow(Labels::FindOrCreateService).to receive(:new).and_return(service) + end + + it "fails with a labels error and persists nothing" do + expect(service_result).to be_failure + expect(service_result.errors.symbols_for(:labels)).to contain_exactly(:blank) + expect(WorkPackage.where(subject: "Labelled")).not_to exist + end + end + end + describe "versions, target versions and observed in versions" do let!(:version1) { create(:version, project:) } let!(:version2) { create(:version, project:) } diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index 0fb129f118db..f7847451e191 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -2398,6 +2398,18 @@ expect(work_package.label_id_replacements).to be_nil end + it "extracts label_names into name replacements" do + instance.call(label_names: ["Foo", " bar "]) + + expect(work_package.label_name_replacements).to eq %w[Foo bar] + end + + it "leaves name replacements nil when not passed" do + instance.call(subject: "foo") + + expect(work_package.label_name_replacements).to be_nil + end + it "sets empty array as override (does not set to nil)" do instance.call(label_ids: []) diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index d38a9e171079..603bbc7ebaa8 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -1894,6 +1894,61 @@ end end + describe "creating labels typed into the work package", with_flag: { work_package_labels: true } do + let(:attributes) { { label_names: %w[charlie Alpha bravo] } } + let(:saved_work_package) { work_package.reload } + let(:service_result) { subject } + + before { work_package } + + def stub_first_label_lookup_miss + calls = 0 + allow(Label).to receive(:named).and_wrap_original do |original, *args| + (calls += 1) == 1 ? Label.none : original.call(*args) + end + end + + it "creates the labels in case-insensitive name order" do + expect(service_result).to be_success + + created = Label.where(name: %w[bravo Alpha charlie]).order(:id).pluck(:name) + expect(created).to eq(%w[Alpha bravo charlie]) + end + + it "reuses a label another save created after validation" do + other = create(:label, name: "bravo") + stub_first_label_lookup_miss + + expect(service_result).to be_success + expect(Label.where("LOWER(name) = 'bravo'").count).to eq(1) + expect(saved_work_package.labels).to include(other) + end + + it "rolls the created labels back when the work package cannot be saved" do + allow_any_instance_of(WorkPackage).to receive(:save).and_return(false) # rubocop:disable RSpec/AnyInstance + + expect { service_result }.not_to change(Label, :count) + expect(service_result).to be_failure + end + + context "when a label cannot be created" do + let(:failure) do + ServiceResult.failure(result: nil).tap { it.errors.add(:name, :blank) } + end + + before do + service = instance_double(Labels::FindOrCreateService, call: failure) + allow(Labels::FindOrCreateService).to receive(:new).and_return(service) + end + + it "fails with a labels error and leaves the labels alone" do + expect(service_result).to be_failure + expect(service_result.errors.symbols_for(:labels)).to contain_exactly(:blank) + expect(work_package.reload.labels).to be_empty + end + end + end + context "with an automatically generated subject containing labels", with_flag: { work_package_labels: true } do shared_let(:labels_type) do create(:type, name: "Labeled autosubject",