Skip to content

Commit

Permalink
Merge pull request #15311 from opf/bug/54310-rounding-should-not-make…
Browse files Browse the repository at this point in the history 
…-remaining-work-higher-than-work

[54310] Fix remaining work exceeding work when rounding
  • Loading branch information
@aaron-contreras
aaron-contreras committed Apr 18, 2024
2 parents 5c9220c + 67df322 commit 9436efa
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 33 deletions.
9 changes: 8 additions & 1 deletion app/services/work_packages/set_attributes_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ def update_progress_attributes
update_remaining_hours
update_done_ratio
end
round_progress_values
end

def only_percent_complete_initially_set?
Expand Down Expand Up @@ -330,6 +331,11 @@ def update_done_ratio
end
end

def round_progress_values
work_package.estimated_hours = work_package.estimated_hours&.round(2)
work_package.remaining_hours = work_package.remaining_hours&.round(2)
end

def update_remaining_hours_from_percent_complete
return if work_package.estimated_hours&.negative?

Expand Down Expand Up @@ -408,7 +414,8 @@ 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).round(2)
remaining_hours = (work_package.estimated_hours - completed_work).round(2)
remaining_hours.clamp(0.0, work_package.estimated_hours)
end

def set_version_to_nil
Expand Down
20 changes: 17 additions & 3 deletions app/workers/work_packages/update_progress_job.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,12 @@ def fix_only_remaining_work_being_set
def derive_unset_remaining_work_from_work_and_p_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET remaining_hours = ROUND((estimated_hours - (estimated_hours * done_ratio / 100.0))::numeric, 2)
SET remaining_hours =
GREATEST(0,
LEAST(estimated_hours,
ROUND((estimated_hours - (estimated_hours * done_ratio / 100.0))::numeric, 2)
)
)
WHERE estimated_hours IS NOT NULL
AND remaining_hours IS NULL
AND done_ratio IS NOT NULL
Expand All @@ -155,7 +160,12 @@ def derive_unset_remaining_work_from_work_and_p_complete
def derive_remaining_work_from_work_and_p_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET remaining_hours = ROUND((estimated_hours - (estimated_hours * done_ratio / 100.0))::numeric, 2)
SET remaining_hours =
GREATEST(0,
LEAST(estimated_hours,
ROUND((estimated_hours - (estimated_hours * done_ratio / 100.0))::numeric, 2)
)
)
WHERE estimated_hours IS NOT NULL
AND done_ratio IS NOT NULL
SQL
Expand All @@ -164,7 +174,11 @@ def derive_remaining_work_from_work_and_p_complete
def derive_unset_work_from_remaining_work_and_p_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET estimated_hours = ROUND((remaining_hours * 100 / (100 - done_ratio))::numeric, 2)
SET estimated_hours =
CASE done_ratio
WHEN 0 THEN remaining_hours
ELSE ROUND((remaining_hours * 100 / (100 - done_ratio))::numeric, 2)
END
WHERE estimated_hours IS NULL
AND remaining_hours IS NOT NULL
AND done_ratio IS NOT NULL
Expand Down
38 changes: 35 additions & 3 deletions spec/features/work_packages/progress_modal_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ def update_work_package_with(work_package, attributes)

describe "field value format" do
context "with all values set" do
before { update_work_package_with(work_package, estimated_hours: 10.0, remaining_hours: 2.543) }
before { update_work_package_with(work_package, estimated_hours: 10.0, remaining_hours: 2.12345) }

it "populates fields with correctly values formatted " \
"with the minimum fractional part if present, and 2 decimals max" do
Expand All @@ -329,8 +329,40 @@ def update_work_package_with(work_package, attributes)
work_edit_field.activate!

work_edit_field.expect_modal_field_value("10")
remaining_work_edit_field.expect_modal_field_value("2.54")
percent_complete_edit_field.expect_modal_field_value("74", readonly: true)
remaining_work_edit_field.expect_modal_field_value("2.12")
percent_complete_edit_field.expect_modal_field_value("78", readonly: true)
end
end

