Merge pull request #15370 from opf/feature/49409-status-exclude-from-…

…totals

[49409] Exclude work package from totals calculation by status

app/components/statuses/row_component.rb

@@ -50,6 +50,10 @@ def readonly?
       checkmark(status.is_readonly?)
     end
 
+    def excluded_from_totals?
+      checkmark(status.excluded_from_totals?)
+    end
+
     def color
       helpers.icon_for_color status.color
     end

app/components/statuses/table_component.rb

@@ -60,9 +60,10 @@ def headers
         [:name, { caption: Status.human_attribute_name(:name) }],
         [:color, { caption: Status.human_attribute_name(:color) }],
         [:done_ratio, { caption: WorkPackage.human_attribute_name(:done_ratio) }],
-        [:default?, { caption: Status.human_attribute_name(:is_default) }],
-        [:closed?, { caption: Status.human_attribute_name(:is_closed) }],
-        [:readonly?, { caption: Status.human_attribute_name(:is_readonly) }],
+        [:default?, { caption: I18n.t("statuses.index.headers.is_default") }],
+        [:closed?, { caption: I18n.t("statuses.index.headers.is_closed") }],
+        [:readonly?, { caption: I18n.t("statuses.index.headers.is_readonly") }],
+        [:excluded_from_totals?, { caption: I18n.t("statuses.index.headers.excluded_from_totals") }],
         [:sort, { caption: I18n.t(:label_sort) }]
       ]
     end

app/controllers/statuses_controller.rb

@@ -61,7 +61,7 @@ def create
   def update
     @status = Status.find(params[:id])
     if @status.update(permitted_params.status)
-      apply_status_p_complete_change
+      recompute_progress_values
       flash[:notice] = I18n.t(:notice_successful_update)
       redirect_to action: "index"
     else
@@ -97,14 +97,16 @@ def show_local_breadcrumb
     true
   end
 
-  def apply_status_p_complete_change
-    return unless WorkPackage.use_status_for_done_ratio?
-    return unless @status.default_done_ratio_previously_changed?
+  def recompute_progress_values
+    attributes_triggering_recomputing = ["excluded_from_totals"]
+    attributes_triggering_recomputing << "default_done_ratio" if WorkPackage.use_status_for_done_ratio?
+    changes = @status.previous_changes.slice(*attributes_triggering_recomputing)
+    return if changes.empty?
 
-    WorkPackages::Progress::ApplyStatusesPCompleteJob
-      .perform_later(cause_type: "status_p_complete_changed",
+    WorkPackages::Progress::ApplyStatusesChangeJob
+      .perform_later(cause_type: "status_changed",
                      status_name: @status.name,
                      status_id: @status.id,
-                     change: @status.default_done_ratio_previous_change)
+                     changes:)
   end
 end

app/models/journal.rb

@@ -71,13 +71,13 @@ class Journal < ApplicationRecord
                    changed_days
                    status_name
                    status_id
