Merge pull request #1777 from C-Otto/circular-dependencies

fix #1754

app/controllers/todos_controller.rb

@@ -257,9 +257,13 @@ class TodosController < ApplicationController
     @todo = current_user.todos.includes(Todo::DEFAULT_INCLUDES).find(params['successor'])
     @original_state = @todo.state
     unless @predecessor.completed?
-      @todo.add_predecessor(@predecessor)
-      @todo.block! unless @todo.pending?
-      @saved = @todo.save
+      begin
+        @todo.add_predecessor(@predecessor)
+        @todo.block! unless @todo.pending?
+        @saved = @todo.save
+      rescue ActiveRecord::RecordInvalid
+        @saved = false
+      end
 
       @status_message = t('todos.added_dependency', :dependency => @predecessor.description)
       @status_message += t('todos.set_to_pending', :task => @todo.description) unless @original_state == 'pending'

app/models/dependency.rb

@@ -5,5 +5,13 @@ class Dependency < ActiveRecord::Base
   belongs_to :predecessor, :foreign_key => 'predecessor_id', :class_name => 'Todo', :touch => true
   belongs_to :successor,   :foreign_key => 'successor_id',   :class_name => 'Todo', :touch => true
 
+  validate :check_circular_dependencies
+
+  def check_circular_dependencies
+    unless predecessor.nil? or successor.nil?
+      errors.add("Depends on:", "Adding '#{successor.specification}' would create a circular dependency") if successor.is_successor?(predecessor)
+    end
+  end
+
 end
 

app/models/todo.rb

@@ -115,7 +115,6 @@ class Todo < ActiveRecord::Base
   validates_presence_of :show_from, :if => :deferred?
   validates_presence_of :context
   validate :check_show_from_in_future
-  validate :check_circular_dependencies
 
   def check_show_from_in_future
     if show_from_changed? # only check on change of show_from
@@ -125,14 +124,6 @@ class Todo < ActiveRecord::Base
     end
   end
 
-  def check_circular_dependencies
-    unless @predecessor_array.nil? # Only validate predecessors if they changed
-      @predecessor_array.each do |todo|
-        errors.add("Depends on:", "Adding '#{todo.specification}' would create a circular dependency") if is_successor?(todo)
-      end
-    end
-  end
-
   def initialize(*args)
     super(*args)
     @predecessor_array = nil # Used for deferred save of predecessors

test/models/todo_test.rb

@@ -93,10 +93,9 @@ class TodoTest < ActiveSupport::TestCase
     assert_equal 1, @not_completed3.successors.count
     
     # 1 -> 3 -> 2 -> 1 == circle
-    @not_completed3.add_predecessor(@not_completed1)
-    assert !@not_completed3.valid?
-    error_msg = "Adding ''Call Bill Gates to find out how much he makes per day' <'agenda'; 'Make more money than Billy Gates'>' would create a circular dependency"
-    assert_equal error_msg, @not_completed3.errors["Depends on:"][0]
+    assert_raises ActiveRecord::RecordInvalid do
+      @not_completed3.add_predecessor(@not_completed1)
+    end
   end
 
   def test_defer_an_existing_todo