Daily Refactor #23: Extract method in IssuesController#edit

After yesterday’s refactor of the #bulk_edit method, Jean-Philippe Lang finished the refactoring by merging the temporary method I created in the model (Issue#bulk_edit) and simplifying the parameter attributes.

Back to the drawing board, it looks like the method with the next highest flog score is #edit. #edit is a pretty complex action in Redmine, since it serves as both rendering the edit form and saving the edits of issues and time entries. It flows like this:

def edit
  if allowed to change issue
    set issue attribues
  end

  if GET request
    render form
  else
    if time entry is valid
      attach file uploads
      if issue saved
        save time entry
        redirect to issue
      end
    end
    rerender form
  end

  rescue stale object updates
    render form
  end
end

Just from that psuedocode, it’s easy to see how many different responsibilities that action has. To follow the standard REST design it should be two actions; one for rendering the form (#edit) and one for processing the update (#update).

The Refactoring

This refactoring is the first step towards decouping #edit into two 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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
        attachments = attach_files(@issue, params[:attachments])
        attachments.each {|a| journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
        call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => journal})
        if @issue.save
          # Log spend time
          if User.current.allowed_to?(:log_time, @project)
            @time_entry.save
          end
          if !journal.new_record?
            # Only send notification if something was actually changed
            flash[:notice] = l(:notice_successful_update)
          end
          call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => journal})
          respond_to do |format|
            format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
            format.xml  { head :ok }
          end
          return
        end
      end
      # failure
      respond_to do |format|
        format.html { }
        format.xml  { render :xml => @issue.errors, :status => :unprocessable_entity }
      end
    end
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash.now[:error] = l(:notice_locking_conflict)
    # Remove the previously added attachments if issue was not updated
    attachments.each(&:destroy)
  end
end
 
1
2
3
4
5

# app/views/issues/_edit.rhtml
 {:action => 'edit', :id => @issue},
                              :html => {:id => 'issue-form',
                                        :class => nil,
                                        :multipart => true} do |f| %>

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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
        attachments = attach_files(@issue, params[:attachments])
        attachments.each {|a| journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
        call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => journal})
        if @issue.save
          # Log spend time
          if User.current.allowed_to?(:log_time, @project)
            @time_entry.save
          end
          if !journal.new_record?
            # Only send notification if something was actually changed
            flash[:notice] = l(:notice_successful_update)
          end
          call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => journal})
          respond_to do |format|
            format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
            format.xml  { head :ok }
          end
          return
        end
      end
      # failure
      respond_to do |format|
        format.html { render :action => 'edit' }
        format.xml  { render :xml => @issue.errors, :status => :unprocessable_entity }
      end
    end
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash.now[:error] = l(:notice_locking_conflict)
    # Remove the previously added attachments if issue was not updated
    attachments.each(&:destroy)
  end
 
  #--
  # Start converting to the Rails REST controllers
  #++
  def update
    edit
  end
end
 
1
2
3
4
5
6

# app/views/issues/_edit.rhtml
 {:action => update, :id => @issue},
                             :html => {:id => 'issue-form',
                                       :class => nil,
                                       :method => :put,
                                       :multipart => true} do |f| %>

Review

Instead of trying to dig into #edit and refactor everything at once, I put a little shim in place so #edit was mostly unmodified. Instead, I modified the issue forms so they are already using the new #update method. This will make it easier to move code over piece by piece. It will take a few refactorings but with the good test suite included in Redmine, it shouldn’t be difficult.

Reference commit