[37140] Use acts_as_url to generate unique slug for identifier (#9250)

* Use acts_as_url to generate unique slug for identifier

* Re-enable project identifier spec now handled in backend

* Add integration spec for set attribute service

* Adapt spec for new empty identifier

* Avoid double error messages for empty identifier

acts_as_url may return an empty string depending on the name,

but the empty presence validation will suffice in this case, we don't need the invalid validation

* Fix stubbed errors object accessed by acts_as_url

* Avoid trying to get the upstream unique attribute in case of errors

* Remove sequential_project_identifiers setting

* Adapt spec to review feedback

app/models/project.rb

@@ -111,6 +111,15 @@ class Project < ApplicationRecord
   # it implicitly assumes a db:seed-created standard type to be present and currently
   # neither development nor deployment setups are prepared for this
   # validates_presence_of :types
+
+  acts_as_url :name,
+              url_attribute: :identifier,
+              sync_url: false, # Don't update identifier when name changes
+              only_when_blank: true, # Only generate when identifier not set
+              limit: IDENTIFIER_MAX_LENGTH,
+              blacklist: RESERVED_IDENTIFIERS,
+              adapter: OpenProject::ActsAsUrl::Adapter::OpActiveRecord # use a custom adapter able to handle edge cases
+
   validates :identifier,
             presence: true,
             uniqueness: { case_sensitive: true },
@@ -121,7 +130,7 @@ class Project < ApplicationRecord
   # starts with lower-case letter, a-z, 0-9, dashes and underscores afterwards
   validates :identifier,
             format: { with: /\A[a-z][a-z0-9\-_]*\z/ },
-            if: ->(p) { p.identifier_changed? }
+            if: ->(p) { p.identifier_changed? && p.identifier.present? }
   # reserved words
 
   friendly_id :identifier, use: :finders
@@ -352,12 +361,6 @@ class Project < ApplicationRecord
   end
 
   class << self
-    # Returns an auto-generated project identifier based on the last identifier used
-    def next_identifier
-      p = Project.newest.first
-      p.nil? ? nil : p.identifier.to_s.succ
-    end
-
     # builds up a project hierarchy helper structure for use with #project_tree_from_hierarchy
     #
     # it expects a simple list of projects with a #lft column (awesome_nested_set)

app/services/projects/set_attributes_service.rb

@@ -46,22 +46,11 @@ module Projects
     def set_default_attributes(attributes)
       attribute_keys = attributes.keys.map(&:to_s)
 
-      set_default_identifier(attribute_keys.include?('identifier'))
       set_default_public(attribute_keys.include?('public'))
       set_default_module_names(attribute_keys.include?('enabled_module_names'))
       set_default_types(attribute_keys.include?('types') || attribute_keys.include?('type_ids'))
     end
 
-    def set_default_identifier(provided)
-      return if provided
-
-      if Setting.sequential_project_identifiers?
-        model.identifier = Project.next_identifier
-      elsif model.name.present?
-        model.identifier = model.name.to_localized_slug(limit: Project::IDENTIFIER_MAX_LENGTH)
-      end
-    end
-
     def set_default_public(provided)
       model.public = Setting.default_projects_public? unless provided
     end

app/views/admin/settings/projects_settings/show.html.erb

@@ -37,7 +37,6 @@ See docs/COPYRIGHT.rdoc for more details.
         <%= setting_multiselect(:default_projects_modules,
           OpenProject::AccessControl.available_project_modules.collect {|m| [l_or_humanize(m, prefix: "project_module_"), m.to_s]}) %>
       </div>
-      <div class="form--field"><%= setting_check_box :sequential_project_identifiers %></div>
       <div class="form--field"><%= setting_select :new_project_user_role_id,
                         Role.givable.collect {|r| [r.name, r.id.to_s]},
                         blank: "--- #{t(:actionview_instancetag_blank_option)} ---",

config/locales/en.yml

@@ -2411,7 +2411,6 @@ en:
   setting_repository_truncate_at: "Maximum number of files displayed in the repository browser"
   setting_rest_api_enabled: "Enable REST web service"
   setting_self_registration: "Self-registration"
-  setting_sequential_project_identifiers: "Generate sequential project identifiers"
   setting_session_ttl: "Session expiry time after inactivity"
   setting_session_ttl_hint: "Value below 5 works like disabled"
   setting_session_ttl_enabled: "Session expires"

config/settings.yml

@@ -299,8 +299,6 @@ project_gantt_query:
 new_project_user_role_id:
   format: int
   default: ''
-sequential_project_identifiers:
-  default: 0
 # encodings used to convert repository files content to UTF-8
 # multiple values accepted, comma separated
 repositories_encodings:

db/migrate/20210510193438_remove_project_setting.rb

@@ -0,0 +1,9 @@
+class RemoveProjectSetting < ActiveRecord::Migration[6.1]
+  def up
+    Project.where(name: 'sequential_project_identifiers').delete_all
+  end
+
+  def down
+    # Nothing to do
+  end
+end

docs/system-admin-guide/system-settings/project-system-settings/README.md

@@ -14,10 +14,9 @@ To adapt the system project settings, navigate to *Administration -> System sett
 
 1. Check if **new projects are public by default**. This means that users without an account can access the project without login.
 2. Select **which modules should be activated for newly created projects by default**.
-3. Choose whether **sequential project identifiers should be created**. If this option is activated, a project identifier for the next project will be offered automatically, based on the existing project name. For example, if a project “Myproject1” was created, “Myproject2” will be offered as identifier for the next project.
-4. The **role given to a user in a new project when the user creates a new project but is not an (global) admin**. This makes sense when a user receives the permission to create a new project via [global role](../../users-permissions/roles-permissions/#global-roles).
+3. The **role given to a user in a new project when the user creates a new project but is not an (global) admin**. This makes sense when a user receives the permission to create a new project via [global role](../../users-permissions/roles-permissions/#global-roles).
 
-![default-settings-for-new-projects](image-20210309144536716.png)
+![default-settings-for-new-projects](image-20210510144536716.png)
 
 ## Settings for the Projects Overview List
 1. Choose **which columns should be visible** in the Projects Overview List by default.

lib/open_project/acts_as_url/adapter/op_active_record.rb

@@ -37,6 +37,21 @@ module OpenProject
   module ActsAsUrl
     module Adapter
       class OpActiveRecord < Stringex::ActsAsUrl::Adapter::ActiveRecord
+
+        ##
+        # Avoid generating the slug if the attribute is already set
+        # and only_when_blank is true
+        def ensure_unique_url!(instance)
+          attribute = instance.send(settings.url_attribute)
+          super if attribute.blank? || !settings.only_when_blank
+        end
+
+        ##
+        # Always return the stored url, even if it has errors
+        def url_attribute(instance)
+          read_attribute instance, settings.url_attribute
+        end
+
         private
 
         def modify_base_url

spec/contracts/projects/create_contract_spec.rb

@@ -49,5 +49,13 @@ describe Projects::CreateContract do
     end
 
     subject(:contract) { described_class.new(project, current_user) }
+
+    context 'if the identifier is nil' do
+      let(:project_identifier) { nil }
+
+      it 'is replaced for new project' do
+        expect_valid(true)
+      end
+    end
   end
 end

spec/contracts/projects/shared_contract_examples.rb

@@ -104,14 +104,6 @@ shared_examples_for 'project contract' do
     end
   end
 
-  context 'if the identifier is nil' do
-    let(:project_identifier) { nil }
-
-    it 'is invalid' do
-      expect_valid(false, identifier: %i(blank))
-    end
-  end
-
   context 'if the description is nil' do
     let(:project_description) { nil }
 

spec/contracts/projects/update_contract_spec.rb

@@ -45,5 +45,13 @@ describe Projects::UpdateContract do
     let(:permissions) { [:edit_project] }
 
     subject(:contract) { described_class.new(project, current_user) }
+
+    context 'if the identifier is nil' do
+      let(:project_identifier) { nil }
+
+      it 'is replaced for new project' do
+        expect_valid(false, identifier: %i(blank))
+      end
+    end
   end
 end

spec/controllers/projects_controller_spec.rb

@@ -165,12 +165,15 @@ describe ProjectsController, type: :controller do
       end
 
       context 'on failure' do
+        let(:errors) { ActiveModel::Errors.new(project) }
         let(:error_message) { 'error message' }
 
         before do
           expect(update_service).to receive(:call).with([1, 2, 3]).and_return false
 
-          allow(project).to receive_message_chain(:errors, :full_messages).and_return(error_message)
+          # acts_as_url tries to access the errors object which we stub here
+          allow(project).to receive(:errors).and_return errors
+          allow(errors).to receive(:full_messages).and_return(error_message)
 
           patch :types, params: { id: project.id, project: { 'type_ids' => ['1', '2', '3'] } }
         end

spec/features/projects/projects_spec.rb

@@ -74,15 +74,15 @@ describe 'Projects', type: :feature, js: true do
     end
 
     it 'does not create a project with an already existing identifier' do
-      skip "TODO identifier is not yet rendered on error in dynamic form"
-
       click_on 'New project'
 
       name_field.set_value 'Foo project'
       click_on 'Save'
 
-      expect(page).to have_content 'Identifier has already been taken'
-      expect(page).to have_current_path /\/projects\/new\/?/
+      expect(page).to have_current_path /\/projects\/foo-project-1\/?/
+
+      project = Project.last
+      expect(project.identifier).to eq 'foo-project-1'
     end
 
     context 'with a multi-select custom field' do

spec/services/projects/set_attributes_service_integration_spec.rb

@@ -0,0 +1,66 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 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, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe Projects::SetAttributesService, 'integration', type: :model do
+  let(:user) do
+    FactoryBot.create(:user, global_permissions: %w[add_project])
+  end
+  let(:contract) { Projects::CreateContract }
+  let(:instance) { described_class.new(user: user, model: project, contract_class: contract) }
+  let(:attributes) { {} }
+  let(:service_result) do
+    instance.call(attributes)
+  end
+
+  describe 'with an existing project' do
+    let!(:existing) { FactoryBot.create :project, identifier: 'my-new-project' }
+
+    context 'and a new project with no identifier set' do
+      let(:project) { Project.new name: 'My new project' }
+
+      it 'will auto-correct the identifier' do
+        expect(service_result).to be_success
+        expect(service_result.result.identifier).to eq 'my-new-project-1'
+      end
+    end
+
+    context 'and a new project with the same identifier set' do
+      let(:project) { Project.new name: 'My new project', identifier: 'my-new-project' }
+
+      it 'will result in an error' do
+        expect(service_result).not_to be_success
+        expect(service_result.result.identifier).to eq 'my-new-project'
+
+        errors = service_result.errors.full_messages
+        expect(errors).to eq ['Identifier has already been taken.']
+      end
+    end
+  end
+end

spec/services/projects/set_attributes_service_spec.rb

@@ -102,55 +102,27 @@ describe Projects::SetAttributesService, type: :model do
       end
 
       context 'identifier default value' do
-        context 'with a default identifier configured', with_settings: { sequential_project_identifiers: true } do
-          context 'with an identifier provided' do
-            let(:call_attributes) do
-              {
-                identifier: 'lorem'
-              }
-            end
-
-            it 'does not alter the identifier' do
-              expect(subject.result.identifier)
-                .to eql 'lorem'
-            end
+        context 'with an identifier provided' do
+          let(:call_attributes) do
+            {
+              identifier: 'lorem'
+            }
           end
 
-          context 'with no identifier provided' do
-            it 'sets a default identifier' do
-              allow(Project)
-                .to receive(:next_identifier)
-                .and_return('ipsum')
-
-              expect(subject.result.identifier)
-                .to eql 'ipsum'
-            end
+          it 'does not alter the identifier' do
+            expect(subject.result.identifier)
+              .to eql 'lorem'
           end
         end
 
-        context 'without a default identifier configured', with_settings: { sequential_project_identifiers: false } do
-          context 'with an identifier provided' do
-            let(:call_attributes) do
-              {
-                identifier: 'lorem'
-              }
-            end
+        context 'with no identifier provided' do
+          it 'stays nil' do
+            allow(Project)
+              .to receive(:next_identifier)
+              .and_return('ipsum')
 
-            it 'does not alter the identifier' do
-              expect(subject.result.identifier)
-                .to eql 'lorem'
-            end
-          end
-
-          context 'with no identifier provided' do
-            it 'stays nil' do
-              allow(Project)
-                .to receive(:next_identifier)
-                .and_return('ipsum')
-
-              expect(subject.result.identifier)
-                .to be_nil
-            end
+            expect(subject.result.identifier)
+              .to be_nil
           end
         end
       end

spec_legacy/unit/project_spec.rb

@@ -272,17 +272,6 @@ describe Project, type: :model do
     assert !versions.map(&:id).include?(6)
   end
 
-  it 'should next identifier' do
-    ProjectCustomField.delete_all
-    Project.create!(name: 'last', identifier: 'p2008040')
-    assert_equal 'p2008041', Project.next_identifier
-  end
-
-  it 'should next identifier first project' do
-    Project.delete_all
-    assert_nil Project.next_identifier
-  end
-
   context 'with modules',
           with_settings: { default_projects_modules: ['work_package_tracking', 'repository'] } do
     it 'should enabled module names' do