Updated Person#should_suppress to take into account alert level when

deciding to suppress an alert.

2 files changed, 174 insertions, 26 deletions

diff --git a/lib/mauve/person.rb b/lib/mauve/person.rb
index 1aa27ee..eb6e28b 100644
--- a/lib/mauve/person.rb
+++ b/lib/mauve/person.rb
@@ -106,39 +106,74 @@ module Mauve
 
     # Works out if a notification should be suppressed.  If no parameters are supplied, it will 
     #
+    # @param [Symbol] Level of notification that is being tested
     # @param [Time] Theoretical time of notification
     # @param [Time] Current time.
     # @return [Boolean] If suppression is needed.
-    def should_suppress?(with_notification_at = nil, now = Time.now)
-      #
-      # This is the query we use.  It doesn't get polled until later.
-      #
-      previous_notifications = History.all(:order => :created_at.desc, :user => self.username, :type => "notification", :event.like => '% succeeded', :fields => [:created_at])
+    def should_suppress?(level, with_notification_at = nil, now = Time.now)
 
-      #
-      # Find the latest alert.
-      #
-      if with_notification_at.nil?
-        latest_notification = previous_notifications.first
-        latest = (latest_notification.nil? ? nil : latest_notification.created_at)
-      else
-        latest = with_notification_at
-      end
+      self.suppress_notifications_after.any? do |period, number|
+        #
+        # When testing if the next alert will suppress, we know that if only
+        # one alert is needed to suppress, then this function should always
+        # return true.
+        #
+        return true if with_notification_at and number <= 1
 
-      return self.suppress_notifications_after.any? do |period, number|
         #
-        # If no notification time has been specified, use the earliest alert time.
+        # Here are the previous notifications set to this person in the last period.
         #
-        if with_notification_at.nil? or number == 0
-         earliest_notification = previous_notifications[number-1]
+        previous_notifications = History.all(
+          :user => self.username, :type => "notification", 
+          :created_at.gte => now - period, :created_at.lte => now,
+          :event.like => '% succeeded',
+          :order => :created_at.desc)
+
+        #
+        # Defintely not suppressed if no notifications have been found.
+        #
+        return false if previous_notifications.count == 0
+
+        #
+        # If we're suppressed already, we need to check the time of the last alert sent
+        #
+        if @suppressed
+
+          if with_notification_at.is_a?(Time)
+            latest = with_notification_at
+          else
+            latest = previous_notifications.first.created_at
+          end
+          
+          #
+          # We should not suppress this alert if the last one was sent ages ago
+          #
+          if (now - latest) >= period
+            return false
+          end 
+
         else
-         earliest_notification = previous_notifications[number-2]
+          #
+          # We do not suppress if we can't find a sufficient number of previous alerts
+          #
+          if previous_notifications.count < (with_notification_at.nil? ? number : number - 1)
+            return false
+          end
+
         end
 
-        earliest = (earliest_notification.nil? ? nil : earliest_notification.created_at)
+        #
+        # If we're at the lowest level, return true now.
+        #
+        return true if !AlertGroup::LEVELS.include?(level) or AlertGroup::LEVELS.index(level) == 0
+
+        #
+        # Suppress this notification if all of the preceeding notifications were of the same or higher level.
+        #
+        return previous_notifications.alerts.all? do |a|
+          AlertGroup::LEVELS.index(a.level) >= AlertGroup::LEVELS.index(level) 
+        end
 
-        (earliest.is_a?(Time) and (now - earliest) < period) or
-          (latest.is_a?(Time) and @suppressed and (now - latest) < period) 
       end
     end
    
@@ -236,11 +271,11 @@ module Mauve
     def send_alert(level, alert, now=Time.now)
 
       was_suppressed = @suppressed
-      @suppressed    = self.should_suppress?
-      will_suppress  = self.should_suppress?(now)
+      @suppressed    = self.should_suppress?(level)
+      will_suppress  = self.should_suppress?(level, now)
 
       logger.info "Starting to send notifications again for #{username}." if was_suppressed and not @suppressed
-      
+
       #
       # We only suppress notifications if we were suppressed before we started,
       # and are still suppressed.
diff --git a/test/tc_mauve_person.rb b/test/tc_mauve_person.rb
index 14798f1..f3910bf 100644
--- a/test/tc_mauve_person.rb
+++ b/test/tc_mauve_person.rb
@@ -101,7 +101,6 @@ EOF
         assert_equal(0, notification_buffer.length, "Notification sent when it should not have been at #{Time.now}.")
       end
 
-
       logger_pop
     end
 
@@ -183,6 +182,120 @@ EOF
     end
 
   end
+  
+  def test_send_alert_suppression_as_alerts_get_more_urgent
+    #
+    # This configuration is a bit different.  We only want one alert per
+    # minute.
+    #
+    config =<<EOF
+notification_method("email") {
+  debug!
+  deliver_to_queue []
+  disable_normal_delivery!
+}
+
+person ("test") {
+  email "test@example.com"
+  all { email }
+  suppress_notifications_after( 2 => 1.minute )
+}
+
+alert_group("low") {
+  level LOW
+  includes { alert_id =~ /^low-/  }
+
+  notify("test") {
+    every 10.seconds
+  } 
+}
+
+alert_group("normal") {
+  level NORMAL
+  includes { alert_id =~ /^normal-/ } 
+
+  notify("test") {
+    every 10.seconds
+  } 
+}
+
+alert_group("default") {
+  level URGENT
+
+  notify("test") {
+    every 10.seconds
+  } 
+}
+EOF
+  
+    Configuration.current = ConfigurationBuilder.parse(config)
+    notification_buffer = Configuration.current.notification_methods["email"].deliver_to_queue
+    Server.instance.setup
+
+    person = Configuration.current.people["test"]
+
+    alerts = [
+      Alert.new(
+        :alert_id  => "low-test",
+        :source    => "test",
+        :subject   => "test"
+      ),
+      Alert.new(
+        :alert_id  => "normal-test",
+        :source    => "test",
+        :subject   => "test"
+      ),
+      Alert.new(
+        :alert_id  => "urgent-test",
+        :source    => "test",
+        :subject   => "test"
+      )
+    ]
+
+    #
+    # Raise the alerts
+    #
+    alerts.each{|a| a.raise!}
+    assert_equal(false,    person.suppressed?, "Person suppressed before we even begin!")
+
+    assert_equal(:low, alerts[0].level)
+    assert_equal(:normal, alerts[1].level)
+    assert_equal(:urgent, alerts[2].level)
+
+    start_time = Time.now
+
+    #
+    # 
+    #
+    [ [0, true, alerts.first ],
+      [1, true, alerts.first ],
+      [2, false, alerts.first ],
+      [3, false, alerts.first ],
+      [4, true, alerts[1]],
+      [5, false, alerts.first],
+      [6, true, alerts[1]],
+    ].each do |offset, notification_sent, alert|
+      # 
+      # Advance in to the future!
+      #
+      Timecop.freeze(start_time + offset)
+
+      person.send_alert(alert.level, alert)
+
+      if notification_sent 
+        assert_equal(1, notification_buffer.length, "#{alert.level.to_s.capitalize} notification not sent when it should have been at #{Time.now}.")
+        #
+        # Pop the notification off the buffer.
+        #
+        notification_buffer.pop
+      else
+        assert_equal(0, notification_buffer.length, "#{alert.level.to_s.capitalize} notification sent when it should not have been at #{Time.now}.")
+      end
+
+      logger_pop
+    end
+
+  end
 
   def test_current_alerts