context "with % complete set and setting long decimal values in modal" do
before do
work_package.attributes = {
estimated_hours: nil, remaining_hours: nil, done_ratio: 89
}
work_package.save(validate: false)
end

it "does not lose precision due to conversion from ISO duration to hours" do
work_package_table.visit_query(progress_query)
work_package_table.expect_work_package_listed(work_package)

work_edit_field = ProgressEditField.new(work_package_row, :estimatedTime)
remaining_work_edit_field = ProgressEditField.new(work_package_row, :remainingTime)

# set work to 2.5567
work_edit_field.activate!
work_edit_field.update("2.5567")
work_package_table.expect_and_dismiss_toaster(message: "Successful update.")

# work should have been set to 2.56 and remaining work to 0.28
work_package.reload
expect(work_package.estimated_hours).to eq(2.56)
expect(work_package.remaining_hours).to eq(0.28)

# work should be displayed as 2.56, and remaining work as 0.28
work_edit_field.activate!
work_edit_field.expect_modal_field_value("2.56")
remaining_work_edit_field.expect_modal_field_value("0.28")
end
end

Expand Down
60 changes: 34 additions & 26 deletions spec/migrations/update_progress_calculation_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,14 +358,17 @@ def expect_migrates(from:, to:)
it "derives Remaining work from Work and % Complete" do
expect_migrates(
from: <<~TABLE,
subject | work | remaining work | % complete
wp both w and pc set | 10h | | 60%
wp both w and pc set 0h | 0h | | 0%
subject | work | remaining work | % complete
wp w and pc set | 10h | | 60%
wp w and pc set 0h | 0h | | 0%
wp w and pc set 5.678h | 5.678h | | 0%
TABLE
to: <<~TABLE
subject | work | remaining work | % complete
wp both w and pc set | 10h | 4h | 60%
wp both w and pc set 0h | 0h | 0h |
subject | work | remaining work | % complete
wp w and pc set | 10h | 4h | 60%
wp w and pc set 0h | 0h | 0h |
# no rounding if rounding remaining work would exceed work
wp w and pc set 5.678h | 5.678h | 5.678h | 0%
TABLE
)
end
Expand All @@ -376,17 +379,20 @@ def expect_migrates(from:, to:)
expect_migrates(
from: <<~TABLE,
subject | work | remaining work | % complete
wp both rw and pc set | | 4h | 60%
wp both rw and pc set 0% | | 4h | 0%
wp both rw and pc set dec | | 4h | 67%
wp both rw and pc set 0h | | 0h | 0%
wp rw and pc set | | 4h | 60%
wp rw and pc set 0% | | 4h | 0%
wp rw and pc set dec | | 4h | 67%
wp rw and pc set 0h | | 0h | 0%
wp rw and pc set 5.678h | | 5.678h | 0%
TABLE
to: <<~TABLE
subject | work | remaining work | % complete
wp both rw and pc set | 10h | 4h | 60%
wp both rw and pc set 0% | 4h | 4h | 0%
wp both rw and pc set dec | 12.12h | 4h | 67%
wp both rw and pc set 0h | 0h | 0h |
wp rw and pc set | 10h | 4h | 60%
wp rw and pc set 0% | 4h | 4h | 0%
wp rw and pc set dec | 12.12h | 4h | 67%
wp rw and pc set 0h | 0h | 0h |
# no rounding when % complete is 0%
wp rw and pc set 5.678h | 5.678h | 5.678h | 0%
TABLE
)
end
Expand Down Expand Up @@ -575,20 +581,22 @@ def expect_migrates(from:, to:)
it "sets % Complete value to the status value, and derives Remaining work" do
expect_migrates(
from: <<~TABLE,
subject | status | work | remaining work | % complete
wp w 0% | To do (0%) | 10h | |
wp w 30% | Doing (30%) | 10h | |
wp w 100% | Done (100%) | 10h | |
wp w 0% 0h | To do (0%) | 0h | |
wp w 100% 0h| Done (100%) | 0h | |
subject | status | work | remaining work | % complete
wp w 0% | To do (0%) | 10h | |
wp w 0% dec | To do (0%) | 5.678h | |
wp w 30% | Doing (30%) | 10h | |
wp w 100% | Done (100%) | 10h | |
wp w 0% 0h | To do (0%) | 0h | |
wp w 100% 0h| Done (100%) | 0h | |
TABLE
to: <<~TABLE
subject | status | work | remaining work | % complete
wp w 0% | To do (0%) | 10h | 10h | 0%
wp w 30% | Doing (30%) | 10h | 7h | 30%
wp w 100% | Done (100%) | 10h | 0h | 100%
wp w 0% 0h | To do (0%) | 0h | 0h | 0%
wp w 100% 0h| Done (100%) | 0h | 0h | 100%
subject | status | work | remaining work | % complete
wp w 0% | To do (0%) | 10h | 10h | 0%
wp w 0% dec | To do (0%) | 5.678h | 5.678h | 0%
wp w 30% | Doing (30%) | 10h | 7h | 30%
wp w 100% | Done (100%) | 10h | 0h | 100%
wp w 0% 0h | To do (0%) | 0h | 0h | 0%
wp w 100% 0h| Done (100%) | 0h | 0h | 100%
TABLE
)
end
Expand Down
40 changes: 40 additions & 0 deletions spec/services/work_packages/set_attributes_service_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,14 @@
it_behaves_like "service call",
description: "is an error state (to be detected by contract), and remaining work is kept"
end

