From cc9746371cfc049dda97cd9ffcda41e5856ca176 Mon Sep 17 00:00:00 2001 From: Henrik Bohre Date: Tue, 30 Jun 2009 23:17:33 +0200 Subject: [PATCH] #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 | 43 +++++++-------- app/models/todo.rb | 81 ++++++++++++++++++++--------- 2 files changed, 78 insertions(+), 46 deletions(-) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 4ab166fc..26aabab2 100644 --- a/app/controllers/todos_controller.rb +++ b/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? diff --git a/app/models/todo.rb b/app/models/todo.rb index cd195cc8..621cd1b6 100644 --- a/app/models/todo.rb +++ b/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