use new model to handle updating of recurring todos

app/controllers/recurring_todos_controller.rb

@@ -3,7 +3,7 @@ class RecurringTodosController < ApplicationController
   helper :todos, :recurring_todos
 
   append_before_filter :init, :only => [:index, :new, :edit, :create]
-  append_before_filter :get_recurring_todo_from_param, :only => [:destroy, :toggle_check, :toggle_star, :edit, :update]
+  append_before_filter :get_recurring_todo_from_param, :only => [:destroy, :toggle_check, :toggle_star, :edit]
 
   def index
     @page_title = t('todos.recurring_actions_title')
@@ -43,62 +43,15 @@ class RecurringTodosController < ApplicationController
   end
 
   def update
-    # TODO: write tests for updating
+    @recurring_todo = current_user.recurring_todos.find(params[:id])
-    @recurring_todo.tag_with(params[:edit_recurring_todo_tag_list]) if params[:edit_recurring_todo_tag_list]
+
     @original_item_context_id = @recurring_todo.context_id
     @original_item_project_id = @recurring_todo.project_id
 
-    # we needed to rename the recurring_period selector in the edit form because
+    updater = RecurringTodos::RecurringTodosBuilder.new(current_user, edit_recurring_todo_params)
-    # the form for a new recurring todo and the edit form are on the same page.
-    # Same goes for start_from and end_date
-    params['recurring_todo']['recurring_period']=params['recurring_edit_todo']['recurring_period']
-    params['recurring_todo']['end_date']=parse_date_per_user_prefs(params['recurring_todo_edit_end_date'])
-    params['recurring_todo']['start_from']=parse_date_per_user_prefs(params['recurring_todo_edit_start_from'])
 
-    # update project
+    @saved = updater.update(@recurring_todo)
-    if params['recurring_todo']['project_id'].blank? && !params['project_name'].nil?
+    @recurring_todo.reload
-      if params['project_name'] == 'None'
-        project = Project.null_object
-      else
-        project = current_user.projects.where(:name => params['project_name'].strip)
-        unless project
-          project = current_user.projects.build
-          project.name = params['project_name'].strip
-          project.save
-          @new_project_created = true
-        end
-      end
-      params["recurring_todo"]["project_id"] = project.id
-    end
-
-    # update context
-    if params['recurring_todo']['context_id'].blank? && params['context_name'].present?
-      context = current_user.contexts.where(:name => params['context_name'].strip).first
-      unless context
-        context = current_user.contexts.build
-        context.name = params['context_name'].strip
-        context.save
-        @new_context_created = true
-      end
-      params["recurring_todo"]["context_id"] = context.id
-    end
-
-    # make sure that we set weekly_return_xxx to empty (space) when they are
-    # not checked (and thus not present in params["recurring_todo"])
-    %w{monday tuesday wednesday thursday friday saturday sunday}.each do |day|
-      params["recurring_todo"]["weekly_return_"+day]=' ' if params["recurring_todo"]["weekly_return_"+day].nil?
-    end
-
-    selector_attributes = {
-        'recurring_period' => recurring_todo_params['recurring_period'],
-        'daily_selector' => recurring_todo_params['daily_selector'],
-        'monthly_selector' => recurring_todo_params['monthly_selector'],
-        'yearly_selector' => recurring_todo_params['yearly_selector']
-      }
-
-    @recurring_todo.assign_attributes(:recurring_period => recurring_todo_params[:recurring_period])
-    @recurring_todo.assign_attributes(selector_attributes)
-    @saved = @recurring_todo.update_attributes recurring_todo_params
 
     respond_to do |format|
       format.js
@@ -108,16 +61,15 @@ class RecurringTodosController < ApplicationController
   def create
     builder = RecurringTodos::RecurringTodosBuilder.new(current_user, all_recurring_todo_params)
     @saved = builder.save
-    @recurring_todo = builder.saved_recurring_todo
 
     if @saved
-      @status_message = t('todos.recurring_action_saved')
+      @recurring_todo = builder.saved_recurring_todo
       @todo_saved = TodoFromRecurringTodo.new(current_user, @recurring_todo).create.nil? == false
-      if @todo_saved
+
-        @status_message += " / " + t('todos.new_related_todo_created_short')
+      @status_message = 
-      else
+        t('todos.recurring_action_saved') + " / " + 
-        @status_message += " / " + t('todos.new_related_todo_not_created_short')
+        t("todos.new_related_todo_#{@todo_saved ? "" : "not_"}created_short")
-      end
+
       @down_count = current_user.recurring_todos.active.count
       @new_recurring_todo = RecurringTodo.new
     else
