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.
Share
Related posts:
- Redmine Refactor #101: Extract activity from ProjectsController to a new controller
- Redmine Refactor #95: Extract ContextMenusController from IssuesController
- Redmine Refactor #94: Extract PreviewsController from IssuesController
- Redmine Refactor #93: Extract Controller from IssuesController
- Daily Refactor #68: Extract Class to New Controller
