Merge pull request #418 from ebmdatalab/tech-support-in-dm

Don't repost to tech-support from DMs

ebmbot/bot.py

@@ -135,9 +135,9 @@ def im_job_listener(event, say, ack):
         """
         # acknowledge this message to avoid slack retrying in 3s
         ack()
-        _listener(event, say)
+        _listener(event, say, is_im=True)
 
-    def _listener(event, say):
+    def _listener(event, say, is_im=False):
         # Remove the reminder prefix
         text = event["text"].replace("Reminder: ", "")
         # Remove the bot mention; this sometimes includes the bot's name as well as
@@ -160,7 +160,7 @@ def _listener(event, say):
 
         for slack_config in config["slack"]:
             if slack_config["regex"].match(text):
-                handle_command(app, event, say, slack_config)
+                handle_command(app, event, say, slack_config, is_im=is_im)
                 return
 
         for namespace, help_config in config["help"].items():
@@ -338,7 +338,7 @@ def _pluralise(n, noun):
         return f"There are {n} {noun}s"
 
 
-def handle_command(app, message, say, slack_config):
+def handle_command(app, message, say, slack_config, is_im):
     """Give a thumbs-up to the message, and dispatch to another handler."""
     app.client.reactions_add(
         channel=message["channel"], timestamp=message["ts"], name="crossed_fingers"
@@ -351,7 +351,7 @@ def handle_command(app, message, say, slack_config):
         "cancel_suppression": handle_cancel_suppression,
     }[slack_config["action"]]
 
-    handler(message, say, slack_config)
+    handler(message, say, slack_config, is_im)
 
 
 def _remove_url_formatting(arg):
@@ -365,17 +365,19 @@ def _remove_url_formatting(arg):
 
 
 @log_call
-def handle_schedule_job(message, say, slack_config):
+def handle_schedule_job(message, say, slack_config, is_im=False):
     """Schedule a job."""
     match = slack_config["regex"].match(message["text"])
     job_args = dict(zip(slack_config["template_params"], match.groups()))
     deformatted_args = {k: _remove_url_formatting(v) for k, v in job_args.items()}
+    logger.info("scheduling", msg=message)
     existing_job_is_running = scheduler.schedule_job(
         slack_config["job_type"],
         deformatted_args,
         channel=message["channel"],
         thread_ts=message["ts"],
         delay_seconds=slack_config["delay_seconds"],
+        is_im=is_im,
     )
     if existing_job_is_running:
         say(
@@ -386,14 +388,14 @@ def handle_schedule_job(message, say, slack_config):
 
 
 @log_call
-def handle_cancel_job(message, say, slack_config):
+def handle_cancel_job(message, say, slack_config, _is_im):
     """Cancel a job."""
 
     scheduler.cancel_job(slack_config["job_type"])
 
 
 @log_call
-def handle_schedule_suppression(message, say, slack_config):
+def handle_schedule_suppression(message, say, slack_config, _is_im):
     """Schedule a suppression."""
 
     match = slack_config["regex"].match(message["text"])
@@ -411,7 +413,7 @@ def handle_schedule_suppression(message, say, slack_config):
 
 
 @log_call
-def handle_cancel_suppression(message, say, slack_config):
+def handle_cancel_suppression(message, say, slack_config, _is_im):
     """Cancel a suppression."""
 
     scheduler.cancel_suppressions(slack_config["job_type"])

ebmbot/connection.py

@@ -11,7 +11,8 @@
     channel TEXT,
     thread_ts TEXT,
     start_after DATETIME,
-    started_at DATETIME
+    started_at DATETIME,
+    is_im BOOLEAN
 );
 
 CREATE TABLE IF NOT EXISTS suppression (

ebmbot/dispatcher.py

@@ -145,9 +145,10 @@ def notify_end(self, rc):
                 f"Command `{self.job['type']}` failed.\n"
                 f"Find logs in {self.host_log_dir} on dokku3.\n"
                 f"Or check logs here with errorlogs head/tail/show, e.g.\n"
-                f"* `@{settings.SLACK_APP_USERNAME} errorlogs tail {self.host_log_dir}`\n"
-                "Calling tech-support."
+                f"* `@{settings.SLACK_APP_USERNAME} errorlogs tail {self.host_log_dir}`"
             )
+            if not self.job["is_im"]:
+                msg += "\nCalling tech-support."
             error = True
 
         slack_message = notify_slack(
@@ -156,8 +157,10 @@ def notify_end(self, rc):
             msg,
             message_format=self.job_config["report_format"] if rc == 0 else "text",
         )
-        if error:
+        if error and not self.job["is_im"]:
             # If the command failed, repost it to tech-support
+            # Don't repost to tech-support if we're in a DM with the bot, because no-one
+            # else will be able to read the reposted message
             # Note that the bot won't register messages from itself, so we can't just
             # rely on the tech-support listener
             message_url = self.slack_client.chat_getPermalink(

ebmbot/scheduler.py

@@ -6,7 +6,7 @@
 
 
 @log_call
-def schedule_job(type_, args, channel, thread_ts, delay_seconds):
+def schedule_job(type_, args, channel, thread_ts, delay_seconds, is_im=False):
     """Schedule job to be run.
 
     Only one job of any type may be scheduled.  If a job is already scheduled