@@ -219,12 +171,38 @@ class RecurringTodosController < ApplicationController
 
   def all_recurring_todo_params    
     # move context_name, project_name and tag_list into :recurring_todo hash for easier processing
-    params[:recurring_todo][:context_name] = params[:context_name] unless params[:context_name].blank?
+    { context_name: :context_name, project_name: :project_name, tag_list: :tag_list}.each do |target,source|
-    params[:recurring_todo][:project_name] = params[:project_name] unless params[:project_name].blank?
+      move_into_recurring_todo_param(params, target, source)
-    params[:recurring_todo][:tag_list] =     params[:tag_list]     unless params[:tag_list].blank?
+    end
     recurring_todo_params
   end
 
+  def edit_recurring_todo_params
+    # we needed to rename the recurring_period selector in the edit form because
+    # the form for a new recurring todo and the edit form are on the same page.
+    # Same goes for start_from and end_date
+    params['recurring_todo']['recurring_period'] = params['recurring_edit_todo']['recurring_period']
+
+    { context_name: :context_name, 
+      project_name: :project_name, 
+      tag_list:     :edit_recurring_todo_tag_list,
+      end_date:     :recurring_todo_edit_end_date,
+      start_from:   :recurring_todo_edit_start_from}.each do |target,source|
+      move_into_recurring_todo_param(params, target, source)
+    end    
+
+    # make sure that we set weekly_return_xxx to empty (space) when they are
+    # not checked (and thus not present in params["recurring_todo"])
+    %w{monday tuesday wednesday thursday friday saturday sunday}.each do |day|
+      params["recurring_todo"]["weekly_return_#{day}"]=' ' if params["recurring_todo"]["weekly_return_#{day}"].nil?
+    end
+    params['recurring_todo']
+  end
+
+  def move_into_recurring_todo_param(params, target, source)
+    params[:recurring_todo][target] = params[source] unless params[source].blank?
+  end
+
   def init
     @days_of_week = []
     0.upto 6 do |i|

app/models/recurring_todos/abstract_recurring_todos_builder.rb

@@ -5,16 +5,11 @@ module RecurringTodos
     def initialize(user, attributes)
       @user = user
       @attributes = attributes
-      @filterred_attributes = filter_attributes(attributes)
+      @filterred_attributes = filter_attributes(@attributes)
       @saved = false
     end
 
-    def filter_attributes(attributes)
-      raise Exception.new, "filter_attributes should be overridden"
-    end
-
     def filter_generic_attributes(attributes)
