Merge pull request #15159 from opf/feature/40749--tie-work-and-remain…

…ing-work-together

Tie work and remaining work together

app/contracts/work_packages/base_contract.rb

@@ -57,12 +57,15 @@ class BaseContract < ::ModelContract
     attribute :derived_done_ratio,
               writable: false
 
-    attribute :estimated_hours
+    attribute :estimated_hours do
+      validate_work_is_set_when_remaining_work_is_set
+    end
     attribute :derived_estimated_hours,
               writable: false
 
     attribute :remaining_hours do
-      validate_remaining_hours_doesnt_exceed_estimated_hours
+      validate_remaining_work_is_lower_than_work
+      validate_remaining_work_is_set_when_work_is_set
     end
     attribute :derived_remaining_hours,
               writable: false
@@ -320,8 +323,8 @@ def validate_version_is_assignable
       end
     end
 
-    def validate_remaining_hours_doesnt_exceed_estimated_hours
-      if both_remaining_and_estimated_hours_present? && remaining_work_exceeds_work?
+    def validate_remaining_work_is_lower_than_work
+      if work_set? && remaining_work_set? && remaining_work_exceeds_work?
         if model.changed.include?("estimated_hours")
           errors.add(:estimated_hours, :cant_be_inferior_to_remaining_work)
         end
@@ -332,8 +335,24 @@ def validate_remaining_hours_doesnt_exceed_estimated_hours
       end
     end
 
-    def both_remaining_and_estimated_hours_present?
-      model.remaining_hours.present? && model.estimated_hours.present?
+    def validate_remaining_work_is_set_when_work_is_set
+      if work_set? && !remaining_work_set?
+        errors.add(:remaining_hours, :must_be_set_when_work_is_set)
+      end
+    end
+
+    def validate_work_is_set_when_remaining_work_is_set
+      if remaining_work_set? && !work_set?
+        errors.add(:estimated_hours, :must_be_set_when_remaining_work_is_set)
+      end
+    end
+
+    def work_set?
+      model.estimated_hours.present?
+    end
+
+    def remaining_work_set?
+      model.remaining_hours.present?
     end
 
     def remaining_work_exceeds_work?

app/services/work_packages/set_attributes_service.rb

@@ -360,18 +360,16 @@ def invalid_progress_values?
     work && remaining_work && remaining_work > work
   end
 
-  # rubocop:disable Metrics/AbcSize
   def update_estimated_hours
     return unless WorkPackage.use_field_for_done_ratio?
     return if work_package.estimated_hours_changed?
     return if work_package.estimated_hours.present?
     return unless work_package.remaining_hours_changed?
 
-    if work_package.remaining_hours.present? && work_package.done_ratio.present?
+    if work_package.remaining_hours.present?
       work_package.estimated_hours = estimated_hours_from_done_ratio_and_remaining_hours
     end
   end
-  # rubocop:enable Metrics/AbcSize
 
   # When in "Status-based" mode for % Complete, remaining hours are based
   # on the computation of it derived from the status's default done ratio
@@ -397,15 +395,15 @@ def update_remaining_hours
   # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity
 
   def estimated_hours_from_done_ratio_and_remaining_hours
-    remaining_ratio = 1.0 - (work_package.done_ratio / 100.0)
+    remaining_ratio = 1.0 - ((work_package.done_ratio || 0) / 100.0)
     work_package.remaining_hours / remaining_ratio
   end
 
   def remaining_hours_from_done_ratio_and_estimated_hours
     return nil if work_package.done_ratio.nil? || work_package.estimated_hours.nil?
 
     completed_work = work_package.estimated_hours * work_package.done_ratio / 100.0
-    work_package.estimated_hours - completed_work
+    (work_package.estimated_hours - completed_work).round(2)
   end
 
   def set_version_to_nil

config/locales/en.yml

@@ -1024,10 +1024,12 @@ Project attributes and sections are defined in the <a href=%{admin_settings_url}
               only_same_project_categories_allowed: "The category of a work package must be within the same project as the work package."
               does_not_exist: "The specified category does not exist."
             estimated_hours:
+              cant_be_inferior_to_remaining_work: "can't be inferior to 'Remaining work'."
+              must_be_set_when_remaining_work_is_set: "must be set when 'Remaining work' is set."
               only_values_greater_or_equal_zeroes_allowed: "must be >= 0."
-              cant_be_inferior_to_remaining_work: "can't be inferior to 'Remaining work'"
             remaining_hours:
