notification_rule.rb in flapjack-0.9.1

- old
+ new

@@ -66,12 +66,20 @@
 
         self.find_by_id(rule_id, :redis => redis)
       end
 
       def update(update_data, opts = {})
-        rule_data = @redis.hgetall("notification_rule:#{@id}").merge(update_data.merge(:id => @id))
-        errors = self.class.add_or_update(rule_data, :redis => @redis, :logger => opts[:logger])
+        [:entities, :regex_entities, :tags, :regex_tags,
+         :time_restrictions, :unknown_media, :warning_media, :critical_media,
+         :unknown_blackhole, :warning_blackhole, :critical_blackhole].each do |update_key|
+
+          next if update_data.has_key?(update_key)
+          update_data[update_key] = self.send(update_key)
+        end
+
+        update_data.update(:id => @id, :contact_id => @contact_id)
+        errors = self.class.add_or_update(update_data, :redis => @redis, :logger => opts[:logger])
         return errors unless errors.nil? || errors.empty?
 
         refresh
         nil
       end
@@ -83,15 +91,23 @@
       # We may want to consider some sort of timeout-based check around
       # anything that could fall into that.
       #
       # We don't want to replicate IceCube's from_hash behaviour here,
       # but we do need to apply some sanity checking on the passed data.
-      def self.time_restriction_to_icecube_schedule(tr, timezone)
-        return unless !tr.nil? && tr.is_a?(Hash)
-        return if timezone.nil? && !timezone.is_a?(ActiveSupport::TimeZone)
-        return unless tr = prepare_time_restriction(tr, timezone)
-        IceCube::Schedule.from_hash(tr)
+      def self.time_restriction_to_icecube_schedule(tr, timezone, opts = {})
+        return if tr.nil? || !tr.is_a?(Hash) ||
+                  timezone.nil? || !timezone.is_a?(ActiveSupport::TimeZone)
+        prepared_restrictions = prepare_time_restriction(tr, timezone)
+        return if prepared_restrictions.nil?
+        IceCube::Schedule.from_hash(prepared_restrictions)
+      rescue ArgumentError => ae
+        if logger = opts[:logger]
+          logger.error "Couldn't parse rule data #{e.class}: #{e.message}"
+          logger.error prepared_restrictions.inspect
+          logger.error e.backtrace.join("\n")
+        end
+        nil
       end
 
       def to_json(*args)
         self.class.hashify(:id, :contact_id, :tags, :regex_tags, :entities, :regex_entities,
             :time_restrictions, :unknown_media, :warning_media, :critical_media,
@@ -205,10 +221,11 @@
         return errors unless errors.nil? || errors.empty?
 
         # whitelisting fields, rather than passing through submitted data directly
         tag_data       = rule_data[:tags].is_a?(Set) ? rule_data[:tags].to_a : nil
         regex_tag_data = rule_data[:regex_tags].is_a?(Set) ? rule_data[:regex_tags].to_a : nil
+
         json_rule_data = {
           :id                 => rule_data[:id].to_s,
           :contact_id         => rule_data[:contact_id].to_s,
           :entities           => Oj.dump(rule_data[:entities]),
           :regex_entities     => Oj.dump(rule_data[:regex_entities]),
@@ -220,10 +237,11 @@
           :critical_media     => Oj.dump(rule_data[:critical_media]),
           :unknown_blackhole  => rule_data[:unknown_blackhole],
           :warning_blackhole  => rule_data[:warning_blackhole],
           :critical_blackhole => rule_data[:critical_blackhole],
         }
+
         logger.debug("NotificationRule#add_or_update json_rule_data: #{json_rule_data.inspect}") if logger
 
         redis.sadd("contact_notification_rules:#{json_rule_data[:contact_id]}",
                    json_rule_data[:id])
         redis.hmset("notification_rule:#{json_rule_data[:id]}",
@@ -313,13 +331,14 @@
                ( d[:regex_tags].nil? ||
                  d[:regex_tags].is_a?(Flapjack::Data::TagSet) &&
                  d[:regex_tags].all? {|et| et.is_a?(String)} ) } =>
         "regex_tags must be a tag_set of strings",
 
+        # conversion to a schedule needs a time zone, any one will do
         proc {|d| !d.has_key?(:time_restrictions) ||
                ( d[:time_restrictions].nil? ||
                  d[:time_restrictions].all? {|tr|
-                   !!prepare_time_restriction(symbolize(tr))
+                   !!self.time_restriction_to_icecube_schedule(symbolize(tr), ActiveSupport::TimeZone['UTC'])
                  } )
              } =>
         "time restrictions are invalid",
 
         # TODO should the media types be checked against a whitelist?