-      attributes['tag_list'] = 
       {
         recurring_period: attributes["recurring_period"],
         description:      attributes['description'], 
@@ -41,6 +36,16 @@ module RecurringTodos
       @recurring_todo.project = @filterred_attributes[:project]
     end
 
+    def update(recurring_todo)
+      @recurring_todo = @pattern.update_recurring_todo(recurring_todo)
+      @recurring_todo.context = @filterred_attributes[:context]
+      @recurring_todo.project = @filterred_attributes[:project]
+      
+      @saved = @recurring_todo.save
+      @recurring_todo.tag_with(@filterred_attributes[:tag_list]) if @saved && @filterred_attributes[:tag_list].present?
+      return @saved
+    end
+
     def save
       build
       @saved = @recurring_todo.save
@@ -60,6 +65,16 @@ module RecurringTodos
       @pattern.attributes
     end
 
+    def attributes_to_filter
+      raise Exception.new, "attributes_to_filter should be overridden"
+    end
+
+    def filter_attributes(attributes)
+      @filterred_attributes = filter_generic_attributes(attributes)
+      attributes_to_filter.each{|key| @filterred_attributes[key] = attributes[key] if attributes.key?(key)}
+      @filterred_attributes
+    end
+
     private
 
     def tag_list_or_empty_string(attributes)

app/models/recurring_todos/abstract_repeat_pattern.rb

@@ -3,15 +3,22 @@ module RecurringTodos
   class AbstractRepeatPattern
 
     def initialize(user, attributes)
-      @attributes = attributes
       @user = user
+      @attributes = attributes
+      @filterred_attributes = nil
     end
 
     def build_recurring_todo
       @recurring_todo = @user.recurring_todos.build(mapped_attributes)
     end
 
+    def update_recurring_todo(recurring_todo)
+      recurring_todo.assign_attributes(mapped_attributes)
+      recurring_todo
+    end
+
     def mapped_attributes
+      # should be overwritten to map attributes to activerecord model
       @attributes
     end
 

app/models/recurring_todos/daily_recurring_todos_builder.rb

@@ -5,13 +5,12 @@ module RecurringTodos
 
     def initialize(user, attributes)
       super(user, attributes)
+
       @pattern = DailyRepeatPattern.new(user, @filterred_attributes)
     end
 
-    def filter_attributes(attributes)
+    def attributes_to_filter
-      @filterred_attributes = filter_generic_attributes(attributes)
+      %w{daily_selector daily_every_x_days}
-      %w{daily_selector daily_every_x_days}.each{|key| @filterred_attributes[key] = attributes[key] if attributes.key?(key)}
-      @filterred_attributes
     end
 
   end

app/models/recurring_todos/monthly_recurring_todos_builder.rb

@@ -7,17 +7,11 @@ module RecurringTodos
       @pattern = MonthlyRepeatPattern.new(user, @filterred_attributes)
     end
 
-    def filter_attributes(attributes)
+    def attributes_to_filter
-      @filterred_attributes = filter_generic_attributes(attributes)
-
       %w{
         monthly_selector       monthly_every_x_day   monthly_every_x_month 
         monthly_every_x_month2 monthly_every_xth_day monthly_day_of_week    
-      }.each do |key| 
+      }
-        @filterred_attributes[key] = attributes[key] if attributes.key?(key)
-      end
-      
-      @filterred_attributes
     end
 
   end

app/models/recurring_todos/recurring_todos_builder.rb

@@ -19,7 +19,7 @@ module RecurringTodos
       if %w{daily weekly monthly yearly}.include?(selector)
         return eval("RecurringTodos::#{selector.capitalize}RecurringTodosBuilder.new(@user, @attributes)")
       else
-        raise Exception.new("Unknown recurrence selector (#{selector})")
+        raise Exception.new("Unknown recurrence selector in recurring_period (#{selector})")
       end
     end
 
@@ -27,6 +27,10 @@ module RecurringTodos
       @builder.build
     end
 
+    def update(recurring_todo)
+      @builder.update(recurring_todo)
+    end
+
     def save
       @project.save if @new_project_created
       @context.save if @new_context_created

app/models/recurring_todos/weekly_recurring_todos_builder.rb

@@ -7,16 +7,11 @@ module RecurringTodos
       @pattern = WeeklyRepeatPattern.new(user, @filterred_attributes)
     end
 
-    def filter_attributes(attributes)
+    def attributes_to_filter
-      @filterred_attributes = filter_generic_attributes(attributes)
+      %w{weekly_selector weekly_every_x_week} + %w{monday tuesday wednesday thursday friday saturday sunday}.map{|day| "weekly_return_#{day}" }
-
-      weekly_attributes = %w{weekly_selector weekly_every_x_week}
-      %w{monday tuesday wednesday thursday friday saturday sunday}.each{|day| weekly_attributes << "weekly_return_#{day}"}
-      weekly_attributes.each{|key| @filterred_attributes[key] = attributes[key]  if attributes.key?(key)}
-
-      @filterred_attributes
     end
 
+
   end
 
 end

app/models/recurring_todos/yearly_recurring_todos_builder.rb

@@ -7,16 +7,10 @@ module RecurringTodos
       @pattern = YearlyRepeatPattern.new(user, @filterred_attributes)
     end
 
-    def filter_attributes(attributes)
+    def attributes_to_filter
-      @filterred_attributes = filter_generic_attributes(attributes)
-
       %w{ yearly_selector     yearly_month_of_year  yearly_month_of_year2 
           yearly_every_x_day  yearly_every_xth_day  yearly_day_of_week    
-      }.each do |key| 
+      }
-        @filterred_attributes[key] = attributes[key] if attributes.key?(key)
-      end
-      
-      @filterred_attributes
     end
 
   end

test/controllers/recurring_todos_controller_test.rb

@@ -359,4 +359,30 @@ class RecurringTodosControllerTest < ActionController::TestCase
     assert_equal "completed", rt.state, "repeat pattern should be completed"
   end
 
