Daily Refactor #65: Extract Gantt to a new controller

The Refactoring

Today I finally tackled a larger refactoring that I’ve been wanting to do for awhile now. Redmine’s IssuesController has accumulated a lot of extra actions over the years, one of which is an action that renders a Gantt chart. This has never made sense to me, since the Gantt chart collects a bunch of data from the project and it isn’t only about issues. Today, I was able to use extract class and move the #gantt action to it’s own controller.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

# app/controllers/issues_controller.rb
class IssuesController  [:index, :changes, :gantt, :calendar, :preview, :context_menu]
  before_filter :find_optional_project, :only => [:index, :changes, :gantt, :calendar]
 
  def gantt
    @gantt = Redmine::Helpers::Gantt.new(params)
    retrieve_query
    @query.group_by = nil
    if @query.valid?
      events = []
      # Issues that have start and due dates
      events += @query.issues(:include => [:tracker, :assigned_to, :priority],
                              :order => "start_date, due_date",
                              :conditions => ["(((start_date>=? and start_date=? and due_date<=?) or (start_date?)) and start_date is not null and due_date is not null)", @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to]
                              )
      # Issues that don't have a due date but that are assigned to a version with a date
      events += @query.issues(:include => [:tracker, :assigned_to, :priority, :fixed_version],
                              :order => "start_date, effective_date",
                              :conditions => ["(((start_date>=? and start_date=? and effective_date<=?) or (start_date?)) and start_date is not null and due_date is null and effective_date is not null)", @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to]
                              )
      # Versions
      events += @query.versions(:conditions => ["effective_date BETWEEN ? AND ?", @gantt.date_from, @gantt.date_to])
 
      @gantt.events = events
    end
 
    basename = (@project ? "#{@project.identifier}-" : '') + 'gantt'
 
    respond_to do |format|
      format.html { render :template => "issues/gantt.rhtml", :layout => !request.xhr? }
      format.png  { send_data(@gantt.to_image, :disposition => 'inline', :type => 'image/png', :filename => "#{basename}.png") } if @gantt.respond_to?('to_image')
      format.pdf  { send_data(gantt_to_pdf(@gantt, @project), :type => 'application/pdf', :filename => "#{basename}.pdf") }
    end
  end
 
end

After

1
2
3
4
5
6

# app/controllers/issues_controller.rb
class IssuesController  [:index, :changes, :calendar, :preview, :context_menu]
  before_filter :find_optional_project, :only => [:index, :changes, :calendar]
 
  # Removed gantt method
end
 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60

# app/controllers/gantts_controller.rb
class GanttsController  :query_statement_invalid
 
  helper :issues
  helper :projects
  helper :queries
  include QueriesHelper
  include Redmine::Export::PDF
 
  def show
    @gantt = Redmine::Helpers::Gantt.new(params)
    retrieve_query
    @query.group_by = nil
    if @query.valid?
      events = []
      # Issues that have start and due dates
      events += @query.issues(:include => [:tracker, :assigned_to, :priority],
                              :order => "start_date, due_date",
                              :conditions => ["(((start_date>=? and start_date=? and due_date<=?) or (start_date?)) and start_date is not null and due_date is not null)", @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to]
                              )
      # Issues that don't have a due date but that are assigned to a version with a date
      events += @query.issues(:include => [:tracker, :assigned_to, :priority, :fixed_version],
                              :order => "start_date, effective_date",
                              :conditions => ["(((start_date>=? and start_date=? and effective_date<=?) or (start_date?)) and start_date is not null and due_date is null and effective_date is not null)", @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to, @gantt.date_from, @gantt.date_to]
                              )
      # Versions
      events += @query.versions(:conditions => ["effective_date BETWEEN ? AND ?", @gantt.date_from, @gantt.date_to])
 
      @gantt.events = events
    end
 
    basename = (@project ? "#{@project.identifier}-" : '') + 'gantt'
 
    respond_to do |format|
      format.html { render :action => "show", :layout => !request.xhr? }
      format.png  { send_data(@gantt.to_image, :disposition => 'inline', :type => 'image/png', :filename => "#{basename}.png") } if @gantt.respond_to?('to_image')
      format.pdf  { send_data(gantt_to_pdf(@gantt, @project), :type => 'application/pdf', :filename => "#{basename}.pdf") }
    end
  end
 
  private
 
  # Rescues an invalid query statement. Just in case...
  # TODO: Refactor, move to ApplicationController with IssuesController
  def query_statement_invalid(exception)
    logger.error "Query::StatementInvalid: #{exception.message}" if logger
    session.delete(:query)
    sort_clear
    render_error "An error occurred while executing the query and has been logged. Please report this error to your Redmine administrator."
  end
 
  # TODO: Refactor, duplicates IssuesController
  def find_optional_project
    @project = Project.find(params[:project_id]) unless params[:project_id].blank?
    allowed = User.current.allowed_to?({:controller => params[:controller], :action => params[:action]}, @project, :global => true)
    allowed ? true : deny_access
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

Review

In order to make this refactoring work, I had to duplicate a few of the private utility methods from IssuesController. Even if those don’t get refactored soon, this refactoring is still a great win because of the responsibility split. There is some additional code that was needed in this commit that I’ve left out of this post, including the routing and view changes.

Reference commit