Refactoring Redmine

Copyright 2010 Eric Davis, All rights reserved. 2

Refactoring Redmine: 82 Real World Ruby on Rails RefactoringsEric DavisCopyright © 2010 Eric Davis, All rights reserved.

Redmine

Copyright 2010 Eric Davis, All rights reserved. 1

Part I. Redmine

Move recipients method to acts_as_event

Copyright 2010 Eric Davis, All rights reserved. 2

Chapter 1. Move recipients method toacts_as_eventStarting off the refactoring of Redmine, I used a tool called Flay1. Flay is a great tool to check for duplicated Ruby codeby searching across all of the files in a project.

Redmine has several model objects that generate events, which are listed on it’s activity page. Many of the events willsend out email notifications based on each user’s preferences. A few of these models had identical recipients methodsdefined, which caused a Flay score of 368 (bad). Using move method2 I was able to move 3 of the 4 methods intoRedmine’s acts_as_event plugin. The fourth will need some more refactoring before it could be moved.

Before

# vendor/plugins/acts_as_event/lib/acts_as_event.rb# ... nothing related to recipients

# app/models/news.rbclass News < ActiveRecord::Base # Returns the mail adresses of users that should be notified def recipients notified = project.notified_users notified.reject! {|user| !visible?(user)} notified.collect(&:mail) endend

# app/models/document.rbclass Document < ActiveRecord::Base # Returns the mail adresses of users that should be notified def recipients notified = project.notified_users notified.reject! {|user| !visible?(user)} notified.collect(&:mail) endend

# app/models/message.rbclass Message < ActiveRecord::Base # Returns the mail adresses of users that should be notified def recipients notified = project.notified_users notified.reject! {|user| !visible?(user)} notified.collect(&:mail) endend

1 http://ruby.sadi.st/Flay.html2 http://www.refactoring.com/catalog/moveMethod.html

Move recipients method to acts_as_event

Copyright 2010 Eric Davis, All rights reserved. 3

After# vendor/plugins/acts_as_event/lib/acts_as_event.rb # Returns the mail adresses of users that should be notified def recipients # Moved to new shared method notified = project.notified_users notified.reject! {|user| !visible?(user)} notified.collect(&:mail) end

# app/models/news.rbclass News < ActiveRecord::Base # ...end

# app/models/document.rbclass Document < ActiveRecord::Base # ...end

# app/models/message.rbclass Message < ActiveRecord::Base # ...end

While this refactor is pretty simple, duplication in a large system like Redmine can really affect development. That’swhy Test Driven Development3 has that final refactor step at the end of every cycle. Now changes to Redmine’s eventnotification system are easier to make and less prone to duplication bugs.

Reference commit4

3 http://en.wikipedia.org/wiki/Test-driven_development4 http://github.com/edavis10/redmine/commit/358e3194d79b9e4503edd48d2179b9f8f2920cd3

Extract method in fetch_changesets

Copyright 2010 Eric Davis, All rights reserved. 4

Chapter 2. Extract method infetch_changesetsStill working off of the flay report for Redmine1, I was able to find another refactoring in the fetch_changesets method.

Redmine supports connecting to seven different source control systems to import their changes 2. When Redmine runsthe import, it adds each Changeset (a commit) into the database and also information about each Change (e.g. filemodification, file added, etc) using the fetch_changesets method.

The fetch_changesets method for the darcs, mercurial, and subversion repositories all used the same code to adda new Change to the database.

Before

# app/models/changeset.rbclass Changeset < ActiveRecord::Base # ...end

# app/models/repository/darcs.rbclass Repository::Darcs < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| Change.create(:changeset => changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end # ...end

# app/models/repository/mercurial.rbclass Repository::Mercurial < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| Change.create(:changeset => changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end # ...end

1 http://www.redmine.org2git [http://git-scm.com/], subversion [http://subversion.tigris.org/], mercurial [http://mercurial.selenic.com/], darcs [http://darcs.net/], CVS [http://www.nongnu.org/cvs/], Bazaar [http://bazaar.canonical.com], and a simple File System based one

