Add better support for private notes in Slack (#562)

* Add better support for private notes in Slack

* More tests

lib/chat_api/messages/notification.ex

@@ -17,14 +17,16 @@ defmodule ChatApi.Messages.Notification do
   end
 
   @spec broadcast_to_customer!(Message.t()) :: Message.t()
-  def broadcast_to_customer!(%Message{} = message) do
+  def broadcast_to_customer!(%Message{private: false} = message) do
     message
     |> Helpers.get_conversation_topic()
     |> ChatApiWeb.Endpoint.broadcast!("shout", Helpers.format(message))
 
     message
   end
 
+  def broadcast_to_customer!(message), do: message
+
   @spec broadcast_to_admin!(Message.t()) :: Message.t()
   def broadcast_to_admin!(%Message{} = message) do
     message
@@ -80,7 +82,7 @@ defmodule ChatApi.Messages.Notification do
     message
   end
 
-  def notify(%Message{} = message, :conversation_reply_email, _opts) do
+  def notify(%Message{private: false} = message, :conversation_reply_email, _opts) do
     Logger.info("Sending notification: :conversation_reply_email")
     # 20 minutes (TODO: make this configurable?)
     schedule_in = 20 * 60
@@ -97,7 +99,7 @@ defmodule ChatApi.Messages.Notification do
     message
   end
 
-  def notify(%Message{} = message, :slack_company_channel, _opts) do
+  def notify(%Message{private: false} = message, :slack_company_channel, _opts) do
     Logger.info("Sending notification: :slack_company_channel")
 
     Task.start(fn ->
@@ -108,7 +110,7 @@ defmodule ChatApi.Messages.Notification do
   end
 
   # TODO: come up with a better name... it's not super clear what `slack_support_channel` means!
-  def notify(%Message{} = message, :slack_support_channel, _opts) do
+  def notify(%Message{private: false} = message, :slack_support_channel, _opts) do
     Logger.info("Sending notification: :slack_support_channel")
 
     Task.start(fn ->
@@ -118,6 +120,14 @@ defmodule ChatApi.Messages.Notification do
     message
   end
 
+  def notify(%Message{private: true} = message, type, _opts) do
+    Logger.debug(
+      "Skipping notification type #{inspect(type)} for private message #{inspect(message)}"
+    )
+
+    message
+  end
+
   def notify(message, type, _opts) do
     Logger.error(
       "Unrecognized notification type #{inspect(type)} for message #{inspect(message)}"

lib/chat_api/slack/event.ex

@@ -58,7 +58,9 @@ defmodule ChatApi.Slack.Event do
          primary_reply_authorization <-
            SlackAuthorizations.get_authorization_by_account(account_id, %{type: "reply"}) do
       if Slack.Helpers.is_primary_channel?(primary_reply_authorization, slack_channel_id) do
-        %{
+        text
+        |> Slack.Helpers.parse_message_type_params()
+        |> Map.merge(%{
           "body" => Slack.Helpers.sanitize_slack_message(text, primary_reply_authorization),
           "conversation_id" => conversation_id,
           "account_id" => account_id,
@@ -70,7 +72,7 @@ defmodule ChatApi.Slack.Event do
               slack_user_id,
               conversation.assignee_id
             )
-        }
+        })
         |> Messages.create_and_fetch!()
         |> Messages.Notification.broadcast_to_customer!()
         |> Messages.Notification.broadcast_to_admin!()

lib/chat_api/slack/helpers.ex

@@ -586,6 +586,7 @@ defmodule ChatApi.Slack.Helpers do
     |> sanitize_slack_user_ids(access_token)
     |> sanitize_slack_links()
     |> sanitize_slack_mailto_links()
+    |> sanitize_private_note()
   end
 
   @spec get_slack_message_metadata(binary()) :: map() | nil
@@ -683,6 +684,27 @@ defmodule ChatApi.Slack.Helpers do
     end
   end
 
+  @private_note_prefix_v1 ~S(\\ )
+  @private_note_prefix_v2 ~S(;; )
+  @private_note_prefix_regex_v1 ~r/^\\\\ /
+  @private_note_prefix_regex_v2 ~r/^;; /
+
+  @spec sanitize_private_note(binary()) :: binary()
+  def sanitize_private_note(text) do
+    text
+    |> String.replace(@private_note_prefix_regex_v1, "")
+    |> String.replace(@private_note_prefix_regex_v2, "")
+  end
+
+  @spec parse_message_type_params(binary()) :: map()
+  def parse_message_type_params(text) do
+    case text do
+      @private_note_prefix_v1 <> _note -> %{"private" => true, "type" => "note"}
+      @private_note_prefix_v2 <> _note -> %{"private" => true, "type" => "note"}
+      _ -> %{}
+    end
+  end
+
   @spec slack_link_to_markdown(binary()) :: binary()
   def slack_link_to_markdown(text) do
     text
@@ -712,13 +734,8 @@ defmodule ChatApi.Slack.Helpers do
   # Formatters
   #####################
 
-  @spec get_message_text(map()) :: binary()
-  def get_message_text(%{
-        conversation: %Conversation{id: conversation_id, customer: %Customer{} = customer},
-        message: %Message{body: text},
-        authorization: _authorization,
-        thread: nil
-      }) do
+  @spec get_dashboard_conversation_url(binary()) :: binary()
+  def get_dashboard_conversation_url(conversation_id) do
     url = System.get_env("BACKEND_URL") || ""
 
     base =
@@ -728,22 +745,57 @@ defmodule ChatApi.Slack.Helpers do
         "https://" <> url
       end
 
-    url = base <> "/conversations/all?cid=" <> conversation_id
-    dashboard = "<#{url}|dashboard>"
+    "#{base}/conversations/all?cid=#{conversation_id}"
+  end
 
-    # TODO: this isn't always a "customer" -- with proactive messaging, it would be an agent
-    "*:wave: #{identify_customer(customer)} says*: #{text}" <>
-      "\n\nReply to this thread to start chatting, or view in the #{dashboard} :rocket:"
+  @spec get_message_text(map()) :: binary()
+  def get_message_text(%{
+        conversation: %Conversation{customer: %Customer{}} = conversation,
+        message: %Message{body: text} = message,
+        authorization: _authorization,
+        thread: nil
+      }) do
+    dashboard_link = "<#{get_dashboard_conversation_url(conversation.id)}|dashboard>"
+
+    primary_message_text =
+      case message do
+        %Message{user: %User{} = user} ->
+          "*:female-technologist: #{Slack.Notification.format_user_name(user)}*: #{text}"
+
+        %Message{customer: %Customer{} = customer} ->
+          "*:wave: #{identify_customer(customer)}*: #{text}"
+
+        %Message{customer_id: nil, user_id: user_id} when not is_nil(user_id) ->
+          "*:female-technologist: Agent*: #{text}"
+
+        %Message{customer_id: customer_id, user_id: nil} when not is_nil(customer_id) ->
+          "*:wave: #{identify_customer(conversation.customer)}*: #{text}"
+
+        _ ->
+          Logger.error("Unrecognized message format: #{inspect(message)}")
+
+          text
+      end
+
+    primary_message_text <>
+      "\n\n" <>
+      "Reply to this thread to start chatting, or view in the #{dashboard_link} :rocket:"
   end
 
   @slack_chat_write_customize_scope "chat:write.customize"
 
   def get_message_text(%{
         conversation: %Conversation{} = conversation,
-        message: %Message{body: text} = message,
+        message: %Message{body: body} = message,
         authorization: %SlackAuthorization{} = authorization,
         thread: %SlackConversationThread{}
       }) do
+    text =
+      case message do
+        %Message{private: true, type: "note"} -> "\\\\ _#{body}_"
+        _ -> body
+      end
+
     if SlackAuthorizations.has_authorization_scope?(
          authorization,
          @slack_chat_write_customize_scope

lib/chat_api_web/channels/notification_channel.ex

@@ -95,12 +95,14 @@ defmodule ChatApiWeb.NotificationChannel do
 
   @spec broadcast_new_message(Message.t(), any()) :: Message.t()
   defp broadcast_new_message(%Message{private: true} = message, socket) do
-    # For private messages, we only need to broadcast back to the admin channel
-    # (We avoid broadcasting to the customer channel or Slack or email)
-    # TODO: broadcast to webhooks and internal Slack channel?
+    # For private messages, we only need to broadcast back to the admin channel,
+    # the internal Slack channel, and webhooks. (We avoid broadcasting to the
+    # customer channel or any public Slack channel or email.)
     broadcast(socket, "shout", Messages.Helpers.format(message))
 
     message
+    |> Messages.Notification.notify(:slack)
+    |> Messages.Notification.notify(:webhooks)
   end
 
   defp broadcast_new_message(message, socket) do

test/chat_api/slack_test.exs

@@ -869,6 +869,29 @@ defmodule ChatApi.SlackTest do
       end)
     end
 
+    test "Helpers.sanitize_slack_message/2 removes private note indicator prefixes",
+         %{account: account} do
+      authorization = insert(:slack_authorization, account: account)
+
+      assert "reply" = Slack.Helpers.sanitize_slack_message("reply", authorization)
+      assert "note" = Slack.Helpers.sanitize_slack_message("\\\\ note", authorization)
+      assert "note" = Slack.Helpers.sanitize_slack_message(~S(\\ note), authorization)
+      assert "note" = Slack.Helpers.sanitize_slack_message(";; note", authorization)
+    end
+
+    test "Helpers.parse_message_type_params/1 removes private note indicator prefixes" do
+      assert Slack.Helpers.parse_message_type_params("reply") == %{}
+
+      assert %{"private" => true, "type" => "note"} =
+               Slack.Helpers.parse_message_type_params("\\\\ note")
+
+      assert %{"private" => true, "type" => "note"} =
+               Slack.Helpers.parse_message_type_params(~S(\\ note))
+
+      assert %{"private" => true, "type" => "note"} =
+               Slack.Helpers.parse_message_type_params(";; note")
+    end
+
     test "Helpers.find_slack_user_mentions/1 extracts @mentions in a Slack message" do
       assert ["<@UABC123>"] =
                Slack.Helpers.find_slack_user_mentions("Hi <@UABC123>! How can we help you?")

test/chat_api_web/controllers/slack_controller_test.exs

@@ -106,6 +106,56 @@ defmodule ChatApiWeb.SlackControllerTest do
       assert body == event_params["text"]
     end
 
+    test "sending a new thread message event to the webhook from the primary channel as a private note",
+         %{
+           conn: conn,
+           auth: auth,
+           thread: thread
+         } do
+      account_id = thread.account_id
+
+      post(conn, Routes.slack_path(conn, :webhook), %{
+        "event" => %{
+          "type" => "message",
+          "text" => ~S(\\ This should be private),
+          "ts" => "12345",
+          "thread_ts" => thread.slack_thread_ts,
+          "channel" => @slack_channel,
+          "user" => auth.authed_user_id
+        }
+      })
+
+      assert [%{body: body, source: "slack", type: "note", private: true}] =
+               Messages.list_messages(account_id)
+
+      assert body == "This should be private"
+    end
+
+    test "sending a new thread message event to the webhook from the primary channel as a private note (alternative)",
+         %{
+           conn: conn,
+           auth: auth,
+           thread: thread
+         } do
+      account_id = thread.account_id
+
+      post(conn, Routes.slack_path(conn, :webhook), %{
+        "event" => %{
+          "type" => "message",
+          "text" => ";; This should be private",
+          "ts" => "12345",
+          "thread_ts" => thread.slack_thread_ts,
+          "channel" => @slack_channel,
+          "user" => auth.authed_user_id
+        }
+      })
+
+      assert [%{body: body, source: "slack", type: "note", private: true}] =
+               Messages.list_messages(account_id)
+
+      assert body == "This should be private"
+    end
+
     test "updates the conversation with the assignee after the first agent reply", %{
       conn: conn,
       auth: auth,