Daily Refactor #8: Move method in the ReportsController

The Refactoring

This is the first part to a larger refactoring of the ReportsController in Redmine. This controller is responsible for calculating the total number of issues based on different criteria (e.g. issue status, issue priority). There are two big problems with the current 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 they belong.

(I’ve included the entire ReportsController to make it easier to see how much of it will be changed. Don’t let your eyes glaze over in awe over this code.)

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
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179

# app/controllers/reports_controller.rb
class ReportsController  'position')
 
    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      @trackers = @project.trackers
      @versions = @project.shared_versions.sort
      @priorities = IssuePriority.all
      @categories = @project.issue_categories
      @assignees = @project.members.collect { |m| m.user }.sort
      @authors = @project.members.collect { |m| m.user }.sort
      @subprojects = @project.descendants.active
      issues_by_tracker
      issues_by_version
      issues_by_priority
      issues_by_category
      issues_by_assigned_to
      issues_by_author
      issues_by_subproject
 
      render :template => "reports/issue_report"
    end
  end  
 
private
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])        
  rescue ActiveRecord::RecordNotFound
    render_404
  end
 
  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
 
  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
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
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103

# app/controllers/reports_controller.rb
class ReportsController  'position')
 
    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      @trackers = @project.trackers
      @versions = @project.shared_versions.sort
      @priorities = IssuePriority.all
      @categories = @project.issue_categories
      @assignees = @project.members.collect { |m| m.user }.sort
      @authors = @project.members.collect { |m| m.user }.sort
      @subprojects = @project.descendants.active
      issues_by_tracker
      issues_by_version
      issues_by_priority
      issues_by_category
      issues_by_assigned_to
      issues_by_author
      issues_by_subproject
 
      render :template => "reports/issue_report"
    end
  end  
 
private
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])        
  rescue ActiveRecord::RecordNotFound
    render_404
  end
 
  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 ||= []
  end
end
 
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
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103

# app/models/issue.rb
class Issue < ActiveRecord::Base
  # Extracted from the ReportsController.
  # TODO: refactor into a common factory or named scopes
  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)
    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)
    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
 
  def self.by_assigned_to(project)
    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)
    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)
    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

Review

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

Reference commit