Extract method in fetch_changesets

Copyright 2010 Eric Davis, All rights reserved. 5

# app/models/repository/subversion.rbclass Repository::Subversion < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| Change.create(:changeset => changeset, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) end unless changeset.new_record? # ...end

After# app/models/changeset.rbclass Changeset < ActiveRecord::Base # Creates a new Change from it's common parameters def create_change(change) # New method Change.create(:changeset => self, :action => change[:action], :path => change[:path], :from_path => change[:from_path], :from_revision => change[:from_revision]) endend

# app/models/repository/darcs.rbclass Repository::Darcs < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) # Use new method end # ...end

# app/models/repository/mercurial.rbclass Repository::Mercurial < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) # Use new method end # ...end

# app/models/repository/subversion.rbclass Repository::Subversion < ActiveRecord::Base # Deep inside the fetch_changesets method revision.paths.each do |change| changeset.create_change(change) # Use new method end unless changeset.new_record? # ...end

Even though this duplication looks like it was caused by a copy and paste, notice that the Subversion version has anextra guard to make sure the Changeset was saved. That tells me that at some point in it’s history, there was a bug thatneeded the guard in Subversion. Sure enough git blame shows that exact conclusion in commit 7bd4590cd3.

So why wasn’t it ported to the other versions? Might they have similar bugs?

Reference commit4

3 http://github.com/edavis10/redmine/commit/7bd4590cd4 http://github.com/edavis10/redmine/commit/39c585740d45cbe50e3fed8a58834eb82532bea0

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 6

Chapter 3. Move method in theReportsControllerThis is the first part to a larger refactoring of the ReportsController in Redmine. This controller is responsible for calcu-lating the total number of issues based on different criteria (e.g. status, priority). There are two big problems with thecurrent code:

1. It has a lot of duplication, both in the main methods and the utility methods

2. It has raw query methods in the controller (ActiveRecord::Base.connection.select_all)

So the first step will be to move all of the raw query methods out of the controller and into the Issue model where theybelong.

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 7

Before

# app/controllers/reports_controller.rb def issues_by_tracker @issues_by_tracker ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, t.id as tracker_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t where i.status_id=s.id and i.tracker_id=t.id and i.project_id=#{@project.id} group by s.id, s.is_closed, t.id") end

def issues_by_version @issues_by_version ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, v.id as fixed_version_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v where i.status_id=s.id and i.fixed_version_id=v.id and i.project_id=#{@project.id} group by s.id, s.is_closed, v.id") end

def issues_by_priority @issues_by_priority ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, p.id as priority_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p where i.status_id=s.id and i.priority_id=p.id and i.project_id=#{@project.id} group by s.id, s.is_closed, p.id") end

def issues_by_category @issues_by_category ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, c.id as category_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c where i.status_id=s.id and i.category_id=c.id and i.project_id=#{@project.id} group by s.id, s.is_closed, c.id") end

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 8

# app/controllers/reports_controller.rb def issues_by_assigned_to @issues_by_assigned_to ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as assigned_to_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.assigned_to_id=a.id and i.project_id=#{@project.id} group by s.id, s.is_closed, a.id") end

def issues_by_author @issues_by_author ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as author_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.author_id=a.id and i.project_id=#{@project.id} group by s.id, s.is_closed, a.id") end

