Redmine Refactor #98: Extract method from IssuesController#bulk_edit

After thinking about how IssuesController#bulk_edit works, I decided to leave it in IssuesController. It doesn’t match Rail’s REST conventions but it does match the REST principles for resource representations. IssuesController returns representations of issue collections and issue objects. Since #bulk_edit works on an issue collection, keeping it in the IssuesController makes sense.

There still is refactoring that needs to happen to #bulk_edit though. The single method is responsible for displaying the bulk edit form and also for performing the bulk edit, which is one too many responsibilities.

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

class IssuesController  [:bulk_edit, :move, :perform_move, :destroy]
 
  def bulk_edit
    @issues.sort!
    if request.post?
      attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
      attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
      attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
 
      unsaved_issue_ids = []
      @issues.each do |issue|
        issue.reload
        journal = issue.init_journal(User.current, params[:notes])
        issue.safe_attributes = attributes
        call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
        unless issue.save
          # Keep unsaved issue ids to display them in flash error
          unsaved_issue_ids < 'issues', :action => 'index', :project_id => @project})
      return
    end
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  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

class IssuesController  [:bulk_edit, :bulk_update, :move, :perform_move, :destroy]
 
  verify :method => :post, :only => :bulk_update, :render => {:nothing => true, :status => :method_not_allowed }
 
  def bulk_edit
    @issues.sort!
    @available_statuses = Workflow.available_statuses(@project)
    @custom_fields = @project.all_issue_custom_fields
  end
 
  def bulk_update
    @issues.sort!
 
    attributes = (params[:issue] || {}).reject {|k,v| v.blank?}
    attributes.keys.each {|k| attributes[k] = '' if attributes[k] == 'none'}
    attributes[:custom_field_values].reject! {|k,v| v.blank?} if attributes[:custom_field_values]
 
    unsaved_issue_ids = []
    @issues.each do |issue|
      issue.reload
      journal = issue.init_journal(User.current, params[:notes])
      issue.safe_attributes = attributes
      call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
      unless issue.save
        # Keep unsaved issue ids to display them in flash error
        unsaved_issue_ids < 'issues', :action => 'index', :project_id => @project})
  end
 
end

By splitting #bulk_edit and #bulk_update with extract method, I now have greater control over how each method works. For example, using the routing and verify, only POST requests are sent to #bulk_update so the if request.post? guard isn’t needed anymore. Eventually even that will be refactored once IssuesController becomes a full Rails resource.

Reference commit