context "when work is set with 2nd decimal rounding up" do
let(:call_attributes) { { estimated_hours: 3.567 } }
let(:expected_attributes) { { estimated_hours: 3.57, remaining_hours: 3.57 } }

it_behaves_like "service call",
description: "values are rounded up to 2 decimals and set to the same value"
end
end
end
end
Expand Down Expand Up @@ -355,6 +363,14 @@
it_behaves_like "service call", description: "is an error state (to be detected by contract), and % Complete is kept"
end

context "when work and remaining work are both changed to values with more than 2 decimals" do
let(:call_attributes) { { estimated_hours: 10.123456, remaining_hours: 5.6789 } }
let(:expected_attributes) { { estimated_hours: 10.12, remaining_hours: 5.68, done_ratio: 43 } }

it_behaves_like "service call", description: "rounds work and remaining work to 2 decimals " \
"and updates % complete accordingly"
end

context "when remaining work is changed to a value greater than work" do
let(:call_attributes) { { remaining_hours: 200.0 } }
let(:expected_kept_attributes) { %w[done_ratio] }
Expand Down Expand Up @@ -435,6 +451,20 @@
it_behaves_like "service call", description: "% complete is kept and work is updated accordingly"
end

context "when % complete is 0% and remaining work is changed to a decimal rounded up" do
let(:call_attributes) { { remaining_hours: 5.679 } }
let(:expected_attributes) { { estimated_hours: 5.68, remaining_hours: 5.68 } }
let(:expected_kept_attributes) { %w[done_ratio] }

before do
work_package.done_ratio = 0
work_package.send(:clear_changes_information)
end

it_behaves_like "service call",
description: "% complete is kept, values are rounded, and work is updated accordingly"
end

context "when work is set" do
let(:call_attributes) { { estimated_hours: 10.0 } }
let(:expected_attributes) { call_attributes.merge(done_ratio: 80.0) }
Expand Down Expand Up @@ -529,6 +559,16 @@
it_behaves_like "service call", description: "% complete is kept and remaining work is updated accordingly"
end

context "when work is set to a number with with 4 decimals" do
let(:call_attributes) { { estimated_hours: 2.5678 } }
let(:expected_attributes) { { estimated_hours: 2.57, remaining_hours: 1.03 } }
let(:expected_kept_attributes) { %w[done_ratio] }

it_behaves_like "service call",
description: "% complete is kept, work is rounded to 2 decimals, " \
"and remaining work is updated and rounded to 2 decimals"
end

context "when work and remaining work are set" do
let(:call_attributes) { { estimated_hours: 10.0, remaining_hours: 0 } }
let(:expected_attributes) { call_attributes.merge(done_ratio: 100) }
Expand Down

0 comments on commit 9436efa

Please sign in to comment.