Better errors on time automation trigger

homeassistant/components/automation/time.py

@@ -27,8 +27,7 @@ def trigger(hass, config, action):
     if CONF_AFTER in config:
         after = dt_util.parse_time_str(config[CONF_AFTER])
         if after is None:
-            _LOGGER.error(
-                'Received invalid after value: %s', config[CONF_AFTER])
+            _error_time(config[CONF_AFTER], CONF_AFTER)
             return False
         hours, minutes, seconds = after.hour, after.minute, after.second
     elif (CONF_HOURS in config or CONF_MINUTES in config
@@ -63,27 +62,27 @@ def if_action(hass, config):
             CONF_BEFORE, CONF_AFTER, CONF_WEEKDAY)
         return None
 
+    if before is not None:
+        before = dt_util.parse_time_str(before)
+        if before is None:
+            _error_time(before, CONF_BEFORE)
+            return None
+
+    if after is not None:
+        after = dt_util.parse_time_str(after)
+        if after is None:
+            _error_time(after, CONF_AFTER)
+            return None
+
     def time_if():
         """ Validate time based if-condition """
         now = dt_util.now()
-        if before is not None:
-            time = dt_util.parse_time_str(before)
-            if time is None:
+        if before is not None and now > now.replace(hour=before.hour,
+                                                    minute=before.minute):
                 return False
 
-            before_point = now.replace(hour=time.hour, minute=time.minute)
-
-            if now > before_point:
-                return False
-
-        if after is not None:
-            time = dt_util.parse_time_str(after)
-            if time is None:
-                return False
-
-            after_point = now.replace(hour=time.hour, minute=time.minute)
-
-            if now < after_point:
+        if after is not None and now < now.replace(hour=after.hour,
+                                                   minute=after.minute):
                 return False
 
         if weekday is not None:
@@ -96,3 +95,11 @@ def if_action(hass, config):
         return True
 
     return time_if
+
+
+def _error_time(value, key):
+    """ Helper method to print error. """
+    _LOGGER.error(
+        "Received invalid value for '%s': %s", key, value)
+    if isinstance(value, int):
+        _LOGGER.error('Make sure you wrap time values in quotes')

tests/components/automation/test_time.py

@@ -285,9 +285,9 @@ class TestAutomationTime(unittest.TestCase):
             automation.DOMAIN: {
                 'trigger': {
                     'platform': 'time',
-                    'hours': 0,
-                    'minutes': 0,
-                    'seconds': 0,
+                    'hours': 1,
+                    'minutes': 2,
+                    'seconds': 3,
                 },
                 'action': {
                     'execute_service': 'test.automation'
@@ -296,7 +296,7 @@ class TestAutomationTime(unittest.TestCase):
         }))
 
         fire_time_changed(self.hass, dt_util.utcnow().replace(
-            hour=0, minute=0, second=0))
+            hour=1, minute=2, second=3))
 
         self.hass.pool.block_till_done()
         self.assertEqual(1, len(self.calls))
@@ -320,6 +320,30 @@ class TestAutomationTime(unittest.TestCase):
         self.hass.pool.block_till_done()
         self.assertEqual(1, len(self.calls))
 
+    @patch('homeassistant.components.automation.time._LOGGER.error')
+    def test_if_not_fires_using_wrong_after(self, mock_error):
+        """ YAML translates time values to total seconds. This should break the
+            before rule. """
+        self.assertTrue(automation.setup(self.hass, {
+            automation.DOMAIN: {
+                'trigger': {
+                    'platform': 'time',
+                    'after': 3605,
+                    # Total seconds. Hour = 3600 second
+                },
+                'action': {
+                    'execute_service': 'test.automation'
+                }
+            }
+        }))
+
+        fire_time_changed(self.hass, dt_util.utcnow().replace(
+            hour=1, minute=0, second=5))
+
+        self.hass.pool.block_till_done()
+        self.assertEqual(0, len(self.calls))
+        self.assertEqual(2, mock_error.call_count)
+
     def test_if_action_before(self):
         automation.setup(self.hass, {
             automation.DOMAIN: {