Skip to content

Commit

Permalink
[59539] Make work packages manually scheduled by default
Browse files Browse the repository at this point in the history
The `schedule_manually` column is also non-nullable now.

This includes the following changes:

- Automatically scheduled parent dates are and `ignore_non_working_days`
  attributes are now always derived from children's values, even if the
  children are scheduled manually.

  It's more natural. Without that, adding a child to a work package
  would not change the parent's dates.

  As a consequence, the parent can start on a non-working day if one of
  its children is manually scheduled, ignores non-working days, and
  starts on a non-working day. That's why the parent's
  `ignore_non_working_days` attribute is now also derived from all its
  children regardless of the scheduling mode.

  If the parent is manually scheduled, its dates and it's ability to
  ignore non-working days will still be defined independently from its
  children.

- Fix tests broken by scheduling mode being manual by default.

  The tests had to be adapted to explicitly set scheduling mode to
  automatic for followers and parents, and sometimes even follower's
  children. Without it, work packages would not be rescheduled
  automatically.

- Replace schedule helpers with table helpers.

  Schedule helpers helped well, but table helpers are more flexible and
  support more column types.

- Add "days counting" and "scheduling mode" columns to table helpers.

  "days counting" to set `ignore_non_working_days` attribute.
    - "all days" value maps to `ignore_non_working_days: true`.
    - "working days" value maps to `ignore_non_working_days: false`.
  "scheduling mode" to set `schedule_manually` attribute.
    - "manual" value maps to `schedule_manually: true`.
    - "automatic" value maps to `schedule_manually: false`.
  • Loading branch information
cbliard committed Dec 10, 2024
1 parent 391d65b commit f4b2975
Show file tree
Hide file tree
Showing 46 changed files with 1,924 additions and 3,111 deletions.
21 changes: 15 additions & 6 deletions app/models/work_packages/scopes/for_scheduling.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ module ForScheduling
# statement works, is that a work package is considered to be scheduled
# manually if *all* of its descendants are scheduled manually.
#
# One notable exception is if one of the manually scheduled children is
# the origin work package of the rescheduling. In this case, the parent is
# also subject to reschedule as the origin work package dates may have
# changed.
#
# For example in case of the hierarchy:
# A and B <- hierarchy (C is parent of both A and B) - C <- hierarchy - D
# * A and B are work packages
Expand Down Expand Up @@ -157,21 +162,25 @@ def for_scheduling(work_packages)
def scheduling_paths_sql(work_packages)
values = work_packages.map do |wp|
::OpenProject::SqlSanitization
.sanitize "(:id, false, false)",
.sanitize "(:id, false, false, true)",
id: wp.id
end.join(", ")