-              cant_exceed_work: "can't exceed 'Work'"
+              cant_exceed_work: "can't exceed 'Work'."
+              must_be_set_when_work_is_set: "must be set when 'Work' is set."
           readonly_status: "The work package is in a readonly status so its attributes cannot be changed."
         type:
           attributes:

db/migrate/20240328154805_change_status_default_done_ratio_to_zero.rb

@@ -0,0 +1,11 @@
+class ChangeStatusDefaultDoneRatioToZero < ActiveRecord::Migration[7.1]
+  def change
+    change_column_default :statuses, :default_done_ratio, from: nil, to: 0
+    reversible do |dir|
+      dir.up do
+        execute "UPDATE statuses SET default_done_ratio = 0 WHERE default_done_ratio IS NULL"
+      end
+    end
+    change_column_null :statuses, :default_done_ratio, false
+  end
+end

modules/backlogs/spec/api/work_package_resource_spec.rb

@@ -39,6 +39,7 @@
     create(:work_package,
            project:,
            story_points: 8,
+           estimated_hours: 5,
            remaining_hours: 5)
   end
   let(:wp_path) { "/api/v3/work_packages/#{work_package.id}" }

modules/backlogs/spec/services/stories/create_service_spec.rb

@@ -30,22 +30,12 @@
 
 RSpec.describe Stories::CreateService, type: :model do
   let(:priority) { create(:priority) }
-  let(:project) do
-    project = create(:project, types: [type_feature])
-
-    create(:member,
-           principal: user,
-           project:,
-           roles: [role])
-    project
-  end
-  let(:role) { create(:project_role, permissions:) }
-  let(:permissions) { %i(add_work_packages manage_subtasks assign_versions) }
+  let(:project) { create(:project, types: [type_feature]) }
   let(:status) { create(:status) }
   let(:type_feature) { create(:type_feature) }
 
   let(:user) do
-    create(:user)
+    create(:user, member_with_permissions: { project => %i(add_work_packages manage_subtasks assign_versions) })
   end
 
   let(:instance) do
@@ -67,8 +57,6 @@
   let(:version) { create(:version, project:) }
 
   let(:story) do
