Merge pull request #19297 from opf/bugfix/65062-systemstackerror-in-workpackage-schedulingrules-schedule_automatically

[65062] Fix infinite loop when getting automatically scheduled ancestors

app/contracts/work_packages/base_contract.rb

@@ -307,11 +307,22 @@ module WorkPackages
       if model.parent_id_changed? &&
          model.parent_id &&
          errors.exclude?(:parent) &&
-         WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty?
+         current_parent_unrelatable?
+        # closure_tree adds an error on :parent_id because of the cycle
+        # detection, and active_record sees the error when saving the children
+        # association and adds an error on :children as well. We need to remove
+        # them.
+        errors.delete(:parent_id) # remove the error added by closure_tree
+        errors.delete(:children) # remove the error added by active_record
+        # add our own error
         errors.add :parent, :cant_link_a_work_package_with_a_descendant
       end
     end
 
+    def current_parent_unrelatable?
+      WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty?
+    end
+
     def validate_status_exists
       errors.add :status, :does_not_exist if model.status && !status_exists?
     end

app/services/work_packages/schedule_dependency.rb

@@ -102,13 +102,21 @@ class WorkPackages::ScheduleDependency
   def automatically_scheduled_ancestors(work_package)
     @automatically_scheduled_ancestors ||= {}
     @automatically_scheduled_ancestors[work_package] ||= begin
-      parent = parent_of(work_package)
+      work_packages_to_process = [work_package]
+      result = []
+      processed_ids = Set.new
 
-      if parent&.schedule_automatically?
-        [parent, *automatically_scheduled_ancestors(parent)]
-      else
-        []
+      while current = work_packages_to_process.shift
+        processed_ids.add(current.id)
+
+        parent = parent_of(current)
+
+        if parent&.schedule_automatically?
+          result << parent unless parent.id == work_package.id
+          work_packages_to_process << parent unless processed_ids.include?(parent.id)
+        end
       end
+      result
     end
   end
 

spec/services/work_packages/schedule_dependency_spec.rb

@@ -75,4 +75,117 @@ RSpec.describe WorkPackages::ScheduleDependency do
       end
     end
   end
+
+  describe "#automatically_scheduled_ancestors" do
+    shared_let(:work_package) { create(:work_package, subject: "work_package") }
+    let(:schedule_dependency) { described_class.new(work_package) }
+
+    context "with no parent" do
+      it "returns an empty array" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty
+      end
+    end
+
+    context "with a single automatically scheduled parent" do
+      let_work_packages(<<~TABLE)
+        hierarchy | scheduling mode
+        parent    | automatic
+      TABLE
+
+      before do
+        work_package.update(parent:)
+      end
+
+      it "returns the parent" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent)
+      end
+    end
+
+    context "with a manually scheduled parent" do
+      let_work_packages(<<~TABLE)
+        hierarchy | scheduling mode
+        parent    | manual
+      TABLE
+
+      before do
+        work_package.update(parent:)
+      end
+
+      it "returns an empty array" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty
+      end
+    end
+
+    context "with multiple levels of automatically scheduled ancestors" do
+      let_work_packages(<<~TABLE)
+        hierarchy   | scheduling mode
+        grandparent | automatic
+          parent    | automatic
+      TABLE
+
+      before do
+        work_package.update(parent:)
+      end
+
+      it "returns all automatically scheduled ancestors" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent, grandparent)
+      end
+    end
+
+    context "with mixed scheduling modes in the hierarchy" do
+      let_work_packages(<<~TABLE)
+        hierarchy   | scheduling mode
+        grandparent | automatic
+          parent    | manual
+      TABLE
+
+      before do
+        work_package.update(parent:)
+      end
+
+      it "returns only automatically scheduled ancestors" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty
+      end
+    end
+
+    context "with a cycle in the hierarchy" do
+      let_work_packages(<<~TABLE)
+        hierarchy    | scheduling mode
+        child        | automatic
+          grandchild | manual
+      TABLE
+
+      before do
+        child.update(parent: work_package)
+        work_package.update(schedule_manually: false)
+
+        # Create the cycle: set the parent with a current child, but do not
+        # save, like when set by an update during a set attributes service call
+        work_package.parent = child
+      end
+
+      it "handles the cycle gracefully and does not cause an infinite loop" do
+        expect { schedule_dependency.automatically_scheduled_ancestors(work_package) }.not_to raise_error
+        # Should return the parent but not get stuck in an infinite loop
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(child)
+      end
+    end
+
+    context "with a complex hierarchy" do
+      let_work_packages(<<~TABLE)
+        hierarchy         | scheduling mode
+        great_grandparent | automatic
+          grandparent     | manual
+            parent        | automatic
+      TABLE
+
+      before do
+        work_package.update(parent:)
+      end
+
+      it "returns only automatically scheduled ancestors" do
+        expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent)
+      end
+    end
+  end
 end

spec/services/work_packages/update_service_integration_spec.rb

@@ -64,7 +64,7 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do
     set_factory_default(:user, user)
   end
 
-  shared_let(:work_package, refind: true, reload: false) do
+  let(:work_package) do
     create(:work_package,
            subject: "work_package")
   end
@@ -1892,9 +1892,33 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do
       }
     end
 
-    # Bug #63499: this was causing an infinite loop when computing the future
+    # Bug #64973: this was causing an infinite loop when computing the future
     # dates of the predecessor.
-    it "displays an error about the inability to have multiple relations between the same work packages (Bug #63499)" do
+    it "displays an error about the inability to have multiple relations between the same work packages (Bug #64973)" do
+      expect(subject).to be_failure
+
+      expect(subject.errors.attribute_names).to contain_exactly(:parent)
+      # the error message in this case is far from ideal
+      expect(subject.errors.details).to include(parent: [{ error: :cant_link_a_work_package_with_a_descendant }])
+    end
+  end
+
+  context "when a work package with a child and a grandchild is made a child of its child" do
+    let_work_packages(<<~TABLE)
+      hierarchy      | scheduling mode
+      work_package   | automatic
+        child        | automatic
+          grandchild | manual
+    TABLE
+    let(:attributes) do
+      {
+        parent: child
+      }
+    end
+
+    # Bug #65062: this was causing an infinite loop when computing automatically
+    # scheduled ancestors of the updated work package.
+    it "displays an error about the inability to have multiple relations between the same work packages (Bug #65062)" do
       expect(subject).to be_failure
 
       expect(subject.errors.attribute_names).to contain_exactly(:parent)