From 0884a6e0e8a94ba446c14477ffe2a692bab63694 Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Thu, 28 Mar 2024 16:42:48 +0100
Subject: [PATCH] Work or Remaining work cannot be unset if the other is set

This is to ensure that the total work, total remaining work, and total
% Complete are always consistent.

See https://community.openproject.org/wp/40749 for the detailed
specification.
---
 app/contracts/work_packages/base_contract.rb  |  31 ++++-
 config/locales/en.yml                         |   6 +-
 .../work_packages/base_contract_spec.rb       | 114 ++++++------------
 3 files changed, 69 insertions(+), 82 deletions(-)

diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb
index 65342ddf3166..a43c9ef95279 100644
--- a/app/contracts/work_packages/base_contract.rb
+++ b/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?
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 8f79157233f8..7e8a37cfb57d 100644
--- a/config/locales/en.yml
+++ b/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:
diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb
index a57540730ba2..a24c86069d2d 100644
--- a/spec/contracts/work_packages/base_contract_spec.rb
+++ b/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