-    project.enabled_module_names += ["backlogs"]
-
     create(:story,
            version:,
            project:,
@@ -83,24 +71,30 @@
 
   subject { instance.call(attributes:) }
 
-  describe "remaining_hours" do
+  describe "creating a feature in a story" do
     before do
       subject
     end
 
-    context "with the story having remaining_hours" do
+    context "with the feature being created with some remaining work (remaining hours)" do
       let(:remaining_hours) { 15.0 }
 
-      it "does update the parents remaining hours" do
-        expect(story.reload.derived_remaining_hours).to eq(15)
+      it "updates the story total work, total remaining work, and total % complete" do
+        expect(subject).to be_success
+        expect(story.reload).to have_attributes(derived_estimated_hours: 15,
+                                                derived_remaining_hours: 15,
+                                                derived_done_ratio: 0)
       end
     end
 
-    context "with the subtask not having remaining_hours" do
+    context "with the feature being created without any remaining work (remaining hours)" do
       let(:remaining_hours) { nil }
 
-      it "does not note remaining hours to be changed" do
-        expect(story.reload.remaining_hours).to be_nil
+      it "does not change the story total work, total remaining work, and total % complete" do
+        expect(subject).to be_success
+        expect(story.reload).to have_attributes(derived_estimated_hours: nil,
+                                                derived_remaining_hours: nil,
+                                                derived_done_ratio: nil)
       end
     end
   end

spec/contracts/work_packages/base_contract_spec.rb

@@ -30,6 +30,8 @@
 require "contracts/shared/model_contract_shared_context"
 
 RSpec.describe WorkPackages::BaseContract do
+  include_context "ModelContract shared context"
+
   let(:work_package) do
     build_stubbed(:work_package,
                   type:,
@@ -66,8 +68,6 @@
 
   subject(:contract) { described_class.new(work_package, current_user) }
 
-  include_context "ModelContract shared context"
-
   shared_examples_for "invalid if changed" do |attribute|
     before do
       allow(work_package).to receive(:changed).and_return(changed_values.map(&:to_s))
@@ -290,76 +290,58 @@
     end
   end
 
-  describe "estimated hours" do
+  describe "work (estimated hours)" do
     let(:estimated_hours) { 1 }
+    let(:remaining_hours) { 0 }
 
     before do
       work_package.estimated_hours = estimated_hours
-      work_package.remaining_hours = nil
+      work_package.remaining_hours = remaining_hours
+      work_package.clear_remaining_hours_change
     end
 
     context "when > 0" do
       let(:estimated_hours) { 1 }
 
-      it "is valid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to be_empty
-      end
+      include_examples "contract is valid"
     end
 
     context "when 0" do
       let(:estimated_hours) { 0 }
 
-      it "is valid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to be_empty
-      end
+      include_examples "contract is valid"
     end
 
-    context "when nil" do
+    context "when nil while remaining work is nil" do
       let(:estimated_hours) { nil }
+      let(:remaining_hours) { nil }
 
-      it "is valid" do
-        contract.validate
+      include_examples "contract is valid"
+    end
 
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to be_empty
-      end
+    context "when nil while remaining work is set" do
+      let(:estimated_hours) { nil }
+      let(:remaining_hours) { 2.0 }
+
+      include_examples "contract is invalid", estimated_hours: :must_be_set_when_remaining_work_is_set
     end
 
     context "when < 0" do
       let(:estimated_hours) { -1 }
+      let(:remaining_hours) { -2 }
 
-      it "is invalid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to contain_exactly(:only_values_greater_or_equal_zeroes_allowed)
-      end
+      include_examples "contract is invalid", estimated_hours: :only_values_greater_or_equal_zeroes_allowed
     end
 
-    context "when inferior to remaining_hours" do
+    context "when inferior to remaining work" do
       let(:estimated_hours) { 5.0 }
+      let(:remaining_hours) { 7.0 }
 
-      before do
-        work_package.remaining_hours = 6.0
-        allow(work_package).to receive(:changed).and_return(%w[estimated_hours])
-      end
-
-      it "is invalid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to contain_exactly(:cant_be_inferior_to_remaining_work)
-      end
+      include_examples "contract is invalid", estimated_hours: :cant_be_inferior_to_remaining_work
     end
   end
 
-  describe "derived estimated hours" do
+  describe "total work (derived estimated hours)" do
     let(:changed_values) { [] }
     let(:attribute) { :derived_estimated_hours }
 
@@ -384,63 +366,47 @@
     end
   end
 
-  describe "remaining hours" do
+  describe "remaining work (remaining hours)" do
     before do
       work_package.estimated_hours = 5.0
+      work_package.clear_remaining_hours_change
       work_package.remaining_hours = remaining_hours
-
-      allow(work_package).to receive(:changed).and_return(%w[remaining_hours])
     end
 
-    context "when it's the same as estimated_hours" do
+    context "when it's the same as work" do
       let(:remaining_hours) { 5.0 }
 
-      it "is valid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:remaining_hours))
-          .to be_empty
-      end
+      include_examples "contract is valid"
     end
 
-    context "when it's less than estimated_hours" do
+    context "when it's less than work" do
       let(:remaining_hours) { 4.0 }
 
-      it "is valid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:remaining_hours))
-          .to be_empty
-      end
+      include_examples "contract is valid"
     end
 
-    context "when it's greater than estimated_hours" do
+    context "when it's greater than work" do
       let(:remaining_hours) { 6.0 }
 
-      it "is invalid" do
-        contract.validate
+      include_examples "contract is invalid", remaining_hours: :cant_exceed_work
+    end
 
-        expect(subject.errors.symbols_for(:remaining_hours))
-          .to contain_exactly(:cant_exceed_work)
-      end
+    context "when unset" do
+      let(:remaining_hours) { nil }
+
+      include_examples "contract is invalid", remaining_hours: :must_be_set_when_work_is_set
     end
   end
 
-  describe "estimated_hours and remaining_hours" do
-    context "when changing both and remaining_hours exceeds estimated_hours" do
+  describe "work and remaining work" do
+    context "when changing both and remaining work exceeds work" do
       before do
         work_package.estimated_hours = 5.0
         work_package.remaining_hours = 6.0
       end
 
-      it "is invalid" do
-        contract.validate
-
-        expect(subject.errors.symbols_for(:estimated_hours))
-          .to contain_exactly(:cant_be_inferior_to_remaining_work)
-        expect(subject.errors.symbols_for(:remaining_hours))
-          .to contain_exactly(:cant_exceed_work)
-      end
+      include_examples "contract is invalid", estimated_hours: :cant_be_inferior_to_remaining_work,
+                                              remaining_hours: :cant_exceed_work
     end
   end