Bug/64292 disabling an incomplete phase leads to inconsistencies #19058

dombesz · 2025-05-29T09:17:34Z

Ticket

https://community.openproject.org/wp/64292

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

https://community.openproject.org/work_packages/64292 Reschedule following phases to the closest active preceding phase when activating/deactivating a phase. This is an important step, because otherwise unset phases' default dates will be left behind during rescheduling. Move default start date on phases during rescheduling on phases where no finish date is set.

This change is required because we do not refresh the form on every keystroke to dynamically display the clear button, so it's better to display it all the time as we do on the work package datepicker.

Copilot

Pull Request Overview

This PR ensures consistent rescheduling when disabling or updating phases without a complete date range, adds working‐day awareness, and updates related specs and UI behavior.

UpdateService now skips to the next working day after a finish date and supports rescheduling a following phase that only has a start date.
RescheduleService refactored to handle phases with only start dates and to separate date‐range updates into a helper.
ActivationService simplified preceding‐phase logic; form behavior for clear buttons was relaxed.
Tests and feature specs updated to cover the new scenarios and permission.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
spec/services/project_life_cycle_steps/update_service_spec.rb	Added test for following phase with only a start date.
spec/services/project_life_cycle_steps/reschedule_service_spec.rb	Renamed example and adjusted expected dates for start‐date‐only phases.
spec/services/project_life_cycle_steps/activation_service_spec.rb	Updated descriptions and expected dates to use closest preceding date range.
spec/features/projects/life_cycle/active_in_project_spec.rb	Added `edit_project_phases` permission to feature spec.
app/services/project_life_cycle_steps/update_service.rb	Use `Day.next_working` for initial reschedule date.
app/services/project_life_cycle_steps/reschedule_service.rb	Refactored looping logic and extracted `set_phase_dates_and_next_start`.
app/services/project_life_cycle_steps/activation_service.rb	Removed date‐range guard to always pick a preceding phase.
app/forms/projects/life_cycles/form.rb	Simplified `show_clear_button?`, now ignores value presence.

Comments suppressed due to low confidence (2)

app/forms/projects/life_cycles/form.rb:106

Removing the presence check causes the clear button to appear even when no date is set, which can confuse users. Consider reintroducing value(field_name).present? to hide the button when there is nothing to clear.

when :start_date

app/services/project_life_cycle_steps/reschedule_service.rb:50

[nitpick] Mutating the method argument from inside the loop can be error‐prone. Introduce a separate local variable (e.g. next_from) for clarity and to avoid side effects on the original parameter.

def reschedule_phases(phases:, from:)

toy

Looks good, what do you think about first two points?

toy · 2025-06-06T15:05:22Z

app/services/project_life_cycle_steps/activation_service.rb

It is probably better to use RescheduleService instead of UpdateService after upsert in persist, unless it leads to much code duplication

toy · 2025-06-06T15:36:55Z

app/services/project_life_cycle_steps/update_service.rb

@@ -48,7 +48,7 @@ def reschedule_following_phases
    end

    def initial_reschedule_date
-      model.active? ? model.finish_date + 1 : model.start_date
+      model.active? ? Day.next_working(from: model.finish_date).date : model.start_date


I think it is better to handle finding next working day in RescheduleService

Also looking at Day.next_working I have a feeling that it is easy for it to return nil - it is not rare to have even few weeks of non working days on organisation level (like CERN)

toy · 2025-06-06T15:39:20Z

app/services/project_life_cycle_steps/reschedule_service.rb

+          next_start_date = set_phase_dates_and_next_start(phase, from)
+          from = next_start_date unless next_start_date.nil?


Suggested change

next_start_date = set_phase_dates_and_next_start(phase, from)

from = next_start_date unless next_start_date.nil?

from = set_phase_dates_and_next_start(phase, from) || from

I believe it reads a bit better to have the condition instead of having just the ||.

toy · 2025-06-06T15:44:53Z

spec/services/project_life_cycle_steps/update_service_spec.rb

