Daily Refactor #61: Extract Method in IssuesController for REST

Now I’m starting to refactor Redmine’s IssuesController again in order to have it match the standard Rails REST controllers. This time I’m working on the new/create action pair.

The Refactoring

This refactoring splits the single #new action into the two separate actions.

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
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54

# app/controllers/issues_controller.rb
class IssuesController  [:new, :update_form, :preview, :auto_complete]
 
  def new
    @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
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    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
 
    if request.get? || request.xhr?
      @issue.start_date ||= Date.today
    else
      call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
      if @issue.save
        attachments = Attachment.attach_files(@issue, params[:attachments])
        render_attachment_warning_if_needed(@issue)
        flash[:notice] = l(:notice_successful_create)
        call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
        respond_to do |format|
          format.html {
            redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, 
                                                                           :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } :
                                            { :action => 'show', :id => @issue })
          }
          format.xml  { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) }
        end
        return
      else
        respond_to do |format|
          format.html { }
          format.xml  { render(:xml => @issue.errors, :status => :unprocessable_entity); return }
        end
      end
    end 
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
    render :layout => !request.xhr?
  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
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
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76

# app/controllers/issues_controller.rb
class IssuesController  [:new, :create, :update_form, :preview, :auto_complete]
 
  verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed }
 
  def new
    @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
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    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)
    render :action => 'new', :layout => !request.xhr?
  end
 
  def create
    @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
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    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
 
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
 
    call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
    if @issue.save
      attachments = Attachment.attach_files(@issue, params[:attachments])
      render_attachment_warning_if_needed(@issue)
      flash[:notice] = l(:notice_successful_create)
      call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
      respond_to do |format|
        format.html {
          redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } :
                      { :action => 'show', :id => @issue })
        }
        format.xml  { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) }
      end
      return
    else
      respond_to do |format|
        format.html { render :action => 'new' }
        format.xml  { render(:xml => @issue.errors, :status => :unprocessable_entity); return }
      end
    end
  end
 
end

Review

This helped #new because it’s now not concerned with creating a new Issue, it only builds an Issue object for the view. Now #create will only need to be concerned with saving the Issue.

This refactoring did introduce a lot of duplication, which I’ll be resolving over the next few days. Something taking on more code debt, in the form of duplication, is required over the short term in order to reach your goal.

Reference commit