Redmine Refactor #138: Unify Interfaces

Today’s refactoring is a bit different. I’ve noticed a code smell in WikiController that I wanted to fix before I got into it’s other actions.

WikiController is working with the WikiPage model pretty intimately. The naming mismatch isn’t the problem though, the problem is with the parameter names that are passed around.

  • :id – the project id
  • :page – the wiki page

If you’re familiar with how Rails uses resource routing, you should be able to quickly see that this just won’t work. Rails will be trying to route the WikiPage ids but Redmine will use that as the project ids and all hell will break loose (or at least part of hell will break loose, the part that causes bugs…)

So in this “refactoring”, I’ve converted all of WikiController‘s :id to the standard :project_id.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

# 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_index', :action => 'page_index'
      wiki_views.connect 'projects/:id/wiki/date_index', :action => 'date_index'
      wiki_views.connect 'projects/:id/wiki/:page', :action => 'index', :page => nil
      wiki_views.connect 'projects/:id/wiki/:page/edit', :action => 'edit'
      wiki_views.connect 'projects/:id/wiki/:page/rename', :action => 'rename'
      wiki_views.connect 'projects/:id/wiki/:page/history', :action => 'history'
      wiki_views.connect 'projects/:id/wiki/:page/diff/:version/vs/:version_from', :action => 'diff'
      wiki_views.connect 'projects/:id/wiki/:page/annotate/:version', :action => 'annotate'
    end
 
    wiki_routes.connect 'projects/:id/wiki/:page/:action', 
      :action => /edit|rename|destroy|preview|protect/,
      :conditions => {:method => :post}
  end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

# 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/:project_id/wiki/export', :action => 'export'
      wiki_views.connect 'projects/:project_id/wiki/page_index', :action => 'page_index'
      wiki_views.connect 'projects/:project_id/wiki/date_index', :action => 'date_index'
      wiki_views.connect 'projects/:project_id/wiki/:page', :action => 'index', :page => nil
      wiki_views.connect 'projects/:project_id/wiki/:page/edit', :action => 'edit'
      wiki_views.connect 'projects/:project_id/wiki/:page/rename', :action => 'rename'
      wiki_views.connect 'projects/:project_id/wiki/:page/history', :action => 'history'
      wiki_views.connect 'projects/:project_id/wiki/:page/diff/:version/vs/:version_from', :action => 'diff'
      wiki_views.connect 'projects/:project_id/wiki/:page/annotate/:version', :action => 'annotate'
    end
 
    wiki_routes.connect 'projects/:project_id/wiki/:page/:action', 
      :action => /edit|rename|destroy|preview|protect/,
      :conditions => {:method => :post}
  end

(I’m only showing the route changes here, the views and redirection changes aren’t very interesting.)

With this refactoring, the parameters are now:

  • :project_id – the project id
  • :page – the wiki page

Next I’m going to see if I can add :id back, but as the id of the WikiPage and not the Project.

Reference commit