Redmine Refactor #140: Extract Method WikiController#edit to #update

Digging into the next method in WikiController, I found that the #edit method is doing way too much.

  1. render a form for new wiki page
  2. render a form for editing an existing wiki page
  3. render a form for rolling back a wiki page
  4. creating a new wiki page
  5. updating an existing wiki page
  6. updating an existing wiki page from a rollback

This refactoring uses extract method to split up #edit so the creating and updating actions happen in a different method (#update).

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

class WikiController  @page) if @page.new_record?
 
    @content = @page.content_for_version(params[:version])
    @content.text = initial_page_content(@page) if @content.text.blank?
    # don't keep previous comment
    @content.comments = nil
    if request.get?
      # To prevent StaleObjectError exception when reverting to a previous version
      @content.version = @page.content.version
    else
      if !@page.new_record? && @content.text == params[:content][:text]
        attachments = Attachment.attach_files(@page, params[:attachments])
        render_attachment_warning_if_needed(@page)
        # don't save if text wasn't changed
        redirect_to :action => 'show', :project_id => @project, :page => @page.title
        return
      end
      #@content.text = params[:content][:text]
      #@content.comments = params[:content][:comments]
      @content.attributes = params[:content]
      @content.author = User.current
      # if page is new @page.save will also save content, but not if page isn't a new record
      if (@page.new_record? ? @page.save : @content.save)
        attachments = Attachment.attach_files(@page, params[:attachments])
        render_attachment_warning_if_needed(@page)
        call_hook(:controller_wiki_edit_after_save, { :params => params, :page => @page})
        redirect_to :action => 'show', :project_id => @project, :page => @page.title
      end
    end
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash[:error] = l(:notice_locking_conflict)
  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

class WikiController  @page) if @page.new_record?
 
    @content = @page.content_for_version(params[:version])
    @content.text = initial_page_content(@page) if @content.text.blank?
    # don't keep previous comment
    @content.comments = nil
 
    # To prevent StaleObjectError exception when reverting to a previous version
    @content.version = @page.content.version
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash[:error] = l(:notice_locking_conflict)
  end
 
  verify :method => :post, :only => :update, :render => {:nothing => true, :status => :method_not_allowed }
  # Creates a new page or updates an existing one
  def update
    @page = @wiki.find_or_new_page(params[:page])    
    return render_403 unless editable?
    @page.content = WikiContent.new(:page => @page) if @page.new_record?
 
    @content = @page.content_for_version(params[:version])
    @content.text = initial_page_content(@page) if @content.text.blank?
    # don't keep previous comment
    @content.comments = nil
 
    if !@page.new_record? && params[:content].present? && @content.text == params[:content][:text]
      attachments = Attachment.attach_files(@page, params[:attachments])
      render_attachment_warning_if_needed(@page)
      # don't save if text wasn't changed
      redirect_to :action => 'show', :project_id => @project, :page => @page.title
      return
    end
    @content.attributes = params[:content]
    @content.author = User.current
    # if page is new @page.save will also save content, but not if page isn't a new record
    if (@page.new_record? ? @page.save : @content.save)
      attachments = Attachment.attach_files(@page, params[:attachments])
      render_attachment_warning_if_needed(@page)
      call_hook(:controller_wiki_edit_after_save, { :params => params, :page => @page})
      redirect_to :action => 'show', :project_id => @project, :page => @page.title
    end
 
  rescue ActiveRecord::StaleObjectError
    # Optimistic locking exception
    flash[:error] = l(:notice_locking_conflict)
  end
end

I had to duplicate a bunch of code for this to work but at least the REST refactoring is advancing. I think once I’m done refactoring this round of controllers, I’ll come back to WikiController and start ripping out all of it’s code duplication.

Reference commit