Skip to content

Commit

Permalink
Merge pull request #16775 from opf/feature/57990-preserve-percent-com…
Browse files Browse the repository at this point in the history 
…plete-over-remaining-work-when-migrating

Feature/57990 preserve percent complete over remaining work when migrating
  • Loading branch information
@cbliard
cbliard authored Sep 24, 2024
2 parents 05ba624 + 98811fa commit 496acf5
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 49 deletions.
8 changes: 4 additions & 4 deletions app/workers/work_packages/progress/apply_statuses_change_job.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,12 @@ def adjust_progress_values
if WorkPackage.work_based_mode?
clear_percent_complete_when_0h_work
elsif WorkPackage.status_based_mode?
set_p_complete_from_status
set_percent_complete_from_status
if OpenProject::FeatureDecisions.percent_complete_edition_active?
fix_remaining_work_set_with_100p_complete
derive_unset_work_from_remaining_work_and_p_complete
fix_remaining_work_set_with_100_percent_complete
derive_unset_work_from_remaining_work_and_percent_complete
end
derive_remaining_work_from_work_and_p_complete
derive_remaining_work_from_work_and_percent_complete
end
end

Expand Down
60 changes: 32 additions & 28 deletions app/workers/work_packages/progress/migrate_values_job.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,33 @@ def perform(current_mode:, previous_mode:)
def adjust_progress_values
case current_mode
when "field"
unset_all_percent_complete_values if previous_mode == "disabled"
fix_remaining_work_set_with_100p_complete
fix_remaining_work_exceeding_work
fix_only_work_being_set
fix_only_remaining_work_being_set
derive_unset_remaining_work_from_work_and_p_complete
derive_unset_work_from_remaining_work_and_p_complete
derive_p_complete_from_work_and_remaining_work
adjust_progress_values_for_work_based_mode
when "status"
set_p_complete_from_status
fix_remaining_work_set_with_100p_complete
derive_unset_work_from_remaining_work_and_p_complete
derive_remaining_work_from_work_and_p_complete
adjust_progress_values_for_status_based_mode
else
raise "Unknown progress calculation mode: #{current_mode}, aborting."
end
end

def adjust_progress_values_for_work_based_mode
unset_all_percent_complete_values if previous_mode == "disabled"
fix_remaining_work_set_with_100_percent_complete
fix_remaining_work_exceeding_work
fix_only_work_being_set
fix_only_remaining_work_being_set
derive_unset_work_from_remaining_work_and_percent_complete
derive_unset_percent_complete_from_work_and_remaining_work
fix_percent_complete_and_remaining_work_when_work_is_0h
derive_remaining_work_from_work_and_percent_complete
end

def adjust_progress_values_for_status_based_mode
set_percent_complete_from_status
fix_remaining_work_set_with_100_percent_complete
derive_unset_work_from_remaining_work_and_percent_complete
derive_remaining_work_from_work_and_percent_complete
end

def unset_all_percent_complete_values
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
Expand Down Expand Up @@ -97,32 +106,26 @@ def fix_only_work_being_set
SQL
end

def fix_only_remaining_work_being_set
def fix_percent_complete_and_remaining_work_when_work_is_0h
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET estimated_hours = remaining_hours
WHERE estimated_hours IS NULL
AND remaining_hours IS NOT NULL
AND done_ratio IS NULL
SET remaining_hours = 0,
done_ratio = NULL
WHERE estimated_hours = 0
SQL
end

def derive_unset_remaining_work_from_work_and_p_complete
def fix_only_remaining_work_being_set
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
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
SET estimated_hours = remaining_hours
WHERE estimated_hours IS NULL
AND remaining_hours IS NOT NULL
AND done_ratio IS NULL
SQL
end

def derive_p_complete_from_work_and_remaining_work
def derive_unset_percent_complete_from_work_and_remaining_work
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET done_ratio = CASE
Expand All @@ -131,6 +134,7 @@ def derive_p_complete_from_work_and_remaining_work
END
WHERE estimated_hours >= 0
AND remaining_hours >= 0
AND done_ratio IS NULL
SQL
end

Expand Down
8 changes: 4 additions & 4 deletions app/workers/work_packages/progress/sql_commands.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def drop_temporary_progress_table
SQL
end

def derive_remaining_work_from_work_and_p_complete
def derive_remaining_work_from_work_and_percent_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET remaining_hours =
Expand All @@ -74,7 +74,7 @@ def derive_remaining_work_from_work_and_p_complete
SQL
end

def set_p_complete_from_status
def set_percent_complete_from_status
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET done_ratio = statuses.default_done_ratio
Expand All @@ -83,7 +83,7 @@ def set_p_complete_from_status
SQL
end

def fix_remaining_work_set_with_100p_complete
def fix_remaining_work_set_with_100_percent_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET estimated_hours = remaining_hours,
Expand All @@ -94,7 +94,7 @@ def fix_remaining_work_set_with_100p_complete
SQL
end

def derive_unset_work_from_remaining_work_and_p_complete
def derive_unset_work_from_remaining_work_and_percent_complete
execute(<<~SQL.squish)
UPDATE temp_wp_progress_values
SET estimated_hours =
Expand Down
2 changes: 1 addition & 1 deletion config/locales/en.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1933,7 +1933,7 @@ en:
progress_mode_changed_to_status_based: Progress calculation mode set to status-based
status_excluded_from_totals_set_to_false_message: now included in hierarchy totals
status_excluded_from_totals_set_to_true_message: now excluded from hierarchy totals
status_percent_complete_changed: "% complete changed from %{old_value}% to %{new_value}%"
status_percent_complete_changed: "% Complete changed from %{old_value}% to %{new_value}%"
system_update:
file_links_journal: >
From now on, activity related to file links (files stored in external storages) will appear here in the
Expand Down
8 changes: 4 additions & 4 deletions spec/lib/journal_formatter/cause_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,11 @@ def render(cause, html:)