-                   status_p_complete_change
+                   status_changes
                  ],
                  prefix: true
   VALID_CAUSE_TYPES = %w[
     default_attribute_written
     progress_mode_changed_to_status_based
-    status_p_complete_changed
+    status_changed
     system_update
     work_package_children_changed_times
     work_package_parent_changed_times

app/models/journal/caused_by_status_p_complete_changed.rb → app/models/journal/caused_by_status_changed.rb

@@ -26,14 +26,14 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 #
-class Journal::CausedByStatusPCompleteChanged < CauseOfChange::Base
-  def initialize(status_name:, status_id:, status_p_complete_change:)
+class Journal::CausedByStatusChanged < CauseOfChange::Base
+  def initialize(status_name:, status_id:, status_changes:)
     additional = {
       "status_name" => status_name,
       "status_id" => status_id,
-      "status_p_complete_change" => status_p_complete_change
+      "status_changes" => status_changes
     }
 
-    super("status_p_complete_changed", additional)
+    super("status_changed", additional)
   end
 end

app/models/permitted_params.rb

@@ -593,6 +593,7 @@ def self.permitted_attributes
           name
           color_id
           default_done_ratio
+          excluded_from_totals
           is_closed
           is_default
           is_readonly

app/models/work_package.rb

@@ -294,6 +294,10 @@ def milestone?
 
   alias_method :is_milestone?, :milestone?
 
+  def included_in_totals_calculation?
+    !status.excluded_from_totals
+  end
+
   def done_ratio
     if WorkPackage.use_status_for_done_ratio? && status && status.default_done_ratio
       status.default_done_ratio

app/services/settings/update_service.rb

@@ -46,7 +46,7 @@ def set_setting_value(name, value)
     new_value = derive_value(value)
     Setting[name] = new_value
     if name == :work_package_done_ratio && old_value != "status" && new_value == "status"
-      WorkPackages::Progress::ApplyStatusesPCompleteJob.perform_later(cause_type: "progress_mode_changed_to_status_based")
+      WorkPackages::Progress::ApplyStatusesChangeJob.perform_later(cause_type: "progress_mode_changed_to_status_based")
     end
   end
 

app/services/work_packages/update_ancestors/loader.rb

@@ -25,6 +25,22 @@
 #  See COPYRIGHT and LICENSE files for more details.
 
 class WorkPackages::UpdateAncestors::Loader
+  DESCENDANT_ATTRIBUTES = {
+    id: "id",
+    parent_id: "parent_id",
+    estimated_hours: "estimated_hours",
+    remaining_hours: "remaining_hours",
+    status_excluded_from_totals: "statuses.excluded_from_totals",
+    schedule_manually: "schedule_manually",
+    ignore_non_working_days: "ignore_non_working_days"
+  }.freeze
+
+  WorkPackageLikeStruct = Data.define(*DESCENDANT_ATTRIBUTES.keys) do
+    def included_in_totals_calculation?
+      !status_excluded_from_totals
+    end
+  end
+
   def initialize(work_package, include_former_ancestors)
     self.work_package = work_package
     self.include_former_ancestors = include_former_ancestors
@@ -40,7 +56,7 @@ def select
 
   def descendants_of(queried_work_package)
     @descendants ||= Hash.new do |hash, wp|
-      hash[wp] = replaced_related_of(wp, :descendants)
+      hash[wp] = replaced_related_descendants(wp)
     end
 
     @descendants[queried_work_package]
@@ -72,7 +88,7 @@ def ancestors
                    end
   end
 
-  # Replace descendants/leaves by ancestors if they are the same.
+  # Replace descendants by ancestors if they are the same.
   # This can e.g. be the case in scenario of
   # grandparent
   #      |
@@ -84,31 +100,27 @@ def ancestors
   # Then grandparent and parent are already in ancestors.
   # Parent might be modified during the UpdateAncestorsService run,
   # and the descendants of grandparent need to have the updated value.
-  def replaced_related_of(queried_work_package, relation_type)
-    related_of(queried_work_package, relation_type).map do |leaf|
+  def replaced_related_descendants(queried_work_package)
+    related_descendants(queried_work_package).map do |leaf|
       if work_package.id == leaf.id
         work_package
       elsif (ancestor = ancestors.detect { |a| a.id == leaf.id })
         ancestor
       else
-        yield leaf if block_given?
         leaf
       end
     end
   end
 
-  def related_of(queried_work_package, relation_type)
+  def related_descendants(queried_work_package)
     scope = queried_work_package
-              .send(relation_type)
+              .descendants
               .where.not(id: queried_work_package.id)
 
-    if send(:"#{relation_type}_joins")
-      scope = scope.joins(send(:"#{relation_type}_joins"))
-    end
-
     scope
-      .pluck(*send(:"selected_#{relation_type}_attributes"))
-      .map { |p| LoaderStruct.new(send(:"selected_#{relation_type}_attributes").zip(p).to_h) }
+      .left_joins(:status)
+      .pluck(*DESCENDANT_ATTRIBUTES.values)
+      .map { |p| WorkPackageLikeStruct.new(**DESCENDANT_ATTRIBUTES.keys.zip(p).to_h) }
   end
 
   # Returns the current ancestors sorted by distance (called generations in the table)
@@ -128,23 +140,6 @@ def former_ancestors
                           end
   end
 
-  def selected_descendants_attributes
-    # By having the id in here, we can avoid DISTINCT queries squashing duplicate values
-    %i(id estimated_hours parent_id schedule_manually ignore_non_working_days remaining_hours)
-  end
-
-  def descendants_joins
-    nil
-  end
-
-  def selected_leaves_attributes
-    %i(id done_ratio derived_estimated_hours estimated_hours is_closed remaining_hours derived_remaining_hours)
-  end
-
-  def leaves_joins
-    :status
-  end
-
   ##
   # Get the previous parent ID
   # This could either be +parent_id_was+ if parent was changed
@@ -165,9 +160,6 @@ def previous_change_parent_id
 
     previous_parent_changes = previous[:parent_id] || previous[:parent]
 
-    previous_parent_changes ? previous_parent_changes.first : nil
+    previous_parent_changes&.first
   end
-
-  class LoaderStruct < Hashie::Mash; end
-  LoaderStruct.disable_warnings
 end

app/services/work_packages/update_ancestors_service.rb

@@ -99,7 +99,7 @@ def derive_attributes(work_package, loader, attributes)
       # or the derived remaining hours, depending on the % Complete mode
       # currently active.
       #
-      %i[estimated_hours remaining_hours] => :derive_total_estimated_and_remaining_hours,
+      %i[estimated_hours remaining_hours status status_id] => :derive_total_estimated_and_remaining_hours,
       %i[estimated_hours remaining_hours done_ratio status status_id] => :derive_done_ratio,
       %i[ignore_non_working_days] => :derive_ignore_non_working_days
     }.each do |derivative_attributes, method|
