fix several issues with recurring todos

* fix case where some fields were not saved
* fix several fields that were not filled with the saved value when editing
* hide storage details of recurring_todo by adding public getters and refactoring view to use them in stead of database fields

app/controllers/recurring_todos_controller.rb

@@ -30,9 +30,9 @@ class RecurringTodosController < ApplicationController
     @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 the form for a new recurring todo and the edit form are on the
-    # same page. Same goes for start_from and end_date
+    # 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']
     params['recurring_todo']['end_date']=params['recurring_todo_edit_end_date']
     params['recurring_todo']['start_from']=params['recurring_todo_edit_start_from']
@@ -81,7 +81,7 @@ class RecurringTodosController < ApplicationController
   end
   
   def create
-    p = RecurringTodoCreateParamsHelper.new(params)        
+    p = RecurringTodoCreateParamsHelper.new(params)
     @recurring_todo = current_user.recurring_todos.build(p.attributes)
 
     if p.project_specified_by_name?
@@ -185,7 +185,17 @@ class RecurringTodosController < ApplicationController
 
     def initialize(params)
       @params = params['request'] || params
-      @attributes = params['request'] && params['request']['recurring_todo']  || params['recurring_todo']
+      attr = params['request'] && params['request']['recurring_todo']  || params['recurring_todo']
+      
+      # make sure all selectors (recurring_period, recurrence_selector,
+      # daily_selector, monthly_selector and yearly_selector) are first in hash
+      # so that they are processed first by the model
+      @attributes = {
+        'recurring_period' => attr['recurring_period'],
+        'daily_selector' => attr['daily_selector'],
+        'monthly_selector' => attr['monthly_selector'],
+        'yearly_selector' => attr['yearly_selector']
+      }.merge(attr)
     end
       
     def attributes

app/models/recurring_todo.rb

@@ -58,6 +58,8 @@ class RecurringTodo < ActiveRecord::Base
       self.only_work_days = false
     when 'daily_every_work_day'
       self.only_work_days = true
+    else
+      raise Exception.new, "unknown daily recurrence pattern: '#{selector}'"      
     end
   end
   
@@ -67,12 +69,18 @@ class RecurringTodo < ActiveRecord::Base
     end
   end
   
+  def daily_every_x_days
+    return self.every_other1
+  end
+  
   # WEEKLY
   
   def weekly_every_x_week=(x)
-    if recurring_period=='weekly'
-      self.every_other1 = x
-    end
+    self.every_other1 = x if recurring_period=='weekly'
+  end
+  
+  def weekly_every_x_week
+    return self.every_other1
   end
   
   def switch_week_day (day, position)
@@ -83,9 +91,7 @@ class RecurringTodo < ActiveRecord::Base
   end
   
   def weekly_return_monday=(selector)
-    if recurring_period=='weekly'
-      switch_week_day(selector,1)
-    end
+    switch_week_day(selector,1) if recurring_period=='weekly'
   end
 
   def weekly_return_tuesday=(selector)
@@ -154,31 +160,58 @@ class RecurringTodo < ActiveRecord::Base
     if recurring_period=='monthly'
       self.recurrence_selector= (selector=='monthly_every_x_day')? 0 : 1
     end
-    # todo
   end
   
   def monthly_every_x_day=(x)
-    if recurring_period=='monthly'
-      self.every_other1 = x
-    end
+    self.every_other1 = x if recurring_period=='monthly'
+  end
+  
+  def monthly_every_x_day
+    return self.every_other1
   end
   
   def monthly_every_x_month=(x)
-    if recurring_period=='monthly'
-      self.every_other2 = x
+    self.every_other2 = x if recurring_period=='monthly' && recurrence_selector == 0
+  end
+  
+  def monthly_every_x_month
+    # in case monthly pattern is every day x, return every_other2 otherwise
+    # return a default value
+    if self.recurrence_selector == 0
+      return self.every_other2
+    else
+      return 1
+    end
+  end
+
+  def monthly_every_x_month2=(x)    
+    self.every_other2 = x if recurring_period=='monthly' && recurrence_selector == 1
+  end
+
+  def monthly_every_x_month2
+    # in case monthly pattern is every xth day, return every_other2 otherwise
+    # return a default value
+    if self.recurrence_selector == 1
+      return self.every_other2
+    else
+      return 1
     end
   end
   
   def monthly_every_xth_day=(x)
