diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index cadd43670b4..15913e79a8c 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -48,6 +48,14 @@ module WorkPackage::SemanticIdentifier # specifically when needed. class UnsupportedLookup < ArgumentError; end + # Validation context that permits deliberate changes to identifier/sequence_number + # on a persisted work package: + # + # work_package.save(context: WorkPackage::SemanticIdentifier::IDENTIFIER_REWRITE_CONTEXT) + # + # See #semantic_identifier_fields_not_accidentally_changed. + IDENTIFIER_REWRITE_CONTEXT = :identifier_rewrite + included do has_many :semantic_aliases, class_name: "WorkPackageSemanticAlias", @@ -69,7 +77,7 @@ module WorkPackage::SemanticIdentifier after_create :allocate_and_register_semantic_id, if: -> { Setting::WorkPackageIdentifier.semantic? && !skip_semantic_id_allocation } - validate :semantic_identifier_fields_consistent + validate :validate_semantic_identifier_fields end class_methods do @@ -194,6 +202,11 @@ module WorkPackage::SemanticIdentifier private + def validate_semantic_identifier_fields + semantic_identifier_fields_consistent + semantic_identifier_fields_not_accidentally_changed + end + # Ensures identifier and sequence_number are always written together. # One field set without the other indicates a partial write and is never valid. def semantic_identifier_fields_consistent @@ -201,4 +214,30 @@ module WorkPackage::SemanticIdentifier errors.add(:identifier, :semantic_identifier_incomplete) end + + # Guards against accidental edits, e.g. from a Rails console: identifiers are + # allocated automatically and resolved through the alias table, so a + # hand-written change silently breaks links and identifier history. + # Deliberate changes must be saved with the IDENTIFIER_REWRITE_CONTEXT + # validation context. + def semantic_identifier_fields_not_accidentally_changed + return unless persisted? + return if Array(validation_context).include?(IDENTIFIER_REWRITE_CONTEXT) + return if cleared_for_project_move? + + %w[identifier sequence_number].each do |attribute| + next unless attribute_changed?(attribute) + + errors.add(attribute, :must_not_be_changed, context: IDENTIFIER_REWRITE_CONTEXT) + end + end + + # Clearing both fields while moving to another project is part of the move + # operation itself: the identifier belongs to the source project and must be + # cleared in the same UPDATE that changes project_id (unique index on + # project_id/sequence_number). A fresh identifier is allocated after the move. + # See WorkPackages::SetAttributesService#clear_semantic_identifier. + def cleared_for_project_move? + project_id_changed? && identifier.nil? && sequence_number.nil? + end end diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index ab704c79dab..52489f162a5 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -274,6 +274,10 @@ class WorkPackages::SetAttributesService < BaseServices::SetAttributes end end + # The identifier belongs to the source project; a fresh one is allocated + # after the move (WorkPackages::UpdateService#update_semantic_ids). The + # fields must be cleared in the same UPDATE that changes project_id because + # of the unique index on (project_id, sequence_number). def clear_semantic_identifier work_package.sequence_number = nil work_package.identifier = nil diff --git a/config/locales/en.yml b/config/locales/en.yml index 9f8e0446a38..c5e13ba615d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2515,6 +2515,9 @@ en: blank: "ID can't be blank." identifier: semantic_identifier_incomplete: "and sequence_number must both be set at the same time." + 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." + sequence_number: + must_not_be_changed: "must not be changed manually. It is allocated automatically together with the identifier. If this change is deliberate, save with `save(context: :%{context})`." assigned_to: format: "%{message}" done_ratio: diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index c07349d42c1..f3c4e65952e 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -693,6 +693,69 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end + describe "semantic_identifier_fields_not_accidentally_changed validation" do + it "forbids changing the identifier in the default validation context" do + work_package.identifier = "MYPROJ-99" + work_package.sequence_number = 99 + + expect(work_package).not_to be_valid + expect(work_package.errors[:identifier]).to include(a_string_matching(/must not be changed manually/)) + end + + it "points to the rewrite context in the error message" do + work_package.identifier = "MYPROJ-99" + work_package.sequence_number = 99 + + work_package.validate + expect(work_package.errors[:identifier]).to include(a_string_matching(/identifier_rewrite/)) + end + + it "forbids changing the sequence_number" do + work_package.sequence_number = 99 + + expect(work_package).not_to be_valid + expect(work_package.errors[:sequence_number]).to include(a_string_matching(/must not be changed manually/)) + end + + it "forbids clearing the identifier fields" do + work_package.identifier = nil + work_package.sequence_number = nil + + expect(work_package).not_to be_valid + end + + it "allows clearing the fields as part of a project move" do + work_package.project = create(:project) + work_package.identifier = nil + work_package.sequence_number = nil + + work_package.validate + expect(work_package.errors[:identifier]).to be_empty + expect(work_package.errors[:sequence_number]).to be_empty + end + + it "allows the change when saved with the rewrite context" do + work_package.identifier = "MYPROJ-99" + work_package.sequence_number = 99 + + expect(work_package.save(context: WorkPackage::SemanticIdentifier::IDENTIFIER_REWRITE_CONTEXT)).to be(true) + expect(work_package.reload.identifier).to eq("MYPROJ-99") + expect(work_package.reload.sequence_number).to eq(99) + end + + it "does not interfere with unrelated changes" do + work_package.subject = "Updated subject" + + expect(work_package).to be_valid + end + + it "does not apply to new records" do + wp = build(:work_package, project:, identifier: "MYPROJ-5", sequence_number: 5) + + expect(wp).to be_valid + end + end + describe "#allocate_and_register_semantic_id", with_settings: { work_packages_identifier: "semantic" } do let(:project) { create(:project, identifier: "PROJ", wp_sequence_counter: 0) } let(:target_project) { create(:project, identifier: "OTHER", wp_sequence_counter: 0) } diff --git a/spec/services/work_packages/semantic_ids/integration_spec.rb b/spec/services/work_packages/semantic_ids/integration_spec.rb index d6862cafc68..3bc9cfaccdc 100644 --- a/spec/services/work_packages/semantic_ids/integration_spec.rb +++ b/spec/services/work_packages/semantic_ids/integration_spec.rb @@ -279,6 +279,21 @@ RSpec.describe "SemanticIds registry integration", result = WorkPackages::UpdateService.new(user:, model: wp).call(subject: "Updated subject") expect(result).to be_success end + + it "UpdateService clears stale identifier fields on a project move" do + wp = WorkPackages::CreateService.new(user:).call(**attributes).result + # Simulate a WP that received a semantic id before the instance was + # switched (back) to classic mode. + wp.update_columns(identifier: "OLDPROJ-7", sequence_number: 7) + + result = WorkPackages::UpdateService.new(user:, model: wp).call(project: target_project) + + expect(result).to be_success + expect(wp.identifier).to be_nil + expect(wp.sequence_number).to be_nil + expect(wp.reload.identifier).to be_nil + expect(wp.sequence_number).to be_nil + end end context "in semantic mode", with_settings: { work_packages_identifier: "semantic" } do