Avoid enabling new CFs by default

app/controllers/custom_fields_controller.rb

@@ -31,7 +31,6 @@ class CustomFieldsController < ApplicationController
   layout 'admin'
 
   before_action :require_admin
-  before_action :find_types, except: [:index, :destroy]
   before_action :find_custom_field, only: [:edit, :update, :destroy, :move, :delete_option]
   before_action :blank_translation_attributes_as_nil, only: [:create, :update]
 
@@ -69,12 +68,6 @@ class CustomFieldsController < ApplicationController
     end
 
     if ok
-      if @custom_field.is_a? WorkPackageCustomField
-        @custom_field.types.each do |type|
-          TypesHelper.update_type_attribute_visibility! type
-        end
-      end
-
       flash[:notice] = t(:notice_successful_update)
       call_hook(:controller_custom_fields_edit_after_save, custom_field: @custom_field)
       redirect_to edit_custom_field_path(id: @custom_field.id)
@@ -187,10 +180,6 @@ class CustomFieldsController < ApplicationController
     render_404
   end
 
-  def find_types
-    @types = ::Type.order('position')
-  end
-
   protected
 
   def default_breadcrumb

app/helpers/types_helper.rb

@@ -47,136 +47,12 @@ module ::TypesHelper
                 style: "background-color: #{color}")
   end
 
