Commit e549876493e453971c28cd048c333f0e56f59cb9
1 parent
03c9a613
Exists in
master
and in
1 other branch
add the ability to sort the errs table by its
headers; - cache sortable attributes on Problem - index sortable attributes for performance
Showing
8 changed files
with
118 additions
and
12 deletions
Show diff stats
app/controllers/apps_controller.rb
| @@ -8,10 +8,14 @@ class AppsController < InheritedResources::Base | @@ -8,10 +8,14 @@ class AppsController < InheritedResources::Base | ||
| 8 | respond_to do |format| | 8 | respond_to do |format| |
| 9 | format.html do | 9 | format.html do |
| 10 | @all_errs = !!params[:all_errs] | 10 | @all_errs = !!params[:all_errs] |
| 11 | + @sort = params[:sort] | ||
| 12 | + @order = params[:order] | ||
| 13 | + @sort = "app" unless %w{message last_notice_at last_deploy_at count}.member?(@sort) | ||
| 14 | + @order = "asc" unless (@order == "desc") | ||
| 11 | 15 | ||
| 12 | @problems = resource.problems | 16 | @problems = resource.problems |
| 13 | @problems = @problems.unresolved unless @all_errs | 17 | @problems = @problems.unresolved unless @all_errs |
| 14 | - @problems = @problems.in_env(params[:environment]).ordered.paginate(:page => params[:page], :per_page => current_user.per_page) | 18 | + @problems = @problems.in_env(params[:environment]).ordered_by(@sort, @order).paginate(:page => params[:page], :per_page => current_user.per_page) |
| 15 | 19 | ||
| 16 | @selected_problems = params[:problems] || [] | 20 | @selected_problems = params[:problems] || [] |
| 17 | @deploys = @app.deploys.order_by(:created_at.desc).limit(5) | 21 | @deploys = @app.deploys.order_by(:created_at.desc).limit(5) |
app/controllers/errs_controller.rb
| @@ -9,7 +9,13 @@ class ErrsController < ApplicationController | @@ -9,7 +9,13 @@ class ErrsController < ApplicationController | ||
| 9 | 9 | ||
| 10 | def index | 10 | def index |
| 11 | app_scope = current_user.admin? ? App.all : current_user.apps | 11 | app_scope = current_user.admin? ? App.all : current_user.apps |
| 12 | - @problems = Problem.for_apps(app_scope).in_env(params[:environment]).unresolved.ordered | 12 | + |
| 13 | + @sort = params[:sort] | ||
| 14 | + @order = params[:order] | ||
| 15 | + @sort = "app" unless %w{message last_notice_at last_deploy_at count}.member?(@sort) | ||
| 16 | + @order = "asc" unless (@order == "desc") | ||
| 17 | + | ||
| 18 | + @problems = Problem.for_apps(app_scope).in_env(params[:environment]).unresolved.ordered_by(@sort, @order) | ||
| 13 | @selected_problems = params[:problems] || [] | 19 | @selected_problems = params[:problems] || [] |
| 14 | respond_to do |format| | 20 | respond_to do |format| |
| 15 | format.html do | 21 | format.html do |
| @@ -0,0 +1,14 @@ | @@ -0,0 +1,14 @@ | ||
| 1 | +# encoding: utf-8 | ||
| 2 | +module SortHelper | ||
| 3 | + | ||
| 4 | + def link_for_sort(name, field=nil) | ||
| 5 | + field ||= name.underscore | ||
| 6 | + current = (@sort == field) | ||
| 7 | + order = (current && (@order == "asc")) ? "desc" : "asc" | ||
| 8 | + url = request.path + "?sort=#{field}&order=#{order}" | ||
| 9 | + options = {} | ||
| 10 | + options.merge(:class => "current") if current | ||
| 11 | + link_to(name, url, options) | ||
| 12 | + end | ||
| 13 | + | ||
| 14 | +end |
app/models/app.rb
| @@ -28,6 +28,7 @@ class App | @@ -28,6 +28,7 @@ class App | ||
| 28 | 28 | ||
| 29 | before_validation :generate_api_key, :on => :create | 29 | before_validation :generate_api_key, :on => :create |
| 30 | before_save :normalize_github_url | 30 | before_save :normalize_github_url |
| 31 | + after_update :store_cached_attributes_on_problems | ||
| 31 | 32 | ||
| 32 | validates_presence_of :name, :api_key | 33 | validates_presence_of :name, :api_key |
| 33 | validates_uniqueness_of :name, :allow_blank => true | 34 | validates_uniqueness_of :name, :allow_blank => true |
| @@ -148,7 +149,11 @@ class App | @@ -148,7 +149,11 @@ class App | ||
| 148 | end | 149 | end |
| 149 | 150 | ||
| 150 | protected | 151 | protected |
| 151 | - | 152 | + |
| 153 | + def store_cached_attributes_on_problems | ||
| 154 | + problems.each(&:cache_app_attributes) | ||
| 155 | + end | ||
| 156 | + | ||
| 152 | def generate_api_key | 157 | def generate_api_key |
| 153 | self.api_key ||= ActiveSupport::SecureRandom.hex | 158 | self.api_key ||= ActiveSupport::SecureRandom.hex |
| 154 | end | 159 | end |
app/models/deploy.rb
| @@ -14,6 +14,7 @@ class Deploy | @@ -14,6 +14,7 @@ class Deploy | ||
| 14 | 14 | ||
| 15 | after_create :deliver_notification, :if => :should_notify? | 15 | after_create :deliver_notification, :if => :should_notify? |
| 16 | after_create :resolve_app_errs, :if => :should_resolve_app_errs? | 16 | after_create :resolve_app_errs, :if => :should_resolve_app_errs? |
| 17 | + after_create :store_cached_attributes_on_problems | ||
| 17 | 18 | ||
| 18 | validates_presence_of :username, :environment | 19 | validates_presence_of :username, :environment |
| 19 | 20 | ||
| @@ -39,5 +40,8 @@ class Deploy | @@ -39,5 +40,8 @@ class Deploy | ||
| 39 | app.resolve_errs_on_deploy? | 40 | app.resolve_errs_on_deploy? |
| 40 | end | 41 | end |
| 41 | 42 | ||
| 43 | + def store_cached_attributes_on_problems | ||
| 44 | + Problem.where(:app_id => app.id).each(&:cache_app_attributes) | ||
| 45 | + end | ||
| 46 | + | ||
| 42 | end | 47 | end |
| 43 | - |
app/models/problem.rb
| @@ -7,18 +7,24 @@ class Problem | @@ -7,18 +7,24 @@ class Problem | ||
| 7 | include Mongoid::Timestamps | 7 | include Mongoid::Timestamps |
| 8 | 8 | ||
| 9 | field :last_notice_at, :type => DateTime | 9 | field :last_notice_at, :type => DateTime |
| 10 | + field :last_deploy_at, :type => Time | ||
| 10 | field :resolved, :type => Boolean, :default => false | 11 | field :resolved, :type => Boolean, :default => false |
| 11 | field :issue_link, :type => String | 12 | field :issue_link, :type => String |
| 12 | 13 | ||
| 13 | # Cached fields | 14 | # Cached fields |
| 15 | + field :app_name, :type => String | ||
| 14 | field :notices_count, :type => Integer, :default => 0 | 16 | field :notices_count, :type => Integer, :default => 0 |
| 15 | field :message | 17 | field :message |
| 16 | field :environment | 18 | field :environment |
| 17 | field :klass | 19 | field :klass |
| 18 | field :where | 20 | field :where |
| 19 | 21 | ||
| 20 | - index :last_notice_at | ||
| 21 | index :app_id | 22 | index :app_id |
| 23 | + index :app_name | ||
| 24 | + index :message | ||
| 25 | + index :last_notice_at | ||
| 26 | + index :last_deploy_at | ||
| 27 | + index :notices_count | ||
| 22 | 28 | ||
| 23 | belongs_to :app | 29 | belongs_to :app |
| 24 | has_many :errs, :inverse_of => :problem, :dependent => :destroy | 30 | has_many :errs, :inverse_of => :problem, :dependent => :destroy |
| @@ -29,6 +35,10 @@ class Problem | @@ -29,6 +35,10 @@ class Problem | ||
| 29 | scope :ordered, order_by(:last_notice_at.desc) | 35 | scope :ordered, order_by(:last_notice_at.desc) |
| 30 | scope :for_apps, lambda {|apps| where(:app_id.in => apps.all.map(&:id))} | 36 | scope :for_apps, lambda {|apps| where(:app_id.in => apps.all.map(&:id))} |
| 31 | 37 | ||
| 38 | + before_create :cache_app_attributes | ||
| 39 | + | ||
| 40 | + | ||
| 41 | + | ||
| 32 | def self.in_env(env) | 42 | def self.in_env(env) |
| 33 | env.present? ? where(:environment => env) : scoped | 43 | env.present? ? where(:environment => env) : scoped |
| 34 | end | 44 | end |
| @@ -67,6 +77,18 @@ class Problem | @@ -67,6 +77,18 @@ class Problem | ||
| 67 | merged_problem | 77 | merged_problem |
| 68 | end | 78 | end |
| 69 | 79 | ||
| 80 | + def self.ordered_by(sort, order) | ||
| 81 | + case sort | ||
| 82 | + when "app"; order_by(["app_name", order]) | ||
| 83 | + when "message"; order_by(["message", order]) | ||
| 84 | + when "last_notice_at"; order_by(["last_notice_at", order]) | ||
| 85 | + when "last_deploy_at"; order_by(["last_deploy_at", order]) | ||
| 86 | + when "count"; order_by(["notices_count", order]) | ||
| 87 | + else raise("\"#{sort}\" is not a recognized sort") | ||
| 88 | + end | ||
| 89 | + end | ||
| 90 | + | ||
| 91 | + | ||
| 70 | def merged? | 92 | def merged? |
| 71 | errs.length > 1 | 93 | errs.length > 1 |
| 72 | end | 94 | end |
| @@ -85,9 +107,18 @@ class Problem | @@ -85,9 +107,18 @@ class Problem | ||
| 85 | 107 | ||
| 86 | def reset_cached_attributes | 108 | def reset_cached_attributes |
| 87 | update_attribute(:notices_count, notices.count) | 109 | update_attribute(:notices_count, notices.count) |
| 110 | + cache_app_attributes | ||
| 88 | cache_notice_attributes | 111 | cache_notice_attributes |
| 89 | end | 112 | end |
| 90 | 113 | ||
| 114 | + def cache_app_attributes | ||
| 115 | + if app | ||
| 116 | + self.app_name = app.name | ||
| 117 | + self.last_deploy_at = app.last_deploy_at | ||
| 118 | + self.save if persisted? | ||
| 119 | + end | ||
| 120 | + end | ||
| 121 | + | ||
| 91 | def cache_notice_attributes(notice=nil) | 122 | def cache_notice_attributes(notice=nil) |
| 92 | notice ||= notices.first | 123 | notice ||= notices.first |
| 93 | attrs = {:last_notice_at => notices.max(:created_at)} | 124 | attrs = {:last_notice_at => notices.max(:created_at)} |
app/views/errs/_table.html.haml
| @@ -3,11 +3,11 @@ | @@ -3,11 +3,11 @@ | ||
| 3 | %thead | 3 | %thead |
| 4 | %tr | 4 | %tr |
| 5 | %th | 5 | %th |
| 6 | - %th App | ||
| 7 | - %th What & Where | ||
| 8 | - %th Latest | ||
| 9 | - %th Deploy | ||
| 10 | - %th Count | 6 | + %th= link_for_sort "App" |
| 7 | + %th= link_for_sort "What & Where".html_safe, "message" | ||
| 8 | + %th= link_for_sort "Latest", "last_notice_at" | ||
| 9 | + %th= link_for_sort "Deploy", "last_deploy_at" | ||
| 10 | + %th= link_for_sort "Count" | ||
| 11 | %th Resolve | 11 | %th Resolve |
| 12 | %tbody | 12 | %tbody |
| 13 | - errs.each do |problem| | 13 | - errs.each do |problem| |
| @@ -24,7 +24,7 @@ | @@ -24,7 +24,7 @@ | ||
| 24 | = link_to problem.message, app_err_path(problem.app, problem) | 24 | = link_to problem.message, app_err_path(problem.app, problem) |
| 25 | %em= problem.where | 25 | %em= problem.where |
| 26 | %td.latest #{time_ago_in_words(last_notice_at problem)} ago | 26 | %td.latest #{time_ago_in_words(last_notice_at problem)} ago |
| 27 | - %td.deploy= problem.app.last_deploy_at ? problem.app.last_deploy_at.to_s(:micro) : 'n/a' | 27 | + %td.deploy= problem.last_deploy_at ? problem.last_deploy_at.to_s(:micro) : 'n/a' |
| 28 | %td.count= link_to problem.notices.count, app_err_path(problem.app, problem) | 28 | %td.count= link_to problem.notices.count, app_err_path(problem.app, problem) |
| 29 | %td.resolve= link_to image_tag("thumbs-up.png"), resolve_app_err_path(problem.app, problem), :title => "Resolve", :method => :put, :confirm => err_confirm, :class => 'resolve' if problem.unresolved? | 29 | %td.resolve= link_to image_tag("thumbs-up.png"), resolve_app_err_path(problem.app, problem), :title => "Resolve", :method => :put, :confirm => err_confirm, :class => 'resolve' if problem.unresolved? |
| 30 | - if errs.none? | 30 | - if errs.none? |
spec/models/problem_spec.rb
| @@ -131,7 +131,6 @@ describe Problem do | @@ -131,7 +131,6 @@ describe Problem do | ||
| 131 | 131 | ||
| 132 | 132 | ||
| 133 | context "notice counter cache" do | 133 | context "notice counter cache" do |
| 134 | - | ||
| 135 | before do | 134 | before do |
| 136 | @app = Factory(:app) | 135 | @app = Factory(:app) |
| 137 | @problem = Factory(:problem, :app => @app) | 136 | @problem = Factory(:problem, :app => @app) |
| @@ -158,4 +157,47 @@ describe Problem do | @@ -158,4 +157,47 @@ describe Problem do | ||
| 158 | end | 157 | end |
| 159 | 158 | ||
| 160 | 159 | ||
| 160 | + context "#app_name" do | ||
| 161 | + before do | ||
| 162 | + @app = Factory(:app) | ||
| 163 | + end | ||
| 164 | + | ||
| 165 | + it "is set when a problem is created" do | ||
| 166 | + problem = Factory(:problem, :app => @app) | ||
| 167 | + assert_equal @app.name, problem.app_name | ||
| 168 | + end | ||
| 169 | + | ||
| 170 | + it "is updated when an app is updated" do | ||
| 171 | + problem = Factory(:problem, :app => @app) | ||
| 172 | + lambda { | ||
| 173 | + @app.update_attributes!(:name => "Bar App") | ||
| 174 | + problem.reload | ||
| 175 | + }.should change(problem, :app_name).to("Bar App") | ||
| 176 | + end | ||
| 177 | + end | ||
| 178 | + | ||
| 179 | + | ||
| 180 | + context "#last_deploy_at" do | ||
| 181 | + before do | ||
| 182 | + @app = Factory(:app) | ||
| 183 | + @last_deploy = 10.days.ago.localtime.round(0) | ||
| 184 | + deploy = Factory(:deploy, :app => @app, :created_at => @last_deploy) | ||
| 185 | + end | ||
| 186 | + | ||
| 187 | + it "is set when a problem is created" do | ||
| 188 | + problem = Factory(:problem, :app => @app) | ||
| 189 | + assert_equal @last_deploy, problem.last_deploy_at | ||
| 190 | + end | ||
| 191 | + | ||
| 192 | + it "is updated when a deploy is created" do | ||
| 193 | + problem = Factory(:problem, :app => @app) | ||
| 194 | + next_deploy = 5.minutes.ago.localtime.round(0) | ||
| 195 | + lambda { | ||
| 196 | + @deploy = Factory(:deploy, :app => @app, :created_at => next_deploy) | ||
| 197 | + problem.reload | ||
| 198 | + }.should change(problem, :last_deploy_at).from(@last_deploy).to(next_deploy) | ||
| 199 | + end | ||
| 200 | + end | ||
| 201 | + | ||
| 202 | + | ||
| 161 | end | 203 | end |