Merge pull request #15337 from opf/feature/53771-add-%-complete-to-se…

…ction-estimates-and-time

Feature/53771 add % complete to section estimates and time

app/models/type/attribute_groups.rb

@@ -41,11 +41,11 @@ module Type::AttributeGroups
     # May be extended by plugins
     mattr_accessor :default_group_map do
       {
-        author: :people,
         assignee: :people,
         responsible: :people,
         estimated_time: :estimates_and_time,
         remaining_time: :estimates_and_time,
+        percentage_done: :estimates_and_time,
         spent_time: :estimates_and_time,
         priority: :details
       }
@@ -117,6 +117,7 @@ def attribute_groups=(groups)
   # the default group map.
   def default_attribute_groups
     values = work_package_attributes_by_default_group_key
+    values.reject! { |k, _| k == :estimates_and_time } if is_milestone?
 
     default_groups.keys.each_with_object([]) do |groupkey, array|
       members = values[groupkey]
@@ -167,9 +168,11 @@ def default_group_key(key)
   # it will put them into the other group.
   def work_package_attributes_by_default_group_key
     active_cfs = active_custom_field_attributes
+
     work_package_attributes
       .keys
       .select { |key| default_attribute?(active_cfs, key) }
+      .sort_by { |key| default_group_map.keys.index(key.to_sym) || default_group_map.keys.size }
       .group_by { |key| default_group_key(key.to_sym) }
   end
 

app/services/base_type_service.rb

@@ -71,12 +71,13 @@ def update(params, options)
   def set_params_and_validate(params)
     # Only set attribute groups when it exists
     # (Regression #28400)
-    unless params[:attribute_groups].nil?
-      set_attribute_groups(params)
-    end
+    set_attribute_groups(params) unless params[:attribute_groups].nil?
 
     # This should go before `set_scalar_params` call to get the
-    # project_ids, custom_field_ids diffs from the type and the params
+    # project_ids, custom_field_ids diffs from the type and the params.
+    # For determining the active custom fields for the type, it is necessary
+    # to know whether the type is a milestone or not.
+    set_milestone_param(params) unless params[:is_milestone].nil?
     set_active_custom_fields
 
     if params[:project_ids].present?
@@ -88,6 +89,10 @@ def set_params_and_validate(params)
     validate_and_save(type, user)
   end
 
+  def set_milestone_param(params)
+    type.is_milestone = params[:is_milestone]
+  end
+
   def set_scalar_params(params)
     type.attributes = params.except(:attribute_groups)
   end

config/locales/en.yml

@@ -2038,7 +2038,7 @@ Project attributes and sections are defined in the <a href=%{admin_settings_url}
   label_enterprise_support: "Enterprise support"
   label_enterprise_addon: "Enterprise add-on"
   label_environment: "Environment"
-  label_estimates_and_time: "Estimates and time"
+  label_estimates_and_time: "Estimates and progress"
   label_equals: "is"
   label_everywhere: "everywhere"
   label_example: "Example"

spec/features/types/form_configuration_spec.rb

@@ -85,7 +85,7 @@
 
       it "can remove all groups to be left with an invisible one (Regression #33592)" do
         form.remove_group "Details"
-        form.remove_group "Estimates and Time"
+        form.remove_group "Estimates and progress"
         form.remove_group "People"
         form.remove_group "Costs"
 
@@ -125,19 +125,20 @@
         #
         form.expect_group "people",
                           "People",
-                          key: :responsible, translation: "Accountable"
+                          { key: :assignee, translation: "Assignee" },
+                          { key: :responsible, translation: "Accountable" }
 
         form.expect_group "estimates_and_time",
-                          "Estimates and time",
+                          "Estimates and progress",
                           { key: :estimated_time, translation: "Work" },
-                          { key: :spent_time, translation: "Spent time" },
-                          { key: :remaining_time, translation: "Remaining work" }
+                          { key: :remaining_time, translation: "Remaining work" },
+                          { key: :percentage_done, translation: "% Complete" },
+                          { key: :spent_time, translation: "Spent time" }
 
         form.expect_group "details",
                           "Details",
                           { key: :category, translation: "Category" },
                           { key: :date, translation: "Date" },
-                          { key: :percentage_done, translation: "% Complete" },
                           { key: :priority, translation: "Priority" },
                           { key: :version, translation: "Version" }
 
@@ -180,15 +181,15 @@
                           { key: :responsible, translation: "Accountable" }
 
         form.expect_group "estimates_and_time",
-                          "Estimates and time",
+                          "Estimates and progress",
                           { key: :estimated_time, translation: "Work" },
-                          { key: :spent_time, translation: "Spent time" },
-                          { key: :remaining_time, translation: "Remaining work" }
+                          { key: :remaining_time, translation: "Remaining work" },
+                          { key: :percentage_done, translation: "% Complete" },
+                          { key: :spent_time, translation: "Spent time" }
 
         form.expect_group "Whatever",
                           "Whatever",
-                          { key: :date, translation: "Date" },
-                          { key: :percentage_done, translation: "% Complete" }
+                          { key: :date, translation: "Date" }
 
         form.expect_group "New Group",
                           "New Group",
@@ -213,7 +214,7 @@
         end
 
         wp_page.expect_group("Whatever") do
-          wp_page.expect_attributes percentageDone: "10"
+          wp_page.expect_attributes combinedDate: "no start date - no finish date"
         end
 
         wp_page.expect_group("Cool Stuff") do
@@ -224,7 +225,7 @@
         wp_page.expect_hidden_field(:assignee)
         wp_page.expect_hidden_field(:spent_time)
 
-        wp_page.expect_group("Estimates and time") do
+        wp_page.expect_group("Estimates and progress") do
           wp_page.expect_attributes estimated_time: "-"
           wp_page.expect_attributes spent_time: "0 h"
         end
@@ -234,7 +235,7 @@
         wp_page.expect_hidden_field(:spent_time)
         wp_page.click_create_wp_button(type)
 
-        wp_page.expect_group("Estimates and time") do
+        wp_page.expect_group("Estimates and progress") do
           expect(page).to have_css(".inline-edit--container.estimatedTime")
         end
 

spec/services/create_type_service_spec.rb

@@ -36,5 +36,25 @@
   let(:instance) { described_class.new(user) }
   let(:service_call) { instance.call({ name: "foo" }.merge(params), {}) }
 
-  it_behaves_like "type service"
+  it_behaves_like "type service" do
+    describe "default attribute_groups" do
+      context "for a milestone type" do
+        let(:params) { { is_milestone: true } }
+
+        it "does not include the progress attribute group" do
+          expect(service_call.result.attribute_groups.map(&:key))
+            .to eql %i[people details costs]
+        end
+      end
+
+      context "for a non milestone type" do
+        let(:params) { { is_milestone: false } }
+
+        it "does include the progress attribute group" do
+          expect(service_call.result.attribute_groups.map(&:key))
+            .to eql %i[people estimates_and_time details costs]
+        end
+      end
+    end
+  end
 end