it do
expect(cause).to render_html_variant("<strong>Status &#39;In progress&#39;</strong> " \
"% complete changed from 20% to 40%")
"% Complete changed from 20% to 40%")
end

it do
expect(cause).to render_raw_variant("Status 'In progress' % complete changed from 20% to 40%")
expect(cause).to render_raw_variant("Status 'In progress' % Complete changed from 20% to 40%")
end

it_behaves_like "XSS-proof rendering of status name"
Expand Down Expand Up @@ -412,12 +412,12 @@ def render(cause, html:)

it do
expect(cause).to render_html_variant("<strong>Status &#39;In progress&#39;</strong> " \
"% complete changed from 20% to 40% and now excluded from hierarchy totals")
"% Complete changed from 20% to 40% and now excluded from hierarchy totals")
end

it do
expect(cause).to render_raw_variant("Status 'In progress' " \
"% complete changed from 20% to 40% and now excluded from hierarchy totals")
"% Complete changed from 20% to 40% and now excluded from hierarchy totals")
end

it_behaves_like "XSS-proof rendering of status name"
Expand Down
24 changes: 16 additions & 8 deletions spec/migrations/update_progress_calculation_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ def expect_migrates(from:, to:)
wp both w and pc set | 10h | | 60%
wp only w set 0h | 0h | |
TABLE
# % Complete is discarded and derived because of the "disabled" mode
to: <<~TABLE
subject | work | remaining work | % complete
wp only w set | 10h | 10h | 0%
Expand All @@ -213,6 +214,7 @@ def expect_migrates(from:, to:)
wp both rw and pc set | | 4h | 60%
wp only rw set 0h | | 0h |
TABLE
# % Complete is discarded and derived because of the "disabled" mode
to: <<~TABLE
subject | work | remaining work | % complete
wp only rw set | 4h | 4h | 0%
Expand All @@ -233,6 +235,7 @@ def expect_migrates(from:, to:)
wp all set consistent | 10h | 4h | 60%
wp all set inconsistent | 10h | 1h | 10%
TABLE
# % Complete is discarded and derived because of the "disabled" mode
to: <<~TABLE
subject | work | remaining work | % complete
wp both w and rw set | 10h | 4h | 60%
Expand All @@ -254,6 +257,7 @@ def expect_migrates(from:, to:)
wp all set inconsistent | 10h | 10h | 60%
wp all set inconsistent 0h| 0h | 0h | 60%
TABLE
# % Complete is discarded and derived because of the "disabled" mode
to: <<~TABLE
subject | work | remaining work | % complete
wp both w and rw set | 10h | 10h | 0%
Expand All @@ -273,6 +277,7 @@ def expect_migrates(from:, to:)
wp both w and big rw set | 10h | 99h |
wp all set big wp | 10h | 99h | 60%
TABLE
# % Complete is discarded and derived because of the "disabled" mode
to: <<~TABLE
subject | work | remaining work | % complete
wp both w and big rw set | 10h | 10h | 0%
Expand Down Expand Up @@ -493,7 +498,7 @@ def expect_migrates(from:, to:)
to: <<~TABLE
subject | work | remaining work | % complete
wp all set consistent | 10h | 4h | 60%
wp all set inconsistent | 10h | 1h | 90%
wp all set inconsistent | 10h | 9h | 10%
TABLE
)
end
Expand All @@ -511,7 +516,7 @@ def expect_migrates(from:, to:)
to: <<~TABLE
subject | work | remaining work | % complete
wp all set consistent | 10h | 10h | 0%
wp all set inconsistent | 10h | 10h | 0%
wp all set inconsistent | 10h | 4h | 60%
wp all set 0h | 0h | 0h |
TABLE
)
Expand Down Expand Up @@ -823,14 +828,14 @@ def expect_migrates(from:, to:)
run_migration
end

it "fixes the total values and sets ∑ % complete to nil (not 0) but keeps % complete (unless wrong)" do
it "fixes the total values and sets ∑ % complete to nil (not 0) and keeps % complete (unless wrong)" do
expect_work_packages(table_work_packages.map(&:reload), <<~TABLE)
subject | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete |
wp zero | | | 0 | | | |
wp correct | 100h | 50h | 50 | 100h | 50h | 50 |
wp correct child | | | | | | |
wp wrong | | | 90 | 100h | 50h | 50 |
wp wrong child | 100h | 50h | 50 | | | |
wp wrong | | | 90 | 100h | 80h | 20 |
wp wrong child | 100h | 80h | 20 | | | |
TABLE
end

Expand Down Expand Up @@ -858,14 +863,17 @@ def expect_migrates(from:, to:)
.to eql [nil, 50]
end

it "creates a journal for the work package transitioning ∑ % complete from 90 to 50" do
it "creates a journal for the work package transitioning ∑ remaining work from 50h to 80h " \
"and ∑ % complete from 90% to 20%" do
expect(wp_wrong.journals.count).to eq(2)

expect(wp_wrong.journals.first.get_changes.keys)
.not_to include("derived_done_ratio")

expect(wp_wrong.journals.last.get_changes["derived_done_ratio"])
.to eql [nil, 50]
expect(wp_wrong.journals.last.get_changes).to include(
"derived_remaining_hours" => [50.0, 80.0],
"derived_done_ratio" => [nil, 20]
)
end
end
end
Expand Down

0 comments on commit 496acf5

Please sign in to comment.