small refactorings and add some tests

app/models/recurring_todos/abstract_repeat_pattern.rb

@@ -164,8 +164,10 @@ module RecurringTodos
     
     private
 
-    # Determine start date to calculate next date for recurring todo
+    # Determine start date to calculate next date for recurring todo which
-    # offset needs to be 1.day for daily patterns
+    # takes start_from and previous into account.
+    # offset needs to be 1.day for daily patterns or the start will be the
+    # same day as the previous
     def determine_start(previous, offset=0.day)
       start = self.start_from || NullTime.new
       now = Time.zone.now
@@ -179,9 +181,14 @@ module RecurringTodos
       end
     end
 
+    # Example: get 3rd (x) wednesday  (weekday) of december (month) 2014 (year)
+    # 5th means last, so it will return the 4th if there is no 5th
     def get_xth_day_of_month(x, weekday, month, year)
+      raise "Weekday should be between 0 and 6 with 0=sunday. You supplied #{weekday}" unless (0..6).include?(weekday)
+      raise "x should be 1-4 for first-fourth or 5 for last. You supplied #{x}" unless (0..5).include?(x)
+
       if x == 5
-        # last -> count backwards. use UTC to avoid strange timezone oddities
+        # 5 means last -> count backwards. use UTC to avoid strange timezone oddities
         # where last_day -= 1.day seems to shift tz+0100 to tz+0000
         last_day = Time.utc(year, month, Time.days_in_month(month))
         while last_day.wday != weekday

app/models/recurring_todos/monthly_repeat_pattern.rb

@@ -66,54 +66,56 @@ module RecurringTodos
 
     def get_next_date(previous)
       start = determine_start(previous)
-      day = every_x_day
       n = get(:every_other2)
 
       case recurrence_selector
       when 0 # specific day of the month
-        if (previous && start.mday >= day) || (previous.nil? && start.mday > day)
+        return find_specific_day_of_month(previous, start, n)
-          # there is no next day n in this month, search in next month
-          #
-          #  start += n.months
-          #
-          # The above seems to not work. Fiddle with timezone. Looks like we hit a
-          # bug in rails here where 2008-12-01 +0100 plus 1.month becomes
-          # 2008-12-31 +0100. For now, just calculate in UTC and convert back to
-          # local timezone.
-          #
-          #  TODO: recheck if future rails versions have this problem too
-          start = Time.utc(start.year, start.month, start.day)+n.months
-          start = Time.zone.local(start.year, start.month, start.day)
-
-          # go back to day
-        end
-        Time.zone.local(start.year, start.month, day)
-
       when 1 # relative weekday of a month
-        the_next = get_xth_day_of_month(every_xth_day, day_of_week, start.month, start.year)
+        return find_relative_day_of_month(start, n)
-        if the_next.nil? || the_next <= start
-          # the nth day is already passed in this month, go to next month and try
-          # again
-
-          # fiddle with timezone. Looks like we hit a bug in rails here where
-          # 2008-12-01 +0100 plus 1.month becomes 2008-12-31 +0100. For now, just
-          # calculate in UTC and convert back to local timezone.
-          #  TODO: recheck if future rails versions have this problem too
-          the_next = Time.utc(the_next.year, the_next.month, the_next.day)+n.months
-          the_next = Time.zone.local(the_next.year, the_next.month, the_next.day)
-
-          # TODO: if there is still no match, start will be set to nil. if we ever
-          # support 5th day of the month, we need to handle this case
-          the_next = get_xth_day_of_month(every_xth_day, day_of_week, the_next.month, the_next.year)
-        end
-        the_next
-      else
-        raise Exception.new, "unknown monthly recurrence selection (#{self.recurrence_selector})"
       end
+      nil
     end
 
     private
 