-    if recurring_period=='monthly' 
-      self.every_other3 = x
-    end
+    self.every_other3 = x if recurring_period=='monthly' 
+  end
+  
+  def monthly_every_xth_day
+    return self.every_other3
   end
   
   def monthly_day_of_week=(dow)
-    if recurring_period=='monthly'
-      self.every_count = dow
-    end
+    self.every_count = dow if recurring_period=='monthly'
+  end
+  
+  def monthly_day_of_week
+    return self.every_count
   end
   
   # YEARLY
@@ -190,27 +223,55 @@ class RecurringTodo < ActiveRecord::Base
   end
   
   def yearly_month_of_year=(moy)
-    if recurring_period=='yearly'
-      self.every_other2 = moy
+    self.every_other2 = moy if self.recurring_period=='yearly'  && self.recurrence_selector == 0
+  end
+  
+  def yearly_month_of_year
+    # if recurrence pattern is every x day in a month, return month otherwise
+    # return a default value
+    if self.recurrence_selector == 0
+      return self.every_other2
+    else
+      return Time.now.month
+    end
+  end
+
+  def yearly_month_of_year2=(moy)
+    self.every_other2 = moy if self.recurring_period=='yearly' && self.recurrence_selector == 1
+  end
+  
+  def yearly_month_of_year2
+    # if recurrence pattern is every xth day in a month, return month otherwise
+    # return a default value
+    if self.recurrence_selector == 1
+      return self.every_other2
+    else
+      return Time.now.month
     end
   end
   
   def yearly_every_x_day=(x)
-    if recurring_period=='yearly'
-      self.every_other1 = x
-    end
+    self.every_other1 = x if recurring_period=='yearly'
+  end
+  
+  def yearly_every_x_day
+    return self.every_other1
   end
   
   def yearly_every_xth_day=(x)
-    if recurring_period=='yearly'
-      self.every_other3 = x
-    end
+    self.every_other3 = x if recurring_period=='yearly'
+  end
+  
+  def yearly_every_xth_day
+    return self.every_other3
   end
   
   def yearly_day_of_week=(dow)
-    if recurring_period=='yearly'
-      self.every_count=dow
-    end
+    self.every_count=dow if recurring_period=='yearly'
+  end
+  
+  def yearly_day_of_week
+    return self.every_count
   end
   
   # target
@@ -243,7 +304,7 @@ class RecurringTodo < ActiveRecord::Base
       end
     when 'monthly'
       if self.recurrence_selector == 0
-        return "every month on day #{self.every_other1}"
+        return "every #{self.every_other2} month#{self.every_other2>1?'s':''} on day #{self.every_other1}"
       else
         return "every #{self.xth} #{self.day_of_week} of every #{self.every_other2} month#{self.every_other2>1?'s':''}"
       end
@@ -514,4 +575,10 @@ class RecurringTodo < ActiveRecord::Base
     self.save
   end
   
+  protected
+  
+  def validate
+    errors.add("", "At least one day must be selected in the weekly pattern") if self.every_day == '       '
+  end
+  
 end

app/views/recurring_todos/_edit_form.html.erb

@@ -7,7 +7,7 @@
     :complete => "$('recurring_todo_edit_action_submit').stopWaiting();",
     :condition => "!$('recurring_todo_edit_action_submit').isWaiting()") do 
     -%>
-    <div id="status"><%= error_messages_for("item", :object_name => 'action') %></div>
+    <div id="edit_status"><%= error_messages_for("item", :object_name => 'action') %></div>
   
     <div id="recurring_todo_form_container">
       <div id="recurring_todo">
@@ -86,12 +86,12 @@
     <div id="recurring_edit_daily" style="display:<%= @recurring_todo.recurring_period == 'daily' ? 'block' : 'none' %> ">
       <label>Recurrence</label><br/>
       <%= radio_button_tag('recurring_todo[daily_selector]', 'daily_every_x_day', !@recurring_todo.only_work_days)%> Every <%= 
