Daily Refactor #63: Extract Method to before_filter in IssuesController

The Refactoring

Mark Thomas pointed out some odd code in Redmine’s IssuesController#build_new_issue_from_params, specifically that it’s doing some error checking in the middle of a method body.

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

# app/controllers/issues_controller.rb
class IssuesController < ApplicationController
  def build_new_issue_from_params
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return false
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return false
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
    @issue.start_date ||= Date.today
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
  end
 
end

After

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

# app/controllers/issues_controller.rb
class IssuesController  [:new, :create]
 
  def build_new_issue_from_params
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return false
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
    @issue.start_date ||= Date.today
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
  end
 
  def check_for_default_issue_status
    if IssueStatus.default.nil?
      render_error l(:error_no_default_issue_status)
      return false
    end
  end
 
end

Review

The error checking almost looks like they should be validations but they are done systemwide. By using extract method I was able to get the error checking out of the action until I can figure out the best way to handle systemwide validations.

Reference commit