+    def find_specific_day_of_month(previous, start, n)
+      if (previous && start.mday >= every_x_day) || (previous.nil? && start.mday > every_x_day)
+        # there is no next day n in this month, search in next month
+        #
+        #  start += n.months
+        #
+        # The above seems to not work. Fiddle with timezone. Looks like we hit a
+        # bug in rails here where 2008-12-01 +0100 plus 1.month becomes
+        # 2008-12-31 +0100. For now, just calculate in UTC and convert back to
+        # local timezone.
+        #
+        #  TODO: recheck if future rails versions have this problem too
+        start = Time.utc(start.year, start.month, start.day)+n.months
+      end
+      Time.zone.local(start.year, start.month, every_x_day)
+    end      
+
+    def find_relative_day_of_month(start, n)
+      the_next = get_xth_day_of_month(every_xth_day, day_of_week, start.month, start.year)
+      if the_next.nil? || the_next <= start
+        # the nth day is already passed in this month, go to next month and try
+        # again
+
+        # fiddle with timezone. Looks like we hit a bug in rails here where
+        # 2008-12-01 +0100 plus 1.month becomes 2008-12-31 +0100. For now, just
+        # calculate in UTC and convert back to local timezone.
+        #  TODO: recheck if future rails versions have this problem too
+        the_next = Time.utc(the_next.year, the_next.month, the_next.day)+n.months
+        the_next = Time.zone.local(the_next.year, the_next.month, the_next.day)
+
+        # TODO: if there is still no match, start will be set to nil. if we ever
+        # support 5th day of the month, we need to handle this case
+        the_next = get_xth_day_of_month(every_xth_day, day_of_week, the_next.month, the_next.year)
+      end
+      the_next      
+    end
+
     def recurrence_pattern_for_specific_day
       on_day = " #{I18n.t('todos.recurrence.pattern.on_day_n', :n => every_x_day)}"
       if every_xth_day(0) > 1

app/models/recurring_todos/weekly_repeat_pattern.rb

@@ -36,22 +36,7 @@ module RecurringTodos
     end
 
     def get_next_date(previous)
-      # determine start
+      start = determine_start_date(previous)
-      if previous.nil?
-        start = start_from.nil? ? Time.zone.now : self.start_from
-      else
-        start = previous + 1.day
-        if start.wday() == 0
-          # we went to a new week , go to the nth next week and find first match
-          # that week. Note that we already went into the next week, so -1
-          start += (every_x_week-1).week
-        end
-        unless self.start_from.nil?
-          # check if the start_from date is later than previous. If so, use
-          # start_from as start to search for next date
-          start = self.start_from if self.start_from > previous
-        end
-      end
 
       day = find_first_day_in_this_week(start)
       return day unless day == -1
@@ -68,6 +53,25 @@ module RecurringTodos
 
     private
 
+    def determine_start_date(previous)
+      if previous.nil?
+        return self.start_from || Time.zone.now
+      else
+        start = previous + 1.day
+        if start.wday() == 0
+          # we went to a new week, go to the nth next week and find first match
+          # that week. Note that we already went into the next week, so -1
+          start += (every_x_week-1).week
+        end
+        unless self.start_from.nil?
+          # check if the start_from date is later than previous. If so, use
+          # start_from as start to search for next date
+          start = self.start_from if self.start_from > previous
+        end
+        return start
+      end
+    end
+
     def find_first_day_in_this_week(start)
       # check if there are any days left this week for the next todo
       start.wday().upto 6 do |i|

app/models/recurring_todos/yearly_repeat_pattern.rb

@@ -34,8 +34,7 @@ module RecurringTodos
 
     def recurrence_pattern
       if self.recurrence_selector == 0
