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/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 68d8d8d4bfd6..acff442134c2 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/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,7 +395,7 @@ 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 @@ -405,7 +403,7 @@ 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 diff --git a/config/locales/en.yml b/config/locales/en.yml index f14f2ab790d9..2327f8e7ddb5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1024,10 +1024,12 @@ Project attributes and sections are defined in the = 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/db/migrate/20240328154805_change_status_default_done_ratio_to_zero.rb b/db/migrate/20240328154805_change_status_default_done_ratio_to_zero.rb new file mode 100644 index 000000000000..b430e0966946 --- /dev/null +++ b/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 diff --git a/modules/backlogs/spec/api/work_package_resource_spec.rb b/modules/backlogs/spec/api/work_package_resource_spec.rb index 8954e078c884..d2cc6590f82e 100644 --- a/modules/backlogs/spec/api/work_package_resource_spec.rb +++ b/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}" } diff --git a/modules/backlogs/spec/services/stories/create_service_spec.rb b/modules/backlogs/spec/services/stories/create_service_spec.rb index 4f0caf212801..c1ca11146b69 100644 --- a/modules/backlogs/spec/services/stories/create_service_spec.rb +++ b/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 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 diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index 0d1f36d5f95c..6fedf2e13bd9 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe WorkPackages::SetAttributesService, type: :model do - shared_let(:status_no_pct_complete) { create(:status, default_done_ratio: nil, name: "no % complete") } + shared_let(:status_0_pct_complete) { create(:status, default_done_ratio: 0, name: "0% complete") } shared_let(:status_50_pct_complete) { create(:status, default_done_ratio: 50, name: "50% complete") } shared_let(:status_70_pct_complete) { create(:status, default_done_ratio: 70, name: "70% complete") } @@ -43,7 +43,7 @@ p end let(:work_package) do - wp = build_stubbed(:work_package, project:, status: status_no_pct_complete) + wp = build_stubbed(:work_package, project:, status: status_0_pct_complete) wp.type = initial_type wp.send(:clear_changes_information) @@ -184,33 +184,25 @@ it_behaves_like "service call", description: "recomputes remaining work accordingly" end - context "when another status with a default % complete value is set" do + context "when another status is set" do let(:call_attributes) { { status: status_70_pct_complete } } let(:expected_attributes) { { remaining_hours: 3.0 } } it_behaves_like "service call", description: "recomputes remaining work according to the % complete value of the new status" end - - context "when another status without any default % complete value is set" do - let(:call_attributes) { { status: status_no_pct_complete } } - let(:expected_attributes) { { remaining_hours: nil } } - - it_behaves_like "service call", - description: "unsets remaining work" - end end - context "given a work package with work and remaining work unset, and a status with no % complete" do + context "given a work package with work and remaining work unset, and a status with 0% complete" do before do - work_package.status = status_no_pct_complete + work_package.status = status_0_pct_complete work_package.done_ratio = work_package.status.default_done_ratio work_package.estimated_hours = nil work_package.remaining_hours = nil work_package.send(:clear_changes_information) end - context "when another status with a default % complete value is set" do + context "when another status with another % complete value is set" do let(:call_attributes) { { status: status_70_pct_complete } } let(:expected_attributes) { { remaining_hours: nil } } @@ -220,10 +212,10 @@ context "when work is set" do let(:call_attributes) { { estimated_hours: 10.0 } } - let(:expected_attributes) { { remaining_hours: nil } } + let(:expected_attributes) { { remaining_hours: 10.0 } } it_behaves_like "service call", - description: "remaining work remains unset" + description: "remaining work is updated accordingly from work and % complete value of the status" end end end @@ -246,13 +238,6 @@ it_behaves_like "service call", description: "sets the % complete value to the status default % complete value" end - - context "when another status with no % complete value is set" do - let(:call_attributes) { { status: status_no_pct_complete } } - let(:expected_attributes) { { done_ratio: nil } } - - it_behaves_like "service call", description: "unsets the % complete value" - end end end @@ -261,8 +246,8 @@ context "given a work package with work, remaining work, and % complete being set" do before do work_package.estimated_hours = 10.0 - work_package.remaining_hours = 6.0 - work_package.done_ratio = 40 + work_package.remaining_hours = 3.0 + work_package.done_ratio = 70 work_package.send(:clear_changes_information) end @@ -291,7 +276,7 @@ # work changed by +10h let(:call_attributes) { { estimated_hours: 10.0 + 10.0 } } let(:expected_attributes) do - { remaining_hours: 6.0 + 10.0, done_ratio: 20 } + { remaining_hours: 3.0 + 10.0, done_ratio: 35 } end it_behaves_like "service call", @@ -302,7 +287,7 @@ # work changed by -2h let(:call_attributes) { { estimated_hours: 10.0 - 2.0 } } let(:expected_attributes) do - { remaining_hours: 6.0 - 2.0, done_ratio: 50 } + { remaining_hours: 3.0 - 2.0, done_ratio: 87 } end it_behaves_like "service call", @@ -355,6 +340,14 @@ it_behaves_like "service call", description: "updates % complete accordingly" end + + context "when work is changed and remaining work is unset" do + let(:call_attributes) { { estimated_hours: 8.0, remaining_hours: nil } } + let(:expected_attributes) { { remaining_hours: 2.4 } } # would be 2.4000000000000004 without rounding + let(:expected_kept_attributes) { %w[done_ratio] } + + it_behaves_like "service call", description: "% complete is kept and remaining work is recomputed (and rounded)" + end end context "given a work package with work and % complete being set, and remaining work being unset" do @@ -481,6 +474,13 @@ it_behaves_like "service call", description: "remaining work is set to the same value and % complete is set to 0%" end + + context "when remaining work is set" do + let(:call_attributes) { { remaining_hours: 10.0 } } + let(:expected_attributes) { { estimated_hours: 10.0, done_ratio: 0 } } + + it_behaves_like "service call", description: "work is set to the same value and % complete is set to 0%" + end end end end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 33ed8bf16756..05c495d178fb 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -56,9 +56,8 @@ def set_attributes_on(work_package, attributes) describe "done_ratio/estimated_hours/remaining_hours propagation" do context "when setting the status of a work package" do - shared_let(:open_status) { create(:status) } - shared_let(:new_status_with_done_ratio) { create(:status, default_done_ratio: 100) } - shared_let(:new_status_without_done_ratio) { create(:status, default_done_ratio: nil) } + shared_let(:open_status) { create(:status, name: "open", default_done_ratio: 0) } + shared_let(:complete_status_with_100p_done_ratio) { create(:status, name: "complete", default_done_ratio: 100) } context 'when using the "status-based" % complete mode', with_settings: { work_package_done_ratio: "status" } do @@ -92,8 +91,8 @@ def call_update_ancestors_service(work_package) it "recomputes child remaining work and update ancestors total % complete accordingly" do value = case field - when :status then new_status_with_done_ratio - when :status_id then new_status_with_done_ratio.id + when :status then complete_status_with_100p_done_ratio + when :status_id then complete_status_with_100p_done_ratio.id end set_attributes_on(child, field => value) call_update_ancestors_service(child) @@ -107,28 +106,6 @@ def call_update_ancestors_service(work_package) end end end - - context "when changing child status to a status without any default done ratio" do - %i[status status_id].each do |field| - context "with the #{field} field" do - it "unsets child remaining work and update ancestors total % complete accordingly" do - value = - case field - when :status then new_status_without_done_ratio - when :status_id then new_status_without_done_ratio.id - end - set_attributes_on(child, field => value) - call_update_ancestors_service(child) - - expect_work_packages([parent, child], <<~TABLE) - | subject | work | total work | remaining work | total remaining work | % complete | total % complete | - | parent | 10h | 15h | 10h | 10h | 0% | 33% | - | child | 5h | 5h | | | | | - TABLE - end - end - end - end end end end