Merge pull request opf#17647 from opf/bug/60666-user-cant-save-lifecy…

…cle-modal-if-project-is-invalid

[#60666] User can't save lifecycle modal if project is invalid

app/contracts/project_life_cycle_steps/base_contract.rb

@@ -31,9 +31,7 @@ class BaseContract < ::ModelContract
     validate :select_custom_fields_permission
     validate :consecutive_steps_have_increasing_dates
 
-    def valid?(context = :saving_life_cycle_steps)
-      super
-    end
+    def valid?(context = :saving_life_cycle_steps) = super
 
     def select_custom_fields_permission
       return if user.allowed_in_project?(:edit_project_stages_and_gates, model)

app/contracts/projects/base_contract.rb

@@ -57,6 +57,8 @@ class BaseContract < ::ModelContract
 
     validate :validate_user_allowed_to_manage
 
+    def valid?(context = :saving_custom_fields) = super
+
     def assignable_parents
       Project
         .allowed_to(user, :add_subprojects)

app/models/project.rb

@@ -100,9 +100,29 @@ class Project < ApplicationRecord
 
   acts_as_favorable
 
-  acts_as_customizable # extended in Projects::CustomFields in order to support sections
+  acts_as_customizable validate_on: :saving_custom_fields
+  # extended in Projects::CustomFields in order to support sections
   # and project-level activation of custom fields
 
+  # Override the `validation_context` getter to include the `default_validation_context` when the
+  # context is `:saving_custom_fields`. This is required, because the `acts_as_url` plugin from
+  # `stringex` defines a callback on the `:create` context for initialising the `identifier` field.
+  # Providing a custom context while creating the project, will not execute the callbacks on the
+  # `:create` or `:update` contexts, meaning the identifier will not get initialised.
+  # In order to initialise the identifier, the `default_validation_context` (`:create`, or `:update`)
+  # should be included when validating via the `:saving_custom_fields`. This way every create
+  # or update callback will also be executed alongside the `:saving_custom_fields` callbacks.
+  # This problem does not affect the contextless callbacks, they are always executed.
+
+  def validation_context
+    case Array(@validation_context)
+    in [*, :saving_custom_fields, *] => context
+      context << default_validation_context
+    else
+      @validation_context
+    end
+  end
+
   acts_as_searchable columns: %W(#{table_name}.name #{table_name}.identifier #{table_name}.description),
                      date_column: "#{table_name}.created_at",
                      project_key: "id",

lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb

@@ -49,7 +49,9 @@ def acts_as_customizable(options = {})
              dependent: :delete_all,
              validate: false,
              autosave: true
-          validate :validate_custom_values
+
+          validation_options = options[:validate_on] ? { on: options[:validate_on] } : {}
+          validate :validate_custom_values, **validation_options
           send :include, Redmine::Acts::Customizable::InstanceMethods
 
           before_save :ensure_custom_values_complete

spec/features/projects/life_cycle/overview_page/dialog/update_spec.rb

@@ -154,5 +154,24 @@
         dialog.expect_closed
       end
     end
+
+    context "when there is an invalid custom field on the project (Regression#60666)" do
+      let(:custom_field) { create(:string_project_custom_field, is_required: true, is_for_all: true) }
+
+      before do
+        project.custom_field_values = { custom_field.id => nil }
+        project.save(validate: false)
+      end
+
+      it "allows saving and closing the dialog without the custom field validation to interfere" do
+        dialog = overview_page.open_edit_dialog_for_life_cycles
+
+        expect_angular_frontend_initialized
+
+        # Saving the dialog is successful
+        dialog.submit
+        dialog.expect_closed
+      end
+    end
   end
 end

spec/models/project_spec.rb

@@ -455,6 +455,38 @@
   it_behaves_like "acts_as_customizable included" do
     let(:model_instance) { project }
     let(:custom_field) { create(:string_project_custom_field) }
+
+    describe "valid?" do
+      let(:custom_field) { create(:string_project_custom_field, is_required: true) }
+
+      before do
+        model_instance.custom_field_values = { custom_field.id => "test" }
+        model_instance.save
+        model_instance.custom_field_values = { custom_field.id => nil }
+      end
+
+      context "without a validation context" do
+        it "does not validates the custom fields" do
+          expect(model_instance).to be_valid
+        end
+
+        it "does not includes the default validation context in the validation_context" do
+          model_instance.send(:validation_context=, :custom_context)
+          expect(model_instance.validation_context).to eq(:custom_context)
+        end
+      end
+
+      context "with the :saving_custom_fields validation context" do
+        it "validates the custom fields" do
+          expect(model_instance).not_to be_valid(:saving_custom_fields)
+        end
+
+        it "includes the default validation context too in the validation_context" do
+          model_instance.send(:validation_context=, :saving_custom_fields)
+          expect(model_instance.validation_context).to eq(%i(saving_custom_fields update))
+        end
+      end
+    end
   end
 
   describe "url identifier" do
@@ -484,5 +516,30 @@
         expect(project.identifier).not_to eq(word)
       end
     end
+
+    # The acts_as_url plugin defines validation callbacks on :create and it is not automatically
+    # called when calling a custom context. However we need the acts_as_url callback to set the
+    # identifier when the validations are called with the :saving_custom_fields context.
+    context "when validating with :saving_custom_fields context" do
+      it "is set from name" do
+        project = described_class.new(name: "foo")
+
+        project.validate(:saving_custom_fields)
+
+        expect(project.identifier).to eq("foo")
+      end
+
+      it "is not allowed to clash with projects routing" do
+        expect(reserved).not_to be_empty
+
+        reserved.each do |word|
+          project = described_class.new(name: word)
+
+          project.validate(:saving_custom_fields)
+
+          expect(project.identifier).not_to eq(word)
+        end
+      end
+    end
   end
 end

spec/models/projects/customizable_spec.rb

@@ -224,9 +224,9 @@
                           bool_custom_field.id => true
                         })
 