@@ -165,6 +165,17 @@
      end
    end

+    context "for following phase with a start date only" do
+      let!(:phase) { create(:project_phase, project:, start_date: date, finish_date: date) }
+      let!(:following) { create(:project_phase, project:, start_date: date + 1, finish_date: nil, duration: 3) }


duration should be nil here, probably easier implicitly:

Suggested change

let!(:following) { create(:project_phase, project:, start_date: date + 1, finish_date: nil, duration: 3) }

let!(:following) { create(:project_phase, project:, start_date: date + 1, finish_date: nil) }

Having an unrelated duration makes sure we do not accidentally clear or re-calculate the duration while rescheduling with a start date only.

toy · 2025-06-06T15:45:17Z

spec/services/project_life_cycle_steps/update_service_spec.rb

+      it "reschedules by moving the start date only" do
+        expect(service.call(start_date: date, finish_date: date + 1)).to be_success
+
+        expect(following).to have_attributes(start_date: date + 2, finish_date: nil, duration: 3)


Suggested change

expect(following).to have_attributes(start_date: date + 2, finish_date: nil, duration: 3)

expect(following).to have_attributes(start_date: date + 2, finish_date: nil, duration: nil)

#19058 (comment)

dombesz changed the base branch from dev to release/16.1 June 5, 2025 10:06

dombesz force-pushed the bug/64292-disabling-an-incomplete-phase-leads-to-inconsistencies branch 4 times, most recently from 0d78305 to e84e1ea Compare June 6, 2025 06:28

dombesz added 4 commits June 6, 2025 09:55

Always show clear button if the field is enabled.

0b0c479

This change is required because we do not refresh the form on every keystroke to dynamically display the clear button, so it's better to display it all the time as we do on the work package datepicker.

Move scheduling changes to the RescheduleService

4e393f2

Fix rubocop errors

a9e18cc

dombesz force-pushed the bug/64292-disabling-an-incomplete-phase-leads-to-inconsistencies branch from e84e1ea to a9e18cc Compare June 6, 2025 06:55

dombesz marked this pull request as ready for review June 6, 2025 08:53

dombesz added the needs review label Jun 6, 2025

dombesz requested a review from Copilot June 6, 2025 08:54

Copilot AI reviewed Jun 6, 2025

View reviewed changes

toy reviewed Jun 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bug/64292 disabling an incomplete phase leads to inconsistencies #19058

Bug/64292 disabling an incomplete phase leads to inconsistencies #19058

Uh oh!

dombesz commented May 29, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

toy left a comment

Uh oh!

toy Jun 6, 2025

Uh oh!

toy Jun 6, 2025

Uh oh!

toy Jun 6, 2025

Uh oh!

dombesz Jun 6, 2025

Uh oh!

toy Jun 6, 2025

Uh oh!

dombesz Jun 6, 2025

Uh oh!

toy Jun 6, 2025

Uh oh!

dombesz Jun 6, 2025

Uh oh!

Uh oh!

		next_start_date = set_phase_dates_and_next_start(phase, from)
		from = next_start_date unless next_start_date.nil?

	next_start_date = set_phase_dates_and_next_start(phase, from)
	from = next_start_date unless next_start_date.nil?
	from = set_phase_dates_and_next_start(phase, from) \|\| from

	let!(:following) { create(:project_phase, project:, start_date: date + 1, finish_date: nil, duration: 3) }
	let!(:following) { create(:project_phase, project:, start_date: date + 1, finish_date: nil) }

	expect(following).to have_attributes(start_date: date + 2, finish_date: nil, duration: 3)
	expect(following).to have_attributes(start_date: date + 2, finish_date: nil, duration: nil)

Bug/64292 disabling an incomplete phase leads to inconsistencies #19058

Are you sure you want to change the base?

Bug/64292 disabling an incomplete phase leads to inconsistencies #19058

Uh oh!

Conversation

dombesz commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

toy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dombesz commented May 29, 2025 •

edited

Loading