Fix/add child rescheduling (#5499)

A work package that is to be added as a child needs to be rescheduled to not validate any constraints posed by relations the parent work package might have

[ci skip]

app/contracts/work_packages/base_contract.rb

@@ -86,6 +86,12 @@ module WorkPackages
       if !model.leaf? && model.changed.include?('start_date')
         errors.add :start_date, :error_readonly
       end
+      if model.start_date && model.parent && model.start_date < model.parent.soonest_start
+        message = I18n.t('activerecord.errors.models.work_package.attributes.start_date.violates_parent_relationships',
+                         soonest_start: Date.today + 4.days)
+
+        errors.add :start_date, message, error_symbol: :violates_parent_relationships
+      end
     end
 
     attribute :due_date do

app/models/work_package.rb

@@ -352,13 +352,14 @@ class WorkPackage < ActiveRecord::Base
   end
 
   def soonest_start
-    @soonest_start ||= (
+    @soonest_start ||=
       self_and_ancestors.includes(relations_to: :from)
                         .where(relations: { relation_type: Relation::TYPE_PRECEDES })
                         .map(&:relations_to)
                         .flatten
                         .map(&:successor_soonest_start)
-    ).compact.max
+                        .compact
+                        .max
   end
 
   # Users/groups the work_package can be assigned to
@@ -669,7 +670,7 @@ class WorkPackage < ActiveRecord::Base
 
   # Updates start/due dates of following work packages.
   # If
-  #   * there no start/due dates are set
+  #   * no start/due dates are set
   #     => no scheduling will happen.
   #   * a due date is set and the due date is moved backwards
   #     => following work package is moved backwards as well

app/services/schedule_work_package_service.rb

@@ -0,0 +1,80 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+# Currently this is only a stub.
+# The intend for this service is for it to include all the vast scheduling rules that make up the work package scheduling.
+
+class ScheduleWorkPackageService
+  include Concerns::Contracted
+
+  attr_accessor :user, :work_package
+
+  self.contract = WorkPackages::UpdateContract
+
+  def initialize(user:, work_package:)
+    self.user = user
+    self.work_package = work_package
+
+    self.contract = self.class.contract.new(work_package, user)
+  end
+
+  def call(attributes: {})
+    update(attributes)
+  end
+
+  private
+
+  def update(attributes)
+    set_dates_on_parent_updates unless attributes[:start_date]
+  end
+
+  def set_dates_on_parent_updates
+    return unless date_before_newly_added_parents_soonest_start?
+
+    new_start_date = work_package.parent.soonest_start
+
+    current_duration = work_package.duration
+
+    work_package.start_date = new_start_date
+    work_package.due_date = new_start_date + current_duration
+  end
+
+  def date_before_newly_added_parents_soonest_start?
+    work_package.parent_id_changed? &&
+      work_package.parent &&
+      date_before_soonest_start?(work_package.parent)
+  end
+
+  def date_before_soonest_start?(other_work_package)
+    other_work_package.soonest_start &&
+      (!work_package.start_date ||
+      work_package.start_date < other_work_package.soonest_start)
+  end
+end

app/services/update_work_package_service.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -72,6 +73,8 @@ class UpdateWorkPackageService
     work_package.attributes = attributes
 
     unify_dates if work_package_now_milestone?
+
+    reschedule(attributes)
   end
 
   def cleanup(attributes)
@@ -118,6 +121,12 @@ class UpdateWorkPackageService
     [true, work_package.errors]
   end
 
+  def reschedule(attributes)
+    ScheduleWorkPackageService
+      .new(user: user, work_package: work_package)
+      .call(attributes: attributes)
+  end
+
   def unify_dates
     unified_date = work_package.due_date || work_package.start_date
     work_package.start_date = work_package.due_date = unified_date

config/locales/en.yml

@@ -488,6 +488,7 @@ en:
               not_a_valid_parent: "is invalid."
             start_date:
               violates_relationships: "can only be set to %{soonest_start} or later so as not to violate the work package's relationships."
+              violates_parent_relationships: "can only be set to %{soonest_start} or later so as not to violate the relationships of the work_package's ancestors."
             status_id:
               status_transition_invalid: "is invalid because no valid transition exists from old to new status for the current user's roles."
             priority_id:

spec/contracts/work_packages/base_contract_spec.rb

@@ -36,9 +36,9 @@ describe WorkPackages::BaseContract do
                        project: project)
   end
   let(:member) { FactoryGirl.create(:user, member_in_project: project, member_through_role: role) }