+  def test_update_recurring_todo
+    login_as(:admin_user)
+    rt = recurring_todos(:call_bill_gates_every_day)
+    current_descr = rt.description
+
+    put :update, 
+      "recurring_todo" => { 
+        "description"        => "changed", 
+        "daily_selector"     => "daily_every_x_day",
+        "daily_every_x_days" => "2",
+        "ends_on"            => "no_end_date",
+        "recurring_target"   => "show_from_date"
+        },
+      "recurring_edit_todo"  => {
+        "recurring_period"   => rt.recurring_period,
+      },
+      "recurring_todo_edit_start_from" => "2/1/2013",
+      "end_date"                       => nil,
+      "ends_on"                        => "no_end_date",
+      "id"                             => "#{rt.id}", 
+      "context_name"                   => "library",
+      format: :js
+
+    assert_equal "changed", rt.reload.description
+  end
+
 end

test/models/recurring_todos/abstract_recurring_todos_builder_test.rb

@@ -68,7 +68,7 @@ module RecurringTodos
       assert_equal "", builder.attributes[:tag_list]
     end
 
-    def test_tags_should_be_saved
+    def test_tags_should_be_saved_on_create_and_update
       attributes = {
         'recurring_period'  => "daily",
         'description'       => "test",
@@ -91,6 +91,14 @@ module RecurringTodos
       assert !builder.tag_list.present?, "tag list should not be present"
       assert builder.save, "it should be saved"
       assert_equal "", builder.saved_recurring_todo.tag_list, "tag list should be empty"
+
+      # tags should be updated
+      rt = builder.saved_recurring_todo
+      attributes['tag_list'] = "bar, foo"
+      updater = RecurringTodosBuilder.new(@admin, attributes)
+      updater.update(rt)
+      rt.reload
+      assert_equal "bar, foo", rt.tag_list
     end
 
 

test/models/recurring_todos/recurring_todos_builder_test.rb

@@ -102,7 +102,7 @@ module RecurringTodos
     end
 
     def test_project_is_optional
-      builder = RecurringTodosBuilder.new(@admin, {
+      attributes = {
         'recurring_period' => "daily",
         'description'      => "test",
         'context_name'     => "my new context",
@@ -110,13 +110,42 @@ module RecurringTodos
         'recurring_target' => 'show_from_date',
         'show_always'      => true,
         'start_from'       => '01/01/01',
-        'ends_on'          => 'no_end_date'})
+        'ends_on'          => 'no_end_date'}
+
+      builder = RecurringTodosBuilder.new(@admin, attributes)
 
       assert_nil builder.project, "project should not exist"
       builder.save
       assert_nil builder.saved_recurring_todo.project
     end
 
+    def test_builder_can_update_description
+      attributes = {
+        'recurring_period' => "daily",
+        'description'      => "test",
+        'context_name'     => "my new context",
+        'daily_selector'   => 'daily_every_work_day', 
+        'recurring_target' => 'show_from_date',
+        'show_always'      => true,
+        'start_from'       => '01/01/01',
+        'ends_on'          => 'no_end_date'}
+
+      builder = RecurringTodosBuilder.new(@admin, attributes)
+      builder.save
+      rt = builder.saved_recurring_todo
+
+      assert_equal "test", rt.description
+
+      attributes['description'] = 'updated'
+
+      updater = RecurringTodosBuilder.new(@admin, attributes)
+      updater.update(rt)
+      rt.reload
+
+      assert_equal rt.id, builder.saved_recurring_todo.id
+      assert_equal "updated", rt.description
+    end
+
   end
 
 end

test/models/recurring_todos/weekly_recurring_todos_builder_test.rb

@@ -30,6 +30,19 @@ module RecurringTodos
       assert_equal "a repeating todo",  result[:description], "description should be preserved"
     end
 
+    def test_attributes_to_filter
+      attributes = {
+        'recurring_period'     => 'weekly',
+        'description'          => 'a repeating todo',  # generic
+        'weekly_return_monday' => 'm',                 # weekly specific
+      }
+
+      w = WeeklyRecurringTodosBuilder.new(@admin, attributes)
+      assert_equal 9, w.attributes_to_filter.size
+      assert w.attributes_to_filter.include?('weekly_selector'), "attributes_to_filter should return static attribute weekly_selector"
+      assert w.attributes_to_filter.include?('weekly_return_monday'), "attributes_to_filter should return generated weekly_return_xyz"
+    end
+
   end
 
 end