-      text_field_tag( 'recurring_todo[daily_every_x_days]', @recurring_todo.every_other1, {"size" => 3, "tabindex" => 9}) %> day(s)<br/>
+      text_field_tag( 'recurring_todo[daily_every_x_days]', @recurring_todo.daily_every_x_days, {"size" => 3, "tabindex" => 9}) %> day(s)<br/>
       <%= radio_button_tag('recurring_todo[daily_selector]', 'daily_every_work_day', @recurring_todo.only_work_days)%> Every work day<br/>
     </div>
     <div id="recurring_edit_weekly" style="display:<%= @recurring_todo.recurring_period == 'weekly' ? 'block' : 'none' %>">
       <label>Recurrence</label><br/>
-      Returns every <%= text_field_tag('recurring_todo[weekly_every_x_week]', @recurring_todo.every_other1, {"size" => 3, "tabindex" => 9}) %> week on<br/>
+      Returns every <%= text_field_tag('recurring_todo[weekly_every_x_week]', @recurring_todo.weekly_every_x_week, {"size" => 3, "tabindex" => 9}) %> week on<br/>
       <%= check_box_tag('recurring_todo[weekly_return_monday]', 'm', @recurring_todo.on_monday ) %> Monday
       <%= check_box_tag('recurring_todo[weekly_return_tuesday]', 't', @recurring_todo.on_tuesday) %> Tuesday
       <%= check_box_tag('recurring_todo[weekly_return_wednesday]', 'w', @recurring_todo.on_wednesday) %> Wednesday
@@ -103,22 +103,22 @@
     <div id="recurring_edit_monthly" style="display:<%= @recurring_todo.recurring_period == 'monthly' ? 'block' : 'none' %>">
       <label>Recurrence</label><br/>
       <%= radio_button_tag('recurring_todo[monthly_selector]', 'monthly_every_x_day', @recurring_todo.recurrence_selector == 0)%> Day <%= 
-      text_field_tag('recurring_todo[monthly_every_x_day]', Time.now.mday, {"size" => 3, "tabindex" => 9}) %> on every <%= 
-      text_field_tag('recurring_todo[monthly_every_x_month]', 1, {"size" => 3, "tabindex" => 10}) %> month<br/>
+      text_field_tag('recurring_todo[monthly_every_x_day]', @recurring_todo.monthly_every_x_day, {"size" => 3, "tabindex" => 9}) %> on every <%= 
+      text_field_tag('recurring_todo[monthly_every_x_month]', @recurring_todo.monthly_every_x_month, {"size" => 3, "tabindex" => 10}) %> month<br/>
       <%= radio_button_tag('recurring_todo[monthly_selector]', 'monthly_every_xth_day', @recurring_todo.recurrence_selector == 1)%> The <%= 
-      select_tag('recurring_todo[monthly_every_xth_day]', options_for_select(@xth_day), {}) %> <%= 
-      select_tag('recurring_todo[monthly_day_of_week]' , options_for_select(@days_of_week, Time.now.wday), {}) %> of every <%= 
-      text_field_tag('recurring_todo[monthly_every_x_month]', 1, {"size" => 3, "tabindex" => 11}) %> month<br/>
+      select_tag('recurring_todo[monthly_every_xth_day]', options_for_select(@xth_day, @xth_day[@recurring_todo.monthly_every_xth_day-1][1])) %> <%= 
+      select_tag('recurring_todo[monthly_day_of_week]' , options_for_select(@days_of_week, @recurring_todo.monthly_day_of_week), {}) %> of every <%= 
+      text_field_tag('recurring_todo[monthly_every_x_month2]', @recurring_todo.monthly_every_x_month2, {"size" => 3, "tabindex" => 11}) %> month<br/>
     </div>
     <div id="recurring_edit_yearly" style="display:<%= @recurring_todo.recurring_period == 'yearly' ? 'block' : 'none' %>">
       <label>Recurrence</label><br/>
       <%= radio_button_tag('recurring_todo[yearly_selector]', 'yearly_every_x_day', @recurring_todo.recurrence_selector == 0)%> Every <%= 
-      select_tag('recurring_todo[yearly_month_of_year]', options_for_select(@months_of_year, @recurring_todo.every_other2), {}) %> <%= 
-      text_field_tag('recurring_todo[yearly_every_x_day]', @recurring_todo.every_other1, "size" => 3, "tabindex" => 9) %><br/>
+      select_tag('recurring_todo[yearly_month_of_year]', options_for_select(@months_of_year, @recurring_todo.yearly_month_of_year), {}) %> <%= 
+      text_field_tag('recurring_todo[yearly_every_x_day]', @recurring_todo.yearly_every_x_day, "size" => 3, "tabindex" => 9) %><br/>
       <%= radio_button_tag('recurring_todo[yearly_selector]', 'yearly_every_xth_day', @recurring_todo.recurrence_selector == 1)%> The <%= 
