diff --git a/app/models/projects/identifier.rb b/app/models/projects/identifier.rb index c97e5fafdc6..9e910446d23 100644 --- a/app/models/projects/identifier.rb +++ b/app/models/projects/identifier.rb @@ -39,10 +39,6 @@ module Projects::Identifier extend FriendlyId normalizes :identifier, with: OpenProject::RemoveAsciiControlCharacters - normalizes :identifier, with: ->(value) { - stripped = value.to_s.strip - Setting::WorkPackageIdentifier.alphanumeric? ? stripped.upcase : stripped.downcase - } acts_as_url :name, url_attribute: :identifier, @@ -93,12 +89,6 @@ module Projects::Identifier end class_methods do - # Normalize the input to FriendlyID finders so that lookups are case-insensitive. - # FriendlyID's default parse_friendly_id returns the value unchanged. - def parse_friendly_id(value) - normalize_value_for(:identifier, value) - end - def suggest_identifier(name) if Setting::WorkPackageIdentifier.alphanumeric? WorkPackages::IdentifierAutofix::ProjectIdentifierSuggestionGenerator.suggest_identifier(name) @@ -148,13 +138,14 @@ module Projects::Identifier # Checks friendly_id_slugs for any project that previously used this identifier and # has since changed it. It allows a project to switch back to an identifier it has - # used before. Plain equality works because `normalizes :identifier` ensures consistent - # casing before FriendlyId records the slug. + # used before. Uses LOWER() because slugs may be stored in a different case than the + # incoming identifier (e.g. old lowercase slug vs new uppercase alphanumeric identifier). def identifier_not_historically_reserved return if errors.any? { |error| error.attribute == :identifier && error.type == :taken } already_existing = FriendlyId::Slug - .where(slug: identifier, sluggable_type: self.class.to_s) + .where("LOWER(slug) = LOWER(?)", identifier) + .where(sluggable_type: self.class.to_s) .where.not(sluggable_id: id) .exists? diff --git a/spec/models/projects/identifier_spec.rb b/spec/models/projects/identifier_spec.rb index 5e30b9295ce..9f5a4a31252 100644 --- a/spec/models/projects/identifier_spec.rb +++ b/spec/models/projects/identifier_spec.rb @@ -37,56 +37,6 @@ RSpec.describe Projects::Identifier do it { is_expected.to normalize(:identifier).from("my\n\x00project\t").to("myproject") } end - describe ".normalize_value_for" do - context "when in numeric mode (default)" do - before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(false) } - - it "downcases the value" do - expect(Project.normalize_value_for(:identifier, "PROJ")).to eq("proj") - end - - it "strips whitespace" do - expect(Project.normalize_value_for(:identifier, " proj ")).to eq("proj") - end - - it "returns nil unchanged" do - expect(Project.normalize_value_for(:identifier, nil)).to be_nil - end - end - - context "when in alphanumeric mode" do - before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(true) } - - it "upcases the value" do - expect(Project.normalize_value_for(:identifier, "proj")).to eq("PROJ") - end - - it "strips whitespace" do - expect(Project.normalize_value_for(:identifier, " proj ")).to eq("PROJ") - end - end - end - - describe "normalizes :identifier on Project" do - context "when in numeric mode (default)" do - before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(false) } - - it "downcases identifier on assignment" do - project = Project.new(identifier: "MyProject") - expect(project.identifier).to eq("myproject") - end - end - - context "when in alphanumeric mode" do - before { allow(Setting::WorkPackageIdentifier).to receive(:alphanumeric?).and_return(true) } - - it "upcases identifier on assignment" do - project = Project.new(identifier: "myproject") - expect(project.identifier).to eq("MYPROJECT") - end - end - end - describe "url identifier", with_settings: { work_packages_identifier: "numeric" } do let(:reserved) do Rails.application.routes.routes @@ -126,9 +76,9 @@ RSpec.describe Projects::Identifier do it "is not allowed to clash with another project case-insensitively" do create(:project, identifier: "existing") - project = build(:project, identifier: "EXISTING") - expect(project).not_to be_valid - expect(project.errors[:identifier]).to include("has already been taken.") + expect do + Project.where(id: create(:project).id).update_all(identifier: "EXISTING") + end.to raise_error(ActiveRecord::RecordNotUnique) end it "is not allowed to clash with a former identifier of another project" do @@ -144,8 +94,10 @@ RSpec.describe Projects::Identifier do other_project = create(:project, identifier: "former-id") other_project.update!(identifier: "new-id") - project = build(:project, identifier: "FORMER-ID") - expect(project).not_to be_valid + # Bypass format validation to test the LOWER() slug check directly + project = create(:project) + project.identifier = "FORMER-ID" + project.valid? expect(project.errors[:identifier]).to include("has already been taken.") end