diff --git a/app/services/projects/concerns/new_project_service.rb b/app/services/projects/concerns/new_project_service.rb index bad9e78e9b61..009e1d7086b4 100644 --- a/app/services/projects/concerns/new_project_service.rb +++ b/app/services/projects/concerns/new_project_service.rb @@ -42,7 +42,6 @@ def after_persist(attributes_call) new_project = attributes_call.result set_default_role(new_project) unless user.admin? - disable_custom_fields_with_empty_values(new_project) notify_project_created(new_project) if new_project.persisted? super @@ -84,32 +83,12 @@ def send_project_creation_email(new_project) ProjectMailer.project_created(new_project, user:).deliver_later end - def disable_custom_fields_with_empty_values(new_project) - # Ideally, `build_missing_project_custom_field_project_mappings` would not activate custom fields - # with empty values, but: - # This hook is required as acts_as_customizable build custom values with their default value - # even if a blank value was provided in the project creation form. - # `build_missing_project_custom_field_project_mappings` will then activate the custom field, - # although the user explicitly provided a blank value. In order to not patch `acts_as_customizable` - # further, we simply identify these custom values and deactivate the custom field. - - custom_field_ids = new_project.custom_values.select { |cv| cv.value.blank? && !cv.is_for_all? }.pluck(:custom_field_id) - custom_field_project_mappings = new_project.project_custom_field_project_mappings - - custom_field_project_mappings - .where(custom_field_id: custom_field_ids) - .or(custom_field_project_mappings - .where.not(custom_field_id: new_project.available_custom_fields.select(:id))) - .destroy_all - end - def build_missing_project_custom_field_project_mappings(project) # Activate all custom fields (via mapping table) that have no mapping, but are either # intended for all projects, or have a value provided by the user. + custom_field_ids = Set.new(activatable_custom_field_ids_from_project(project)) + custom_field_ids.merge(activatable_custom_field_ids_from_params) - custom_field_ids = project.custom_values - .select { |cv| cv.value? || cv.is_for_all? } - .pluck(:custom_field_id).uniq activated_custom_field_ids = project.project_custom_field_project_mappings.pluck(:custom_field_id).uniq mappings = (custom_field_ids - activated_custom_field_ids).uniq @@ -118,6 +97,30 @@ def build_missing_project_custom_field_project_mappings(project) project.project_custom_field_project_mappings.build(mappings) end + def activatable_custom_field_ids_from_project(project) + # We will activate fields with existing values, unless it's the default value, which might + # be generated automatically + project.custom_values + .select { |cv| cv.is_for_all? || (cv.value? && !cv.default?) } + .pluck(:custom_field_id) + end + + def activatable_custom_field_ids_from_params + # We will activate fields that are explicitly set via params + + # Extract custom field IDs from custom_field_values + via_cf_values = params.fetch(:custom_field_values, {}) + .keys + .map { it.to_s.to_i } + + # Extract custom field IDs from params keys that match 'custom_field_' + via_cf = params.keys + .grep(/\Acustom_field_\d+\z/) + .map { |k| k.to_s[/custom_field_(\d+)/, 1].to_i } + + via_cf_values + via_cf + end + def update_calculated_value_custom_fields(model) changed_cf_ids = model.custom_values.map(&:custom_field_id) @@ -125,7 +128,7 @@ def update_calculated_value_custom_fields(model) # edits a custom field which is used by an admin only calculated value # field. Without this unscoping, admin only value and all fields # referencing it (recursively) will not be recalculated and there will - # even be no place for that recalculatin to be triggered unless an admin + # even be no place for that recalculation to be triggered unless an admin # edits same value again. # # This may need to be handled differently to make it work for other custom diff --git a/spec/features/projects/create_spec.rb b/spec/features/projects/create_spec.rb index 1ef2eacd56d3..503055021dec 100644 --- a/spec/features/projects/create_spec.rb +++ b/spec/features/projects/create_spec.rb @@ -267,6 +267,14 @@ project_custom_field_section:) end + shared_let(:required_inactive_custom_field_with_default_value) do + create(:text_project_custom_field, + name: "Required inactive with default value", + is_required: true, + default_value: "foo", + project_custom_field_section:) + end + it "renders activated required custom fields for new" do visit new_project_path @@ -290,6 +298,7 @@ # Inactive fields, even if required, should not be shown expect(page).to have_no_field "Required Inactive *" + expect(page).to have_no_field "Required Inactive with default value *" end end diff --git a/spec/features/projects/creation_wizard/project_creation_wizard_spec.rb b/spec/features/projects/creation_wizard/project_creation_wizard_spec.rb index 63b076e1f06e..9830579f711a 100644 --- a/spec/features/projects/creation_wizard/project_creation_wizard_spec.rb +++ b/spec/features/projects/creation_wizard/project_creation_wizard_spec.rb @@ -29,6 +29,10 @@ #++ require "spec_helper" +# This is a feature spec for the project creation wizard, but only when creating +# a new project from a template with the wizard enabled (Project Initiation Request, PIR). +# The wizard that is shown when creating a new blank project from scratch is NOT tested here. +# See `spec/features/projects/create_spec.rb` for that. RSpec.describe "Project creation wizard", :js, :with_cuprite do diff --git a/spec/requests/api/v3/projects/create_resource_spec.rb b/spec/requests/api/v3/projects/create_resource_spec.rb index e588cec7dbad..0b8d30085923 100644 --- a/spec/requests/api/v3/projects/create_resource_spec.rb +++ b/spec/requests/api/v3/projects/create_resource_spec.rb @@ -119,38 +119,38 @@ end describe "custom fields" do - context "with an optional custom field" do - shared_let(:optional_custom_field) do - create(:text_project_custom_field, - name: "Department", - is_for_all: true) + shared_examples "creates a project with a custom value" do |custom_value| + it "responds with 201" do + expect(last_response).to have_http_status(:created) end - shared_examples "creates a project with an empty custom value" do - it "responds with 201" do - expect(last_response).to have_http_status(:created) - end + it "returns the newly created project" do + expect(last_response.body) + .to be_json_eql("Project".to_json) + .at_path("_type") - it "returns the newly created project" do - expect(last_response.body) - .to be_json_eql("Project".to_json) - .at_path("_type") + expect(last_response.body) + .to be_json_eql("Project name".to_json) + .at_path("name") + end - expect(last_response.body) - .to be_json_eql("Project name".to_json) - .at_path("name") - end + it "creates a project with an empty custom field value" do + project = Project.last + expect(project.typed_custom_value_for(shared_custom_field)) + .to eq(custom_value) + end - it "creates a project with an empty custom field value" do - project = Project.last - expect(project.typed_custom_value_for(optional_custom_field)) - .to eq("") - end + it "automatically activates the cf for project" do + expect(Project.last.project_custom_fields) + .to contain_exactly(shared_custom_field) + end + end - it "automatically activates the cf for project" do - expect(Project.last.project_custom_fields) - .to contain_exactly(optional_custom_field) - end + context "with an optional custom field" do + shared_let(:shared_custom_field) do + create(:text_project_custom_field, + name: "Department", + is_for_all: true) end context "when no custom field value is provided" do @@ -161,7 +161,7 @@ }.to_json end - it_behaves_like "creates a project with an empty custom value" + it_behaves_like "creates a project with a custom value", "" end context "when the custom field is provided but empty" do @@ -169,13 +169,13 @@ { identifier: "new_project_identifier", name: "Project name", - optional_custom_field.attribute_name(:camel_case) => { + shared_custom_field.attribute_name(:camel_case) => { raw: "" } }.to_json end - it_behaves_like "creates a project with an empty custom value" + it_behaves_like "creates a project with a custom value", "" end context "when the custom field value is provided and valid" do @@ -183,41 +183,69 @@ { identifier: "new_project_identifier", name: "Project name", - optional_custom_field.attribute_name(:camel_case) => { + shared_custom_field.attribute_name(:camel_case) => { raw: "Engineering" } }.to_json end - it "responds with 201" do - expect(last_response).to have_http_status(:created) - end + it_behaves_like "creates a project with a custom value", "Engineering" + end + end - it "returns the newly created project" do - expect(last_response.body) - .to be_json_eql("Project".to_json) - .at_path("_type") + context "when a custom field has a default value" do + shared_let(:shared_custom_field) do + create(:text_project_custom_field, + name: "Location", + is_for_all: false, + default_value: "Default Location") + end - expect(last_response.body) - .to be_json_eql("Project name".to_json) - .at_path("name") + context "when the custom field value is provided and valid" do + let(:body) do + { + identifier: "new_project_identifier", + name: "Project name", + shared_custom_field.attribute_name(:camel_case) => { + raw: "Custom Location" + } + }.to_json end - it "creates a project with the custom field value" do - project = Project.last - expect(project.typed_custom_value_for(optional_custom_field)) - .to eq("Engineering") + it_behaves_like "creates a project with a custom value", "Custom Location" + end + + context "when the custom field value is identical to the default" do + let(:body) do + { + identifier: "new_project_identifier", + name: "Project name", + shared_custom_field.attribute_name(:camel_case) => { + raw: "Default Location" + } + }.to_json end - it "automatically activates the cf for project if the value was provided" do - expect(Project.last.project_custom_fields) - .to contain_exactly(optional_custom_field) + it_behaves_like "creates a project with a custom value", "Default Location" + end + + context "when the custom field value is blank" do + let(:body) do + { + identifier: "new_project_identifier", + name: "Project name", + shared_custom_field.attribute_name(:camel_case) => { + raw: "" + } + }.to_json end + + it_behaves_like "creates a project with a custom value", "" end end context "with a required for_all custom field" do - shared_let(:required_custom_field) do + shared_let(:shared_custom_field) do create(:text_project_custom_field, name: "Department", is_required: true, @@ -246,7 +274,7 @@ { identifier: "new_project_identifier", name: "Project name", - required_custom_field.attribute_name(:camel_case) => { + shared_custom_field.attribute_name(:camel_case) => { raw: "" } }.to_json @@ -266,36 +294,13 @@ { identifier: "new_project_identifier", name: "Project name", - required_custom_field.attribute_name(:camel_case) => { + shared_custom_field.attribute_name(:camel_case) => { raw: "Engineering" } }.to_json end - it "responds with 201" do - expect(last_response).to have_http_status(:created) - end - - it "returns the newly created project" do - expect(last_response.body) - .to be_json_eql("Project".to_json) - .at_path("_type") - - expect(last_response.body) - .to be_json_eql("Project name".to_json) - .at_path("name") - end - - it "creates a project with the custom field value" do - project = Project.last - expect(project.typed_custom_value_for(required_custom_field)) - .to eq("Engineering") - end - - it "automatically activates the cf for project if the value was provided" do - expect(Project.last.project_custom_fields) - .to contain_exactly(required_custom_field) - end + it_behaves_like "creates a project with a custom value", "Engineering" end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index f4527f710150..241f06a5574c 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -134,34 +134,37 @@ project_custom_field_section: section) end - context "if the default value is not explicitly set to blank" do - let(:project_attributes) do - { custom_field_values: { - text_custom_field.id => "foo", - bool_custom_field.id => true - } } - end - - it "activates custom fields with default values" do - subject - expect(project.project_custom_field_project_mappings.pluck(:custom_field_id)) - .to contain_exactly(text_custom_field.id, bool_custom_field.id, text_custom_field_with_default.id) - end - end - - context "if the default value is explicitly set to blank" do - let(:project_attributes) do - { custom_field_values: { + def project_attributes_with_default_value(value_for_default_field) + { + custom_field_values: { text_custom_field.id => "foo", bool_custom_field.id => true, - text_custom_field_with_default.id => "" - } } - end + text_custom_field_with_default.id => value_for_default_field + }.compact + } + end - it "does not activate custom fields with default values" do - subject - expect(project.project_custom_field_project_mappings.pluck(:custom_field_id)) - .to contain_exactly(text_custom_field.id, bool_custom_field.id) + describe "activation of custom fields with default values" do + [ + { description: "implicitly set to its default", value: nil, should_activate: false }, + { description: "explicitly set to its default", value: "default", should_activate: true }, + { description: "explicitly set to blank", value: "", should_activate: true }, + { description: "explicitly set to a user value", value: "a user set value", should_activate: true } + ].each do |test_case| + context "if the default value is #{test_case[:description]}" do + let(:project_attributes) { project_attributes_with_default_value(test_case[:value]) } + + it "#{test_case[:should_activate] ? 'does' : 'does not'} activate custom fields with default values" do + subject + expected_ids = if test_case[:should_activate] + [text_custom_field.id, bool_custom_field.id, text_custom_field_with_default.id] + else + [text_custom_field.id, bool_custom_field.id] + end + expect(project.project_custom_field_project_mappings.pluck(:custom_field_id)) + .to match_array(expected_ids) + end + end end end end