[#53704] Visible=false project attribute values are deleted when a non-admins edit the attributes

app/models/projects/acts_as_customizable_patches.rb

@@ -108,7 +108,7 @@ def with_all_available_custom_fields
       # in order to support implicit activation of custom fields when values are provided during an update
       self._query_available_custom_fields_on_global_level = true
       result = yield
-      self._query_available_custom_fields_on_global_level = false
+      self._query_available_custom_fields_on_global_level = nil
 
       result
     end
@@ -118,10 +118,21 @@ def available_custom_fields(global: false)
       # in contrast to acts_as_customizable, custom_fields are enabled per project
       # thus we need to check the project_custom_field_project_mappings
       custom_fields = ProjectCustomField
-        .visible
         .includes(:project_custom_field_section)
         .order("custom_field_sections.position", :position_in_custom_field_section)
 
+      # Do not hide the invisble fields when accessing via the _query_available_custom_fields_on_global_level
+      # flag. Due to the internal working of the acts_as_customizable plugin, when a project admin updates
+      # the custom fields, it will clear out all the hidden fields that are not visible for them.
+      # This happens because the `#ensure_custom_values_complete` will gather all the `custom_field_values`
+      # and assigns them to the custom_fields association. If the `custom_field_values` do not contain the
+      # hidden fields, they will be cleared from the association. The `custom_field_values` will contain the
+      # hidden fields, only if they are returned from this method. Hence we should not hide them,
+      # when accessed with the _query_available_custom_fields_on_global_level flag on.
+      unless _query_available_custom_fields_on_global_level
+        custom_fields = custom_fields.visible
+      end
+
       # available_custom_fields is called from within the acts_as_customizable module
       # we don't want to adjust these calls, but need a way to query the available custom fields on a global level in some cases
       # thus we pass in this parameter as an instance flag implicitly here,
@@ -155,8 +166,11 @@ def custom_field_values=(values)
       with_all_available_custom_fields { super }
     end
 
-    # we need to query the available custom fields on a global level when
-    # trying to set a custom field which is not enabled via e.g. custom_field_123="foo"
+    # We need to query the available custom fields on a global level when
+    # trying to set a custom field which is not enabled via the API e.g. custom_field_123="foo"
+    # This implies implicit activation of the disabled custom fields via the API. As a side effect,
+    # we will have empty CustomValue objects created for each custom field, regardless of its
+    # enabled/disabled state in the project.
     def for_custom_field_accessor(method_symbol)
       with_all_available_custom_fields { super }
     end

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

@@ -651,5 +651,57 @@
         end
       end
     end
+
+    describe "with hidden fields" do
+      let(:section) { section_for_input_fields }
+      let(:dialog) { Components::Projects::ProjectCustomFields::EditDialog.new(project, section) }
+      let(:custom_field) { string_project_custom_field }
+      let(:field) { FormFields::Primerized::InputField.new(custom_field) }
+
+      before do
+        all_fields.without(string_project_custom_field).each { |cf| cf.update(visible: false) }
+      end
+
+      it "does not clears them after a project admin updates" do
+        # TODO: To make the expectations correct, we create an empty custom value for the other
+        # project's custom field too. This would happen in the code anyway.
+        # Due to the design of the acts_as_customizable plugin and the patch, it will create
+        # empty custom values for all the existing custom fields, regardless if they are
+        # enabled in the project or not. This happens, because we want to maintain backward
+        # compatibility with the existing api, and allow the API to automatically enable
+        # custom fields without being activated in the project. This implies in defining
+        # all the custom field accessors on every project, and that leads to having the
+        # empty custom values created. This is a compromise to avoid further patching the
+        # aac plugin and increase complexity. We will also get rid of the patch and this
+        # behaviour in a latter ticket https://community.openproject.org/wp/53729 .
+        # An extra expectation is added to make sure we don't activate other custom fields,
+        # so there are no unwanted side effects.
+
+        create(:custom_value,
+               custom_field: boolean_project_custom_field_activated_in_other_project,
+               customized: project)
+
+        expected_custom_values =
+          project.custom_values.where.not(custom_field: string_project_custom_field)
+          .pluck(:customized_type, :customized_id, :custom_field_id, :value)
+
+        expected_custom_fields = project.project_custom_fields
+
+        overview_page.visit_page
+
+        overview_page.open_edit_dialog_for_section(section)
+
+        field.fill_in(with: "new value")
+        dialog.submit
+        dialog.expect_closed
+
+        custom_values =
+          project.custom_values.where.not(custom_field: string_project_custom_field)
+          .pluck(:customized_type, :customized_id, :custom_field_id, :value)
+
+        expect(custom_values).to eq(expected_custom_values)
+        expect(project.project_custom_fields.reload).to eq(expected_custom_fields)
+      end
+    end
   end
 end

spec/features/projects/project_custom_fields/overview_page/shared_context.rb

@@ -225,8 +225,11 @@
     ]
   end
 
+  let(:all_fields) { input_fields + select_fields + multi_select_fields }
+
   let!(:boolean_project_custom_field_activated_in_other_project) do
-    create(:boolean_project_custom_field, projects: [other_project], name: "Other Boolean field",
+    create(:boolean_project_custom_field, projects: [other_project],
+                                          name: "Other Boolean field",
                                           project_custom_field_section: section_for_input_fields)
   end
 end

spec/models/projects/customizable_spec.rb

@@ -462,12 +462,27 @@
       let(:user) { create(:user) }
 
       it "does not activate hidden custom fields" do
+        pending <<~REASON.squish
+          Due to backward compatibility for the API,
+          we have to activate the hidden_custom_field too,
+          but it won't receive any value, so its value is not changed.
+          The frontend will not send the hidden field anyway,
+          so this behaviour does not present a real problem.
+        REASON
         # project creation happens with an non-admin user as let(:project) called after setting the current user to an non-admin
         expect(project.project_custom_field_project_mappings.pluck(:custom_field_id))
           .to contain_exactly(text_custom_field.id, bool_custom_field.id)
 
         expect(project.custom_value_for(hidden_custom_field)).to be_nil
       end
+
+      it "does not set a value for the hidden custom fields" do
+        # project creation happens with an non-admin user as let(:project) called after setting the current user to an non-admin
+        expect(project.project_custom_field_project_mappings.pluck(:custom_field_id))
+          .to contain_exactly(text_custom_field.id, bool_custom_field.id, hidden_custom_field.id)
+
+        expect(project.custom_value_for(hidden_custom_field)).to be_nil
+      end
     end
   end
 end