#300: First shot at validation of dependencies

Implemented by deferring save of dependencies until after saving (and validating) the todo, as described by Andrew Timberlake on http://www.ruby-forum.com/topic/175552.

app/controllers/todos_controller.rb

@@ -71,6 +71,7 @@ class TodosController < ApplicationController
       @todo.context_id = context.id
     end
 
+    @todo.add_predecessor_list(predecessor_list)
     @todo.update_state_from_project
     @saved = @todo.save
     unless (@saved == false) || tag_list.blank?
@@ -78,13 +79,11 @@ class TodosController < ApplicationController
       @todo.tags.reload
     end
 
-    # TODO: Check if we can reload cache here instead of saving twice    
-    unless predecessor_list.blank?
-      @todo.add_predecessor_list(predecessor_list)
+    unless (@aved == false)
       unless @todo.uncompleted_predecessors.empty? || @todo.state == 'project_hidden'
         @todo.state = 'pending'
       end
-      @saved = @todo.save
+      @todo.save!
     end
 
     respond_to do |format|
@@ -239,23 +238,6 @@ class TodosController < ApplicationController
     @original_item_due = @todo.due
     @original_item_due_id = get_due_id_for_calendar(@todo.due)
     @original_item_predecessor_list = @todo.predecessors.collect{|t| t.description}.join(', ')
-    if params[:predecessor_list]
-      logger.debug "---old #{@original_item_predecessor_list}"
-      logger.debug "---new #{params[:predecessor_list]}"
-      @todo.add_predecessor_list(params[:predecessor_list])
-      if @original_item_predecessor_list != params[:predecessor_list]
-        # Recalculate dependencies for this todo
-        if @todo.uncompleted_predecessors.empty?
-          if @todo.state == 'pending'
-            @todo.activate! # Activate pending if no uncompleted predecessors
-          end
-        else
-          if @todo.state == 'active'
-            @todo.block! # Block active if we got uncompleted predecessors
-          end
-        end
-      end
-    end
     
     if params['todo']['project_id'].blank? && !params['project_name'].nil?
       if params['project_name'] == 'None'
@@ -304,7 +286,26 @@ class TodosController < ApplicationController
     end
     
     @todo.attributes = params["todo"]
+
+    @todo.add_predecessor_list(params[:predecessor_list])
     @saved = @todo.save
+    if @saved && params[:predecessor_list]
+      if @original_item_predecessor_list != params[:predecessor_list]
+        # Possible state change with new dependencies
+        if @todo.uncompleted_predecessors.empty?
+          if @todo.state == 'pending'
+            @todo.activate! # Activate pending if no uncompleted predecessors
+          end
+        else
+          if @todo.state == 'active'
+            @todo.block! # Block active if we got uncompleted predecessors
+          end
+        end
+      end
+      @todo.save!
+    end
+
+
 
     @context_changed = @original_item_context_id != @todo.context_id
     @todo_was_activated_from_deferred_state = @original_item_was_deferred && @todo.active?

app/models/todo.rb

@@ -14,6 +14,7 @@ class Todo < ActiveRecord::Base
   has_many :pending_successors, :through => :predecessor_dependencies,
            :source => :successor, :conditions => ['state = ?', 'pending']
   
+  after_save :save_predecessors
 
   named_scope :active, :conditions => { :state => 'active' }
   named_scope :not_completed, :conditions =>  ['NOT (state = ? )', 'completed']
@@ -69,10 +70,63 @@ class Todo < ActiveRecord::Base
   validates_presence_of :show_from, :if => :deferred?
   validates_presence_of :context
   
+  def initialize(*args)
+    super(*args)
+    @predecessor_array = [] # Used for deferred save of predecessors
+  end
+  
+  # TODO: Handle duplicate descriptions
   def validate
     if !show_from.blank? && show_from < user.date
       errors.add("show_from", "must be a date in the future")
     end
+    # Validate predecessors array
+    @predecessor_array.each do |description|
+      t = Todo.find_by_description(description)
+      if t.nil?
+        errors.add("Depends on:", "Could not find action '#{description}'")
+      else
+        errors.add("Depends on:", "Adding '#{description}' would create a circular dependency") if is_successor?(t)
+      end
+    end
+  end
+  
+  def save_predecessors
+    current_array = predecessors.map(&:description)
+    @predecessor_array = [] if @predecessor_array.nil?
+    remove_array = current_array - @predecessor_array
+    add_array = @predecessor_array - current_array
+    
+    # This is probably a bit naive code...
+    remove_array.each do |description|
+      t = Todo.find_by_description(description)
+      self.predecessors.delete(t)
+    end
+    # ... as is this?
+    add_array.each do |description|
+      t = Todo.find_by_description(description)
+      unless t.nil?
+        self.predecessors << t unless self.predecessors.include?(t)
+      else
+        logger.error "Could not find #{description}" # Unexpected since validation passed
+      end
+    end
+  end
+  
+  # Returns true if t is equal to self or a successor of self
+  def is_successor?(t)
+    if self == t
+      return true
+    elsif self.successors.empty?
+      return false
+    else
+      self.successors.each do |item|
+        if item.is_successor?(t)
+          return true
+        end
+      end
+    end
+    return false
   end
 
   def update_state_from_project
@@ -162,36 +216,13 @@ class Todo < ActiveRecord::Base
   end
   
   # TODO: DELIMITER
-  # TODO: Todo::Error
   # TODO: Handle todos with the same description
+  # TODO: Should possibly handle state changes also?
   def add_predecessor_list(predecessor_list)
-    logger.debug "add_predecessor_list #{predecessor_list}"
     raise "Can't handle other types than string for now" unless predecessor_list.kind_of? String
-    list = predecessor_list.split(',').map do |description| 
+    @predecessor_array = predecessor_list.split(',').map do |description| 
       description.strip.squeeze(" ")            
     end
-    current = self.predecessors.map(&:description)
-    remove_list = current - list
-    # This is probably a bit naive code...
-    remove_list.each do |description|
-      t = Todo.find_by_description(description)
-      logger.debug "Removing #{t.description} from #{self.description} as predecessor"
-      self.predecessors.delete(t)
-    end
-    add_list = list - current
-    # ... as is this?
-    add_list.each do |description|
-      t = Todo.find_by_description(description)
-      #raise Todo::Error, "predecessor could not be found: #{description}" if t.nil?
-      # Create dependency record
-      unless t.nil?
-        self.predecessors << t unless self.predecessors.include?(t)
-      else
-        logger.error "Could not find #{description}"
-      end
-    end
-#    debugger
-    
   end
   
   # Rich Todo API