diff --git a/app/services/project_identifiers/revert_project_to_classic_service.rb b/app/services/project_identifiers/revert_project_to_classic_service.rb index 907e280b25a..26e21d3e297 100644 --- a/app/services/project_identifiers/revert_project_to_classic_service.rb +++ b/app/services/project_identifiers/revert_project_to_classic_service.rb @@ -51,24 +51,26 @@ module ProjectIdentifiers attr_reader :project def restore_classic_identifier - classic = identifier_generator.restore_identifier(project) || - identifier_generator.suggest_identifier(project.name) + classic_id = identifier_generator.restore_identifier(project) || + identifier_generator.suggest_identifier(project.name) # Suppress notifications: this is a background system operation, not a user edit. Journal::NotificationConfiguration.with(false) do - project.update!(identifier: classic) + project.update!(identifier: classic_id) rescue ActiveRecord::RecordInvalid => e - handle_identifier_conflict(classic, e) + handle_update_failure(classic_id, e) end end - def handle_identifier_conflict(classic, error) - Rails.logger.warn "#{self.class}: identifier '#{classic}' taken for project #{project.id}, " \ - "falling back. (#{error.message})" - project.update!(identifier: identifier_generator.suggest_identifier(project.name)) + def handle_update_failure(classic_id, error) + Rails.logger.warn "#{self.class}: Could not set identifier '#{classic_id}' for project #{project.id}; " \ + "falling back to a randomized suffix. (#{error.message})" + suffix = SecureRandom.alphanumeric(5).downcase + base = classic_id.first(Projects::Identifier::CLASSIC_IDENTIFIER_MAX_LENGTH - 6) + project.update!(identifier: "#{base}-#{suffix}") end def identifier_generator - ProjectIdentifiers::ClassicIdentifierSuggestionGenerator.new + @identifier_generator ||= ProjectIdentifiers::ClassicIdentifierSuggestionGenerator.new end end end diff --git a/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb b/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb index a67e01a5c9b..3b5f11fa59e 100644 --- a/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb +++ b/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb @@ -116,7 +116,13 @@ RSpec.describe ProjectIdentifiers::RevertProjectToClassicService do let!(:project) do create(:project).tap do |p| p.update_columns(identifier: "MYAPP", wp_sequence_counter: 0) - FriendlyId::Slug.create!(sluggable: p, slug: "my-app") + # Remove p's own initial slug so "my-app" is the only entry in its slug history. + # Without this, the factory slug (newer created_at) would be returned first by + # restore_identifier, the update would succeed, and the conflict path would never fire. + FriendlyId::Slug.where(sluggable_id: p.id, sluggable_type: "Project").delete_all + # blocking_project already owns the "my-app" FriendlyId slug; reassign it so that + # restore_identifier returns "my-app" and project.update! conflicts with blocking_project. + FriendlyId::Slug.where(slug: "my-app", sluggable_type: "Project").update_all(sluggable_id: p.id) end end @@ -124,11 +130,9 @@ RSpec.describe ProjectIdentifiers::RevertProjectToClassicService do expect { described_class.new(project).call }.not_to raise_error end - it "assigns a valid classic identifier that is not the conflicting one" do + it "assigns the conflicting identifier with a 5-character random suffix" do described_class.new(project).call - reloaded = project.reload - expect(reloaded.identifier).to match(Projects::Identifier::CLASSIC_FORMAT) - expect(reloaded.identifier).not_to eq("my-app") + expect(project.reload.identifier).to match(/\Amy-app-[a-z0-9]{5}\z/) end it "logs a warning containing the project id and the conflicting identifier" do