-  let (:project) { FactoryGirl.create(:project) }
+  let(:project) { FactoryGirl.create(:project) }
   let(:current_user) { member }
-  let(:permissions) {
+  let(:permissions) do
     [
       :view_work_packages,
       :view_work_package_watchers,
@@ -48,7 +48,7 @@ describe WorkPackages::BaseContract do
       :manage_work_package_relations,
       :add_work_package_notes
     ]
-  }
+  end
   let(:role) { FactoryGirl.create :role, permissions: permissions }
   let(:changed_values) { [] }
 
@@ -129,6 +129,31 @@ describe WorkPackages::BaseContract do
 
   describe 'start date' do
     it_behaves_like 'a parent unwritable property', :start_date
+
+    context 'before soonest start date of parent' do
+      before do
+        work_package.parent = FactoryGirl.build_stubbed(:work_package)
+        allow(work_package.parent)
+          .to receive(:soonest_start)
+          .and_return(Date.today + 4.days)
+
+        work_package.start_date = Date.today + 2.days
+      end
+
+      it 'is invalid' do
+        expect(contract).not_to be_valid
+      end
+
+      it 'notes the error' do
+        contract.valid?
+
+        message = I18n.t('activerecord.errors.models.work_package.attributes.start_date.violates_parent_relationships',
+                         soonest_start: Date.today + 4.days)
+
+        expect(contract.errors[:start_date])
+          .to match_array [message]
+      end
+    end
   end
 
   describe 'due date' do

spec/models/work_package/work_package_scheduling_spec.rb

@@ -30,10 +30,10 @@ require 'spec_helper'
 
 describe WorkPackage, type: :model do
   describe '#overdue' do
-    let(:work_package) {
+    let(:work_package) do
       FactoryGirl.create(:work_package,
                          due_date: due_date)
-    }
+    end
 
     shared_examples_for 'overdue' do
       subject { work_package.overdue? }
@@ -73,24 +73,26 @@ describe WorkPackage, type: :model do
 
     context 'status closed' do
       let(:due_date) { 1.day.ago.to_date }
-      let(:status) {
+      let(:status) do
         FactoryGirl.create(:status,
                            is_closed: true)
-      }
+      end
 
-      before do work_package.status = status end
+      before do
+        work_package.status = status
+      end
 
       it_behaves_like 'on time'
     end
   end
 
   describe '#behind_schedule?' do
-    let(:work_package) {
+    let(:work_package) do
       FactoryGirl.create(:work_package,
                          start_date: start_date,
                          due_date: due_date,
                          done_ratio: done_ratio)
-    }
+    end
 
     shared_examples_for 'behind schedule' do
       subject { work_package.behind_schedule? }
@@ -148,18 +150,18 @@ describe WorkPackage, type: :model do
   describe 'rescheduling' do
     let(:work_package1_start) { Date.today }
     let(:work_package1_due) { Date.today + 3 }
-    let(:work_package1) {
+    let(:work_package1) do
       FactoryGirl.create(:work_package,
                          start_date: work_package1_start,
                          due_date: work_package1_due)
-    }
+    end
     let(:work_package2_start) { nil }
     let(:work_package2_due) { nil }
-    let(:work_package2) {
+    let(:work_package2) do
       FactoryGirl.create(:work_package,
                          start_date: work_package2_start,
                          due_date: work_package2_due)
-    }
+    end
 
     shared_examples_for 'scheduled work package' do
       before do
@@ -177,13 +179,13 @@ describe WorkPackage, type: :model do
 
     context 'for preceds/follows relationships' do
       let(:delay) { 0 }
-      let(:follows_relation) {
+      let(:follows_relation) do
         FactoryGirl.create(:relation,
                            relation_type: Relation::TYPE_PRECEDES,
                            from: work_package1,
                            to: work_package2,
                            delay: delay)
-      }
+      end
 
       before do
         follows_relation
@@ -389,17 +391,17 @@ describe WorkPackage, type: :model do
         end
 
         context 'when there is another work package also preceding the wp' do
