Update to suppression code.

XMPP interface fixed.

14 files changed, 151 insertions, 80 deletions

diff --git a/debian/changelog b/debian/changelog
index ec000a3..3aa6d08 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+mauvealert (3.5.5) stable; urgency=low
+
+  * Further updates to the suppression code.  Should be much better now.
+  * XMPP interface fixed.
+
+ -- Patrick J Cherry <patrick@bytemark.co.uk>  Tue, 06 Sep 2011 15:30:11 +0100
+
 mauvealert (3.5.4) stable; urgency=low
 
   * Fixed up alert suppression to work better
diff --git a/lib/mauve/alert.rb b/lib/mauve/alert.rb
index ee5c1ba..786ecab 100644
--- a/lib/mauve/alert.rb
+++ b/lib/mauve/alert.rb
@@ -24,7 +24,7 @@ module Mauve
     # http://www.mail-archive.com/datamapper@googlegroups.com/msg02314.html
     #
     def self.create_view!
-      the_distant_future = Time.now + 10000.days # it is the year 2000 - the humans are dead
+      the_distant_future = Time.now + 2000.days # it is the year 2000 - the humans are dead
       ["BEGIN TRANSACTION",
        "DROP VIEW IF EXISTS mauve_alert_earliest_dates",
        "CREATE VIEW 
diff --git a/lib/mauve/notifiers/email.rb b/lib/mauve/notifiers/email.rb
index d450149..3134135 100644
--- a/lib/mauve/notifiers/email.rb
+++ b/lib/mauve/notifiers/email.rb
@@ -52,7 +52,7 @@ module Mauve
         
         def prepare_message(destination, alert, all_alerts, conditions = {})
           was_suppressed = conditions[:was_suppressed] || false
-          is_suppressed  = conditions[:is_suppressed]  || false
+          will_suppress  = conditions[:will_suppress]  || false
           
           m = RMail::Message.new
          
diff --git a/lib/mauve/notifiers/sms_aql.rb b/lib/mauve/notifiers/sms_aql.rb
index 54a3104..82b83bc 100644
--- a/lib/mauve/notifiers/sms_aql.rb
+++ b/lib/mauve/notifiers/sms_aql.rb
@@ -52,7 +52,7 @@ module Mauve
         protected
         def prepare_message(destination, alert, all_alerts, conditions={})
           was_suppressed = conditions[:was_suppressed] || false
-          is_suppressed  = conditions[:is_suppressed]  || false
+          will_suppress  = conditions[:will_suppress]  || false
           
           template_file = File.join(File.dirname(__FILE__),"templates","sms.txt.erb")
 
diff --git a/lib/mauve/notifiers/templates/email.html.erb b/lib/mauve/notifiers/templates/email.html.erb
index fb33de6..d63f837 100644
--- a/lib/mauve/notifiers/templates/email.html.erb
+++ b/lib/mauve/notifiers/templates/email.html.erb
@@ -19,9 +19,9 @@ end
 <h2>Detail</h2>
 <div><%= RedCloth.new(alert.detail).to_html %></div>
 <hr />
-<% if was_suppressed and not is_suppressed %>
+<% if was_suppressed and not will_suppress %>
 <p><strong>Better now.  Your notifications have now restarted.</strong></p>
-<% elsif is_suppressed and not was_suppressed %>
+<% elsif will_suppress and not was_suppressed %>
 <p><strong>Too noisy!  Your notifications have been suppressed.</strong></p>
 <% end %>
 <address>-- <br />Love mauve<br />xxx.</address></body></html>
diff --git a/lib/mauve/notifiers/templates/email.txt.erb b/lib/mauve/notifiers/templates/email.txt.erb
index aab44a8..3ea7626 100644
--- a/lib/mauve/notifiers/templates/email.txt.erb
+++ b/lib/mauve/notifiers/templates/email.txt.erb
@@ -19,9 +19,9 @@ end
 
 -----------------------------------------------------------------------
 
-<% if was_suppressed and not is_suppressed 
+<% if was_suppressed and not will_suppress 
 %>Better now.  Your notifications have now restarted.<% 
-elsif is_suppressed and not was_suppressed 
+elsif will_suppress and not was_suppressed 
 %>Too noisy!  Your notifications have been suppressed.<% end %>
 
 --
diff --git a/lib/mauve/notifiers/templates/email_subject.txt.erb b/lib/mauve/notifiers/templates/email_subject.txt.erb
index 802c711..119c742 120000..100644
--- a/lib/mauve/notifiers/templates/email_subject.txt.erb
+++ b/lib/mauve/notifiers/templates/email_subject.txt.erb
@@ -1 +1,22 @@
-xmpp.txt.erb
\ No newline at end of file
+<%=alert.id %>: <%= alert.update_type.upcase %>: <% 
+case alert.update_type
+when "cleared"
+%><%= alert.cleared_at.to_s_relative %><% 
+when "acknowledged"
+%><%= alert.acknowledged_at.to_s_relative %> by <%= alert.acknowledged_by %> until <%= alert.will_unacknowledge_at.to_s_human %><% 
+else
+%><%= alert.raised_at.to_s_relative %><% 
+end 
+%>: <%= alert.subject %>: <%= alert.summary %><%
+if alert.source != alert.subject 
+%> -- from <%= alert.source %><%
+end 
+%>. <%=WebInterface.url_for(alert)%><%
+if defined? was_suppressed and defined? will_suppress
+  if was_suppressed and not will_suppress 
+%> (Normal service has resumed.)<%
+  elsif will_suppress and not was_suppressed
+%> (Further alerts suppressed until things calm down.)<%
+  end 
+end
+%>
diff --git a/lib/mauve/notifiers/templates/sms.txt.erb b/lib/mauve/notifiers/templates/sms.txt.erb
index 802c711..faec37d 120000..100644
--- a/lib/mauve/notifiers/templates/sms.txt.erb
+++ b/lib/mauve/notifiers/templates/sms.txt.erb
@@ -1 +1,22 @@
-xmpp.txt.erb
\ No newline at end of file
+<%= alert.update_type.upcase %>: <% 
+case alert.update_type
+when "cleared"
+%><%= alert.cleared_at.to_s_relative %><% 
+when "acknowledged"
+%><%= alert.acknowledged_at.to_s_relative %> by <%= alert.acknowledged_by %> until <%= alert.will_unacknowledge_at.to_s_human %><% 
+else
+%><%= alert.raised_at.to_s_relative %><% 
+end 
+%>: <%= alert.subject %>: <%= alert.summary %><%
+if alert.source != alert.subject 
+%> -- from <%= alert.source %><%
+end 
+%>. <%=WebInterface.url_for(alert)%><%
+if defined? was_suppressed and defined? will_suppress
+  if was_suppressed and not will_suppress 
+%> (Normal service has resumed.)<%
+  elsif will_suppress and not was_suppressed
+%> (Further alerts suppressed until things calm down.)<%
+  end 
+end
+%>
diff --git a/lib/mauve/notifiers/templates/xmpp.html.erb b/lib/mauve/notifiers/templates/xmpp.html.erb
index c6bfaed..7c0bbfd 100644
--- a/lib/mauve/notifiers/templates/xmpp.html.erb
+++ b/lib/mauve/notifiers/templates/xmpp.html.erb
@@ -1,5 +1,5 @@
   <html xmlns="http://jabber.org/protocol/xhtml-im"><body xmlns="OAhttp://www.w3.org/1999/xhtml">
-<a href="<%=WebInterface.url_for(alert)%>"><%= alert.update_type.upcase %></a>: <% 
+<a href="<%=WebInterface.url_for(alert)%>"><%= alert.id%>: <%= alert.update_type.upcase %></a>: <% 
 case alert.update_type
 when "cleared"
 %><%= alert.cleared_at.to_s_relative %><% 
@@ -8,14 +8,16 @@ when "acknowledged"
 else
 %><%= alert.raised_at.to_s_relative %><% 
 end 
-%>: <%= alert.subject %>: <%= alert.summary %><%
+%>: <strong><%= alert.subject %>:</strong> <%= alert.summary %><%
 if alert.source != alert.subject 
 %> -- from <%= alert.source %><%
 end 
 %>.<%
-if was_suppressed and not is_suppressed 
-%><em> Normal service has resumed.</em><%
-elsif is_suppressed and not was_suppressed
-%><em> Further alerts suppressed until things calm down.</em><%
-end 
+if defined? was_suppressed and defined? will_suppress
+  if was_suppressed and not will_suppress 
+%><br /><em>Normal service has resumed.</em><%
+  elsif will_suppress and not was_suppressed
+%><br /><em>Further alerts suppressed until things calm down.</em><%
+  end
+end
 %></body></html>
diff --git a/lib/mauve/notifiers/templates/xmpp.txt.erb b/lib/mauve/notifiers/templates/xmpp.txt.erb
index 57f16f4..119c742 100644
--- a/lib/mauve/notifiers/templates/xmpp.txt.erb
+++ b/lib/mauve/notifiers/templates/xmpp.txt.erb
@@ -1,4 +1,4 @@
-<%= alert.update_type.upcase %>: <% 
+<%=alert.id %>: <%= alert.update_type.upcase %>: <% 
 case alert.update_type
 when "cleared"
 %><%= alert.cleared_at.to_s_relative %><% 
@@ -12,10 +12,10 @@ if alert.source != alert.subject
 %> -- from <%= alert.source %><%
 end 
 %>. <%=WebInterface.url_for(alert)%><%
-if defined? was_suppressed and defined? is_suppressed
-  if was_suppressed and not is_suppressed 
+if defined? was_suppressed and defined? will_suppress
+  if was_suppressed and not will_suppress 
 %> (Normal service has resumed.)<%
-  elsif is_suppressed and not was_suppressed
+  elsif will_suppress and not was_suppressed
 %> (Further alerts suppressed until things calm down.)<%
   end 
 end
diff --git a/lib/mauve/notifiers/xmpp.rb b/lib/mauve/notifiers/xmpp.rb
index 6a94ba8..a2ca14d 100644
--- a/lib/mauve/notifiers/xmpp.rb
+++ b/lib/mauve/notifiers/xmpp.rb
@@ -196,7 +196,7 @@ module Mauve
           destination_jid = JID.new(destination)         
  
           was_suppressed = conditions[:was_suppressed] || false
-          is_suppressed  = conditions[:is_suppressed]  || false
+          will_suppress  = conditions[:will_suppress]  || false
           
           if conditions && !check_alert_conditions(destination_jid, conditions) 
             logger.info("Alert conditions not met, not sending XMPP alert to #{destination_jid}")
@@ -442,15 +442,14 @@ module Mauve
 
         def parse_command(msg)
           case msg.body
+            when /help(\s+\w+)?/i
+              do_parse_help(msg)
             when /show\s?/i
               do_parse_show(msg)
             when /ack/i
               do_parse_ack(msg)
-            when /(\w+\W+){5,}\w/
-              do_parse_help(msg)
-              File.executable?('/usr/games/fortune') ? `/usr/games/fortune -s -n 60`.chomp : "I'd love to stay and chat, but I'm really quite busy"
             else
-              do_parse_help(msg)
+              File.executable?('/usr/games/fortune') ? `/usr/games/fortune -s -n 60`.chomp : "I'd love to stay and chat, but I'm really quite busy"
           end          
         end
 
@@ -462,18 +461,30 @@ module Mauve
             when /^show/
                <<EOF
 Show command: Lists all raised or acknowledged alerts, or the first or last few.
+
 e.g.
   show  -- shows all raised alerts
   show ack -- shows all acknowledged alerts
   show first 10 acknowledged -- shows first 10 acknowledged
-  show last 5 raised -- shows last 5 raised alerts 
+  show last 5 raised -- shows last 5 raised alerts
 EOF
             when /^ack/
               <<EOF
-Acknowledge command: Acknowledges one or more alerts for a set period of time.  This can only be done from a "private" chat.
+Acknowledge command: Acknowledges one or more alerts for a set period of time.
+
+The syntax is
+
+  acknowledge <alert list> for <time period> because <note>
+
+ * The alert list is a comma separated list.
+ * The time period can be spefied in terms of days, hours, minutes, seconds,
+    which can be wall-clock (default), working, or daytime (see the examples).
+ * The note is appended to the acknowledgement.
+
 e.g.
-  ack 1 for 2 hours -- acknowledges alert no. 1 for 2 wall-clock hours
+  acknowledge 1 for 2 hours -- acknowledges alert no. 1 for 2 wall-clock hours
   ack 1,2,3 for 2 working hours -- acknowledges alerts 1, 2, and 3 for 2 working hours
+  ack 4 for 3 days because something bad happened -- acknowledge alert 4 for 3 wall-clock days with the note "something bad happened"
 EOF
             else
               "I am Mauve #{Mauve::VERSION}.  I understand \"help\", \"show\" and \"acknowledge\" commands.  Try \"help show\"."
@@ -518,28 +529,38 @@ EOF
           end 
 
           (["Alerts #{type}:"] + items.collect do |alert| 
-            "#{alert.id}: " + ERB.new(template).result(binding).chomp
+            ERB.new(template).result(binding).chomp
           end).join("\n")
         end
 
         def do_parse_ack(msg)
           return "Sorry -- I don't understand your acknowledge command." unless
-             msg.body =~ /ack(?:nowledge)?\s+([\d,]+)\s+for\s+(\d+)\s+(work(?:ing)|day(?:time)|wall(?:-?clock))?\s*hours?(?:\s(.*))?/i
+             msg.body =~ /ack(?:nowledge)?\s+([\d,]+)\s+for\s+(\d+(?:\.\d+)?)\s+(work(?:ing)?|day(?:time)?|wall(?:-?clock)?)?\s*(day|hour|min(?:ute)?|sec(?:ond))s?(?:\s+because\s+(.*))?/i
           
-          alerts, n_hours, type_hours, note = [$1,$2, $3, $4]
+          alerts, n_hours, type_hours, dhms, note = [$1,$2, $3, $4, $5]
 
           alerts = alerts.split(",")
-          n_hours = n_hours.to_i
+
+          n_hours = case dhms
+            when /^day/
+              n_hours.to_f * 24.0
+            when /^min/
+              n_hours.to_f / 60.0
+            when /^sec/
+              n_hours.to_f / 3600.0
+            else
+              n_hours.to_f
+          end
 
           type_hours = case type_hours
-            when /^wall/
-              "wallclock"
+            when /^day/
+              "daytime"
             when /^work/
               "working"
             else
-              "daytime"
+              "wallclock"
           end
-          
+
           ack_until = Time.now.in_x_hours(n_hours, type_hours)
           username = get_username_for(msg.from)
 
diff --git a/lib/mauve/person.rb b/lib/mauve/person.rb
index c066a6d..0df31ee 100644
--- a/lib/mauve/person.rb
+++ b/lib/mauve/person.rb
@@ -21,15 +21,21 @@ module Mauve
 
     def suppressed? ; @suppressed ; end
  
-    def should_suppress?
-      now = Time.now
+    #
+    # This 
+    #
+    def should_suppress?(with_notification_at = nil, now = Time.now)
 
       return self.notification_thresholds.any? do |period, previous_alert_times|
         #
-        # Choose the second one as the first, apart from if the array is only one in length.
-        #
-        first = previous_alert_times.first
-        last  = previous_alert_times.last
+        # This is going to work out if we would be suppressed if 
+        if with_notification_at.nil?
+         first = previous_alert_times.first
+         last  = previous_alert_times.last
+        else
+         first = previous_alert_times[1]
+         last  = with_notification_at
+        end
    
         (first.is_a?(Time) and (now - first) < period) or
           (last.is_a?(Time) and @suppressed and (now - last) < period) 
@@ -111,14 +117,15 @@ module Mauve
 
       was_suppressed = @suppressed
       @suppressed    = self.should_suppress?
+      will_suppress  = self.should_suppress?(now)
 
-      logger.info "Starting to send notifications again for #{username}." if was_suppressed and not self.suppressed?
+      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.
       #
-      if (was_suppressed and self.suppressed?) or self.is_on_holiday?
+      if @suppressed or self.is_on_holiday?
         note =  "#{alert.update_type.capitalize} notification to #{self.username} suppressed"
         logger.info note + " about #{alert}."
         History.create(:alerts => [alert], :type => "notification", :event => note)
@@ -132,7 +139,7 @@ module Mauve
         alert,
         [],
         # current_alerts,
-        {:is_suppressed  => @suppressed,
+        {:will_suppress  => will_suppress,
          :was_suppressed => was_suppressed, }
       ).instance_eval(&__send__(level))
 
@@ -144,14 +151,10 @@ module Mauve
           #
           # Hmm.. not sure how to make this thread-safe.
           #
-          self.notification_thresholds[period].push Time.now
+          self.notification_thresholds[period].push now
           self.notification_thresholds[period].shift
         end
 
-        #
-        # Re-run the suppression check, to see if we should be suppressed now.
-        #
-        @suppressed    = self.should_suppress?
 
         return true
       end
diff --git a/lib/mauve/version.rb b/lib/mauve/version.rb
index 6a78983..9900fee 100644
--- a/lib/mauve/version.rb
+++ b/lib/mauve/version.rb
@@ -1,5 +1,5 @@
 module Mauve
 
-  VERSION="3.5.4"
+  VERSION="3.5.5"
 
 end
diff --git a/test/tc_mauve_person.rb b/test/tc_mauve_person.rb
index 30051b0..5303b65 100644
--- a/test/tc_mauve_person.rb
+++ b/test/tc_mauve_person.rb
@@ -59,23 +59,23 @@ EOF
     #
     # 6 alerts every 60 seconds.
     #
-    [ [0, true, false],
-      [5, true, false],
-      [10, true, false],
-      [15, true, false],
-      [20, true, false],
-      [25, true, true], # 6th alert -- suppress from now on
-      [30, false, true], 
-      [35, false, true],
-      [40, false, true],
-      [60, false, true], # One minute after starting -- should still be suppressed
-      [65, false, true],
-      [70, false, true],
-      [75, false, true],
-      [80, false, true],
-      [85, true, false], # One minute after the last alert was sent, start sending again.
-      [90, true, false]
-    ].each do |offset, notification_sent, suppressed|
+    [ [0, true],
+      [5, true],
+      [10, true],
+      [15, true],
+      [20, true],
+      [25, true], # 6th alert -- suppress from now on
+      [30, false], 
+      [35, false],
+      [40, false],
+      [60, false], # One minute after starting -- should still be suppressed
+      [65, false],
+      [70, false],
+      [75, false],
+      [80, false],
+      [85, true], # One minute after the last alert was sent, start sending again.
+      [90, true]
+    ].each do |offset, notification_sent|
       # 
       # Advance in to the future!
       #
@@ -83,8 +83,6 @@ EOF
 
       person.send_alert(alert.level, alert)
 
-      assert_equal(suppressed,    person.suppressed?, "Suppressed (or not) when it should (or shouldn't) be at #{Time.now}.")
-
       if notification_sent 
         assert_equal(1, $sent_notifications.length, "Notification not sent when it should have been at #{Time.now}.")
         #
@@ -145,14 +143,14 @@ EOF
     #
     # 1 alerts every 60 seconds.
     #
-    [ [0, true,   true],
-      [5, false,  true],
-      [15, false, true],
-      [30, false, true],
-      [60, true,  true], # One minute after starting -- should send an alert, but still be suppressed.
-      [90, false, true],
-      [120, true, true] # Two minutes after starting -- should send an alert, but still be suppressed.
-    ].each do |offset, notification_sent, suppressed|
+    [ [0, true ],
+      [5, false],
+      [15, false],
+      [30, false],
+      [60, true ], # One minute after starting -- should send an alert.
+      [90, false],
+      [120, true] # Two minutes after starting -- should send an alert.
+    ].each do |offset, notification_sent|
       # 
       # Advance in to the future!
       #
@@ -160,8 +158,6 @@ EOF
 
       person.send_alert(alert.level, alert)
 
-      assert_equal(suppressed,    person.should_suppress?, "Suppressed (or not) when it should (or shouldn't) be at #{Time.now}.")
-
       if notification_sent 
         assert_equal(1, $sent_notifications.length, "Notification not sent when it should have been at #{Time.now}.")
         #