Several improvements of functionality and UI usability

README.md

@@ -26,7 +26,7 @@ You should now be able to see the plugin list in Administration -> Plugins.
 
    Crontab example (running the check for recurrence every 6 hours):
    ```bash
-   * */4 * * * /bin/sh "cd {path_to_redmine} && rake RAILS_ENV=production redmine:recur_tasks" >> log/cron_rake.log 2>&1
+   27 */4 * * * /bin/sh "cd {path_to_redmine} && rake RAILS_ENV=production redmine:recur_tasks" >> log/cron_rake.log 2>&1
    ```
 
 ## Upgrade or Migrate Plugin
@@ -57,4 +57,4 @@ Follow standard Redmine plugin un-installation -- (barely) modified from http://
 
 2. Remove the plugin from the plugins folder (#{RAILS_ROOT}/plugins)
 
-3. Restart Redmine (or web server)
+3. Restart Redmine (or web server)

app/controllers/recurring_tasks_controller.rb

@@ -7,15 +7,12 @@ class RecurringTasksController < ApplicationController
   before_filter :find_recurring_task, :except => [:index, :new, :create]
   before_filter :set_interval_units, :except => [:index, :show]
   before_filter :set_recurrable_issues, :except => [:index, :show]
+  before_filter :cancel_edit, :only => [:new, :create, :edit, :update]
 
   def index
     @recurring_tasks = RecurringTask.all_for_project(@project)
   end
 
-  def show
-    # default behavior is fine
-  end
-
   def new
     @recurring_task = RecurringTask.new
 
@@ -26,11 +23,10 @@ def new
 
   # creates a new recurring task
   def create
-    params[:recurring_task][:interval_unit] = RecurringTask.get_interval_from_localized_name(params[:recurring_task][:interval_localized_name])
     @recurring_task = RecurringTask.new(params[:recurring_task])
     if @recurring_task.save
       flash[:notice] = l(:recurring_task_created)
-      redirect_to :action => :show, :id => @recurring_task.id
+      redirect_to :controller => :issues, :action => :show, :id => @recurring_task.issue.id
     else
       logger.debug "Could not create recurring task from #{params[:post]}"
       render :new # errors are displayed to user on form
@@ -41,14 +37,21 @@ def edit
     # default behavior is fine
   end
 
-  # saves the task and redirects to show
+  def cancel_edit
+    if params[:commit] == l(:button_cancel)
+      redirect_to :back
+    end
+  rescue ActionController::RedirectBackError
+    redirect_to default
+  end
+
+  # saves the task and redirects to issue view
   def update
     logger.info "Updating recurring task #{params[:id]}"
 
-    params[:recurring_task][:interval_unit] = RecurringTask.get_interval_from_localized_name(params[:recurring_task][:interval_localized_name])
     if @recurring_task.update_attributes(params[:recurring_task])
       flash[:notice] = l(:recurring_task_saved)
-      redirect_to :action => :show
+      redirect_to :controller => :issues, :action => :show, :id => @recurring_task.issue.id
     else
       logger.debug "Could not save recurring task #{@recurring_task}"
       render :edit # errors are displayed to user on form
@@ -60,10 +63,10 @@ def destroy
 
     if @recurring_task.destroy
       flash[:notice] = l(:recurring_task_removed)
-      redirect_to :action => :index
+      redirect_to :back
     else
       flash[:notice] = l(:error_recurring_task_could_not_remove)
-      redirect_to :action => :show, :id => @recurring_task
+      render :back
     end
   end
 
@@ -85,6 +88,9 @@ def find_recurring_task
   end
 
   def set_interval_units
-    @interval_units = RecurringTask::INTERVAL_UNITS_LOCALIZED
+    @interval_units = 
+      RecurringTask::INTERVAL_UNITS_LOCALIZED.collect{|k,v| [v, k]}
   end
+
 end
+

app/helpers/recurring_tasks_helper.rb

@@ -6,7 +6,7 @@ def show_error msg
 
   def delete_button recurring_task
     if User.current.allowed_to?(:delete_issue_recurrence, recurring_task.project)
-      button_to(l(:button_delete), {:action => 'destroy', :id => recurring_task, :project_id => recurring_task.project.id}, :method => :delete, :class => 'icon icon-del', :confirm => l(:text_are_you_sure))
+      link_to(l(:button_delete), {:action => 'destroy', :id => recurring_task, :project_id => recurring_task.project.id}, :method => :delete, :class => 'icon icon-del', :confirm => l(:text_are_you_sure))
     end
   end
 

app/models/recurring_task.rb

@@ -10,77 +10,95 @@ class RecurringTask < ActiveRecord::Base
   INTERVAL_WEEK = 'w'
   INTERVAL_MONTH = 'm'
   INTERVAL_YEAR = 'y'
+
+  # similar flags for denoting more complex recurrence schemes
+  # they are modyfing how due dates are scheduled when INTERVAL_MONTH is in
+  # effect
+  MONTH_MODIFIER_DAY_FROM_FIRST = 'mdff'
+  MONTH_MODIFIER_DAY_TO_LAST = 'mdtl'
+  MONTH_MODIFIER_DOW_FROM_FIRST = 'mdowff'
+  MONTH_MODIFIER_DOW_TO_LAST = 'mdowtl'
 
   # must come before validations otherwise uninitialized
-  INTERVAL_UNITS_LOCALIZED = [l(:interval_day), l(:interval_week), l(:interval_month), l(:interval_year)]
+  INTERVAL_UNITS_LOCALIZED = {
+    INTERVAL_DAY => l(:interval_day),
+    INTERVAL_WEEK => l(:interval_week),
+    INTERVAL_MONTH => l(:interval_month),
+    INTERVAL_YEAR => l(:interval_year)
+  }
+  MONTH_MODIFIERS_LOCALIZED = {
+    MONTH_MODIFIER_DAY_FROM_FIRST => l(:month_modifier_day_from_first),
+    MONTH_MODIFIER_DAY_TO_LAST => l(:month_modifier_day_to_last),
+    MONTH_MODIFIER_DOW_FROM_FIRST => l(:month_modifier_dow_from_first),
+    MONTH_MODIFIER_DOW_TO_LAST => l(:month_modifier_dow_to_last)
+  }
 
   # pulled out validates_presence_of separately
   # for older Rails compatibility
-  validates_presence_of :interval_localized_name
+  validates_presence_of :interval_unit
+  validates_presence_of :interval_modifier, if: "interval_unit == INTERVAL_MONTH"
   validates_presence_of :interval_number
 
-  validates_inclusion_of :interval_localized_name, :in => RecurringTask::INTERVAL_UNITS_LOCALIZED, :message => "#{l(:error_invalid_interval)} '%{value}' (Validation)"
+  validates_inclusion_of :interval_unit,
+    :in => RecurringTask::INTERVAL_UNITS_LOCALIZED.keys,
+    :message => "#{l(:error_invalid_interval)} '%{value}' (Validation)"
+  validates_inclusion_of :interval_modifier,
+    :in => RecurringTask::MONTH_MODIFIERS_LOCALIZED.keys,
+    :message => "#{l(:error_invalid_modifier)} '%{value}' (Validation)",
+    if: "interval_unit == INTERVAL_MONTH"
   validates_numericality_of :interval_number, :only_integer => true, :greater_than => 0
   # cannot validate presence of issue if want to use other features; requiring presence of fixed_schedule requires it to be true
 
   validates_associated :issue # just in case we build in functionality to add an issue at the same time, verify the issue is ok  
-  
+
   # text for the interval name
   def interval_localized_name
     if new_record?
       @interval_localized_name
     else
-      case interval_unit
-      when INTERVAL_DAY
-        l(:interval_day)
-      when INTERVAL_WEEK
-        l(:interval_week)
-      when INTERVAL_MONTH
-        l(:interval_month)
-      when INTERVAL_YEAR
-        l(:interval_year)
+      if INTERVAL_UNITS_LOCALIZED.has_key?(interval_unit)
+        INTERVAL_UNITS_LOCALIZED[interval_unit]
       else
         raise "#{l(:error_invalid_interval)} #{interval_unit} (interval_localized_name)"
       end
     end
   end
-
-  # interval database name for the localized text
-  def interval_localized_name=(value)
-    @interval_localized_name = value
-    interval_unit= RecurringTask.get_interval_from_localized_name(value)
-  end  
-
-  # used for migration #2
-  def self.get_interval_from_localized_name(value)
-    case value
-      when l(:interval_day)
-        INTERVAL_DAY
-      when l(:interval_week)
-        INTERVAL_WEEK
-      when l(:interval_month)
-        INTERVAL_MONTH
-      when l(:interval_year)
-        INTERVAL_YEAR
+
+  # text for the interval modifier
+  def interval_localized_modifier
+    if new_record?
+      @interval_localized_modifier
+    else
+      modifiers_names = get_modifiers_descriptions
+      if modifiers_names.has_key?(interval_modifier)
+        modifiers_names[interval_modifier]
       else
-        raise "#{l(:error_invalid_interval)} #{value} (interval_localized_name=)"
+        raise "#{l(:error_invalid_modifier)} #{interval_modifier} (interval_localized_modifier)"
       end
+    end
   end
 
-  # time interval value of the recurrence pattern
-  def recurrence_pattern
-    case interval_unit
-    when INTERVAL_DAY
-      interval_number.days
-    when INTERVAL_WEEK
-      interval_number.weeks
-    when INTERVAL_MONTH
-      interval_number.months
-    when INTERVAL_YEAR
-      interval_number.years
-    else
-      raise "#{l(:error_invalid_interval)} #{interval_unit} (recurrence_pattern)"
+  # used for migration #002
+  def self.get_interval_from_localized_name(value)
+    retval = INTERVAL_UNITS_LOCALIZED.key(value)
+    if retval.nil?
+      raise "#{l(:error_invalid_interval)} #{value} (interval_localized_name=)"
     end
+    retval
+  end
+
+  def get_modifiers_descriptions
+    prev_date = previous_date_for_recurrence
+    days_to_eom = (prev_date.end_of_month.mday - prev_date.mday + 1).to_i
+    #print days_to_eom, " ", prev_date.end_of_month
+    values = {
+      :days_from_bom => prev_date.mday.ordinalize,
+      :days_to_eom => days_to_eom.ordinalize,
+      :day_of_week => prev_date.strftime("%A"),
+      :dows_from_bom => ((prev_date.mday - 1) / 7 + 1).ordinalize,
+      :dows_to_eom => (((prev_date.end_of_month.mday - prev_date.mday).to_i / 7) + 1).ordinalize,
+    }
+    Hash[MONTH_MODIFIERS_LOCALIZED.map{|k,v| [k, v % values]}]
   end
 
   def self.find_by_issue issue
@@ -95,17 +113,53 @@ def self.all_for_project project
 
   # next due date for the task, if there is one (relative tasks won't have a next schedule until the current issue is closed)
   def next_scheduled_recurrence
-    if previous_date_for_recurrence.nil? 
+    prev_date = previous_date_for_recurrence
+    if prev_date.nil? 
       logger.error "Previous date for recurrence was nil for recurrence #{id}"
       Date.today
     else 
-      previous_date_for_recurrence + recurrence_pattern
+      case interval_unit
+      when INTERVAL_DAY
+        (prev_date + interval_number.days).to_date
+      when INTERVAL_WEEK
+        (prev_date + interval_number.weeks).to_date
+      when INTERVAL_MONTH
+        case interval_modifier
+        when MONTH_MODIFIER_DAY_FROM_FIRST
+          (prev_date + interval_number.months).to_date
+        when MONTH_MODIFIER_DAY_TO_LAST
+          days_to_last = prev_date.end_of_month - prev_date
+          ((prev_date + interval_number.months).end_of_month - days_to_last).to_date
+        when MONTH_MODIFIER_DOW_FROM_FIRST
+          source_dow = prev_date.days_to_week_start
+          target_bom = (prev_date + interval_number.months).beginning_of_month
+          target_bom_dow = target_bom.days_to_week_start
+          week = ((prev_date.mday - 1) / 7) + ((source_dow >= target_bom_dow) ? 0 : 1)
+          (target_bom + week.weeks + source_dow - target_bom_dow).to_date
+        when MONTH_MODIFIER_DOW_TO_LAST
+          source_dow = prev_date.days_to_week_start
+          target_eom = (prev_date + interval_number.months).end_of_month
+          target_eom_dow = target_eom.days_to_week_start
+          week = ((prev_date.end_of_month - prev_date).to_i / 7) + ((source_dow > target_eom_dow) ? 1 : 0)
+          (target_eom - week.weeks + source_dow - target_eom_dow).to_date
+        else
+          raise "#{l(:error_invalid_modifier)} #{interval_modifier} (next_scheduled_recurrence)"
+        end
+      when INTERVAL_YEAR
+        (prev_date + interval_number.years).to_date
+      else
+        raise "#{l(:error_invalid_interval)} #{interval_unit} (next_scheduled_recurrence)"
+      end
     end
   end
 
   # whether a recurrence needs to be added
   def need_to_recur?
-    if(fixed_schedule and (previous_date_for_recurrence + recurrence_pattern) <= (Time.now.to_date + 1.day)) then true else issue.closed? end
+    if fixed_schedule
+      previous_date_for_recurrence <= Time.now.to_date
+    else
+      issue.closed?
+    end
   end
 
   # check whether a recurrence is needed, and add one if not
@@ -121,7 +175,7 @@ def recur_issue_if_needed!
 
     while need_to_recur?      
       new_issue = issue.copy
-      new_issue.due_date = previous_date_for_recurrence + recurrence_pattern
+      new_issue.due_date = next_scheduled_recurrence
       new_issue.start_date = new_issue.due_date
       new_issue.done_ratio = 0
       new_issue.status = IssueStatus.default # issue status is NOT automatically new, default is whatever the default status for new issues is
@@ -133,12 +187,18 @@ def recur_issue_if_needed!
     end
   end
 
+  def recurrence_to_s
+    modifier = (interval_unit == INTERVAL_MONTH) ? " #{interval_localized_modifier}" : ""
+    schedule = fixed_schedule ? l(:label_recurs_fixed) : l(:label_recurs_dependent)
+    "#{l(:label_recurrence_pattern)} #{interval_number} #{interval_localized_name.pluralize(interval_number)}#{modifier}, #{schedule}"
+  end
+
   def to_s
     i = "No issue associated "
     if !(issue.nil?)
       i = issue.subj_date
     end
-    "#{i} (#{l(:label_recurrence_pattern)} #{interval_number} #{interval_unit}s " + (:fixed_schedule ? l(:label_recurs_fixed) : l(:label_recurs_dependent)) + ")"
+    "#{i} (#{recurrence_to_s})"
   end
 
   def to_s_long
@@ -169,3 +229,4 @@ def previous_date_for_recurrence
     end
   end
 end
+

app/views/issues/_show_recurrence.html.erb

@@ -1,18 +1,31 @@
 <hr />
-<tr>
-	<td class="recurrence" colspan="2"></th>
-		<% if User.current.allowed_to?(:add_issue_recurrence, project) %>		
-		    <div class="contextual"><%= link_to(l(:label_add_recurring_task), new_recurring_task_path(:issue_id => issue.id, :project_id => project.id), :class => 'icon icon-add') %></div>
-		<% end %>
-		<p><strong><%= l(:field_recurrence)%></strong></p>
-		<% if issue.recurs? %>
-			<ul>
-				<% issue.recurring_tasks.each do |rt| %>
-					<li><%= if User.current.allowed_to?(:view_issue_recurrence, project) then link_to(rt.to_s, recurring_task_path(:id => rt.id, :project_id => project.id)) else rt.to_s end %></li>
-				<% end %>
-			</ul>
-		<% else %>
-			<p><%= l(:label_no_recurrence)%></p>
-		<% end %>
-	</td>
-</tr>
+<div class="recurrence">
+  <% if User.current.allowed_to?(:add_issue_recurrence, project) %>
+      <div class="contextual"><%= link_to(l(:label_add_recurring_task), new_recurring_task_path(:issue_id => issue.id, :project_id => project.id)) %></div>
+  <% end %>
+  <p><strong><%= l(:field_recurrence).pluralize(2) %></strong></p>
+  <% if issue.recurs? %>
+    <table class="list">
+    <tbody>
+      <% issue.recurring_tasks.each do |rt| %>
+        <tr>
+          <% if User.current.allowed_to?(:view_issue_recurrence, project) %>
+            <td class="subject">
+              <%= link_to(rt.recurrence_to_s, edit_recurring_task_path(:id => rt.id, :project_id => project.id)) %>
+            </td>
+            <td>
+              <%= "#{l(:label_next_scheduled_run)}: #{rt.next_scheduled_recurrence}" %>
+            </td>
+            <td class="buttons">
+              <%= link_to(l(:button_edit), edit_recurring_task_path(:id => rt.id, :project_id => project.id), :class => 'icon icon-edit') %>
+              <%= link_to(l(:button_delete), destroy_recurring_task_path(:id => rt.id, :project_id => project.id), :method => :delete, :class => 'icon icon-del') %></td>
+          <% else %>
+            <td colspan=3><%= rt.recurrence_to_s %></td>
+          <% end %>
+        </tr>
+      <% end %>
+    </tbody>
+    </table>
+  <% end %>
+</div>
+