<<~SQL.squish
to_schedule (id, manually) AS (
to_schedule (id, manually, hierarchy_up, origin) AS (
SELECT * FROM (VALUES#{values}) AS t(id, manually, hierarchy_up)
SELECT * FROM (VALUES#{values}) AS t(id, manually, hierarchy_up, origin)
UNION
SELECT
relations.from_id id,
(related_work_packages.schedule_manually OR COALESCE(descendants.manually, false)) manually,
relations.hierarchy_up
(related_work_packages.schedule_manually
OR (COALESCE(descendants.manually, false)
AND NOT (to_schedule.origin AND relations.hierarchy_up))
) manually,
relations.hierarchy_up,
false origin
FROM
to_schedule
JOIN LATERAL
Expand All @@ -196,7 +205,7 @@ def scheduling_paths_sql(work_packages)
FROM
work_package_hierarchies
WHERE
NOT to_schedule.manually
(NOT to_schedule.manually OR to_schedule.origin)
AND ((work_package_hierarchies.ancestor_id = to_schedule.id AND NOT to_schedule.hierarchy_up AND work_package_hierarchies.generations = 1)
OR (work_package_hierarchies.descendant_id = to_schedule.id AND work_package_hierarchies.generations > 0))
) relations ON relations.to_id = to_schedule.id
Expand Down
4 changes: 1 addition & 3 deletions app/services/work_packages/update_ancestors_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,7 @@ def modified_attributes_justify_derivation?(attributes)
end

def ignore_non_working_days_of_descendants(ancestor, loader)
children = loader
.children_of(ancestor)
.reject(&:schedule_manually)
children = loader.children_of(ancestor)

if children.any?
children.any?(&:ignore_non_working_days)
Expand Down
10 changes: 10 additions & 0 deletions db/migrate/20241120095318_update_scheduling_mode_and_lags.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
class UpdateSchedulingModeAndLags < ActiveRecord::Migration[7.1]
def up
change_column_default :work_packages, :schedule_manually, from: false, to: true
execute "UPDATE work_packages SET schedule_manually = false WHERE schedule_manually IS NULL"
change_column_null :work_packages, :schedule_manually, false

migration_job = WorkPackages::AutomaticMode::MigrateValuesJob
if Rails.env.development?
migration_job.perform_now
else
migration_job.perform_later
end
end

def down
change_column_default :work_packages, :schedule_manually, from: true, to: false
# Keep the not-null constraint when rolling back
change_column_null :work_packages, :schedule_manually, false
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
project:,
subject: "Parent work package",
assigned_to: other_user,
schedule_manually: false, # because parent of child_wp
start_date: Time.zone.today.beginning_of_week.next_occurring(:wednesday),
due_date: Time.zone.today.beginning_of_week.next_occurring(:thursday),
derived_start_date: Time.zone.today.beginning_of_week.next_occurring(:wednesday),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
require "spec_helper"
require_relative "shared_context"

RSpec.describe "Team planner drag&dop and resizing",
RSpec.describe "Team planner drag&drop and resizing",
:js,
with_ee: %i[team_planner_view],
with_settings: { start_of_week: 1 } do
Expand All @@ -45,6 +45,7 @@
create(:work_package,
project:,
assigned_to: other_user,
schedule_manually: false, # because parent of second_wp
start_date: Time.zone.today.beginning_of_week.next_occurring(:tuesday),
due_date: Time.zone.today.beginning_of_week.next_occurring(:thursday))
end
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/work_packages/bulk_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@
end

let(:new_parent) do
create(:work_package, project: project1)
create(:work_package, schedule_manually: false, project: project1)
end

before do
Expand Down
32 changes: 17 additions & 15 deletions spec/features/admin/working_days_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@
shared_let(:week_days) { week_with_saturday_and_sunday_as_weekend }
shared_let(:admin) { create(:admin) }

let_schedule(<<~CHART)
days | MTWTFSSmtwtfss |
earliest_work_package | XXXXX |
second_work_package | XX..XX |
follower | XXX | follows earliest_work_package, follows second_work_package
CHART
let_work_packages(<<~TABLE)
subject | MTWTFSSmtwtfss | scheduling mode | properties
earliest_work_package | XXXXX | manual |
second_work_package | XX..XX | manual |
follower | XXX | automatic | follows earliest_work_package, follows second_work_package
TABLE

let(:dialog) { Components::ConfirmationDialog.new }
let(:datepicker) { Components::DatepickerModal.new }
Expand All @@ -48,6 +48,8 @@

before do
visit admin_settings_working_days_and_hours_path
# wait for "holidays and closures" calendar to load
find(".fc-next-button")
end

describe "week days" do
Expand Down Expand Up @@ -81,12 +83,12 @@ def working_days_setting

expect(working_days_setting).to eq([1, 2, 3, 4, 5])

expect_schedule(WorkPackage.all, <<~CHART)
days | MTWTFSSmtwtfss |
expect_work_packages(WorkPackage.all, <<~TABLE)
subject | MTWTFSSmtwtfss |
earliest_work_package | XXXXX |
second_work_package | XX..XX |
follower | XXX |
CHART
TABLE
end

it "updates the values and saves the settings" do
Expand All @@ -112,12 +114,12 @@ def working_days_setting

expect(working_days_setting).to eq([2, 3, 4])

expect_schedule(WorkPackage.all, <<~CHART)
days | MTWTFSSmtwtfssmtwt |
expect_work_packages(WorkPackage.all, <<~TABLE)
subject | MTWTFSSmtwtfssmtwt |
earliest_work_package | XXX....XX |
second_work_package | X....XXX |
follower | XXX |
CHART
TABLE

# The updated work packages will have a journal entry informing about the change
wp_page = Pages::FullWorkPackage.new(earliest_work_package)
Expand Down Expand Up @@ -152,12 +154,12 @@ def working_days_setting
expect(page).to have_unchecked_field "Sunday"
expect(working_days_setting).to eq([1, 2, 3, 4, 5])

expect_schedule(WorkPackage.all, <<~CHART)
days | MTWTFSSmtwtfss |
expect_work_packages(WorkPackage.all, <<~TABLE)
subject | MTWTFSSmtwtfss |
earliest_work_package | XXXXX |
second_work_package | XX..XX |
follower | XXX |
CHART
TABLE
end

it "shows an error when a previous change to the working days configuration isn't processed yet" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@
end

context "if the follower is a task" do
let!(:follower) { create(:work_package, type:, project:) }
let!(:follower) { create(:work_package, type:, project:, schedule_manually: false) }
let!(:relation) { create(:follows_relation, from: follower, to: predecessor) }
let(:date_field) { work_packages_page.edit_field(:combinedDate) }

it_behaves_like "keeps the minimum date from the predecessor when toggling NWD"
end

context "if the follower is a milestone" do
let!(:follower) { create(:work_package, type: milestone_type, project:) }
let!(:follower) { create(:work_package, type: milestone_type, project:, schedule_manually: false) }
let!(:relation) { create(:follows_relation, from: follower, to: predecessor) }
let(:date_field) { work_packages_page.edit_field(:date) }

Expand Down
47 changes: 38 additions & 9 deletions spec/features/work_packages/datepicker/datepicker_parent_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
datepicker.expect_visible
end

context "with the child having set dates" do
context "with the child having set dates and the parent being scheduled automatically" do
let(:child_attributes) do
{
start_date: "2021-02-01",
Expand All @@ -71,17 +71,46 @@
}
end

it "disables the non working days options" do
datepicker.expect_ignore_non_working_days_disabled
datepicker.expect_scheduling_mode false
context "when the parent is scheduled automatically" do
let(:parent_attributes) do
{
schedule_manually: false
}
end

first_monday = Time.zone.today.beginning_of_month.next_occurring(:monday)
datepicker.expect_disabled(first_monday)
it "disables the non-working days options" do
datepicker.expect_ignore_non_working_days_disabled
datepicker.expect_automatic_scheduling_mode

datepicker.toggle_scheduling_mode
datepicker.expect_scheduling_mode true
first_monday = Time.zone.today.beginning_of_month.next_occurring(:monday)
datepicker.expect_disabled(first_monday)

datepicker.expect_not_disabled(first_monday)
datepicker.toggle_scheduling_mode
datepicker.expect_manual_scheduling_mode

datepicker.expect_not_disabled(first_monday)
end
end

context "when the parent is scheduled manually" do
let(:parent_attributes) do
{
schedule_manually: true
}
end

it "enables the non-working days options" do
datepicker.expect_ignore_non_working_days_enabled
datepicker.expect_manual_scheduling_mode

first_monday = Time.zone.today.beginning_of_month.next_occurring(:monday)
datepicker.expect_not_disabled(first_monday)

datepicker.toggle_scheduling_mode
datepicker.expect_automatic_scheduling_mode

datepicker.expect_disabled(first_monday)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,22 @@
let!(:wp) do
create(:work_package,
project:,
schedule_manually: false, # because parent of wp_child and follows wp_pre
start_date: Date.parse("2016-01-01"),
due_date: Date.parse("2016-01-05"),
parent: wp_parent)
end
let!(:wp_parent) do
create(:work_package,
project:,
schedule_manually: false, # because parent of wp
start_date: Date.parse("2016-01-01"),
due_date: Date.parse("2016-01-05"))
end
let!(:wp_child) do
create(:work_package,
project:,
schedule_manually: false, # because needed to have rescheduling working
start_date: Date.parse("2016-01-01"),
due_date: Date.parse("2016-01-05"),
parent: wp)
Expand All @@ -80,6 +83,7 @@
let!(:wp_suc) do
create(:work_package,
project:,
schedule_manually: false, # because parent of wp_suc_child and follows wp
start_date: Date.parse("2016-01-06"),
due_date: Date.parse("2016-01-10"),
parent: wp_suc_parent).tap do |suc|
Expand All @@ -89,12 +93,14 @@
let!(:wp_suc_parent) do
create(:work_package,
project:,
schedule_manually: false, # because parent of wp_suc
start_date: Date.parse("2016-01-06"),
due_date: Date.parse("2016-01-10"))
end
let!(:wp_suc_child) do
create(:work_package,
project:,
schedule_manually: false, # because needed to have rescheduling working
start_date: Date.parse("2016-01-06"),
due_date: Date.parse("2016-01-10"),
parent: wp_suc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
create(:work_package,
project:,
type:,
subject: "Parent")
subject: "Parent",
schedule_manually: false)
end

let!(:child) do
Expand Down Expand Up @@ -117,8 +118,4 @@
expect(parent.due_date.iso8601).to eq("2020-07-25")
end
end

context "with a user allowed to view only" do
let(:role) { create(:project_role, permissions: %i[view_work_packages]) }
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,10 @@
end

context "when scheduled automatically" do
before do
work_package.schedule_manually = false
end

it "is not writable" do
expect(subject).not_to be_writable(:start_date)
end
Expand Down Expand Up @@ -246,6 +250,10 @@
end

context "when scheduled automatically" do
before do
work_package.schedule_manually = false
end

it "is not writable" do
expect(subject).not_to be_writable(:due_date)
end
Expand Down Expand Up @@ -277,11 +285,18 @@
allow(work_package.type).to receive(:is_milestone?).and_return(true)
end

it "is not writable when the work package is a parent" do
it "is not writable when the work package is a parent scheduled automatically" do
allow(work_package).to receive(:leaf?).and_return(false)
work_package.schedule_manually = false
expect(subject).not_to be_writable(:date)
end

it "is writable when the work package is a parent scheduled manually" do
allow(work_package).to receive(:leaf?).and_return(false)
work_package.schedule_manually = true
expect(subject).to be_writable(:date)
end

it "is writable when the work package is a leaf" do
allow(work_package).to receive(:leaf?).and_return(true)
expect(subject).to be_writable(:date)
Expand Down
Loading

0 comments on commit f4b2975

Please sign in to comment.