Updated structure, added more tests

homeassistant/components/automation/state.py

@@ -26,27 +26,28 @@ CONF_FOR = "for"
 
 def get_time_config(config):
     """ Helper function to extract the time specified in the config """
-    if CONF_FOR in config:
-        hours = config[CONF_FOR].get(CONF_HOURS)
-        minutes = config[CONF_FOR].get(CONF_MINUTES)
-        seconds = config[CONF_FOR].get(CONF_SECONDS)
+    if CONF_FOR not in config:
+        return None
 
-        if hours is None and minutes is None and seconds is None:
-            logging.getLogger(__name__).error(
-                "Received invalid value for '%s': %s",
-                config[CONF_FOR], CONF_FOR)
-            return False
+    hours = config[CONF_FOR].get(CONF_HOURS)
+    minutes = config[CONF_FOR].get(CONF_MINUTES)
+    seconds = config[CONF_FOR].get(CONF_SECONDS)
 
-        if config.get(CONF_TO) is None and config.get(CONF_STATE) is None:
-            logging.getLogger(__name__).error(
-                "For: requires a to: value'%s': %s",
-                config[CONF_FOR], CONF_FOR)
-            return False
+    if hours is None and minutes is None and seconds is None:
+        logging.getLogger(__name__).error(
+            "Received invalid value for '%s': %s",
+            config[CONF_FOR], CONF_FOR)
+        return None
 
-        return timedelta(hours=(hours or 0.0),
-                         minutes=(minutes or 0.0),
-                         seconds=(seconds or 0.0))
-    return False
+    if config.get(CONF_TO) is None and config.get(CONF_STATE) is None:
+        logging.getLogger(__name__).error(
+            "For: requires a to: value'%s': %s",
+            config[CONF_FOR], CONF_FOR)
+        return None
+
+    return timedelta(hours=(hours or 0.0),
+                     minutes=(minutes or 0.0),
+                     seconds=(seconds or 0.0))
 
 
 def trigger(hass, config, action):
@@ -56,20 +57,19 @@ def trigger(hass, config, action):
     if entity_id is None:
         logging.getLogger(__name__).error(
             "Missing trigger configuration key %s", CONF_ENTITY_ID)
-        return False
+        return None
 
     from_state = config.get(CONF_FROM, MATCH_ALL)
     to_state = config.get(CONF_TO) or config.get(CONF_STATE) or MATCH_ALL
+    time_delta = get_time_config(config)
 
     if isinstance(from_state, bool) or isinstance(to_state, bool):
         logging.getLogger(__name__).error(
             'Config error. Surround to/from values with quotes.')
-        return False
+        return None
 
-    if CONF_FOR in config:
-        time_delta = get_time_config(config)
-        if time_delta is False:
-            return False
+    if CONF_FOR in config and time_delta is None:
+        return None
 
     def state_automation_listener(entity, from_s, to_s):
         """ Listens for state changes and calls action. """
@@ -89,7 +89,7 @@ def trigger(hass, config, action):
             hass.bus.remove_listener(
                 EVENT_STATE_CHANGED, for_state_listener)
 
-        if CONF_FOR in config:
+        if time_delta is not None:
             target_tm = dt_util.utcnow() + time_delta
             for_time_listener = track_point_in_time(
                 hass, state_for_listener, target_tm)
@@ -110,28 +110,24 @@ def if_action(hass, config):
     entity_id = config.get(CONF_ENTITY_ID)
     state = config.get(CONF_STATE)
 
-    if CONF_FOR in config:
-        time_delta = get_time_config(config)
-        if not time_delta:
-            return False
-
     if entity_id is None or state is None:
         logging.getLogger(__name__).error(
             "Missing if-condition configuration key %s or %s", CONF_ENTITY_ID,
             CONF_STATE)
         return None
 
+    time_delta = get_time_config(config)
+    if CONF_FOR in config and time_delta is None:
+        return None
+
     state = str(state)
 
     def if_state():
         """ Test if condition. """
         is_state = hass.states.is_state(entity_id, state)
-
-        if CONF_FOR not in config:
-            return is_state
-
-        target_tm = dt_util.utcnow() - time_delta
-        return (is_state and
-                target_tm > hass.states.get(entity_id).last_changed)
+        return (time_delta is None and is_state or
+                time_delta is not None and
+                dt_util.utcnow() - time_delta >
+                hass.states.get(entity_id).last_changed)
 
     return if_state

tests/components/automation/test_state.py

@@ -462,3 +462,22 @@ class TestAutomationState(unittest.TestCase):
             self.hass.bus.fire('test_event')
             self.hass.pool.block_till_done()
             self.assertEqual(1, len(self.calls))
+
+    def test_if_fails_setup_for_without_time(self):
+        self.assertIsNone(state.if_action(
+            self.hass, {
+                'platform': 'state',
+                'entity_id': 'test.entity',
+                'state': 'on',
+                'for': {},
+            }))
+
+    def test_if_fails_setup_for_without_entity(self):
+        self.assertIsNone(state.if_action(
+            self.hass, {
+                'platform': 'state',
+                'state': 'on',
+                'for': {
+                    'seconds': 5
+                },
+            }))