diff --git a/app/models/project.rb b/app/models/project.rb index 1b6e9643440..3e4aa2823c2 100644 --- a/app/models/project.rb +++ b/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) diff --git a/app/services/projects/set_attributes_service.rb b/app/services/projects/set_attributes_service.rb index fa1fe8991ef..de6b3e28628 100644 --- a/app/services/projects/set_attributes_service.rb +++ b/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 diff --git a/app/views/admin/settings/projects_settings/show.html.erb b/app/views/admin/settings/projects_settings/show.html.erb index 0ce08f389e6..8afc25bf339 100644 --- a/app/views/admin/settings/projects_settings/show.html.erb +++ b/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]}) %> -
<%= setting_check_box :sequential_project_identifiers %>
<%= setting_select :new_project_user_role_id, Role.givable.collect {|r| [r.name, r.id.to_s]}, blank: "--- #{t(:actionview_instancetag_blank_option)} ---", diff --git a/config/locales/en.yml b/config/locales/en.yml index 3c6f58527d2..af8066dba7b 100644 --- a/config/locales/en.yml +++ b/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" diff --git a/config/settings.yml b/config/settings.yml index dec51f69282..3a46d7cd541 100644 --- a/config/settings.yml +++ b/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: diff --git a/db/migrate/20210510193438_remove_project_setting.rb b/db/migrate/20210510193438_remove_project_setting.rb new file mode 100644 index 00000000000..dcc0a73cf7e --- /dev/null +++ b/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 diff --git a/docs/system-admin-guide/system-settings/project-system-settings/README.md b/docs/system-admin-guide/system-settings/project-system-settings/README.md index 5da37145659..1d2c6da12f4 100644 --- a/docs/system-admin-guide/system-settings/project-system-settings/README.md +++ b/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. diff --git a/docs/system-admin-guide/system-settings/project-system-settings/image-20210309144536716.png b/docs/system-admin-guide/system-settings/project-system-settings/image-20210309144536716.png deleted file mode 100644 index dae27094d63..00000000000 Binary files a/docs/system-admin-guide/system-settings/project-system-settings/image-20210309144536716.png and /dev/null differ diff --git a/docs/system-admin-guide/system-settings/project-system-settings/image-20210510144536716.png b/docs/system-admin-guide/system-settings/project-system-settings/image-20210510144536716.png new file mode 100644 index 00000000000..90abba52712 Binary files /dev/null and b/docs/system-admin-guide/system-settings/project-system-settings/image-20210510144536716.png differ diff --git a/lib/open_project/acts_as_url/adapter/op_active_record.rb b/lib/open_project/acts_as_url/adapter/op_active_record.rb index d2340305109..812dccbb438 100644 --- a/lib/open_project/acts_as_url/adapter/op_active_record.rb +++ b/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 diff --git a/spec/contracts/projects/create_contract_spec.rb b/spec/contracts/projects/create_contract_spec.rb index a2bf0b67b7c..82a357f2040 100644 --- a/spec/contracts/projects/create_contract_spec.rb +++ b/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 diff --git a/spec/contracts/projects/shared_contract_examples.rb b/spec/contracts/projects/shared_contract_examples.rb index b7328e88326..e457b002055 100644 --- a/spec/contracts/projects/shared_contract_examples.rb +++ b/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 } diff --git a/spec/contracts/projects/update_contract_spec.rb b/spec/contracts/projects/update_contract_spec.rb index 97ac9013f55..7168c2cc010 100644 --- a/spec/contracts/projects/update_contract_spec.rb +++ b/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 diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 95a09130cf3..a968bf6ffb7 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/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 diff --git a/spec/features/projects/projects_spec.rb b/spec/features/projects/projects_spec.rb index 49b55aea849..d5b4fa9150e 100644 --- a/spec/features/projects/projects_spec.rb +++ b/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 diff --git a/spec/services/projects/set_attributes_service_integration_spec.rb b/spec/services/projects/set_attributes_service_integration_spec.rb new file mode 100644 index 00000000000..e3031b1a098 --- /dev/null +++ b/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 diff --git a/spec/services/projects/set_attributes_service_spec.rb b/spec/services/projects/set_attributes_service_spec.rb index 7ae48b932a2..7899adee193 100644 --- a/spec/services/projects/set_attributes_service_spec.rb +++ b/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 diff --git a/spec_legacy/unit/project_spec.rb b/spec_legacy/unit/project_spec.rb index fc68361482c..bf2cd18be0b 100644 --- a/spec_legacy/unit/project_spec.rb +++ b/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