-        I18n.t("todos.recurrence.pattern.every_year_on",
+        I18n.t("todos.recurrence.pattern.every_year_on", :date => date_as_month_day)
-          :date => I18n.l(DateTime.new(Time.zone.now.year, month_of_year, every_x_day), :format => :month_day))
       else
         I18n.t("todos.recurrence.pattern.every_year_on",
           :date => I18n.t("todos.recurrence.pattern.the_xth_day_of_month", 
@@ -63,34 +62,47 @@ module RecurringTodos
 
     def get_next_date(previous)
       start = determine_start(previous)
-      day = every_x_day
       month = get(:every_other2)
 
       case recurrence_selector
       when 0 # specific day of a specific month
-        if start.month > month || (start.month == month && start.day >= day)
+        return get_specific_day_of_month(start, month)
-          # if there is no next month n and day m in this year, search in next
-          # year
-          start = Time.zone.local(start.year+1, month, 1)
-        else
-          # if there is a next month n, stay in this year
-          start = Time.zone.local(start.year, month, 1)
-        end
-        Time.zone.local(start.year, month, day)
-
       when 1 # relative weekday of a specific month
-        # if there is no next month n in this year, search in next year
+        return get_relative_weekday_of_month(start, month)
-        the_next = start.month > month ? Time.zone.local(start.year+1, month, 1) : start
-
-        # get the xth day of the month
-        the_next = get_xth_day_of_month(self.every_xth_day, day_of_week, month, the_next.year)
-
-        # if the_next is before previous, we went back into the past, so try next
-        # year
-        the_next = get_xth_day_of_month(self.every_xth_day, day_of_week, month, start.year+1) if the_next <= start
-
-        the_next
       end
+      nil
+    end
+
+    private
+
+    def date_as_month_day
+      I18n.l(DateTime.new(Time.zone.now.year, month_of_year, every_x_day), :format => :month_day)
+    end
+
+    def get_specific_day_of_month(start, month)
+      if start.month > month || (start.month == month && start.day >= every_x_day)
+        # if there is no next month n and day m in this year, search in next
+        # year
+        start = Time.zone.local(start.year+1, month, 1)
+      else
+        # if there is a next month n, stay in this year
+        start = Time.zone.local(start.year, month, 1)
+      end
+      Time.zone.local(start.year, month, every_x_day)
+    end
+
+    def get_relative_weekday_of_month(start, month)
+      # if there is no next month n in this year, search in next year
+      the_next = start.month > month ? Time.zone.local(start.year+1, month, 1) : start
+
+      # get the xth day of the month
+      the_next = get_xth_day_of_month(self.every_xth_day, day_of_week, month, the_next.year)
+
+      # if the_next is before previous, we went back into the past, so try next
+      # year
+      the_next = get_xth_day_of_month(self.every_xth_day, day_of_week, month, start.year+1) if the_next <= start
+
+      the_next
     end
 
   end  

test/models/recurring_todos/abstract_repeat_pattern_test.rb

@@ -100,6 +100,32 @@ module RecurringTodos
       assert !rt.continues_recurring?(Time.zone.now), "should end since all occurences are there"
     end
 
+    def test_determine_start
+      Timecop.travel(2013,1,1) do
+        rt = create_recurring_todo
+        assert_equal "2013-01-01 00:00:00 UTC", rt.send(:determine_start, nil).to_s, "no previous date, use today"
+        assert_equal "2013-01-01 00:00:00 UTC", rt.send(:determine_start, nil, 1.day).to_s, "no previous date, use today without offset"
+        assert_equal "2013-01-02 00:00:00 UTC", rt.send(:determine_start, Time.zone.now, 1.day).to_s, "use previous date and offset"
+      end
+    end
+
+    def test_xth_day_of_month
+      rt = create_recurring_todo
+
+      # march 2014 has 5 saturdays, the last will return the 5th
+      assert_equal "2014-03-01 00:00:00 UTC", rt.send(:get_xth_day_of_month, 1, 6, 3, 2014).to_s
+      assert_equal "2014-03-22 00:00:00 UTC", rt.send(:get_xth_day_of_month, 4, 6, 3, 2014).to_s
+      assert_equal "2014-03-29 00:00:00 UTC", rt.send(:get_xth_day_of_month, 5, 6, 3, 2014).to_s
+
+      # march 2014 has 4 fridays, the last will return the 4th
+      assert_equal "2014-03-07 00:00:00 UTC", rt.send(:get_xth_day_of_month, 1, 5, 3, 2014).to_s
+      assert_equal "2014-03-28 00:00:00 UTC", rt.send(:get_xth_day_of_month, 4, 5, 3, 2014).to_s
+      assert_equal "2014-03-28 00:00:00 UTC", rt.send(:get_xth_day_of_month, 5, 5, 3, 2014).to_s
+
+      assert_raise(RuntimeError, "should check on valid weekdays"){ rt.send(:get_xth_day_of_month, 5, 9, 3, 2014) }
+      assert_raise(RuntimeError, "should check on valid count x"){ rt.send(:get_xth_day_of_month, 6, 5, 3, 2014) }
+    end
+
     private
 
     def create_pattern(attributes)
@@ -108,7 +134,7 @@ module RecurringTodos
       builder.pattern
     end
 
-    def create_recurring_todo(attributes)      
+    def create_recurring_todo(attributes={})
       create_pattern(attributes.reverse_merge({
         'recurring_period'     => 'weekly',
         'recurring_target'     => 'due_date',