@@ -155,7 +155,9 @@ def derive_total(work_package, attribute, loader)
     return if no_children?(work_package, loader)
 
     work_packages = [work_package] + loader.descendants_of(work_package)
-    values = work_packages.filter_map(&attribute)
+    values = work_packages
+      .filter(&:included_in_totals_calculation?)
+      .filter_map(&attribute)
     return if values.empty?
 
     values.sum.to_f

app/views/statuses/_form.html.erb

@@ -34,6 +34,7 @@ See COPYRIGHT and LICENSE files for more details.
   <div class="form--field">
     <%= f.select :default_done_ratio,
                  (0..100).step(10).map { |r| ["#{r} %", r] },
+                 { container_class: "-xslim" },
                  label: WorkPackage.human_attribute_name(:done_ratio) %>
   </div>
   <div class="form--field"><%= f.check_box "is_closed" %></div>
@@ -63,6 +64,13 @@ See COPYRIGHT and LICENSE files for more details.
     <% end %>
   </div>
 
+  <div class="form--field">
+    <%= f.check_box "excluded_from_totals" %>
+    <div class="form--field-instructions">
+      <p><%= t("statuses.edit.status_excluded_from_totals_text") %></p>
+    </div>
+  </div>
+
   <%= render partial: "/colors/color_autocomplete_field",
              locals: {
                form: f,

app/workers/work_packages/progress/apply_statuses_p_complete_job.rb → app/workers/work_packages/progress/apply_statuses_change_job.rb

@@ -28,34 +28,40 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-class WorkPackages::Progress::ApplyStatusesPCompleteJob < WorkPackages::Progress::Job
+class WorkPackages::Progress::ApplyStatusesChangeJob < WorkPackages::Progress::Job
   VALID_CAUSE_TYPES = %w[
     progress_mode_changed_to_status_based
-    status_p_complete_changed
+    status_changed
   ].freeze
 
+  attr_reader :cause_type, :status_name, :status_id, :changes
+
   # Updates % complete and remaining work of all work packages after a status %
   # complete value has been changed or the progress calculation mode was set to
   # status-based.
   #
-  # It creates a journal entry with the System user describing the change.
+  # It creates a journal entry with the System user describing the changes.
   #
   # @param status_name [String] The cause of the update to be put in the journal
   #   entry. Must be one of `VALID_CAUSE_TYPES`.
   # @param status_name [String] The name of the status to apply.
   # @param status_id [Integer] The ID of the status to apply. Not used
   #   currently, but here in case we need it in a later version.
-  # @param change [Object] The change object containing an array of [old, new]
-  #   values of the change.
+  # @param changes [Object] The changes object containing a map of array of
+  #   [old, new] values of the change, for instance
+  #   `{"default_done_ratio" => [20, 40], "excluded_from_totals" => [false, true]}`.
   # @return [void]
-  def perform(cause_type:, status_name: nil, status_id: nil, change: nil)
-    return if WorkPackage.use_field_for_done_ratio?
-
-    journal_cause = journal_cause_from(cause_type, status_name:, status_id:, change:)
+  def perform(cause_type:, status_name: nil, status_id: nil, changes: nil)
+    @cause_type = cause_type
+    @status_name = status_name
+    @status_id = status_id
+    @changes = changes
 
     with_temporary_progress_table do
-      set_p_complete_from_status
-      derive_remaining_work_from_work_and_p_complete
+      if WorkPackage.use_status_for_done_ratio?
+        set_p_complete_from_status
+        derive_remaining_work_from_work_and_p_complete
+      end
       update_totals
 
       copy_progress_values_to_work_packages_and_update_journals(journal_cause)
@@ -64,23 +70,43 @@ def perform(cause_type:, status_name: nil, status_id: nil, change: nil)
 
   private
 
-  def journal_cause_from(cause_type, status_name:, status_id:, change:)
-    if VALID_CAUSE_TYPES.exclude?(cause_type)
+  def journal_cause
+    assert_valid_cause_type!
+
+    @journal_cause ||=
+      case cause_type
+      when "progress_mode_changed_to_status_based"
+        Journal::CausedByProgressModeChangedToStatusBased.new
+      when "status_changed"
+        assert_status_information_present!
+        Journal::CausedByStatusChanged.new(
+          status_name:,
+          status_id:,
+          status_changes: changes
+        )
+      else
+        raise "Unable to handle cause type #{cause_type.inspect}"
+      end
+  end
+
+  def assert_valid_cause_type!
+    unless VALID_CAUSE_TYPES.include?(cause_type)
       raise ArgumentError, "Invalid cause type #{cause_type.inspect}. " \
                            "Valid values are #{VALID_CAUSE_TYPES.inspect}"
     end
+  end
+
+  def assert_status_information_present!
+    if status_name.blank?
+      raise ArgumentError, "status_name must be provided"
+    end
 
-    case cause_type
-    when "progress_mode_changed_to_status_based"
-      { type: cause_type }
-    when "status_p_complete_changed"
-      raise ArgumentError, "status_name must be provided" if status_name.blank?
-      raise ArgumentError, "status_id must be provided" if status_id.nil?
-      raise ArgumentError, "change must be provided" if change.nil?
+    if status_id.nil?
+      raise ArgumentError, "status_id must be provided"
+    end
 
-      { type: cause_type, status_name:, status_id:, status_p_complete_change: change }
-    else
-      raise "Unable to handle cause type #{cause_type.inspect}"
+    if changes.nil?
+      raise ArgumentError, "changes must be provided"
     end
   end
 end

app/workers/work_packages/progress/migrate_remove_totals_from_childless_work_packages_job.rb

@@ -27,6 +27,8 @@
 #++
 
 class WorkPackages::Progress::MigrateRemoveTotalsFromChildlessWorkPackagesJob < WorkPackages::Progress::Job
+  include WorkPackages::Progress::SqlCommandsForMigration
+
   def perform
     updated_work_package_ids = remove_totals_from_childless_work_packages
     create_journals_for_updated_work_packages(updated_work_package_ids, cause: journal_cause)