def issues_by_subproject @issues_by_subproject ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, i.project_id as project_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s where i.status_id=s.id and i.project_id IN (#{@project.descendants.active.collect{|p| p.id}.join(',')}) group by s.id, s.is_closed, i.project_id") if @project.descendants.active.any? @issues_by_subproject ||= [] end

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 9

After

# app/controllers/reports_controller.rb

# Find project of id params[:id] def find_project @project = Project.find(params[:id]) rescue ActiveRecord::RecordNotFound render_404 end

# Use new methods def issues_by_tracker @issues_by_tracker ||= Issue.by_tracker(@project) end

def issues_by_version @issues_by_version ||= Issue.by_version(@project) end

def issues_by_priority @issues_by_priority ||= Issue.by_priority(@project) end

def issues_by_category @issues_by_category ||= Issue.by_category(@project) end

def issues_by_assigned_to @issues_by_assigned_to ||= Issue.by_assigned_to(@project) end

def issues_by_author @issues_by_author ||= Issue.by_author(@project) end

def issues_by_subproject @issues_by_subproject ||= Issue.by_subproject(@project) @issues_by_subproject ||= [] endend

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 10

# app/models/issue.rbclass Issue < ActiveRecord::Base # Extracted from the ReportsController. # TODO: refactor into a common factory or named scopes def self.by_tracker(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, t.id as tracker_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t where i.status_id=s.id and i.tracker_id=t.id and i.project_id=#{project.id} group by s.id, s.is_closed, t.id") end

def self.by_version(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, v.id as fixed_version_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v where i.status_id=s.id and i.fixed_version_id=v.id and i.project_id=#{project.id} group by s.id, s.is_closed, v.id") end

def self.by_priority(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, p.id as priority_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p where i.status_id=s.id and i.priority_id=p.id and i.project_id=#{project.id} group by s.id, s.is_closed, p.id") end

def self.by_category(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, c.id as category_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c where i.status_id=s.id and i.category_id=c.id and i.project_id=#{project.id} group by s.id, s.is_closed, c.id") end

Move method in the ReportsController

Copyright 2010 Eric Davis, All rights reserved. 11

def self.by_assigned_to(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as assigned_to_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.assigned_to_id=a.id and i.project_id=#{project.id} group by s.id, s.is_closed, a.id") end

def self.by_author(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as author_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.author_id=a.id and i.project_id=#{project.id} group by s.id, s.is_closed, a.id") end

def self.by_subproject(project) # New method on model ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, i.project_id as project_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s where i.status_id=s.id and i.project_id IN (#{project.descendants.active.collect{|p| p.id}.join(',')}) group by s.id, s.is_closed, i.project_id") if project.descendants.active.any? end # End ReportsController extraction

end

This refactor alone moved over 30% of the code to the model and makes it easier to see that it is only one public methodin this entire controller. There is still a bit that needs to be done in the Issue model, since all I really did was to movesome code duplication down to the model. But now that the model has all this code, it’s easier to see some similaritiesin the methods in order to extract it to a utility method.

Reference commit1

1 http://github.com/edavis10/redmine/commit/b86b9b898e4dd5c0d9eb2d7362c47a6a513b2015

Move Issue#by methods

Copyright 2010 Eric Davis, All rights reserved. 12

Chapter 4. Move Issue#by methodsLast chapter’s refactoring to the ReportsController helped to move some of the ActiveRecord::Base#select_allmethods out of the controller, but I was still left with the duplication in the Issue model. Now I can dig into those modelmethods and remove most of the duplication.

Before

# app/models/issue.rbclass Issue < ActiveRecord::Base def self.by_tracker(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, t.id as tracker_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t where i.status_id=s.id and i.tracker_id=t.id and i.project_id=#{project.id} group by s.id, s.is_closed, t.id") end

def self.by_version(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, v.id as fixed_version_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v where i.status_id=s.id and i.fixed_version_id=v.id and i.project_id=#{project.id} group by s.id, s.is_closed, v.id") end

def self.by_priority(project) # More duplication end

def self.by_category(project) # More duplication end

def self.by_assigned_to(project) # More duplication end

def self.by_author(project) # More duplication end

Move Issue#by methods

Copyright 2010 Eric Davis, All rights reserved. 13

After

# app/models/issue.rbclass Issue < ActiveRecord::Base # Using the new shared method def self.by_tracker(project) count_and_group_by(:project => project, :field => 'tracker_id', :joins => Tracker.table_name) end

def self.by_version(project) count_and_group_by(:project => project, :field => 'fixed_version_id', :joins => Version.table_name) end

def self.by_priority(project) count_and_group_by(:project => project, :field => 'priority_id', :joins => IssuePriority.table_name) end

def self.by_category(project) count_and_group_by(:project => project, :field => 'category_id', :joins => IssueCategory.table_name) end

def self.by_assigned_to(project) count_and_group_by(:project => project, :field => 'assigned_to_id', :joins => User.table_name) end

def self.by_author(project) count_and_group_by(:project => project, :field => 'author_id', :joins => User.table_name) end

Move Issue#by methods

Copyright 2010 Eric Davis, All rights reserved. 14

# app/models/issue.rb

private # New extracted method # Query generator for selecting groups of issue counts for a project # based on specific criteria # # Options # * project - Project to search in. # * field - String. Issue field to key off of in the grouping. # * joins - String. The table name to join against. def self.count_and_group_by(options) project = options.delete(:project) select_field = options.delete(:field) joins = options.delete(:joins)

where = "i.#{select_field}=j.id"

ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, j.id as #{select_field}, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{joins} as j where i.status_id=s.id and #{where} and i.project_id=#{project.id} group by s.id, s.is_closed, j.id") endend

The big duplication was from the six select_all statements that only differed in what fields are used and what tableto join. I was able to slim down the public methods by:

1. moving the general code1 to a separate method

2. providing parameters for the parts of the statement that would change

3. building the select_all statement based on a simpler set of options

Now it would be trivial to add new public methods to count and group based on other fields. Also if the SQL needs tobe rewritten at any point in the future, it only needs to be done in one place, not six.

Next I’ll be heading back up to the controllers to refactor some more duplication up there.

Reference commit2

2 http://github.com/edavis10/redmine/commit/5bd912e9a2601b00d564a475d37eafb8942cb687

Pull up the #find_project method to the ApplicationController

Copyright 2010 Eric Davis, All rights reserved. 15

Chapter 5. Pull up the #find_project methodto the ApplicationControllerBack up in the ReportsController, there is still some duplication that needs to be resolved. There are nine methods:

• 1 controller action (issue_report)

• 1 private before_filter method (find_project)

• 7 private utility methods

The controller action is ripe for a few refactorings, but I wanted to start with the find_project first. I used the pull upmethod1 to move find_project to the superclass (ApplicationController).

Before# app/controllers/reports_controller.rbclass ReportsController < ApplicationController menu_item :issues before_filter :find_project, :authorize

private # Find project of id params[:id] def find_project @project = Project.find(params[:id]) rescue ActiveRecord::RecordNotFound render_404 end

# ... other private methodsend

After# app/controllers/reports_controller.rbclass ReportsController < ApplicationController menu_item :issues before_filter :find_project, :authorize

private # find_project movedend

# app/controllers/application_controller.rbclass ApplicationController < ActionController::Base # Find project of id params[:id] def find_project # Pulled up @project = Project.find(params[:id]) rescue ActiveRecord::RecordNotFound render_404 endend

Just looking at the ReportsController, this refactoring doesn’t look that effective. But I didn’t include the four other con-trollers in Redmine that had the exact same find_project method. Or the dozen plugins where I’ve had to implementan identical find_project.

1 http://www.refactoring.com/catalog/pullUpMethod.html

Pull up the #find_project method to the ApplicationController

Copyright 2010 Eric Davis, All rights reserved. 16

So now ReportsController is starting to clean up quite nicely. Next I should be able to start refactoring the large controlleraction and clean up a lot of the internal duplication. I’m even thinking of splitting up the action2 a bit to remove thecase statement.

Reference commit3

2 http://github.com/edavis10/redmine/blob/master/app/controllers/reports_controller.rb#L253 http://github.com/edavis10/redmine/commit/e5d300af0a4d6703b043c05d4178353e7e252bf2