-        expect(project).not_to be_valid
+        expect(project).not_to be_valid(:saving_custom_fields)
 
-        expect { project.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        expect { project.save!(context: :saving_custom_fields) }.to raise_error(ActiveRecord::RecordInvalid)
       end
 
       it "validates only custom values of a section if section scope is provided while updating" do
@@ -236,7 +236,7 @@
                            required_text_custom_field.id => "bar"
                          })
 
-        expect(project).to be_valid
+        expect(project).to be_valid(:saving_custom_fields)
 
         # after a project is created, a new required custom field is added
         # which gets automatically activated for all projects
@@ -245,20 +245,20 @@
                project_custom_field_section: another_section)
 
         # thus, the project is invalid in total
-        expect(project.reload).not_to be_valid
-        expect { project.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        expect(project.reload).not_to be_valid(:saving_custom_fields)
+        expect { project.save!(context: :saving_custom_fields) }.to raise_error(ActiveRecord::RecordInvalid)
 
         # but we still want to allow updating other sections without invalid required custom field values
         # by limiting the validation scope to a section temporarily
         project._limit_custom_fields_validation_to_section_id = section.id
 
-        expect(project).to be_valid
+        expect(project).to be_valid(:saving_custom_fields)
 
-        expect { project.save! }.not_to raise_error
+        expect { project.save!(context: :saving_custom_fields) }.not_to raise_error
 
         # Removing the section scoped limitation should result a validation error again.
         project._limit_custom_fields_validation_to_section_id = nil
-        expect { project.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        expect { project.save!(context: :saving_custom_fields) }.to raise_error(ActiveRecord::RecordInvalid)
       end
     end
   end
@@ -312,7 +312,7 @@
           bool_custom_field.id => true
         }
 
-        project.save!
+        project.save!(context: :saving_custom_fields)
       end
 
       it_behaves_like "implicitly enabled and saved custom values"
@@ -426,7 +426,7 @@
       before do
         project.send(:"custom_field_#{text_custom_field.id}=", "foo")
         project.send(:"custom_field_#{bool_custom_field.id}=", true)
-        project.save!
+        project.save!(context: :saving_custom_fields)
       end
 
       it_behaves_like "implicitly enabled and saved custom values"