@@ -33,12 +33,12 @@ def schedule_job(type_, args, channel, thread_ts, delay_seconds):
     existing_jobs = list(conn.execute(sql, [type_]))
     existing_job_running = False
     if len(existing_jobs) == 0:
-        _create_job(type_, args, channel, thread_ts, start_after)
+        _create_job(type_, args, channel, thread_ts, start_after, is_im)
     elif len(existing_jobs) == 1:
         job = existing_jobs[0]
         if job["has_started"]:
             existing_job_running = True
-            _create_job(type_, args, channel, thread_ts, start_after)
+            _create_job(type_, args, channel, thread_ts, start_after, is_im)
         else:
             id_ = job["id"]
             _update_job(id_, args, channel, thread_ts, start_after)
@@ -54,11 +54,11 @@ def schedule_job(type_, args, channel, thread_ts, delay_seconds):
     return existing_job_running
 
 
-def _create_job(type_, args, channel, thread_ts, start_after):
+def _create_job(type_, args, channel, thread_ts, start_after, is_im):
     with get_connection() as conn:
         conn.execute(
-            "INSERT INTO job (type, args, channel, thread_ts, start_after) VALUES (?, ?, ?, ?, ?)",
-            [type_, args, channel, thread_ts, start_after],
+            "INSERT INTO job (type, args, channel, thread_ts, start_after, is_im) VALUES (?, ?, ?, ?, ?, ?)",
+            [type_, args, channel, thread_ts, start_after, is_im],
         )
 
 

tests/test_dispatcher.py

@@ -196,6 +196,28 @@ def test_job_failure(mock_client):
         assert f.read() == "cat: no-poem: No such file or directory\n"
 
 
+def test_job_failure_in_dm(mock_client):
+    log_dir = build_log_dir("test_bad_job")
+
+    scheduler.schedule_job("test_bad_job", {}, "IM0001", TS, 0, is_im=True)
+    job = scheduler.reserve_job()
+    do_job(mock_client.client, job)
+    assert_slack_client_sends_messages(
+        mock_client.recorder,
+        # NOTE: NOT reposted to tech support from a DM with the bot
+        messages_kwargs=[
+            {"channel": "logs", "text": "about to start"},
+            {"channel": "IM0001", "text": "failed"},
+        ],
+    )
+
+    with open(os.path.join(log_dir, "stdout")) as f:
+        assert f.read() == ""
+
+    with open(os.path.join(log_dir, "stderr")) as f:
+        assert f.read() == "cat: no-poem: No such file or directory\n"
+
+
 def test_job_failure_when_command_not_found(mock_client):
     log_dir = build_log_dir("test_really_bad_job")