[Bug #4332] Fixed node categories, failing nodes from being displayed…

… forever, many performance problems.

* Fixed failing nodes from being displayed forever.
* Fixed some performance problems by eliminating and optimizing queries.
* Added "currently successful", "currently failing", "ever successful", and "ever failed" categories; and removed "successful" and "failed" categories.
* Added colors to sidebar to indicate errors.
* Improved sidebar efficiency and eliminated redundant code.
* Removed unnecessary numbers next to sidebar section titles for groups and classes.

app/controllers/nodes_controller.rb

@@ -10,11 +10,11 @@ def index
   end
 
   def successful
-    scoped_index :successful
+    redirect_to nodes_path(:current => true.to_s, :successful => true.to_s)
   end
 
   def failed
-    scoped_index :failed
+    redirect_to nodes_path(:current => true.to_s, :successful => false.to_s)
   end
 
   def unreported
@@ -45,9 +45,19 @@ def resource
 
   # Render the index using the +scope_name+ (e.g. :successful for Node.successful).
   def scoped_index(scope_name=nil)
-    set_collection_ivar(end_of_association_chain.search(params[:q]).by_report_date)
-    set_collection_ivar(end_of_association_chain.send(scope_name)) if scope_name
     index! do |format|
+      scope = end_of_association_chain
+      if params[:q]
+        scope = scope.search(params[:q])
+      end
+      if scope_name
+        scope = scope.send(scope_name)
+      end
+      if params[:current] or params[:successful]
+        scope = scope.by_currentness_and_successfulness(params[:current].to_b, params[:successful].to_b)
+      end
+      set_collection_ivar(scope.with_last_report.by_report_date)
+
       format.html { render :index }
       format.yaml { render :text => collection.to_yaml, :content_type => 'application/x-yaml' }
       format.json { render :json => collection.to_json }

app/controllers/pages_controller.rb

@@ -2,7 +2,8 @@ class PagesController < ApplicationController
   def home
     @statuses = Status.by_interval(:limit => 30)
     @reports = Report.all(:limit => 20, :order => 'time ASC')
-    @failed_nodes = Node.failed
+
+    @currently_failing_nodes = Node.by_currentness_and_successfulness(true, false)
     @unreported_nodes = Node.unreported
     @no_longer_reporting_nodes = Node.no_longer_reporting
 

app/models/node.rb

@@ -32,9 +32,31 @@ def self.per_page; 20 end # Pagination
   fires :removed, :on => :destroy
 
   # RH:TODO: Denormalize last report status into nodes table.
-  named_scope :successful, :select => 'DISTINCT `nodes`.name, `nodes`.*', :joins => 'INNER JOIN reports on reports.time = reported_at', :conditions => 'reports.success = 1 AND (`nodes`.id = reports.node_id)', :order => "reported_at DESC"
-  named_scope :failed, :select => 'DISTINCT `nodes`.name, `nodes`.*', :joins => 'LEFT OUTER JOIN reports on reports.time = reported_at', :conditions => 'reports.success = 0 AND (`nodes`.id = reports.node_id)', :order => "reported_at DESC"
 
+  # Return nodes based on their currentness and successfulness.
+  #
+  # The terms are:
+  # * currentness: +true+ uses the latest report (current) and +false+ uses any report.
+  # * successfulness: +true+ means a successful report, +false+ a failing report.
+  #
+  # Thus:
+  # * current and successful: Return only nodes that are currently successful.
+  # * current and failing: Return only nodes that are currently failing.
+  # * non-current and successful: Return any nodes that ever had a successful report.
+  # * non-current and failing: Return any nodes that ever had a failing report.
+  named_scope :by_currentness_and_successfulness, lambda {|currentness, successfulness|
+    if currentness
+      { :conditions => ['nodes.success = ?', successfulness] }
+    else
+      {
+        :conditions => ['reports.success = ?', successfulness],
+        :joins => :reports,
+        :group => 'nodes.id',
+      }
+    end
+  }
+
+  # Return nodes that have never reported.
   named_scope :unreported, :conditions => {:reported_at => nil}
 
   # Seconds in the past since a node's last report for a node to be considered no longer reporting.
@@ -43,18 +65,24 @@ def self.per_page; 20 end # Pagination
   # Return nodes that haven't reported recently.
   named_scope :no_longer_reporting, :conditions => ['reported_at < ?', NO_LONGER_REPORTING_CUTOFF.ago]
 
-  def self.count_successful
-    successful.count(:name, :distinct => true)
+  def self.count_by_currentness_and_successfulness(currentness, successfulness)
+    # FIXME The #length call is inefficient, but how do I make #count work since it lacks support for :having?
+    # self.by_currentness_and_successfulness(currentness, successfulness).count(:id, :distinct => :id)
+    self.by_currentness_and_successfulness(currentness, successfulness).length
   end
 
-  def self.count_failed
-    failed.count(:name, :distinct => true)
+  def self.label_for_currentness_and_successfulness(currentness, successfulness)
+    return "#{currentness ? 'Currently' : 'Ever'} #{successfulness ? (currentness ? 'successful' : 'succeeded') : (currentness ? 'failing' : 'failed')}"
   end
 
   def self.count_unreported
     unreported.count
   end
 
+  def self.count_no_longer_reporting
+    no_longer_reporting.count
+  end
+
   def to_param
     name.to_s
   end

app/views/nodes/_search.haml

@@ -1,3 +1,5 @@
-- form_tag request.url, :class => 'search', :method => :get do
+- form_tag url_for(:action => action_name), :class => 'search', :method => :get do
   = text_field_tag 'q', params[:q]
+  = hidden_field_tag 'current', params[:current]
+  = hidden_field_tag 'successful', params[:successful]
   %button Search

app/views/nodes/index.html.haml

@@ -2,11 +2,19 @@
 #main
 
   .header
-    %h2.half== #{action_name.titleize unless action_name == 'index' } Nodes
+    %h2.half
+      - if action_name != 'index'
+        = action_name.titleize.capitalize
+        nodes
+      - elsif params[:current] or params[:successful]
+        = Node.label_for_currentness_and_successfulness(params[:current].to_b, params[:successful].to_b).capitalize
+        nodes
+      - else
+        Nodes
     = render 'search'
     %br.clear
 
-  .item{:style => 'margin-bottom: 0;'}
+  .item
     .section
       = render 'statuses/run_failure', :nodes => @nodes
     .section

app/views/pages/home.html.haml

@@ -3,28 +3,28 @@
   .header
     %h2 Dashboard
   .item
-    - unless @failed_nodes.empty?
+    - if @currently_failing_nodes.present?
       .section.error
-        %h3 Node Failures
+        %h3 Node failures
         %p
-          = pluralize @failed_nodes.length, 'node'
-          are reporting failures:
+          = pluralize @currently_failing_nodes.length, 'node'
+          are currently reporting failures:
           = succeed '.' do
-            = truncated_node_sentence(@failed_nodes, :more_link => failed_nodes_path)
+            = truncated_node_sentence(@currently_failing_nodes, :more_link => nodes_path(:current => true, :successful => false))
 
-    - unless @no_longer_reporting_nodes.empty?
+    - if @no_longer_reporting_nodes.present?
       .section.warning
-        %h3 Nodes Failed To Report
+        %h3 Nodes no longer reporting
         %p
           = pluralize @no_longer_reporting_nodes.length, 'node'
           = @no_longer_reporting_nodes.length == 1 ? 'has' : 'have'
           not reported in the last #{time_ago_in_words Node::NO_LONGER_REPORTING_CUTOFF.ago}:
           = succeed '.' do
             = truncated_node_sentence(@no_longer_reporting_nodes, :more_link => no_longer_reporting_nodes_path)
 
-    - unless @unreported_nodes.empty?
+    - if @unreported_nodes.present?
       .section.warning
-        %h3 Nodes Not Reporting
+        %h3 Nodes never reported
         %p
           = pluralize @unreported_nodes.length, 'node'
           = @unreported_nodes.length == 1 ? 'has' : 'have'

app/views/shared/_node_manager_sidebar.html.haml

@@ -2,36 +2,23 @@
   %h3{:class => active_if(controller_name == "nodes" && action_name == "index")}= link_to "Nodes", nodes_path
   %span.count= Node.count
   %ul
-    %li{:class => active_if(controller_name == 'nodes' && action_name == 'successful' && parent.nil?)}
-      = link_to "Successful", successful_nodes_path
-      %span.count= Node.count_successful
-    %li{:class => active_if(controller_name == 'nodes' && action_name == 'failed' && parent.nil?)}
-      = link_to "Failed", failed_nodes_path
-      %span.count= Node.count_failed
+    - for currentness in [true, false]
+      - for successfulness in [true, false]
+        %li{:class => active_if(controller_name == 'nodes' && params[:current] && params[:current].to_b == currentness && params[:successful] && params[:successful].to_b == successfulness && parent.nil?)}
+          - label = Node.label_for_currentness_and_successfulness(currentness, successfulness)
+          = link_to label, nodes_path(:current => currentness.to_s, :successful => successfulness.to_s)
+          - count = Node.count_by_currentness_and_successfulness(currentness, successfulness)
+          %span.count{:class => counter_class(count, currentness && !successfulness)}= count
     %li{:class => active_if(controller_name == 'nodes' && action_name == 'unreported' && parent.nil?)}
-      = link_to "Unreported", unreported_nodes_path
-      %span.count= Node.count_unreported
+      = link_to "Never reported", unreported_nodes_path
+      - count = Node.count_unreported
+      %span.count{:class => counter_class(count, true)}= count
+    %li{:class => active_if(controller_name == 'nodes' && action_name == 'no_longer_reporting' && parent.nil?)}
+      = link_to "Not currently reporting", no_longer_reporting_nodes_path
+      - count = Node.count_no_longer_reporting
+      %span.count{:class => counter_class(count, true)}= count
   .footer.actionbar
-    = link_to "Add Node", new_node_path, :class => 'button'
+    = link_to "Add node", new_node_path, :class => 'button'
 
-.group
-  %h3{:class => active_if(controller_name == "node_classes" && action_name == "index")}= link_to "Classes", node_classes_path
-  %span.count= NodeClass.count
-  %ul
-    - NodeClass.all(:include => :nodes).each do |node_class|
-      %li{:class => active_if(controller_name == "node_classes" && @node_class && @node_class == node_class)}
-        = link_to node_class.name, node_class_path(node_class)
-        %span.count= node_class.nodes.count
-  .footer.actionbar
-    = link_to "Add Class", new_node_class_path, :class => 'button'
-
-.group
-  %h3{:class => active_if(controller_name == "node_groups" && action_name == "index")}= link_to "Groups", node_groups_path
-  %span.count= NodeGroup.count
-  %ul
-    - NodeGroup.all(:include => :nodes).each do |node_group|
-      %li{:class => active_if(controller_name == "node_groups" && @node_group && @node_group == node_group)}
-        = link_to node_group.name, node_group_path(node_group)
-        %span.count= node_group.nodes.count
-  .footer.actionbar
-    = link_to "Add Group", new_node_group_path, :class => 'button'
+- for type in [NodeClass, NodeGroup]
+  = render "shared/node_manager_sidebar_for_type", :type => type

app/views/shared/_node_manager_sidebar_for_type.html.haml

@@ -0,0 +1,19 @@
+-# Render a sidebar section for a +type+ (e.g. NodeGroup or NodeClass).
+
+- table = type.name.tableize
+- label = type.name.sub(/Node/, '')
+- ivar = instance_variable_get "@#{table.singularize}"
+- path_for_index = "#{table}_path"
+- path_for_new = "new_#{table.singularize}_path"
+- path_for_show = "#{table.singularize}_path"
+
+.group
+  %h3{:class => active_if(controller_name == table && action_name == "index")}= link_to(label, send(path_for_index))
+  - entries = type.with_nodes_count
+  %ul
+    - for entry in entries
+      %li{:class => active_if(controller_name == table && ivar && ivar == entry)}
+        = link_to entry.name, send(path_for_show, entry)
+        %span.count= entry.nodes_count
+  .footer.actionbar
+    = link_to "Add #{label}", send(path_for_new), :class => 'button'