-  module_function
-
-  def form_configuration_groups(type)
-    attributes = type.work_package_attributes
-    # First we create a complete list of all attributes.
-    # Later we will remove those that are members of an attribute group.
-    # This way attributes that were created after the las group definitions
-    # will fall back into the inactives group.
-    inactive_attributes = attributes.clone
-
-    actives = type.attribute_groups.map do |group|
-      extended_attributes = group.second.select do |key|
-        # The group's attribute keys could be out of date. Check presence.
-        if inactive_attributes.key?(key)
-          inactive_attributes.delete(key)
-          true
-        else
-          false
-        end
-      end
-      extended_attributes.map! do |key|
-        {
-          key: key,
-          always_visible: attr_visibility(key, type) == 'visible',
-          translation: translated_attribute_name(key, attributes[key])
-        }
-      end
-
-      [group[0], extended_attributes]
-    end
-    inactives = inactive_attributes.map do |key, attribute|
-      {
-        key: key,
-        attribute: attribute,
-        translation: translated_attribute_name(key, attribute)
-      }
-    end.sort_by { |_key, _attribute, translation| translation }
-
-    { actives: actives, inactives: inactives }
-  end
-
-  def attr_i18n_key(name)
-    if name == 'percentage_done'
-      'done_ratio'
-    else
-      name
-    end
-  end
-
-  def attr_translate(name)
-    if name == 'date'
-      I18n.t('label_date')
-    else
-      key = attr_i18n_key(name)
-      I18n.t("activerecord.attributes.work_package.#{key}", default: '')
-        .presence || I18n.t("attributes.#{key}")
-    end
-  end
-
-  def translated_attribute_name(name, attr)
-    if attr[:name_source]
-      attr[:name_source].call
-    else
-      attr[:display_name] || attr_translate(name)
-    end
-  end
-
-  ##
-  # Calculates the visibility for all attributes of the given type.
-  #
-  # @param type [Type] Type for which to get the attribute visibilities.
-  # @return [Hash{String => String}] A map from each attribute name to the attribute's visibility.
-  def type_attribute_visibility(type)
-    enabled_cfs = type.custom_field_ids.join("|")
-    visibility = ::Type.all_work_package_form_attributes
-      .keys
-      .select { |name| name !~ /^custom_field/ || name =~ /^custom_field_(#{enabled_cfs})$/ }
-      .map { |name| [name, attr_visibility(name, type) || "default"] }
-      .to_h
-  end
-
   ##
   # Updates the given type's attribute visibility map.
   #
   # @param type [Type] The type to be updated
   # @return [Type] The updated type
   def update_type_attribute_visibility!(type)
-    type.update! attribute_visibility: type_attribute_visibility(type)
-  end
-
-  ##
-  # Checks visibility of a work package type's attribute.
-  #
-  # @param name [String] Name of the field of which to check the visibility.
-  # @param type [Type] Work package type whose field visibility is checked.
-  # @return [String] Either 'hidden', 'default' or 'visible'.
-  def attr_visibility(name, type)
-    if name =~ /^custom_field_/
-      custom_field_visibility name, type
-    elsif name == 'date' && !type.is_milestone
-      non_milestone_date_field_visibility type
-    else
-      type.attribute_visibility[name]
-    end
-  end
-
-  #
-  # Bases visibility of custom fields on `type.custom_field_ids`
-  # if no visibility is defined yet. After the first update
-  # attribute_visibility and custom_field_ids will be kept in sync
-  # by the type service.
-  def custom_field_visibility(name, type)
-    id = name.split('_').last.to_i
-    value = type.attribute_visibility[name]
-
-    if value.nil? || value == 'hidden'
-      if type.custom_field_ids.include?(id)
-        'default'
-      else
-        'hidden'
-      end
-    else
-      value
-    end
-  end
-
-  def non_milestone_date_field_visibility(type)
-    values = [type.attribute_visibility['start_date'],
-              type.attribute_visibility['due_date']]
-
-    BaseTypeService::Functions.max_visibility values
+    type.update! attribute_visibility: type.type_attribute_visibility
   end
 end

app/models/type/attribute_groups.rb

@@ -88,9 +88,8 @@ module Type::AttributeGroups
     # group, the admin saving such a form configuration would encounter an
     # unexpected/unexplicable validation error.
     valid_keys = work_package_attributes.keys
-    groups.map do |_,attributes|
+    groups.each do |_, attributes|
       attributes.select! { |attribute| valid_keys.include? attribute }
-      [_, attributes]
     end
     groups.select! { |_,attributes| attributes.any? }
 
@@ -101,7 +100,10 @@ module Type::AttributeGroups
   # Returns the default +attribute_groups+ put together by
   # the default group map.
   def default_attribute_groups
-    values = work_package_attributes.keys.group_by { |key| map_attribute_to_group key }
+    values = work_package_attributes
+             .keys
+             .reject { |key| key.start_with? 'custom_field_' }
+             .group_by { |key| default_group_map.fetch(key.to_sym, :details) }
 
     ordered = []
     default_groups.map do |groupkey, label_key|
@@ -113,13 +115,38 @@ module Type::AttributeGroups
   end
 
   ##
-  # Map an AR attribute name to a group symbol,
-  # using the +default_group_map+ as fallback.
-  def map_attribute_to_group(name)
-    if name =~ /custom/
-      :other
-    else
-      default_group_map.fetch(name.to_sym, :details)
+  # Collect active and inactive form configuration groups for editing.
+  def form_configuration_groups
+    available = work_package_attributes
+    # First we create a complete list of all attributes.
+    # Later we will remove those that are members of an attribute group.
+    # This way attributes that were created after the las group definitions
+    # will fall back into the inactives group.
+    inactive = available.clone
+
+    active_form = get_active_groups(available, inactive)
+    inactive_form = inactive
+                    .map { |key, attribute| attr_form_map(key, attribute) }
+                    .sort_by { |_key, _attribute, translation| translation }
+
+    {
+      actives: active_form,
+      inactives: inactive_form
+    }
+  end
+
+  ##
+  # Collect active attributes from the current form configuration.
+  # Using the available attributes from +work_package_attributes+,
+  # determines which attributes are not used
+  def get_active_groups(available, inactive)
+    attribute_groups.map do |group|
+      extended_attributes =
+        group.second
+             .select { |key| inactive.delete(key) }
+             .map! { |key| attr_form_map(key, available[key]) }
+
+      [group[0], extended_attributes]
     end
   end
 

app/models/type/attribute_visibility.rb

@@ -57,4 +57,64 @@ module Type::AttributeVisibility
       'visible'
     end
   end
+
+  ##
+  # Calculates the visibility for all attributes of the given type.
+  #
+  # @param type [Type] Type for which to get the attribute visibilities.
+  # @return [Hash{String => String}] A map from each attribute name to the attribute's visibility.
+  def type_attribute_visibility
+    enabled_cfs = custom_field_ids.join("|")
+    visibility = ::Type.all_work_package_form_attributes
+      .keys
+      .select { |name| name !~ /^custom_field/ || name =~ /^custom_field_(#{enabled_cfs})$/ }
+      .map { |name| [name, attr_visibility(name) || "default"] }
+      .to_h
+  end
+
+  ##
+  # Checks visibility of a work package type's attribute.
+  #
+  # @param name [String] Name of the field of which to check the visibility.
+  # @param type [Type] Work package type whose field visibility is checked.
+  # @return [String] Either 'hidden', 'default' or 'visible'.
+  def attr_visibility(name)
+    if name =~ /^custom_field_/
+      custom_field_visibility name
+    elsif name == 'date' && !is_milestone
+      non_milestone_date_field_visibility
+    else
+      attribute_visibility[name]
+    end
+  end
+
+  #
+  # Bases visibility of custom fields on `type.custom_field_ids`
+  # if no visibility is defined yet. After the first update
+  # attribute_visibility and custom_field_ids will be kept in sync
+  # by the type service.
+  def custom_field_visibility(name)
+    id = name.split('_').last.to_i
+    value = attribute_visibility[name]
+
+    if value.nil? || value == 'hidden'
+      if custom_field_ids.include?(id)
+        'default'
+      else
+        'hidden'
+      end
+    else
+      value
+    end
+  end
+
+  def non_milestone_date_field_visibility
+    values = [
+      attribute_visibility['start_date'],
+      attribute_visibility['due_date']
+    ]
+
+    BaseTypeService::Functions.max_visibility values
+  end
+
 end

app/models/type/attributes.rb

@@ -78,6 +78,7 @@ module Type::Attributes
         attributes["custom_field_#{field.id}"] = {
           required: field.is_required,
           has_default: field.default_value.present?,
+          is_cf: true,
           display_name: field.name
         }
       end
@@ -86,6 +87,40 @@ module Type::Attributes
     end
   end
 
+  def attr_form_map(key, represented)
+    {
+      key: key,
+      always_visible: attr_visibility(key) == 'visible',
+      translation: translated_attribute_name(key, represented)
+    }
+  end
+
+  def attr_i18n_key(name)
+    if name == 'percentage_done'
+      'done_ratio'
+    else
+      name
+    end
+  end
+
+  def attr_translate(name)
+    if name == 'date'
+      I18n.t('label_date')
+    else
+      key = attr_i18n_key(name)
+      I18n.t("activerecord.attributes.work_package.#{key}", default: '')
+          .presence || I18n.t("attributes.#{key}")
+    end
+  end
+
+  def translated_attribute_name(name, attr)
+    if attr[:name_source]
+      attr[:name_source].call
+    else
+      attr[:display_name] || attr_translate(name)
+    end
+  end
+
   ##
   # Get all applicale work package attributes
   def work_package_attributes(merge_date: true)

app/views/custom_fields/_form.html.erb

@@ -104,28 +104,6 @@ See doc/COPYRIGHT.rdoc for more details.
 <section class="form--section">
   <% case @custom_field.class.name
      when "WorkPackageCustomField" %>
-    <fieldset class="form--fieldset">
-      <legend class="form--fieldset-legend"><%=l(:label_type_plural)%></legend>
-      <div class="form--field">
-        <div class="form--field-container -wrap-around">
-          <% for type in @types %>
-            <%= content_tag :label, '',
-                  class: "form--label-with-check-box",
-                  for: "custom_field_type_ids_#{type.id}" do %>
-              <div class="form--check-box-container">
-                <%= check_box_tag "custom_field[type_ids][]", type.id, (@custom_field.types.include? type),
-                      id: "custom_field_type_ids_#{type.id}",
-                      class: 'form--check-box' %>
-              </div>
-              <%= (type.is_standard) ? l(:label_custom_field_default_type) : h(type) %>
-            <% end %>
-          <% end %>
-        </div>
-      </div>
-      <%= hidden_field_tag "custom_field[type_ids][]", '' %>
-    </fieldset>
-    &nbsp;
-
     <div class="form--field"><%= f.check_box :is_required %></div>
     <div class="form--field"><%= f.check_box :is_for_all %></div>
     <div class="form--field"><%= f.check_box :is_filter %></div>

app/views/types/form/_form_configuration.html.erb

@@ -27,6 +27,8 @@ See doc/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
+<% form_attributes = @type.form_configuration_groups %>
+
 <section class="form--section">
   <div class="grid-block wrap">
     <div class="grid-content small-12 large-6">
@@ -70,7 +72,7 @@ See doc/COPYRIGHT.rdoc for more details.
         </div>
 
         <div id="draggable-groups" dragula='"groups"'>
-          <% form_configuration_groups(@type)[:actives].each do |group, attributes| %>
+          <% form_attributes[:actives].each do |group, attributes| %>
             <div class="type-form-conf-group" data-original-key="<%= group %>" data-key="<%= group %>">
               <div class="group-head">
                 <span class="group-handle">&#xe0b5;</span>
@@ -113,7 +115,7 @@ See doc/COPYRIGHT.rdoc for more details.
             <span class="advice">(Drag attributes from here to activate them)</span>
           </div>
           <div class="attributes" dragula='"attributes"'>
-            <% form_configuration_groups(@type)[:inactives].each do |inactive_attribute| %>
+            <% form_attributes[:inactives].each do |inactive_attribute| %>
               <div class="type-form-conf-attribute" data-key="<%= inactive_attribute[:key] %>">
                 <span class="attribute-handle">&#xe0b5;</span>
                 <span class="attribute-name">