Commit 6c8532e8abd691b55be7675d19e7d3ed17d4fa18
Exists in
master
and in
1 other branch
Merge pull request #300 from lest/patch-1
refactor apps sorting
Showing
6 changed files
with
44 additions
and
32 deletions
Show diff stats
app/controllers/apps_controller.rb
... | ... | @@ -52,23 +52,7 @@ class AppsController < InheritedResources::Base |
52 | 52 | |
53 | 53 | protected |
54 | 54 | def collection |
55 | - @unresolved_counts, @problem_counts = {}, {} | |
56 | - @apps ||= begin | |
57 | - apps = end_of_association_chain.all | |
58 | - | |
59 | - # Cache counts for unresolved errs and problems | |
60 | - apps.each do |app| | |
61 | - @unresolved_counts[app.id] ||= app.problems.unresolved.count | |
62 | - @problem_counts[app.id] ||= app.problems.count | |
63 | - end | |
64 | - | |
65 | - # Sort apps by number of unresolved errs, then problem counts. | |
66 | - apps.sort do |a,b| | |
67 | - (@unresolved_counts[b.id] <=> @unresolved_counts[a.id]).nonzero? || | |
68 | - (@problem_counts[b.id] <=> @problem_counts[a.id]).nonzero? || | |
69 | - a.name <=> b.name | |
70 | - end | |
71 | - end | |
55 | + @apps ||= end_of_association_chain.all.sort | |
72 | 56 | end |
73 | 57 | |
74 | 58 | def initialize_subclassed_issue_tracker | ... | ... |
app/models/app.rb
1 | 1 | class App |
2 | 2 | include Mongoid::Document |
3 | 3 | include Mongoid::Timestamps |
4 | + include Comparable | |
4 | 5 | |
5 | 6 | field :name, :type => String |
6 | 7 | field :api_key |
... | ... | @@ -169,6 +170,21 @@ class App |
169 | 170 | end |
170 | 171 | end |
171 | 172 | |
173 | + def unresolved_count | |
174 | + @unresolved_count ||= problems.unresolved.count | |
175 | + end | |
176 | + | |
177 | + def problem_count | |
178 | + @problem_count ||= problems.count | |
179 | + end | |
180 | + | |
181 | + # Compare by number of unresolved errs, then problem counts. | |
182 | + def <=>(other) | |
183 | + (other.unresolved_count <=> unresolved_count).nonzero? || | |
184 | + (other.problem_count <=> problem_count).nonzero? || | |
185 | + name <=> other.name | |
186 | + end | |
187 | + | |
172 | 188 | protected |
173 | 189 | |
174 | 190 | def store_cached_attributes_on_problems | ... | ... |
app/views/apps/index.html.haml
... | ... | @@ -47,8 +47,8 @@ |
47 | 47 | - revision = app.deploys.last.short_revision |
48 | 48 | = link_to( app.last_deploy_at.to_s(:micro) << (revision.present? ? " (#{revision})" : ""), app_deploys_path(app)) |
49 | 49 | %td.count |
50 | - - if @problem_counts[app.id] > 0 | |
51 | - - unresolved = @unresolved_counts[app.id] | |
50 | + - if app.problem_count > 0 | |
51 | + - unresolved = app.unresolved_count | |
52 | 52 | = link_to unresolved, app_path(app), :class => (unresolved == 0 ? "resolved" : nil) |
53 | 53 | - if @apps.none? |
54 | 54 | %tr | ... | ... |
spec/controllers/apps_controller_spec.rb
... | ... | @@ -31,17 +31,6 @@ describe AppsController do |
31 | 31 | assigns(:apps).should_not include(unwatched_app) |
32 | 32 | end |
33 | 33 | end |
34 | - | |
35 | - context 'when there is only one app' do | |
36 | - it 'sets unresolved_counts and problem_counts variables' do | |
37 | - sign_in Fabricate(:admin) | |
38 | - app = Fabricate(:app) | |
39 | - get :index | |
40 | - | |
41 | - assigns(:unresolved_counts).should == {app.id => 0} | |
42 | - assigns(:problem_counts).should == {app.id => 0} | |
43 | - end | |
44 | - end | |
45 | 34 | end |
46 | 35 | |
47 | 36 | describe "GET /apps/:id" do | ... | ... |
spec/models/app_spec.rb
... | ... | @@ -23,6 +23,31 @@ describe App do |
23 | 23 | end |
24 | 24 | end |
25 | 25 | |
26 | + describe '<=>' do | |
27 | + it 'is compared by unresolved count' do | |
28 | + app_0 = stub_model(App, :name => 'app', :unresolved_count => 1, :problem_count => 1) | |
29 | + app_1 = stub_model(App, :name => 'app', :unresolved_count => 0, :problem_count => 1) | |
30 | + | |
31 | + app_0.should < app_1 | |
32 | + app_1.should > app_0 | |
33 | + end | |
34 | + | |
35 | + it 'is compared by problem count' do | |
36 | + app_0 = stub_model(App, :name => 'app', :unresolved_count => 0, :problem_count => 1) | |
37 | + app_1 = stub_model(App, :name => 'app', :unresolved_count => 0, :problem_count => 0) | |
38 | + | |
39 | + app_0.should < app_1 | |
40 | + app_1.should > app_0 | |
41 | + end | |
42 | + | |
43 | + it 'is compared by name' do | |
44 | + app_0 = stub_model(App, :name => 'app_0', :unresolved_count => 0, :problem_count => 0) | |
45 | + app_1 = stub_model(App, :name => 'app_1', :unresolved_count => 0, :problem_count => 0) | |
46 | + | |
47 | + app_0.should < app_1 | |
48 | + app_1.should > app_0 | |
49 | + end | |
50 | + end | |
26 | 51 | |
27 | 52 | context 'being created' do |
28 | 53 | it 'generates a new api-key' do | ... | ... |
spec/views/apps/index.html.haml_spec.rb
... | ... | @@ -4,8 +4,6 @@ describe "apps/index.html.haml" do |
4 | 4 | before do |
5 | 5 | app = stub_model(App, :deploys => [stub_model(Deploy, :created_at => Time.now, :revision => "123456789abcdef")]) |
6 | 6 | assign :apps, [app] |
7 | - assign :problem_counts, {app.id => 0} | |
8 | - assign :unresolved_counts, {app.id => 0} | |
9 | 7 | controller.stub(:current_user) { stub_model(User) } |
10 | 8 | end |
11 | 9 | ... | ... |