Redmine Refactor #133: Extract Method in WikiController#special

I started trying to refactor the routes.rb for Redmine in order to nest the resources and remove some duplication. Unfortunately, I kept running into trouble and had to scrap that plan. So instead I decided to continue on with my controller refactoring and try to improve things there first.

Using the wc technique I wrote about on RailsInside, I found that the WikiController is the next largest controller that needs to be converted to a REST design. I’ve also had many requests to refactor this controller so I’m going to work on it next.

In Redmine the WikiController handles all of the wiki pages that a user sees. It has some interesting actions since it needs to be able to detect and create new wiki pages if they don’t exist. While reading through the class, an ugly case statement caught my eye in the #special action. From the looks of it, #special handles three separate actions in one. This makes for a good first refactoring.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

class WikiController  "#{WikiPage.table_name}.*, #{WikiContent.table_name}.updated_on",
                                      :joins => "LEFT JOIN #{WikiContent.table_name} ON #{WikiContent.table_name}.page_id = #{WikiPage.table_name}.id",
                                      :order => 'title'
      @pages_by_date = @pages.group_by {|p| p.updated_on.to_date}
      @pages_by_parent_id = @pages.group_by(&:parent_id)
    # export wiki to a single html file
    when 'export'
      if User.current.allowed_to?(:export_wiki_pages, @project)
        @pages = @wiki.pages.find :all, :order => 'title'
        export = render_to_string :action => 'export_multiple', :layout => false
        send_data(export, :type => 'text/html', :filename => "wiki.html")
      else
        redirect_to :action => 'index', :id => @project, :page => nil
      end
      return      
    else
      # requested special page doesn't exist, redirect to default page
      redirect_to :action => 'index', :id => @project, :page => nil
      return
    end
    render :action => "special_#{page_title}"
  end
end
 
1
2
3
4
5
6

# config/routes.rb
  map.with_options :controller => 'wiki' do |wiki_routes|
    wiki_routes.with_options :conditions => {:method => :get} do |wiki_views|
      wiki_views.connect 'projects/:id/wiki/:page', :action => 'special', :page => /page_index|date_index|export/i
    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

class WikiController  "#{WikiPage.table_name}.*, #{WikiContent.table_name}.updated_on",
                                      :joins => "LEFT JOIN #{WikiContent.table_name} ON #{WikiContent.table_name}.page_id = #{WikiPage.table_name}.id",
                                      :order => 'title'
      @pages_by_date = @pages.group_by {|p| p.updated_on.to_date}
      @pages_by_parent_id = @pages.group_by(&:parent_id)
    when 'export'
      redirect_to :action => 'export', :id => @project # Compatibility stub while refactoring
      return
    else
      # requested special page doesn't exist, redirect to default page
      redirect_to :action => 'index', :id => @project, :page => nil
      return
    end
    render :action => "special_#{page_title}"
  end
 
  # Export wiki to a single html file
  def export
    if User.current.allowed_to?(:export_wiki_pages, @project)
      @pages = @wiki.pages.find :all, :order => 'title'
      export = render_to_string :action => 'export_multiple', :layout => false
      send_data(export, :type => 'text/html', :filename => "wiki.html")
    else
      redirect_to :action => 'index', :id => @project, :page => nil
    end
  end
end
 
1
2
3
4
5
6
7
8

# config/routes.rb
  map.with_options :controller => 'wiki' do |wiki_routes|
    wiki_routes.with_options :conditions => {:method => :get} do |wiki_views|
      wiki_views.connect 'projects/:id/wiki/export', :action => 'export'
      wiki_views.connect 'projects/:id/wiki/:page', :action => 'special', :page => /page_index|date_index/i
      # ...
    end
  end

By using extract method on #special I was able to create a completely new method and remove a bunch of the logic deep inside the case statement. Since #export is a full action now, it’s a lot easier to link to and it would now be easier to add other export formats too (e.g. XML, JSON, PDF). Now that I’m thinking about it, this should match the #index action for a standard REST controller but with different formats.

Extra Tip: Since the export was using a GET request, I was able to put a small redirect stub in place of the #special action. This will automatically redirect any requests to the new #export action. Hopefully, this will prevent any major bugs while I work on refactoring the rest of this action. Especially on long term refactoring sessions, it’s good to put in little compatibility hacks and wrappers like this in order to keep the system running while you make changes.

Reference commit