Remove case-transforming normalizes and parse_friendly_id override

Per code review: the model should not auto-transform identifier casing
on every read/write. Format validators already enforce correct casing
(lowercase for numeric, uppercase for alphanumeric), and the background
job will handle migrating existing identifiers.

Restores LOWER() in identifier_not_historically_reserved since without
normalizes, slugs and identifiers may differ in case.
This commit is contained in:
Kabiru Mwenja
2026-03-23 17:09:02 +03:00
parent 14a84b2f3a
commit bfeee22232
2 changed files with 11 additions and 68 deletions
+4 -13
View File
@@ -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?
+7 -55
View File
@@ -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