-          let(:work_package3) {
+          let(:work_package3) do
             FactoryGirl.create(:work_package,
                                start_date: work_package1_start,
                                due_date: work_package1_due)
-          }
-          let(:follows_relation2) {
+          end
+          let(:follows_relation2) do
             FactoryGirl.create(:relation,
                                relation_type: Relation::TYPE_PRECEDES,
                                from: work_package3,
                                to: work_package2)
-          }
+          end
 
           before do
             follows_relation2
@@ -417,10 +419,10 @@ describe WorkPackage, type: :model do
             it_behaves_like 'scheduled work package' do
               # moved backwards as much as possible
               let(:expected_start) { work_package3.due_date + delay + 1 }
-              let(:expected_due) {
+              let(:expected_due) do
                 work_package3.due_date + delay + 1 +
                   (work_package3.due_date - work_package3.start_date)
-              }
+              end
             end
           end
         end
@@ -578,12 +580,12 @@ describe WorkPackage, type: :model do
      Relation::TYPE_DUPLICATES,
      Relation::TYPE_RELATES].each do |relation_type|
       context "for #{relation_type} relationships" do
-        let(:blocks_relation) {
+        let(:blocks_relation) do
           FactoryGirl.create(:relation,
                              relation_type: relation_type,
                              from: work_package1,
                              to: work_package2)
-        }
+        end
 
         context 'upon relationship generation' do
           before do

spec/services/update_work_package_service_spec.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -31,19 +32,19 @@ require 'spec_helper'
 
 describe UpdateWorkPackageService, type: :model do
   let(:user) { FactoryGirl.build_stubbed(:user) }
-  let(:project) {
+  let(:project) do
     p = FactoryGirl.build_stubbed(:project)
     allow(p).to receive(:shared_versions).and_return([])
 
     p
-  }
-  let(:work_package) {
+  end
+  let(:work_package) do
     wp = FactoryGirl.build_stubbed(:work_package, project: project)
     wp.type = FactoryGirl.build_stubbed(:type)
     wp.send(:clear_changes_information)
 
     wp
-  }
+  end
   let(:instance) do
     UpdateWorkPackageService.new(user: user,
                                  work_package: work_package)
@@ -169,9 +170,9 @@ describe UpdateWorkPackageService, type: :model do
     end
 
     context 'when switching the project' do
-      let(:target_project) {
+      let(:target_project) do
         FactoryGirl.build_stubbed(:project)
-      }
+      end
       let(:call_attributes) { attributes }
       let(:attributes) { { project: target_project } }
 
@@ -325,5 +326,61 @@ describe UpdateWorkPackageService, type: :model do
         end
       end
     end
+
+    context 'when setting a parent' do
+      let(:parent) { FactoryGirl.build_stubbed(:work_package) }
+
+      context "with the parent being restricted in it's ability to be moved" do
+        let(:soonest_date) { Date.today + 3.days }
+
+        before do
+          allow(parent)
+            .to receive(:soonest_start)
+            .and_return(soonest_date)
+        end
+
+        it 'sets the start date to the earliest possible date' do
+          instance.call(attributes: { parent: parent })
+
+          expect(work_package.start_date).to eql(Date.today + 3.days)
+        end
+      end
+
+      context 'with the parent being resticted but the attributes define a later date' do
+        let(:soonest_date) { Date.today + 3.days }
+
+        before do
+          allow(parent)
+            .to receive(:soonest_start)
+            .and_return(soonest_date)
+        end
+
+        it 'sets the dates to provided dates' do
+          instance.call(attributes: { parent: parent, start_date: Date.today + 4.days, due_date: Date.today + 5.days })
+
+          expect(work_package.start_date).to eql(Date.today + 4.days)
+          expect(work_package.due_date).to eql(Date.today + 5.days)
+        end
+      end
+
+      context 'with the parent being resticted but the attributes define an earlier date' do
+        let(:soonest_date) { Date.today + 3.days }
+
+        before do
+          allow(parent)
+            .to receive(:soonest_start)
+            .and_return(soonest_date)
+        end
+
+        # This would be invalid but the dates should be set nevertheless
+        # so we can have a correct error handling.
+        it 'sets the dates to provided dates' do
+          instance.call(attributes: { parent: parent, start_date: Date.today + 1.days, due_date: Date.today + 2.days })
+
+          expect(work_package.start_date).to eql(Date.today + 1.days)
+          expect(work_package.due_date).to eql(Date.today + 2.days)
+        end
+      end
+    end
   end
 end