fix: report the actual 1-based failed browser-step number (#4200) (#4258)

changedetectionio/notification_service.py

@@ -512,7 +512,7 @@ Thanks - Your omniscient changedetection.io installation.
             return
         threshold = self.datastore.data['settings']['application'].get('filter_failure_notification_threshold_attempts')
 
-        step = step_n + 1
+        step = step_n
         # @todo - This could be a markdown template on the disk, apprise will convert the markdown to HTML+Plaintext parts in the email, and then 'markup_text_links_to_html_links' is not needed
 
         # {{{{ }}}} because this will be Jinja2 {{ }} tokens

changedetectionio/tests/unit/test_step_failure_notification.py

@@ -65,4 +65,28 @@ def test_send_step_failure_notification_queues_item():
     assert not notification_q.empty(), "Expected a notification to be queued"
     item = notification_q.get_nowait()
     assert 'notification_title' in item
-    assert 'position 2' in item['notification_title']
+    # step_n is already 1-based (base.py increments step_n before running each step, so the
+    # first browser step raises with step_n=1), so the reported position must equal step_n.
+    assert 'position 1' in item['notification_title']
+
+
+def test_send_step_failure_notification_position_matches_step_number():
+    """Regression for #4200: the reported browser-step position must equal the 1-based
+    step_n, not step_n + 1. step_n arrives already 1-based (BrowserStepsStepException is
+    raised with the post-increment counter, and worker.py passes e.step_n through), and the
+    frontend highlights nth-child(browser_steps_last_error_step) / compares === i+1, both
+    1-based. An extra +1 mis-numbers the notification and highlights the wrong step."""
+    from changedetectionio.notification_service import NotificationService
+
+    for step_n in (1, 2, 5):
+        watch_uuid = f'test-uuid-pos-{step_n}'
+        notification_q = queue.Queue()
+        datastore, _ = _make_datastore(watch_uuid, 'post://localhost/test')
+        service = NotificationService(datastore=datastore, notification_q=notification_q)
+
+        service.send_step_failure_notification(watch_uuid=watch_uuid, step_n=step_n)
+
+        item = notification_q.get_nowait()
+        assert f'position {step_n} could not be run' in item['notification_title']
+        assert f'position {step_n + 1}' not in item['notification_title']
+        assert f'position {step_n} for the web page watch' in item['notification_body']

changedetectionio/worker.py

@@ -324,7 +324,7 @@ async def async_update_worker(worker_id, q, notification_q, app, datastore, exec
                     if not datastore.data['watching'].get(uuid):
                         continue
 
-                    error_step = e.step_n + 1
+                    error_step = e.step_n
                     from playwright._impl._errors import TimeoutError, Error
 
                     # Generally enough info for TimeoutError (couldnt locate the element after default seconds)