-      select_tag('recurring_todo[yearly_every_xth_day]', options_for_select(@xth_day, @recurring_todo.every_other3), {}) %> <%= 
-      select_tag('recurring_todo[yearly_day_of_week]', options_for_select(@days_of_week, @recurring_todo.every_count), {}) %> of <%= 
-      select_tag('recurring_todo[yearly_month_of_year]', options_for_select(@months_of_year, @recurring_todo.every_other2), {}) %><br/>
+      select_tag('recurring_todo[yearly_every_xth_day]', options_for_select(@xth_day, @recurring_todo.yearly_every_xth_day), {}) %> <%= 
+      select_tag('recurring_todo[yearly_day_of_week]', options_for_select(@days_of_week, @recurring_todo.yearly_day_of_week), {}) %> of <%= 
+      select_tag('recurring_todo[yearly_month_of_year2]', options_for_select(@months_of_year, @recurring_todo.yearly_month_of_year2), {}) %><br/>
     </div>
     <div id="recurring_target">
       <label>Set recurrence on</label><br/>

app/views/recurring_todos/_recurring_todo_form.erb

@@ -6,7 +6,7 @@
     :complete => "$('recurring_todo_new_action_submit').stopWaiting();",
     :condition => "!$('recurring_todo_new_action_submit').isWaiting()") do 
     -%>
-    <div id="status"><%= error_messages_for("item", :object_name => 'action') %></div>
+    <div id="new_status"><%= error_messages_for("item", :object_name => 'action') %></div>
   
     <div id="recurring_todo_form_container">
       <div id="recurring_todo">
@@ -106,7 +106,7 @@
       <%= radio_button_tag('recurring_todo[monthly_selector]', 'monthly_every_xth_day')%> The <%= 
       select_tag('recurring_todo[monthly_every_xth_day]', options_for_select(@xth_day), {}) %> <%= 
       select_tag('recurring_todo[monthly_day_of_week]' , options_for_select(@days_of_week, Time.now.wday), {}) %> of every <%= 
-      text_field_tag('recurring_todo[monthly_every_x_month]', 1, {"size" => 3, "tabindex" => 11}) %> month<br/>
+      text_field_tag('recurring_todo[monthly_every_x_month2]', 1, {"size" => 3, "tabindex" => 11}) %> month<br/>
     </div>
     <div id="recurring_yearly" style="display:none">
       <label>Recurrence</label><br/>
@@ -116,7 +116,7 @@
       <%= radio_button_tag('recurring_todo[yearly_selector]', 'yearly_every_xth_day')%> The <%= 
       select_tag('recurring_todo[yearly_every_xth_day]', options_for_select(@xth_day), {}) %> <%= 
       select_tag('recurring_todo[yearly_day_of_week]', options_for_select(@days_of_week, Time.now.wday), {}) %> of <%= 
-      select_tag('recurring_todo[yearly_month_of_year]', options_for_select(@months_of_year, Time.now.month), {}) %><br/>
+      select_tag('recurring_todo[yearly_month_of_year2]', options_for_select(@months_of_year, Time.now.month), {}) %><br/>
     </div>
     <div id="recurring_target">
       <label>Set recurrence on</label><br/>

app/views/recurring_todos/create.js.rjs

@@ -1,5 +1,5 @@
-page.show 'status'
-page.replace_html 'status', "#{error_messages_for('recurring_todo')}"
+page.show 'new_status'
+page.replace_html 'new_status', "#{error_messages_for('recurring_todo')}"
 
 page.notify :notice, @message, 5.0
 if @recurring_saved

app/views/recurring_todos/update.js.rjs

@@ -17,6 +17,6 @@ if @saved
   page.visual_effect :highlight, dom_id(@recurring_todo), :duration => 3
 
 else
-  page.show 'error_status'
-  page.replace_html 'error_status', "#{error_messages_for('todo')}"
+  page.show 'edit_status'
+  page.replace_html 'edit_status', "#{error_messages_for('recurring_todo')}"
 end